Repository navigation
node:crypto: read integer arguments by value, not by JSValue representation - #39977
Conversation
…tation
An integer-valued Number is not always an int32 JSValue. An element of
an array that also holds a non-int32 number, or a value read out of a
Float64Array, is stored as a double. Four node:crypto entry points
looked at the representation:
- getIntOption() (the padding and saltLength options of sign, verify,
Sign#sign and Verify#verify) rejected such a value, and -0, with
ERR_INVALID_ARG_VALUE. Node accepts `value === value >> 0`.
- validateOneOf() for int32 lists (generateKey('aes').length) rejected
it. Node compares with Array#includes.
- createDiffieHellman(prime, generator) and getCipherInfo(nid) validated
the value and then read it with asInt32(), which asserts in debug
builds and reads 0 in release builds, so the generator was reported
as bad and the cipher as unknown.
The first two now decide by numeric value. The other two take the value
from the validator, which already handles both representations.
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 30 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 13 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Status: ready for review. Reproduced with (Bun 1.4 main release build; Node v26.3.0 accepts all four): const crypto = require("crypto");
const asDouble = n => new Float64Array([n])[0]; // 20, stored as a double JSValue
const { privateKey, publicKey } = crypto.generateKeyPairSync("rsa", { modulusLength: 1024 });
const pss = crypto.constants.RSA_PKCS1_PSS_PADDING, d = Buffer.from("msg");
const sig = crypto.sign("sha256", d, { key: privateKey, padding: pss, saltLength: 20 });
crypto.verify("sha256", d, { key: publicKey, padding: pss, saltLength: asDouble(20) }, sig); // ERR_INVALID_ARG_VALUE ... Received 20
crypto.generateKeySync("aes", { length: asDouble(128) }); // must be one of: 128, 192, 256
crypto.createDiffieHellman(crypto.getDiffieHellman("modp5").getPrime(), asDouble(2)); // BAD_GENERATOR (debug: ASSERTION FAILED: isInt32())
crypto.getCipherInfo(asDouble(crypto.getCipherInfo("aes-128-cbc").nid)); // undefined (debug: ASSERTION FAILED: isInt32())Test: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted fix replacing isInt32()/asInt32() representation checks with value-based reads at four node:crypto entry points.
Checked that getIntOption() still rejects fractions, values past int32, non-numbers and NaN (the toInt32 round-trip fails for each), and now accepts -0 like Node's value === value >> 0. Checked validateOneOf's only int32 caller (aes length) — semantics widen only to accept double-encoded members of the list. Traced the DH constructor's generatorNumber through the falsy-generator branch (stays at the initial 2, matching the old jsNumber(2) → asInt32() path) and confirmed the removed second validateInt32 was redundant with the first. getCipherInfo's nid local is only read on the isNumber() branch.
Extended reasoning...
Overview
Four node:crypto argument-reading sites tested JSValue::isInt32() / called asInt32(), which check the JSValue storage form rather than the numeric value. An integer that happens to be stored as a double (element of a mixed-width array, a Float64Array read, -0) was wrongly rejected or, in two places, read as garbage after a debug assertion. The fix:
getIntOption()(CryptoUtil.cpp) now accepts any Number that survives ECMAScript ToInt32 unchanged, matching Node'svalue === value >> 0.- The int32
V::validateOneOf()(NodeValidator.cpp) comparesasNumber()against the list, matching Node'sArray#includes. constructDiffieHellmanandjsGetCipherInfonow take the validated integer throughvalidateInt32's out parameter (which already handles both encodings) instead of re-reading withasInt32(). The DH constructor also drops a redundant secondvalidateInt32on the same argument.
Tests added in node-crypto.test.js cover all four entry points (sign/verify padding & saltLength via both one-shot and streaming APIs, generateKeySync('aes') length, DH generator, getCipherInfo nid), plus negative cases showing fractions, out-of-range values and non-numbers are still rejected with the same error codes.
Security risks
None. This is argument-validation compatibility work. Validation is not weakened: every input the old code rejected that Node also rejects (fractions, 2**31, strings, null) is still rejected on the same path with the same error. The change only stops rejecting integer-valued Numbers that happen to be double-encoded — inputs Node has always accepted.
Level of scrutiny
Moderate. The touched code is C++ JSC bindings for node:crypto argument handling — no memory ownership, no allocation, no threading, no user-callback re-entry. asNumber() and validateInt32's out param are the established idioms for reading integers regardless of representation, already used throughout the file. The DH-constructor refactor moves where the integer is captured but I traced all three branches of the first generatorValue block (falsy → default 2 preserved via initializer; number → validator writes out param; string/buffer → integer never read) and each reaches the later use with the correct value.
Other factors
validateOneOfint32 overload has exactly one caller (aes key length), so no unintended widening elsewhere.JSC::toInt32(double)is the standard ToInt32 helper already used in napi.cpp / NodeSqlite.cpp; the newMathCommon.hinclude is the right header.- Tests use
toThrow(prefix)for thevalidateOneOfmessage so they stay green when #38401 adds the. Receivedsuffix. - PR description reports the four new tests fail on main and the surrounding crypto suites (including Node's own
test-crypto-*) still pass.
… not by JSValue representation (#39991) ### Problem - Three readers test how JSC boxes a Number, not its value. With an integer boxed as a double: `clearTimeout(id)` / `clearInterval(id)` clears nothing and the timer fires (`Timer.rs:318`, `is_int32()`). `SourceTextModule#evaluate({ timeout })` never arms the deadline (`NodeVMModule.cpp:547`, `isUInt32()`). `Bun.inspect(v, { depth })` throws `expected depth to be an integer, got 1`, also for `-0` (`ConsoleObject.rs:1289`, `:1321`). - Such a value comes from a `Float64Array` read, `Math.sqrt()`, `-0` or a sum of fractions. It is `===` to the int32, so the bugs look intermittent. Node accepts all of these. ### Fix - `clear_timer()` takes any Number whose value round-trips through `i32` as an id, like Node's `knownTimersById[id]`. Fractions, `NaN` and values outside int32 still clear nothing. - `jsNodeVmModuleEvaluate()` reads the timeout with `isUInt32AsAnyInt()`. `vm.ts` already validated it by value, and its `-1` for "no timeout" still maps to none. - The two copies of the depth reader become one `FormatOptions::set_depth()` that works on the value. Same messages. `-0` is depth 0. An integer past int32 is clamped to `u16::MAX`, as a large int32 was. - Verified: seven new tests in `test/js/web/timers/setTimeout.test.js`, `test/js/node/vm/vm.test.ts` and `test/js/node/util/bun-inspect.test.ts`. Six fail on the unfixed build. The full files and Node's timer clear tests pass. ### Background - A `JSValue` holds a Number as an int32 or as a double. Literals give the int32 form. Typed array reads, `Math` functions, `-0` and double arithmetic give the double form, even for an integer value. - `isInt32()` / `isUInt32()` / `is_int32()` report the form. `as_number()` and `isUInt32AsAnyInt()` report the value in either form. - Same class: #39977 (`node:crypto`), and open PRs #32274 (`dgram` port) and #38236 (`node:vm` offsets) for the two other known sites. #35289 edits the same depth blocks, so the second one to land needs a small rebase. <details><summary>Notes</summary> Probed on bun 1.4.0 with `Bun.inspect(obj, { depth: v })` as the detector. Double-boxed: `new Float64Array([1])[0]`, `DataView#getFloat64()`, `JSON.parse("[4294967296,1]")[1]`, `Math.sqrt(1)`, `-0`, `x = 0.5; x += 0.5`. Int32-boxed: a literal, `[1.5, 1][1]`, `JSON.parse("1.0")`, `Math.floor(1.5)`, `0.5 + 0.5` in one expression, `parseFloat("1")`. Node v26.3.0 accepts every double-boxed case at all three APIs. Timer ids are `i32` (`All::last_id` wraps), so the int32 range is the whole id space. `timer_id_from_number()` uses the saturating `as i32` cast and checks that the result converts back to the same `f64`. That rejects `NaN` (the cast gives 0), fractions and anything outside int32 with one comparison. `-0` maps to id 0, as Node's `knownTimersById[-0]` reads `knownTimersById["0"]`. `set_depth()` is unchanged for int32 inputs: the same two messages, the same `u16::MAX` clamp, and `Infinity` of either sign is unlimited, as before. Two double-only inputs change: a double-boxed negative now gets the "greater than or equal to 0" message instead of "an integer", and an integral double past int32 (`2**32`) is clamped instead of rejected. The vm test spins for at most 2 seconds, so the unfixed build fails in about 2 seconds with "resolved evaluated" instead of hanging. The fixed build stops it at the 20 ms deadline. The seventh test ("non-integers still throw") passes on both builds by design. A census of `isInt32` / `isUInt32` / `asInt32` / `is_int32` / `as_int32` / `isAnyInt` over `src/` found no other user-reachable site that decides by representation. The remaining tag checks are fast paths with a matching double path, or read values that internal code builds (`JSBundler.rs` loader ids, `#internalFlags` in `bun:sqlite`, subprocess exit codes, `$lazy` ids). Suites run against the debug build: `vm.test.ts` (277 pass), `setTimeout.test.js`, `setInterval.test.js`, `bun/util/inspect.test.js`, `console/bun-inspect-table.test.ts`, `console/console-table.test.ts`, `node/util/bun-inspect.test.ts`, and Node's `test-timers-clear-null-does-not-throw-error`, `test-timers-clear-object-does-not-throw-error`, `test-timers-clear-timeout-interval-equivalent`, `test-timers-invalid-clear`, `test-timers-to-primitive`, `test-timers-destroyed`, `test-timers-timeout-to-interval`. The two RSS leak fixtures in `test/js/web/timers/` fail against a debug build with or without this change: they detect ASAN by the binary name (`bun-asan`), and their protected-Timeout checks pass. Reported separately. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/vm/vm.test.ts test/js/web/timers/setTimeout.test.js <!-- robobun:evidence:end -->
2 participants
Problem
JSValue.JSON.parse("[1e10, 20]")[1], an element of any array that also holds a non-int32 number, or a value read out of aFloat64Arrayis stored as a double. Fournode:cryptoentry points decide by that representation. Node accepts every case below (checked against v26.3.0). Bun 1.3.14 behaves like main, so this is not a regression.crypto.sign(),crypto.verify(),Sign#sign()andVerify#verify()reject such apaddingorsaltLengthwithERR_INVALID_ARG_VALUE: The property 'options.saltLength' is invalid. Received 20. They also rejectsaltLength: -0. Cause:getIntOption()insrc/jsc/bindings/node/crypto/CryptoUtil.cpp:523testsisInt32().generateKeySync("aes", { length })rejects a double-encoded 128 or 256 for the same reason inV::validateOneOf()(NodeValidator.cpp:586).createDiffieHellman(prime, generator)andgetCipherInfo(nid)validate the number and then read it withasInt32()(JSDiffieHellmanConstructor.cpp:138,node_crypto_binding.cpp:229). A debug build failsASSERTION FAILED: isInt32(). A release build reads 0, so the generator is reported asBAD_GENERATORand the cipher comes backundefined.Fix
getIntOption()does what Node'svalue === value >> 0does: a Number whose value survivesToInt32unchanged is accepted, whatever its representation, so-0becomes 0. Fractions, values outside int32 and non-numbers are still rejected with the same error.validateOneOf()compares the numeric value against the list, which is what Node'sArray#includesdoes. Non-numbers still fail the same way.getCipherInfo()take the value from thevalidateInt32()call they already make, through its out parameter, which handles both representations. The constructor also loses its second, identical validation of the same argument.test/js/node/crypto/node-crypto.test.js("integer arguments whose JSValue is a double"): all four tests fail on main. The rest of that file (212 tests),crypto.key-objects.test.ts,crypto-rsa.test.js,crypto-oneshot.test.tsand Node'stest-crypto-secret-keygen,test-crypto-dh*,test-crypto-getcipherinfoandtest-crypto-sign-verifypass.. Received xsuffix to thevalidateOneOf()message. This PR does not touch that message. The test here matches the message prefix, so it holds either way.Background
JSC stores a Number either as an int32 or as a double inside the
JSValue. Arithmetic and literals usually produce the int32 form, which is whysaltLength: 20works and the bug looks intermittent from the outside. Arrays switch to double storage as soon as one element does not fit an int32, and typed arrays always produce doubles, so the same value 20 can arrive in either form.JSValue::isInt32()andasInt32()ask about the storage, not the value.asNumber()and thevalidateInt32()out parameter give the value in both cases. The other integer options innode:crypto(pbkdf2, scrypt, hkdf, key generation, cipher options) already go through the validators, so they are not affected.