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.

Sign up for GitHub

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

Jump to bottom

doc: Provide a warning around child_process.exec() #10466

Closed
wants to merge 1 commit into from
Closed

doc: Provide a warning around child_process.exec() #10466

wants to merge 1 commit into from
+16 −0

Conversation

mjg59
Copy link

@mjg59 mjg59 commented Dec 26, 2016 •
edited by sam-github
Loading

child_process.exec() allows trivial arbitrary command execution if code
passes unsanitised user input to it. Add a warning in the docs to make that
clear.

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. lts-watch-v6.x labels Dec 26, 2016
@MylesBorins
Copy link
Contributor

/cc @nodejs/security

@MylesBorins
Copy link
Contributor

Should this perhaps be a general message for all child process commands?

@mjg59
Copy link
Author

mjg59 commented Dec 27, 2016

@thealphanerd It's much less true for the others in the family that don't launch shells.

jasnell
jasnell approved these changes Dec 27, 2016
View reviewed changes
doc/api/child_process.md
@@ -149,6 +149,9 @@ added: v0.1.90
Spawns a shell then executes the `command` within that shell, buffering any
generated output.

**Note: Never pass unsanitised user input to this function, as otherwise
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest rephrasing slightly...

*Note*: Passing unsanitized user input containing shell metacharacters can be
used to trigger arbitrary command execution.

LGTM either way tho.

@Trott
Copy link
Member

Trott commented Dec 27, 2016

@nodejs/documentation

cjihrig
cjihrig requested changes Dec 28, 2016
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the only function that spawns a shell. If we include this, it should probably be in top level text.

@mjg59
Copy link
Author

mjg59 commented Dec 28, 2016

People will frequently look at function documentation without reading top level text. Adding it to all relevant calls seems reasonable?

@mjg59
Copy link
Author

mjg59 commented Dec 28, 2016

Ok I've added the same warning to execSync(). I don't think it's as relevant for the spawn() family.

@cjihrig
Copy link
Contributor

cjihrig commented Dec 28, 2016

I don't think it's as relevant for the spawn() family.

What about with the shell: true option?

@mjg59
Copy link
Author

mjg59 commented Dec 29, 2016

If the arguments are passed as an array rather than being passed as a single argument to the shell, they won't be interpreted by the shell.

@mjg59
Copy link
Author

mjg59 commented Dec 29, 2016

Hm actually this may not be true - sorry, I'm making assumptions about how Unix handles exec(), which is exactly the problem in the first place! Let me look a little more at exactly what child_process does here.

sam-github
sam-github suggested changes Dec 29, 2016
View reviewed changes
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spawn doesn't use the shell by default, exec does use shell by default, execFile never uses the shell, sync vs. async is not relevant here

@mjg59
Copy link
Author

mjg59 commented Dec 29, 2016

Added warnings to spawn() and spawnSync() as well.

sam-github
sam-github reviewed Dec 29, 2016
View reviewed changes
doc/api/child_process.md
@@ -149,6 +149,10 @@ added: v0.1.90
Spawns a shell then executes the `command` within that shell, buffering any
generated output.

**Note: Never pass unsanitised user input to this function. Any input
containing shell metacharacters may be used to trigger arbitrary command
execution**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing . at end of sentence

sam-github
sam-github suggested changes Dec 29, 2016
View reviewed changes
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the PR description for you, please follow directions in the template next time.

The commit message is now inaccurate since the scope expanded.

I suggest

test: warn about shell metas in child_process

or something of the like.

@mjg59
Copy link
Author

mjg59 commented Dec 29, 2016

Thanks - fixed up the commit message and added the missing full stops.

@sam-github
Copy link
Contributor

Landed in f60aba2, thanks.

@sam-github sam-github closed this Dec 30, 2016
sam-github pushed a commit that referenced this pull request Dec 30, 2016
@sam-github
doc: warn about unvalidated input in child_process
f60aba2 
child_process.exec*() and child_process.spawn*() (if options.shell is
true) allow trivial arbitrary command execution if code passes
unsanitised user input to it. Add warnings in the docs to make that
clear.

PR-URL: #10466
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@ChALkeR ChALkeR added the security Issues and PRs related to security. label Dec 31, 2016
@ChALkeR
Copy link
Member

ChALkeR commented Dec 31, 2016

Thanks for doing this! 👍

@evanlucas evanlucas mentioned this pull request Jan 3, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
@evanlucas
doc: warn about unvalidated input in child_process
a1464a4 
child_process.exec*() and child_process.spawn*() (if options.shell is
true) allow trivial arbitrary command execution if code passes
unsanitised user input to it. Add warnings in the docs to make that
clear.

PR-URL: #10466
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
@evanlucas
doc: warn about unvalidated input in child_process
deb0917 
child_process.exec*() and child_process.spawn*() (if options.shell is
true) allow trivial arbitrary command execution if code passes
unsanitised user input to it. Add warnings in the docs to make that
clear.

PR-URL: #10466
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@MylesBorins
doc: warn about unvalidated input in child_process
f720bc8 
child_process.exec*() and child_process.spawn*() (if options.shell is
true) allow trivial arbitrary command execution if code passes
unsanitised user input to it. Add warnings in the docs to make that
clear.

PR-URL: #10466
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@MylesBorins
doc: warn about unvalidated input in child_process
692dce7 
child_process.exec*() and child_process.spawn*() (if options.shell is
true) allow trivial arbitrary command execution if code passes
unsanitised user input to it. Add warnings in the docs to make that
clear.

PR-URL: #10466
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@MylesBorins
doc: warn about unvalidated input in child_process
678c563 
child_process.exec*() and child_process.spawn*() (if options.shell is
true) allow trivial arbitrary command execution if code passes
unsanitised user input to it. Add warnings in the docs to make that
clear.

PR-URL: #10466
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@MylesBorins
doc: warn about unvalidated input in child_process
394e85e 
child_process.exec*() and child_process.spawn*() (if options.shell is
true) allow trivial arbitrary command execution if code passes
unsanitised user input to it. Add warnings in the docs to make that
clear.

PR-URL: #10466
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@MylesBorins
doc: warn about unvalidated input in child_process
6317567 
child_process.exec*() and child_process.spawn*() (if options.shell is
true) allow trivial arbitrary command execution if code passes
unsanitised user input to it. Add warnings in the docs to make that
clear.

PR-URL: #10466
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This was referenced Jan 24, 2017
Merged
Merged
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
@MylesBorins
doc: warn about unvalidated input in child_process
c3882f4 
child_process.exec*() and child_process.spawn*() (if options.shell is
true) allow trivial arbitrary command execution if code passes
unsanitised user input to it. Add warnings in the docs to make that
clear.

PR-URL: #10466
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
@MylesBorins
doc: warn about unvalidated input in child_process
c7378c4 
child_process.exec*() and child_process.spawn*() (if options.shell is
true) allow trivial arbitrary command execution if code passes
unsanitised user input to it. Add warnings in the docs to make that
clear.

PR-URL: #10466
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sam-github sam-github sam-github approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gibfahn gibfahn gibfahn approved these changes

Assignees
No one assigned
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. security Issues and PRs related to security.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@mjg59 @MylesBorins @Trott @cjihrig @sam-github @ChALkeR @jasnell @gibfahn @nodejs-github-bot