Skip to content

Commit 0249248

Browse files
ljharbjuanarbol
authored andcommitted
crypto: coerce -0 keylen to +0 in pbkdf2 and scrypt
`validateInt32(keylen, 'keylen', 0)` lets `-0` through: `typeof -0` is `'number'`, `Number.isInteger(-0)` is `true`, and `-0 < 0` is `false`. The value then reaches the PBKDF2Job binding, whose `IsInt32()` check fails (V8 boxes `-0` as a HeapNumber rather than a tagged SMI) and aborts the process with SIGABRT. Coerce `keylen` to `+0` after validation so the binding sees a true Int32. Reachable from any caller that forwards a JSON-parsed value, since `JSON.parse('{"keylen":-0}').keylen` preserves the sign. Mirror of the prior pbkdf2 fix. `validateInt32(keylen, 'keylen', 0)` lets `-0` through (since `-0 < 0` is `false`), and the ScryptJob binding's `IsInt32()` check at `crypto_scrypt.cc` aborts the process with SIGABRT because V8 boxes `-0` as a HeapNumber rather than a tagged SMI. Coerce `keylen` to `+0` after validation. Signed-off-by: Jordan Harband <ljharb@gmail.com> PR-URL: #63531 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
1 parent 310f60a commit 0249248

4 files changed

Lines changed: 60 additions & 0 deletions

File tree

‎lib/internal/crypto/pbkdf2.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,8 @@ function check(password, salt, iterations, keylen, digest) {
8787
// to the 31-bit range here (which is plenty).
8888
validateInt32(iterations,'iterations',1);
8989
validateInt32(keylen,'keylen',0);
90+
// Coerce -0 to +0.
91+
keylen+=0;
9092

9193
return{ password, salt, iterations, keylen, digest };
9294
}

‎lib/internal/crypto/scrypt.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,8 @@ function check(password, salt, keylen, options) {
8383
password=getArrayBufferOrView(password,'password');
8484
salt=getArrayBufferOrView(salt,'salt');
8585
validateInt32(keylen,'keylen',0);
86+
// Coerce -0 to +0.
87+
keylen+=0;
8688

8789
let{ N, r, p, maxmem }=defaults;
8890
if(options&&options!==defaults){

‎test/parallel/test-crypto-pbkdf2.js‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,35 @@ for (const iterations of [-1, 0, 2147483648]) {
110110
});
111111
});
112112

113+
// `-0` keylen must not abort the process via the native binding's
114+
// IsInt32() assertion. Behavior of `keylen=0` itself varies by OpenSSL
115+
// build (bundled returns an empty buffer; some shared OpenSSL builds
116+
// throw); the requirement here is only that `-0` produces the same
117+
// outcome as `+0`.
118+
{
119+
letposError;
120+
letposResult;
121+
try{
122+
posResult=crypto.pbkdf2Sync('password','salt',1,0,'sha256');
123+
}catch(err){
124+
posError=err;
125+
}
126+
letnegError;
127+
letnegResult;
128+
try{
129+
negResult=crypto.pbkdf2Sync('password','salt',1,-0,'sha256');
130+
}catch(err){
131+
negError=err;
132+
}
133+
if(posError!==undefined){
134+
assert.strictEqual(negError?.message,posError.message);
135+
}else{
136+
assert.deepStrictEqual(negResult,posResult);
137+
}
138+
139+
crypto.pbkdf2('password','salt',1,-0,'sha256',common.mustCall());
140+
}
141+
113142
// Should not get FATAL ERROR with empty password and salt
114143
// https://github.com/nodejs/node/issues/8571
115144
crypto.pbkdf2('','',1,32,'sha256',common.mustSucceed());

‎test/parallel/test-crypto-scrypt.js‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,3 +256,30 @@ for (const { args, expected } of badargs) {
256256
['p',1],['parallelization',1],
257257
].forEach((arg)=>testParameter(...arg));
258258
}
259+
260+
// `-0` keylen must not abort the process via the native binding's
261+
// IsInt32() assertion. Assert that `-0` produces the same outcome as
262+
// `+0` (which differs by OpenSSL build).
263+
{
264+
letposError;
265+
letposResult;
266+
try{
267+
posResult=crypto.scryptSync('','',0);
268+
}catch(err){
269+
posError=err;
270+
}
271+
letnegError;
272+
letnegResult;
273+
try{
274+
negResult=crypto.scryptSync('','',-0);
275+
}catch(err){
276+
negError=err;
277+
}
278+
if(posError!==undefined){
279+
assert.strictEqual(negError?.message,posError.message);
280+
}else{
281+
assert.deepStrictEqual(negResult,posResult);
282+
}
283+
284+
crypto.scrypt('','',-0,common.mustCall());
285+
}

0 commit comments

Comments
 (0)