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

Out of bounds memory write via child_process.spawn #15622

Closed
tarqd opened this issue Sep 26, 2017 · 2 comments
Closed

Out of bounds memory write via child_process.spawn #15622

tarqd opened this issue Sep 26, 2017 · 2 comments
Labels
child_process Issues and PRs related to the child_process subsystem.

Comments

@tarqd
Copy link

tarqd commented Sep 26, 2017

  • Version: v8.2.1
  • Platform: OSX
  • Subsystem: child_process

In process wrap, the length of the array is not checked for potential overflows before it is passed to new.

With a very long array, argc + 1 would overflow to zero, causing a null value to be written &options.args[0xffffffffffffffff], despite the fact that we called new [0]

Test JS function:

function test() {x = []; x.length = 4294967294;x[1]=x[4294967296]='test';require('child_process').spawn('echo', x);}

while(true) test(); // eventually you'll crash with bad memory access or a malloc free error

Using this test code, eax (array length) ends up being set to 0xFFFFFF and rolls over to 0 before calling new:

frame #0: 0x0000000100ac0557 node`node::(anonymous namespace)::ProcessWrap::Spawn(v8::FunctionCallbackInfo<v8::Value> const&) + 755
node`node::(anonymous namespace)::ProcessWrap::Spawn:
->  0x100ac0557 <+755>: callq  0x100c80d58               ; symbol stub for: operator new[](unsigned long)
    0x100ac055c <+760>: movq   %rax, -0xcb0(%rbp)
    0x100ac0563 <+767>: testl  %r15d, %r15d
    0x100ac0566 <+770>: jle    0x100ac05f6               ; <+914>
(lldb) reg read rdi
     rdi = 0x0000000000000000   < --- argc (0xFFFFFF) + 1 = 0 
..snip ...
(lldb) reg read rax
     rax = 0x0000000102507920 <-- pointer to args

...snip...after loop which seems to get skipped for some reason....
node`node::(anonymous namespace)::ProcessWrap::Spawn:
->  0x100ac05f9 <+917>: movq   $0x0, (%rax,%r15,8) <-- we write to args[0xffffffffffffffff] which is way out of bounds of what we allocated with new
    0x100ac0601 <+925>: movq   0x8(%r12), %rax
    0x100ac0606 <+930>: movq   0xc8(%rax), %rsi
    0x100ac060d <+937>: movq   %r13, %rdi
(lldb) reg read rax r15
     rax = 0x0000000102507920
     r15 = 0xffffffffffffffff

@tarqd
Copy link
Author

tarqd commented Sep 26, 2017

On older versions of Node, this seems to reliably cause the following error as well:

node(80054,0x7fff9bd0c3c0) malloc: *** error for object 0x102034cd8: pointer being freed was not allocated

On newer versions, it seems like it only crashes like this after the optimizer runs (from what I can tell, always seems to crash after the third time you call the function)

@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Sep 26, 2017
@tarqd
Copy link
Author

tarqd commented Sep 28, 2017

screen shot 2017-09-27 at 9 55 00 pm

Added screenshot of problem from xdebug (argc = -1)

cjihrig added a commit to cjihrig/node that referenced this issue Nov 7, 2017
This commit adds checks for overflow to args and env in Spawn().
It seems extremely unlikely that either of these values would
overflow from a valid use case.

Fixes: nodejs#15622
PR-URL: nodejs#16761
Reviewed-By: Gireesh Punathil <[email protected]>
evanlucas pushed a commit that referenced this issue Nov 13, 2017
This commit adds checks for overflow to args and env in Spawn().
It seems extremely unlikely that either of these values would
overflow from a valid use case.

Fixes: #15622
PR-URL: #16761
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 17, 2017
This commit adds checks for overflow to args and env in Spawn().
It seems extremely unlikely that either of these values would
overflow from a valid use case.

Fixes: #15622
PR-URL: #16761
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 17, 2017
This commit adds checks for overflow to args and env in Spawn().
It seems extremely unlikely that either of these values would
overflow from a valid use case.

Fixes: #15622
PR-URL: #16761
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 21, 2017
This commit adds checks for overflow to args and env in Spawn().
It seems extremely unlikely that either of these values would
overflow from a valid use case.

Fixes: #15622
PR-URL: #16761
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 28, 2017
This commit adds checks for overflow to args and env in Spawn().
It seems extremely unlikely that either of these values would
overflow from a valid use case.

Fixes: #15622
PR-URL: #16761
Reviewed-By: Gireesh Punathil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants