http: never pool a proxy tunnel with a fatal TLS error or pending I/O - #32742
Conversation
A CONNECT tunnel's SSLWrapper read loop flushes decrypted bytes to ProxyTunnel::on_data and then runs the close callback, guarded only by `closed_notified`. When the flushed bytes complete a keep-alive response and the next record is invalid, the fatal record error sets no shutdown flag, so `tunnel_poolable` accepted the wrapper and the tunnel was handed to the keep-alive pool: nothing called `wrapper.shutdown()`, so `closed_notified` was never latched. Dispatching the final result then freed the ThreadlocalAsyncHTTP that embeds the HTTPClient, and the still-running read loop fired `on_close` into the freed `handlers.ctx`. Require the wrapper to have no fatal error and no pending unconsumed input/output before pooling. Every non-poolable terminal path goes through `close_proxy_tunnel(true)`, which latches `closed_notified` before the client is freed. The pending-I/O check closes the same hole for a tunnel pooled from a mid-loop data callback while more decrypted bytes or queued output remain. Both are also required for the pool to be correct on its own terms: a poisoned or dirty TLS session must not be handed to the next request.
|
Updated 2:10 AM PT - Jun 26th, 2026
❌ @Jarred-Sumner, your commit 0ae24b7 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 32742That installs a local version of the PR into your bun-32742 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
BIO_ctrl_pending only sees encrypted bytes still in the BIOs. When SSL_read partially returns a record (the output buffer filled mid record), the record has already been pulled out of the rbio and its remaining decrypted bytes are buffered inside the SSL, reported only by SSL_pending. Without it, the pooling guard added in the previous commit misses exactly the buffer-full flush case it exists for: both BIOs read empty, the tunnel is pooled, and the read loop's next SSL_read returns the rest of that record into a callback whose ctx was just freed.
|
Good catch — fixed in 0ae24b7. |
There was a problem hiding this comment.
The SSL_pending follow-up in 0ae24b7 addresses my earlier comment and the predicate now matches the stated invariant; I didn't find anything further, but a UAF fix in the HTTP client's TLS pooling path is worth a human sign-off.
Extended reasoning...
Overview
Tightens the tunnel_poolable predicate in src/http/lib.rs so a CONNECT proxy tunnel whose SSLWrapper has fatal_error set or any pending I/O is never returned to the keep-alive pool, routing it through the orderly teardown that latches closed_notified instead. SSLWrapper::has_pending_data() (src/uws/lib.rs) is broadened to also consult SSL_pending(ssl) so decrypted-but-unread bytes from a partially-returned record count as pending. src/boringssl_sys/boringssl.rs adds the SSL_pending FFI declaration, and a new regression test in test/js/bun/http/proxy.test.ts reproduces the original ASAN heap-use-after-free.
Security risks
The bug being fixed is itself a memory-safety / security issue: a UAF in the HTTP client thread, plus the possibility of handing a poisoned or dirty TLS session to an unrelated next request. The change is strictly a tightening — it only refuses to pool in additional cases and falls through to the existing close path — so it shouldn't introduce new exposure. SSLWrapper::has_pending_data() has no other callers, so broadening its semantics doesn't perturb anything else. No new attack surface is added.
Level of scrutiny
High. This is production-critical networking code at the intersection of TLS state-machine handling, callback re-entrancy, and connection-pool lifetime management — exactly the kind of subtle ownership reasoning where a second pair of human eyes is warranted even when the diff is small. The correctness argument hinges on the invariant that every non-poolable terminal path reaches closed_notified before handlers.ctx can go stale, and on has_pending_data() now covering all three places BoringSSL can hide bytes (rbio, wbio, SSL_pending).
Other factors
My earlier inline comment (the SSL_pending gap at the buffer-full flush) was addressed exactly as suggested in 0ae24b7, and the author's reply correctly notes that for TLS (vs DTLS) SSL_pending is the only remaining hiding place. The new test is well-constructed and the author reports it deterministically reproduces the UAF on an unfixed ASAN build. The robobun CI comment still shows a ❌ against the earlier commit 1eeb03b and hasn't refreshed for 0ae24b7, so CI status on the final revision is unconfirmed from the thread alone.
2 participants
What does this PR do?
Fixes a use-after-free in the HTTP client's CONNECT proxy tunnel, caught by ASAN:
SSLWrapper::handle_readingflushes pending decrypted bytes to the data callback, then runs the close callback, guarded only byclosed_notified:fatal_error— none of the shutdown flags — so the wrapper passedtunnel_poolable's!is_shutdown()check and the tunnel was handed to the keep-alive pool. Nothing calledwrapper.shutdown(), soclosed_notifiedwas never latched. Dispatching the final result then freed theThreadlocalAsyncHTTPthat embeds theHTTPClient.ssl.is_none() || closed_notified()) passes.trigger_close_callback()invokeson_close(handlers.ctx)withctxpointing at the freed client.The pooling branch is the only terminal path that doesn't go through
close_proxy_tunnel(true)→wrapper.shutdown()→closed_notified, which is the latch the read loop relies on.SSLWrapper::shutdownalready special-cases the close_notify flavor of this for exactly that reason; the fatal-error flavor never reachesshutdown().The fix is one predicate: a tunnel whose wrapper has a fatal error or pending unconsumed input/output is not poolable. That routes it through the orderly teardown that latches
closed_notified, and the pending-I/O half closes the same hole for a tunnel pooled from a mid-loop data callback while more decrypted bytes or queued output remain. Both are also required for the pool to be correct on its own terms — a poisoned or dirty TLS session must not be handed to the next request.How did you verify your code works?
New regression test in
test/js/bun/http/proxy.test.ts(next to the existing close_notify sibling): an HTTPS keep-alive response through a CONNECT proxy with a corrupt TLS record appended to the same TCP burst, followed by a second request that can only complete if the HTTP client thread survived the first.Against an unfixed ASAN debug build the fixture aborts every run:
With this change it prints
4096 200 200and exits 0 with no ASAN report.test/js/bun/http/proxy.test.ts(49/49),fetch-proxy-connect-tunnel-split-envelope.test.ts,fetch-proxy-tls-intern-race.test.ts, andfetch-keepalive.test.tsall pass.