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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Apr 9, 2020 •
edited by himself65
Loading

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.

Sign in to view
himself65
himself65 reviewed Apr 9, 2020
View reviewed changes
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

node/lib/internal/crypto/diffiehellman.js

Lines 44 to 52 in aeb7084

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.

cjihrig
cjihrig approved these changes Apr 9, 2020
View reviewed changes
lib/internal/crypto/diffiehellman.js
// 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.

himself65
himself65 approved these changes Apr 9, 2020
View reviewed changes
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

@hassaanp hassaanp mentioned this pull request Apr 10, 2020
Closed
@himself65 himself65 self-requested a review April 10, 2020 04:35
@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 •
edited
Loading

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.

@bnoordhuis
crypto: key size must be int32 in DiffieHellman()
c090e6d 
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
@bnoordhuis
crypto: generator must be int32 in DiffieHellman()
a3d413a 
Validate the generator argument in `crypto.createDiffieHellman(key, g)`.
When it's a number, it should be an int32.

Fixes: nodejs#32748
@bnoordhuis
crypto: check DiffieHellman p and g params
80ee6c0 
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

CI: https://ci.nodejs.org/job/node-test-pull-request/30612/

@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

CI: https://ci.nodejs.org/job/node-test-pull-request/30625/

@cjihrig
Copy link
Contributor

cjihrig commented Apr 10, 2020

Still LGTM

BridgeAR
BridgeAR reviewed Apr 11, 2020
View reviewed changes
test/parallel/test-crypto-dh.js
@@ -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

CI: https://ci.nodejs.org/job/node-test-pull-request/31037/

@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
@bnoordhuis @addaleax
crypto: key size must be int32 in DiffieHellman()
6fdced4 
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 <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Apr 28, 2020
@bnoordhuis @addaleax
crypto: generator must be int32 in DiffieHellman()
0694401 
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 <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Apr 28, 2020
@bnoordhuis @addaleax
crypto: check DiffieHellman p and g params
38146e7 
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 <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request May 4, 2020
@bnoordhuis @targos
crypto: key size must be int32 in DiffieHellman()
637442f 
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 <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request May 4, 2020
@bnoordhuis @targos
crypto: generator must be int32 in DiffieHellman()
e07cca6 
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 <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request May 4, 2020
@bnoordhuis @targos
crypto: check DiffieHellman p and g params
83e165b 
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 <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request May 4, 2020
Merged
targos pushed a commit that referenced this pull request May 7, 2020
@bnoordhuis @targos
crypto: key size must be int32 in DiffieHellman()
adfa469 
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 <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request May 7, 2020
@bnoordhuis @targos
crypto: generator must be int32 in DiffieHellman()
22c08cd 
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 <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request May 7, 2020
@bnoordhuis @targos
crypto: check DiffieHellman p and g params
ff70ecb 
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 <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request May 7, 2020
Merged
targos pushed a commit that referenced this pull request May 13, 2020
@bnoordhuis @targos
crypto: key size must be int32 in DiffieHellman()
4236175 
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 <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request May 13, 2020
@bnoordhuis @targos
crypto: generator must be int32 in DiffieHellman()
c1b7674 
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 <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request May 13, 2020
@bnoordhuis @targos
crypto: check DiffieHellman p and g params
71bccdd 
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 <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@himself65 himself65 himself65 approved these changes

@hassaanp hassaanp hassaanp approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

crypto.createDiffieHellman results in an abort
9 participants
@bnoordhuis @nodejs-github-bot @himself65 @targos @cjihrig @addaleax @jasnell @BridgeAR @hassaanp