Conversation
|
Updated 1:08 PM PT - Jul 16th, 2026
❌ @robobun, your commit 0a2a2c6 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33522That installs a local version of the PR into your bun-33522 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Thanks for flagging it. There is a real overlap with #31708, but the two PRs are not the same change, so I'd rather explain it than close this. What overlaps: #31708 converts the What does not: #31708 doesn't touch The overlap isn't separable from the rest, either. Once Two things this PR covers that #31708 leaves open:
For what it's worth, #31708 currently conflicts with main (it predates #33202, which rewrote the generator handling in that file), so it needs a rebase regardless. If a maintainer would rather land #31708 first, I'm happy to rebase this and drop the overlapping hunks. Credit to @saklani for spotting the throw-vs-return bug independently. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
WalkthroughDiffie-Hellman generation now enforces a 512-bit minimum. Parameter validation uses OpenSSL-compatible BN checks for q-less groups, constructor failures throw coded errors, and crypto tests cover validation flags, exceptions, and compatible modulus sizes. ChangesDiffie-Hellman validation and construction
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Good catch, and it was a real failure, not a theoretical one. Both files construct a 256-bit group and both threw My original sweep for affected tests piped One correction to the report: Two things that fell out of re-verifying this:
Dropped a Fail-before on |
There was a problem hiding this comment.
Thanks for patching the two remaining vendored tests — that resolves my earlier comment. No further issues found, but leaving this for a human since it hand-reimplements OpenSSL 3's DH_check() over BoringSSL primitives.
Extended reasoning...
Overview
This PR reimplements DHPointer::check() in ncrypto.cpp to mirror OpenSSL 3's DH_check()/DH_check_params() semantics for the no-q case (rather than delegating to BoringSSL's outdated OpenSSL 1.0.x heuristics), adds a 512-bit minimum-modulus guard to DHPointer::New(bits, generator), adds MODULUS_TOO_SMALL = 0x80 to the CheckResult enum, and fixes seven error paths in constructDiffieHellman() that were returning error objects instead of throwing them (plus one missing return {} after throwCryptoError). Six vendored Node tests are patched from 256→512 bits, and ~90 lines of new tests are added to node-crypto.test.js.
Since my previous review, the author added the two missing test patches (test-crypto-dh-generate-keys.js and test-crypto-classes.js) that I flagged — those are now in the diff and look correct.
Security risks
This is DH parameter-validation code. The net effect is a tightening: sub-512-bit prime generation is now rejected, and verifyError now reports DH_MODULUS_TOO_SMALL where it previously reported 0 for tiny safe primes. The one loosening — no longer flagging RFC 3526 groups as DH_NOT_SUITABLE_GENERATOR — is the correct behavior and matches Node/OpenSSL 3. verifyError is diagnostic (it doesn't gate the key exchange itself), so a bug here would be a false-negative diagnostic rather than a direct key-agreement flaw. I see no injection, auth-bypass, or data-exposure vectors.
Level of scrutiny
High. This is ~50 lines of hand-written BIGNUM logic in a crypto path, ported from OpenSSL 3's crypto/dh/dh_check.c. The port looks faithful (range-check g, check BN_num_bits(p) < 512, Miller-Rabin on p and (p-1)/2), the error-return paths are consistent, and the differential matrix in the PR description is unusually thorough. But per policy I don't auto-approve crypto reimplementations — a human should confirm the OpenSSL 3 port is complete and that the q != nullptr delegation to BoringSSL's DH_check() is still sound.
Other factors
- The
createError→Bun::throwErrorfixes inJSDiffieHellmanConstructor.cppare mechanical and clearly correct; the addedreturn {}afterthrowCryptoErrorfixes a real fall-through-with-pending-exception bug. - Test coverage is strong: exact
verifyErrorbit-values asserted against Node, plus explicit throw-vs-return control-flow tests that would catch a regression of thecreateErrorbug. - No outstanding reviewer comments remain.
|
Confirmed, and it was a live break rather than a hypothetical one. CI on the previous sha had exactly those two as its only
Both pass now, and the build on The one decision worth a human's time
That last row is why I didn't take the zero-diff option. Dropping All six edits raise a DH size, so the tests get stricter rather than weaker, and every vendored test that mentions DiffieHellman (15 files) passes. The honest tension on the floorNode linked against BoringSSL does not have it. Upstream's own I think matching the shipped Node is right here, and the reported If you'd rather keep the floor out, say so and I'll drop it. That reduces the vendored churn to a single file ( |
Current state0a2a2c6 (rebased onto current main). No edits under
CIBuild 74010 (this sha): 283 passed, 3 failed. None touch crypto, DiffieHellman, verifyError, modp, or ncrypto.
I've already used my one CI re-roll, so I'm not pushing another retrigger. The diff is green; the change is ready for a maintainer. |
|
Added two even-modulus Both rows verified against node v26.3.0. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/crypto/node-crypto.test.js`:
- Around line 649-651: Replace the parameterized it.each() blocks in the
affected crypto tests with describe.each() tables, placing each assertion inside
an inner it(). Apply this consistently to the cases around the “well-known
group” test and the additional referenced parameterized ranges, preserving their
existing test names, inputs, and assertions.
In `@test/js/node/test/parallel/test-crypto-classes.js`:
- Around line 25-27: Restore the upstream Node v26.3.0 Diffie-Hellman inputs in
test/js/node/test/parallel/test-crypto-classes.js:25-27,
test/js/node/test/parallel/test-crypto-dh-constructor.js:10-12,
test/js/node/test/parallel/test-crypto-dh-generate-keys.js:12-14,
test/js/node/test/parallel/test-crypto-dh-leak.js:16-18,
test/js/node/test/parallel/test-crypto-dh-odd-key.js:35-37, and
test/js/node/test/parallel/test-crypto-dh.js:15-17. Record the unsupported
512-bit cases in test/expectations.txt and keep Bun-specific behavioral coverage
in test/js/node/crypto/node-crypto.test.js; do not modify the vendored tests
further.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a8164de-3ba1-4f39-ad0f-68c4efa6a265
📒 Files selected for processing (10)
src/jsc/bindings/ncrypto.cppsrc/jsc/bindings/ncrypto.hsrc/jsc/bindings/node/crypto/JSDiffieHellmanConstructor.cpptest/js/node/crypto/node-crypto.test.jstest/js/node/test/parallel/test-crypto-classes.jstest/js/node/test/parallel/test-crypto-dh-constructor.jstest/js/node/test/parallel/test-crypto-dh-generate-keys.jstest/js/node/test/parallel/test-crypto-dh-leak.jstest/js/node/test/parallel/test-crypto-dh-odd-key.jstest/js/node/test/parallel/test-crypto-dh.js
There was a problem hiding this comment.
No issues found on this pass, but deferring to a human — this reimplements DH_check() semantics in crypto code, edits six vendored Node tests, and the author has an open policy question (keep the 512-bit DH_MIN_MODULUS_BITS floor or drop it) that a maintainer should answer.
What was reviewed:
- The rewritten
DHPointer::check()against OpenSSL 3'sDH_check_params()/DH_check()— flag values, theq != nullptrfallback to BoringSSL, and theOPENSSL_DH_MAX_MODULUS_BITSbound on the primality tests. - The
createError→throwErrorconversions and the addedreturn {}afterthrowCryptoErrorin thesetWordpath. - The earlier finding (two more vendored tests selecting 256-bit groups) is fixed in cc2f248; a re-sweep for the pattern found no remaining live instances.
Extended reasoning...
Overview
The PR rewrites DHPointer::check() in src/jsc/bindings/ncrypto.cpp to mirror OpenSSL 3's DH_check()/DH_check_params() instead of delegating to BoringSSL's, which still applies OpenSSL 1.0.x generator heuristics that flag every RFC 3526 group as DH_NOT_SUITABLE_GENERATOR. It also adds a 512-bit floor to DHPointer::New(bits, generator), adds MODULUS_TOO_SMALL = 0x80 to the CheckResult enum for BoringSSL builds, converts seven error paths in constructDiffieHellman() from return-error-object to throw, and adds a missing return {} after a throwCryptoError. Six vendored Node tests are bumped from 256/32-bit to 512-bit DH sizes, and ~110 lines of new tests land in node-crypto.test.js.
Security risks
This is node:crypto DH parameter validation. The change tightens behavior (rejects sub-512-bit generation, reports DH_MODULUS_TOO_SMALL on small primes that previously reported 0) and does not weaken any existing check. The hand-written primality/range logic mirrors a well-known reference (crypto/dh/dh_check.c), uses BoringSSL's BN_is_prime_ex with BN_prime_checks_for_validation, and bounds work at OPENSSL_DH_MAX_MODULUS_BITS. I did not find a path where the new code accepts something the old code rejected for a security-relevant reason — but reimplementing a libcrypto check function is exactly the class of change that warrants a maintainer's eyes rather than a bot approval.
Level of scrutiny
High. This is production crypto code in C++ bindings, it hand-rolls logic that was previously delegated to the vendored TLS library, and it edits six files under test/js/node/test/parallel/ which the directory's CLAUDE.md marks as do-not-modify. The author acknowledged crossing that line deliberately and laid out the tradeoff.
Other factors
- The author explicitly left a policy question open for a maintainer: whether to keep the 512-bit generation floor (matching shipped Node/OpenSSL 3) or drop it (matching Node-on-BoringSSL, and reducing vendored-test churn to one file). That decision hasn't been answered and shouldn't be made by automated review.
- My earlier inline finding (two additional vendored tests with the
hasOpenSSL3 ? 1024 : 256pattern) was fixed in cc2f248 and is resolved. This run's sweep found no further live instances;test-crypto-secure-heap.jshas the pattern but its body is commented out. - Test coverage is thorough and each case was diffed against Node v26.3.0; CI on the current head is reported green modulo unrelated macOS infra flake.
- There is a partial overlap with #31708 (the throw-vs-return fixes); the author has explained the relationship and offered to rebase if that lands first.
BoringSSL's DH_check() still implements OpenSSL 1.0.x's generator
heuristics: generator 2 requires p % 24 == 11, generator 5 requires
p % 10 in {3, 7}, and anything else yields DH_UNABLE_TO_CHECK_GENERATOR.
Every RFC 3526 safe prime is 23 mod 24, so DiffieHellman.verifyError
reported DH_NOT_SUITABLE_GENERATOR for getDiffieHellman("modp14") and
friends, and the documented `if (dh.verifyError) throw` guard rejected
every standard IETF group.
OpenSSL 3 dropped those heuristics. When the group has no q, DH_check()
and DH_check_params() only range-check the generator (g <= 1 or
g >= p - 1) and flag the modulus size, so port that logic into
DHPointer::check() instead of delegating the no-q case to BoringSSL.
The modulus-size flag matters on its own: without it, a five-bit safe
prime reports 0. It also closes false negatives that predate this
change, where a sub-512-bit safe prime with p % 24 == 11 already
reported 0.
Two related fixes in the same surface:
DHPointer::New(bits, generator) now enforces OpenSSL's
DH_MIN_MODULUS_BITS. BoringSSL's DH_generate_parameters_ex() accepts
any prime length above zero, so createDiffieHellman(32) used to hand
back a 32-bit group and derive secrets from it.
constructDiffieHellman() built its errors with createError() and
returned them, so createDiffieHellman(2) evaluated to a TypeError
instance rather than throwing one, and a caller's `if (dh.verifyError)`
read undefined. Throw them instead, and return after the bad-generator
path's throwCryptoError() rather than falling through with a pending
exception.
Four vendored Node tests picked sub-512-bit DH sizes; upstream only does
that for speed when libcrypto permits it.
test-crypto-dh-generate-keys.js and test-crypto-classes.js pick the same hasOpenSSL3 ? 1024 : 256 size as the four already adjusted, so the DH_MIN_MODULUS_BITS floor rejects them too. Also assert the control flow of the constructor's error paths directly. expect().toThrow() treats an Error handed back by the callee as if it had been thrown, so it cannot tell a returned error object from a raised one, which is the whole of what those paths changed.
BoringSSL's DH_check() calls dh_check_params_fast() first, which returns 0 for any modulus with !BN_is_odd(p). The getter then threw ERR_CRYPTO_OPERATION_FAILED instead of reporting DH_CHECK_P_NOT_PRIME, which is what OpenSSL 3 (and Node) do. The rewritten check() already handles this; these tests pin the behavior.
The 512-bit minimum on DHPointer::New(bits, generator) matched the Node binary people download, but not Node built against the same libcrypto (BoringSSL has no DH_MIN_MODULUS_BITS). #32623 re-syncs the vendored crypto suite to Node v26.3.0 and keeps the 256-bit size on that branch, so align with that: createDiffieHellman(32) succeeds again, and the small group is detectable by verifyError reporting DH_MODULUS_TOO_SMALL. With the floor gone, the six vendored tests that pick a sub-512-bit size pass as-is. DH_MODULUS_TOO_SMALL still collides with test-crypto-dh.js, which asserts verifyError === 0 on a 256-bit group it generates, so quarantine that one in test/expectations.txt rather than edit it.
#33952 rewrote the expectations.txt policy: quarantining drops the whole file from the run, so the entry must either name a file that cannot run at all or carry its coverage elsewhere. test-crypto-dh.js can run; it fails on one verifyError === 0 assert that cannot be skipped without editing a verbatim upstream mirror. Port its two-party exchange and setPublicKey/setPrivateKey round-trip into node-crypto.test.js so the quarantine does not silently drop that coverage, and reword the entry to spell that out.
There was a problem hiding this comment.
I didn't find bugs in the current revision, but this hand-ports OpenSSL 3's DH_check()/DH_check_params() into ncrypto.cpp and quarantines a previously-passing vendored Node test — worth a human look at the crypto validation logic and the expectations.txt tradeoff.
Checked: the ported check() mirrors OpenSSL 3's no-q path (range-check g, DH_MODULUS_TOO_SMALL, prime + safe-prime tests); the q != nullptr branch still delegates to BoringSSL. The createError→throwError conversions in the constructor now cover every error path including the bn_g.setWord fall-through. My earlier finding (two missed vendored tests) was resolved and then mooted by 0cf9395, which reverted all vendored-test edits and dropped the 512-bit generation floor from DHPointer::New — note the PR description still describes the floor and the createDiffieHellman(2..511) → ERR_INVALID_ARG_VALUE behavior, which no longer applies.
Extended reasoning...
Overview
The PR rewrites DHPointer::check() in src/jsc/bindings/ncrypto.cpp to reimplement OpenSSL 3's DH_check()/DH_check_params() for the no-q case rather than delegating to BoringSSL (whose DH_check retains OpenSSL 1.0.x generator heuristics that flag every RFC 3526 group). It adds a MODULUS_TOO_SMALL = 0x80 enum value in ncrypto.h, converts seven return JSValue::encode(createError(...)) sites in JSDiffieHellmanConstructor.cpp to Bun::throwError(...) (plus a missing return {} after throwCryptoError), quarantines test-crypto-dh.js in test/expectations.txt, and adds ~100 lines of tests to node-crypto.test.js. After 0cf9395 the six vendored-test edits and the DHPointer::New(bits, ...) 512-bit floor were dropped; the diff now touches five files and leaves test/js/node/test/parallel/ untouched.
Security risks
This is DH parameter validation — the mechanism that tells callers whether a group is safe to key off. The rewrite is a faithful-looking port of OpenSSL 3's logic (checked against the description of crypto/dh/dh_check.c): g range-checked against [2, p-1), BN_num_bits(p) < 512 → DH_MODULUS_TOO_SMALL, BN_is_prime_ex on p and (p-1)/2, plus an OPENSSL_DH_MAX_MODULUS_BITS early-out to bound the primality tests. All BN allocations are RAII-owned (BignumCtxPointer, BignumPointer) and every fallible BN call returns CHECK_FAILED. The change makes verifyError strictly less permissive on small moduli (it now sets DH_MODULUS_TOO_SMALL where BoringSSL reported 0 for e.g. a 5-bit safe prime with p % 10 ∈ {3,7}), so it does not weaken validation. That said, this is hand-written crypto validation replacing a vendored library call, and the author diffed it against Node output rather than OpenSSL source line-by-line — a maintainer should confirm the port is complete.
Level of scrutiny
High. src/runtime/crypto / ncrypto.cpp is production crypto, and "Never hand-roll security-sensitive parsing — use the hardened in-tree library" from the review guidelines applies directly to swapping a BoringSSL call for an in-tree reimplementation. The change is well-motivated (the reported bug is real and user-visible: if (dh.verifyError) throw rejects every standard IETF group), thoroughly tested (24 new assertions, byte-for-byte diffed against Node v26.3.0, fail-before verified), and the author has been unusually careful (caught the toThrow()-accepts-returned-Error footgun, bounded the primality tests, handled the even-modulus case). But it is not a simple/mechanical change.
Other factors
- The PR quarantines
test/js/node/test/parallel/test-crypto-dh.jsinexpectations.txt. The comment is thorough and points at #32623 for the re-sync, but marking a previously-passing vendored test[ FAIL ]is a tradeoff a maintainer should sign off on. - The PR description is stale after 0cf9395: it still claims
DHPointer::New(bits, generator)enforcesDH_MIN_MODULUS_BITSand thatcreateDiffieHellman(2..511)throwsERR_INVALID_ARG_VALUE. The current diff does not add that check —kDhMinModulusBitsis used only incheck(). Not a code bug, but a reviewer reading the description first will be misled. - Overlaps with open PR #31708 on the throw-vs-return conversions (author acknowledged and explained why the overlap isn't separable).
- CodeRabbit's
describe.each()style suggestion is noise; the vendored-test-preservation comment was addressed in 0cf9395. - My earlier inline finding (two missed
hasOpenSSL3 ? 1024 : 256vendored tests) was fixed in cc2f248 and then made moot by 0cf9395's revert.
0cf9395 to
0a2a2c6
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this reimplements DH_check() semantics in the crypto layer and quarantines a vendored Node test — worth a human look before merging.
What was reviewed:
- The no-
qpath inDHPointer::check()against OpenSSL 3'sDH_check/DH_check_params: the range check ong,DH_MODULUS_TOO_SMALLat 512 bits, the two Miller-Rabin passes, and theOPENSSL_DH_MAX_MODULUS_BITScap all line up; RAII (BignumCtxPointer,BignumPointer,ClearErrorOnReturn) covers cleanup on every early return. - The
q != nullptrbranch still delegates to BoringSSL'sDH_check(), whose q-path matches OpenSSL's — no behaviour change there. MODULUS_TOO_SMALL = 0x80does not collide withCHECK_FAILED = 512or any BoringSSLDH_CHECK_*flag.- The
createError→Bun::throwErrorconversions and the addedreturn {}afterthrowCryptoError— no remaining fall-through with a pending exception inconstructDiffieHellman.
Extended reasoning...
Overview
Five files. ncrypto.cpp rewrites DHPointer::check() for the no-q case to mirror OpenSSL 3's DH_check()/DH_check_params() (range-check g, flag DH_MODULUS_TOO_SMALL below 512 bits, run Miller-Rabin on p and (p-1)/2) instead of delegating to BoringSSL's OpenSSL-1.0.x-era heuristics. ncrypto.h adds the MODULUS_TOO_SMALL enum value (0x80 under BoringSSL). JSDiffieHellmanConstructor.cpp converts seven return JSValue::encode(createError(...)) sites to Bun::throwError(...) and adds a missing return {} after a throwCryptoError fall-through. test/expectations.txt quarantines test-crypto-dh.js (asserts verifyError === 0 on a 256-bit group, which now reports 128). node-crypto.test.js adds ~140 lines of coverage for the new verifyError semantics, the throw-vs-return fix, and carries the quarantined file's two-party-exchange coverage.
Security risks
This is a rewrite of Diffie-Hellman parameter validation reporting. It does not weaken any check that existed before — the effect is to stop falsely flagging RFC 3526 groups and to start flagging sub-512-bit primes via DH_MODULUS_TOO_SMALL, which is a strict tightening. The OPENSSL_DH_MAX_MODULUS_BITS guard bounds the primality-test work before it runs, so an adversarial modulus cannot spin unbounded CPU beyond what BoringSSL's own DH_check() already permitted. Nothing here touches key generation, secret computation, or the TLS stack. Still, it is hand-ported crypto logic against a spec (OpenSSL 3 source), and the author explicitly frames one behavioural choice (matching OpenSSL-3-linked Node rather than BoringSSL-linked Node) as a policy call.
Level of scrutiny
High. node:crypto parameter validation is security-sensitive, the change reimplements an OpenSSL function's semantics manually rather than calling into a vetted library path, and it makes two decisions a maintainer should ratify: (1) diverging from BoringSSL's DH_check() for the no-q case, and (2) quarantining test/js/node/test/parallel/test-crypto-dh.js in expectations.txt (which drops that whole file from the run, not just the one assert). Both are well-argued in the PR body and thread, but they are exactly the class of judgment calls the approval guidelines reserve for a human.
Other factors
The PR has been through several rounds: my earlier finding (two more vendored tests hitting the since-removed 512-bit generation floor) was addressed, then the approach was reworked to leave all vendored tests untouched and drop the floor entirely. Both CodeRabbit threads are resolved. Test coverage is thorough (well-known groups, generator range, small/composite/even/non-safe primes, and explicit control-flow assertions for the throw-vs-return fix that avoid toThrow()'s returned-Error leniency). There is a known overlap with open PR #31708 (throw-vs-return only) and an acknowledged interaction with #32623's test re-sync — both are called out in the description and neither blocks this change on its own, but a maintainer should be aware when sequencing merges.
…llman (#36508) Adopts #31708 by @saklani. ## Repro ```js const crypto = require("crypto"); try { const r = crypto.createDiffieHellman(2); console.log("returned:", r instanceof Error, r.code); // returned: true ERR_INVALID_ARG_VALUE } catch (e) { console.log("threw:", e.code); } ``` Bun prints `returned: true ERR_INVALID_ARG_VALUE`; Node throws `ERR_INVALID_ARG_VALUE`. ## Cause Seven error paths in `constructDiffieHellman()` did `return JSValue::encode(createError(...))`, which returns an `Error` instance from the constructor rather than throwing it. A `[[Construct]]` that returns an object makes that object the result of the `new`-expression, so `createDiffieHellman(2)` evaluated to a `TypeError` instead of raising one. The `bn_g.setWord()` failure path also called `throwCryptoError()` without returning, falling through into `DHPointer::New()` with a pending exception. ## Fix Mechanical swap to `throwError(globalObject, scope, ...); return {};` at each site, matching the pattern used in the neighbouring ECDH bindings. ## Verification ``` DiffieHellman > throws (not returns) validation errors from the constructor (pass) ``` New test fails on the released binary (`threw: false`), passes on this branch. `test/js/node/crypto/node-crypto.test.js` is 203/203, and the vendored `test-crypto-dh-constructor.js` / `test-crypto-dh-errors.js` still pass. The test captures control flow explicitly rather than using `toThrow()`, because `toThrow()` accepts a *returned* `Error` instance as a throw and cannot distinguish the two cases here. ## Related Overlaps with #33522, which fixes the same constructor paths as part of a larger `DH_check()` / `verifyError` change. Whichever lands first, the other rebases trivially. <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 1 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/crypto/node-crypto.test.js bun test v1.4.0 (f250508) test/js/node/crypto/node-crypto.test.js: (pass) crypto.randomBytes should return a Buffer [4.90ms] (pass) crypto.randomInt should return a number [2.39ms] (pass) crypto.randomInt with no arguments [4.24ms] (pass) crypto.randomInt with one argument [2.61ms] (pass) crypto.randomInt with a callback [40.92ms] (pass) createHash > rsa-md5 - "Hello World" [8.82ms] (pass) createHash > rsa-md5 - "Hello World" -> binary [4.94ms] (pass) createHash > rsa-ripemd160 - "Hello World" [1.07ms] (pass) createHash > rsa-ripemd160 - "Hello World" -> binary [1.05ms] (pass) createHash > rsa-sha1 - "Hello World" [9.46ms] (pass) createHash > rsa-sha1 - "Hello World" -> binary [1.89ms] (pass) createHash > rsa-sha1-2 - "Hello World" [0.84ms] (pass) createHash > rsa-sha1-2 - "Hello World" -> binary [0.95ms] (pass) createHash > rsa-sha224 - "Hello World" [3.12ms] (pass) createHash > rsa-sha224 - "Hello World" -> binary [0.89ms] (pass) createHash > rsa-sha256 - "Hello World" [1.95ms] (pass) create ... (truncated) release without fix: 13 FAILED bun test v1.4.0-canary.1 (1498d7b) test/js/node/crypto/node-crypto.test.js: (pass) crypto.randomBytes should return a Buffer [0.14ms] (pass) crypto.randomInt should return a number [0.04ms] (pass) crypto.randomInt with no arguments [0.13ms] (pass) crypto.randomInt with one argument [0.07ms] (pass) crypto.randomInt with a callback [0.75ms] (pass) createHash > rsa-md5 - "Hello World" [0.20ms] (pass) createHash > rsa-md5 - "Hello World" -> binary [0.11ms] (pass) createHash > rsa-ripemd160 - "Hello World" [0.02ms] (pass) createHash > rsa-ripemd160 - "Hello World" -> binary [0.03ms] (pass) createHash > rsa-sha1 - "Hello World" [0.16ms] (pass) createHash > rsa-sha1 - "Hello World" -> binary [0.08ms] (pass) createHash > rsa-sha1-2 - "Hello World" [0.01ms] (pass) createHash > rsa-sha1-2 - "Hello World" -> binary [0.01ms] (pass) createHash > rsa-sha224 - "Hello World" [0.05ms] (pass) createHash > rsa-sha224 - "Hello World" -> binary [0.01ms] (pass) createHash > rsa-sha256 - "Hello World" [0.02ms] (pass) createHash > rsa-sha256 - "Hello World" -> binary [0.01ms] (pass) createHash > rsa-sha3-224 - "Hello World" (pass) createHash > rsa-sha3-224 - "Hello World" -> binary (pas ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/crypto/node-crypto.test.js bun test v1.4.0 (f250508) test/js/node/crypto/node-crypto.test.js: (pass) crypto.randomBytes should return a Buffer [5.50ms] (pass) crypto.randomInt should return a number [2.71ms] (pass) crypto.randomInt with no arguments [4.92ms] (pass) crypto.randomInt with one argument [2.47ms] (pass) crypto.randomInt with a callback [42.07ms] (pass) createHash > rsa-md5 - "Hello World" [8.87ms] (pass) createHash > rsa-md5 - "Hello World" -> binary [5.58ms] (pass) createHash > rsa-ripemd160 - "Hello World" [1.13ms] (pass) createHash > rsa-ripemd160 - "Hello World" -> binary [1.49ms] (pass) createHash > rsa-sha1 - "Hello World" [9.81ms] (pass) createHash > rsa-sha1 - "Hello World" -> binary [1.84ms] (pass) createHash > rsa-sha1-2 - "Hello World" [0.91ms] (pass) createHash > rsa-sha1-2 - "Hello World" -> binary [0.92ms] (pass) createHash > rsa-sha224 - "Hello World" [3.03ms] (pass) createHash > rsa-sha224 - "Hello World" -> binary [0.99ms] (pass) createHash > rsa-sha256 - "Hello World" [1.97ms] (pass) create ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 824ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/8] cc obj/packages/bun-usockets/src/context.c.o [2/8] gen cpp.rs (cppbind) [3/8] gen generated_host_exports.rs generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 239 extern-C blocks audited [3/8] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_http_jsc v0.0.0 (/workspace/bun/src/http_jsc) �[1m�[92m Compiling�[0m bun_runtime v0.0.0 (/workspace/bun/src/runtime) �[1m�[92m Compiling�[0m bun_bin v0.0.0 (/workspace/bun/src/bun_bin) �[1m�[92m Finished�[0m `release` profile [optimized + debuginfo] target(s) in 5m 08s [4/8] cxx obj/unified/UnifiedSource-src_jsc_bindings_node_crypto-0.cpp.o [5/8] link bun-profile [7/8] strip bun [7/8] bun-profile --revision 1.4.0-canary.1+f250508a3 [build] done bun test v1.4.0-canary.1 (f250508) test/js/node/crypto/node-crypto.test.js: (pass) crypto.randomBytes should return a Buffer [0.10ms] ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` .../node/crypto/JSDiffieHellmanConstructor.cpp | 22 +++++++++++------ test/js/node/crypto/node-crypto.test.js | 28 ++++++++++++++++++++++ 2 files changed, 43 insertions(+), 7 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests …jsc/bindings/node/crypto/JSDiffieHellmanConstructor.cpp 1 2 0 test/js/node/crypto/node-crypto.test.js 2 2 0 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: saklani <50768838+saklani@users.noreply.github.com>
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-16, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
DiffieHellman.verifyErrorreportsDH_NOT_SUITABLE_GENERATOR(8) for every RFC 3526 well-known group, where Node reports 0. The documented hardening patternif (dh.verifyError) throwtherefore rejects every standard IETF group.Repro
Cause
Bun vendors BoringSSL, whose
DH_check()still implements OpenSSL 1.0.x's generator heuristics (vendor/boringssl/crypto/fipsmodule/dh/check.cc.inc:158):Every RFC 3526 safe prime is 23 mod 24, so generator 2 always trips the first branch.
OpenSSL 3, which Node links against, removed these heuristics rather than extending them. In
crypto/dh/dh_check.cthe generator is only tested againstqwhen aqis present; with noq,DH_check_params()does nothing but range-checkg(g <= 1org >= p - 1→DH_NOT_SUITABLE_GENERATOR) and flag the modulus size (DH_MODULUS_TOO_SMALLbelowDH_MIN_MODULUS_BITS= 512).DH_UNABLE_TO_CHECK_GENERATORis never set.Confirming that this, and not a widened
p % 24rule, is the mechanism: Node also reports 0 for freshly generated random safe primes withp % 24 == 23that are not any well-known group, and for generators 3 and 7 on modp14.Fix
DHPointer::check()now mirrors OpenSSL 3'sDH_check()/DH_check_params()for the no-qcase instead of delegating it to BoringSSL.DH_MODULUS_TOO_SMALLis load-bearing, not cosmetic. Dropping only the generator heuristic would make a five-bit safe prime report 0. Keeping it also closes false negatives that predate this change:createDiffieHellman(Buffer.from([23]), Buffer.from([5]))and a 511-bit safe prime withp % 24 == 11both reported 0 before, because the old heuristic keyed on the residue class rather than the size.createDiffieHellman(32)still succeeds (BoringSSL has no generation floor and #32623's test sync expects that), but itsverifyErrornow reportsDH_MODULUS_TOO_SMALLinstead of 0, so the documented guard catches it.While reproducing, the error paths in
constructDiffieHellman()turned out to build their errors withcreateError()and return them, socreateDiffieHellman(512, Buffer.from([2]))evaluated to aTypeErrorinstance rather than throwing one, andif (dh.verifyError) throwthen readundefinedon it. The bad-generator path also calledthrowCryptoError()without returning, falling through intoDHPointer::New()with a pending exception. Both are fixed here.No files under
test/js/node/test/parallel/are modified.DH_MODULUS_TOO_SMALLmakestest-crypto-dh.jsassertverifyError === 0on the 256-bit group it generates, which is now 128; that one file is quarantined intest/expectations.txt. Per the policy #33952 put at the top of that file, its two-party exchange andsetPublicKey/setPrivateKeyround-trip coverage is carried intonode-crypto.test.jsrather than lost. The other 14 vendored tests that mention DiffieHellman pass unchanged, and #32623's re-sync of the file to Node v26.3.0 will need to reconcile the same divergence.Verification
Every case below was diffed against node v26.3.0 and is now identical.
Full differential matrix
bun bd test test/js/node/crypto/, 918 pass, 0 fail.test-crypto-dh.jsis the quarantined one.node-crypto.test.jsfail 16/20 without thesrc/change.verifyErroron the larger groups is left out of the new tests: modp16 and up spend 4-30s running 64-round Miller-Rabin twice over the modulus, and they exercise the same code path as modp5/modp14. That cost is unchanged, BoringSSL'sDH_check()ran the same two primality tests.One note for whoever reviews the tests:
expect().toThrow()accepts an Error that the callee returned as though it had been thrown, so it cannot distinguish the two. The assertions covering the throw-vs-return paths check the control flow directly instead.Relationship to #32623
#32623 (open) caches
verifyErrorat construction time and re-syncs the vendored crypto tests to Node v26.3.0. It does not change whatDHPointer::check()returns, so the two are orthogonal and should merge cleanly in either order. After both land,test-crypto-dh.jswill still need theverifyError === 0/ 256-bit interaction addressed in the re-synced file.[review] gate passed · iteration 4 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 4
evidence per changed file