Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 23 additions & 7 deletions src/http/HTTPContext.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
40 changes: 26 additions & 14 deletions src/http/ProxyTunnel.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
Expand Down Expand Up @@ -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| {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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.
Expand All @@ -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;
};
Comment thread
claude[bot] marked this conversation as resolved.
Comment thread
claude[bot] marked this conversation as resolved.
log("ProxyTunnel onClose {s}", .{if (this.proxy_tunnel == null) "tunnel is detached" else "tunnel exists"});
if (this.proxy_tunnel) |proxy| {
proxy.ref();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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 {
Expand All @@ -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.
Expand All @@ -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
Expand All @@ -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 {
Expand Down
9 changes: 8 additions & 1 deletion src/http/http.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;

Expand Down
108 changes: 108 additions & 0 deletions test/js/web/fetch/fetch-proxy-tunnel-onclose-uaf-fixture.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

45 changes: 45 additions & 0 deletions test/js/web/fetch/fetch-proxy-tunnel-onclose-uaf.test.ts
Original file line number Diff line number Diff line change
@@ -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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit: test/CLAUDE.md says "Do not set a timeout on tests. Bun already has timeouts." — though given this fixture does 4×32 proxied TLS handshakes the 30s is probably load-bearing on slow CI, so if you want to drop it consider trimming the round/batch counts instead.

Extended reasoning...

What this is

test/CLAUDE.md:120 states:

CRITICAL: Do not set a timeout on tests. Bun already has timeouts.

The new test passes 30_000 as the second argument to test() at test/js/web/fetch/fetch-proxy-tunnel-onclose-uaf.test.ts:45, which is an explicit per-test timeout. That's a documented-convention violation, not a functional bug — the test passes/fails identically either way.

Step-by-step

  1. test/CLAUDE.md:120 documents the rule.
  2. Line 45 of the new file ends the test("...", async () => { ... }, 30_000); call with an explicit 30-second timeout literal.
  3. There is no other mechanism in this file (no jest.setTimeout, no harness flag) that would supply a timeout, so the 30_000 is the only thing overriding Bun's default.

Why the rule isn't a clean fit here

The fixture spawns a subprocess that performs 4 rounds × 32 = 128 proxied HTTPS requests. Each one is TCP connect → CONNECT → inner TLS handshake → request/response. On slow CI runners (Windows in particular) that can comfortably exceed Bun's default 5s test timeout, so simply deleting 30_000 would likely make the test flaky — i.e. the "fix" is worse than the violation.

This is also far from unique in the codebase: a grep shows 150+ test files passing a numeric second argument to test(), including the immediately-neighbouring fetch-leak.test.ts, fetch-proxy-tls-intern-race.test.ts, and fetch-redirect.test.ts. So enforcement of this rule is already inconsistent and the new file matches the local pattern rather than diverging from it.

Why flag it anyway

It's new code being added, the guideline is explicitly marked CRITICAL in the repo's own docs, and there's a straightforward alternative that satisfies both concerns: the iteration counts in the fixture are arbitrary load-generators, not correctness-bearing constants.

Suggested fix

Either leave the timeout (matching neighbours) and ignore this nit, or — if you'd prefer to follow test/CLAUDE.md — drop the 30_000 and reduce the fixture's workload so it fits the default timeout, e.g.:

for (let round = 0; round < 2; round++) {
  for (let i = 0; i < 16; i++) {

32 concurrent + 2 rounds is still plenty to trip the UAF under ASAN (the original repro is a single-request race; the batching just amortises scheduling jitter).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving the 30s timeout — matches the neighbouring fetch-proxy-tls-intern-race.test.ts and is load-bearing for 128 proxied TLS handshakes on slow CI. Trimming the batch count risks losing determinism on the UAF repro.

Loading