diff --git a/CHANGELOG.md b/CHANGELOG.md index 00379c24..8340887c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,13 @@ +# 0.4.15 (June 15, 2026) + +* Fix closing a connection when header size is "way too large" (currently x4 configured limit). +* Fix overflow calculating padding length if a DATA frame had 255 bytes of padding. +* Fix ignoring library-initiated resets in the connection state loop. +* Fix decoding panic with an absurd amount of headers and no limit to now use `try_append()`. +* Fix rejecting frames on streams whose HEADERS have not been sent. +* Fix `poll_capacity()` to not return `Some(Ok(0))`. +* Fix discarding of buffered DATA frames when a reset is scheduled. + # 0.4.14 (May 5, 2026) * Add `header_table_size()` option to server builder. diff --git a/src/codec/framed_read.rs b/src/codec/framed_read.rs index d90d9a21..3ea36034 100644 --- a/src/codec/framed_read.rs +++ b/src/codec/framed_read.rs @@ -175,6 +175,13 @@ fn decode_frame( proto_err!(stream: "malformed header block; stream={:?}", id); return Err(Error::library_reset(id, Reason::PROTOCOL_ERROR)); }, + Err(frame::Error::HeaderListWayTooLarge) => { + proto_err!(conn: "decoded header list size over abuse limit"); + return Err(Error::library_go_away_data( + Reason::ENHANCE_YOUR_CALM, + "header_list_way_too_large", + )); + }, Err(_e) => { proto_err!(conn: "failed HPACK decoding; err={:?}", _e); return Err(Error::library_go_away(Reason::PROTOCOL_ERROR)); @@ -347,6 +354,13 @@ fn decode_frame( proto_err!(stream: "malformed CONTINUATION frame; stream={:?}", id); return Err(Error::library_reset(id, Reason::PROTOCOL_ERROR)); } + Err(frame::Error::HeaderListWayTooLarge) => { + proto_err!(conn: "decoded CONTINUATION header list size over abuse limit"); + return Err(Error::library_go_away_data( + Reason::ENHANCE_YOUR_CALM, + "header_list_way_too_large", + )); + } Err(_e) => { proto_err!(conn: "failed HPACK decoding; err={:?}", _e); return Err(Error::library_go_away(Reason::PROTOCOL_ERROR)); diff --git a/src/frame/data.rs b/src/frame/data.rs index 3af0af11..c67d2dfc 100644 --- a/src/frame/data.rs +++ b/src/frame/data.rs @@ -148,10 +148,47 @@ impl Data { self.data.len() } } +} + +#[cfg(test)] +mod tests { + use super::*; + use bytes::Bytes; + + fn data_frame(data: &[u8], pad_len: Option) -> Data { + Data { + stream_id: StreamId::from(1), + data: Bytes::copy_from_slice(data), + flags: DataFlags::default(), + pad_len, + } + } + + #[test] + fn padding_overhead_no_padding() { + let frame = data_frame(b"hello", None); + assert_eq!(frame.flow_controlled_len() - frame.payload().len(), 0); + } + + #[test] + fn padding_overhead_small() { + let frame = data_frame(b"hello", Some(10)); + assert_eq!(frame.flow_controlled_len() - frame.payload().len(), 11); + } + + #[test] + fn padding_overhead_max_does_not_overflow() { + // Regression: the old padded_len() returned u8, so pad_len=255 + // caused 255u8 + 1 = 0. The correct overhead is 256. + let frame = data_frame(b"", Some(255)); + assert_eq!(frame.flow_controlled_len() - frame.payload().len(), 256); + } - /// If this frame is PADDED, it returns the pad len + 1 (length field). - pub(crate) fn padded_len(&self) -> Option { - self.pad_len.map(|n| n + 1) + #[test] + fn padding_overhead_max_with_data() { + let frame = data_frame(b"hello", Some(255)); + assert_eq!(frame.flow_controlled_len() - frame.payload().len(), 256); + assert_eq!(frame.flow_controlled_len(), 261); } } diff --git a/src/frame/headers.rs b/src/frame/headers.rs index 0cf318ab..43ec9cce 100644 --- a/src/frame/headers.rs +++ b/src/frame/headers.rs @@ -12,9 +12,12 @@ use smallvec::SmallVec; use std::fmt; use std::io::Cursor; +use std::ops::ControlFlow; type EncodeBuf<'a> = bytes::buf::Limit<&'a mut BytesMut>; +const MAX_HEADER_LIST_ABUSE_MULTIPLIER: usize = 4; + /// Header frame /// /// This could be either a request or a response. @@ -985,7 +988,26 @@ impl HeaderBlock { ) -> Result<(), Error> { let mut reg = !self.fields.is_empty(); let mut malformed = false; + let mut header_list_way_too_large = false; let mut headers_size = self.calculate_header_list_size(); + let max_header_list_abuse_size = + max_header_list_size.saturating_mul(MAX_HEADER_LIST_ABUSE_MULTIPLIER); + + macro_rules! check_size { + () => {{ + if headers_size > max_header_list_abuse_size { + tracing::trace!("load_hpack; header list size over abuse max"); + header_list_way_too_large = true; + ControlFlow::Break(()) + } else { + if headers_size >= max_header_list_size && !self.is_over_size { + tracing::trace!("load_hpack; header list size over max"); + self.is_over_size = true; + } + ControlFlow::Continue(()) + } + }}; + } macro_rules! set_pseudo { ($field:ident, $val:expr) => {{ @@ -999,11 +1021,11 @@ impl HeaderBlock { let __val = $val; headers_size += decoded_header_size(stringify!($field).len() + 1, __val.as_str().len()); - if headers_size < max_header_list_size { + if check_size!().is_break() { + return ControlFlow::Break(()); + } + if !self.is_over_size { self.pseudo.$field = Some(__val); - } else if !self.is_over_size { - tracing::trace!("load_hpack; header list size over max"); - self.is_over_size = true; } } }}; @@ -1040,19 +1062,19 @@ impl HeaderBlock { } else { reg = true; - headers_size += decoded_header_size(name.as_str().len(), value.len()); - if headers_size < max_header_list_size { - self.field_size += - decoded_header_size(name.as_str().len(), value.len()); + let header_size = decoded_header_size(name.as_str().len(), value.len()); + headers_size += header_size; + if check_size!().is_break() { + return ControlFlow::Break(()); + } + if !self.is_over_size { + self.field_size += header_size; if let Err(_) = self.fields.try_append(name, value) { // HeaderMap capacity exceeded — treat as over-size // so the stream is rejected downstream (RST_STREAM / 431) // instead of panicking on the 24,577th unique header. self.is_over_size = true; } - } else if !self.is_over_size { - tracing::trace!("load_hpack; header list size over max"); - self.is_over_size = true; } } } @@ -1063,11 +1085,21 @@ impl HeaderBlock { Protocol(v) => set_pseudo!(protocol, v), Status(v) => set_pseudo!(status, v), } + + ControlFlow::Continue(()) }); - if let Err(e) = res { - tracing::trace!("hpack decoding error; err={:?}", e); - return Err(e.into()); + match res { + Ok(()) => {} + Err(e) => { + tracing::trace!("hpack decoding error; err={:?}", e); + return Err(e.into()); + } + } + + if header_list_way_too_large { + tracing::trace!("header list way too large; aborting connection"); + return Err(Error::HeaderListWayTooLarge); } if malformed { diff --git a/src/frame/mod.rs b/src/frame/mod.rs index fad52169..fb7e53cb 100644 --- a/src/frame/mod.rs +++ b/src/frame/mod.rs @@ -168,6 +168,9 @@ pub enum Error { /// A request or response is malformed. MalformedMessage, + /// The decoded header list was too large to continue processing. + HeaderListWayTooLarge, + /// An invalid stream dependency ID was provided /// /// This is returned if a HEADERS or PRIORITY frame is received with an diff --git a/src/fuzz_bridge.rs b/src/fuzz_bridge.rs index 3ea8b591..6c0c8b0b 100644 --- a/src/fuzz_bridge.rs +++ b/src/fuzz_bridge.rs @@ -4,12 +4,13 @@ pub mod fuzz_logic { use bytes::BytesMut; use http::header::HeaderName; use std::io::Cursor; + use std::ops::ControlFlow; pub fn fuzz_hpack(data_: &[u8]) { let mut decoder_ = hpack::Decoder::new(0); let mut buf = BytesMut::new(); buf.extend(data_); - let _dec_res = decoder_.decode(&mut Cursor::new(&mut buf), |_h| {}); + let _dec_res = decoder_.decode(&mut Cursor::new(&mut buf), |_h| ControlFlow::Continue(())); if let Ok(s) = std::str::from_utf8(data_) { if let Ok(h) = http::Method::from_bytes(s.as_bytes()) { diff --git a/src/hpack/decoder.rs b/src/hpack/decoder.rs index bb688727..f820aa5b 100644 --- a/src/hpack/decoder.rs +++ b/src/hpack/decoder.rs @@ -9,6 +9,7 @@ use http::status::{self, StatusCode}; use std::cmp; use std::collections::VecDeque; use std::io::Cursor; +use std::ops::ControlFlow; use std::str::Utf8Error; /// Decodes headers using HPACK @@ -179,7 +180,7 @@ impl Decoder { mut f: F, ) -> Result<(), DecoderError> where - F: FnMut(Header), + F: FnMut(Header) -> ControlFlow<()>, { use self::Representation::*; @@ -203,7 +204,9 @@ impl Decoder { can_resize = false; let entry = self.decode_indexed(src)?; consume(src); - f(entry); + if f(entry).is_break() { + break; + } } LiteralWithIndexing => { tracing::trace!(rem = src.remaining(), kind = %"LiteralWithIndexing"); @@ -214,14 +217,18 @@ impl Decoder { self.table.insert(entry.clone()); consume(src); - f(entry); + if f(entry).is_break() { + break; + } } LiteralWithoutIndexing => { tracing::trace!(rem = src.remaining(), kind = %"LiteralWithoutIndexing"); can_resize = false; let entry = self.decode_literal(src, false)?; consume(src); - f(entry); + if f(entry).is_break() { + break; + } } LiteralNeverIndexed => { tracing::trace!(rem = src.remaining(), kind = %"LiteralNeverIndexed"); @@ -231,7 +238,9 @@ impl Decoder { // TODO: Track that this should never be indexed - f(entry); + if f(entry).is_break() { + break; + } } SizeUpdate => { tracing::trace!(rem = src.remaining(), kind = %"SizeUpdate"); @@ -914,7 +923,8 @@ mod test { fn test_decode_empty() { let mut de = Decoder::new(0); let mut buf = BytesMut::new(); - let _: () = de.decode(&mut Cursor::new(&mut buf), |_| {}).unwrap(); + de.decode(&mut Cursor::new(&mut buf), |_| ControlFlow::Continue(())) + .unwrap(); } #[test] @@ -930,6 +940,7 @@ mod test { let mut res = vec![]; de.decode(&mut Cursor::new(&mut buf), |h| { res.push(h); + ControlFlow::Continue(()) }) .unwrap(); @@ -970,6 +981,7 @@ mod test { let e = de .decode(&mut Cursor::new(&mut buf), |h| { res.push(h); + ControlFlow::Continue(()) }) .unwrap_err(); // decode error because the header value is partial @@ -979,6 +991,7 @@ mod test { buf.extend(&value[1..]); de.decode(&mut Cursor::new(&mut buf), |h| { res.push(h); + ControlFlow::Continue(()) }) .unwrap(); diff --git a/src/hpack/test/fixture.rs b/src/hpack/test/fixture.rs index ed2f11c2..70c823ea 100644 --- a/src/hpack/test/fixture.rs +++ b/src/hpack/test/fixture.rs @@ -7,6 +7,7 @@ use serde_json::Value; use std::fs::File; use std::io::prelude::*; use std::io::Cursor; +use std::ops::ControlFlow; use std::path::Path; use std::str; @@ -78,6 +79,7 @@ fn test_story(story: Value) { let (name, value) = expect.remove(0); assert_eq!(name, key_str(&e)); assert_eq!(value, value_str(&e)); + ControlFlow::Continue(()) }) .unwrap(); @@ -112,6 +114,7 @@ fn test_story(story: Value) { decoder .decode(&mut Cursor::new(&mut buf), |e| { assert_eq!(e, input.remove(0).reify().unwrap()); + ControlFlow::Continue(()) }) .unwrap(); diff --git a/src/hpack/test/fuzz.rs b/src/hpack/test/fuzz.rs index af9e8ea2..81214995 100644 --- a/src/hpack/test/fuzz.rs +++ b/src/hpack/test/fuzz.rs @@ -9,6 +9,7 @@ use rand::rngs::StdRng; use rand::{thread_rng, Rng, SeedableRng}; use std::io::Cursor; +use std::ops::ControlFlow; const MAX_CHUNK: usize = 2 * 1024; @@ -170,6 +171,7 @@ impl FuzzHpack { .decode(&mut Cursor::new(&mut buf), |h| { let e = expect.remove(0); assert_eq!(h, e); + ControlFlow::Continue(()) }) .expect("full decode"); } diff --git a/src/proto/connection.rs b/src/proto/connection.rs index 92bd2e95..89ec584a 100644 --- a/src/proto/connection.rs +++ b/src/proto/connection.rs @@ -451,11 +451,17 @@ where Ok(()) } // Attempting to read a frame resulted in a stream level error. - // This is handled by resetting the frame then trying to read - // another frame. + // Locally detected stream errors are reported to the peer with + // RST_STREAM. Remotely initiated resets have already been applied + // by the streams state machine and must not be echoed back. Err(Error::Reset(id, reason, initiator)) => { + if initiator == Initiator::Remote { + tracing::trace!(?id, ?reason, ?initiator, "stream reset"); + return Ok(()); + } + debug_assert_eq!(initiator, Initiator::Library); - tracing::trace!(?id, ?reason, "stream error"); + tracing::trace!(?id, ?reason, ?initiator, "stream error"); match self.streams.send_reset(id, reason) { Ok(()) => (), Err(crate::proto::error::GoAway { debug_data, reason }) => { diff --git a/src/proto/streams/recv.rs b/src/proto/streams/recv.rs index 67a26de4..790c989c 100644 --- a/src/proto/streams/recv.rs +++ b/src/proto/streams/recv.rs @@ -737,14 +737,16 @@ impl Recv { // Track the data as in-flight stream.in_flight_recv_data += sz; - // We auto-release the padded length, since the user cannot. - if let Some(padded_len) = frame.padded_len() { + // Auto-release padding overhead (pad_len field + padding bytes), + // since the user only sees the data payload via `payload()`. + let padding = (frame.flow_controlled_len() - frame.payload().len()) as WindowSize; + if padding > 0 { tracing::trace!( - "recv_data; auto-releasing padded length of {:?} for {:?}", - padded_len, + "recv_data; auto-releasing padding of {:?} for {:?}", + padding, stream.id, ); - let _res = self.release_capacity(padded_len.into(), stream, &mut None); + let _res = self.release_capacity(padding, stream, &mut None); // cannot fail, we JUST added more in_flight data above. debug_assert!(_res.is_ok()); } diff --git a/tests/h2-tests/tests/client_request.rs b/tests/h2-tests/tests/client_request.rs index 66059969..ea7b8a55 100644 --- a/tests/h2-tests/tests/client_request.rs +++ b/tests/h2-tests/tests/client_request.rs @@ -827,7 +827,7 @@ async fn recv_too_big_headers() { let srv = async move { let settings = srv.assert_client_handshake().await; - assert_frame_eq(settings, frames::settings().max_header_list_size(10)); + assert_frame_eq(settings, frames::settings().max_header_list_size(40)); srv.recv_frame( frames::headers(1) .request("GET", "https://http2.akamai.com/") @@ -850,7 +850,7 @@ async fn recv_too_big_headers() { let client = async move { let (mut client, mut conn) = client::Builder::new() - .max_header_list_size(10) + .max_header_list_size(40) .handshake::<_, Bytes>(io) .await .expect("handshake"); diff --git a/tests/h2-tests/tests/push_promise.rs b/tests/h2-tests/tests/push_promise.rs index b2d8fde0..59596ebf 100644 --- a/tests/h2-tests/tests/push_promise.rs +++ b/tests/h2-tests/tests/push_promise.rs @@ -237,7 +237,7 @@ async fn recv_push_promise_over_max_header_list_size() { let srv = async move { let settings = srv.assert_client_handshake().await; - assert_frame_eq(settings, frames::settings().max_header_list_size(10)); + assert_frame_eq(settings, frames::settings().max_header_list_size(64)); srv.recv_frame( frames::headers(1) .request("GET", "https://http2.akamai.com/") @@ -255,7 +255,7 @@ async fn recv_push_promise_over_max_header_list_size() { let client = async move { let (mut client, mut conn) = client::Builder::new() - .max_header_list_size(10) + .max_header_list_size(64) .handshake::<_, Bytes>(io) .await .expect("handshake"); @@ -265,13 +265,13 @@ async fn recv_push_promise_over_max_header_list_size() { .unwrap(); let req = async move { - let err = client + let resp = client .send_request(request, true) .expect("send_request") .0 .await - .expect_err("response"); - assert_eq!(err.reason(), Some(Reason::PROTOCOL_ERROR)); + .expect("response"); + assert_eq!(resp.status(), StatusCode::OK); }; conn.drive(req).await; diff --git a/tests/h2-tests/tests/server.rs b/tests/h2-tests/tests/server.rs index ef2b1d36..249438d5 100644 --- a/tests/h2-tests/tests/server.rs +++ b/tests/h2-tests/tests/server.rs @@ -976,7 +976,7 @@ async fn too_big_headers_sends_431() { let client = async move { let settings = client.assert_server_handshake().await; - assert_frame_eq(settings, frames::settings().max_header_list_size(10)); + assert_frame_eq(settings, frames::settings().max_header_list_size(64)); client .send_frame( frames::headers(1) @@ -993,7 +993,7 @@ async fn too_big_headers_sends_431() { let srv = async move { let mut srv = server::Builder::new() - .max_header_list_size(10) + .max_header_list_size(64) .handshake::<_, Bytes>(io) .await .expect("handshake"); @@ -1012,7 +1012,7 @@ async fn too_big_headers_sends_reset_after_431_if_not_eos() { let client = async move { let settings = client.assert_server_handshake().await; - assert_frame_eq(settings, frames::settings().max_header_list_size(10)); + assert_frame_eq(settings, frames::settings().max_header_list_size(64)); client .send_frame( frames::headers(1) @@ -1028,7 +1028,7 @@ async fn too_big_headers_sends_reset_after_431_if_not_eos() { let srv = async move { let mut srv = server::Builder::new() - .max_header_list_size(10) + .max_header_list_size(64) .handshake::<_, Bytes>(io) .await .expect("handshake"); @@ -1040,6 +1040,43 @@ async fn too_big_headers_sends_reset_after_431_if_not_eos() { join(client, srv).await; } +#[tokio::test] +async fn abusive_headers_send_goaway() { + h2_support::trace_init!(); + let (io, mut client) = mock::new(); + + let client = async move { + let settings = client.assert_server_handshake().await; + assert_frame_eq(settings, frames::settings().max_header_list_size(64)); + client + .send_frame( + frames::headers(1) + .request("GET", "https://example.com/") + .field("x-abuse", "a".repeat(200)) + .eos(), + ) + .await; + client + .recv_frame(frames::go_away(0).calm().data("header_list_way_too_large")) + .await; + }; + + let srv = async move { + let mut srv = server::Builder::new() + .max_header_list_size(64) + .handshake::<_, Bytes>(io) + .await + .expect("handshake"); + + let err = srv.next().await.unwrap().expect_err("server"); + assert!(err.is_go_away()); + assert!(err.is_library()); + assert_eq!(err.reason(), Some(Reason::ENHANCE_YOUR_CALM)); + }; + + join(client, srv).await; +} + #[tokio::test] async fn too_many_continuation_frames_sends_goaway() { h2_support::trace_init!(); @@ -1750,3 +1787,52 @@ async fn init_window_size_smaller_than_default_should_use_default_before_ack() { join(client, h2).await; } + +#[tokio::test] +async fn remote_reset_does_not_panic_connection_driver() { + h2_support::trace_init!(); + + const ADVERSARIAL_WIRE: &[u8] = &[ + // Client connection preface. + 0x50, 0x52, 0x49, 0x20, 0x2a, 0x20, 0x48, 0x54, 0x54, 0x50, 0x2f, 0x32, 0x2e, 0x30, 0x0d, + 0x0a, 0x0d, 0x0a, 0x53, 0x4d, 0x0d, 0x0a, 0x0d, 0x0a, + // SETTINGS len=0, flags=0, stream=0. + 0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, + // Unknown frame type 0x87, len=5, flags=0xc1, stream=257. + 0x00, 0x00, 0x05, 0x87, 0xc1, 0x00, 0x00, 0x01, 0x01, 0x05, 0x94, 0x05, 0x01, 0x00, + // Unknown frame type 0xc1, len=0, flags=0x94, stream=1281. + 0x00, 0x00, 0x00, 0xc1, 0x94, 0x00, 0x00, 0x05, 0x01, + // HEADERS len=4, flags=END_STREAM | END_HEADERS, stream=4353. + 0x00, 0x00, 0x04, 0x01, 0x05, 0x00, 0x00, 0x11, 0x01, 0x83, 0x87, 0x01, 0x00, + // RST_STREAM len=4, flags=0x05, stream=4353. + 0x00, 0x00, 0x04, 0x03, 0x05, 0x00, 0x00, 0x11, 0x01, 0x83, 0x87, 0x01, 0x00, + // HEADERS len=5, flags=0xf6, stream=4353. + 0x00, 0x00, 0x05, 0x01, 0xf6, 0x00, 0x00, 0x11, 0x01, 0x01, 0x94, 0x00, 0x3d, 0x01, + // PUSH_PROMISE len=5, flags=0xf6, stream=4353. + 0x00, 0x00, 0x05, 0x05, 0xf6, 0x00, 0x00, 0x11, 0x01, 0x3d, 0x94, 0x81, 0x00, 0x95, + // HEADERS len=0, flags=END_STREAM | END_HEADERS, stream=4353. + 0x00, 0x00, 0x00, 0x01, 0x05, 0x00, 0x00, 0x11, 0x01, + ]; + + let (mut client_io, server_io) = tokio::io::duplex(256 * 1024); + let server_task = tokio::spawn(async move { + let Ok(mut server) = server::handshake(server_io).await else { + return; + }; + + while let Some(result) = server.next().await { + let _ = result; + } + }); + + client_io + .write_all(ADVERSARIAL_WIRE) + .await + .expect("write adversarial wire"); + drop(client_io); + + tokio::time::timeout(std::time::Duration::from_secs(1), server_task) + .await + .expect("server task timed out") + .expect("server task panicked"); +}