Update BoringSSL, add SHA3 to WebCrypto and node:crypto#29323
Update BoringSSL, add SHA3 to WebCrypto and node:crypto#29323Jarred-Sumner merged 9 commits intomainfrom
Conversation
Bumps BoringSSL to oven-sh/boringssl@0c5fce43b (synced with upstream beafe3db1). Adds SHA3-256/384/512 to crypto.subtle.digest and HMAC, plus sha3-224/256/384/512 to node:crypto createHash/createHmac. Updates the Zig env_md_ctx_st struct to match the new EVP_MAX_MD_DATA_SIZE (240, was 208), which is required since the md_data buffer now holds the larger Keccak state.
|
Updated 2:05 AM PT - Apr 15th, 2026
❌ @Jarred-Sumner, your commit 4be6752 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 29323That installs a local version of the PR into your bun-29323 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds SHA‑3 support (SHA3‑224/256/384/512) across Bun: updates BoringSSL commit and Zig bindings to expose EVP_sha3_*; implements SHA‑3 WebCrypto classes and EVP-based digest/HMAC plumbing in C++/headers; extends enums/serialization, HMAC/key handling, PBKDF2 acceptance, allowlist, and tests. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/webcrypto/CryptoDigest.cpp`:
- Around line 107-137: Add defensive ASSERT checks for the return values of the
OpenSSL EVP calls inside CryptoDigestContextEVP: verify
EVP_MD_CTX_init/EVP_DigestInit_ex in the constructor, EVP_DigestUpdate in
addBytes, and EVP_DigestFinal_ex (and EVP_MD_CTX_cleanup if desired) in the
destructor/computeHash to catch unexpected failures; update the constructor
(CryptoDigestContextEVP), addBytes, and computeHash methods to assert the EVP_*
functions return success (non-zero) and provide clear assertion messages
referencing the function names.
In `@test/js/web/crypto/web-crypto-sha3.test.ts`:
- Around line 40-42: The test "SHA3-256 large input (>64 bytes, async path)"
uses "a".repeat(1_000_000) which violates the test guideline; update the input
creation in that test to use Buffer.alloc(1_000_000, "a").toString() instead of
.repeat(). Locate the test case with description "SHA3-256 large input (>64
bytes, async path)" in web-crypto-sha3.test.ts and replace the TextEncoder input
source accordingly so the rest of the test (crypto.subtle.digest call) remains
unchanged.
- Around line 95-116: Replace the inline require(...) calls with module-scope
imports: remove the in-test require usages of createHash, createHmac, and
getHashes and add a single top-level import like "import { createHash,
createHmac, getHashes } from 'node:crypto';" so the tests use the module-scoped
symbols (createHash, createHmac, getHashes) instead of calling require() inside
each it() block.
🪄 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: c2128a47-27dd-428f-a0ee-70a30875aaa3
📒 Files selected for processing (16)
scripts/build/deps/boringssl.tssrc/bun.js/api/crypto/EVP.zigsrc/bun.js/bindings/AsymmetricKeyValue.cppsrc/bun.js/bindings/webcore/SerializedScriptValue.cppsrc/bun.js/bindings/webcrypto/CryptoAlgorithmHMAC.cppsrc/bun.js/bindings/webcrypto/CryptoAlgorithmIdentifier.hsrc/bun.js/bindings/webcrypto/CryptoAlgorithmRegistryOpenSSL.cppsrc/bun.js/bindings/webcrypto/CryptoAlgorithmSHA3.cppsrc/bun.js/bindings/webcrypto/CryptoAlgorithmSHA3.hsrc/bun.js/bindings/webcrypto/CryptoDigest.cppsrc/bun.js/bindings/webcrypto/CryptoDigest.hsrc/bun.js/bindings/webcrypto/CryptoKeyHMAC.cppsrc/bun.js/bindings/webcrypto/OpenSSLUtilities.cppsrc/bun.js/bindings/webcrypto/SubtleCrypto.cppsrc/deps/boringssl.translated.zigtest/js/web/crypto/web-crypto-sha3.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this PR updates a major crypto dependency (~690 BoringSSL commits) and adds new algorithm support across WebCrypto, node:crypto, and Zig FFI — a human should sign off on the memory layout change in boringssl.translated.zig and the BoringSSL bump itself.
Extended reasoning...
Overview
This PR bumps BoringSSL (~690 upstream commits) and wires SHA3-256/384/512 through the full WebCrypto stack: new CryptoAlgorithmSHA3 classes, an EVP-based CryptoDigestContext, SerializedScriptValue structured-clone tags, HMAC key-length defaults, OpenSSLUtilities digest mapping, SubtleCrypto normalization, and Zig-side EVP bindings. SHA3-224 is also added to node:crypto only. 15 new NIST FIPS 202 test vectors are included.
Security Risks
This touches the core cryptographic primitives layer. The BoringSSL bump is ~690 commits — any regression in the upstream library or misapplied patch would affect TLS, all digest/HMAC operations, and key handling across all of Bun. The boringssl.translated.zig struct layout change (md_data: [208]u8 → [240]u8, EVP_MAX_MD_BLOCK_SIZE 128 → 144) is a low-level ABI detail that must exactly match the compiled BoringSSL; a mismatch causes silent memory corruption or segfaults. The author has verified sizeof(EVP_MD_CTX) = 264 in C and Zig, which is good, but this class of change benefits from a second pair of eyes.
Level of Scrutiny
High. This is a security-library dependency update combined with new algorithm support in production-critical WebCrypto code. The implementation looks correct and follows existing patterns precisely, test coverage is thorough, and NIST vectors are validated. Still, the BoringSSL bump and Zig ABI change are exactly the kind of thing that should have explicit human sign-off.
Other Factors
No bugs were identified by the automated review. The HMAC key-length defaults (SHA3-256: 1088 bits, SHA3-384: 832 bits, SHA3-512: 576 bits) match the correct Keccak rate values. JWK export for SHA3 intentionally leaves alg empty due to the absence of a standardized JWK algorithm name, which is reasonable. The structured-clone tag values (24–26) are correctly appended and the max-value constant is updated. Overall the logic is sound, but the dependency bump scope alone warrants a human reviewer.
Upstream BoringSSL commit f7543e6db moved internal symbols into the bssl:: namespace. The x25519 ADX scalar multiply functions are runtime-gated by CRYPTO_is_BMI2_capable() && CRYPTO_is_ADX_capable() in crypto/curve25519/curve25519.cc.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/js/web/crypto/web-crypto-sha3.test.ts (2)
35-37:⚠️ Potential issue | 🟡 MinorReplace
.repeat()withBuffer.alloc(...).toString()for the large-input fixture.This test currently uses
"a".repeat(1_000_000), which violates the test guideline for repetitive strings.Suggested patch
- const input = new TextEncoder().encode("a".repeat(1_000_000)); + const input = new TextEncoder().encode(Buffer.alloc(1_000_000, "a").toString());As per coding guidelines: “Use
Buffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/js/web/crypto/web-crypto-sha3.test.ts` around lines 35 - 37, In the test "SHA3-256 large input (>64 bytes, async path)" replace the repetitive-string fixture that uses "a".repeat(1_000_000) with Buffer.alloc(1_000_000, "a").toString() so the TextEncoder().encode(...) call uses the Buffer-generated string; update the const input assignment (the line creating input before calling crypto.subtle.digest) to use Buffer.alloc(...) to conform to the guideline.
76-77:⚠️ Potential issue | 🟡 MinorUse module-scope
node:cryptoimports instead of inlinerequire()in test bodies.These tests are not validating dynamic module loading behavior, so inline
require()should be replaced with top-level imports.Suggested patch
import { describe, expect, it } from "bun:test"; +import { createHash, createHmac, getHashes, pbkdf2Sync } from "node:crypto"; @@ it("createHash sha3-256", () => { - const { createHash } = require("node:crypto"); expect(createHash("sha3-256").update("abc").digest("hex")).toBe( @@ it("createHash sha3-384", () => { - const { createHash } = require("node:crypto"); expect(createHash("sha3-384").update("abc").digest("hex")).toBe( @@ it("createHmac sha3-512", () => { - const { createHmac } = require("node:crypto"); const out = createHmac("sha3-512", Buffer.from("key")).update("data").digest("hex"); @@ it("getHashes includes sha3", () => { - const { getHashes } = require("node:crypto"); const hashes = getHashes(); @@ it("pbkdf2Sync sha3-256", () => { - const { pbkdf2Sync } = require("node:crypto"); expect(pbkdf2Sync("pw", "salt", 1000, 32, "sha3-256").toString("hex")).toBe(As per coding guidelines: “Only use dynamic import or require when the test is specifically testing dynamic import or require behavior. Otherwise, always use module-scope import statements.”
Also applies to: 83-84, 90-91, 96-97, 104-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/js/web/crypto/web-crypto-sha3.test.ts` around lines 76 - 77, The tests use inline require("node:crypto") inside test bodies to get createHash; change these to module-scope imports by adding a top-level import of createHash from "node:crypto" and remove the inline require calls in the tests (references to createHash in web-crypto-sha3.test.ts should be updated accordingly); apply the same replacement for the other inline requires in this file where createHash (or other crypto symbols) are required in test bodies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/web/crypto/web-crypto-sha3.test.ts`:
- Around line 27-44: The new WebCrypto test suite "crypto.subtle.digest SHA-3"
(the describe block in web-crypto-sha3.test.ts) triggers ASAN SIGILL under
BUN_JSC_validateExceptionChecks=1; to fix, add the test file name
web-crypto-sha3.test.ts to test/no-validate-exceptions.txt under the "#
normalizeCryptoAlgorithmParameters" section so it is skipped by the ASAN
validation; ensure the exact filename is listed on its own line within that
section so CI will not run the validation for this test.
---
Duplicate comments:
In `@test/js/web/crypto/web-crypto-sha3.test.ts`:
- Around line 35-37: In the test "SHA3-256 large input (>64 bytes, async path)"
replace the repetitive-string fixture that uses "a".repeat(1_000_000) with
Buffer.alloc(1_000_000, "a").toString() so the TextEncoder().encode(...) call
uses the Buffer-generated string; update the const input assignment (the line
creating input before calling crypto.subtle.digest) to use Buffer.alloc(...) to
conform to the guideline.
- Around line 76-77: The tests use inline require("node:crypto") inside test
bodies to get createHash; change these to module-scope imports by adding a
top-level import of createHash from "node:crypto" and remove the inline require
calls in the tests (references to createHash in web-crypto-sha3.test.ts should
be updated accordingly); apply the same replacement for the other inline
requires in this file where createHash (or other crypto symbols) are required in
test bodies.
🪄 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: 10b7c468-a7b8-49f6-84d9-d297c8acf43e
📒 Files selected for processing (2)
src/bun.js/api/crypto/PBKDF2.zigtest/js/web/crypto/web-crypto-sha3.test.ts
| describe("crypto.subtle.digest SHA-3", () => { | ||
| for (const [alg, input, expected] of vectors) { | ||
| it(`${alg}(${JSON.stringify(input)})`, async () => { | ||
| const buf = await crypto.subtle.digest(alg, new TextEncoder().encode(input)); | ||
| expect(hex(buf)).toBe(expected); | ||
| }); | ||
| } | ||
|
|
||
| it("SHA3-256 large input (>64 bytes, async path)", async () => { | ||
| const input = new TextEncoder().encode("a".repeat(1_000_000)); | ||
| const buf = await crypto.subtle.digest("SHA3-256", input); | ||
| expect(hex(buf)).toBe("5c8875ae474a3634ba4fd55ec85bffd661f32aca75c6d699d0cdcb6c115891c1"); | ||
| }); | ||
|
|
||
| it("rejects unknown digest", async () => { | ||
| expect(crypto.subtle.digest("SHA3-1024" as any, new Uint8Array())).rejects.toThrow(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
ASAN stability fix is likely still missing for this new WebCrypto test file.
This suite matches the known BUN_JSC_validateExceptionChecks=1 SIGILL pattern in ASAN runs. Please add this test file under the # normalizeCryptoAlgorithmParameters section in test/no-validate-exceptions.txt to avoid CI-only crashes unrelated to SHA3 logic.
Based on learnings: “In oven-sh/bun, SIGILL crashes in ASAN CI for webcrypto tests … are caused by BUN_JSC_validateExceptionChecks=1 … Adding the test under the # normalizeCryptoAlgorithmParameters section of that file is the correct and complete fix.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/web/crypto/web-crypto-sha3.test.ts` around lines 27 - 44, The new
WebCrypto test suite "crypto.subtle.digest SHA-3" (the describe block in
web-crypto-sha3.test.ts) triggers ASAN SIGILL under
BUN_JSC_validateExceptionChecks=1; to fix, add the test file name
web-crypto-sha3.test.ts to test/no-validate-exceptions.txt under the "#
normalizeCryptoAlgorithmParameters" section so it is skipped by the ASAN
validation; ensure the exact filename is listed on its own line within that
section so CI will not run the validation for this test.
- Handle SHA3 in RSA-PSS/OAEP/RSASSA JWK import/export switches: omit alg field on export (no IETF-registered identifier exists), accept null alg on import. Prevents ASSERT_NOT_REACHED in debug builds when exporting RSA keys generated with SHA3 hash. - Remove stale ripemd160 HMAC rejection from CryptoHasher.zig - it works now that RIPEMD160 is in libcrypto. - Move sha3-* and ripemd160 from CryptoHasher's "unsupported" test list to the supported HMAC vectors (cross-validated against Python). - ASSERT on EVP_DigestInit_ex return in CryptoDigestContextEVP. - Fix missing await on rejects.toThrow(), use Buffer.alloc instead of string repeat, hoist node:crypto imports to module scope.
The scope was declared but never checked, tripping JSC's exception scope validation in ASAN builds. Matches the pattern in sign/verify/encrypt/decrypt which also call normalizeCryptoAlgorithmParameters without declaring a local scope (the IDL binding wrapper handles pending exceptions).
| case CryptoAlgorithmIdentifier::SHA3_256: | ||
| case CryptoAlgorithmIdentifier::SHA3_384: | ||
| case CryptoAlgorithmIdentifier::SHA3_512: |
There was a problem hiding this comment.
🔴 Adding SHA3 to Operations::Digest in SubtleCrypto.cpp (lines 207-209) causes toHashIdentifier() to succeed for SHA3 identifiers in every algorithm that calls it, inadvertently enabling crypto.subtle.sign/verify({name:'ECDSA', hash:'SHA3-256'}, ...) and crypto.subtle.deriveBits({name:'HKDF', hash:'SHA3-256'}, ...) without throwing NotSupportedError. ECDSA+SHA3 and HKDF+SHA3 are not defined in the W3C WebCrypto Level 1/2 spec, so Bun would silently produce non-interoperable signatures and key material that no other browser (Chrome, Firefox, Safari) can replicate.
Extended reasoning...
Root cause: toHashIdentifier() in SubtleCrypto.cpp validates hash parameters by calling normalizeCryptoAlgorithmParameters with Operations::Digest. This PR adds SHA3_256, SHA3_384, and SHA3_512 to the Operations::Digest switch (lines 207-209) so that crypto.subtle.digest() and HMAC can use SHA3. However, every other algorithm that validates its hash parameter — ECDSA sign/verify (line ~183), HKDF DeriveBits (line ~312), RSA key generation, and PBKDF2 — also calls toHashIdentifier() through the same path. Adding SHA3 to Operations::Digest therefore silently enables SHA3 as a valid hash parameter for all of these algorithms simultaneously.
ECDSA impact (bug_002): Before this PR, toHashIdentifier('SHA3-256') returned NotSupportedError because SHA3_256 was not registered. After this PR, normalizeCryptoAlgorithmParameters matches SHA3_256 in the Digest switch and returns successfully, so ECDSA sign/verify normalization (SubtleCrypto.cpp ~183-188) stores SHA3_256 in params.hashIdentifier. CryptoAlgorithmECDSAOpenSSL.cpp then calls digestAlgorithm(parameters.hashIdentifier) at line 41, which now returns EVP_sha3_256() (because this PR also updated OpenSSLUtilities.cpp). Since the result is non-null, the if (\!md) return Exception { NotSupportedError }; guard is bypassed and BoringSSL actually computes a real ECDSA-SHA3 signature. ECDSA with SHA-3 is not defined in the W3C WebCrypto Level 1 or Level 2 spec, nor in NIST FIPS 186-5 (which only standardizes ECDSA with SHA-2 families). Chrome, Firefox, and Safari all reject this operation; Bun would produce signatures that no other WebCrypto implementation can verify.
HKDF impact (bug_003): The same mechanism applies to HKDF. The HKDF DeriveBits normalization (SubtleCrypto.cpp ~309-312) calls toHashIdentifier(state, params.hash), which now resolves 'SHA3-256' successfully. CryptoAlgorithmHKDFOpenSSL.cpp:40-42 then calls digestAlgorithm(parameters.hashIdentifier), receives EVP_sha3_256() instead of nullptr, and proceeds to derive key material using HKDF-SHA3-256. The WebCrypto spec Section 33.5 defines HKDF only with SHA-1 and SHA-2 hashes. No other WebCrypto implementation accepts HKDF+SHA3, so derived key material cannot be reproduced by any spec-compliant peer.
Addressing the HKDF refutation: One verifier argued HKDF+SHA3 is intentional because PBKDF2+SHA3 was explicitly enabled (by removing SHA3 from the blocklist in PBKDF2.zig), RFC 5869 is hash-agnostic, and the WICG Modern Algorithms proposal covers derivation functions. However: (1) PBKDF2+SHA3 was an explicit, deliberate decision with matching Zig-side code changes; HKDF+SHA3 was not explicitly enabled anywhere — it is a side effect. (2) The WICG proposal is not yet ratified and no other browser has shipped HKDF+SHA3. (3) Producing key material that diverges from all other WebCrypto implementations without a spec justification is a silent interoperability break, not an extension. The fix described below makes HKDF+SHA3 opt-in when the spec catches up, rather than silently on by default today.
How to fix: The cleanest fix is to explicitly reject SHA3 identifiers inside the ECDSA sign/verify normalization and the HKDF DeriveBits normalization, after toHashIdentifier returns — for example, by checking if (hashIdentifier == SHA3_256 || hashIdentifier == SHA3_384 || hashIdentifier == SHA3_512) return Exception { NotSupportedError };. Alternatively, toHashIdentifier itself could be split into a general variant and a spec-restricted variant that only allows SHA-1/2 families, with callers choosing which to use. The Operations::Digest switch should remain as-is since SHA3 is valid for crypto.subtle.digest() and HMAC.
Step-by-step proof for ECDSA: (1) Before PR: CryptoAlgorithmRegistry::singleton().identifier('SHA3-256') returns std::nullopt → normalizeCryptoAlgorithmParameters returns NotSupportedError at the if (\!identifier) guard → ECDSA sign rejects. (2) After PR: SHA3_256 is registered via registerAlgorithm<CryptoAlgorithmSHA3_256>(); the Operations::Digest switch matches → toHashIdentifier returns SHA3_256 → ECDSA normalization stores it → digestAlgorithm(SHA3_256) returns EVP_sha3_256() → BoringSSL signs with SHA3 → non-interoperable signature returned with no error.
There was a problem hiding this comment.
This is intentional — Bun matches Node.js's webcrypto behavior here, not browsers'.
Node.js supports SHA3 as a hash for all of these (just verified with Node 24):
await crypto.subtle.sign({name:"ECDSA", hash:"SHA3-256"}, key, data) // works
await crypto.subtle.deriveBits({name:"HKDF", hash:"SHA3-256", ...}, k, n) // works
await crypto.subtle.generateKey({name:"HMAC", hash:"SHA3-256", length:256}, ...) // worksNode emits an ExperimentalWarning for SHA3 in webcrypto, but accepts it everywhere a hash is taken — not just digest().
On the NIST claim: FIPS 186-5 §6.4 says "An approved hash function or XOF, as specified in FIPS 180 or FIPS 202, shall be used" — FIPS 202 is SHA-3, so ECDSA-SHA3 is NIST-approved. RFC 5869 (HKDF) is hash-agnostic by design.
On interop: the "non-interoperable with browsers" concern applies equally to subtle.digest("SHA3-256") itself — no browser ships any SHA3 in WebCrypto yet, including the WICG-draft digest operation. Restricting SHA3 to digest-only wouldn't make the feature browser-portable; it would just force users to fall back to node:crypto for sign/HKDF (which already supports the same combos).
Keeping as-is for Node parity. Happy to add Node's experimental warning in a follow-up if wanted.
| return alg.isNull() || alg == ALG384; | ||
| case CryptoAlgorithmIdentifier::SHA_512: | ||
| return alg.isNull() || alg == ALG512; | ||
| case CryptoAlgorithmIdentifier::SHA3_256: | ||
| case CryptoAlgorithmIdentifier::SHA3_384: | ||
| case CryptoAlgorithmIdentifier::SHA3_512: | ||
| return alg.isNull(); | ||
| default: | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🟡 When exporting an HMAC-SHA3-256/384/512 key as JWK, no alg field is emitted (the switch in exportKey falls through with a bare break at lines 182-188 of CryptoAlgorithmHMAC.cpp), and the import checkAlgCallback at lines 125-131 accepts any SHA3 JWK whenever alg is null — so a SHA3-256 key exported to JWK can be silently re-imported as SHA3-512. The JWK roundtrip is lossy with respect to the hash variant; consider throwing NotSupportedError for SHA3 HMAC JWK export/import, or documenting that JWK does not preserve the SHA3 hash variant.
Extended reasoning...
What the bug is and how it manifests
When exportKey('jwk', hmacSha3Key) is called, CryptoAlgorithmHMAC.cpp lines 182-188 match all three SHA3 variants and execute a bare break without setting jwk.alg. This is intentional — no IANA-registered JWK alg identifiers exist for HMAC-SHA3. The exported JWK therefore looks like {kty:'oct', k:'...'} with no alg field for any SHA3 variant.
The specific code path that triggers it
During importKey('jwk', jwkData, {name:'HMAC', hash:'SHA3-512'}), the code reaches CryptoKeyHMAC::importJwk which calls the checkAlgCallback lambda at lines 125-131. For all three SHA3 identifiers the callback returns alg.isNull(). Since the exported JWK never sets alg regardless of which SHA3 variant was used, alg.isNull() is always true — the check passes silently. The import then proceeds with SHA3_512 as the hash but with the key material of the original SHA3-256 key.
Why existing code does not prevent it
For SHA-2 HMAC, exportKey always sets jwk.alg to a specific string (HS256, HS384, etc.), and the import callback verifies alg == 'HS512' (for example). Cross-variant confusion is rejected at import time. For SHA3, no analogous IANA-registered alg string exists, so the export produces no alg field and the import check is reduced to the vacuous alg.isNull() condition — which is true for all SHA3 export JWKs and therefore cannot distinguish between variants.
What the impact would be
The JWK roundtrip does not preserve the SHA3 hash variant. A developer who exports a SHA3-256 HMAC key, transmits the JWK, and re-imports it with hash:'SHA3-512' will get an HMAC-SHA3-512 key backed by 32-byte key material without any error. HMAC values computed with the original key will not match those from the re-imported key, silently breaking correctness guarantees.
Step-by-step proof
- const key256 = await crypto.subtle.generateKey({name:'HMAC', hash:'SHA3-256'}, true, ['sign','verify']); — creates a SHA3-256 HMAC key (default 1088 bits per CryptoKeyHMAC.cpp line 46, i.e. 136 bytes).
- const jwk = await crypto.subtle.exportKey('jwk', key256); — the switch in exportKey hits SHA3_256 case at line 182 and breaks; jwk.alg is never set (null/absent).
- const key512 = await crypto.subtle.importKey('jwk', jwk, {name:'HMAC', hash:'SHA3-512'}, true, ['sign','verify']); — importJwk calls checkAlgCallback(SHA3_512, null_string); the case at line 128 returns alg.isNull() == true; import succeeds.
- key512 is now an HMAC-SHA3-512 key backed by the 136-byte key material of the original SHA3-256 key — wrong variant, no error.
How to fix it
Option A (clean): Return NotSupportedError from exportKey and importKey for SHA3 HMAC in JWK format, making the limitation explicit and consistent with the existing rejection of SHA3 as a standalone importKey algorithm in SubtleCrypto.cpp lines 381-387.
Option B (best-effort): Emit a non-standard alg string (e.g. HS3-256) and verify it in the import callback, preventing cross-variant confusion at the cost of interoperability with other implementations that follow the JWK spec strictly.
There was a problem hiding this comment.
This matches Node.js's webcrypto behavior:
> const k = await crypto.subtle.generateKey({name:"HMAC",hash:"SHA3-256",length:256},true,["sign"])
> (await crypto.subtle.exportKey("jwk", k)).alg
undefinedThere's no IETF-registered JWK alg value for HMAC-SHA3 or RSA-SHA3, so omitting it is the only correct behavior — emitting a made-up identifier would break import in other implementations. The hash variant must be supplied at import time, same as it would be for an alg-less JWK from any source. Throwing on export would make the feature useless for key persistence with no real safety benefit (re-importing with wrong hash → wrong-size signatures → fails immediately on first verify, not silent corruption).
|
|
||
| invalid: { | ||
| switch (try EVP.Algorithm.map.fromJSCaseInsensitive(globalThis, arg4) orelse break :invalid) { | ||
| .shake128, .shake256, .@"sha3-224", .@"sha3-256", .@"sha3-384", .@"sha3-512" => break :invalid, | ||
| .shake128, .shake256 => break :invalid, | ||
| else => |alg| break :brk alg, | ||
| } | ||
| } |
There was a problem hiding this comment.
🟣 Calling pbkdf2Sync('pw', 'salt', 1000, 32, 'blake2s256') panics the Zig runtime because blake2s256 passes the PBKDF2 blocklist check (only .shake128 and .shake256 are blocked) but EVP.Algorithm.md() returns null for it — and PBKDF2.run() force-unwraps that null via .? at line 25. This is a pre-existing bug (blake2s256 was never blocked before this PR either), but this PR modifies the exact switch statement that should also guard it.
Extended reasoning...
What the bug is and how it manifests
blake2s256 is a valid entry in EVP.Algorithm.map (EVP.zig line ~77), so PBKDF2.fromJS resolves it to the .blake2s256 algorithm variant and proceeds past the invalid-algorithm gate. However, EVP.Algorithm.md() only handles .blake2b256 and .blake2b512 in its explicit match arms; .blake2s256 falls through to the else => null branch and returns null. PBKDF2.run() then calls algorithm.md().? (line 25), which force-unwraps an optional null — a Zig runtime panic that crashes the entire process.
The specific code path that triggers it
pbkdf2Sync('pw', 'salt', 1000, 32, 'blake2s256')callsPBKDF2.fromJS.EVP.Algorithm.map.fromJSCaseInsensitiveresolves'blake2s256'to.blake2s256.- The blocklist switch (lines 163-169, as modified by this PR) only matches
.shake128and.shake256;.blake2s256falls to theelse => |alg| break :brk algarm and is accepted. PBKDF2.run()callsalgorithm.md().?—md()returnsnullfor.blake2s256— Zig panics.
Why existing code doesn't prevent it
Before this PR, sha3 variants were also listed in the blocklist. The PR correctly removed them (since sha3 now has real EVP implementations). However, blake2s256 was also never blocked, and the modification to this switch statement is the exact right place to also block it. EVP.Algorithm.md() has no blake2s256 case because BoringSSL does not expose an EVP_blake2s256() function (only EVP_blake2b256/EVP_blake2b512 exist in BoringSSL's public API).
Impact
Any call to Node's pbkdf2 / pbkdf2Sync with digest 'blake2s256' will terminate the process with a Zig panic. This is a process-crashing denial-of-service if user-supplied digest names flow into PBKDF2.
How to fix it
Add .blake2s256 to the break :invalid list in PBKDF2.fromJS:
.shake128, .shake256, .blake2s256 => break :invalid,Alternatively, change PBKDF2.run() to handle a null md() gracefully instead of force-unwrapping.
Step-by-step proof
EVP.Algorithm.mapincludes"blake2s256" => .blake2s256— the name resolves successfully.EVP.Algorithm.md()match arms:.blake2b256,.blake2b512,.md4, ...,.@"sha3-512"— no.blake2s256case; hitselse => null.PBKDF2.fromJSblocklist:.shake128, .shake256 => break :invalid;.blake2s256is not listed, falls toelse => |alg| break :brk alg.PBKDF2.run()line 25:algorithm.md().?force-unwrapsnull→ Zig runtime panic, process terminates.
…n digest RETURN_IF_EXCEPTION immediately after the call so the scope is satisfied on both the JS-exception path and the WebCore-exception path. Removing the scope (previous attempt) just moved the unchecked-exception assertion to JSDOMPromiseDeferred::reject. Also: ASSERT_UNUSED on EVP_DigestUpdate/Final return values, and replace HMAC-SHA3-512 length check with a Python-validated vector.
| auto& vm = state.vm(); | ||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
| auto paramsOrException = normalizeCryptoAlgorithmParameters(state, WTF::move(algorithmIdentifier), Operations::Digest); | ||
| RETURN_IF_EXCEPTION(scope, void()); | ||
| if (paramsOrException.hasException()) { | ||
| promise->reject(paramsOrException.releaseException().code(), "Unrecognized algorithm name"_s); | ||
| return; |
There was a problem hiding this comment.
🔴 In SubtleCrypto::digest, RETURN_IF_EXCEPTION(scope, void()) fires BEFORE the paramsOrException.hasException() check — the opposite of the established pattern in every other SubtleCrypto method. When normalizeCryptoAlgorithmParameters encounters a JS exception (e.g., from a getter on the algorithm object), the function returns void() and destroys the Ref<DeferredPromise> without ever rejecting it, leaving the JS Promise permanently pending. While post-PR commit 4be6752 added this ordering deliberately to avoid an ASAN assertion in JSDOMPromiseDeferred::reject, this trades one bug for another — the correct fix is to clear or scope the pending exception before rejecting the promise, not to abandon the promise entirely.
Extended reasoning...
What the bug is and how it manifests
In SubtleCrypto::digest (lines 784–790 of the modified SubtleCrypto.cpp), the ordering is:
auto paramsOrException = normalizeCryptoAlgorithmParameters(state, WTF::move(algorithmIdentifier), Operations::Digest);
RETURN_IF_EXCEPTION(scope, void()); // fires BEFORE hasException() check
if (paramsOrException.hasException()) {
promise->reject(paramsOrException.releaseException().code(), "Unrecognized algorithm name");
return;
}Every other SubtleCrypto method (generateKey, deriveKey, deriveBits, importKey) consistently does the opposite — hasException() check first, reject the promise, then RETURN_IF_EXCEPTION after. The digest method inverts this order, meaning that on the JS-exception path the promise is never rejected.
The specific code path that triggers it
When the caller provides an algorithm object whose name getter throws a JavaScript exception:
await crypto.subtle.digest({ get name() { throw new Error('boom') } }, new Uint8Array());normalizeCryptoAlgorithmParameterscallsconvertDictionary<CryptoAlgorithmParameters>, which accesses.nameand triggers the getter.- The getter throws → JSC VM records a pending exception →
convertDictionarypropagatesException { ExistingExceptionError }. normalizeCryptoAlgorithmParametersreturns withExistingExceptionError; the JSC VM still has a live pending exception.- Back in
digest:RETURN_IF_EXCEPTION(scope, void())sees the pending VM exception and returnsvoid()immediately — beforehasException()is ever checked. SubtleCrypto::digestexits without callingpromise->reject().- The
Ref<DeferredPromise>is destroyed; the JS Promise is permanently pending.
Addressing the refutation: why the deliberate ordering still constitutes a bug
Post-PR commit 4be6752 placed RETURN_IF_EXCEPTION before hasException() deliberately, because the prior attempt (commit 4514cea, which removed the throw scope entirely) caused an ASAN unchecked-exception assertion inside JSDOMPromiseDeferred::reject. The assertion occurs because JSDOMPromiseDeferred::reject must execute JS (to create a DOMException object) while a pending JSC exception already exists in the VM — which is illegal per JSC's throw-scope discipline.
However, this is a choice between two bugs, not a correct fix: an ASAN assertion in debug builds is replaced by a silent promise hang in all builds. The correct resolution is to clear the pending exception (via scope.clearException() or equivalent) before calling promise->reject(), allowing the promise to be properly rejected without triggering the unchecked-exception assertion. All other SubtleCrypto methods work because their normalization paths do not commonly produce ExistingExceptionError in the way digest does when algorithm property getters throw.
What the impact is
Any call to crypto.subtle.digest() where the algorithm parameter's property getter throws a JS exception produces a Promise that never settles. The exception is consumed by RETURN_IF_EXCEPTION inside the C++ layer, so neither the promise rejection handler nor the try/catch around await ever fires. The result is an effectively leaked Promise and unresponsive code path.
How to fix it
The correct approach — matching how WebKit handles analogous ExistingExceptionError situations — is: (1) check paramsOrException.hasException() first, (2) if the exception is ExistingExceptionError, call scope.clearException() to remove the pending VM exception before calling promise->reject(), then return. This allows promise rejection without triggering the JSDOMPromiseDeferred assertion, and matches the pattern used by every other SubtleCrypto method.
Step-by-step proof
crypto.subtle.digest({ get name() { throw new Error('x') } }, buf)called from JS.- WebIDL glue creates a
DeferredPromiseand callsSubtleCrypto::digest. normalizeCryptoAlgorithmParametersaccesses.name; getter throws; JSC VM exception is set at SubtleCrypto.cpp:112 (insideconvertDictionary); function returnsExistingExceptionError.RETURN_IF_EXCEPTION(scope, void())at line 787 checks scope — pending exception found — returnsvoid().Ref<DeferredPromise>destructor runs; no resolve/reject called.- The JS
Promiseobject the caller isawait-ing will never settle —.catch()never fires,.then()never fires,awaitnever resumes.
| describe("crypto.subtle.digest SHA-3", () => { | ||
| for (const [alg, input, expected] of vectors) { | ||
| it(`${alg}(${JSON.stringify(input)})`, async () => { | ||
| const buf = await crypto.subtle.digest(alg, new TextEncoder().encode(input)); | ||
| expect(hex(buf)).toBe(expected); | ||
| }); | ||
| } | ||
|
|
||
| it("SHA3-256 large input (>64 bytes, async path)", async () => { | ||
| const input = Buffer.alloc(1_000_000, "a"); | ||
| const buf = await crypto.subtle.digest("SHA3-256", input); | ||
| expect(hex(buf)).toBe("5c8875ae474a3634ba4fd55ec85bffd661f32aca75c6d699d0cdcb6c115891c1"); | ||
| }); | ||
|
|
||
| it("rejects unknown digest", async () => { | ||
| await expect(crypto.subtle.digest("SHA3-1024" as any, new Uint8Array())).rejects.toThrow(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🔴 The new test file test/js/web/crypto/web-crypto-sha3.test.ts is missing from test/no-validate-exceptions.txt under the # normalizeCryptoAlgorithmParameters section, causing real ASAN CI failures. The robobun bot already reported the failure: "unchecked exception at reject @ JSDOMPromiseDeferred.cpp:190 (thrown from SubtleCrypto.cpp:112) on x64-asan". Fix: add test/js/web/crypto/web-crypto-sha3.test.ts to that section of test/no-validate-exceptions.txt.
Extended reasoning...
What the bug is and how it manifests
The new test file test/js/web/crypto/web-crypto-sha3.test.ts was not added to test/no-validate-exceptions.txt under the # normalizeCryptoAlgorithmParameters section. This omission is confirmed by actual CI failure: robobun already reported "unchecked exception at reject @ src/bun.js/bindings/webcore/JSDOMPromiseDeferred.cpp:190 (thrown from src/bun.js/bindings/webcrypto/SubtleCrypto.cpp:112) on 🐧 13 x64-asan (new)". This is not a hypothetical failure — it is happening in the current build.
The specific code path that triggers it
The ASAN CI runner sets BUN_JSC_validateExceptionChecks=1, which causes JavaScriptCore to assert (SIGILL) whenever a C++ call returns a JSC exception but the caller has not checked for it via RETURN_IF_EXCEPTION or similar. The new test makes calls to crypto.subtle.digest, crypto.subtle.sign, crypto.subtle.verify, crypto.subtle.generateKey, crypto.subtle.importKey, and crypto.subtle.exportKey — all of which pass through normalizeCryptoAlgorithmParameters in SubtleCrypto.cpp, which can throw JSC exceptions. Some of these call paths lack a DECLARE_THROW_SCOPE / RETURN_IF_EXCEPTION guard (specifically the sign and verify paths), triggering the SIGILL under validateExceptionChecks=1.
Why existing code does not prevent it
The test/no-validate-exceptions.txt file is the project's mechanism for marking test files that exercise these WebCrypto code paths. The sibling file test/js/web/crypto/web-crypto.test.ts is already listed on line 145 of that file for exactly this reason. Every other WebCrypto test file that calls crypto.subtle operations has been added to this list. The new SHA3 test file was simply never added when the PR was authored.
What the impact would be
The bug is not theoretical — the robobun comment documents a live x64-asan CI failure caused by this exact test file. Without the exception entry, every ASAN CI run will SIGILL on this test, producing a false-positive failure that blocks merging and obscures real regressions.
How to fix it
Add one line to test/no-validate-exceptions.txt under the existing # normalizeCryptoAlgorithmParameters section:
test/js/web/crypto/web-crypto-sha3.test.ts
Step-by-step proof
test/no-validate-exceptions.txtline 145 liststest/js/web/crypto/web-crypto.test.tsunder# normalizeCryptoAlgorithmParameters— the sibling file is already excepted.- The new file calls
crypto.subtle.sign(line 52) andcrypto.subtle.verify(lines 56, 58). InSubtleCrypto.cpp,SubtleCrypto::signandSubtleCrypto::verifycallnormalizeCryptoAlgorithmParameterswithout aDECLARE_THROW_SCOPE, so a thrown JSC exception is unchecked. - Under
BUN_JSC_validateExceptionChecks=1, JSC asserts on unchecked exceptions — SIGILL. - robobun Build #45756 confirms the crash on x64-asan with this exact file and stack location.
2 participants
Summary
beafe3db1, ~690 commits, with Bun's patches re-applied on top using a proper merge commit so future syncs don't conflict.crypto.subtle.digest()andsubtle.sign/verifywith HMAC. Wraps BoringSSL's existing internal Keccak via a new EVP-basedCryptoDigestContext.node:cryptocreateHash/createHmac/getHashes(viaEVP.zigandEVP_get_digestbynamefallback inncrypto.cpp).boringssl.translated.zigenv_md_ctx_st.md_datafrom[208]u8to[240]u8to match BoringSSL's newEVP_MAX_MD_DATA_SIZE(the Keccak state is 240 bytes). Without this, Zig-allocated digest contexts read SHA state bytes as thedigestpointer and segfault.SerializedScriptValueso HMAC-SHA3 keys structured-clone, and throughOpenSSLUtilities/CryptoKeyHMACfor key derivation defaults.The new BoringSSL also brings ML-KEM-768/1024 and ML-DSA-44/65/87 (NIST FIPS 203/204) into
EVP_PKEY, sonode:cryptocan support them in a follow-up.Fixes #24075
Test plan
bun test test/js/web/crypto/web-crypto-sha3.test.ts— 15 new tests (NIST FIPS 202 vectors, HMAC-SHA3, large async path)bun test test/js/web/crypto/ test/js/bun/crypto/— 10,269 pass, 0 failbun test test/js/node/crypto/— 797 pass, 0 failHash.copy()+ diverging updates against Pythonhashlib.sha3_256sizeof(EVP_MD_CTX)= 264 in both C and Zig