Repository navigation
Conversation
|
Updated 5:05 PM PT - Oct 21st, 2026
❌ @robobun, your commit 9a355e2 has some failures in 🧪 To try this PR locally: bunx bun-pr 45952That installs a local version of the PR into your bun-45952 --bun |
|
Status How I reproduced it:
PR: #45952 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughRSA key creation rejects zero-valued components. Serialized key deserialization validates ChaCha20-Poly1305 lengths, detects failed imports, and clears BoringSSL errors from failed imports and PEM parses. Tests cover malformed records, error cleanup, and IPC rejection. ChangesCrypto key handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly summarizes both primary changes: rejecting CryptoKey records that fail import and rejecting RSA JWKs with zero components. |
| 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 is complete and relevant. It clearly explains the problem, fix, verification, background, downsides, test results, and review considerations. It does not use the exact template heading… |
Full details: Description check
-
Explanation
The description is complete and relevant. It clearly explains the problem, fix, verification, background, downsides, test results, and review considerations. It does not use the exact template headings
### What does this PR do?and### How did you verify your code works?, but it provides the required information in equivalent sections.
- 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.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked that moving the ERR_clear_error() out of readAKPKey is behavior-preserving: it now returns true with a null result, which reaches the central !result branch in readCryptoKey that clears the queue and fails the decode, so the ML-DSA path keeps its previous contract. I also checked the isZero change in CryptoKeyRSA::create against the JWK and record-reader callers: for the CRT params a zero component becomes nullptr, and RSA_set0_crt_params rejects a null parameter, so the resulting DataError is the behavior the PR description states rather than an accident.
Extended reasoning...
The change touches the structured-clone CryptoKey reader in src/jsc/bindings/webcore/SerializedScriptValue.cpp and RSA key construction in src/jsc/bindings/webcrypto/CryptoKeyRSAOpenSSL.cpp, both crypto and deserialization surfaces that parse untrusted bytes. Three inline findings were posted (a pre-existing ChaCha20 raw-key hole, a test assertion that does not pin the error class, and bare ERR_clear_error calls instead of the RAII helper), and the WebCrypto import contract change is one the author explicitly flags as needing a maintainer, so a human review is warranted rather than approval.
3abd2fa to
56a1dba
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the readAKPKey refactor — its null result now reaches the centralized !result branch in readCryptoKey, so the ML-DSA/ML-KEM clear-and-fail behavior is preserved; the new readRawKey early return needs no ERR_clear_error() since neither the length check nor CryptoKeyRaw::create touches BoringSSL; and isZero() on the CRT params yields the same nullptr to RSA_set0_crt_params that an empty component already did, so no previously-accepted CRT shape changes.
Extended reasoning...
The push since the prior review adds the ChaCha20-Poly1305 key-length check in src/jsc/bindings/webcore/SerializedScriptValue.cpp and switches the bun-jsc and IPC tests to compare the thrown error's class as well as its message. This is WebCrypto and structured-clone deserialization code, so a human maintainer should still weigh the RSA zero-component import rejection the PR description flags as a compat decision.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/jsc/bindings/webcore/SerializedScriptValue.cpp— pre-existing: a CryptoKey record whose HMAC or RSA hash tag names a non-digest algorithm still deserializes to a key that importKey would never create. readHMACKey at SerializedScriptValue.cpp:3063 and readRSAKey at :3095 accept any CryptoAlgorithmIdentifier as the hash, and CryptoKeyHMAC::importRaw never checks it. Fix: have both readers reject a record whose hash is not a digest, e.g.if (!digestAlgorithm(hash)) return false;(OpenSSLUtilities.h:40), so the reader matches what importKey accepts. The resulting key reports algorithm.hash.name such as "AES-GCM", sign() fails with OperationError, and exportKey("jwk") reaches ASSERT_NOT_REACHED at CryptoAlgorithmHMAC.cpp:257 in debug builds.Why this was flagged
A bun:jsc or node:v8 deserialize call, an advanced-IPC frame, or a postMessage payload carries a CryptoKey record of class HMAC (or RSA with isRestrictedToHash set) whose hash byte is any valid CryptoAlgorithmIdentifierTag that is not SHA-*, for example the tag for AES_GCM. read(CryptoAlgorithmIdentifier&) at SerializedScriptValue.cpp:2899 maps every tag, so readHMACKey at :3063 and readRSAKey at :3095 accept it. CryptoKeyHMAC::importRaw at CryptoKeyHMAC.cpp:87 only checks the key length, and CryptoKeyRSA::create never looks at the hash, so a non-null key is produced and the new null check at SerializedScriptValue.cpp:3362 does not fire. crypto.subtle.importKey cannot build such a key because normalizeImportParameters only accepts digest algorithms for hash. The caller gets a CryptoKey whose algorithm.hash.name is "AES-GCM"; platformSign at CryptoAlgorithmHMACOpenSSL.cpp:64 returns OperationError, and exportKey("jwk") hits ASSERT_NOT_REACHED at CryptoAlgorithmHMAC.cpp:257 in debug builds. The base branch behaves the same.
Verification: pre-existing. Triggered when a deserialize call or postMessage payload carries an HMAC (or RSA) CryptoKey record whose hash byte is a valid non-digest tag (e.g. AES_GCM).
readHMACKeyat src/jsc/bindings/webcore/SerializedScriptValue.cpp:3062-3065 passes thathashstraight toCryptoKeyHMAC::importRaw, which checks onlykeyData.size()andlengthBits. The new check at :3362 does not fire. The base branch behaves identically.
CryptoKeyRSA::create rejected an empty component. It now also rejects
a component whose bytes are all zero, for the modulus, the exponents,
the primes and the CRT values. No valid RSA key has such a component,
and exportData() writes it as no bytes, so the key could not be
created again from its own components.
importKey("jwk") and the structured-clone reader are the two callers.
…port readCryptoKey returns false when the import of the key bytes gives no key, and clears the BoringSSL error that the failed import can leave. Before, the record deserialized to null. The clear that readAKPKey had for ML-DSA and ML-KEM keys moves to that one place. readKeyObject clears the BoringSSL error of a PEM that does not parse.
…length readRawKey built the key from any number of bytes, because CryptoKeyRaw::create does not fail. importKey rejects a ChaCha20-Poly1305 key that is not 256 bits, and the reader now does the same. HKDF and PBKDF2 keys still take any length. The tests compare the class of the thrown error and not only its message.
56a1dba to
9a355e2
Compare
|
The second review found one more pre-existing case: a record whose HMAC or RSA hash tag is not a digest still decodes to a key. I confirmed it. A record with the AES-GCM tag in the hash field gives a key with I do not change that in this PR. This PR checks the key bytes of a record, and the hash tag is another field. The description now lists the case under "Not changed here". The branch is rebased onto the current |
Problem
bun:jscandnode:v8deserialize()and advanced IPC returnnullfor aCryptoKeyrecord whose key bytes do not import. No user reported it.readCryptoKey(src/jsc/bindings/webcore/SerializedScriptValue.cpp:3362) turns the null import result into JSnulland reports success.KeyObjectPEM parse leaves a BoringSSL error queued. The next faileddecipher.final()reportsERR_OSSL_EC_POINT_IS_NOT_ON_CURVE.Fix
readCryptoKeyreturns false when the import gives no key.readRawKeyrejects a ChaCha20-Poly1305 key that is not 32 bytes. The decode throwsTypeError: Unable to deserialize data.readKeyObjectdoes after a bad PEM.CryptoKeyRSA::createrejects a zero component. Otherwise a JWK withe: "AA"imports and its clone goes fromnullto a throw.bun-jsc.test.ts(16 new, 14 fail on main),web-crypto.test.ts(1),spawn.ipc.test.ts(1). Self-reviewed: 27 concerns raised, 22 addressed (Notes).Background
CryptoKeyrecord is plain bytes, validated only byreadCryptoKeyand the import functions.node:cryptocan report its oldest entry.Downsides
deserialize()returnednullfor these records since Bun 1.2.11 and now throws. Advanced IPC closes the channel.importKey("jwk")rejects an RSA key with a zero component. Chrome rejects all 8 forms, Node.js accepts 6. This needs a maintainer.Notes
Reproduction (Bun 1.2.11, 1.3.13, 1.4.2 and
main, release, Linux x64):Bun 1.2.10 throws
DataCloneErrorwhen it serializes aCryptoKey. Bun 1.2.11 serializes it (#19040), and thenulldates from there.Records that gave
null. 21 of 21 malformed records, throughbun:jscand throughnode:v8:An ML-DSA record already threw, because
readAKPKeyreturned!!result. With this PR all 22 throw.A ChaCha20-Poly1305 record with 0, 31 or 33 key bytes did not give
null. It decoded to a key, becauseCryptoKeyRaw::createdoes not fail, and everyencryptwith that key throwsOperationError.importKeyrejects those bytes withDataError: Invalid key length. The record now throws too. HKDF and PBKDF2 keys of any length still clone. A malformed key inside[key, key, { key }]decoded to[null, null, { key: null }]and now fails the whole value. Advanced IPC deliverednullas a message. It now reports theTypeErrorand closes the channel.235 keys made through
crypto.subtleandKeyObject.toCryptoKeyclone to a key with and without this change.Why the RSA change is in this PR.
importKey("jwk")onmainaccepts an RSA key with one component of value zero:n,e,d,p,q,dp,dqorqi. These are the only keys fromimportKeywhose record does not import. Each one clones tonullonmain.BIGNUMof value zero exports as zero bytes (convertToBytes,src/jsc/bindings/webcrypto/OpenSSLUtilities.cpp:97), the record stores that empty component, andCryptoKeyRSA::create(CryptoKeyRSAOpenSSL.cpp:71) rejected an empty component but accepted a zero one.nullto a throw.worker_threads.receiveMessageOnPortaborts the process for a value that serializes but does not deserialize, so the check would add a way to reach that abort.createnow treats a component whose bytes are all zero like an empty one. It has two callers,importKey("jwk")and the record reader, so the guard is there and not in the JWK code.DataErrorfor a JWK that is not a valid RSA key. Bun'sspki,pkcs8andKeyObject.toCryptoKeyimports already reject these keys.mainn,e,d,dp,dq,qinullDataErrorp,qnullDataErrorDataErrorNo serialized bytes change. The records of 8 ordinary keys (HMAC, AES-GCM, RSA, ECDSA, Ed25519) are byte-identical to
main, andcurrentKeyFormatVersionstays 2. A record of one of the zero-component keys that Bun 1.3.13, 1.4.2 ormainwrote reads asnullthere and throws with this PR.An earlier draft of this change kept those keys: the record writer stored one
0x00byte for a zero component. The self-review rejected that draft. It changed serialized bytes without a version bump (REVIEW.md:40), and it kept keys that the open #33903 rejects withRSA_check_keyin the same function.Error queue. One process per case. The case counts as stale when the next failed
decipher.final()reports a code other thanERR_OSSL_BAD_DECRYPT.mainCryptoKey, 8 casesKeyObjectwith a PEM that does not parse, 3 casesCryptoKey, 2 casesA failed HMAC, AES, RSA or Ed25519 import leaves no entry (0 of 9 cases on
main). A plain failedcrypto.subtle.importKeyleaves the same kind of entry as a failed EC record. That is #44863, and #44940 covers it.Measurements (release builds of
main6f58f6b and of this change on that commit, Linux x64, gdb.perf,valgrind,bloatyandstraceare not installed here.)readCryptoKey, own instructions per successful decode (10th decode): 87 to 86 for HMAC, RSA public, RSA private, ChaCha20-Poly1305 and HKDF. 88 to 87 for AES-GCM, P-256 public, P-256 private, Ed25519 and ML-DSA-44. The null test was already there fortoJS. The success path loses onejmp.readCryptoKeywith its callees (10th decode, two equal runs each): HMAC 368 to 367, AES-GCM 381 to 380, Ed25519 521 to 520. RSA-2048 public 1723 to 1750 and RSA-2048 private 5298 to 5353: the zero checks increate. ChaCha20-Poly1305 358 to 370 and HKDF 355 to 366: the length check inreadRawKey.CryptoKeyRSA::create, own instructions inimportKey("jwk"): public 106 to 113, private 187 to 222 (measured on the first version of this PR, the function did not change since).ERR_clear_errorcalls over 1000 decodes: valid HMAC records 0 (main0), EC records with a point off the curve 1000 (main0), HMAC records with 0 key bytes 1000 (main0), malformed payloads with no key 0 (main0).ERR_set_markandERR_pop_to_mark: 0 in every row.mi_malloc/mi_free, 10th decode): HMAC 3 / 1, AES-GCM 3 / 1, ChaCha20-Poly1305 3 / 1, RSA public 12 / 6, RSA private 36 / 18, P-256 public 9 / 5, ML-DSA-44 5 / 2. The same onmain.buntext (llvm-size): 107098365 B to 107098717 B. By symbol (nm -S):readCryptoKey524 to 527,readAKPKey297 to 267,readRawKey334 to 355,readKeyObject1749 to 1754,CryptoKeyRSA::create971 to 1123.readTerminal(6035),readRSAKey(1823) and the writer (7190) do not change.Tests.
bun-jsc.test.ts: 16 new tests. 14 fail onmain. The 2 ML-DSA rows pass before and after: they pin the clear that moved out ofreadAKPKey.toThrow(error)compares the message only.web-crypto.test.ts: 1 new test. Onmainall 8 zero-component keys import.spawn.ipc.test.ts: 1 new test. It fails onmainwithMESSAGE null.ERR_clear_error()removed from it, exactly the 3 queue rows fail (EC public, EC private, ML-DSA-44).decipher.final()reports the oldest entry of the queue, so it shows an entry that the deserialize left.structured-clone.test.ts,structuredClone-classes.test.ts,message-channel.test.ts,structured-clone-fastpath.test.ts,worker-postmessage-transfer.test.ts,v8-serdes-buffer.test.ts,node-crypto.test.js,crypto.key-objects.test.ts,child_process_ipc.test.js, and 45 Node test files (test-webcrypto-*,test-crypto-key-objects*,test-crypto-rsa*,test-crypto-keygen-rsa*,test-crypto-sign-verify).Review follow-up.
ncrypto::ClearErrorOnReturnin place of the threeERR_clear_error()calls. The guard clears on entry and on return, so every successful key clone would make 2 calls. The calls in this PR run on the failure branches only: 0 calls per 1000 valid decodes.Self-review: the 5 concerns not taken.
readArrayBufferViewImplstill decodes a view that lies outside its buffer tonull, through the same nullabletoJS: 16 of 47 one-byte changes to aUint8Array(16)record givenull, onmainand with this PR. It is a different reader with its own inputs, and structured-clone: stop decoding zero bytes to null #45366 lists it as separate work.Not changed here.
readOKPKeyandreadHMACKeyare not edited (webcrypto: accept HMAC key lengths that are not a multiple of 8 #42107 rewrites the second). The check after the class switch covers both.node:cryptocreatePublicKeystill accepts an RSA JWK witheof zero, as Node.js does. The clone of thatKeyObjectthrows, andreceiveMessageOnPortaborts for it (exit 134), onmainand with this PR.worker_threads.receiveMessageOnPortaborts for any value that serializes but does not deserialize, for example theKeyObjectabove (Fire messageerror when a posted message fails to deserialize #39408 is about the failed decode).algorithm.hash.nameis then, for example,AES-GCM, andsignthrowsOperationError.importKeycannot make such a key. This PR checks the key bytes of a record, not its other fields.CryptoKeyRSA::createdoes not check that the components agree. A record withp = q = nstill decodes to a key, as the same JWK imports (webcrypto: validate RSA private key components on JWK import #33903).readX509Certificateclears the error queue on entry and on return, for a successful clone too.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/bun/spawn/spawn.ipc.test.ts, test/js/bun/jsc/bun-jsc.test.ts