Security Fix for Command Injection - huntr.dev #10644
Conversation
Command Injection vul fix: Replace execSync with execFileSync
huntr-helper
requested review from
ianschmitz,
iansu,
mrmckeb and
petetnt
as code owners
|
Who signed the CLA? I'm a bit confused. @zpbrent Did you sign it? |
|
To be clear for random readers — this vulnerability does not affect Create React App itself, only people who are using its internal utilities directly. |
|
I think ideally we'd have @zpbrent sign the CLA explicitly since they're the primary author. |
|
On a side note, @gaearon, would it make sense for the bot to check for CLAs signatures against all PR commit users? |
|
Yea I flagged this to our tooling team but it will involve some work and probably won't happen very soon. |
|
@gaearon 👍 |
@gaearon and @JamieSlome thanks for your advice, and I have signed the CLA just now. |
@gaearon would you kindly help me to request a CVE for this vul. (I am also the discloser of this vul.), many thanks! |
|
@zpbrent - great, thanks for the quick follow up! 👍 |
|
Have you verified that this function works on Windows? |
Thanks for the CVE :-) |
Sorry, I have not verified it in the Windows. |
|
@gaearon - thanks for the merge! 👍 If you are interested in more fixes like this in the future, you can let security researchers know that they can win bounties protecting your repository by copying this small code snippet into your README.md:
|
|
This case is a bit awkward because this vulnerability is extremely unlikely in practice (CRA's usage of this function does not supply user input there, and I find it difficult to imagine a situation in which you would pass it in the context of this tool). So while I appreciate a fix, I expect that this will cause downstream maintenance burden because once the CVE goes out, we'll get dozens of people asking to backport this fix to old majors because audit tools scream at them, and people don't understand the nuance of when a vulnerability applies or not. I understand vulnerabilities need to be addressed but the noise/signal ratio has been pretty bad historically and I'm not sure I'd like to incentivise increasing it. |
|
@gaearon - appreciate your comments above... I am curious about your comments about the noise/signal ratio that you have seen historically. I would be eager to get some more of your thoughts/feedback in this area. Perhaps I could shoot over an e-mail with some questions or schedule a 15-minute call? ☎️ |
|
@gaearon - great! 👍 Just had a browse through those PRs/comments. Certainly come across other maintainers experiencing similar issues. I will get some questions together and send them over to you on your FB e-mail. Cheers! 🍰 |
* Update getProcessForPort.js * Update getProcessForPort.js Co-authored-by: Zhou Peng <zpbrent@gmail.com> Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
* Update getProcessForPort.js * Update getProcessForPort.js Co-authored-by: Zhou Peng <zpbrent@gmail.com> Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
* Update getProcessForPort.js * Update getProcessForPort.js Co-authored-by: Zhou Peng <zpbrent@gmail.com> Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
@zpbrent (https://huntr.dev/users/zpbrent) has fixed a potential Command Injection vulnerability in your repository 🔨. For more information, visit our website (https://huntr.dev/) or click the bounty URL below...
Q | A
Version Affected | *
Bug Fix | YES
Original Pull Request | 418sec#2
Vulnerability README | https://github.com/418sec/huntr/blob/master/bounties/npm/react-dev-utils/1/README.md
User Comments:
📊 Metadata *
react-dev-utils includes some utilities used by Create React App.
The function getProcessForPort in react-dev-utils is vulnerable to command injection.
Bounty URL: https://www.huntr.dev/bounties/1-npm-react-dev-utils/
⚙️ Description *
Used child_process.execFileSync() instead of child_process.execSync().
💻 Technical Description *
The use of the child_process function execSync() is highly discouraged if you accept user input and don't sanitize/escape them. This PR replaces it with execFileSync() which mitigates any possible Command Injections as it accepts input as arrays.
🐛 Proof of Concept (PoC) *
Create a .js file with the content below and run it, then the file pzhou@shu can be illegally created.
// poc.js
var getProcessForPort = require('react-dev-utils/getProcessForPort');
getProcessForPort('11;$(touch pzhou@shu)');
🔥 Proof of Fix (PoF) *
use "return execFileSync('lsof', ['-i:'+port, '-P', '-t', '-sTCP:LISTEN'], execOptions)" to replace "return execSync('lsof -i:' + port + ' -P -t -sTCP:LISTEN', execOptions)"
👍 User Acceptance Testing (UAT)
var getProcessForPort = require('react-dev-utils/getProcessForPort');
getProcessForPort(3000) // works correctly
🔗 Relates to...
418sec/huntr#1962