Conversation
|
Updated 1:22 AM PT - Aug 28th, 2026
❌ @robobun, your commit e173a17 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32129That installs a local version of the PR into your bun-32129 --bun |
WalkthroughThis PR fixes an AES-GCM correctness bug in BoringSSL where VAES (AVX-512 Vector AES) implementations could silently produce incorrect ciphertext. The fix gates VAES selection on hardware AES support, a patch is wired into the build, and a regression test validates the fix across different CPU capability masks. ChangesAES-GCM VAES capability-gating fix and regression test
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides a detailed explanation of the bug, root cause, fix, reproduction steps, verification results, and affected files. It uses equivalent sections instead of the exact template headings, but it fully covers the required information. 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: 1
🤖 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 879-901: Test must assert the VAES/x64 precondition before
validating masked vs unmasked behavior: detect whether the runner is x64 and
supports VAES (or whether runWithCaps(undefined) reports VAES-capable behavior)
and if not, skip the regression check; modify the test around runWithCaps / the
unmasked/aesniMasked/bothMasked comparison to assert the precondition (e.g.,
require process.arch==='x64' and a VAES capability flag or a runtime check via
runWithCaps result) and call test.skip or return early when the precondition is
not met so the masked-case comparisons only run on hosts where VAES would change
the implementation.
🪄 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: 7575454a-e6d0-4808-9ed6-667478db307a
📒 Files selected for processing (3)
patches/boringssl/gcm-vaes-requires-hwaes-key-schedule.patchscripts/build/deps/boringssl.tstest/js/node/crypto/node-crypto.test.js
|
CI status for the rebased head (e173a17, build 107511): 180 of 181 jobs passed. The one red lane is macOS x64 failing test/js/web/url/url.test.ts ("special-scheme hosts use the Unicode 16 IDNA table", TypeError: Invalid URL). That test is unrelated to this change and fails on main's code path for ICU IDNA tables; it has been reported for main-break triage. The only other blip was a known-flaky inspector-protocol test that passed on retry. The AES-GCM test and the node:crypto shards passed on every lane. The branch is rebased onto the BoringSSL 606d3a344 bump (#32521), which still predates the upstream fix, and the patch applies cleanly to the new pin. Ready for review. |
|
@robobun what if we upgraded boringssl to the latest upstream version? does it contain a fix? |
|
Checked just now against upstream google/boringssl main at 0676117 (committed today, 2026-06-11): no, it does not contain a fix. CRYPTO_gcm128_init_aes_key still selects the VAES implementations on CRYPTO_is_VAES_capable() alone, without checking is_hwaes: The aarch64 branches right below gate on is_hwaes, so this looks like an oversight in the x86-64 branches rather than a design choice. So an upgrade alone would not fix this, and the patch stays necessary either way. Two things that make the patch cheap to carry across a bump:
Still worth reporting on the BoringSSL tracker so the patch can eventually go away. |
|
@Jarred-Sumner I reported this upstream to BoringSSL/Google, and it has now been fixed: https://issues.chromium.org/issues/552666770 BoringSSL fix: Since Bun statically links its vendored BoringSSL, updating Bun's BoringSSL revision to include this fix ( |
|
Thanks for reporting it upstream. I verified the upstream fix (997bafbf, https://boringssl-review.googlesource.com/c/boringssl/+/101787): it is the same change this PR carries, an is_hwaes gate on the two x86-64 VAES branches in CRYPTO_gcm128_init_aes_key. On taking it via a revision bump instead of this patch: Bun pins the oven-sh/boringssl fork, so picking up 997bafbf means a fork sync that pulls in months of unrelated upstream changes plus a regeneration of the build source lists. That is a much larger change than this bug needs, and it deserves its own PR and full CI run. This PR fixes the bug now with the same one-line-per-branch gate. The patch header and the dep comment now cite 997bafbf (commit 5fc83f7). When a future BoringSSL bump includes that commit, the patch stops applying and dep-fetch fails loudly, which is the signal to delete the patch file. So the fix cannot silently duplicate or rot. |
…VAES BoringSSL's CRYPTO_gcm128_init_aes_key picked the stitched VAES AES-GCM implementation (gcm_x86_vaes_avx2/avx512) from CPU capability bits alone, without checking that the AES key schedule was produced by aes_hw_set_encrypt_key. When OPENSSL_ia32cap masks AES-NI but leaves VAES set, the key schedule comes from vpaes (transformed layout) and the VAES bulk assembly silently produces wrong ciphertext, and decrypts garbage whose tag still verifies. Add the same is_hwaes gate the aarch64 branches already use, shipped as a vendor patch applied at dep fetch time. Upstream BoringSSL main has the same bug. Fixes #32126
Upstream fixed the same bug in google/boringssl 997bafbf73886ec19ac3eb70b31db64fe42832c3. Point the patch header and the dep comment at that commit so the patch is deleted when a future pin bump includes it.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it patches a vendored crypto path (BoringSSL AES-GCM dispatch), a human look is still worthwhile before merging.
What was reviewed:
- The patch adds
&& is_hwaesto both x86-64 VAES branches ingcm.cc.inc, matching the aarch64 branches directly below and upstream BoringSSL 997bafbf7388 — no other behavior touched. - The
patches: [...]entry inscripts/build/deps/boringssl.tsfollows the same mechanism used by other deps; patch application will fail loudly once the pinned commit already contains the fix. - The regression test spawns subprocesses (OPENSSL_ia32cap is read once at startup), spreads
bunEnv, drains pipes concurrently, and asserts against published NIST GCM test vectors 2/8/14 rather than relying on self-round-trip alone.
Extended reasoning...
Overview
This PR adds a small vendored patch to BoringSSL fixing AES-GCM implementation dispatch on x86-64. The bug: when OPENSSL_ia32cap masks AES-NI but leaves VAES enabled, CRYPTO_gcm128_init_aes_key selected the stitched VAES bulk assembly while the key schedule was built by vpaes (which uses a transformed round-key basis), producing silently wrong ciphertext whose auth tag still verified. The two-line patch adds && is_hwaes to the two x86-64 VAES selection branches, mirroring both the adjacent aarch64 branches and the identical upstream BoringSSL fix (997bafbf7388). Three files changed: the new patch under patches/boringssl/, a patches: [...] registration in scripts/build/deps/boringssl.ts, and a regression test appended to test/js/node/crypto/node-crypto.test.js.
Security risks
The change touches a cryptographic primitive, which is inherently security-sensitive. However, the risk profile is low: the patch is additive (a stricter gate that can only cause fallback to a slower-but-correct implementation), it mirrors upstream verbatim, and the failure mode being fixed is "silently wrong crypto" — the worst possible outcome — so leaving it unfixed is the greater risk. On real hardware with consistent capability bits (VAES implies AES-NI), the added is_hwaes condition is always true, so unmasked runs are unchanged. I did not identify any way this patch could weaken crypto on any input.
Level of scrutiny
High, per the approval guidelines for crypto paths. Even though the diff is small and mechanically matches an accepted upstream commit, changes to AES-GCM dispatch warrant a human maintainer's sign-off rather than automated approval. The patches: mechanism is well-established in this repo (used by 12 other deps), and the comment in boringssl.ts documents the upstream commit and self-obsoleting condition, satisfying the vendoring conventions in CLAUDE.md.
Other factors
The test is well-constructed against the review checklist: it appends to the existing node-crypto.test.js rather than creating a new file, carries the issue-URL comment, spawns fresh subprocesses because OPENSSL_ia32cap is read once at process init, spreads bunEnv, drains stdout/stderr/exited concurrently, and asserts exact known-answer vectors from the McGrew & Viega GCM spec (test cases 2/8/14) plus an independent decrypt of the authentic NIST ciphertext — addressing the "self-round-trip proves nothing for codecs" rule. The multi-block case is compared across all three capability masks. On non-x86-64 or non-VAES CI lanes the masked runs dispatch identically and pass trivially, so the test is safe everywhere. The PR description states the test fails against unpatched builds with the exact bytes from the issue report.
5fc83f7 to
e173a17
Compare
This branch has not been deployed
3 participants
Fixes #32126
Repro
On any x86-64 CPU with VAES (the reporter's i5-13420H, or any recent Intel/AMD), with the issue's NIST-test-vector script:
produces wrong AES-GCM output on every released build (reproduced with bun-linux-x64 1.3.14 and current main, same bytes as the Windows report):
decipher.final()does not throw: decrypting the authentic NIST ciphertext yields garbage whose tag still verifies.Cause
~0x200000000000000clears AES-NI (CPUID.1:ECX bit 25) from the first OPENSSL_ia32cap word but leaves VAES/VPCLMULQDQ (CPUID.7:ECX bits 9-10) set. In vendored BoringSSL,crypto/fipsmodule/aes/gcm.cc.incCRYPTO_gcm128_init_aes_key:aes_ctr_set_keyseeshwaes_capable()false and builds the key schedule withvpaes_set_encrypt_key, which stores round keys in a transformed basis (is_hwaes = 0).gcm_x86_vaes_avx2/avx512based only on the ghash choice andCRYPTO_is_VAES_capable(), without checkingis_hwaes.aes_hw_set_encrypt_keyround-key layout, so it encrypts with a misinterpreted key schedule: deterministic garbage ciphertext. H and EK0 still come from the consistent vpaes block function, so tags verify over the garbage.The aarch64 branches directly below gate on
is_hwaes; the two x86-64 VAES branches are missing the same gate. Upstream BoringSSL has since fixed this the same way in 997bafbf, after the reporter filed it upstream. The commit Bun pins (upstream 606d3a344, via #32521) predates that fix, so the patch is still required. It only fires when capability bits are split artificially via OPENSSL_ia32cap, but that is exactly what users do to work around CPU errata (e.g. the VAES masking suggested in #32124), and silently wrong crypto is the worst possible failure mode. Bothnode:cryptocipheriv and WebCrypto AES-GCM funnel through this init.Node is unaffected because OpenSSL keys its stitched paths off AES-NI.
Fix
patches/boringssl/gcm-vaes-requires-hwaes-key-schedule.patch, applied at dep fetch time via the existingpatchesmechanism: addis_hwaesto the two x86-64 VAES impl branches, mirroring the aarch64 branches and the upstream fix. Behavior on consistent capability sets is unchanged (real VAES hardware always has AES-NI). When a future BoringSSL bump includes the upstream fix, the patch stops applying and dep fetch fails loudly. That is the signal to delete the patch file.Verification
New test in
test/js/node/crypto/node-crypto.test.jsspawns subprocesses with the masks set (the env var is read once at startup) and checks GCM spec test vectors for AES-128/192/256-GCM, the decrypt-of-authentic-ciphertext case, and that multi-block output is identical across masked/unmasked runs. On CPUs without VAES the masked runs dispatch identically and pass trivially, so it is safe on all CI lanes.USE_SYSTEM_BUN=1 bun test ... -t "AES-GCM stays correct": fails with the exact bytes from the issue (all three key sizes corrupted)bun bd test test/js/node/crypto/node-crypto.test.js: all pass (213 after the rebase onto the BoringSSL 606d3a344 bump, which still lacks the upstream fix; the patch applies cleanly to the new pin)test/js/web/crypto/web-crypto.test.tsunder the mask: 10 passbun bdwith the mask now matches the NIST vectorNote for reviewers: the production change lives in
patches/+scripts/build/deps/boringssl.ts(vendored dep patch), notsrc/, so fail-before verification means running the test against a build without the patch (or any released bun).[stamp-90s] gate passed · iteration 6 · 3 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 6
evidence per changed file
root cause · written by the author bot
The bug was in the vendored BoringSSL's AES-GCM dispatch, where
CRYPTO_gcm128_init_aes_keyselected the VAES bulk implementation based solely onCRYPTO_is_VAES_capable(), without confirming that hardware AES was also available. WhenOPENSSL_ia32cap=~0x200000000000000masked the AES-NI bit on a VAES-capable CPU, the key schedule was generated by the vpaes fallback, whose layout is incompatible with the VAES assembly, so encryption silently produced incorrect ciphertext and tags. The fix patches the dispatch to additionally requireis_hwaesbefore taking the VAES path, ensuring the VA…