deps: upgrade BoringSSL to upstream 606d3a344 - #32521
Conversation
Bumps oven-sh/boringssl to 1a41b9025 (merge of google/boringssl@606d3a344, 343 commits since beafe3db1). Fork patches preserved unchanged: SHA-512/224, SHA3 EVP_MD, BLAKE2b-512, RIPEMD160 EVP, HMAC-SHA3, scrypt validation, Electron SSL_want/cipher. Source-list delta from gen/sources.json: +crypto/evp/p_xwing.cc, -crypto/rand/passive.cc. Fixes a latent UAF in UpgradedDuplex::teardown() that the new error-path behavior exposed: handle_reading -> trigger_close_callback -> on_close -> teardown() ran 'self.wrapper = None' while an SSLWrapper::handle_traffic frame was still live with a *mut into the Some payload. The Option assignment runs Drop (which correctly nulls ssl), then memmoves a fresh None value over the slot — whose payload bytes are stack garbage — so the in-flight frame's 'Self::r(this).ssl' read junk and flush_pending_events called SSL_get_ex_data on it. Neuter via deinit() in place instead so the closed_notified/ssl=None guards stay readable; the Option drops for real when DuplexUpgradeContext frees on the next tick. WindowsNamedPipe already handles this with WRAPPER_BUSY. Raises the ASAN/debug RSS bound in tls-connect-socket-churn.test.ts: the sslCtxLiveCount regression guard and LSAN are clean, but BoringSSL's larger per-handshake allocation churn (PQ key shares) sits in the ASAN quarantine when this file runs after the rest of the tls/ suite. Adds .claude/commands/upgrade-boringssl.md documenting the procedure.
|
Updated 9:07 PM PT - Jun 19th, 2026
✅ @Jarred-Sumner, your commit d3ac55b221c82571b1f09b80e2e0ddf1890ab62f passed in 🧪 To try this PR locally: bunx bun-pr 32521That installs a local version of the PR into your bun-32521 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughBumps the pinned BoringSSL commit SHA, updates the compiled crypto source list (adds ChangesBoringSSL Upgrade
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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: 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 @.claude/commands/upgrade-boringssl.md:
- Around line 52-56: The runbook step in upgrade-boringssl.md currently uses the
bun -e flag directly, which bypasses the repository's documented debug-build
command flow. Replace the bun -e invocation with bun bd -e to conform to the
coding guidelines that require all bun commands to use the debug-build command
wrapper instead of executing bun directly.
🪄 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: 129b8bb3-b460-489e-a1af-219a03648b17
📒 Files selected for processing (4)
.claude/commands/upgrade-boringssl.mdscripts/build/deps/boringssl.tssrc/runtime/socket/UpgradedDuplex.rstest/js/node/tls/tls-connect-socket-churn.test.ts
| bun -e 'const j=require("./vendor/boringssl/gen/sources.json"); | ||
| const f=l=>l.map(JSON.stringify).join(", "); | ||
| for(const k of ["bcm","crypto","ssl","decrepit"]) console.log(k,"\n",f(j[k].srcs)); | ||
| console.log("asm\n",f([...j.bcm.asm,...j.crypto.asm])); | ||
| console.log("nasm\n",f([...j.bcm.nasm,...j.crypto.nasm]))' |
There was a problem hiding this comment.
Use bun bd -e in this runbook step.
This snippet currently uses bun -e, which bypasses the repo’s documented debug-build command flow.
Suggested doc fix
- bun -e 'const j=require("./vendor/boringssl/gen/sources.json");
+ bun bd -e 'const j=require("./vendor/boringssl/gen/sources.json");
const f=l=>l.map(JSON.stringify).join(", ");
for(const k of ["bcm","crypto","ssl","decrepit"]) console.log(k,"\n",f(j[k].srcs));
console.log("asm\n",f([...j.bcm.asm,...j.crypto.asm]));
console.log("nasm\n",f([...j.bcm.nasm,...j.crypto.nasm]))'As per coding guidelines: “Never use bun test or bun <file> directly - always use bun bd ....”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bun -e 'const j=require("./vendor/boringssl/gen/sources.json"); | |
| const f=l=>l.map(JSON.stringify).join(", "); | |
| for(const k of ["bcm","crypto","ssl","decrepit"]) console.log(k,"\n",f(j[k].srcs)); | |
| console.log("asm\n",f([...j.bcm.asm,...j.crypto.asm])); | |
| console.log("nasm\n",f([...j.bcm.nasm,...j.crypto.nasm]))' | |
| bun bd -e 'const j=require("./vendor/boringssl/gen/sources.json"); | |
| const f=l=>l.map(JSON.stringify).join(", "); | |
| for(const k of ["bcm","crypto","ssl","decrepit"]) console.log(k,"\n",f(j[k].srcs)); | |
| console.log("asm\n",f([...j.bcm.asm,...j.crypto.asm])); | |
| console.log("nasm\n",f([...j.bcm.nasm,...j.crypto.nasm]))' |
🤖 Prompt for 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.
In @.claude/commands/upgrade-boringssl.md around lines 52 - 56, The runbook step
in upgrade-boringssl.md currently uses the bun -e flag directly, which bypasses
the repository's documented debug-build command flow. Replace the bun -e
invocation with bun bd -e to conform to the coding guidelines that require all
bun commands to use the debug-build command wrapper instead of executing bun
directly.
Source: Coding guidelines
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… bound to 2x headroom Also adds the process.test.js step to the upgrade-boringssl skill so it isn't missed next time.
| import { depSourceDir } from "../source.ts"; | ||
|
|
||
| const BORINGSSL_COMMIT = "0c5fce43b7ed5eb6001487ee48ac65766f5ddcd1"; | ||
| const BORINGSSL_COMMIT = "1a41b9025c2c0a37edd07ff10f6944f03e028522"; |
There was a problem hiding this comment.
🟡 This bump restores SSL_OP_LEGACY_SERVER_CONNECT as a real flag (0x4) and defines SSL_OP_ALL = SSL_OP_LEGACY_SERVER_CONNECT, but Bun's hand-mirrored constants in src/boringssl_sys/boringssl.rs:552-554 and src/boringssl_sys/boringssl.zig:18214,18218 still hardcode 0 (with a now-incorrect "BoringSSL defines this as 0 (no-op flag)" comment). There's no runtime regression today because the new BoringSSL applies this flag by default at SSL_CTX_new, so configure_http_client_with_alpn's SSL_clear_options(ssl, 0)/SSL_set_options(ssl, 0) no-op is harmless — but the comment is now wrong, crypto.constants.SSL_OP_LEGACY_SERVER_CONNECT (C++ #includes the real header) will expose 4 while the Rust/Zig FFI uses 0, and upstream's TODO(crbug.com/41393419) to disable the flag by default would turn this into a real fetch() break on a future bump. Suggest updating both constants to 0x4 and adding an "SSL_OP_* constant mirrors" bullet to the new runbook's §Things that have broken before.
Extended reasoning...
What changed upstream
This PR bumps BORINGSSL_COMMIT from 0c5fce43b to 1a41b9025 (a merge of google/boringssl@606d3a344). One of the upstream changes — called out in this PR's own description as "Restored SSL_OP_LEGACY_SERVER_CONNECT (and fixed it for TLS 1.3), added SSL_OP_ALL defaults" — moved two macros out of the "deprecated, defined as zero" block in include/openssl/ssl.h:
| Macro | Old (0c5fce43b, ssl.h:5831/5835) |
New (1a41b9025, ssl.h:605/622) |
|---|---|---|
SSL_OP_LEGACY_SERVER_CONNECT |
0 |
0x00000004L |
SSL_OP_ALL |
0 |
SSL_OP_LEGACY_SERVER_CONNECT |
What's stale in Bun
Bun hand-mirrors these constants in two places that this PR did not touch:
-
src/boringssl_sys/boringssl.rs:552-554:/// `SSL_OP_LEGACY_SERVER_CONNECT` — BoringSSL defines this as 0 (no-op flag); /// kept so callers can mirror the OpenSSL clear/set dance verbatim. pub const SSL_OP_LEGACY_SERVER_CONNECT: u32 = 0;
The comment is now factually wrong.
-
src/boringssl_sys/boringssl.zig:18214,18218:pub const SSL_OP_ALL = @as(c_int, 0); pub const SSL_OP_LEGACY_SERVER_CONNECT = @as(c_int, 0);
These flow into configure_http_client_with_alpn — run on every outgoing fetch()/WebSocket TLS open — at src/http/lib.rs:940-941 and src/boringssl_sys/boringssl.zig:19068-19069:
boringssl::c::SSL_clear_options(ssl, boringssl::c::SSL_OP_LEGACY_SERVER_CONNECT);
boringssl::c::SSL_set_options(ssl, boringssl::c::SSL_OP_LEGACY_SERVER_CONNECT);The function's doc comment says it "Sets … the legacy-server-connect option", but with the constant still 0 this is now SSL_clear_options(ssl, 0) + SSL_set_options(ssl, 0) — a pure no-op.
Why there's no runtime regression today
The new ssl.h doc for SSL_OP_LEGACY_SERVER_CONNECT explicitly states "This option is enabled by default", and the new SSL_OP_ALL doc says it "is not necessary to pass". Concretely, ssl/internal.h:3985 now initializes SSL_CTX::options = SSL_OP_ALL, and ssl_lib.cc:476 inherits ctx options into each SSL. So:
SSL_CTX_new()sets bit0x4on the context.SSL_new()copies it onto the connection.- Bun's
configure_http_client_with_alpncallsSSL_clear_options(ssl, 0)→ clears nothing → bit0x4stays set. SSL_set_options(ssl, 0)→ ORs in nothing → bit0x4stays set.- Handshake against a non-RFC5746 server proceeds exactly as before.
No SSL_CTX_clear_options caller elsewhere in Bun clears bit 4, so the inherited default survives. fetch() to legacy servers continues to work — the Rust/Zig code is correct only by coincidence of upstream's current default.
What is real
- Wrong comment —
boringssl.rs:552now lies about what BoringSSL defines. - Internal/external divergence —
crypto.constants.SSL_OP_LEGACY_SERVER_CONNECTis exposed viaNodeConstantsModule.h:794/ProcessBindingConstants.cpp:822, which#includethe realopenssl/ssl.hand will now return4to JS, while the Rust/Zig FFI passes0internally. This is cosmetic today (JS-suppliedsecureOptionsreaches BoringSSL via the C path with the real value), but it's exactly the kind of skew the bindings file is supposed to prevent. - Latent break on the next bump — the new header carries
TODO(crbug.com/41393419): Disable SSL_OP_LEGACY_SERVER_CONNECT by defaultatssl.h:621. When upstream lands that, step (1) above stops setting bit0x4, Bun's no-op clear/set still does nothing, andfetch()against non-RFC5746 servers starts failing withunsafe legacy renegotiation disabled— even though Bun's code intends to allow it. - Runbook gap — this PR's new
.claude/commands/upgrade-boringssl.md§"Things that have broken before" lists theasn1_string_st/GENERAL_NAME_ststruct mirrors but not the hardcodedSSL_OP_*constants inboringssl.rs/boringssl.zig. This is precisely the silent-drift class that section is meant to catch.
Suggested fix
// src/boringssl_sys/boringssl.rs
/// `SSL_OP_LEGACY_SERVER_CONNECT` — allow initial connections to servers that
/// don't support RFC 5746 secure renegotiation. BoringSSL applies this in
/// `SSL_OP_ALL` by default; mirrored so callers can clear/set explicitly.
pub const SSL_OP_LEGACY_SERVER_CONNECT: u32 = 0x00000004;// src/boringssl_sys/boringssl.zig
pub const SSL_OP_LEGACY_SERVER_CONNECT = @as(c_int, 0x00000004);
pub const SSL_OP_ALL = SSL_OP_LEGACY_SERVER_CONNECT;And add to upgrade-boringssl.md §"Things that have broken before":
- **`SSL_OP_*` constant values** — Bun mirrors a handful in `src/boringssl_sys/boringssl.{rs,zig}`; diff `include/openssl/ssl.h` for value changes (`SSL_OP_LEGACY_SERVER_CONNECT` went 0→4 in the 606d3a344 bump).Marking as a nit since there's no behavioral regression in this PR — but worth a one-line cleanup commit so the next BoringSSL bump doesn't silently break fetch().
2 participants
Bumps
oven-sh/boringsslto1a41b9025, a merge ofgoogle/boringssl@606d3a344— 343 commits since the previous merge-basebeafe3db1.Fork patches preserved unchanged (SHA-512/224, SHA3 EVP_MD, BLAKE2b-512, RIPEMD160 EVP, HMAC-SHA3,
EVP_PBE_validate_scrypt_params, ElectronSSL_want/cipher). All conflicts were upstream's|...|→`...`doc-comment restyle landing adjacent to our additions.gen/sources.jsondelta:+crypto/evp/p_xwing.cc,-crypto/rand/passive.cc.Upstream highlights
TLS / libssl
SSL_get_signature_algorithm_used,SSL_CTX_set1_available_trust_anchors; Merkle Tree Certificate (plants-04) verification in libpki.SSL_OP_LEGACY_SERVER_CONNECT(and fixed it for TLS 1.3), addedSSL_OP_ALLdefaults, removedSSL_set_enforce_rsa_key_usage, addedTLS_RSA_WITH_AES_256_GCM_SHA384to the CNSA 1.0 compliance policy.SSL_CTX,SSL_ECH_KEYS,SSL_CREDENTIALare now opaque structs;CRYPTO_BUFFER_POOLis now reference-counted.Crypto primitives
EVP_KEM; new genericEVP_PKEYKEM encap/decap adapter;EVP_HPKE_KEY_derive.EVP_CIPHER_CTX_max_next_update/_max_final.ASN.1 / X.509 / PEM
ASN1_BIT_STRING_set1/_unused_bits(implicit truncation removed); tightened URI name-constraint matching; libpki now defaults to a path-building iteration limit of 20.PEM_read/PEM_write, unexportedIMPLEMENT_PEM_*, removedPEM_TYPE_*, capped PEM data at 1 GiB.Security / correctness
max_early_data_size> 2^16 handling,beeu_mod_inverse_vartimeon aarch64,EVP_AEAD_CTX_seal_scatterbounds check, several X.509/OBJ/BN edge-case bugs.API / misc
BIO_write_ex,CBS_get_u48,CBS_peek_any_asn1_tag,sk_FOO_sort_and_dedup; fixed non-blocking connect BIOs,BIO_find_type,BIO_set_retry_special.rfork().BORINGSSL_API_VERSIONbumped.Bun-side changes
src/runtime/socket/UpgradedDuplex.rs— fixes a latent UAF the upgrade exposed.handle_reading→trigger_close_callback→on_close→teardown()ranself.wrapper = Nonewhile anSSLWrapper::handle_trafficframe was still live with a*mutinto theSomepayload. TheOptionassignment runsDrop(which correctly nullsssl), then memmoves a freshNonevalue over the slot — whose payload bytes are stack garbage — so the in-flight frame'sSelf::r(this).sslread junk andflush_pending_eventscalledSSL_get_ex_dataon it (SEGV inOPENSSL_sk_value).WindowsNamedPipealready guards this withWRAPPER_BUSY; here the simpler fix is todeinit()in place so thessl=None/closed_notifiedguards stay readable, and let theOptiondrop whenDuplexUpgradeContextfrees on the next tick.test/js/node/tls/tls-connect-socket-churn.test.ts— raises the ASAN/debug RSS bound (64 → 192 MB). ThesslCtxLiveCountregression guard and LSAN are both clean; the new BoringSSL just has more per-handshake allocation churn (PQ key shares, transcript buffers) that sits in the ASAN quarantine when this file runs after the rest oftest/js/node/tls/..claude/commands/upgrade-boringssl.md— documents this procedure for next time.