http2: reject PADDED frames whose Pad Length exceeds the payload#29905
Conversation
A malicious peer can send a HEADERS or DATA frame with the PADDED flag set and a Pad Length octet that is larger than the remaining frame payload. In handleHeadersFrame the expression payload.len - padding wrapped around in ReleaseFast, producing a slice end far past the buffer and causing an out-of-bounds heap read when the header block was decoded. In Debug/Safe builds the same input triggered an integer overflow panic. Validate the Pad Length against the frame payload before performing the subtraction, and reject the frame with a connection error as required by RFC 7540 sections 6.1 and 6.2. Also reject zero-length PADDED frames, which previously read payload[0] on an empty slice.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds RFC7540-aligned padding validation and bounds checks to HTTP/2 DATA and HEADERS frame parsing (flag-driven PADDED handling, explicit Pad Length presence checks, and GOAWAY error responses) and adds integration tests that send malformed or split PADDED frames over raw TCP. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 0/5 reviews remaining, refill in 55 minutes and 37 seconds. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Keep FRAME_SIZE_ERROR for HEADERS frames that are too short for the mandatory Pad Length / priority fields (RFC 7540 Section 4.2), and only emit PROTOCOL_ERROR when the Pad Length value itself exceeds the remaining header block fragment (Section 6.2). Use saturating subtraction when computing the remaining data-byte budget for a chunk of a padded DATA frame so that a chunk landing entirely in the trailing padding region yields 0 instead of underflowing.
|
Updated 6:31 AM PT - Apr 29th, 2026
❌ @Jarred-Sumner, your commit 4549dd8 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29905That installs a local version of the PR into your bun-29905 --bun |
node-http2.test.js has unrelated pre-existing timeouts on slow debug builds. Keep the padding-underflow tests in their own file so they can be run in isolation, and add a case for HEADERS with PRIORITY set and fewer than 5 payload bytes to guard the FRAME_SIZE_ERROR behaviour.
Two pre-existing correctness issues in handleDataFrame that surfaced while reworking the padded-chunk math: - Pad Length = 0 is valid per RFC 7540 Section 6.1 but the Pad Length octet was only stripped when its value was > 0, so the 0x00 byte leaked into the emitted body. - On non-first chunks of a split padded frame, the Pad Length octet was subtracted both via start_idx and via an explicit -1, dropping the last data byte of the frame. Compute the data region as frame-relative offsets [1, length - pad) and intersect it with the current chunk instead of subtracting a byte count from an offset.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/http2/node-http2-invalid-padding.test.ts (1)
1-224: 🛠️ Refactor suggestion | 🟠 MajorPlease colocate these cases in the existing HTTP/2 node test suite.
This new standalone
*.test.tsfile conflicts with the repository test-placement rule for changed coverage; these cases should be folded into the existing HTTP/2 test file for this area.As per coding guidelines,
test/**/*.test.ts: "Add new tests to existing test files for the code being changed, not in new separate files".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/js/node/http2/node-http2-invalid-padding.test.ts` around lines 1 - 224, Move the tests from the standalone node-http2-invalid-padding.test.ts into the existing HTTP/2 node test file that covers similar cases (co-locate rather than adding a new test file); copy over the helper functions sendFrames and receiveBody and all test cases (tests titled "should reject HEADERS frame with Pad Length >= payload length", "should reject zero-length HEADERS frame with PADDED flag", "should reject HEADERS frame with truncated priority fields", "should reject DATA frame with Pad Length >= payload length", "should strip Pad Length octet from DATA frame when Pad Length is 0", and "should not drop trailing data byte from padded DATA frame split across reads") into that file, adjust imports to reuse the existing http2utils/http2/net helpers, remove the new file from the commit, and run the test suite to ensure there are no duplicate imports or name collisions with existing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/js/node/http2/node-http2-invalid-padding.test.ts`:
- Around line 1-224: Move the tests from the standalone
node-http2-invalid-padding.test.ts into the existing HTTP/2 node test file that
covers similar cases (co-locate rather than adding a new test file); copy over
the helper functions sendFrames and receiveBody and all test cases (tests titled
"should reject HEADERS frame with Pad Length >= payload length", "should reject
zero-length HEADERS frame with PADDED flag", "should reject HEADERS frame with
truncated priority fields", "should reject DATA frame with Pad Length >= payload
length", "should strip Pad Length octet from DATA frame when Pad Length is 0",
and "should not drop trailing data byte from padded DATA frame split across
reads") into that file, adjust imports to reuse the existing
http2utils/http2/net helpers, remove the new file from the commit, and run the
test suite to ensure there are no duplicate imports or name collisions with
existing tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 29d163a2-b8f5-41ff-8117-a35b70e1f9bf
📒 Files selected for processing (2)
src/bun.js/api/bun/h2_frame_parser.zigtest/js/node/http2/node-http2-invalid-padding.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/node/http2/node-http2-invalid-padding.test.ts`:
- Line 1: The new tests in test/js/node/http2/node-http2-invalid-padding.test.ts
should not live in a separate file; move the test cases into the existing HTTP/2
test file (e.g., node-http2.test.ts) for the same area, remove the new .test.ts
file, and update imports as needed (ensure you use the existing import { expect,
test } from "bun:test"; and preserve any helper setup/teardown already present
in the existing HTTP/2 test file); run the suite to confirm no duplicate test
names or conflicting fixtures after merging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d95ceeb7-23b8-40b8-81c6-687cc12e2d73
📒 Files selected for processing (1)
test/js/node/http2/node-http2-invalid-padding.test.ts
Include the first data byte in the initial chunk and have the server wait for the client request to emit 'data' before sending the rest. This proves the parser has consumed chunk 1 and will re-enter handleDataFrame with start_idx > 0 for chunk 2, rather than relying on an arbitrary number of event-loop ticks to separate the writes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/node/http2/node-http2-invalid-padding.test.ts`:
- Around line 158-165: The function awaiting `promise` may throw before the
`close` handle is returned, leaking `client`/`server`; modify `receiveBody` (the
block that does `const body = await promise; return { body, close: () => {
client.destroy(); server.close(); } }`) to ensure cleanup on rejection by
wrapping the await in try/catch (or try/finally) so that on error you call
`client.destroy()` and `server.close()` and then rethrow the error, guaranteeing
the http2 handles are closed even when `promise` rejects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 789105a7-66fb-4209-8499-d9e1d072559d
📒 Files selected for processing (1)
test/js/node/http2/node-http2-invalid-padding.test.ts
robobun
force-pushed
the
farm/0685f7a5/h2-padding-underflow
branch
from
d3dc3d9 to
ca76114
Compare
There was a problem hiding this comment.
All earlier feedback has been addressed and I have nothing further to flag — deferring to a human for final sign-off since this rewrites offset arithmetic in the network-facing HTTP/2 frame parser (memory-safety fix).
Extended reasoning...
Overview
This PR hardens handleHeadersFrame and handleDataFrame in src/bun.js/api/bun/h2_frame_parser.zig against malformed PADDED frames: it adds zero-length / Pad Length bounds checks (sending FRAME_SIZE_ERROR / PROTOCOL_ERROR per RFC 7540 §4.2/§6.1/§6.2), and rewrites the DATA-frame chunk-slicing math as a frame-offset intersection (data_region_end - max(start_idx, data_region_start) with saturating subtraction) keyed on the PADDED flag rather than padding > 0. A new test file exercises six cases including invalid padding, Pad Length = 0, truncated priority fields, and a padded DATA frame split across reads.
Security risks
The bug being fixed is itself a security issue — a peer-controlled integer underflow leading to an OOB heap read fed into the HPACK decoder in ReleaseFast. The fix only adds bounds checks and reorders subtractions to occur after validation, so it strictly reduces the attack surface. I don't see any new risk introduced; the new error paths reuse the existing sendGoAway(..., true) connection-error machinery.
Level of scrutiny
High. This is a network-facing binary protocol parser handling untrusted input, and the handleDataFrame change is not a one-line guard — it replaces the payload-extent computation with new offset arithmetic that went through three revisions during review (saturating subtraction, then the count-vs-offset off-by-one fix, then the PADDED-flag keying). I've traced the final form for the single-read, split-read, Pad Length = 0, and all-padding-chunk cases and believe it is correct, but a human should confirm the data_region_start = padded ? max(start_idx, 1) : start_idx interaction with the in-place payload = payload[1..] strip.
Other factors
Every inline comment I and CodeRabbit raised across the revisions has been addressed and resolved (split FRAME_SIZE/PROTOCOL_ERROR checks, saturating subtraction, off-by-one on non-first chunks, Pad Length = 0 leak, deterministic split-read test, receiveBody cleanup on rejection, stale comment text). The CI failures reported by robobun (websocket-server.test.ts, serve-stream-reject-flush-leak.test.ts, test-integration-rspack.ts, fetch-http2-client.test.ts ASAN timeout) are pre-existing flakes unrelated to padding handling. Jarred has already engaged on the thread, so this is ready for maintainer review.
…n-sh#29905) ## What does this PR do? Fixes an integer underflow in the HTTP/2 frame parser when handling `HEADERS` and `DATA` frames with the `PADDED` flag set. ## Problem A peer can send a `HEADERS` frame with `PADDED | END_HEADERS`, `length=1`, and payload `[0xFF]`. In `handleHeadersFrame`: ```zig padding = payload[0]; // 255 ... const end = payload.len - padding; // 1 - 255 -> wraps to ~2^64 ... payload[offset..end] // out-of-bounds heap read ``` In Debug/Safe builds this panics with `integer overflow` at [`h2_frame_parser.zig:2380`](https://github.com/oven-sh/bun/blob/97f683e26d/src/bun.js/api/bun/h2_frame_parser.zig#L2380). In ReleaseFast (the default release mode) runtime safety is off, so the subtraction silently wraps and the subsequent slice `payload[offset..end]` reads past the end of the input buffer into adjacent heap memory, which is then fed to the HPACK decoder. A related issue exists in `handleDataFrame` where `frame.length - padding - 1` can underflow with the same input shape, and both handlers read `payload[0]` without checking that the payload is non-empty when `PADDED` is set. ## Fix Per RFC 7540 §6.1 / §6.2, a Pad Length that is equal to or larger than the remaining frame payload MUST be treated as a connection error of type `PROTOCOL_ERROR`. - `handleHeadersFrame`: reject with `FRAME_SIZE_ERROR` if `PADDED` is set on a zero-length frame; reject with `PROTOCOL_ERROR` if `offset + padding > payload.len` before computing `payload.len - padding`. - `handleDataFrame`: reject with `FRAME_SIZE_ERROR` if `PADDED` is set on a zero-length frame; reject with `PROTOCOL_ERROR` if `padding >= frame.length`. ## How did you verify your code works? Added three tests to `test/js/node/http2/node-http2.test.js` that craft the malicious frames from a raw TCP server and assert the client closes the session with the correct error: - `should reject HEADERS frame with Pad Length >= payload length` - `should reject zero-length HEADERS frame with PADDED flag` - `should reject DATA frame with Pad Length >= payload length` Without the fix, `bun bd test` panics at `h2_frame_parser.zig:2380` with `integer overflow`, and the release build hangs (silent OOB read). With the fix, all tests pass and the session is closed with `ERR_HTTP2_SESSION_ERROR`. --------- Co-authored-by: robobun <robobun@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
2 participants
What does this PR do?
Fixes an integer underflow in the HTTP/2 frame parser when handling
HEADERSandDATAframes with thePADDEDflag set.Problem
A peer can send a
HEADERSframe withPADDED | END_HEADERS,length=1, and payload[0xFF]. InhandleHeadersFrame:In Debug/Safe builds this panics with
integer overflowath2_frame_parser.zig:2380. In ReleaseFast (the default release mode) runtime safety is off, so the subtraction silently wraps and the subsequent slicepayload[offset..end]reads past the end of the input buffer into adjacent heap memory, which is then fed to the HPACK decoder.A related issue exists in
handleDataFramewhereframe.length - padding - 1can underflow with the same input shape, and both handlers readpayload[0]without checking that the payload is non-empty whenPADDEDis set.Fix
Per RFC 7540 §6.1 / §6.2, a Pad Length that is equal to or larger than the remaining frame payload MUST be treated as a connection error of type
PROTOCOL_ERROR.handleHeadersFrame: reject withFRAME_SIZE_ERRORifPADDEDis set on a zero-length frame; reject withPROTOCOL_ERRORifoffset + padding > payload.lenbefore computingpayload.len - padding.handleDataFrame: reject withFRAME_SIZE_ERRORifPADDEDis set on a zero-length frame; reject withPROTOCOL_ERRORifpadding >= frame.length.How did you verify your code works?
Added three tests to
test/js/node/http2/node-http2.test.jsthat craft the malicious frames from a raw TCP server and assert the client closes the session with the correct error:should reject HEADERS frame with Pad Length >= payload lengthshould reject zero-length HEADERS frame with PADDED flagshould reject DATA frame with Pad Length >= payload lengthWithout the fix,
bun bd testpanics ath2_frame_parser.zig:2380withinteger overflow, and the release build hangs (silent OOB read). With the fix, all tests pass and the session is closed withERR_HTTP2_SESSION_ERROR.