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

child_process.spawnSync is memory unsafe and can be used to dump core #8539

Closed
deian opened this issue Sep 14, 2016 · 3 comments · Fixed by #8312
Closed

child_process.spawnSync is memory unsafe and can be used to dump core #8539

deian opened this issue Sep 14, 2016 · 3 comments · Fixed by #8312
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem.

Comments

@deian
Copy link
Member

deian commented Sep 14, 2016

  • Version: 6.5.0
  • Platform:
  • Subsystem: child_process

child_process.spawnSync is memory unsafe and segfaults when given an array/object for the file argument with a throwing toString definition.

This doesn't seem like a serious security vulnerability (hence my reporting here), but can certainly be used to cause DOS and it might be nice to have a stdlib that is memory safe.

const file = {};
file.toString = () => { throw 'w00t'; };
const child_process = require('child_process');
child_process.spawnSync(file);
// causes ToString in src/spawn_sync.cc:933 to return empty handle which is then
// used on line 933 and thus leads to SEGFAULT

Related to: #8537, #8538, #7902

@deian deian changed the title child_process.spawnSync is memory unsafe and segfaults when given an array/object for the file argument with a throwing toString definition. child_process.spawnSync is memory unsafe and can be used to dump core Sep 14, 2016
@deian deian changed the title child_process.spawnSync is memory unsafe and can be used to dump core child_process.spawnSync is memory unsafe and can be used to dump core Sep 14, 2016
@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Sep 14, 2016
imyller added a commit to imyller/node that referenced this issue Sep 15, 2016
child_process.spawnSync segfaults when given an array/object
for the file argument with a throwing toString definition.

Refs: nodejs#8539

Signed-off-by: Ilkka Myller <[email protected]>
@imyller imyller added the confirmed-bug Issues with confirmed bugs. label Sep 15, 2016
@imyller
Copy link
Member

imyller commented Sep 15, 2016

@deian Thank you for reporting this.

Ideas for fixing this /cc @bnoordhuis, @cjihrig

@imyller
Copy link
Member

imyller commented Sep 15, 2016

This is a known issue and affects most C++ API functions accepting non-primitive values.

For more information:

#7902 (comment)

@imyller imyller added c++ Issues and PRs that require attention from people who are familiar with C++. and removed confirmed-bug Issues with confirmed bugs. labels Sep 15, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Sep 15, 2016

Ideas for fixing this

I'm working on some validation in #8312. I can add a check that the file is a string there.

cjihrig added a commit to cjihrig/node that referenced this issue Dec 25, 2016
This commit verifies that the child process handle is of the
correct type before trying to close it in
CloseHandlesAndDeleteLoop(). This catches the case where input
validation failed, and the child process was never actually
spawned.

Fixes: nodejs#8096
Fixes: nodejs#8539
Refs: nodejs#9722
PR-URL: nodejs#8312
Reviewed-By: Ben Noordhuis <[email protected]>
cjihrig added a commit to cjihrig/node that referenced this issue Dec 25, 2016
This commit applies stricter input validation in
normalizeSpawnArguments(), which is run by all of the
child_process methods. Additional checks are added for spawnSync()
specific inputs.

Fixes: nodejs#8096
Fixes: nodejs#8539
Refs: nodejs#9722
PR-URL: nodejs#8312
Reviewed-By: Ben Noordhuis <[email protected]>
cjihrig added a commit to cjihrig/node that referenced this issue Dec 25, 2016
This commit removes C++ checks from spawn() and spawnSync()
that are duplicates of the JavaScript type checking.

Fixes: nodejs#8096
Fixes: nodejs#8539
Refs: nodejs#9722
PR-URL: nodejs#8312
Reviewed-By: Ben Noordhuis <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 18, 2017
This commit verifies that the child process handle is of the
correct type before trying to close it in
CloseHandlesAndDeleteLoop(). This catches the case where input
validation failed, and the child process was never actually
spawned.

Fixes: nodejs#8096
Fixes: nodejs#8539
Refs: nodejs#9722
PR-URL: nodejs#8312
Reviewed-By: Ben Noordhuis <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 18, 2017
This commit applies stricter input validation in
normalizeSpawnArguments(), which is run by all of the
child_process methods. Additional checks are added for spawnSync()
specific inputs.

Fixes: nodejs#8096
Fixes: nodejs#8539
Refs: nodejs#9722
PR-URL: nodejs#8312
Reviewed-By: Ben Noordhuis <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 18, 2017
This commit removes C++ checks from spawn() and spawnSync()
that are duplicates of the JavaScript type checking.

Fixes: nodejs#8096
Fixes: nodejs#8539
Refs: nodejs#9722
PR-URL: nodejs#8312
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants