From d1b6b8d950f17f1a078db7f945a0dd8df5d402ec Mon Sep 17 00:00:00 2001 From: disarray09 Date: Fri, 31 Jul 2026 10:41:31 -0400 Subject: [PATCH 1/3] Bound buffered frames by their declared length, before reading the payload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FrameDecoder accumulates into a BufList until a frame's whole declared payload has arrived. Consuming each chunk is what releases the flow-control credit for the next one, so credit is returned as fast as the decoder accumulates and no transport window bounds the buffer. A peer declaring a large HEADERS is held in memory in full before max_field_section_size can reject it, and a peer declaring a large unknown frame is held in full before it can be ignored. The bound is the sender's rate times how long the connection is allowed to live. Frame::decode now decides from the type and the declared length alone: * DATA is untouched — it carries a length and streams its body. * A reserved HTTP/2 type is a connection error decided by the type, so its payload is no longer waited for. * An unknown type returns SkipUnknown with its length, and FrameDecoder discards the payload as it arrives instead of storing it to drop it. RFC 9114 asks that unknown frames be ignored; ignoring one used to cost its declared length. Refusing it instead would bound memory just as well and would not be conformant. * A frame that must be buffered to be parsed is refused with TooLarge if its declared length exceeds the decoder's ceiling, before any of it is read, and the accumulated state is released rather than left for the caller. The ceiling defaults to 1 MiB — generous beside anything legitimate, since field sections run to kilobytes and the control frames to tens of bytes, and finite, which is the property that was missing. An endpoint that advertises a larger max_field_section_size raises it to match: the ceiling is a memory bound, and must never be tighter than what the endpoint has told peers it accepts, or a field section would be refused from its encoded length before QPACK can report the uncompressed size that RFC 9114 4.2.2 is written in terms of. What the refusal means depends on where the frame arrived, so FrameStreamError reports it rather than deciding: an oversized HEADERS on a request stream is HeaderTooBig, a message-level rejection that leaves the connection alone, while on the control stream — where every legitimate frame is tens of bytes and the stream itself is critical — it is H3_EXCESSIVE_LOAD. Two existing tests changed behaviour rather than shape: unknown_frame_type and reserved_frame now observe SkipUnknown from the header instead of UnknownFrame after the payload. --- examples/server.rs | 4 +- h3/src/client/connection.rs | 6 +- h3/src/client/stream.rs | 14 +- h3/src/connection.rs | 49 ++++- h3/src/error/connection_error_creators.rs | 16 ++ h3/src/frame.rs | 225 +++++++++++++++++++++- h3/src/proto/frame.rs | 178 ++++++++++++++++- h3/src/server/connection.rs | 6 +- h3/src/server/request.rs | 10 + 9 files changed, 486 insertions(+), 22 deletions(-) diff --git a/examples/server.rs b/examples/server.rs index a38c873a..ee5ec568 100644 --- a/examples/server.rs +++ b/examples/server.rs @@ -112,7 +112,9 @@ async fn main() -> Result<(), Box> { Ok(conn) => { info!("new connection established"); - let mut h3_conn = h3::server::Connection::new(h3_quinn::Connection::new(conn)) + let mut h3_conn = h3::server::builder() + .max_field_section_size(1024 * 16) + .build(h3_quinn::Connection::new(conn)) .await .unwrap(); diff --git a/h3/src/client/connection.rs b/h3/src/client/connection.rs index 82b800e3..a5a60445 100644 --- a/h3/src/client/connection.rs +++ b/h3/src/client/connection.rs @@ -216,7 +216,11 @@ where let request_stream = RequestStream { inner: connection::RequestStream::new( - FrameStream::new(BufRecvStream::new(stream)), + FrameStream::new_with_max_buffered_frame( + BufRecvStream::new(stream), + self.max_field_section_size + .max(crate::frame::DEFAULT_MAX_BUFFERED_FRAME), + ), self.max_field_section_size, self.conn_state.clone(), self.send_grease_frame, diff --git a/h3/src/client/stream.rs b/h3/src/client/stream.rs index b1a9641f..90f41907 100644 --- a/h3/src/client/stream.rs +++ b/h3/src/client/stream.rs @@ -12,6 +12,7 @@ use crate::{ internal_error::InternalConnectionError, Code, StreamError, }, + frame::FrameStreamError, proto::{frame::Frame, headers::Header}, qpack, quic::{self}, @@ -97,8 +98,17 @@ where /// [`recv_data()`]: #method.recv_data #[cfg_attr(feature = "tracing", instrument(skip_all, level = "trace"))] pub async fn recv_response(&mut self) -> Result, StreamError> { - let mut frame = future::poll_fn(|cx| self.inner.stream.poll_next(cx)) - .await + let frame = future::poll_fn(|cx| self.inner.stream.poll_next(cx)).await; + if matches!( + frame, + Err(FrameStreamError::PayloadTooLarge { + frame_type: 0x1, + .. + }) + ) { + self.inner.stop_sending(Code::H3_REQUEST_CANCELLED); + } + let mut frame = frame .map_err(|e| self.handle_frame_stream_error_on_request_stream(e))? .ok_or_else(|| { //= https://www.rfc-editor.org/rfc/rfc9114#section-4.1 diff --git a/h3/src/connection.rs b/h3/src/connection.rs index 44302f1a..b19ff254 100644 --- a/h3/src/connection.rs +++ b/h3/src/connection.rs @@ -565,6 +565,22 @@ where ), ))); } + Err(FrameStreamError::PayloadTooLarge { + frame_type: _ty, + actual_size: _actual, + max_size: _max, + }) => { + // Unlike a request stream, there is no message to reject here: + // every frame the control stream may legitimately carry is tens + // of bytes, and the stream itself is critical. + return Poll::Ready(Err(self.handle_connection_error( + InternalConnectionError::new( + Code::H3_EXCESSIVE_LOAD, + "control frame declared a payload larger than this endpoint will buffer" + .to_string(), + ), + ))); + } Err(FrameStreamError::Proto(frame_error)) => { return Poll::Ready(Err(self.handle_connection_error( InternalConnectionError::got_frame_error(frame_error), @@ -858,9 +874,18 @@ where if !self.stream.has_data() { match ready!(self.stream.poll_next(cx)) { Err(frame_stream_error) => { + if matches!( + frame_stream_error, + FrameStreamError::PayloadTooLarge { + frame_type: 0x1, + .. + } + ) { + self.stream.stop_sending(Code::H3_REQUEST_CANCELLED); + } return Poll::Ready(Err( self.handle_frame_stream_error_on_request_stream(frame_stream_error) - )) + )); } Ok(None) => return Poll::Ready(Ok(None)), Ok(Some(Frame::Headers(encoded))) => { @@ -919,9 +944,18 @@ where } else { match ready!(self.stream.poll_next(cx)) { Err(frame_stream_error) => { + if matches!( + frame_stream_error, + FrameStreamError::PayloadTooLarge { + frame_type: 0x1, + .. + } + ) { + self.stream.stop_sending(Code::H3_REQUEST_CANCELLED); + } return Poll::Ready(Err( self.handle_frame_stream_error_on_request_stream(frame_stream_error) - )) + )); } Ok(None) => return Poll::Ready(Ok(None)), Ok(Some(Frame::Headers(encoded))) => encoded, @@ -967,9 +1001,18 @@ where match self.stream.poll_next(cx) { Poll::Ready(Err(frame_stream_error)) => { + if matches!( + frame_stream_error, + FrameStreamError::PayloadTooLarge { + frame_type: 0x1, + .. + } + ) { + self.stream.stop_sending(Code::H3_REQUEST_CANCELLED); + } return Poll::Ready(Err( self.handle_frame_stream_error_on_request_stream(frame_stream_error) - )) + )); } Poll::Ready(Ok(Some(trailing_frame))) => { // Received a known frame after trailers -> fail. diff --git a/h3/src/error/connection_error_creators.rs b/h3/src/error/connection_error_creators.rs index 93193e20..c99e95fc 100644 --- a/h3/src/error/connection_error_creators.rs +++ b/h3/src/error/connection_error_creators.rs @@ -197,6 +197,22 @@ where FrameStreamError::Proto(frame_error) => self.handle_connection_error_on_stream( InternalConnectionError::got_frame_error(frame_error), ), + // A HEADERS limit is per message: rejecting one request must not + // close unrelated streams on the same connection. + FrameStreamError::PayloadTooLarge { + frame_type: 0x1, + actual_size, + max_size, + } => StreamError::HeaderTooBig { + actual_size, + max_size, + }, + FrameStreamError::PayloadTooLarge { .. } => { + self.handle_connection_error_on_stream(InternalConnectionError::new( + Code::H3_EXCESSIVE_LOAD, + "frame declared a payload larger than this endpoint will buffer".to_string(), + )) + } FrameStreamError::UnexpectedEnd => { self.handle_connection_error_on_stream(InternalConnectionError::new( Code::H3_FRAME_ERROR, diff --git a/h3/src/frame.rs b/h3/src/frame.rs index f9efc57b..61fd5fdc 100644 --- a/h3/src/frame.rs +++ b/h3/src/frame.rs @@ -29,9 +29,23 @@ pub struct FrameStream { impl FrameStream { pub fn new(stream: BufRecvStream) -> Self { + Self::new_with_max_buffered_frame(stream, DEFAULT_MAX_BUFFERED_FRAME) + } + + /// `max_buffered_frame` is a memory bound, not a policy limit: it must not + /// be tighter than what this endpoint has advertised it will accept, or a + /// field section would be refused from its encoded length before QPACK can + /// report the uncompressed size that [`RFC 9114 §4.2.2`] is written in + /// terms of, and that a peer needs in order to retry. + /// + /// [`RFC 9114 §4.2.2`]: https://www.rfc-editor.org/rfc/rfc9114#section-4.2.2 + pub(crate) fn new_with_max_buffered_frame( + stream: BufRecvStream, + max_buffered_frame: u64, + ) -> Self { Self { stream, - decoder: FrameDecoder::default(), + decoder: FrameDecoder::new(max_buffered_frame), remaining_data: 0, } } @@ -88,7 +102,7 @@ where Poll::Ready(false) => continue, Poll::Pending => return Poll::Pending, Poll::Ready(true) => { - if self.stream.buf_mut().has_remaining() { + if self.stream.buf_mut().has_remaining() || self.decoder.is_incomplete() { // Reached the end of receive stream, but there is still some data: // The frame is incomplete. return Poll::Ready(Err(FrameStreamError::UnexpectedEnd)); @@ -212,12 +226,51 @@ where } } -#[derive(Default)] +/// Default ceiling on the declared length of a frame that must be buffered in +/// full before it can be parsed. +/// +/// Generous next to anything legitimate — field sections in the wild are a few +/// kilobytes, and the control frames are tens of bytes — and finite, which is +/// the property that was missing. Nothing that works today against a +/// well-behaved peer stops working. +pub const DEFAULT_MAX_BUFFERED_FRAME: u64 = 1024 * 1024; + pub struct FrameDecoder { expected: Option, + /// Payload bytes of an unknown frame still to be discarded. + /// + /// Ignoring an unknown frame used to mean buffering it in full and then + /// dropping it, because the type was only recognised once `Frame::decode` + /// had the whole payload. It is recognised from the header now, and this is + /// what is left to throw away as it arrives. + skipping: u64, + /// See [`DEFAULT_MAX_BUFFERED_FRAME`]. + max_buffered: u64, +} + +impl Default for FrameDecoder { + fn default() -> Self { + Self { + expected: None, + skipping: 0, + max_buffered: DEFAULT_MAX_BUFFERED_FRAME, + } + } } impl FrameDecoder { + fn new(max_buffered: u64) -> Self { + Self { + expected: None, + skipping: 0, + max_buffered, + } + } + + fn is_incomplete(&self) -> bool { + self.expected.is_some() || self.skipping > 0 + } + fn decode( &mut self, src: &mut BufList, @@ -229,6 +282,18 @@ impl FrameDecoder { return Ok(None); } + // An unknown frame's payload, discarded as it arrives rather than + // after all of it has been stored. + if self.skipping > 0 { + let discard = self.skipping.min(src.remaining() as u64); + src.advance(discard as usize); + self.skipping -= discard; + if self.skipping > 0 { + return Ok(None); + } + continue; + } + if let Some(min) = self.expected { if src.remaining() < min { return Ok(None); @@ -237,12 +302,12 @@ impl FrameDecoder { let (pos, decoded) = { let mut cur = src.cursor(); - let decoded = Frame::decode(&mut cur); + let decoded = Frame::decode(&mut cur, self.max_buffered); (cur.position(), decoded) }; match decoded { - Err(frame::FrameError::UnknownFrame(_ty)) => { + Err(frame::FrameError::SkipUnknown { ty: _ty, len }) => { //= https://www.rfc-editor.org/rfc/rfc9114#section-4.1 //# Frames of unknown types (Section 9), including reserved frames //# (Section 7.2.8) MAY be sent on a request or push stream before, @@ -250,6 +315,42 @@ impl FrameDecoder { //= https://www.rfc-editor.org/rfc/rfc9114#section-7.2.8 //# Endpoints MUST //# NOT consider these frames to have any meaning upon receipt. + #[cfg(feature = "tracing")] + trace!("ignore unknown frame type {:#x}, {} bytes", _ty, len); + + // Only the header is consumed here. The payload goes to the + // branch above as it arrives, so ignoring an enormous + // unknown frame costs nothing to hold. + src.advance(pos); + self.expected = None; + self.skipping = len; + continue; + } + Err(frame::FrameError::TooLarge { + ty: _ty, + len: _len, + limit: _limit, + }) => { + #[cfg(feature = "tracing")] + trace!( + "frame type {:#x} declared {} bytes, over the {} ceiling", + _ty, + _len, + _limit + ); + + // Nothing of this frame was accumulated, and what came + // before it is released rather than left for a caller that + // is about to tear the stream down anyway. + src.advance(src.remaining()); + self.expected = None; + return Err(FrameStreamError::PayloadTooLarge { + frame_type: _ty, + actual_size: _len, + max_size: _limit, + }); + } + Err(frame::FrameError::UnknownFrame(_ty)) => { #[cfg(feature = "tracing")] trace!("ignore unknown frame type {:#x}", _ty); @@ -304,6 +405,14 @@ pub enum FrameStreamError { Proto(FrameProtocolError), Quic(StreamErrorIncoming), UnexpectedEnd, + /// A frame that must be buffered to be parsed declared a payload past the + /// decoder's ceiling. The owner decides from the frame context whether + /// this is a message-level rejection or a connection error. + PayloadTooLarge { + frame_type: u64, + actual_size: u64, + max_size: u64, + }, } #[derive(Debug, PartialEq)] @@ -399,6 +508,81 @@ mod tests { assert_matches!(decoder.decode(&mut buf), Ok(None)); } + #[test] + fn skips_unknown_payload_incrementally_then_decodes_next_frame() { + // Unknown type 22, declared payload length 63. Only ten payload bytes + // arrive with the header; they must be discarded, not retained. + let mut first = vec![22, 63]; + first.extend([0_u8; 10]); + let mut buf = BufList::from(Bytes::from(first)); + let mut decoder = FrameDecoder::default(); + + assert_matches!(decoder.decode(&mut buf), Ok(None)); + assert_eq!(buf.remaining(), 0); + assert_eq!(decoder.skipping, 53); + + // The rest of the unknown payload and a valid DATA header arrive + // together. The decoder must discard exactly the former and preserve + // framing for the latter. + let mut rest = vec![0_u8; 53]; + rest.extend([0, 4]); + buf.push(Bytes::from(rest)); + + assert_matches!( + decoder.decode(&mut buf), + Ok(Some(Frame::Data(PayloadLen(4)))) + ); + assert_eq!(decoder.skipping, 0); + assert_eq!(buf.remaining(), 0); + } + + #[test] + fn an_enormous_unknown_frame_costs_nothing_to_ignore() { + // 0x21 is the first of the reserved GREASE frame types: an unknown + // frame is not by itself an attack, it is something h3 sends on + // purpose, and it must be ignored whatever length it declares. + // Ignoring it is what must not cost the declared length. + const DECLARED: u64 = 4 * 1024 * 1024 * 1024; + const CHUNK: usize = 1024 * 1024; + + let mut header = BytesMut::new(); + VarInt::from_u64(0x21).unwrap().encode(&mut header); + VarInt::from_u64(DECLARED).unwrap().encode(&mut header); + + let mut buf = BufList::from(header.freeze()); + let mut decoder = FrameDecoder::default(); + + // The header on its own settles what happens to the payload. Note that + // the declared length exceeds u32 and, on a 32-bit target, `usize`: + // what is left to discard is counted, never sized into an allocation. + assert_matches!(decoder.decode(&mut buf), Ok(None)); + assert_eq!(decoder.skipping, DECLARED); + assert_eq!(buf.remaining(), 0); + + // Stream the payload past the decoder. `BufList::advance` drops the + // chunks it consumes, so "nothing remaining" is "nothing retained": + // the peak cost here is one chunk, not the four gigabytes declared. + let chunk = Bytes::from(vec![0_u8; CHUNK]); + for _ in 0..(DECLARED / CHUNK as u64) - 1 { + buf.push(chunk.clone()); + assert_matches!(decoder.decode(&mut buf), Ok(None)); + assert_eq!(buf.remaining(), 0, "a payload being ignored was retained"); + } + assert_eq!(decoder.skipping, CHUNK as u64); + + // The last of the ignored payload arrives in the same chunk as a real + // frame. The decoder has to discard exactly the former — one byte + // either way desynchronises the stream — and then parse the latter. + let mut tail = BytesMut::with_capacity(CHUNK + 16); + tail.put_bytes(0, CHUNK); + Frame::headers(&b"salut"[..]).encode_with_payload(&mut tail); + buf.push(tail.freeze()); + + assert_matches!(decoder.decode(&mut buf), Ok(Some(Frame::Headers(_)))); + assert_eq!(decoder.skipping, 0); + assert_eq!(buf.remaining(), 0); + } + // FrameStream macro_rules! assert_poll_matches { @@ -496,6 +680,37 @@ mod tests { ); } + #[tokio::test] + async fn truncated_unknown_frame_is_an_unexpected_end() { + let mut recv = FakeRecv::default(); + // Unknown type 22 declares four payload bytes but sends only two. + recv.chunk(Bytes::from_static(&[22, 4, b'a', b'b'])); + let mut stream: FrameStream<_, ()> = FrameStream::new(BufRecvStream::new(recv)); + + assert_poll_matches!( + |cx| stream.poll_next(cx), + Err(FrameStreamError::UnexpectedEnd) + ); + } + + #[tokio::test] + async fn oversized_buffered_frame_fails_from_header_alone() { + let mut recv = FakeRecv::default(); + // HEADERS declares 63 bytes and sends no payload. + recv.chunk(Bytes::from_static(&[1, 63])); + let mut stream: FrameStream<_, ()> = + FrameStream::new_with_max_buffered_frame(BufRecvStream::new(recv), 62); + + assert_poll_matches!( + |cx| stream.poll_next(cx), + Err(FrameStreamError::PayloadTooLarge { + frame_type: 1, + actual_size: 63, + max_size: 62 + }) + ); + } + #[tokio::test] #[should_panic( expected = "There is still data to read, please call poll_data() until it returns None" diff --git a/h3/src/proto/frame.rs b/h3/src/proto/frame.rs index a09520bb..5e51b373 100644 --- a/h3/src/proto/frame.rs +++ b/h3/src/proto/frame.rs @@ -25,6 +25,27 @@ pub enum FrameError { Settings(SettingsError), InvalidStreamId(InvalidStreamId), InvalidPushId(InvalidPushId), + /// A buffered frame declared a payload larger than the configured ceiling. + /// + /// Returned from the declared length alone, before any of the payload is + /// accumulated — which is the whole point. Every other error here can be + /// reached only once the frame is in memory. + TooLarge { + ty: u64, + len: u64, + limit: u64, + }, + /// An unknown frame type, reported *before* its payload has been read so + /// the caller can discard it as it arrives. + /// + /// Distinct from [`FrameError::UnknownFrame`], which is returned once the + /// payload is already buffered. RFC 9114 requires unknown frames to be + /// ignored; accumulating one in full in order to ignore it satisfies the + /// letter and not the substance. + SkipUnknown { + ty: u64, + len: u64, + }, } impl std::error::Error for FrameError {} @@ -40,6 +61,14 @@ impl fmt::Display for FrameError { FrameError::Settings(x) => write!(f, "invalid settings: {}", x), FrameError::InvalidStreamId(x) => write!(f, "{}", x), FrameError::InvalidPushId(x) => write!(f, "{}", x), + FrameError::TooLarge { ty, len, limit } => write!( + f, + "frame 0x{:x} declared {} bytes, over the {} byte ceiling", + ty, len, limit + ), + FrameError::SkipUnknown { ty, len } => { + write!(f, "frame 0x{:x} ignored, {} bytes skipped", ty, len) + } } } } @@ -79,7 +108,22 @@ impl Frame { pub const MAX_ENCODED_SIZE: usize = VarInt::MAX_SIZE * 7; /// Decodes a Frame from the stream according to - pub fn decode(buf: &mut T) -> Result { + /// + /// `max_buffered` bounds the declared payload length of a frame this + /// function has to hold in memory to parse. It is checked against the + /// length field, before the caller is ever told to accumulate — which is + /// the difference that matters. Previously the completeness check came + /// first, so a peer declaring a large `HEADERS` had its payload buffered in + /// full before `max_field_section_size` could reject it, and a peer + /// declaring a large *unknown* frame had it buffered in full before it + /// could be ignored. Neither was bounded by flow control: the caller + /// consumes each chunk as it arrives, which is what releases the credit for + /// the next one. + /// + /// `DATA` is unaffected. Its payload is never held here — the frame carries + /// only a length and the body is streamed — so the ceiling does not apply + /// to it and request bodies keep whatever bound their reader already has. + pub fn decode(buf: &mut T, max_buffered: u64) -> Result { let remaining = buf.remaining(); let ty = FrameType::decode(buf).map_err(|_| FrameError::Incomplete(remaining + 1))?; @@ -101,6 +145,28 @@ impl Frame { return Ok(Frame::Data((len as usize).into())); } + // Everything below is decided from the type and the declared length + // alone, before anything asks the caller to hold a payload. + + // A reserved HTTP/2 type is a connection error whatever it carries, so + // reading what it carries is pure cost. + if ty.is_reserved() { + return Err(FrameError::UnsupportedFrame(ty.0)); + } + + // Unknown: to be ignored, and ignoring it must not mean storing it. + if !ty.is_buffered() { + return Err(FrameError::SkipUnknown { ty: ty.0, len }); + } + + if len > max_buffered { + return Err(FrameError::TooLarge { + ty: ty.0, + len, + limit: max_buffered, + }); + } + if buf.remaining() < len as usize { return Err(FrameError::Incomplete(2 + len as usize)); } @@ -317,6 +383,43 @@ impl FrameType { pub fn grease() -> Self { FrameType(fastrand::u64(0..0x210842108421083) * 0x1f + 0x21) } + + /// Whether parsing this type requires its whole payload in memory. + /// + /// The list is exactly the arms of the match in [`Frame::decode`] that read + /// from `payload`. `DATA` is absent because its body is streamed rather + /// than held, and `WEBTRANSPORT_BI_STREAM` because it carries no length at + /// all — both are handled before this is consulted. + /// + /// Kept beside the type list rather than derived from the match so that + /// adding a frame type to one and not the other is a compile-time + /// omission with a visible home, instead of a type that silently becomes + /// unbounded. + fn is_buffered(self) -> bool { + matches!( + self, + FrameType::HEADERS + | FrameType::SETTINGS + | FrameType::CANCEL_PUSH + | FrameType::PUSH_PROMISE + | FrameType::GOAWAY + | FrameType::MAX_PUSH_ID + ) + } + + /// The HTTP/2 types RFC 9114 §7.2.8 requires be treated as a connection + /// error of type `H3_FRAME_UNEXPECTED`. + /// + /// Decided by the type, so the payload is never worth reading. + fn is_reserved(self) -> bool { + matches!( + self, + FrameType::H2_PRIORITY + | FrameType::H2_PING + | FrameType::H2_WINDOW_UPDATE + | FrameType::H2_CONTINUATION + ) + } } #[derive(Copy, Clone, Eq, PartialEq, Debug)] @@ -619,31 +722,85 @@ mod tests { use assert_matches::assert_matches; use std::io::Cursor; + /// An unknown type is now reported from its header, with the payload left + /// for the caller to discard as it arrives. + /// + /// It used to be reported as `UnknownFrame` *after* the whole payload had + /// been buffered, which is how ignoring an unknown frame came to cost its + /// full declared length in memory. `FrameDecoder` does the skipping, and + /// `frame::tests` covers reading the frame that follows. #[test] fn unknown_frame_type() { let mut buf = Cursor::new(&[22, 4, 0, 255, 128, 0, 3, 1, 2]); - assert_matches!(Frame::decode(&mut buf), Err(FrameError::UnknownFrame(22))); - assert_matches!(Frame::decode(&mut buf), Ok(Frame::CancelPush(PushId(2)))); + assert_matches!( + Frame::decode(&mut buf, u64::MAX), + Err(FrameError::SkipUnknown { ty: 22, len: 4 }) + ); + } + + /// The ceiling is applied to the declared length, with no payload present. + /// + /// The buffer holds a two-byte header and nothing else, so a decoder that + /// checked completeness first would answer `Incomplete` and wait — which is + /// precisely the accumulation this exists to prevent. + #[test] + fn oversized_buffered_frame_is_refused_from_its_length_alone() { + // HEADERS, declared length 63, no payload. 63 is the largest value a + // single-byte QUIC varint holds, which keeps the fixture two bytes. + let mut buf = Cursor::new(&[1, 63]); + assert_matches!( + Frame::decode(&mut buf, 62), + Err(FrameError::TooLarge { + ty: 1, + len: 63, + limit: 62 + }) + ); + + // Exactly at the ceiling is not over it: this must reach the ordinary + // completeness path instead. + let mut buf = Cursor::new(&[1, 63]); + assert_matches!(Frame::decode(&mut buf, 63), Err(FrameError::Incomplete(_))); + } + + /// A reserved HTTP/2 type is a connection error decided by the type, so its + /// payload must never be waited for either. + #[test] + fn a_reserved_frame_is_refused_without_its_payload() { + // H2_PING (0x6), declared length 63, no payload. + let mut buf = Cursor::new(&[6, 63]); + assert_matches!( + Frame::decode(&mut buf, u64::MAX), + Err(FrameError::UnsupportedFrame(6)) + ); + } + + /// DATA is untouched: it declares a length and streams its body, so the + /// ceiling does not apply and request bodies keep their own bound. + #[test] + fn data_frames_are_not_subject_to_the_buffered_ceiling() { + let mut buf = Cursor::new(&[0, 63]); + assert_matches!(Frame::decode(&mut buf, 1), Ok(Frame::Data(PayloadLen(63)))); } #[test] fn len_unexpected_end() { let mut buf = Cursor::new(&[0, 255]); - let decoded = Frame::decode(&mut buf); + let decoded = Frame::decode(&mut buf, u64::MAX); assert_matches!(decoded, Err(FrameError::Incomplete(3))); } #[test] fn type_unexpected_end() { let mut buf = Cursor::new(&[255]); - let decoded = Frame::decode(&mut buf); + let decoded = Frame::decode(&mut buf, u64::MAX); assert_matches!(decoded, Err(FrameError::Incomplete(2))); } #[test] fn buffer_too_short() { let mut buf = Cursor::new(&[4, 4, 0, 255, 128]); - let decoded = Frame::decode(&mut buf); + let decoded = Frame::decode(&mut buf, u64::MAX); assert_matches!(decoded, Err(FrameError::Incomplete(6))); } @@ -653,7 +810,7 @@ mod tests { assert_eq!(&buf, &wire); let mut read = Cursor::new(&buf); - let decoded = Frame::decode(&mut read).unwrap(); + let decoded = Frame::decode(&mut read, u64::MAX).unwrap(); assert_eq!(check_frame, decoded); } @@ -756,7 +913,10 @@ mod tests { VarInt::from_u32(0x21 + 2 * 0x1f).encode(&mut raw); raw.extend(&[6, 0, 255, 128, 0, 250, 218]); let mut buf = Cursor::new(&raw); - let decoded = Frame::decode(&mut buf); - assert_matches!(decoded, Err(FrameError::UnknownFrame(95))); + let decoded = Frame::decode(&mut buf, u64::MAX); + // Reported from the header now rather than after the payload has been + // buffered: a GREASE type is unknown, and ignoring an unknown frame no + // longer costs its declared length in memory. + assert_matches!(decoded, Err(FrameError::SkipUnknown { ty: 95, len: 6 })); } } diff --git a/h3/src/server/connection.rs b/h3/src/server/connection.rs index 1a3a35d8..f4dee369 100644 --- a/h3/src/server/connection.rs +++ b/h3/src/server/connection.rs @@ -127,7 +127,11 @@ where pub async fn accept(&mut self) -> Result>, ConnectionError> { // Accept the incoming stream let stream = match poll_fn(|cx| self.poll_accept_request_stream_internal(cx)).await? { - Some(s) => FrameStream::new(BufRecvStream::new(s)), + Some(s) => FrameStream::new_with_max_buffered_frame( + BufRecvStream::new(s), + self.max_field_section_size + .max(crate::frame::DEFAULT_MAX_BUFFERED_FRAME), + ), None => { // We always send a last GoAway frame to the client, so it knows which was the last // non-rejected request. diff --git a/h3/src/server/request.rs b/h3/src/server/request.rs index 01599921..bf291aa5 100644 --- a/h3/src/server/request.rs +++ b/h3/src/server/request.rs @@ -125,6 +125,16 @@ where ); } Err(e) => { + if matches!( + e, + FrameStreamError::PayloadTooLarge { + frame_type: 0x1, + .. + } + ) { + self.frame_stream.stop_sending(Code::H3_REQUEST_CANCELLED); + self.frame_stream.reset(Code::H3_REQUEST_CANCELLED.value()); + } return Err(self.handle_frame_stream_error_on_request_stream(e)); } }; From 6702eb0a7bacfa9f8ebe9bce33827a70d1220627 Mon Sep 17 00:00:00 2001 From: Jakob Povsic Date: Thu, 22 Oct 2026 00:49:07 +0000 Subject: [PATCH 2/3] Bound decoded header memory by default and stop panicking on huge field sections QPACK field sections decode to many times their wire size, so a peer could make this endpoint commit unbounded memory per request ("header accordion" amplification, https://foxio.io/blog/header-accordions-qpack-amplification-attacks-against-http-3-servers). * Config now defaults max_field_section_size to 64 KiB instead of the varint maximum. Settings::default keeps the RFC 9114 7.2.4.2 initial values, which are what we assume about a peer until its SETTINGS arrive, so clients still send freely before that. * Header::try_from uses HeaderMap::try_with_capacity and try_append, mapping overflow to HeaderError::TooManyFields; http::HeaderMap panics past 32768 entries, which was reachable from the wire. Co-Authored-By: Claude Mythos 5.1 --- h3/src/config.rs | 21 ++++++++++---- h3/src/proto/headers.rs | 19 +++++++++++-- h3/src/tests/request.rs | 62 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 8 deletions(-) diff --git a/h3/src/config.rs b/h3/src/config.rs index c2a6fe64..efff0c64 100644 --- a/h3/src/config.rs +++ b/h3/src/config.rs @@ -22,11 +22,8 @@ pub struct Config { /// HTTP/3 Settings #[derive(Debug, Clone, Copy)] pub struct Settings { - /// The MAX_FIELD_SECTION_SIZE in HTTP/3 refers to the maximum size of the dynamic table used in HPACK compression. - /// HPACK is the compression algorithm used in HTTP/3 to reduce the size of the header fields in HTTP requests and responses. - - /// In HTTP/3, the MAX_FIELD_SECTION_SIZE is set to 12. - /// This means that the dynamic table used for HPACK compression can have a maximum size of 2^12 bytes, which is 4KB. + /// SETTINGS_MAX_FIELD_SECTION_SIZE: the largest decoded field section (name + value + 32 bytes + /// per field, ) this endpoint accepts. pub(crate) max_field_section_size: u64, /// https://datatracker.ietf.org/doc/html/draft-ietf-webtrans-http3/#section-3.1 @@ -134,6 +131,15 @@ impl TryFrom for frame::Settings { } } +/// Default for the field section size this endpoint accepts, see [`Config`]. +/// +/// Bounds the memory a peer can make this endpoint commit per request or response; a HEADERS +/// frame decodes to many times its wire size, so the limit must be finite by default. Raise it +/// with the client or server builder if peers legitimately send larger headers. +pub const DEFAULT_MAX_FIELD_SECTION_SIZE: u64 = 64 * 1024; + +/// The initial values of , assumed for a +/// peer until its SETTINGS frame arrives. Not the local defaults: those are set by [`Config`]. impl Default for Settings { fn default() -> Self { Self { @@ -173,7 +179,10 @@ impl Default for Config { send_grease: true, #[cfg(test)] send_settings: true, - settings: Default::default(), + settings: Settings { + max_field_section_size: DEFAULT_MAX_FIELD_SECTION_SIZE, + ..Default::default() + }, } } } diff --git a/h3/src/proto/headers.rs b/h3/src/proto/headers.rs index d84ed0ed..83aada0b 100644 --- a/h3/src/proto/headers.rs +++ b/h3/src/proto/headers.rs @@ -204,7 +204,8 @@ impl Iterator for HeaderIter { impl TryFrom> for Header { type Error = HeaderError; fn try_from(headers: Vec) -> Result { - let mut fields = HeaderMap::with_capacity(headers.len()); + let mut fields = + HeaderMap::try_with_capacity(headers.len()).map_err(|_| HeaderError::TooManyFields)?; let mut pseudo = Pseudo::default(); let mut regular_field_seen = false; @@ -247,7 +248,9 @@ impl TryFrom> for Header { } Field::Header((n, v)) => { regular_field_seen = true; - fields.append(n, v); + fields + .try_append(n, v) + .map_err(|_| HeaderError::TooManyFields)?; } Field::Protocol(p) => { pseudo.protocol = Some(p); @@ -466,6 +469,8 @@ pub enum HeaderError { MissingAuthority, ContradictedAuthority, PseudoAfterRegularField, + /// More fields than an `http::HeaderMap` can hold + TooManyFields, } impl HeaderError { @@ -494,6 +499,7 @@ impl std::error::Error for HeaderError {} impl fmt::Display for HeaderError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { + HeaderError::TooManyFields => write!(f, "too many header fields"), HeaderError::InvalidHeaderName(h) => write!(f, "invalid header name: {}", h), HeaderError::InvalidHeaderValue(v) => write!(f, "invalid header value: {}", v), HeaderError::InvalidRequest(r) => write!(f, "invalid request: {}", r), @@ -518,6 +524,15 @@ mod tests { use super::*; use assert_matches::assert_matches; + #[test] + fn too_many_fields_is_an_error_not_a_panic() { + // http::HeaderMap panics past 32768 entries; a peer must not be able to trigger that. + let fields = (0..40_000) + .map(|i| (format!("h{}", i), "v").into()) + .collect::>(); + assert_matches!(Header::try_from(fields), Err(HeaderError::TooManyFields)); + } + #[test] fn request_has_no_authority_nor_host() { //= https://www.rfc-editor.org/rfc/rfc9114#section-4.3.1 diff --git a/h3/src/tests/request.rs b/h3/src/tests/request.rs index 158db7b5..2eb3d11f 100644 --- a/h3/src/tests/request.rs +++ b/h3/src/tests/request.rs @@ -288,6 +288,68 @@ async fn post() { tokio::join!(server_fut, client_fut); } +#[tokio::test] +async fn header_too_big_by_default() { + // A HEADERS frame decodes to many times its wire size; the default limit must be finite. + init_tracing(); + let mut pair = Pair::default(); + let mut server = pair.server(); + + let client_fut = async { + let (mut driver, mut client) = client::new(pair.client().await).await.expect("client init"); + let drive_fut = async { future::poll_fn(|cx| driver.poll_close(cx)).await }; + let req_fut = async move { + let mut request = Request::get("http://localhost/salut").body(()).unwrap(); + for i in 0..2500 { + request.headers_mut().append( + http::HeaderName::from_bytes(format!("h{}", i).as_bytes()).unwrap(), + http::HeaderValue::from_static("v"), + ); + } + let mut request_stream = client.send_request(request).await.expect("request"); + request_stream.finish().await.expect("client finish"); + let response = request_stream.recv_response().await.unwrap(); + assert_eq!( + response.status(), + StatusCode::REQUEST_HEADER_FIELDS_TOO_LARGE + ); + }; + tokio::join!(req_fut, drive_fut); + }; + + let server_fut = async { + let conn = server.next().await; + // Don't send settings, so the client doesn't refuse the request locally + let mut incoming_req = server::builder() + .send_settings(false) + .build(conn) + .await + .unwrap(); + + let resolver = incoming_req.accept().await.unwrap().unwrap(); + assert_matches!( + resolver + .resolve_request() + .await + .err() + .expect("should return an error"), + StreamError::HeaderTooBig { + max_size: crate::config::DEFAULT_MAX_FIELD_SECTION_SIZE, + .. + } + ); + + // connection will end without an error + assert_matches!( + incoming_req.accept().await.err().unwrap(), + ConnectionError::Remote(ConnectionErrorIncoming::ApplicationClose{error_code: code, ..}) + if code == Code::H3_NO_ERROR.value() + ); + }; + + tokio::join!(server_fut, client_fut); +} + #[tokio::test] async fn header_too_big_response_from_server() { init_tracing(); From e40e3f1ecb7db6a3678f815a7ee941a362700b73 Mon Sep 17 00:00:00 2001 From: Jakob Povsic Date: Thu, 22 Oct 2026 01:13:14 +0000 Subject: [PATCH 3/3] Address review: poison the decoder after a refused frame, one 431 path, shared frame bound * FrameDecoder latches after refusing an oversized frame. What follows is the declined payload, not frames; previously a byte of it could parse as a SETTINGS or reserved frame header and turn a per-stream rejection into a connection close. * An oversized request HEADERS now takes the same 431 path as a decoded section over the limit, instead of a RESET the client cannot tell from a cancel. * The request-stream buffer bound is computed once, in FrameStream::new_for_request, and the server resolver builds the stream itself so the WebTransport accept path gets the same bound. * The HEADERS stop_sending lives in one helper on RequestStream, matched on StreamError::HeaderTooBig, replacing five copies keyed on a 0x1 literal. PayloadTooLarge carries a FrameType. * Frame::decode picks its parser from one table, which is also what decides whether a type is buffered; is_buffered, UnknownFrame and the dead match arms are gone. * FrameDecoder::default delegates to new; builder docs state the 64 KiB default. Co-Authored-By: Claude Mythos 5.1 --- h3-webtransport/src/server.rs | 2 - h3/src/client/builder.rs | 3 +- h3/src/client/connection.rs | 5 +- h3/src/client/stream.rs | 27 ++---- h3/src/connection.rs | 50 +++-------- h3/src/error/connection_error_creators.rs | 2 +- h3/src/frame.rs | 105 ++++++++++++++-------- h3/src/proto/frame.rs | 85 +++++------------- h3/src/server/builder.rs | 3 +- h3/src/server/connection.rs | 18 ++-- h3/src/server/request.rs | 60 +++++++------ h3/src/tests/request.rs | 76 ++++++++++++++++ 12 files changed, 233 insertions(+), 203 deletions(-) diff --git a/h3-webtransport/src/server.rs b/h3-webtransport/src/server.rs index a2d711f7..6636a2e6 100644 --- a/h3-webtransport/src/server.rs +++ b/h3-webtransport/src/server.rs @@ -14,7 +14,6 @@ use h3::{ ConnectionError, StreamError, }, ext::Protocol, - frame::FrameStream, proto::frame::Frame, quic::{self, OpenStreams, WriteBuf}, server::{Connection, RequestResolver, RequestStream}, @@ -189,7 +188,6 @@ where .await?; Ok(stream.map(|stream| { - let stream = FrameStream::new(BufRecvStream::new(stream)); let resolver = self.server_conn.lock().unwrap().create_resolver(stream); BiStreamResolver { resolver } })) diff --git a/h3/src/client/builder.rs b/h3/src/client/builder.rs index 5ae628d7..2dde3506 100644 --- a/h3/src/client/builder.rs +++ b/h3/src/client/builder.rs @@ -80,7 +80,8 @@ impl Builder { /// Set the maximum header size this client is willing to accept /// - /// See [header size constraints] section of the specification for details. + /// Defaults to 64 KiB. Measured as decoded name + value + 32 bytes per field, + /// see [header size constraints] section of the specification for details. /// /// [header size constraints]: https://www.rfc-editor.org/rfc/rfc9114.html#name-header-size-constraints pub fn max_field_section_size(&mut self, value: u64) -> &mut Self { diff --git a/h3/src/client/connection.rs b/h3/src/client/connection.rs index a5a60445..aa8fbc31 100644 --- a/h3/src/client/connection.rs +++ b/h3/src/client/connection.rs @@ -216,10 +216,9 @@ where let request_stream = RequestStream { inner: connection::RequestStream::new( - FrameStream::new_with_max_buffered_frame( + FrameStream::new_for_request( BufRecvStream::new(stream), - self.max_field_section_size - .max(crate::frame::DEFAULT_MAX_BUFFERED_FRAME), + self.max_field_section_size, ), self.max_field_section_size, self.conn_state.clone(), diff --git a/h3/src/client/stream.rs b/h3/src/client/stream.rs index 90f41907..68f07351 100644 --- a/h3/src/client/stream.rs +++ b/h3/src/client/stream.rs @@ -8,11 +8,9 @@ use tracing::instrument; use crate::{ connection::{self}, error::{ - connection_error_creators::{CloseStream, HandleFrameStreamErrorOnRequestStream}, - internal_error::InternalConnectionError, - Code, StreamError, + connection_error_creators::CloseStream, internal_error::InternalConnectionError, Code, + StreamError, }, - frame::FrameStreamError, proto::{frame::Frame, headers::Header}, qpack, quic::{self}, @@ -98,18 +96,9 @@ where /// [`recv_data()`]: #method.recv_data #[cfg_attr(feature = "tracing", instrument(skip_all, level = "trace"))] pub async fn recv_response(&mut self) -> Result, StreamError> { - let frame = future::poll_fn(|cx| self.inner.stream.poll_next(cx)).await; - if matches!( - frame, - Err(FrameStreamError::PayloadTooLarge { - frame_type: 0x1, - .. - }) - ) { - self.inner.stop_sending(Code::H3_REQUEST_CANCELLED); - } - let mut frame = frame - .map_err(|e| self.handle_frame_stream_error_on_request_stream(e))? + let mut frame = future::poll_fn(|cx| self.inner.stream.poll_next(cx)) + .await + .map_err(|e| self.inner.frame_stream_error(e))? .ok_or_else(|| { //= https://www.rfc-editor.org/rfc/rfc9114#section-4.1 //# Receipt of an invalid sequence of frames MUST be treated as a @@ -231,11 +220,7 @@ where &mut self, cx: &mut Context<'_>, ) -> Poll, StreamError>> { - let res = self.inner.poll_recv_trailers(cx); - if let Poll::Ready(Err(StreamError::HeaderTooBig { .. })) = &res { - self.inner.stream.stop_sending(Code::H3_REQUEST_CANCELLED); - } - res + self.inner.poll_recv_trailers(cx) } /// Tell the peer to stop sending into the underlying QUIC stream diff --git a/h3/src/connection.rs b/h3/src/connection.rs index b19ff254..bbdee050 100644 --- a/h3/src/connection.rs +++ b/h3/src/connection.rs @@ -867,6 +867,16 @@ where { /// Receive some of the request/response body. #[cfg_attr(feature = "tracing", instrument(skip_all, level = "trace"))] + /// Maps a frame stream error, telling the peer to stop sending when the + /// message was refused for its size: the rest of it is only cost to receive. + pub(crate) fn frame_stream_error(&mut self, error: FrameStreamError) -> StreamError { + let error = self.handle_frame_stream_error_on_request_stream(error); + if matches!(error, StreamError::HeaderTooBig { .. }) { + self.stream.stop_sending(Code::H3_REQUEST_CANCELLED); + } + error + } + pub fn poll_recv_data( &mut self, cx: &mut Context<'_>, @@ -874,18 +884,7 @@ where if !self.stream.has_data() { match ready!(self.stream.poll_next(cx)) { Err(frame_stream_error) => { - if matches!( - frame_stream_error, - FrameStreamError::PayloadTooLarge { - frame_type: 0x1, - .. - } - ) { - self.stream.stop_sending(Code::H3_REQUEST_CANCELLED); - } - return Poll::Ready(Err( - self.handle_frame_stream_error_on_request_stream(frame_stream_error) - )); + return Poll::Ready(Err(self.frame_stream_error(frame_stream_error))); } Ok(None) => return Poll::Ready(Ok(None)), Ok(Some(Frame::Headers(encoded))) => { @@ -944,18 +943,7 @@ where } else { match ready!(self.stream.poll_next(cx)) { Err(frame_stream_error) => { - if matches!( - frame_stream_error, - FrameStreamError::PayloadTooLarge { - frame_type: 0x1, - .. - } - ) { - self.stream.stop_sending(Code::H3_REQUEST_CANCELLED); - } - return Poll::Ready(Err( - self.handle_frame_stream_error_on_request_stream(frame_stream_error) - )); + return Poll::Ready(Err(self.frame_stream_error(frame_stream_error))); } Ok(None) => return Poll::Ready(Ok(None)), Ok(Some(Frame::Headers(encoded))) => encoded, @@ -1001,18 +989,7 @@ where match self.stream.poll_next(cx) { Poll::Ready(Err(frame_stream_error)) => { - if matches!( - frame_stream_error, - FrameStreamError::PayloadTooLarge { - frame_type: 0x1, - .. - } - ) { - self.stream.stop_sending(Code::H3_REQUEST_CANCELLED); - } - return Poll::Ready(Err( - self.handle_frame_stream_error_on_request_stream(frame_stream_error) - )); + return Poll::Ready(Err(self.frame_stream_error(frame_stream_error))); } Poll::Ready(Ok(Some(trailing_frame))) => { // Received a known frame after trailers -> fail. @@ -1039,6 +1016,7 @@ where //# An HTTP/3 implementation MAY impose a limit on the maximum size of //# the message header it will accept on an individual HTTP message. Err(qpack::DecoderError::HeaderTooLong(cancel_size)) => { + self.stream.stop_sending(Code::H3_REQUEST_CANCELLED); return Poll::Ready(Err(StreamError::HeaderTooBig { actual_size: cancel_size, max_size: self.max_field_section_size, diff --git a/h3/src/error/connection_error_creators.rs b/h3/src/error/connection_error_creators.rs index c99e95fc..3b3b2507 100644 --- a/h3/src/error/connection_error_creators.rs +++ b/h3/src/error/connection_error_creators.rs @@ -200,7 +200,7 @@ where // A HEADERS limit is per message: rejecting one request must not // close unrelated streams on the same connection. FrameStreamError::PayloadTooLarge { - frame_type: 0x1, + frame_type: crate::proto::frame::FrameType::HEADERS, actual_size, max_size, } => StreamError::HeaderTooBig { diff --git a/h3/src/frame.rs b/h3/src/frame.rs index 61fd5fdc..6bc9653c 100644 --- a/h3/src/frame.rs +++ b/h3/src/frame.rs @@ -13,7 +13,7 @@ use crate::stream::{BufRecvStream, WriteBuf}; use crate::{ buf::BufList, proto::{ - frame::{self, Frame, PayloadLen}, + frame::{self, Frame, FrameType, PayloadLen}, stream::StreamId, }, quic::{BidiStream, RecvStream, SendStream}, @@ -29,23 +29,30 @@ pub struct FrameStream { impl FrameStream { pub fn new(stream: BufRecvStream) -> Self { - Self::new_with_max_buffered_frame(stream, DEFAULT_MAX_BUFFERED_FRAME) + Self { + stream, + decoder: FrameDecoder::default(), + remaining_data: 0, + } } - /// `max_buffered_frame` is a memory bound, not a policy limit: it must not - /// be tighter than what this endpoint has advertised it will accept, or a + /// A request stream's decoder, whose buffer ceiling is raised to cover + /// `max_field_section_size`, the limit this endpoint advertises. + /// + /// The ceiling is a memory bound, not a policy limit: it must not be + /// tighter than what this endpoint has advertised it will accept, or a /// field section would be refused from its encoded length before QPACK can /// report the uncompressed size that [`RFC 9114 §4.2.2`] is written in /// terms of, and that a peer needs in order to retry. /// /// [`RFC 9114 §4.2.2`]: https://www.rfc-editor.org/rfc/rfc9114#section-4.2.2 - pub(crate) fn new_with_max_buffered_frame( + pub(crate) fn new_for_request( stream: BufRecvStream, - max_buffered_frame: u64, + max_field_section_size: u64, ) -> Self { Self { stream, - decoder: FrameDecoder::new(max_buffered_frame), + decoder: FrameDecoder::new(max_field_section_size.max(DEFAULT_MAX_BUFFERED_FRAME)), remaining_data: 0, } } @@ -246,15 +253,19 @@ pub struct FrameDecoder { skipping: u64, /// See [`DEFAULT_MAX_BUFFERED_FRAME`]. max_buffered: u64, + /// The frame refused over the ceiling, as (type, declared length, ceiling). + /// + /// Refused once, refused for good: what follows on the stream is the + /// payload that was declined, not frames, so it is discarded as it arrives + /// and every further poll reports the same error. Without this a byte of + /// that payload could be read as a SETTINGS or reserved frame header and + /// turn a per-stream rejection into a connection error. + refused: Option<(FrameType, u64, u64)>, } impl Default for FrameDecoder { fn default() -> Self { - Self { - expected: None, - skipping: 0, - max_buffered: DEFAULT_MAX_BUFFERED_FRAME, - } + Self::new(DEFAULT_MAX_BUFFERED_FRAME) } } @@ -264,6 +275,7 @@ impl FrameDecoder { expected: None, skipping: 0, max_buffered, + refused: None, } } @@ -275,6 +287,15 @@ impl FrameDecoder { &mut self, src: &mut BufList, ) -> Result>, FrameStreamError> { + if let Some((ty, len, limit)) = self.refused { + src.advance(src.remaining()); + return Err(FrameStreamError::PayloadTooLarge { + frame_type: ty, + actual_size: len, + max_size: limit, + }); + } + // Decode in a loop since we ignore unknown frames, and there may be // other frames already in our BufList. loop { @@ -326,17 +347,13 @@ impl FrameDecoder { self.skipping = len; continue; } - Err(frame::FrameError::TooLarge { - ty: _ty, - len: _len, - limit: _limit, - }) => { + Err(frame::FrameError::TooLarge { ty, len, limit }) => { #[cfg(feature = "tracing")] trace!( - "frame type {:#x} declared {} bytes, over the {} ceiling", - _ty, - _len, - _limit + "frame type {:?} declared {} bytes, over the {} ceiling", + ty, + len, + limit ); // Nothing of this frame was accumulated, and what came @@ -344,20 +361,13 @@ impl FrameDecoder { // is about to tear the stream down anyway. src.advance(src.remaining()); self.expected = None; + self.refused = Some((ty, len, limit)); return Err(FrameStreamError::PayloadTooLarge { - frame_type: _ty, - actual_size: _len, - max_size: _limit, + frame_type: ty, + actual_size: len, + max_size: limit, }); } - Err(frame::FrameError::UnknownFrame(_ty)) => { - #[cfg(feature = "tracing")] - trace!("ignore unknown frame type {:#x}", _ty); - - src.advance(pos); - self.expected = None; - continue; - } Err(frame::FrameError::Incomplete(min)) => { self.expected = Some(min); return Ok(None); @@ -409,7 +419,7 @@ pub enum FrameStreamError { /// decoder's ceiling. The owner decides from the frame context whether /// this is a message-level rejection or a connection error. PayloadTooLarge { - frame_type: u64, + frame_type: FrameType, actual_size: u64, max_size: u64, }, @@ -439,6 +449,26 @@ mod tests { // Decoder + #[test] + fn refused_frame_poisons_the_decoder() { + let mut decoder = FrameDecoder::new(62); + let mut buf = BufList::new(); + // HEADERS declaring 63 bytes, over the ceiling. + buf.push(&[1u8, 63][..]); + assert_matches!( + decoder.decode(&mut buf), + Err(FrameStreamError::PayloadTooLarge { .. }) + ); + // What arrives next is the refused payload, not frames: these bytes + // would otherwise parse as an empty SETTINGS frame. + buf.push(&[4u8, 0][..]); + assert_matches!( + decoder.decode(&mut buf), + Err(FrameStreamError::PayloadTooLarge { .. }) + ); + assert!(!buf.has_remaining()); + } + #[test] fn one_frame() { let mut buf = BytesMut::with_capacity(16); @@ -698,13 +728,16 @@ mod tests { let mut recv = FakeRecv::default(); // HEADERS declares 63 bytes and sends no payload. recv.chunk(Bytes::from_static(&[1, 63])); - let mut stream: FrameStream<_, ()> = - FrameStream::new_with_max_buffered_frame(BufRecvStream::new(recv), 62); + let mut stream: FrameStream<_, ()> = FrameStream { + stream: BufRecvStream::new(recv), + decoder: FrameDecoder::new(62), + remaining_data: 0, + }; assert_poll_matches!( |cx| stream.poll_next(cx), Err(FrameStreamError::PayloadTooLarge { - frame_type: 1, + frame_type: FrameType::HEADERS, actual_size: 63, max_size: 62 }) diff --git a/h3/src/proto/frame.rs b/h3/src/proto/frame.rs index 5e51b373..d396067a 100644 --- a/h3/src/proto/frame.rs +++ b/h3/src/proto/frame.rs @@ -1,4 +1,4 @@ -use bytes::{Buf, BufMut, Bytes}; +use bytes::{buf::Take, Buf, BufMut, Bytes}; use std::{ convert::TryInto, fmt::{self, Debug}, @@ -19,7 +19,6 @@ use super::{ pub enum FrameError { Malformed, UnsupportedFrame(u64), // Known frames that should generate an error - UnknownFrame(u64), // Unknown frames that should be ignored InvalidFrameValue, Incomplete(usize), Settings(SettingsError), @@ -31,17 +30,15 @@ pub enum FrameError { /// accumulated — which is the whole point. Every other error here can be /// reached only once the frame is in memory. TooLarge { - ty: u64, + ty: FrameType, len: u64, limit: u64, }, /// An unknown frame type, reported *before* its payload has been read so /// the caller can discard it as it arrives. /// - /// Distinct from [`FrameError::UnknownFrame`], which is returned once the - /// payload is already buffered. RFC 9114 requires unknown frames to be - /// ignored; accumulating one in full in order to ignore it satisfies the - /// letter and not the substance. + /// RFC 9114 requires unknown frames to be ignored; accumulating one in + /// full in order to ignore it satisfies the letter and not the substance. SkipUnknown { ty: u64, len: u64, @@ -55,7 +52,6 @@ impl fmt::Display for FrameError { match self { FrameError::Malformed => write!(f, "frame is malformed"), FrameError::UnsupportedFrame(c) => write!(f, "frame 0x{:x} is not allowed h3", c), - FrameError::UnknownFrame(c) => write!(f, "frame 0x{:x} ignored", c), FrameError::InvalidFrameValue => write!(f, "frame value is invalid"), FrameError::Incomplete(x) => write!(f, "internal error: frame incomplete {}", x), FrameError::Settings(x) => write!(f, "invalid settings: {}", x), @@ -64,7 +60,7 @@ impl fmt::Display for FrameError { FrameError::TooLarge { ty, len, limit } => write!( f, "frame 0x{:x} declared {} bytes, over the {} byte ceiling", - ty, len, limit + ty.0, len, limit ), FrameError::SkipUnknown { ty, len } => { write!(f, "frame 0x{:x} ignored, {} bytes skipped", ty, len) @@ -154,14 +150,25 @@ impl Frame { return Err(FrameError::UnsupportedFrame(ty.0)); } - // Unknown: to be ignored, and ignoring it must not mean storing it. - if !ty.is_buffered() { - return Err(FrameError::SkipUnknown { ty: ty.0, len }); - } + // The parser for this type, chosen before anything is buffered: a type + // without one is unknown, to be ignored, and ignoring it must not mean + // storing it. This table is the one list of what gets buffered. + let parse: fn(&mut Take<&mut T>, usize) -> Result = match ty { + FrameType::HEADERS => |p, len| Ok(Frame::Headers(p.copy_to_bytes(len))), + FrameType::SETTINGS => |p, _| Ok(Frame::Settings(Settings::decode(p)?)), + FrameType::CANCEL_PUSH => |p, _| Ok(Frame::CancelPush(p.get_var()?.try_into()?)), + FrameType::PUSH_PROMISE => |p, _| Ok(Frame::PushPromise(PushPromise::decode(p)?)), + FrameType::GOAWAY => |p, _| Ok(Frame::Goaway(VarInt::decode(p)?)), + FrameType::MAX_PUSH_ID => |p, _| Ok(Frame::MaxPushId(p.get_var()?.try_into()?)), + //= https://www.rfc-editor.org/rfc/rfc9114#section-7.2.8 + //# Endpoints MUST + //# NOT consider these frames to have any meaning upon receipt. + _ => return Err(FrameError::SkipUnknown { ty: ty.0, len }), + }; if len > max_buffered { return Err(FrameError::TooLarge { - ty: ty.0, + ty, len, limit: max_buffered, }); @@ -176,30 +183,7 @@ impl Frame { #[cfg(feature = "tracing")] trace!("frame ty: {:?}", ty); - let frame = match ty { - FrameType::HEADERS => Ok(Frame::Headers(payload.copy_to_bytes(len as usize))), - FrameType::SETTINGS => Ok(Frame::Settings(Settings::decode(&mut payload)?)), - FrameType::CANCEL_PUSH => Ok(Frame::CancelPush(payload.get_var()?.try_into()?)), - FrameType::PUSH_PROMISE => Ok(Frame::PushPromise(PushPromise::decode(&mut payload)?)), - FrameType::GOAWAY => Ok(Frame::Goaway(VarInt::decode(&mut payload)?)), - FrameType::MAX_PUSH_ID => Ok(Frame::MaxPushId(payload.get_var()?.try_into()?)), - //= https://www.rfc-editor.org/rfc/rfc9114#section-7.2.8 - //# These frame - //# types MUST NOT be sent, and their receipt MUST be treated as a - //# connection error of type H3_FRAME_UNEXPECTED. - FrameType::H2_PRIORITY - | FrameType::H2_PING - | FrameType::H2_WINDOW_UPDATE - | FrameType::H2_CONTINUATION => Err(FrameError::UnsupportedFrame(ty.0)), - FrameType::WEBTRANSPORT_BI_STREAM | FrameType::DATA => unreachable!(), - _ => { - buf.advance(len as usize); - //= https://www.rfc-editor.org/rfc/rfc9114#section-7.2.8 - //# Endpoints MUST - //# NOT consider these frames to have any meaning upon receipt. - Err(FrameError::UnknownFrame(ty.0)) - } - }; + let frame = parse(&mut payload, len as usize); if let Ok(_frame) = &frame { #[cfg(feature = "tracing")] @@ -384,29 +368,6 @@ impl FrameType { FrameType(fastrand::u64(0..0x210842108421083) * 0x1f + 0x21) } - /// Whether parsing this type requires its whole payload in memory. - /// - /// The list is exactly the arms of the match in [`Frame::decode`] that read - /// from `payload`. `DATA` is absent because its body is streamed rather - /// than held, and `WEBTRANSPORT_BI_STREAM` because it carries no length at - /// all — both are handled before this is consulted. - /// - /// Kept beside the type list rather than derived from the match so that - /// adding a frame type to one and not the other is a compile-time - /// omission with a visible home, instead of a type that silently becomes - /// unbounded. - fn is_buffered(self) -> bool { - matches!( - self, - FrameType::HEADERS - | FrameType::SETTINGS - | FrameType::CANCEL_PUSH - | FrameType::PUSH_PROMISE - | FrameType::GOAWAY - | FrameType::MAX_PUSH_ID - ) - } - /// The HTTP/2 types RFC 9114 §7.2.8 requires be treated as a connection /// error of type `H3_FRAME_UNEXPECTED`. /// @@ -751,7 +712,7 @@ mod tests { assert_matches!( Frame::decode(&mut buf, 62), Err(FrameError::TooLarge { - ty: 1, + ty: FrameType::HEADERS, len: 63, limit: 62 }) diff --git a/h3/src/server/builder.rs b/h3/src/server/builder.rs index 08be4f47..6e043e57 100644 --- a/h3/src/server/builder.rs +++ b/h3/src/server/builder.rs @@ -68,7 +68,8 @@ impl Builder { /// Set the maximum header size this client is willing to accept /// - /// See [header size constraints] section of the specification for details. + /// Defaults to 64 KiB. Measured as decoded name + value + 32 bytes per field, + /// see [header size constraints] section of the specification for details. /// /// [header size constraints]: https://www.rfc-editor.org/rfc/rfc9114.html#name-header-size-constraints pub fn max_field_section_size(&mut self, value: u64) -> &mut Self { diff --git a/h3/src/server/connection.rs b/h3/src/server/connection.rs index f4dee369..0a5109e3 100644 --- a/h3/src/server/connection.rs +++ b/h3/src/server/connection.rs @@ -97,10 +97,7 @@ where { #[cfg(feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes")] /// Create a [`RequestResolver`] to handle an incoming request. - pub fn create_resolver( - &mut self, - stream: FrameStream, - ) -> RequestResolver { + pub fn create_resolver(&mut self, stream: C::BidiStream) -> RequestResolver { self.create_resolver_internal(stream) } @@ -127,11 +124,7 @@ where pub async fn accept(&mut self) -> Result>, ConnectionError> { // Accept the incoming stream let stream = match poll_fn(|cx| self.poll_accept_request_stream_internal(cx)).await? { - Some(s) => FrameStream::new_with_max_buffered_frame( - BufRecvStream::new(s), - self.max_field_section_size - .max(crate::frame::DEFAULT_MAX_BUFFERED_FRAME), - ), + Some(s) => s, None => { // We always send a last GoAway frame to the client, so it knows which was the last // non-rejected request. @@ -145,10 +138,9 @@ where Ok(Some(resolver)) } - fn create_resolver_internal( - &mut self, - stream: FrameStream, - ) -> RequestResolver { + fn create_resolver_internal(&mut self, stream: C::BidiStream) -> RequestResolver { + let stream = + FrameStream::new_for_request(BufRecvStream::new(stream), self.max_field_section_size); let send_grease_frame = self.inner.send_grease_frame; // send the grease frame only once per connection self.inner.send_grease_frame = false; diff --git a/h3/src/server/request.rs b/h3/src/server/request.rs index bf291aa5..ad626201 100644 --- a/h3/src/server/request.rs +++ b/h3/src/server/request.rs @@ -1,5 +1,7 @@ use std::{convert::TryFrom, sync::Arc}; +use crate::proto::frame::FrameType; + use bytes::Buf; use http::{Request, StatusCode}; @@ -88,8 +90,8 @@ where mut self, frame: Result>, FrameStreamError>, ) -> Result, StreamError> { - let mut encoded = match frame { - Ok(Some(Frame::Headers(h))) => h, + let encoded = match frame { + Ok(Some(Frame::Headers(h))) => Ok(h), //= https://www.rfc-editor.org/rfc/rfc9114#section-4.1 //# If a client-initiated @@ -124,34 +126,38 @@ where )), ); } - Err(e) => { - if matches!( - e, - FrameStreamError::PayloadTooLarge { - frame_type: 0x1, - .. - } - ) { - self.frame_stream.stop_sending(Code::H3_REQUEST_CANCELLED); - self.frame_stream.reset(Code::H3_REQUEST_CANCELLED.value()); - } - return Err(self.handle_frame_stream_error_on_request_stream(e)); + // Too large to even buffer: refused the same way a decoded section + // over the limit is, with a 431, so the client sees one behaviour. + // The decoder keeps refusing, so there is nothing more to receive. + Err(FrameStreamError::PayloadTooLarge { + frame_type: FrameType::HEADERS, + actual_size, + .. + }) => { + self.frame_stream.stop_sending(Code::H3_REQUEST_CANCELLED); + Err(actual_size) } + Err(e) => return Err(self.handle_frame_stream_error_on_request_stream(e)), }; - let decoded = match qpack::decode_stateless(&mut encoded, self.max_field_section_size) { - //= https://www.rfc-editor.org/rfc/rfc9114#section-4.2.2 - //# An HTTP/3 implementation MAY impose a limit on the maximum size of - //# the message header it will accept on an individual HTTP message. - Err(qpack::DecoderError::HeaderTooLong(cancel_size)) => Err(cancel_size), - Ok(decoded) => Ok(decoded), - Err(_e) => { - return Err( - self.handle_connection_error_on_stream(InternalConnectionError { - code: Code::QPACK_DECOMPRESSION_FAILED, - message: "Failed to decode headers".to_string(), - }), - ); + let decoded = match encoded { + Err(size) => Err(size), + Ok(mut encoded) => { + match qpack::decode_stateless(&mut encoded, self.max_field_section_size) { + //= https://www.rfc-editor.org/rfc/rfc9114#section-4.2.2 + //# An HTTP/3 implementation MAY impose a limit on the maximum size of + //# the message header it will accept on an individual HTTP message. + Err(qpack::DecoderError::HeaderTooLong(cancel_size)) => Err(cancel_size), + Ok(decoded) => Ok(decoded), + Err(_e) => { + return Err(self.handle_connection_error_on_stream( + InternalConnectionError { + code: Code::QPACK_DECOMPRESSION_FAILED, + message: "Failed to decode headers".to_string(), + }, + )); + } + } } }; diff --git a/h3/src/tests/request.rs b/h3/src/tests/request.rs index 2eb3d11f..f0a1983c 100644 --- a/h3/src/tests/request.rs +++ b/h3/src/tests/request.rs @@ -350,6 +350,82 @@ async fn header_too_big_by_default() { tokio::join!(server_fut, client_fut); } +#[tokio::test] +async fn header_frame_too_big_to_buffer_is_refused_with_431() { + // A HEADERS frame declaring more than the decoder will buffer is refused + // from its length alone, and answered like any other oversized header: 431. + init_tracing(); + let mut pair = Pair::default(); + let mut server = pair.server(); + + const DECLARED: u64 = 2 * 1024 * 1024; + + let client_fut = async { + let connection = pair.client_inner().await; + let (mut driver, send) = client::new(h3_quinn::Connection::new(connection.clone())) + .await + .unwrap(); + let (mut req_send, mut req_recv) = connection.open_bi().await.unwrap(); + + let client = async move { + let mut buf = BytesMut::new(); + FrameType::HEADERS.encode(&mut buf); + VarInt::from_u64(DECLARED).unwrap().encode(&mut buf); + req_send.write_all(&buf[..]).await.unwrap(); + + let mut response = BytesMut::new(); + while let Some(n) = req_recv.read(&mut buf).await.expect("response read") { + response.put_slice(&buf[..n]); + } + let block = match Frame::decode( + &mut std::io::Cursor::new(&response[..]), + crate::frame::DEFAULT_MAX_BUFFERED_FRAME, + ) { + Ok(Frame::Headers(block)) => block, + other => panic!("expected a HEADERS frame, got {:?}", other), + }; + let fields = qpack::decode_stateless(&mut &block[..], u64::MAX) + .unwrap() + .fields; + let (status, _) = Header::try_from(fields) + .unwrap() + .into_response_parts() + .unwrap(); + assert_eq!(status, StatusCode::REQUEST_HEADER_FIELDS_TOO_LARGE); + drop(send); + }; + let drive_fut = async { future::poll_fn(|cx| driver.poll_close(cx)).await }; + tokio::join!(client, drive_fut); + }; + + let server_fut = async { + let conn = server.next().await; + let mut incoming_req = server::builder().build(conn).await.unwrap(); + + let resolver = incoming_req.accept().await.unwrap().unwrap(); + assert_matches!( + resolver + .resolve_request() + .await + .err() + .expect("should return an error"), + StreamError::HeaderTooBig { + actual_size: DECLARED, + max_size: crate::config::DEFAULT_MAX_FIELD_SECTION_SIZE, + } + ); + + // connection will end without an error + assert_matches!( + incoming_req.accept().await.err().unwrap(), + ConnectionError::Remote(ConnectionErrorIncoming::ApplicationClose{error_code: code, ..}) + if code == Code::H3_NO_ERROR.value() + ); + }; + + tokio::join!(server_fut, client_fut); +} + #[tokio::test] async fn header_too_big_response_from_server() { init_tracing();