process.setuid results in an abort

[zyscoder]

• Version: v12.16.0
• Platform: Linux vul337 4.15.0-91-generic new design of error handling #92-Ubuntu SMP Fri Feb 28 11:09:48 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: process

What steps will reproduce the bug?

Directly run the following code snippet using node:

require('process').setuid(-0)

How often does it reproduce? Is there a required condition?

No. This potential bug can always be reproduced.

What is the expected behavior?

The argument to 'process.setuid' should be a Uint32 or string value, but we passed a -0 into it. The function should throw an exception or other similar error-reporting stuff rather than crash the whole nodejs process.

What do you see instead?

This is the stack dump produced during abort:

./node[37487]: ../src/node_credentials.cc:247:void node::credentials::SetUid(const FunctionCallbackInfo<v8::Value> &): Assertion `args[0]->IsUint32() || args[0]->IsString()' failed.
 1: 0x13f9b30 node::Abort() [./node]
 2: 0x13f9709  [./node]
 3: 0x13ea56b  [./node]
 4: 0x17b379c v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [./node]
 5: 0x17b23d5  [./node]
 6: 0x17b1092  [./node]
 7: 0x2717a59  [./node]
[2]    37487 abort      ./node

Additional information

[gireeshpunathil]

I can recreate this on master, v10.x prints a proper error message, so looks like a regression to me

[gireeshpunathil]

definition of unsigned int seem to have diverged between JS and C++? looks like the check passed in the former, but failed later?

node/lib/internal/validators.js

Line 101 in 203776f

const validateUint32 = hideStackFrames((value, name, positive) => {

node/src/node_credentials.cc

Line 247 in 203776f

CHECK(args[0]->IsUint32() || args[0]->IsString());

[gireeshpunathil]

failed later -> failed * in * later

[gireeshpunathil]

node --expose-internals -e 'require("internal/validators").validateInt32(-0)'

does not throw, implies this method (validateInt32) does not consider IsMinusZero which IsUint32 of v8 does.

[gireeshpunathil]

validateInt32 hasn't changed between (passing and failing) versions:

node/lib/internal/validators.js

Line 83 in b023d61

const validateInt32 = hideStackFrames(

does this imply IsUint32 has changed its meaning in V8? that doesn't sound logical!

[zyscoder]

My team is trying to find bugs and vulnerabilities at the interface between js and c++, and we think this problem of inconsistency is an interesting study case for us. Can you give us a confirmation for this bug? As proof for us to make a discussion in our work.

[gireeshpunathil]

@zyscoder - that is an interesting area to research, good luck! I tagged it as bug

/cc @nodejs/v8 in case if there is an obvious explanation.

[gireeshpunathil]

bisecting now.

[gireeshpunathil]

bisect is pointing to the commit(s) in #19973 /cc @targos

[targos]

Interesting! That problem is not only present in process.setuid. The following also fails on Node.js 10:

> node -e "require('fs').chownSync('test', -0, -0)"    
src\node_file.cc:2029: Assertion `args[1]->IsUint32()' failed.

V8 always returns false (and it's not new) for IsInt32 and IsUint32 methods.

I see two things that we can do:

• Throw in our int32/uint32 validators if -0 is passed (potentially breaking change)
• Make sure we coerce -0 to 0 either in the validator itself or before passing the value to C++

Thoughts?

[gireeshpunathil]

technically -0 is signed, so throwing from uint32 validator makes sense to me!

[bnoordhuis]

The counterargument is that 0 and -0 are indistinguishable (i.e., 0 === -0) except when checking with Object.is(). Coercion to positive 0 seems like the principle of least surprise to me.