Skip to content

Type confusion bugs in process_wrap.cc. #12177

New issue

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

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

Already on GitHub? # to your account

Closed
deian opened this issue Apr 3, 2017 · 3 comments
Closed

Type confusion bugs in process_wrap.cc. #12177

deian opened this issue Apr 3, 2017 · 3 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem.

Comments

@deian
Copy link
Member

deian commented Apr 3, 2017

We found two type confusion bugs in process_wrap.cc.

First one uses ToObject unchecked: https://github.com/nodejs/node/blob/master/src/process_wrap.cc#L136
Second one uses As unchecked: https://github.com/nodejs/node/blob/master/src/process_wrap.cc#L92

The two programs below that trigger these bugs. We’re using process.binding here, but we’ve been pretty successful at escalating such things to public API.

— trigger 1:

P=process.binding('process_wrap').Process; new P().spawn();

— trigger 2:

const options = {file:'ls'};
Object.defineProperty(options, 'stdio', {
 get: () => {
   return [1];
 },
 enumerable: true
});
P=process.binding('process_wrap').Process; new P().spawn(options);
@mscdex mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. labels Apr 3, 2017
cjihrig added a commit to cjihrig/node that referenced this issue Apr 17, 2017
This commit improves input validation for the ChildProcess
internals. It became officially supported API a while back, but
never had any validation.

Refs: nodejs#12177
PR-URL: nodejs#12348
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@Trott
Copy link
Member

Trott commented May 27, 2017

@nodejs/child_process isn't a thing but @bnoordhuis @cjihrig @indutny

@Trott
Copy link
Member

Trott commented Mar 4, 2018

@nodejs/child_process

@apapirovski
Copy link
Contributor

As far as I can tell this got sufficiently resolved in #12348. Using process.binding isn't officially supported so that's up to the user to make sure they don't run into these types of issues. Either way though, this has been open for a year now with no significant movement.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants