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

crypto: reject non-int32 values in DiffieHellman() #32739

Closed
wants to merge 3 commits into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Apr 9, 2020

The JS code accepted any value where typeof sizeOrKey === 'number'
was true but the C++ code checked that args[0]->IsInt32() and
subsequently aborted.

Fixes: #32738 #32748

@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Apr 9, 2020
@nodejs-github-bot

This comment has been minimized.

Copy link
Member

@himself65 himself65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we inline this logic to

if (typeof sizeOrKey !== 'number' &&
typeof sizeOrKey !== 'string' &&
!isArrayBufferView(sizeOrKey)) {
throw new ERR_INVALID_ARG_TYPE(
'sizeOrKey',
['number', 'string', 'Buffer', 'TypedArray', 'DataView'],
sizeOrKey
);
}

  if (typeof sizeOrKey !== 'number') {
    if (typeof sizeOrKey !== 'string' &&
        !isArrayBufferView(sizeOrKey))
      throw new ERR_INVALID_ARG_TYPE(
        'sizeOrKey',
        ['number', 'string', 'Buffer', 'TypedArray', 'DataView'],
        sizeOrKey
      );
    else
      validateInt32(sizeOrKey, 'sizeOrKey');
  }

@bnoordhuis
Copy link
Member Author

Can? Sure, but it's another level of indentation. I like my code flat.

// in node_crypto.cc accepts values that are IsInt32() for that reason
// and that's why we do that here too.
if (typeof sizeOrKey === 'number')
validateInt32(sizeOrKey, 'sizeOrKey');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add 0 as a third parameter here if you want to enforce the minimum in JS.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware but that changes the error message on inputs < 0. I added a regression test instead.

Copy link
Member

@himself65 himself65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, code flat better to understand I think

@himself65
Copy link
Member

Could you fixed this by the way? I think they are same reason

#32748

@targos
Copy link
Member

targos commented Apr 10, 2020

In the light of #32750, what happens with this API if -0 is passed?

@himself65
Copy link
Member

In the light of #32750, what happens with this API if -0 is passed?

crash in this PR

C:\Users\Himself65\Desktop\github\node\Release\node.exe[25452]: C:\Users\Himself65\Desktop\github\node\src\util-inl.h:495: Assertion `value->IsArrayBufferView()' failed.
 1: 00007FF65B70879F node::DumpBacktrace+143 [C:\Users\Himself65\Desktop\github\node\src\debug_utils.cc]:L308
 2: 00007FF65B68BE66 node::Abort+22 [C:\Users\Himself65\Desktop\github\node\src\node_errors.cc]:L238
 3: 00007FF65B68C291 node::Assert+145 [C:\Users\Himself65\Desktop\github\node\src\node_errors.cc]:L254
 4: 00007FF65B4FDA02 node::crypto::DiffieHellman::New+882 [C:\Users\Himself65\Desktop\github\node\src\node_crypto.cc]:L5210
 5: 00007FF65C22F98F v8::internal::FunctionCallbackArguments::Call+335 [C:\Users\Himself65\Desktop\github\node\deps\v8\src\api\api-arguments-inl.h]:L159
 6: 00007FF65C22E81C v8::internal::`anonymous namespace'::HandleApiCallHelper<1>+380 [C:\Users\Himself65\Desktop\github\node\deps\v8\src\builtins\builtins-api.cc]:L113
 7: 00007FF65C22F042 v8::internal::Builtin_Impl_HandleApiCall+242 [C:\Users\Himself65\Desktop\github\node\deps\v8\src\builtins\builtins-api.cc]:L137
 8: 00007FF65C22EE73 v8::internal::Builtin_HandleApiCall+51 [C:\Users\Himself65\Desktop\github\node\deps\v8\src\builtins\builtins-api.cc]:L129
 9: 00007FF65C33598D Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit+61 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L14852
