diff --git a/src/bun.js/api/bun/udp_socket.zig b/src/bun.js/api/bun/udp_socket.zig index 2761d1e090f7..5bb58f4f8300 100644 --- a/src/bun.js/api/bun/udp_socket.zig +++ b/src/bun.js/api/bun/udp_socket.zig @@ -601,6 +601,40 @@ pub const UDPSocket = struct { } pub fn sendMany(this: *This, globalThis: *JSGlobalObject, callframe: *CallFrame) bun.JSError!JSValue { + // Iterating the input array can run arbitrary user JS: `iter.next()`'s + // slow path hits `JSObject.getIndex`, and `parseAddr` calls + // `port.coerceToInt32()` / `address.toBunString()`. That JS can drop + // the last reference to an earlier payload and force a GC, or detach + // an earlier ArrayBuffer (`.transfer(n)` frees its backing store + // synchronously), leaving borrowed pointers in `payloads[]` dangling + // before `socket.send` reads them. + // + // Root every payload JSValue in a MarkedArgumentBuffer for the + // duration of the call so GC cannot collect them, and split the work + // into two phases: phase 1 collects/validates payloads and runs all + // user JS; phase 2 borrows byte slices only once no more user JS + // sits between capture and `socket.send`. + const Ctx = struct { + this: *UDPSocket, + globalThis: *JSGlobalObject, + callframe: *CallFrame, + result: bun.JSError!JSValue, + + pub fn run(ctx: *@This(), payload_roots: *jsc.MarkedArgumentBuffer) callconv(.c) void { + ctx.result = sendManyImpl(ctx.this, ctx.globalThis, ctx.callframe, payload_roots); + } + }; + var ctx: Ctx = .{ + .this = this, + .globalThis = globalThis, + .callframe = callframe, + .result = .js_undefined, + }; + jsc.MarkedArgumentBuffer.run(Ctx, &ctx, &Ctx.run); + return ctx.result; + } + + fn sendManyImpl(this: *This, globalThis: *JSGlobalObject, callframe: *CallFrame, payload_roots: *jsc.MarkedArgumentBuffer) bun.JSError!JSValue { if (this.closed) { return globalThis.throw("Socket is closed", .{}); } @@ -634,6 +668,7 @@ pub const UDPSocket = struct { defer arena.deinit(); const alloc = arena.allocator(); + var payload_vals = bun.handleOom(alloc.alloc(JSValue, len)); var payloads = bun.handleOom(alloc.alloc([*]const u8, len)); var lens = bun.handleOom(alloc.alloc(usize, len)); var addr_ptrs = bun.handleOom(alloc.alloc(?*const anyopaque, len)); @@ -641,6 +676,11 @@ pub const UDPSocket = struct { var iter = try arg.arrayIterator(globalThis); + // Phase 1: collect and validate payload JSValues, resolve addresses. + // All user-JS re-entrance happens here. Root each payload in the + // MarkedArgumentBuffer so GC cannot collect it, but do NOT yet borrow + // raw pointers into backing stores — user JS on a later iteration + // could otherwise free or detach that storage. var i: u32 = 0; var port: JSValue = .zero; while (try iter.next()) |val| : (i += 1) { @@ -649,17 +689,20 @@ pub const UDPSocket = struct { } const slice_idx = if (connected) i else i / 3; if (connected or i % 3 == 0) { - const slice = brk: { - if (val.asArrayBuffer(globalThis)) |arrayBuffer| { - break :brk arrayBuffer.slice(); - } else if (val.isString()) { - break :brk (try val.toJSString(globalThis)).toSlice(globalThis, alloc).slice(); - } else { - return globalThis.throwInvalidArguments("Expected ArrayBufferView or string as payload", .{}); - } + const payload_val: JSValue = blk: { + if (val.asArrayBuffer(globalThis) != null) break :blk val; + // `isString()` is `isStringLike()` and accepts boxed + // `StringObject` / `DerivedStringObject`; calling + // `toJSString` on those in phase 2 would run user + // `toString()`/`valueOf()` via `toPrimitive`. Resolve to + // the primitive JSString here — where user-JS re-entrance + // is expected — and root that, so phase 2 only ever sees + // primitive JSString cells. + if (val.isString()) break :blk (try val.toJSString(globalThis)).toJS(); + return globalThis.throwInvalidArguments("Expected ArrayBufferView or string as payload", .{}); }; - payloads[slice_idx] = slice.ptr; - lens[slice_idx] = slice.len; + payload_roots.append(payload_val); + payload_vals[slice_idx] = payload_val; } if (connected) { addr_ptrs[slice_idx] = null; @@ -679,6 +722,34 @@ pub const UDPSocket = struct { if (i != array_len) { return globalThis.throwInvalidArguments("Mismatch between array length property and number of items", .{}); } + + // Phase 2: borrow byte slices now that no more user JS will run before + // `socket.send`. Every `payload_vals` entry is either an + // ArrayBufferView or a *primitive* JSString (boxed strings were + // resolved in phase 1), so nothing here reaches `toPrimitive`. Rope + // resolution / UTF-16 conversion may allocate and GC, but every + // payload is rooted so borrowed WTFStringImpl / backing-store + // pointers stay valid. An ArrayBuffer detached during phase 1 now + // reports a zero-length slice rather than a dangling pointer. + const empty: []const u8 = ""; + for (payload_vals, 0..) |val, slice_idx| { + const slice: []const u8 = brk: { + if (val.asArrayBuffer(globalThis)) |arrayBuffer| { + // `byteSlice()` returns `&.{}` for a detached view; its + // `.ptr` is Zig's zero-length sentinel which the kernel + // rejects with EFAULT even though `iov_len == 0`. Hand + // sendmmsg a valid static address instead. + if (arrayBuffer.isDetached()) break :brk empty; + break :brk arrayBuffer.slice(); + } + // Phase 1 stored the primitive JSString; `asString()` is a + // plain cast (no `toPrimitive`, no user JS). + break :brk val.asString().toSlice(globalThis, alloc).slice(); + }; + payloads[slice_idx] = slice.ptr; + lens[slice_idx] = slice.len; + } + const socket = this.socket orelse return globalThis.throw("Socket is closed", .{}); const res = socket.send(payloads, lens, addr_ptrs); if (getUSError(res, .send, true)) |err| { @@ -712,6 +783,25 @@ pub const UDPSocket = struct { } }; + // Resolve the destination before touching the payload. `parseAddr` + // calls `port.coerceToInt32()` / `address.toBunString()` which can + // run user JS that detaches the payload's ArrayBuffer + // (`.transfer(n)`) or closes this socket. Doing this first means no + // JSC safepoint sits between capturing `payload.ptr` and handing it + // to `socket.send`, so a borrowed pointer cannot be freed out from + // under us. `payload_arg` itself stays rooted in the callframe. + var addr: std.posix.sockaddr.storage = std.mem.zeroes(std.posix.sockaddr.storage); + const addr_ptr = brk: { + if (dst) |dest| { + if (!try this.parseAddr(globalThis, dest.port, dest.address, &addr)) { + return globalThis.throwInvalidArguments("Invalid address", .{}); + } + break :brk &addr; + } else { + break :brk null; + } + }; + const payload_arg = arguments.ptr[0]; var payload_str = jsc.ZigString.Slice.empty; defer payload_str.deinit(); @@ -719,25 +809,21 @@ pub const UDPSocket = struct { if (payload_arg.asArrayBuffer(globalThis)) |array_buffer| { break :brk array_buffer.slice(); } else if (payload_arg.isString()) { - payload_str = payload_arg.asString().toSlice(globalThis, bun.default_allocator); + // `isString()` is `isStringLike()` and accepts boxed + // `StringObject`/`DerivedStringObject`; `asString()` is a raw + // `static_cast` that asserts/type-confuses on those. + // `toJSString` resolves them via `toPrimitive` — safe here: + // `parseAddr` already ran, there is only one payload so + // `toPrimitive` cannot invalidate an earlier captured pointer, + // and `this.socket orelse throw` below handles a + // close-during-`toPrimitive`. + payload_str = (try payload_arg.toJSString(globalThis)).toSlice(globalThis, bun.default_allocator); break :brk payload_str.slice(); } else { return globalThis.throwInvalidArguments("Expected ArrayBufferView or string as first argument", .{}); } }; - var addr: std.posix.sockaddr.storage = std.mem.zeroes(std.posix.sockaddr.storage); - const addr_ptr = brk: { - if (dst) |dest| { - if (!try this.parseAddr(globalThis, dest.port, dest.address, &addr)) { - return globalThis.throwInvalidArguments("Invalid address", .{}); - } - break :brk &addr; - } else { - break :brk null; - } - }; - const socket = this.socket orelse return globalThis.throw("Socket is closed", .{}); const res = socket.send(&.{payload.ptr}, &.{payload.len}, &.{addr_ptr}); if (getUSError(res, .send, true)) |err| { diff --git a/test/js/bun/udp/sendMany-payload-uaf-fixture.ts b/test/js/bun/udp/sendMany-payload-uaf-fixture.ts new file mode 100644 index 000000000000..99db19fc6d5b --- /dev/null +++ b/test/js/bun/udp/sendMany-payload-uaf-fixture.ts @@ -0,0 +1,136 @@ +// Regression fixture: UDPSocket.sendMany() / send() used to capture a raw +// pointer into the payload's ArrayBuffer backing store (or borrowed JSString +// storage) and then run user JS before handing that pointer to +// `bsd_sendmmsg`. In `sendMany` the user JS runs on later iterations (array +// index getters, port `valueOf()`, address `toString()`); in `send` it runs +// inside `parseAddr` (port `valueOf()`, address `toString()`) after the +// payload is captured. That JS can detach the ArrayBuffer via +// `transfer(newLen)`, which synchronously frees the old backing store, and +// the native send path then reads freed memory. +// +// The test driver spawns this fixture with `Malloc=1` so bmalloc routes +// ArrayBuffer backing stores through the system allocator, making the +// allocation visible to ASAN in sanitizer-enabled builds. Release builds fall +// through and we simply verify the correct bytes arrive at the other socket. + +const mode = process.argv[2]; +if (mode !== "sendMany" && mode !== "sendMany-stringobj" && mode !== "send") { + console.error("usage: sendMany-payload-uaf-fixture.ts "); + process.exit(2); +} + +const size = 4096; + +let received: Buffer | undefined; +let resolve!: () => void; +const gotData = new Promise(r => (resolve = r)); + +const server = await Bun.udpSocket({ + port: 0, + hostname: "127.0.0.1", + socket: { + data(_socket, data) { + if (received) return; + const chunk = Buffer.from(data as ArrayBuffer); + // In `send` mode the first call captures the payload from the + // now-detached view (length 0). On Linux that surfaces as EFAULT and + // nothing is sent; on Windows it succeeds and a 0-byte packet arrives + // here before the retry loop delivers the real payload. Ignore it so + // both platforms settle on the 4096-byte retry packet. + if (chunk.length !== size) return; + received = chunk; + resolve(); + }, + }, +}); +const client = await Bun.udpSocket({ port: 0, hostname: "127.0.0.1" }); + +try { + const buf = new ArrayBuffer(size); + const payload = new Uint8Array(buf); + for (let i = 0; i < size; i++) payload[i] = i & 0xff; + const expected = Buffer.from(payload); + + let detached = false; + const evilPort = { + valueOf() { + if (!detached) { + detached = true; + // `transfer(newLen)` with newLen != byteLength allocates a new + // backing store, copies, and synchronously frees the old one (the + // `ArrayBufferContents` destructor runs before `transfer` returns). + // Plain `transfer()` would only move the pointer, not free it. + buf.transfer(0); + } + return server.port; + }, + }; + + // Bun's `isString()` is `isStringLike()` and accepts boxed/derived String + // objects; `toJSString()` on those goes through `toPrimitive` and invokes + // user `toString()`. sendMany must resolve them to primitive JSStrings in + // phase 1 so phase 2 never calls back into JS. + class EvilString extends String { + toString() { + if (!detached) { + detached = true; + buf.transfer(0); + } + return super.toString(); + } + } + + // Unconnected socket: the port is coerced via `valueOf()` after the payload + // JSValue has been captured, so by the time the native send path borrows a + // raw pointer the buffer is already detached. Both send() and sendMany() + // now observe the detached (length-0) view rather than the freed 4096-byte + // region. sendMany substitutes a valid empty pointer and sends a 0-byte + // packet; send()'s detached-view path may surface as EFAULT on Linux (the + // same pre-existing behavior as `send(detachedView, ...)`) or send a 0-byte + // packet on Windows. Either outcome is fine — the regression this fixture + // guards is the ASAN heap-use-after-free, which aborts the process before + // this catch ever runs. + try { + if (mode === "sendMany") { + client.sendMany([payload, evilPort, "127.0.0.1"]); + } else if (mode === "sendMany-stringobj") { + // Second payload is a DerivedStringObject whose `toString()` detaches + // the first payload's backing store. If sendMany deferred the + // `toJSString()` to phase 2, payloads[0] would already hold a borrowed + // pointer into `buf` when `toString()` frees it. + client.sendMany([payload, server.port, "127.0.0.1", new EvilString("x") as any, server.port, "127.0.0.1"]); + } else { + client.send(payload, evilPort as never, "127.0.0.1"); + } + } catch (e: any) { + if (mode !== "send" || e?.code !== "EFAULT") throw e; + } + + if (!detached) throw new Error("re-entrant callback never ran"); + + // Handle unreliable transmission in UDP: the first send already exercised + // the UAF path; retries just let the correctness assertion complete if the + // single packet was dropped on a loaded host. Use the captured `expected` + // bytes since the original buffer is now detached. + function sendRec() { + if (received || client.closed) return; + client.send(expected, server.port, "127.0.0.1"); + setTimeout(sendRec, 10); + } + setTimeout(sendRec, 10); + + await gotData; + + if (!received) throw new Error("no data received"); + if (received.length !== size) { + throw new Error(`expected ${size} bytes, got ${received.length}`); + } + if (!received.equals(expected)) { + throw new Error("received payload does not match original bytes"); + } + + console.log("OK"); +} finally { + client.close(); + server.close(); +} diff --git a/test/js/bun/udp/udp_socket.test.ts b/test/js/bun/udp/udp_socket.test.ts index 7629eb9c9368..3217ccd41714 100644 --- a/test/js/bun/udp/udp_socket.test.ts +++ b/test/js/bun/udp/udp_socket.test.ts @@ -1,7 +1,7 @@ import { udpSocket } from "bun"; import { heapStats } from "bun:jsc"; import { describe, expect, test } from "bun:test"; -import { bunEnv, bunExe, disableAggressiveGCScope, randomPort } from "harness"; +import { bunEnv, bunExe, disableAggressiveGCScope, isWindows, randomPort } from "harness"; import path from "node:path"; import { dataCases, dataTypes } from "./testdata"; @@ -44,6 +44,40 @@ describe("udpSocket()", () => { }, ); + // `isString()` is `isStringLike()` and accepts boxed `new String(...)` / + // `class extends String`, but `asString()` is a raw `static_cast` + // that debug-asserts (and release type-confuses) on a StringObject cell. + // Both send() and sendMany() must resolve via `toJSString()` instead. + test("send/sendMany accept boxed String payloads without crashing", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const server = await Bun.udpSocket({ port: 0, hostname: "127.0.0.1" }); + const client = await Bun.udpSocket({ port: 0, hostname: "127.0.0.1" }); + class Derived extends String {} + client.send(new String("a"), server.port, "127.0.0.1"); + client.send(new Derived("b"), server.port, "127.0.0.1"); + client.sendMany([new String("c"), server.port, "127.0.0.1", new Derived("d"), server.port, "127.0.0.1"]); + client.close(); server.close(); + console.log("OK"); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, rawStderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const stderr = rawStderr + .split("\n") + .filter(l => l && !l.startsWith("WARNING: ASAN interferes")) + .join("\n"); + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("OK"); + expect(exitCode).toBe(0); + }); + test("connect with invalid hostname rejects", async () => { expect(async () => udpSocket({ @@ -330,6 +364,51 @@ describe("udpSocket()", () => { } } + // send()/sendMany() capture a pointer into the payload's backing store and + // then run user JS (port `valueOf()`, address `toString()`, and for + // sendMany also array index getters on later iterations). That JS can + // detach the ArrayBuffer via `transfer(n)` and free the bytes before the + // native send path reads them. sendMany roots each payload JSValue in a + // MarkedArgumentBuffer and defers borrowing byte slices until after all + // user JS has run; send resolves the destination before capturing the + // payload. + describe("detaching an ArrayBuffer during port/address coercion does not use-after-free", () => { + for (const mode of ["sendMany", "sendMany-stringobj", "send"] as const) { + test( + mode, + async () => { + await using proc = Bun.spawn({ + cmd: [bunExe(), path.join(import.meta.dir, "sendMany-payload-uaf-fixture.ts"), mode], + env: { + ...bunEnv, + // Route bmalloc through the system heap so ASAN can observe the + // ArrayBuffer backing-store free in sanitizer-enabled builds. On + // Windows bmalloc's SystemHeap is unimplemented and would + // RELEASE_BASSERT, so leave bmalloc in place there — Windows has + // no ASAN lane anyway, and the fixture still checks correctness. + ...(isWindows ? {} : { Malloc: "1" }), + }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, rawStderr, exitCode] = await Promise.all([ + proc.stdout.text(), + proc.stderr.text(), + proc.exited, + ]); + const stderr = rawStderr + .split("\n") + .filter(l => l && !l.startsWith("WARNING: ASAN interferes")) + .join("\n"); + expect(stderr).toBe(""); + expect(stdout).toBe("OK\n"); + expect(exitCode).toBe(0); + }, + 30_000, + ); + } + }); + // sendMany() iterates the input array and may run user JS (array index // getters, port `valueOf()`, address `toString()`). That user JS can // connect or disconnect the socket; sendMany must snapshot the connection