Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security Fix for Command Injection - huntr.dev #10644

Merged
merged 3 commits into from
Mar 8, 2021

Conversation

huntr-helper
Copy link

@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

zpbrent and others added 2 commits March 3, 2021 18:58
Command Injection vul fix: Replace execSync with execFileSync
@gaearon
Copy link
Contributor

gaearon commented Mar 3, 2021

Who signed the CLA? I'm a bit confused. @zpbrent Did you sign it?

@gaearon
Copy link
Contributor

gaearon commented Mar 3, 2021

To be clear for random readers — this vulnerability does not affect Create React App itself, only people who are using its internal utilities directly.

@JamieSlome
Copy link

@gaearon - we have the CLA signed from our side for the huntr-helper account.

Perhaps the CLA is already signed by @zpbrent as well?

Cheers! 🍰

@gaearon
Copy link
Contributor

gaearon commented Mar 3, 2021

I think ideally we'd have @zpbrent sign the CLA explicitly since they're the primary author.

@JamieSlome
Copy link

@gaearon - sure, makes sense!

@zpbrent, are you able to sign the CLA:

https://code.facebook.com/cla

@JamieSlome
Copy link

On a side note, @gaearon, would it make sense for the bot to check for CLAs signatures against all PR commit users?

@gaearon
Copy link
Contributor

gaearon commented Mar 3, 2021

Yea I flagged this to our tooling team but it will involve some work and probably won't happen very soon.

@JamieSlome
Copy link

@gaearon 👍

@zpbrent
Copy link
Contributor

zpbrent commented Mar 3, 2021

@gaearon - sure, makes sense!

@zpbrent, are you able to sign the CLA:

https://code.facebook.com/cla

@gaearon and @JamieSlome thanks for your advice, and I have signed the CLA just now.

@zpbrent
Copy link
Contributor

zpbrent commented Mar 3, 2021

Yea I flagged this to our tooling team but it will involve some work and probably won't happen very soon.

@gaearon would you kindly help me to request a CVE for this vul. (I am also the discloser of this vul.), many thanks!

@JamieSlome
Copy link

@zpbrent - great, thanks for the quick follow up! 👍

@gaearon
Copy link
Contributor

gaearon commented Mar 4, 2021

CVE-2021-24033

@gaearon
Copy link
Contributor

gaearon commented Mar 4, 2021

Have you verified that this function works on Windows?

@zpbrent
Copy link
Contributor

zpbrent commented Mar 5, 2021

CVE-2021-24033

Thanks for the CVE :-)

@zpbrent
Copy link
Contributor

zpbrent commented Mar 5, 2021

Have you verified that this function works on Windows?

Sorry, I have not verified it in the Windows.

@gaearon gaearon merged commit f5e415f into facebook:master Mar 8, 2021
@JamieSlome
Copy link

@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:

[![huntr](https://cdn.huntr.dev/huntr_security_badge_mono.svg)](https://huntr.dev)

huntr

@gaearon
Copy link
Contributor

gaearon commented Mar 8, 2021

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.

@JamieSlome
Copy link

@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? ☎️

@JamieSlome
Copy link

@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! 🍰

akxcv pushed a commit to akxcv/create-react-app that referenced this pull request Apr 28, 2021
* Update getProcessForPort.js

* Update getProcessForPort.js

Co-authored-by: Zhou Peng <zpbrent@gmail.com>
Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
wombleton pushed a commit to AurorNZ/create-react-app that referenced this pull request Jun 1, 2021
* Update getProcessForPort.js

* Update getProcessForPort.js

Co-authored-by: Zhou Peng <zpbrent@gmail.com>
Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
abhiisheek pushed a commit to abhiisheek/create-react-app that referenced this pull request May 24, 2023
* Update getProcessForPort.js

* Update getProcessForPort.js

Co-authored-by: Zhou Peng <zpbrent@gmail.com>
Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants