Skip to content
This repository has been archived by the owner on Dec 19, 2023. It is now read-only.

Command Injection vul fix: Replace execSync with execFileSync #1

Closed
wants to merge 1 commit into from

Conversation

zpbrent
Copy link

@zpbrent zpbrent commented Mar 3, 2021

📊 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

@JamieSlome
Copy link

❌ Oops, @zpbrent - it looks like you haven't submitted this PR with a valid bounty URL. If this was intentional, please ignore. Otherwise, please close this PR and open a new PR with the following markdown template, and a valid bounty URL included. A valid template and bounty URL ensures that we can reward you for your fix.

If you have any questions, don't hesitate to join our Discord and a member of the team will be happy to help! 👋

@zpbrent
Copy link
Author

zpbrent commented Mar 3, 2021

❌ Oops, @zpbrent - it looks like you haven't submitted this PR with a valid bounty URL. If this was intentional, please ignore. Otherwise, please close this PR and open a new PR with the following markdown template, and a valid bounty URL included. A valid template and bounty URL ensures that we can reward you for your fix.

If you have any questions, don't hesitate to join our Discord and a member of the team will be happy to help! 👋

@JamieSlome Thanks for your advice, and I have modify the PR to include a valid bounty URL as the template presents. Please let me know is that OK, or I must close this PR and open a new one? Thanks!

@zpbrent zpbrent closed this Mar 3, 2021
@zpbrent zpbrent reopened this Mar 3, 2021
@JamieSlome
Copy link

Thanks for your response. The way you have re-defined the template is perfect!

Are you able to close this PR and open an entirely fresh PR?

Cheers! 🍰

@zpbrent zpbrent closed this Mar 3, 2021
@zpbrent
Copy link
Author

zpbrent commented Mar 3, 2021

As required, I have closed this PR and open a new one instead, Please refer to the new PR for the fix!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants