Skip to content

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

Closed
mjg59 wants to merge 1 commit intonodejs:masterfrom
mjg59:master
Closed

doc: Provide a warning around child_process.exec()#10466
mjg59 wants to merge 1 commit intonodejs:masterfrom
mjg59:master

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

MylesBorins commented Dec 27, 2016

/cc @nodejs/security

@MylesBorins
Copy link
Contributor

MylesBorins commented Dec 27, 2016

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 Outdated
Copy link
Member

@jasnell jasnell Dec 27, 2016

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 Outdated
Copy link
Contributor

@sam-github sam-github Dec 29, 2016

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

sam-github commented Dec 30, 2016

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

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

+2 more reviewers

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

@gibfahn gibfahn gibfahn approved these changes

Reviewers whose approvals may not affect merge requirements

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.

9 participants

@mjg59 @MylesBorins @Trott @cjihrig @sam-github @ChALkeR @jasnell @gibfahn @nodejs-github-bot