http: fix use-after-free in onHandshake when checkServerIdentity rejects (u9sf0l)#29829
Conversation
When the native checkServerIdentity() rejects the peer certificate it calls closeAndFail() -> fail() -> result callback, which can destroy the AsyncHTTP (and its embedded HTTPClient). onHandshake then wrote to client.flags and called client.unregisterAbortTracker() on freed memory. Both operations were already performed inside closeAndFail/fail, so the fix is to simply return after a false result. Applied to both HTTPContext.zig and ProxyTunnel.zig. While here, add the related Node.js-compat pieces the grouped tests exercised once the crash was out of the way: - checkX509ServerIdentity now falls back to the Subject CN when the certificate carries no DNS/IP/URI subjectAltName entries, matching Node's tls.checkServerIdentity. - https.request forwards a user-supplied checkServerIdentity to the native fetch layer. - https.Server forwards requestCert/rejectUnauthorized so a server with ca but no requestCert: true no longer rejects clients that don't present a client certificate.
|
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 (2)
WalkthroughRefactors TLS hostname verification to centralize DNS/wildcard matching and CN fallback behavior, caches IP-literal checks, adjusts handshake failure control flow to rely on checkServerIdentity side effects, propagates TLS checkServerIdentity and normalizes server client-cert options, and adds Node-compatible HTTPS tests. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 2:23 PM PT - Apr 28th, 2026
❌ @Jarred-Sumner, your commit 6463c7c has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29829That installs a local version of the PR into your bun-29829 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Status
CI (build #48524)
None of these touch files modified by this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/boringssl.zig`:
- Around line 131-137: The code uses std.mem.indexOfScalar for wildcard label
checks; replace those calls with the bun.strings equivalent to comply with the
repo policy. Specifically, update the two usages inside the hostname/suffix
check (the call on suffix and the call on hostname[0..dot_index]) to the
bun.strings string-search API (matching the same semantics and return-value
checks) so the logic around dot_index, the dot comparison, and the single-label
wildcard constraint remains identical but uses bun.strings instead of std.mem.
In `@src/js/node/_http_server.ts`:
- Around line 253-271: The TLS normalization done in the constructor (using
isTlsSymbol/tlsSymbol and computing requestCert and rejectUnauthorized from
options.requestCert/options.rejectUnauthorized) must also be applied when
Server.prototype.listen receives a tls option; update the listen path that
replaces tls with arguments[0].tls to run the same normalization logic (compute
requestCert = !!tls.requestCert and rejectUnauthorized = requestCert ?
tls.rejectUnauthorized !== false : false) and then assign the normalized object
to this[tlsSymbol] or forward that normalized object into Bun.serve(), so
callers using server.listen({ tls: { key, cert, ca } }) do not inadvertently
require client certificates.
🪄 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: 4b892ead-c40e-425d-bdf5-250d019f1214
📒 Files selected for processing (6)
src/boringssl.zigsrc/http/HTTPContext.zigsrc/http/ProxyTunnel.zigsrc/js/node/_http_client.tssrc/js/node/_http_server.tstest/js/node/http/node-https-checkServerIdentity.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The UAF fix and option-forwarding look correct, but the CN-fallback in checkX509ServerIdentity and the normalizeServerTls change to rejectUnauthorized defaults are security-sensitive enough that a human should sanity-check them.
Extended reasoning...
Overview
This PR bundles four related fixes: (1) a UAF fix in HTTPContext.onHandshake / ProxyTunnel.onHandshake that removes writes to a freed HTTPClient after checkServerIdentity rejects, (2) a Subject-CN fallback in checkX509ServerIdentity (src/boringssl.zig) when the certificate carries no DNS/IP/URI SAN entries, (3) forwarding of the user-supplied checkServerIdentity callback in _http_client.ts, and (4) a new normalizeServerTls() in _http_server.ts that defaults requestCert to false and forces rejectUnauthorized: false when not requesting a client cert.
Security risks
Two of the four changes directly alter TLS trust decisions. The CN fallback widens the set of certificates accepted by the native hostname check — it is gated on !has_identifier_san (only GEN_DNS/GEN_IPADD/GEN_URI count) and !host_is_ip, which mirrors Node's tls.checkServerIdentity, but a subtle miss in the SAN-type accounting or the extracted matchDnsName wildcard logic would weaken hostname verification for every fetch()/https.request in Bun. The normalizeServerTls change flips an HTTPS server with { ca } from rejecting cert-less clients to accepting them; that is the Node-compatible behaviour, but it is still a deliberate relaxation of server-side TLS auth that deserves a second pair of eyes.
Level of scrutiny
High. checkX509ServerIdentity is on the trust boundary for all outbound TLS in the runtime, and _http_server.ts TLS defaults affect inbound auth. The UAF fix and checkServerIdentity forwarding are mechanically simple and well-explained, but they are bundled with the security-relevant pieces.
Other factors
CodeRabbit's two findings were addressed in 8dbbe45. CI passed the new test on every lane and the remaining failures are unrelated infra. The only bug-hunter finding is a test-style nit (describe.concurrent). Given the security surface, I'm deferring rather than approving.
| ca: readFileSync(join(keys, "ca1-cert.pem"), "utf8"), | ||
| }; | ||
|
|
||
| describe("https.request checkServerIdentity", () => { |
There was a problem hiding this comment.
🟡 nit: per test/CLAUDE.md, tests that each spawn an independent subprocess should use describe.concurrent. All four tests here spawn isolated bunExe() children with port: 0 and no shared state, so this block can be describe.concurrent("https.request checkServerIdentity", …) to run them in parallel.
Extended reasoning...
What
test/CLAUDE.md (line 22) states:
Prefer concurrent tests over sequential tests: When multiple tests in the same file spawn processes or write files, make them concurrent with
test.concurrentordescribe.concurrentunless it's very difficult to make them concurrent.
This file declares its suite with plain describe("https.request checkServerIdentity", …) containing four test(...) cases, so they execute serially.
Why it applies here
Each of the four tests is fully self-contained:
- "hostname mismatch emits error without crashing" —
Bun.spawn([bunExe(), "-e", …]), child creates its ownhttps.createServeronport: 0. - "falls back to Subject CN when no SAN is present" — same pattern, own subprocess, own ephemeral port.
- "custom checkServerIdentity overrides the native check" — same pattern.
- "https.Server with ca but no requestCert accepts clients without a cert" — same pattern.
There is no shared mutable state at the test-file level (the only module-level values are the agent1 PEM strings, which are read-only), no beforeAll/afterAll, no shared temp directory, and no fixed port. Each subprocess listens on port: 0 and connects to server.address().port, so concurrent execution cannot collide. This is exactly the case the guideline targets.
Why nothing prevents it
There is no ordering dependency between the tests — none of them reads state produced by another. The await using proc + await Promise.all([stdout, stderr, exited]) pattern is already self-contained per test. Nothing in the file requires sequential execution; it's just the default describe.
Impact
Convention-only nit. Functionally the tests are correct as written. Running four TLS-handshaking subprocesses serially adds avoidable wall-clock time to the suite (especially on debug/ASAN builds where each child startup is slow), and diverges from the documented repo convention for subprocess-spawning test files.
Fix
One-token change on line 27:
-describe("https.request checkServerIdentity", () => {
+describe.concurrent("https.request checkServerIdentity", () => {Step-by-step proof
- Line 27:
describe("https.request checkServerIdentity", () => {— sequential block. - Lines ~31, ~73, ~120, ~169: four
test(...)bodies, each beginning withawait using proc = Bun.spawn({ cmd: [bunExe(), "-e", …], env: bunEnv, … }). - Inside each inline
-escript:server.listen(0, …)thenhttps.request({ port: server.address().port, … })— no fixed port, no cross-test resource. - No
beforeAll/beforeEach/afterAllhooks; the only shared module-scope binding is the immutableagent1cert/key/ca strings. test/CLAUDE.mdline 22 says such files should usedescribe.concurrent. This file does not → convention violation.- Changing
describe→describe.concurrentlets all four subprocesses run in parallel with no possibility of interference, satisfying the guideline.
…cts (u9sf0l) (oven-sh#29829) ## What Fixes an ASAN use-after-poison in `HTTPContext.onHandshake` and ships the related `node:https` compat pieces that the grouped tests exercised once the crash was out of the way. ## Repro Any HTTPS request via `node:https` (or `fetch()`) with `rejectUnauthorized: true`, a trusted CA, and a hostname that does **not** match the certificate's identity: ```js const https = require("https"); // server uses agent1-cert.pem (CN=agent1, no SAN) https.request({ port, ca, servername: "not-agent1" }, ...).end(); ``` On ASAN builds this crashes with `use-after-poison` at `src/http/HTTPContext.zig:420`. ## Cause `checkServerIdentity()` returning `false` means it already called `closeAndFail()` → `terminateSocket()` + `fail()` → `unregisterAbortTracker()` + result callback → `onAsyncHTTPCallback()` → **`threadlocal_http.deinit()` destroys the `AsyncHTTP` that embeds the `HTTPClient`**. `onHandshake` then wrote `client.flags.did_have_handshaking_error = true` and called `client.unregisterAbortTracker()` on the freed `client`. Both operations were redundant (already done inside `closeAndFail` / `fail`). ASAN poison-history trace: ``` Memory was manually poisoned by thread T12: ... mi_free ... http.AsyncHTTP.onAsyncHTTPCallback src/http/AsyncHTTP.zig:402 http.HTTPClientResult.Callback.run src/http.zig:2296 http.fail src/http.zig:2059 http.closeAndFail src/http.zig:1712 http.checkServerIdentity src/http.zig:132 http.HTTPContext...onHandshake src/http/HTTPContext.zig:419 ``` ## Fix ### Use-after-free In `HTTPContext.zig` and `ProxyTunnel.zig`, when `checkServerIdentity` returns `false`, return immediately without touching `client`/`this` — the socket is already terminated, the abort tracker already unregistered, and the result callback may have freed the client. ### CN fallback in `checkX509ServerIdentity` Once the crash was gone, all seven tests still hung: they use Node's fixture certs (`agent1`, `agent3`, `rsa_cert`) which carry **only** a Subject CN with no SAN extension. Bun's native `checkX509ServerIdentity` checked SAN only and rejected these. Node's `tls.checkServerIdentity` (and undici's `fetch`) fall back to the Subject CN when the certificate has no DNS/IP/URI SAN entries. Added that fallback for non-IP hostnames. ### `checkServerIdentity` forwarding in `https.request` `_http_client.ts` copied `ca`/`cert`/`key`/etc. from the merged agent/request options into the native TLS config but dropped `checkServerIdentity`. A user-supplied callback was never invoked; the native check ran instead. Now forwarded and validated. ### `requestCert`/`rejectUnauthorized` forwarding in `https.Server` `_http_server.ts` passed `ca` to `Bun.serve({ tls })` but not `requestCert`/`rejectUnauthorized`. The uSockets SSL context treats `ca` with the (defaulted) `rejectUnauthorized: true` as `SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT`, so `https.createServer({ key, cert, ca })` rejected every client that didn't present a client cert. Node only requests a client cert when `requestCert: true`. Mirror the existing `tls.Server` workaround: default `requestCert` to `false` and, when not requesting, force `rejectUnauthorized` to `false` so the CA is loaded without requiring a client cert. ## Verification New `test/js/node/http/node-https-checkServerIdentity.test.ts` exercises all four fixes. Without this change it crashes with ASAN use-after-poison / times out; with it, all four cases pass. The seven originally-grouped Node tests no longer crash but still need separate feature work to pass end-to-end (`req.socket.authorized`, `agent.sockets`/`freeSockets` tracking, `res.socket.getSession()`, `server.setSecureContext()`), so they are not checked in here. --------- Co-authored-by: robobun <robobun@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
2 participants
What
Fixes an ASAN use-after-poison in
HTTPContext.onHandshakeand ships the relatednode:httpscompat pieces that the grouped tests exercised once the crash was out of the way.Repro
Any HTTPS request via
node:https(orfetch()) withrejectUnauthorized: true, a trusted CA, and a hostname that does not match the certificate's identity:On ASAN builds this crashes with
use-after-poisonatsrc/http/HTTPContext.zig:420.Cause
checkServerIdentity()returningfalsemeans it already calledcloseAndFail()→terminateSocket()+fail()→unregisterAbortTracker()+ result callback →onAsyncHTTPCallback()→threadlocal_http.deinit()destroys theAsyncHTTPthat embeds theHTTPClient.onHandshakethen wroteclient.flags.did_have_handshaking_error = trueand calledclient.unregisterAbortTracker()on the freedclient. Both operations were redundant (already done insidecloseAndFail/fail).ASAN poison-history trace:
Fix
Use-after-free
In
HTTPContext.zigandProxyTunnel.zig, whencheckServerIdentityreturnsfalse, return immediately without touchingclient/this— the socket is already terminated, the abort tracker already unregistered, and the result callback may have freed the client.CN fallback in
checkX509ServerIdentityOnce the crash was gone, all seven tests still hung: they use Node's fixture certs (
agent1,agent3,rsa_cert) which carry only a Subject CN with no SAN extension. Bun's nativecheckX509ServerIdentitychecked SAN only and rejected these. Node'stls.checkServerIdentity(and undici'sfetch) fall back to the Subject CN when the certificate has no DNS/IP/URI SAN entries. Added that fallback for non-IP hostnames.checkServerIdentityforwarding inhttps.request_http_client.tscopiedca/cert/key/etc. from the merged agent/request options into the native TLS config but droppedcheckServerIdentity. A user-supplied callback was never invoked; the native check ran instead. Now forwarded and validated.requestCert/rejectUnauthorizedforwarding inhttps.Server_http_server.tspassedcatoBun.serve({ tls })but notrequestCert/rejectUnauthorized. The uSockets SSL context treatscawith the (defaulted)rejectUnauthorized: trueasSSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, sohttps.createServer({ key, cert, ca })rejected every client that didn't present a client cert. Node only requests a client cert whenrequestCert: true. Mirror the existingtls.Serverworkaround: defaultrequestCerttofalseand, when not requesting, forcerejectUnauthorizedtofalseso the CA is loaded without requiring a client cert.Verification
New
test/js/node/http/node-https-checkServerIdentity.test.tsexercises all four fixes. Without this change it crashes with ASAN use-after-poison / times out; with it, all four cases pass.The seven originally-grouped Node tests no longer crash but still need separate feature work to pass end-to-end (
req.socket.authorized,agent.sockets/freeSocketstracking,res.socket.getSession(),server.setSecureContext()), so they are not checked in here.