10: 00007FF65C2C9911 Builtins_JSBuiltinsConstructStub+97 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L674
11: 00007FF65C3B1EEB Builtins_ConstructHandler+187 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L30919
12: 00007FF65C2CDB28 Builtins_InterpreterEntryTrampoline+184 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L1224
13: 00007FF65C2C9817 Builtins_JSConstructStubGeneric+199 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L661
14: 00007FF65C3B1EEB Builtins_ConstructHandler+187 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L30919
15: 00007FF65C2CDB28 Builtins_InterpreterEntryTrampoline+184 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L1224
16: 00007FF65C2C76C9 Builtins_ArgumentsAdaptorTrampoline+185 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L339
17: 00007FF65C2CDB28 Builtins_InterpreterEntryTrampoline+184 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L1224
18: 00007FF65C2CDB28 Builtins_InterpreterEntryTrampoline+184 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L1224
19: 00007FF65C2CDB28 Builtins_InterpreterEntryTrampoline+184 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L1224
20: 00007FF65C2CDB28 Builtins_InterpreterEntryTrampoline+184 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L1224
21: 00007FF65C2CDB28 Builtins_InterpreterEntryTrampoline+184 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L1224
22: 00007FF65C2CDB28 Builtins_InterpreterEntryTrampoline+184 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L1224
23: 00007FF65C2CDB28 Builtins_InterpreterEntryTrampoline+184 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L1224
24: 00007FF65C2CB70E Builtins_JSEntryTrampoline+94 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L926
25: 00007FF65C2CB2FC Builtins_JSEntry+204 [C:\Users\Himself65\Desktop\github\node\out\Release\obj\v8_snapshot\embedded.S]:L887
26: 00007FF65C13DCAF v8::internal::`anonymous namespace'::Invoke+1247 [C:\Users\Himself65\Desktop\github\node\deps\v8\src\execution\execution.cc]:L374
27: 00007FF65C13D3DF v8::internal::Execution::Call+191 [C:\Users\Himself65\Desktop\github\node\deps\v8\src\execution\execution.cc]:L468
28: 00007FF65C277967 v8::Function::Call+615 [C:\Users\Himself65\Desktop\github\node\deps\v8\src\api\api.cc]:L4921
29: 00007FF65B6C446F node::ExecuteBootstrapper+159 [C:\Users\Himself65\Desktop\github\node\src\node.cc]:L190
30: 00007FF65B6C8C9A node::StartExecution+506 [C:\Users\Himself65\Desktop\github\node\src\node.cc]:L396
31: 00007FF65B6C9151 node::StartExecution+801 [C:\Users\Himself65\Desktop\github\node\src\node.cc]:L438
32: 00007FF65B731A08 node::LoadEnvironment+56 [C:\Users\Himself65\Desktop\github\node\src\api\environment.cc]:L439
33: 00007FF65B631713 node::NodeMainInstance::Run+163 [C:\Users\Himself65\Desktop\github\node\src\node_main_instance.cc]:L124
34: 00007FF65B6C8892 node::Start+274 [C:\Users\Himself65\Desktop\github\node\src\node.cc]:L1055
35: 00007FF65B46BE9D wmain+413 [C:\Users\Himself65\Desktop\github\node\src\node_main.cc]:L75
36: 00007FF65CA82194 __scrt_common_main_seh+268 [d:\agent\_work\4\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl]:L288
37: 00007FFDC3AD7BD4 BaseThreadInitThunk+20
38: 00007FFDC432CED1 RtlUserThreadStart+33```

@bnoordhuis
Copy link
Member Author

bnoordhuis commented Apr 10, 2020

But not because of this PR. It's a flaw in validateInt32(). Example:

