node:crypto: reject RSA modulusLength that BoringSSL would round down

[robobun]

Fixes #5740.

Repro

import crypto from 'node:crypto';
for (const req of [1023, 3000, 4095, 2048]) {
  const { publicKey } = crypto.generateKeyPairSync('rsa', { modulusLength: req });
  console.log(`requested=${req} got=${publicKey.asymmetricKeyDetails.modulusLength}`);
}

On main this silently returns a weaker key than requested:

requested=1023 got=896
requested=3000 got=2944
requested=4095 got=3968
requested=2048 got=2048

Node v26 (OpenSSL) honors 1023, 3000, and 4095 exactly.

Cause

RsaKeyPairJobCtx::fromJS passes modulusLength straight to EVP_PKEY_CTX_set_rsa_keygen_bits, and BoringSSL's RSA_generate_key_ex masks it with bits &= ~127 (vendor/boringssl/crypto/fipsmodule/rsa/rsa_impl.cc.inc:736). Nothing on the way in checked for that rounding and nothing on the way out compared the generated modulus against the request.

This is the node:crypto sibling of #33477; that PR fixes only the WebCrypto path (CryptoKeyRSA::generatePair), which does not reach generateKeyPair.

Fix

Reject a modulusLength that is not a multiple of 128 with ERR_OUT_OF_RANGE in RsaKeyPairJobCtx::fromJS, before the key is generated. The check sits ahead of the rsa/rsa-pss branch so it covers both variants. Sizes BoringSSL generates exactly (512, 1024, 2048, 3072, 4096, ...) are unchanged.

After:

RangeError: The value of "options.modulusLength" is out of range. It must be a multiple of 128. Received 1023
 code: "ERR_OUT_OF_RANGE"

Node's own test-crypto-keygen-bit-length.js already skips on process.features.openssl_is_boringssl for this exact limitation, so no upstream test is affected.

Verification

New tests in test/js/node/crypto/crypto.key-objects.test.ts cover both generateKeyPairSync and the async generateKeyPair.

test output

$ USE_SYSTEM_BUN=1 bun test test/js/node/crypto/crypto.key-objects.test.ts -t "never silently"
  {
-   "1023": { "code": "ERR_OUT_OF_RANGE" },
+   "1023": 896,
    "1024": 1024,
-   "3000": { "code": "ERR_OUT_OF_RANGE" },
+   "3000": 2944,
  }
 0 pass  2 fail

$ bun bd test test/js/node/crypto/crypto.key-objects.test.ts
 87 pass  0 fail

test-crypto-keygen-async-rsa.js, test-crypto-keygen-sync.js, and test-crypto-keygen-bit-length.js still pass.

---

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/crypto/crypto.key-objects.test.ts

[robobun]

Status: fix applied and verified, waiting on maintainer.

Reproduced with: USE_SYSTEM_BUN=1 bun test test/js/node/crypto/crypto.key-objects.test.ts -t "never silently" (1023→896, 3000→2944).

Passes with: bun bd test test/js/node/crypto/crypto.key-objects.test.ts (87 pass, 0 fail).

CI (build 83325): the only red lane is test/js/bun/http/serve.test.ts on darwin (request body backpressure > releases a paused request body...), which has been red on main since #36072 landed and is being fixed in #36088. All other failures passed on retry. This diff touches only CryptoGenRsaKeyPair.cpp and crypto.key-objects.test.ts.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 1 minute

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7252ee5f-6263-4340-bf4d-abf2e7902582

📥 Commits

Reviewing files that changed from the base of the PR and between 4eb6f99 and 871dca7.

📒 Files selected for processing (2)

• src/jsc/bindings/node/crypto/CryptoGenRsaKeyPair.cpp
• test/js/node/crypto/crypto.key-objects.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

No bugs found, but this changes user-facing node:crypto behavior in a way that diverges from Node/OpenSSL (rejecting non-multiple-of-128 modulusLength where Node honors it exactly), so it's worth a human confirming reject-over-round-up is the intended contract.

What was reviewed:

• Verified the ERR::OUT_OF_RANGE overload matches (ErrorCode.cpp:953) and produces the message the test asserts on.
• Grepped every modulusLength: in test/js/node/ — the only non-multiple-of-128 values are in test-crypto-keygen-bit-length.js (already skips on openssl_is_boringssl) and the pre-existing test.skip(513) in this file, so no other tests break.
• Check placement mirrors the existing publicExponent guard just below and covers both rsa and rsa-pss before the branch.

Extended reasoning...

Overview

Adds a 5-line guard in RsaKeyPairJobCtx::fromJS (CryptoGenRsaKeyPair.cpp) that rejects modulusLength % 128 != 0 with ERR_OUT_OF_RANGE, plus two tests in crypto.key-objects.test.ts covering generateKeyPairSync and the sync-throw path of async generateKeyPair. Also rewords the skip comment on the existing 513-bit test.

Security risks

The change is security-positive: it replaces a silent downgrade (BoringSSL's bits &= ~127 masking, e.g. 3000→2944) with a loud, catchable error. There's no new parsing of untrusted input — modulusLength is already run through validateUint32 before the new check reads it. modulusLength: 0 still passes the modulo check but is rejected downstream by BoringSSL as before.

Level of scrutiny

Deferring because this is node:crypto and the fix encodes an API design decision: Node+OpenSSL generates 1023/3000/4095-bit keys exactly, so this PR intentionally diverges from Node by rejecting inputs Node accepts. The alternative (round up to the next multiple of 128, or leave the silent round-down) isn't discussed in the diff, and per the repo's API-design guidance a maintainer should sign off on the chosen contract. The sibling WebCrypto fix (#33477) apparently set precedent, but confirming consistency there is worth a human glance.

Other factors

The implementation itself looks correct and idiomatic — it sits right next to the identically-structured publicExponent < 3 || even guard, uses the same ERR::OUT_OF_RANGE(scope, global, name, msg, actual) overload, and returns std::nullopt after setting the exception. Tests fail on system Bun (1023→896, 3000→2944) and pass on the debug build per the PR body. No exception-scope issues: modulusLengthValue is a plain JSValue already fetched and validated above the new lines.

[robobun]

On reject vs round-up: went with reject to match #33477's choice for the WebCrypto path and because the issue sketch named "validate/reject (or exactly honor)" as the options. Rounding up would also leave asymmetricKeyDetails.modulusLength disagreeing with the caller's input, just in the other direction, and silently; an explicit error names the constraint so the caller can pick a supported size. Either direction diverges from Node+OpenSSL since BoringSSL can't generate arbitrary sizes, and Node's own test-crypto-keygen-bit-length.js already acknowledges that by skipping on openssl_is_boringssl.

Happy to switch to round-up if that's preferred; it's a one-line change from modulusLength % 128 != 0 to modulusLength = (modulusLength + 127) & ~127u.