node:tls: give the exported SecureContext constructor its own SSL_CTX - #33199
Conversation
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
4a3004f to
9b27850
Compare
488cd7d to
e601de3
Compare
9b27850 to
94c4614
Compare
e601de3 to
73206e1
Compare
94c4614 to
0cccf6a
Compare
73206e1 to
892c2de
Compare
0cccf6a to
a979f3d
Compare
892c2de to
f6022cc
Compare
new tls.SecureContext(options) handed back a wrapper around the digest-interned native SSL_CTX, so addCACert() on one instance silently extended the trust store of every context sharing that configuration digest (Node keeps contexts isolated and fails such a connection closed). createSecureContext() already built a private context for exactly this reason; the exported constructor now delegates to it, and the comments that enumerated createSecureContext as the only exclusively-owned constructor are updated so the invariant cannot rot.
a979f3d to
b7b606e
Compare
|
I retriggered CI |
There was a problem hiding this comment.
The fix itself is correct and should land — one architectural concern and two smaller follow-ups.
Should-fix
This is the third caller-side patch for "don't mutate a shared SSL_CTX" — the invariant belongs in add_ca_cert, not in every JS entry point that must remember cached:false. Interned handles are still user-reachable post-PR via Symbol.for("::buntlsnativesecurecontextctor::") (registered symbol → any user can recreate it), and via TLSSocket[ksecureContext] (enumerable via getOwnPropertySymbols). Poisoning one also extends the trust store of Bun-native Bun.listen/Bun.connect sockets that share the digest cache.
Recommendation: add shared: bool to SecureContext (SecureContext.rs:35-44); set true in create_with_digest/intern, false in create_private. In add_ca_cert (the only proto mutator), check it first and either throw ("cannot mutate a shared SecureContext; use tls.createSecureContext()") or COW. Separately, either delete the Symbol.for("::buntlsnativesecurecontextctor::") exposure or route the native constructor() through create_private.
Consider
The bug this PR fixes exists because the safe behavior is opt-in — flipping cached's default to false (with the 3 internal hot-path sites at tls.ts:921/:1111/:1299 opting in explicitly) would make the next forgotten entry point a perf regression instead of a trust-store leak. Node has no digest cache at all, so cached=false is the Node-matching default. ~5-line follow-up.
Also looked at and ruled out: the instanceof tls.SecureContext gap (real but pre-existing, this PR doesn't change it), and cross-cache isolation between private and interned (the existing test at :233-255 already catches that regression).
Adds a `shared` bit to the native SecureContext and has add_ca_cert throw when set, so a stray user-reachable interned handle (Symbol.for constructor, TLSSocket internals) can never poison the per-digest SSL_CTX cache. The user-facing constructors already produce private contexts; this closes the class at the mutator instead of at every entry point. Also pins rejectUnauthorized: true in the new e2e test so its outcome is not env-sensitive, matching the sibling connect calls in the same file.
|
Pushed f1c9041 addressing the should-fix ( |
…rivate SSL_CTX The digest cache is now opt-in: the internal connect/listen paths pass true explicitly. A future entry point that forgets to opt in becomes a perf regression, not a shared trust store.
What
new tls.SecureContext(options)(the exported constructor) returned a wrapper around the digest-interned, shared nativeSSL_CTX. Mutating it therefore mutated every other context with the same configuration digest:Against a server whose chain roots in
ca1(not in the default roots), node v26.3.0 fails closed (UNABLE_TO_GET_ISSUER_CERT_LOCALLY) while Bun completed the handshake withauthorized=true— an extra CA silently trusted by connections that never asked for it. (Surfaced by a security scan of this branch; reproduced end-to-end against both runtimes before fixing.)createSecureContext()already builds a private context for exactly this reason ("a user-constructed context owns its SSL_CTX exclusively, so addCACert can never leak across contexts"); the exported constructor was the one user-constructible path that missed the invariant. It now passescached: falsetoo. Internal paths (tls.connect/Server/fetch) keep the shared cache: their contexts are never exposed for mutation, and that sharing is the cache's purpose.Regression tests (both in
ssl-ctx-cache.test.ts, next to theircreateSecureContextsiblings): (1) twonew tls.SecureContext()instances with identical options get distinct native handles — the interned cache handed both the same cell before the fix; (2) the end-to-end scenario above — aftera.context.addCACert(ca1), a connection usingbstill fails with node's exactUNABLE_TO_GET_ISSUER_CERT_LOCALLY(on the unfixed code it completed withauthorized=true).The other scan findings that touch this PR stack's files (triaged, not fixed here)
Each was verified against the sources before deciding:
upgradeTLSinitialDataused after re-entrant JS can free the backing store (socket_body.rs): real, pre-existing onmain— and since fixed upstream by socket: fix use-after-free when upgradeTLS is called inside the socket open handler #33388, so nothing is owed here anymore.openssl.c): the resumption arm pre-exists onmainand is a faithful port of Node's ownVerifyPeerCertificate(src/crypto/crypto_common.cc), which Node's server path also uses — so Bun matches Node here by construction. Hardening beyond Node would be a deliberate divergence decision.authorized=truedespite failed chain verification (two findings,socket_body.rson_handshake): the scan itself notes the computation is byte-identical to upstream Bun; it concerns the Bun-nativeBun.connect/Bun.listensocket API (node:tls and fetch enforce verification in their own layers). Changing a Bun-native API contract needs a maintainer decision, not a drive-by in a node:tls compat stack.Notes from the pre-PR review pass
createSecureContext()(one source of truth for the ownership contract), and the three comments that enumeratedcreateSecureContextas the only exclusively-owned constructor were updated so the enumeration cannot rot into a regression.addCACertis still reachable if code deliberately digs out the registered internal symbol (::buntlsnativesecurecontextctor::); that is outside the public API surface.Rebased onto the updated base branch (which itself was rebased onto current
main).