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.pbkdf2Sync results in an abort with some arguments #44570

Closed
zyscoder opened this issue Sep 8, 2022 · 5 comments · Fixed by #44575
Closed

crypto.pbkdf2Sync results in an abort with some arguments #44570

zyscoder opened this issue Sep 8, 2022 · 5 comments · Fixed by #44575
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem.

Comments

@zyscoder
Copy link

zyscoder commented Sep 8, 2022 •
edited by VoltrexKeyva
Loading

Version

v18.8.0

Platform

Linux zys-lab204l 5.15.0-46-generic #49~20.04.1-Ubuntu SMP Thu Aug 4 19:15:44 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

crypto

What steps will reproduce the bug?

Setup a node instance,

$ node

and run the following javascript code.

crypto = require('crypto');crypto.pbkdf2Sync('str','str',2147485780,0,'str');

Then the node instance occurs an abort.

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

This abort can always be triggered following the steps above.

What is the expected behavior?

If any error occurs, an exception or other similar error-reporting stuff should be thrown. There is no reason to abort the whole node process.

What do you see instead?

 $ node                                            
Welcome to Node.js v18.8.0.
Type ".help" for more information.
> crypto = require('crypto');crypto.pbkdf2Sync('str','str',2147485780,0,'str');
/home/zys/Toolchains/node/out/Release/node[1177351]: ../src/crypto/crypto_pbkdf2.cc:88:static Maybe<bool> node::crypto::PBKDF2Traits::AdditionalConfig(node::crypto::CryptoJobMode, const FunctionCallbackInfo<v8::Value> &, unsigned int, node::crypto::PBKDF2Config *): Assertion `args[offset + 2]->IsInt32()' failed.
 1: 0x3a4188f node::DumpBacktrace(_IO_FILE*) [/home/zys/Toolchains/node/out/Release/node]
 2: 0x3c22cf4 node::Abort() [/home/zys/Toolchains/node/out/Release/node]
 3: 0x3c22665 node::Assert(node::AssertionInfo const&) [/home/zys/Toolchains/node/out/Release/node]
 4: 0x40306f5 node::crypto::PBKDF2Traits::AdditionalConfig(node::crypto::CryptoJobMode, v8::FunctionCallbackInfo<v8::Value> const&, unsigned int, node::crypto::PBKDF2Config*) [/home/zys/Toolchains/node/out/Release/node]
 5: 0x4115ad2 node::crypto::DeriveBitsJob<node::crypto::PBKDF2Traits>::New(v8::FunctionCallbackInfo<v8::Value> const&) [/home/zys/Toolchains/node/out/Release/node]
 6: 0x4307694 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [/home/zys/Toolchains/node/out/Release/node]
 7: 0x4305840  [/home/zys/Toolchains/node/out/Release/node]
 8: 0x4304365 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [/home/zys/Toolchains/node/out/Release/node]
 9: 0x61a9139  [/home/zys/Toolchains/node/out/Release/node]
[1]    1177351 abort (core dumped)  /home/zys/Toolchains/node/out/Release/node

Additional information

No response
@Trott Trott added the crypto Issues and PRs related to the crypto subsystem. label Sep 8, 2022
@Trott
Copy link
Member

Trott commented Sep 8, 2022

@nodejs/crypto

@Trott
Copy link
Member

Trott commented Sep 8, 2022

Replicated on macOS too with Node.js 18.8.0 and Node.js 16.17.0.

This problem does not exist in Node.js 14.20.0 which throws a TypeError instead:

Uncaught TypeError [ERR_CRYPTO_INVALID_DIGEST]: Invalid digest: str
    at new NodeError (internal/errors.js:322:7)
    at handleError (internal/crypto/pbkdf2.js:68:11)
    at Object.pbkdf2Sync (internal/crypto/pbkdf2.js:48:3)
    at REPL1:1:35
    at Script.runInThisContext (vm.js:134:12)
    at REPLServer.defaultEval (repl.js:566:29)
    at bound (domain.js:421:15)
    at REPLServer.runBound [as eval] (domain.js:432:12)
    at REPLServer.onLine (repl.js:909:10)
    at REPLServer.emit (events.js:412:35) {
  code: 'ERR_CRYPTO_INVALID_DIGEST'
}

@Trott
Copy link
Member

Trott commented Sep 8, 2022

Node.js 15.0.0 also aborts, so perhaps looking at the 15.0.0 changelog might help identify which change likely resulted in this.

@Trott
Copy link
Member

Trott commented Sep 8, 2022

The two semver-major commits in 15.0.0 are dae283d96f and ba77dc8597 so maybe start with those?

@jasnell

@tniessen tniessen added the confirmed-bug Issues with confirmed bugs. label Sep 8, 2022
tniessen added a commit to tniessen/node that referenced this issue Sep 8, 2022
@tniessen
crypto: restrict PBKDF2 args to signed int
3927e1a 
OpenSSL internally represents the output length and the iteration count
as signed integers, which is why node's C++ implementation expects these
arguments to fit into signed integers as well. The JavaScript validation
logic, however, only requires the arguments to be unsigned 32-bit
integers, which is a superset of non-negative (signed) 32-bit integers.

Change the JavaScript validation to match the expectation within C++.

Fixes: nodejs#44570
@tniessen tniessen mentioned this issue Sep 8, 2022
Merged
@tniessen
Copy link
Member

tniessen commented Sep 8, 2022

Sigh. This is why unsigned values should not be stored as signed integers.

#44575

nodejs-github-bot pushed a commit that referenced this issue Sep 10, 2022
@tniessen
crypto: restrict PBKDF2 args to signed int
8bf7754 
OpenSSL internally represents the output length and the iteration count
as signed integers, which is why node's C++ implementation expects these
arguments to fit into signed integers as well. The JavaScript validation
logic, however, only requires the arguments to be unsigned 32-bit
integers, which is a superset of non-negative (signed) 32-bit integers.

Change the JavaScript validation to match the expectation within C++.

Fixes: #44570
PR-URL: #44575
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Fyko pushed a commit to Fyko/node that referenced this issue Sep 15, 2022
@tniessen @Fyko
crypto: restrict PBKDF2 args to signed int
4303bab 
OpenSSL internally represents the output length and the iteration count
as signed integers, which is why node's C++ implementation expects these
arguments to fit into signed integers as well. The JavaScript validation
logic, however, only requires the arguments to be unsigned 32-bit
integers, which is a superset of non-negative (signed) 32-bit integers.

Change the JavaScript validation to match the expectation within C++.

Fixes: nodejs#44570
PR-URL: nodejs#44575
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
RafaelGSS pushed a commit that referenced this issue Sep 26, 2022
@tniessen @RafaelGSS
crypto: restrict PBKDF2 args to signed int
26f25c9 
OpenSSL internally represents the output length and the iteration count
as signed integers, which is why node's C++ implementation expects these
arguments to fit into signed integers as well. The JavaScript validation
logic, however, only requires the arguments to be unsigned 32-bit
integers, which is a superset of non-negative (signed) 32-bit integers.

Change the JavaScript validation to match the expectation within C++.

Fixes: #44570
PR-URL: #44575
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
RafaelGSS pushed a commit that referenced this issue Sep 26, 2022
@tniessen @RafaelGSS
crypto: restrict PBKDF2 args to signed int
9a7bd19 
OpenSSL internally represents the output length and the iteration count
as signed integers, which is why node's C++ implementation expects these
arguments to fit into signed integers as well. The JavaScript validation
logic, however, only requires the arguments to be unsigned 32-bit
integers, which is a superset of non-negative (signed) 32-bit integers.

Change the JavaScript validation to match the expectation within C++.

Fixes: #44570
PR-URL: #44575
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
RafaelGSS pushed a commit that referenced this issue Sep 26, 2022
@tniessen @RafaelGSS
crypto: restrict PBKDF2 args to signed int
993bd9b 
OpenSSL internally represents the output length and the iteration count
as signed integers, which is why node's C++ implementation expects these
arguments to fit into signed integers as well. The JavaScript validation
logic, however, only requires the arguments to be unsigned 32-bit
integers, which is a superset of non-negative (signed) 32-bit integers.

Change the JavaScript validation to match the expectation within C++.

Fixes: #44570
PR-URL: #44575
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
panva pushed a commit to panva/node that referenced this issue Oct 3, 2022
@tniessen @panva
crypto: restrict PBKDF2 args to signed int
d4260a2 
OpenSSL internally represents the output length and the iteration count
as signed integers, which is why node's C++ implementation expects these
arguments to fit into signed integers as well. The JavaScript validation
logic, however, only requires the arguments to be unsigned 32-bit
integers, which is a superset of non-negative (signed) 32-bit integers.

Change the JavaScript validation to match the expectation within C++.

Fixes: nodejs#44570
PR-URL: nodejs#44575
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Backport-PR-URL: nodejs#44872
juanarbol pushed a commit that referenced this issue Oct 4, 2022
@tniessen @juanarbol
crypto: restrict PBKDF2 args to signed int
1e11484 
OpenSSL internally represents the output length and the iteration count
as signed integers, which is why node's C++ implementation expects these
arguments to fit into signed integers as well. The JavaScript validation
logic, however, only requires the arguments to be unsigned 32-bit
integers, which is a superset of non-negative (signed) 32-bit integers.

Change the JavaScript validation to match the expectation within C++.

Fixes: #44570
PR-URL: #44575
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
juanarbol pushed a commit that referenced this issue Oct 4, 2022
@tniessen @juanarbol
crypto: restrict PBKDF2 args to signed int
28b04cb 
OpenSSL internally represents the output length and the iteration count
as signed integers, which is why node's C++ implementation expects these
arguments to fit into signed integers as well. The JavaScript validation
logic, however, only requires the arguments to be unsigned 32-bit
integers, which is a superset of non-negative (signed) 32-bit integers.

Change the JavaScript validation to match the expectation within C++.

Fixes: #44570
PR-URL: #44575
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
juanarbol pushed a commit that referenced this issue Oct 4, 2022
@tniessen @juanarbol
crypto: restrict PBKDF2 args to signed int
2108784 
OpenSSL internally represents the output length and the iteration count
as signed integers, which is why node's C++ implementation expects these
arguments to fit into signed integers as well. The JavaScript validation
logic, however, only requires the arguments to be unsigned 32-bit
integers, which is a superset of non-negative (signed) 32-bit integers.

Change the JavaScript validation to match the expectation within C++.

Fixes: #44570
PR-URL: #44575
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
juanarbol pushed a commit that referenced this issue Oct 7, 2022
@tniessen @juanarbol
crypto: restrict PBKDF2 args to signed int
4ee1d5f 
OpenSSL internally represents the output length and the iteration count
as signed integers, which is why node's C++ implementation expects these
arguments to fit into signed integers as well. The JavaScript validation
logic, however, only requires the arguments to be unsigned 32-bit
integers, which is a superset of non-negative (signed) 32-bit integers.

Change the JavaScript validation to match the expectation within C++.

Fixes: #44570
PR-URL: #44575
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
juanarbol pushed a commit that referenced this issue Oct 10, 2022
@tniessen @juanarbol
crypto: restrict PBKDF2 args to signed int
d976826 
OpenSSL internally represents the output length and the iteration count
as signed integers, which is why node's C++ implementation expects these
arguments to fit into signed integers as well. The JavaScript validation
logic, however, only requires the arguments to be unsigned 32-bit
integers, which is a superset of non-negative (signed) 32-bit integers.

Change the JavaScript validation to match the expectation within C++.

Fixes: #44570
PR-URL: #44575
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
juanarbol pushed a commit that referenced this issue Oct 11, 2022
@tniessen @juanarbol
crypto: restrict PBKDF2 args to signed int
1fc6394 
OpenSSL internally represents the output length and the iteration count
as signed integers, which is why node's C++ implementation expects these
arguments to fit into signed integers as well. The JavaScript validation
logic, however, only requires the arguments to be unsigned 32-bit
integers, which is a superset of non-negative (signed) 32-bit integers.

Change the JavaScript validation to match the expectation within C++.

Fixes: #44570
PR-URL: #44575
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
@tniessen @guangwong
crypto: restrict PBKDF2 args to signed int
f9d6a2b 
OpenSSL internally represents the output length and the iteration count
as signed integers, which is why node's C++ implementation expects these
arguments to fit into signed integers as well. The JavaScript validation
logic, however, only requires the arguments to be unsigned 32-bit
integers, which is a superset of non-negative (signed) 32-bit integers.

Change the JavaScript validation to match the expectation within C++.

Fixes: nodejs/node#44570
PR-URL: nodejs/node#44575
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
@tniessen @guangwong
crypto: restrict PBKDF2 args to signed int
b11d8f7 
OpenSSL internally represents the output length and the iteration count
as signed integers, which is why node's C++ implementation expects these
arguments to fit into signed integers as well. The JavaScript validation
logic, however, only requires the arguments to be unsigned 32-bit
integers, which is a superset of non-negative (signed) 32-bit integers.

Change the JavaScript validation to match the expectation within C++.

Fixes: nodejs/node#44570
PR-URL: nodejs/node#44575
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

crypto: restrict PBKDF2 args to signed int tniessen/node
3 participants
@Trott @tniessen @zyscoder