crypto: reject non-int32 values in DiffieHellman()#32739
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
nodejs-github-bot added the crypto 
This comment has been minimized.
This comment has been minimized.
Member
himself65 left a comment
himself65 left a comment There was a problem hiding this comment.
can we inline this logic to
node/lib/internal/crypto/diffiehellman.js
Lines 44 to 52 in aeb7084
if(typeofsizeOrKey!=='number'){if(typeofsizeOrKey!=='string'&&!isArrayBufferView(sizeOrKey))thrownewERR_INVALID_ARG_TYPE('sizeOrKey',['number','string','Buffer','TypedArray','DataView'],sizeOrKey);elsevalidateInt32(sizeOrKey,'sizeOrKey');}MemberAuthor
bnoordhuis commented Apr 9, 2020
Can? Sure, but it's another level of indentation. I like my code flat. |
cjihrig approved these changes Apr 9, 2020
lib/internal/crypto/diffiehellman.js Outdated
Contributor
There was a problem hiding this comment.
You can add 0 as a third parameter here if you want to enforce the minimum in JS.
MemberAuthor
There was a problem hiding this comment.
I'm aware but that changes the error message on inputs < 0. I added a regression test instead.
himself65 approved these changes Apr 9, 2020
Member
himself65 left a comment
himself65 left a comment There was a problem hiding this comment.
LGTM, code flat better to understand I think
addaleax approved these changes Apr 9, 2020
Member
himself65 commented Apr 10, 2020
Could you fixed this by the way? I think they are same reason |
Member
targos commented Apr 10, 2020
In the light of #32750, what happens with this API if |
Member
himself65 commented Apr 10, 2020
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]:L37427: 00007FF65C13D3DF v8::internal::Execution::Call+191 [C:\Users\Himself65\Desktop\github\node\deps\v8\src\execution\execution.cc]:L46828: 00007FF65C277967 v8::Function::Call+615 [C:\Users\Himself65\Desktop\github\node\deps\v8\src\api\api.cc]:L492129: 00007FF65B6C446F node::ExecuteBootstrapper+159 [C:\Users\Himself65\Desktop\github\node\src\node.cc]:L19030: 00007FF65B6C8C9A node::StartExecution+506 [C:\Users\Himself65\Desktop\github\node\src\node.cc]:L39631: 00007FF65B6C9151 node::StartExecution+801 [C:\Users\Himself65\Desktop\github\node\src\node.cc]:L43832: 00007FF65B731A08 node::LoadEnvironment+56 [C:\Users\Himself65\Desktop\github\node\src\api\environment.cc]:L43933: 00007FF65B631713 node::NodeMainInstance::Run+163 [C:\Users\Himself65\Desktop\github\node\src\node_main_instance.cc]:L12434: 00007FF65B6C8892 node::Start+274 [C:\Users\Himself65\Desktop\github\node\src\node.cc]:L105535: 00007FF65B46BE9D wmain+413 [C:\Users\Himself65\Desktop\github\node\src\node_main.cc]:L7536: 00007FF65CA82194 __scrt_common_main_seh+268 [d:\agent\_work\4\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl]:L28837: 00007FFDC3AD7BD4 BaseThreadInitThunk+2038: 00007FFDC432CED1 RtlUserThreadStart+33``` |
MemberAuthor
But not because of this PR. It's a flaw in
|
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.
Collaborator
nodejs-github-bot commented Apr 10, 2020
MemberAuthor
bnoordhuis commented Apr 10, 2020
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? |
jasnell approved these changes Apr 10, 2020
himself65 approved these changes Apr 10, 2020
Collaborator
nodejs-github-bot commented Apr 10, 2020
Contributor
cjihrig commented Apr 10, 2020
Still LGTM |
| // 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. |
Member
There was a problem hiding this comment.
Note: it does throw ERR_INVALID_ARG_TYPE in case it's not a number.
hassaanp approved these changes Apr 13, 2020
addaleax added the author ready Collaborator
nodejs-github-bot commented Apr 28, 2020
Member
addaleax commented Apr 28, 2020
Landed in 73324cf...38146e7 |
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 <[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
Validate the generator argument in `crypto.createDiffieHellman(key, g)`. When it's a number, it should be an int32. Fixes: #32748 PR-URL: #32739Fixes: #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
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: #32739Fixes: #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
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
Validate the generator argument in `crypto.createDiffieHellman(key, g)`. When it's a number, it should be an int32. Fixes: #32748 PR-URL: #32739Fixes: #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
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: #32739Fixes: #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
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
Validate the generator argument in `crypto.createDiffieHellman(key, g)`. When it's a number, it should be an int32. Fixes: #32748 PR-URL: #32739Fixes: #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
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: #32739Fixes: #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
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
Validate the generator argument in `crypto.createDiffieHellman(key, g)`. When it's a number, it should be an int32. Fixes: #32748 PR-URL: #32739Fixes: #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
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: #32739Fixes: #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 freeto join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The JS code accepted any value where
typeof sizeOrKey === 'number'was true but the C++ code checked that
args[0]->IsInt32()andsubsequently aborted.
Fixes: #32738#32748