Skip to content
Merged
41 changes: 37 additions & 4 deletions src/bun.js/api/bun/h2_frame_parser.zig
Original file line number Diff line number Diff line change
Expand Up @@ -2015,6 +2015,11 @@
this.remainingLength -= @intCast(end);
var padding: u8 = 0;
if (frame.flags & @intFromEnum(DataFrameFlags.PADDED) != 0) {
if (frame.length < 1) {
// PADDED flag set but no room for the Pad Length octet
this.sendGoAway(frame.streamIdentifier, ErrorCode.FRAME_SIZE_ERROR, "Invalid data frame size", this.lastStreamID, true);
return data.len;
}
if (stream.padding) |p| {
padding = p;
} else {
Expand All @@ -2025,6 +2030,14 @@
padding = payload[0];
stream.padding = payload[0];
}
// RFC 7540 Section 6.1: If the length of the padding is the length of
// the frame payload or greater, the recipient MUST treat this as a
// connection error of type PROTOCOL_ERROR. Validate before subtracting
// below to avoid underflowing `frame.length - padding - 1`.
if (@as(usize, padding) >= frame.length) {
Comment thread
robobun marked this conversation as resolved.
this.sendGoAway(frame.streamIdentifier, ErrorCode.PROTOCOL_ERROR, "Invalid data frame padding", this.lastStreamID, true);
return data.len;
}
Comment thread
claude[bot] marked this conversation as resolved.
}
if (this.remainingLength < 0) {
this.sendGoAway(frame.streamIdentifier, ErrorCode.FRAME_SIZE_ERROR, "Invalid data frame size", this.lastStreamID, true);
Expand All @@ -2041,8 +2054,12 @@
if (payload.len > 0) {
// amount of data received so far
const received_size = frame.length - this.remainingLength;
// max size possible for the chunk without padding and skipping the start_idx
const max_payload_size: usize = frame.length - padding - @as(usize, if (padding > 0) 1 else 0) - start_idx;
// Remaining data bytes (excluding Pad Length octet and trailing padding)
// from start_idx onwards. When a frame is split across multiple reads
// and this chunk lands entirely in the padding region, start_idx can
// exceed the data region; saturate to 0 rather than underflowing.
const data_end: usize = frame.length - padding - @as(usize, if (padding > 0) 1 else 0);
const max_payload_size: usize = data_end -| start_idx;

Check notice on line 2062 in src/bun.js/api/bun/h2_frame_parser.zig

View check run for this annotation

Claude / Claude Code Review

data_end math still mishandles Pad Length octet (off-by-one on later chunks; PadLen=0 leaks into body)

Pre-existing, but since this line was rewritten: `data_end` still mishandles the Pad Length octet in two ways. (1) For a padded DATA frame split across reads, on any non-first chunk (`start_idx >= 1`) the `-1` double-counts the Pad Length byte that `start_idx` already covers, so `max_payload_size` is one too small and the last data byte of the frame is silently dropped. (2) Both this `-1` and the strip at line 2049 are gated on `padding > 0` rather than on the PADDED flag, so an RFC-valid frame
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
payload = payload[0..@min(payload.len, max_payload_size)];
log("received_size: {d} max_payload_size: {d} padding: {d} payload.len: {d}", .{
received_size,
Expand Down Expand Up @@ -2369,6 +2386,11 @@
this.readBuffer.reset();

if (frame.flags & @intFromEnum(HeadersFrameFlags.PADDED) != 0) {
if (payload.len < 1) {
// PADDED flag set but no room for the Pad Length octet
this.sendGoAway(frame.streamIdentifier, ErrorCode.FRAME_SIZE_ERROR, "invalid Headers frame size", this.lastStreamID, true);
return content.end;
}
// padding length
padding = payload[0];
offset += 1;
Expand All @@ -2377,11 +2399,22 @@
// skip priority (client dont need to care about it)
offset += 5;
}
const end = payload.len - padding;
if (offset > end) {
// RFC 7540 Section 4.2: A frame that is too small to contain mandatory
// frame data (here: the Pad Length octet and/or the 5-byte priority
// block) MUST be treated as a FRAME_SIZE_ERROR.
if (offset > payload.len) {
this.sendGoAway(frame.streamIdentifier, ErrorCode.FRAME_SIZE_ERROR, "invalid Headers frame size", this.lastStreamID, true);
return content.end;
}
// RFC 7540 Section 6.2: Padding that exceeds the size remaining for the
// header block fragment MUST be treated as a connection error of type
// PROTOCOL_ERROR. Validate before subtracting to avoid underflowing
// `payload.len - padding` when a peer sends Pad Length > payload length.
if (padding > payload.len - offset) {
this.sendGoAway(frame.streamIdentifier, ErrorCode.PROTOCOL_ERROR, "invalid Headers frame padding", this.lastStreamID, true);
return content.end;
}
Comment thread
claude[bot] marked this conversation as resolved.
const end = payload.len - padding;
stream.endAfterHeaders = frame.flags & @intFromEnum(HeadersFrameFlags.END_STREAM) != 0;
stream = (try this.decodeHeaderBlock(payload[offset..end], stream, frame.flags)) orelse {
return content.end;
Expand Down
115 changes: 115 additions & 0 deletions test/js/node/http2/node-http2.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1351,6 +1351,121 @@ for (const nodeExecutable of [nodeExe(), bunExe()]) {
server.close();
}
});

it("should reject HEADERS frame with Pad Length >= payload length", async () => {
// RFC 7540 Section 6.2: Padding that exceeds the size remaining for the header
// block fragment MUST be treated as a PROTOCOL_ERROR. Previously payload.len - padding
// would underflow and trigger an out-of-bounds read.
const { promise: waitToWrite, resolve: allowWrite } = Promise.withResolvers();
const { promise: serverListening, resolve: serverResolve } = Promise.withResolvers();
const server = net.createServer(async socket => {
const settings = new http2utils.SettingsFrame(true);
socket.write(settings.data);
await waitToWrite;
// HEADERS (type=1), flags = PADDED (0x8) | END_HEADERS (0x4), stream=1, length=1
// payload = [0xFF] -> Pad Length = 255, header block fragment would require 255
// trailing padding bytes that do not exist.
const frame = new http2utils.Frame(1, 1, 0x8 | 0x4, 1).data;
socket.write(Buffer.concat([frame, Buffer.from([0xff])]));
});
server.listen(0, "127.0.0.1", () => serverResolve());
await serverListening;

const url = `http://127.0.0.1:${server.address().port}`;
try {
const { promise, resolve } = Promise.withResolvers();
const client = http2.connect(url);
client.on("error", resolve);
client.on("connect", () => {
const req = client.request({ ":path": "/" });
req.on("error", () => {});
req.end();
allowWrite();
});
const result = await promise;
expect(result).toBeDefined();
expect(result.code).toBe("ERR_HTTP2_SESSION_ERROR");
expect(result.message).toBe("Session closed with error code NGHTTP2_PROTOCOL_ERROR");
} finally {
server.close();
}
});

it("should reject zero-length HEADERS frame with PADDED flag", async () => {
const { promise: waitToWrite, resolve: allowWrite } = Promise.withResolvers();
const { promise: serverListening, resolve: serverResolve } = Promise.withResolvers();
const server = net.createServer(async socket => {
const settings = new http2utils.SettingsFrame(true);
socket.write(settings.data);
await waitToWrite;
// HEADERS (type=1), flags = PADDED (0x8) | END_HEADERS (0x4), stream=1, length=0
// PADDED requires at least 1 byte for the Pad Length field.
const frame = new http2utils.Frame(0, 1, 0x8 | 0x4, 1).data;
socket.write(frame);
});
server.listen(0, "127.0.0.1", () => serverResolve());
await serverListening;

const url = `http://127.0.0.1:${server.address().port}`;
try {
const { promise, resolve } = Promise.withResolvers();
const client = http2.connect(url);
client.on("error", resolve);
client.on("connect", () => {
const req = client.request({ ":path": "/" });
req.on("error", () => {});
req.end();
allowWrite();
});
const result = await promise;
expect(result).toBeDefined();
expect(result.code).toBe("ERR_HTTP2_SESSION_ERROR");
expect(result.message).toBe("Session closed with error code NGHTTP2_FRAME_SIZE_ERROR");
} finally {
server.close();
}
});

it("should reject DATA frame with Pad Length >= payload length", async () => {
// RFC 7540 Section 6.1: If the length of the padding is the length of the
// frame payload or greater, the recipient MUST treat this as a connection
// error of type PROTOCOL_ERROR.
const { promise: waitToWrite, resolve: allowWrite } = Promise.withResolvers();
const { promise: serverListening, resolve: serverResolve } = Promise.withResolvers();
const server = net.createServer(async socket => {
const settings = new http2utils.SettingsFrame(true);
socket.write(settings.data);
await waitToWrite;
// Valid HEADERS response first so the DATA frame is accepted on stream 1.
const headers = new http2utils.HeadersFrame(1, http2utils.kFakeResponseHeaders, 0, true, false);
socket.write(headers.data);
// DATA (type=0), flags = PADDED (0x8), stream=1, length=2, payload = [0xFF, 0x00]
// Pad Length = 255 which exceeds the remaining payload (1 byte).
const frame = new http2utils.Frame(2, 0, 0x8, 1).data;
socket.write(Buffer.concat([frame, Buffer.from([0xff, 0x00])]));
});
server.listen(0, "127.0.0.1", () => serverResolve());
await serverListening;

const url = `http://127.0.0.1:${server.address().port}`;
try {
const { promise, resolve } = Promise.withResolvers();
const client = http2.connect(url);
client.on("error", resolve);
client.on("connect", () => {
const req = client.request({ ":path": "/" });
req.on("error", () => {});
req.end();
allowWrite();
});
const result = await promise;
expect(result).toBeDefined();
expect(result.code).toBe("ERR_HTTP2_SESSION_ERROR");
expect(result.message).toBe("Session closed with error code NGHTTP2_PROTOCOL_ERROR");
} finally {
server.close();
}
});
});
});
}
Expand Down
Loading