diff --git a/src/http/HTTPContext.zig b/src/http/HTTPContext.zig index e9a5793b04c9..35dc882eb546 100644 --- a/src/http/HTTPContext.zig +++ b/src/http/HTTPContext.zig @@ -195,13 +195,10 @@ pub fn NewHTTPContext(comptime ssl: bool) type { if (pooled.ssl_config) |*s| s.deinit(); pooled.ssl_config = null; if (pooled.proxy_tunnel) |*rp| { - // Do NOT call rp.data.shutdown() here — it drives - // SSLWrapper.shutdown → triggerCloseCallback → - // onClose(handlers.ctx), and handlers.ctx is the - // stale HTTPClient pointer from detachOwner(). That - // client is freed by now. http_socket.close(.failure) - // below force-closes the TCP without triggering the - // callback, same as addMemoryBackToPool(). + // No shutdown() needed — http_socket.close(.failure) + // below force-closes the TCP, same as addMemoryBackToPool(). + // (onClose would no-op anyway: owner was cleared in + // detachOwner().) rp.deref(); } pooled.proxy_tunnel = null; @@ -699,6 +696,25 @@ pub fn NewHTTPContext(comptime ssl: bool) type { continue; } + // A pooled tunnel's inner TLS session can die after the + // request that pooled it completed — a close_notify or + // SSL error arriving in the same handleReading() call, + // after detachOwner(). tunnel_poolable's snapshot can't + // see that. Don't hand back a dead wrapper: adopt() → + // proxy.write() would swallow error.ConnectionClosed + // (closed_notified is already true so onClose no-ops) + // and the request would hang until timeout. + if (socket.proxy_tunnel) |rp| { + const w = &(rp.data.wrapper orelse { + terminateSocket(http_socket); + continue; + }); + if (w.isShutdown() or w.flags.fatal_error) { + terminateSocket(http_socket); + continue; + } + } + // Release the pool's strong ref (caller has its own via tls_props) if (socket.ssl_config) |*s| s.deinit(); socket.ssl_config = null; diff --git a/src/http/ProxyTunnel.zig b/src/http/ProxyTunnel.zig index 0c1c8d410ab7..244ff3732ba6 100644 --- a/src/http/ProxyTunnel.zig +++ b/src/http/ProxyTunnel.zig @@ -6,6 +6,12 @@ pub const deref = ProxyTunnel.RefCount.deref; pub const RefPtr = bun.ptr.RefPtr(@This()); wrapper: ?ProxyTunnelWrapper = null, +/// The HTTPClient currently using this tunnel. Cleared by detachOwner()/ +/// detachSocket() so SSLWrapper callbacks (which may fire after the request +/// completes inside the same handleReading() call) never dereference a freed +/// client. The wrapper's ctx is the tunnel itself, which is refcounted and +/// kept alive across receive()/onWritable(). +owner: ?*HTTPClient = null, shutdown_err: anyerror = error.ConnectionClosed, // active socket is the socket that is currently being used socket: union(enum) { @@ -29,9 +35,10 @@ did_have_handshaking_error: bool = false, established_with_reject_unauthorized: bool = false, ref_count: RefCount, -const ProxyTunnelWrapper = SSLWrapper(*HTTPClient); +const ProxyTunnelWrapper = SSLWrapper(*ProxyTunnel); -fn onOpen(this: *HTTPClient) void { +fn onOpen(tunnel: *ProxyTunnel) void { + const this = tunnel.owner orelse return; log("ProxyTunnel onOpen", .{}); bun.analytics.Features.http_client_proxy += 1; this.state.response_stage = .proxy_handshake; @@ -62,7 +69,8 @@ fn onOpen(this: *HTTPClient) void { } } -fn onData(this: *HTTPClient, decoded_data: []const u8) void { +fn onData(tunnel: *ProxyTunnel, decoded_data: []const u8) void { + const this = tunnel.owner orelse return; if (decoded_data.len == 0) return; log("ProxyTunnel onData decoded {}", .{decoded_data.len}); if (this.proxy_tunnel) |proxy| { @@ -132,7 +140,8 @@ fn onData(this: *HTTPClient, decoded_data: []const u8) void { } } -fn onHandshake(this: *HTTPClient, handshake_success: bool, ssl_error: uws.us_bun_verify_error_t) void { +fn onHandshake(tunnel: *ProxyTunnel, handshake_success: bool, ssl_error: uws.us_bun_verify_error_t) void { + const this = tunnel.owner orelse return; if (this.proxy_tunnel) |proxy| { log("ProxyTunnel onHandshake", .{}); proxy.ref(); @@ -206,7 +215,8 @@ fn onHandshake(this: *HTTPClient, handshake_success: bool, ssl_error: uws.us_bun } } -pub fn writeEncrypted(this: *HTTPClient, encoded_data: []const u8) void { +pub fn writeEncrypted(tunnel: *ProxyTunnel, encoded_data: []const u8) void { + const this = tunnel.owner orelse return; if (this.proxy_tunnel) |proxy| { // Preserve TLS record ordering: if any encrypted bytes are buffered, // enqueue new bytes and flush them in FIFO via onWritable. @@ -227,7 +237,11 @@ pub fn writeEncrypted(this: *HTTPClient, encoded_data: []const u8) void { } } -fn onClose(this: *HTTPClient) void { +fn onClose(tunnel: *ProxyTunnel) void { + const this = tunnel.owner orelse { + log("ProxyTunnel onClose (no owner)", .{}); + return; + }; log("ProxyTunnel onClose {s}", .{if (this.proxy_tunnel == null) "tunnel is detached" else "tunnel exists"}); if (this.proxy_tunnel) |proxy| { proxy.ref(); @@ -284,17 +298,18 @@ fn progressUpdateForProxySocket(this: *HTTPClient, proxy: *ProxyTunnel) void { pub fn start(this: *HTTPClient, comptime is_ssl: bool, socket: NewHTTPContext(is_ssl).HTTPSocket, ssl_options: jsc.API.ServerConfig.SSLConfig, start_payload: []const u8) void { const proxy_tunnel = bun.new(ProxyTunnel, .{ .ref_count = .init(), + .owner = this, }); // We always request the cert so we can verify it and also we manually abort the connection if the hostname doesn't match const custom_options = ssl_options.forClientVerification(); - proxy_tunnel.wrapper = SSLWrapper(*HTTPClient).init(custom_options, true, .{ + proxy_tunnel.wrapper = ProxyTunnelWrapper.init(custom_options, true, .{ .onOpen = ProxyTunnel.onOpen, .onData = ProxyTunnel.onData, .onHandshake = ProxyTunnel.onHandshake, .onClose = ProxyTunnel.onClose, .write = ProxyTunnel.writeEncrypted, - .ctx = this, + .ctx = proxy_tunnel, }) catch |err| { if (err == error.OutOfMemory) { bun.outOfMemory(); @@ -370,6 +385,7 @@ pub fn write(this: *ProxyTunnel, buf: []const u8) !usize { pub fn detachSocket(this: *ProxyTunnel) void { this.socket = .{ .none = {} }; + this.owner = null; } pub fn detachAndDeref(this: *ProxyTunnel) void { @@ -383,6 +399,7 @@ pub fn detachAndDeref(this: *ProxyTunnel) void { /// to the pool (or dereffed on failure to pool). pub fn detachOwner(this: *ProxyTunnel, client: *const HTTPClient) void { this.socket = .{ .none = {} }; + this.owner = null; // Capture the handshaking-error flag from the client — this is a property // of the inner TLS session, not the client. adopt() restores it to the // next client so re-pooling doesn't erase it. @@ -392,9 +409,6 @@ pub fn detachOwner(this: *ProxyTunnel, client: *const HTTPClient) void { // detaches, it must not downgrade a hostname-verified TLS session to // lax-established; once true, stays true. this.established_with_reject_unauthorized = this.established_with_reject_unauthorized or client.flags.reject_unauthorized; - // We intentionally leave wrapper.handlers.ctx stale here. The tunnel is - // idle in the pool and no callbacks will fire until adopt() reattaches - // a new owner and socket. } /// Reattach a pooled tunnel to a new HTTPClient and socket. The TLS session @@ -408,9 +422,7 @@ pub fn adopt(this: *ProxyTunnel, client: *HTTPClient, comptime is_ssl: bool, soc // (e.g. HTTP 413) with Connection: keep-alive before the full body was // consumed could leave unsent bytes that would corrupt the next request. this.write_buffer.reset(); - if (this.wrapper) |*wrapper| { - wrapper.handlers.ctx = client; - } + this.owner = client; if (is_ssl) { this.socket = .{ .ssl = socket }; } else { diff --git a/src/http/http.zig b/src/http/http.zig index 99c06fc698c2..31ce412f6b7e 100644 --- a/src/http/http.zig +++ b/src/http/http.zig @@ -1620,7 +1620,14 @@ pub fn onWritable(this: *HTTPClient, comptime is_first_call: bool, comptime is_s } if (this.proxy_tunnel) |proxy| { + // onWritable → flush() → handleTraffic() can drain a response that + // completes the request and frees `this` synchronously. Keep the + // tunnel alive across the call and bail if it detached from us. + proxy.ref(); proxy.onWritable(is_ssl, socket); + const detached = proxy.owner != this; + proxy.deref(); + if (detached) return; } switch (this.state.request_stage) { @@ -2287,7 +2294,7 @@ fn sendProgressUpdateWithoutStageCheck(this: *HTTPClient, comptime is_ssl: bool, const tunnel_poolable = if (this.proxy_tunnel) |t| this.state.request_stage == .done and t.write_buffer.isEmpty() and - if (t.wrapper) |*w| !w.isShutdown() else false + if (t.wrapper) |*w| !w.isShutdown() and !w.flags.fatal_error else false else true; diff --git a/test/js/web/fetch/fetch-proxy-tunnel-onclose-uaf-fixture.ts b/test/js/web/fetch/fetch-proxy-tunnel-onclose-uaf-fixture.ts new file mode 100644 index 000000000000..265107f28fec --- /dev/null +++ b/test/js/web/fetch/fetch-proxy-tunnel-onclose-uaf-fixture.ts @@ -0,0 +1,108 @@ +// Fixture for fetch-proxy-tunnel-onclose-uaf.test.ts +// +// Reproduces the ProxyTunnel stale-ctx UAF: SSLWrapper.handleReading decodes +// the HTTP response via triggerDataCallback (which completes the request and +// frees the HTTPClient synchronously), then the same handleReading call hits +// an SSL_ERROR_SSL and fires triggerCloseCallback with the stale ctx. +// +// To land the response and the SSL error in one handleReading(), the proxy +// waits until it has forwarded the client's encrypted HTTP request upstream, +// then appends a malformed TLS record to the next server→client chunk (the +// response). SSL_ERROR_SSL — unlike ZERO_RETURN — leaves +// received_ssl_shutdown=false so the keepalive/pool path is taken and the +// HTTPClient is freed before triggerCloseCallback. + +import { once } from "node:events"; +import net from "node:net"; +import tls from "node:tls"; + +const cert = process.env.TLS_CERT!; +const key = process.env.TLS_KEY!; + +const backend = tls.createServer({ key, cert }, s => { + s.on("error", () => {}); + s.once("data", () => { + // keep-alive so the client pools the tunnel on completion + s.write("HTTP/1.1 200 OK\r\nContent-Length: 2\r\nConnection: keep-alive\r\n\r\nok"); + }); +}); +backend.listen(0, "127.0.0.1"); +await once(backend, "listening"); +const backendPort = (backend.address() as net.AddressInfo).port; + +const proxy = net.createServer(client => { + let head = Buffer.alloc(0); + let upstream: net.Socket | undefined; + client.on("error", () => upstream?.destroy()); + client.on("close", () => upstream?.destroy()); + const onHead = (chunk: Buffer) => { + head = Buffer.concat([head, chunk]); + const headerEnd = head.indexOf("\r\n\r\n"); + if (headerEnd === -1) return; + client.removeListener("data", onHead); + const firstLine = head.subarray(0, head.indexOf("\r\n")).toString("latin1"); + const [, hostPort] = firstLine.split(" "); + const colon = hostPort!.lastIndexOf(":"); + upstream = net.connect(Number(hostPort!.slice(colon + 1)), hostPort!.slice(0, colon), () => { + client.write("HTTP/1.1 200 Connection Established\r\n\r\n"); + const extra = head.subarray(headerEnd + 4); + if (extra.length > 0) upstream!.write(extra); + + // Count client→upstream writes. The first carries the TLS client + // handshake; the second (or later) carries the encrypted HTTP request. + // Only after the request goes out can the next upstream chunk be the + // HTTP response. + let clientWrites = 0; + let injected = false; + client.on("data", d => { + clientWrites++; + upstream!.write(d); + }); + upstream!.on("data", d => { + if (injected) return; + if (clientWrites >= 2) { + // Response: append a bogus TLS record so SSL_read errors right + // after decoding the response in the same BIO fill. + const bad = Buffer.from([0x80, 0x03, 0x03, 0x00, 0x00]); + client.write(Buffer.concat([d, bad])); + injected = true; + client.end(); + upstream!.destroy(); + } else { + client.write(d); + } + }); + }); + upstream.on("error", () => client.destroy()); + upstream.on("close", () => client.end()); + }; + client.on("data", onHead); +}); +proxy.listen(0, "127.0.0.1"); +await once(proxy, "listening"); +const proxyPort = (proxy.address() as net.AddressInfo).port; + +let ok = 0; +let err = 0; +for (let round = 0; round < 4; round++) { + const batch: Promise[] = []; + for (let i = 0; i < 32; i++) { + batch.push( + fetch(`https://127.0.0.1:${backendPort}/`, { + proxy: `http://127.0.0.1:${proxyPort}`, + tls: { rejectUnauthorized: false }, + }) + .then(r => r.text()) + .then(() => { + ok++; + }) + .catch(() => { + err++; + }), + ); + } + await Promise.all(batch); +} +console.log(JSON.stringify({ ok, err })); +backend.close(); +proxy.close(); diff --git a/test/js/web/fetch/fetch-proxy-tunnel-onclose-uaf.test.ts b/test/js/web/fetch/fetch-proxy-tunnel-onclose-uaf.test.ts new file mode 100644 index 000000000000..a0a632f0792f --- /dev/null +++ b/test/js/web/fetch/fetch-proxy-tunnel-onclose-uaf.test.ts @@ -0,0 +1,45 @@ +// Regression: ProxyTunnel SSLWrapper callbacks firing with a freed +// *HTTPClient after the request completed inside the same handleReading(). +// +// SSLWrapper.handleReading → triggerDataCallback → ProxyTunnel.onData → +// progressUpdate → onAsyncHTTPCallback frees the ThreadlocalAsyncHTTP +// (and the embedded HTTPClient) synchronously. If the same handleReading +// then hits SSL_ERROR_SSL, triggerCloseCallback → onClose dereferences the +// freed pointer. The proxy in the fixture appends a malformed TLS record +// right after the HTTP-response record so both land in one BIO fill. +// +// Under debug+ASAN the pre-fix binary aborts with use-after-poison at +// ProxyTunnel.onClose. Release builds read poisoned memory without +// trapping, so this test is only meaningful on sanitizer builds. + +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, tls as tlsCert } from "harness"; +import { join } from "node:path"; + +test("ProxyTunnel onClose does not use freed HTTPClient after response completes", async () => { + await using proc = Bun.spawn({ + cmd: [bunExe(), join(import.meta.dir, "fetch-proxy-tunnel-onclose-uaf-fixture.ts")], + env: { + ...bunEnv, + TLS_CERT: tlsCert.cert, + TLS_KEY: tlsCert.key, + // bunEnv sets NO_PROXY=localhost,127.0.0.1,... which makes fetch + // bypass the explicit `proxy:` option for our 127.0.0.1 target. + NO_PROXY: "", + no_proxy: "", + }, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + if (exitCode !== 0) { + console.error("Fixture stderr:", stderr); + } + expect(exitCode).toBe(0); + + const lastLine = stdout.trim().split("\n").pop()!; + const result = JSON.parse(lastLine); + expect(result.ok).toBeGreaterThan(0); +}, 30_000);