search

danday74 / git-diff

NPM - Returns the git diff of two strings
Other
32 stars 10 forks source link

Shell injection vulnerability #6

Open mk-pmb opened 5 years ago

mk-pmb commented 5 years ago

JSON.stringify does not protect against shell command injection. Please use proper child_process APIs and send the data via an stdin pipe.

Thanks to @sehrope for pointing out this bug on IRC.

joepie91 commented 5 years ago

To add to this: using execFile or spawn instead of exec will prevent most command injection vulnerabities; instead of passing in a concatenated string, you pass in a list of arguments.

danday74 commented 5 years ago

Anyone wanna raise a PR for this? I am busy ATM - If anyone has time, unit tests must continue to pass with 100% coverage (unless a valid reason is given why coverage is not 100% and the relevant block should be commented out with istanbul ignore - happy to add contributors as npm contributors also so they can publish)

AverageHelper commented 2 years ago

+1 on this.

I used git-diff in a project of mine for pretty output in a release script. Worked reliably.

When I copied that script to another project, something about my changelog file and this exec call erased the contents of my project's node_modules directory, everything but its .bin subfolder. I'm not sure what else might've borked, and even this issue wouldn't reliably occur (I'm no computer forensics researcher lol), I only know that that injection can be nasty!

mk-pmb commented 2 years ago

erased the contents of my project's node_modules directory

It's probably because the change log contains concatenated shell commands like && npm run build:clean &&. I wouldn't be surprised if git-diff runs any command quoted in backticks.

AverageHelper commented 2 years ago

Oh hey, nice catch. That's probably what happened!

EDIT: I see npm ci and npm install in there as well. All of those together take a while, which explains why the spicy "diff" took so long. I remember terminating the script early a few times before I discovered node_modules was messed, so I guess I must've caught the script in the middle of reinstalling everything.