$ ./out/Release/node -p 'os.getPriority(-0)'
./out/Release/node[24812]: ../src/node_os.cc:362:void node::os::GetPriority(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `args[0]->IsInt32()' failed.
 1: 0x563e16e51a34 node::Abort() [./out/Release/node]
 2: 0x563e16e51ac8  [./out/Release/node]
 3: 0x563e16ec214c  [./out/Release/node]
 4: 0x563e1705fe1f v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [./out/Release/node]
 5: 0x563e170601e0  [./out/Release/node]
 6: 0x563e17060a7a  [./out/Release/node]
 7: 0x563e1706132a v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [./out/Release/node]
 8: 0x563e178df599  [./out/Release/node]
Aborted (core dumped)

I'll open a separate PR for that. - already being discussed in #32750.

The JS code accepted any value where `typeof sizeOrKey === 'number'`
was true but the C++ code checked that `args[0]->IsInt32()` and
subsequently aborted.

Fixes: nodejs#32738
Validate the generator argument in `crypto.createDiffieHellman(key, g)`.
When it's a number, it should be an int32.

Fixes: nodejs#32748
It's possible to pass in the prime and generator params as buffers
but that mode of input wasn't as rigorously checked as numeric input.
@nodejs-github-bot
Copy link
Collaborator

@bnoordhuis
Copy link
Member Author

Could you fixed this by the way? I think they are same reason #32748

That turns out to be a subtly different issue, see the second commit.

It also turns out that it's possible to slip in invalid prime and generator values. I added additional validation in the third commit.

Sorry @addaleax @cjihrig, can I ask you to review the new commits too?

@nodejs-github-bot
Copy link
Collaborator

@cjihrig
Copy link
Contributor

cjihrig commented Apr 10, 2020

Still LGTM

@@ -20,6 +20,62 @@ assert.strictEqual(secret2.toString('base64'), secret1);
assert.strictEqual(dh1.verifyError, 0);
assert.strictEqual(dh2.verifyError, 0);

// https://github.com/nodejs/node/issues/32738
// XXX(bnoordhuis) validateInt32() throwing ERR_OUT_OF_RANGE and RangeError
// instead of ERR_INVALID_ARG_TYPE and TypeError is questionable, IMO.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: it does throw ERR_INVALID_ARG_TYPE in case it's not a number.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 27, 2020
@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member

Landed in 73324cf...38146e7

@addaleax addaleax closed this Apr 28, 2020
addaleax pushed a commit that referenced this pull request Apr 28, 2020
The JS code accepted any value where `typeof sizeOrKey === 'number'`
was true but the C++ code checked that `args[0]->IsInt32()` and
subsequently aborted.

Fixes: #32738

PR-URL: #32739
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Apr 28, 2020
Validate the generator argument in `crypto.createDiffieHellman(key, g)`.
When it's a number, it should be an int32.

Fixes: #32748

PR-URL: #32739
Fixes: #32738
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Apr 28, 2020
It's possible to pass in the prime and generator params as buffers
but that mode of input wasn't as rigorously checked as numeric input.

PR-URL: #32739
Fixes: #32738
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 4, 2020
The JS code accepted any value where `typeof sizeOrKey === 'number'`
was true but the C++ code checked that `args[0]->IsInt32()` and
subsequently aborted.

Fixes: #32738

PR-URL: #32739
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 4, 2020
Validate the generator argument in `crypto.createDiffieHellman(key, g)`.
When it's a number, it should be an int32.

Fixes: #32748

PR-URL: #32739
Fixes: #32738
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 4, 2020
It's possible to pass in the prime and generator params as buffers
but that mode of input wasn't as rigorously checked as numeric input.

PR-URL: #32739
Fixes: #32738
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request May 4, 2020
targos pushed a commit that referenced this pull request May 7, 2020
The JS code accepted any value where `typeof sizeOrKey === 'number'`
was true but the C++ code checked that `args[0]->IsInt32()` and
subsequently aborted.

Fixes: #32738

PR-URL: #32739
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 7, 2020
Validate the generator argument in `crypto.createDiffieHellman(key, g)`.
When it's a number, it should be an int32.

Fixes: #32748

PR-URL: #32739
Fixes: #32738
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 7, 2020
It's possible to pass in the prime and generator params as buffers
but that mode of input wasn't as rigorously checked as numeric input.

PR-URL: #32739
Fixes: #32738
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 13, 2020
The JS code accepted any value where `typeof sizeOrKey === 'number'`
was true but the C++ code checked that `args[0]->IsInt32()` and
subsequently aborted.

Fixes: #32738

PR-URL: #32739
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 13, 2020
Validate the generator argument in `crypto.createDiffieHellman(key, g)`.
When it's a number, it should be an int32.

Fixes: #32748

PR-URL: #32739
Fixes: #32738
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 13, 2020
It's possible to pass in the prime and generator params as buffers
but that mode of input wasn't as rigorously checked as numeric input.

PR-URL: #32739
Fixes: #32738
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crypto.createDiffieHellman results in an abort
9 participants