Repository navigation
Conversation
…ngth CryptoKeyRSA::algorithm() computed modulusLength as RSA_size() * 8, the byte length of n times 8. For an imported key whose modulus is not a multiple of 8 bits long (2047, 2041, 2049 bits) this reported 2048, 2048 and 2056. Node and Chromium report the bit length. Use RSA_bits().
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Report RSA modulus bit lengths src/jsc/bindings/webcrypto/CryptoKeyRSAOpenSSL.cpp, test/js/web/crypto/web-crypto.test.ts |
getRSAModulusLength uses RSA_bits(). Tests check the reported lengths for imported RSA JWK moduli. |
Suggested reviewers: jarred-sumner, alii
Merge Risk: 
c78a7
-
The change reports actual RSA modulus bit lengths and adds targeted import tests. No actionable merge-blocking risk was identified.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: reporting the RSA modulus bit length through RsaKeyAlgorithm.modulusLength. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description states that the change duplicates PR #47559(bc55d56), which already landed the same one-line fix and test on main, so nothing remains to merge. It is otherwise complete: it explain…Full details: Description check
-
Explanation
The description states that the change duplicates PR
#47559(bc55d56), which already landed the same one-line fix and test on main, so nothing remains to merge. It is otherwise complete: it explains the problem (modulusLength is computed as RSA_size * 8, which is byte length rounded up, not the bit length), gives the fix (RSA_bits), notes the downside (2047-bit imported keys now report 2047), and includes verification (the new test fails on main and passes with the fix, plus vendored Node tests), with a repro. It covers the template's 'What does this PR do?' and 'How did you verify your code works?' content, though it is laid out as Problem/Fix/Background/Downsides rather than the template headings.
-
Finishing Touches
-
Fix failing CI checks
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — RSA_bits(rsa) is the correct primitive for the spec's "length, in bits, of the RSA modulus", and the only other consumers of this helper just compare against zero.
What was reviewed:
- Confirmed
getRSAModulusLengthfeeds onlyCryptoKeyRSA::algorithm()(the field being fixed) andkeySizeInBits(), whose three RSA callers (RSA_OAEP,RSASSA_PKCS1_v1_5,RSA_PSS) all use it as a!= 0gate — no behavioral change there. - Test covers byte-aligned (2048), sub-byte (2047, 2041), and over-byte (2049) moduli; the last three would report 2048/2048/2056 under the old
RSA_size*8path, so the assertion is load-bearing. - Test placement and shape follow the repo conventions (existing file, single
.toEqualon a composite, no network/timing).
Extended reasoning...
Overview
This PR changes one line in src/jsc/bindings/webcrypto/CryptoKeyRSAOpenSSL.cpp: the static helper getRSAModulusLength(RSA*) now returns RSA_bits(rsa) instead of RSA_size(rsa) * 8. RSA_size returns the modulus size in bytes (rounded up), so multiplying by 8 loses the low bits of the true bit-length; RSA_bits returns BN_num_bits(n) directly. The WebCrypto spec defines RsaKeyAlgorithm.modulusLength as the bit-length of the modulus, so the new value is spec-correct and matches Node and Chromium. A new test block in test/js/web/crypto/web-crypto.test.ts imports JWK public keys with 2048/2047/2041/2049-bit moduli and asserts the exact reported bit-lengths.
Security risks
None introduced. The change is a read-only reporting fix on an already-parsed RSA*; no new input parsing, allocation, or trust boundary is touched. If anything it hardens callers that gate on modulusLength >= N, since a 2041-bit key will no longer masquerade as 2048-bit. RSA_bits on a null-n RSA is not reachable here because the helper's own null guard on rsa remains, and every path that produces this RSA* sets n.
Level of scrutiny
Low. This is a one-line substitution between two well-documented BoringSSL primitives with a clear semantic difference, applied at the layer that owns the invariant (the C++ helper feeding the algorithm dictionary). I grepped every caller of getRSAModulusLength and CryptoKeyRSA::keySizeInBits(): the former is used only in algorithm() (the target of the fix) and keySizeInBits(); the latter is called only in CryptoAlgorithmRSA_OAEP.cpp, CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp, and CryptoAlgorithmRSA_PSS.cpp, each as if (!rsaKey.keySizeInBits()). Since RSA_bits and RSA_size*8 agree on zero-ness, those paths are unaffected. The EC/OKP keySizeInBits hits in the grep are unrelated types.
Other factors
The test is placed in the existing WebCrypto test file per repo convention, asserts a single composite object via .toEqual, and covers the variant matrix (exact byte boundary plus three non-byte-aligned cases that each fail under the old code — 2047→2048, 2041→2048, 2049→2056). No CODEOWNERS entry covers src/jsc/bindings/webcrypto/ or the test file. The bug hunt exited on dry_streak with no findings and no prior review activity on the PR.
…imported-modulus-length
… types The test directory is type-checked without lib.dom, so the global RsaKeyAlgorithm type does not exist there.
There was a problem hiding this comment.
LGTM — a one-line fix that makes RsaKeyAlgorithm.modulusLength report the modulus bit length, matching Node and the spec.
What was reviewed:
- Both callers of getRSAModulusLength: algorithm() (the reported dictionary) and keySizeInBits(), whose three RSA consumers only compare against zero, so no sign/encrypt/decrypt path changes.
- Sibling sites: node:crypto's KeyObject already reports the bit count of n, and no other RSA_size8 / EVP_PKEY_size8 sites exist in src/, so this was the only byte-rounded reporter.
- The new test's fixtures: the 256-byte modulus has its top bit set (2048), and the three rewrites produce exactly 2047/2041/2049-bit values; the assertion fails on the old code (2048/2048/2056).
Extended reasoning...
The change replaces RSA_size(rsa) * 8 with RSA_bits(rsa) in the static helper at src/jsc/bindings/webcrypto/CryptoKeyRSAOpenSSL.cpp:45 and adds one describe block to test/js/web/crypto/web-crypto.test.ts. It touches the WebCrypto RSA key bindings, but only the reported modulusLength metadata; the keySizeInBits() consumers in the RSA-OAEP, RSASSA-PKCS1-v1_5 and RSA-PSS algorithms only check for a nonzero value, so no cryptographic operation is affected. The node:crypto KeyObject path already used the bit count of n, so the fix brings WebCrypto in line with the existing in-tree behavior rather than introducing a new convention. The diff is tiny, self-contained, and the new test exercises the exact boundary cases (2047, 2041, 2049 bits) that distinguish the old and new behavior; the commits since the earlier review only merged main and adjusted the test's type cast.
|
This branch has CI on c78a76c (build 134795) is finished. 187 jobs passed. No failure is in a WebCrypto test, and none comes from this change:
The diff is one line ( |
|
Main covers this PR. PR #47559 (bc55d56) made the same one-line change in I checked this on a debug build of bc55d56:
This PR has nothing that main lacks. The one difference is that its test has three more modulus sizes than the test on main (2048, 2041, 2049). They go through the same line. I did not rebase it, because the rebase leaves only those three sizes in a test. A maintainer can close it. |
Problem
algorithm.modulusLengthof an imported RSA key is the byte length ofntimes 8, not the bit length of the modulus. A JWK or SPKI key with a 2047-, 2041- or 2049-bit modulus reports 2048, 2048 and 2056. Node v26.12.1 reports 2047, 2041 and 2049. AmodulusLength >= 2048gate then passes a 2041-bit key in Bun only.getRSAModulusLength(src/jsc/bindings/webcrypto/CryptoKeyRSAOpenSSL.cpp:41), which returnsRSA_size(rsa) * 8.RSA_sizeis the modulus size in bytes, rounded up.Fix
RSA_bits(rsa), which isBN_num_bits(n).CryptoKeyRSA::keySizeInBits()uses the same helper. Its three callers compare the result with zero, so nothing else changes. Generated keys are unaffected: BoringSSL generates exactlymodulusLengthbits.test/js/web/crypto/web-crypto.test.ts("RsaKeyAlgorithm.modulusLength of an imported key", fails on main). Also the vendoredtest-webcrypto-*-rsa.js,test-crypto-key-objects-to-crypto-key.jsandtest-webcrypto-cryptokey-clone-transfer.js.Background
RsaKeyAlgorithm.modulusLengthis "the length, in bits, of the RSA public modulus" (WebCrypto, RsaKeyAlgorithm dictionary).CryptoKeyRSA::algorithm()builds that dictionary from theRSA*inside the key'sEVP_PKEYfor every RSA key, whether generated or imported through jwk, spki or pkcs8.Downsides
modulusLengthwith 2048 gets a different answer for such a key, as it does in Node.RSA_bitsisBN_num_bits(n), andRSA_sizewasBN_num_bytes(n).Notes
Repro:
bun 1.4.3 and main (11c3671): 2048 / 2048 / 2056. node v26.12.1: 2047 / 2041 / 2049. With this change: 2047 / 2041 / 2049. The run that found this reported 2047 and 2041 for Chromium 148 too.
The test imports public keys whose modulus has a rewritten leading byte. Such a modulus is not a product of the original primes, so the keys are useless for encryption, but public key import cannot check that and
modulusLengthonly readsn.The branch has
main(73b5310) merged. There was no conflict.[auto-merge] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file