diff --git a/src/codec/framed_read.rs b/src/codec/framed_read.rs index edb8d9548..2b42b2fec 100644 --- a/src/codec/framed_read.rs +++ b/src/codec/framed_read.rs @@ -160,7 +160,7 @@ fn decode_frame( }, Err(e) => { proto_err!(conn: "failed to load frame; err={:?}", e); - return Err(Error::library_go_away(Reason::PROTOCOL_ERROR)); + return Err(Error::library_go_away(e.reason())); } }; @@ -177,7 +177,7 @@ fn decode_frame( }, Err(e) => { proto_err!(conn: "failed HPACK decoding; err={:?}", e); - return Err(Error::library_go_away(Reason::PROTOCOL_ERROR)); + return Err(Error::library_go_away(e.reason())); } } @@ -203,7 +203,7 @@ fn decode_frame( res.map_err(|e| { proto_err!(conn: "failed to load SETTINGS frame; err={:?}", e); - Error::library_go_away(Reason::PROTOCOL_ERROR) + Error::library_go_away(e.reason()) })? .into() } @@ -212,7 +212,7 @@ fn decode_frame( res.map_err(|e| { proto_err!(conn: "failed to load PING frame; err={:?}", e); - Error::library_go_away(Reason::PROTOCOL_ERROR) + Error::library_go_away(e.reason()) })? .into() } @@ -221,7 +221,7 @@ fn decode_frame( res.map_err(|e| { proto_err!(conn: "failed to load WINDOW_UPDATE frame; err={:?}", e); - Error::library_go_away(Reason::PROTOCOL_ERROR) + Error::library_go_away(e.reason()) })? .into() } @@ -232,7 +232,7 @@ fn decode_frame( // TODO: Should this always be connection level? Probably not... res.map_err(|e| { proto_err!(conn: "failed to load DATA frame; err={:?}", e); - Error::library_go_away(Reason::PROTOCOL_ERROR) + Error::library_go_away(e.reason()) })? .into() } @@ -241,7 +241,7 @@ fn decode_frame( let res = frame::Reset::load(head, &bytes[frame::HEADER_LEN..]); res.map_err(|e| { proto_err!(conn: "failed to load RESET frame; err={:?}", e); - Error::library_go_away(Reason::PROTOCOL_ERROR) + Error::library_go_away(e.reason()) })? .into() } @@ -249,7 +249,7 @@ fn decode_frame( let res = frame::GoAway::load(&bytes[frame::HEADER_LEN..]); res.map_err(|e| { proto_err!(conn: "failed to load GO_AWAY frame; err={:?}", e); - Error::library_go_away(Reason::PROTOCOL_ERROR) + Error::library_go_away(e.reason()) })? .into() } @@ -273,7 +273,7 @@ fn decode_frame( } Err(e) => { proto_err!(conn: "failed to load PRIORITY frame; err={:?};", e); - return Err(Error::library_go_away(Reason::PROTOCOL_ERROR)); + return Err(Error::library_go_away(e.reason())); } } } @@ -349,7 +349,7 @@ fn decode_frame( } Err(e) => { proto_err!(conn: "failed HPACK decoding; err={:?}", e); - return Err(Error::library_go_away(Reason::PROTOCOL_ERROR)); + return Err(Error::library_go_away(e.reason())); } } diff --git a/src/frame/mod.rs b/src/frame/mod.rs index 0e8e7035c..100bb0b6a 100644 --- a/src/frame/mod.rs +++ b/src/frame/mod.rs @@ -141,6 +141,9 @@ pub enum Error { /// An invalid setting value was provided InvalidSettingValue, + /// The SETTINGS_INITIAL_WINDOW_SIZE value exceeds the maximum (2^31-1). + InvalidInitialWindowSize, + /// An invalid window update value InvalidWindowUpdateValue, @@ -169,3 +172,22 @@ pub enum Error { /// Failed to perform HPACK decoding Hpack(hpack::DecoderError), } + +impl Error { + /// Returns the appropriate HTTP/2 reason code for this frame error. + pub(crate) fn reason(&self) -> Reason { + match self { + Error::BadFrameSize + | Error::InvalidPayloadLength + | Error::InvalidPayloadAckSettings => Reason::FRAME_SIZE_ERROR, + Error::InvalidInitialWindowSize => Reason::FLOW_CONTROL_ERROR, + Error::Hpack(_) => Reason::COMPRESSION_ERROR, + Error::TooMuchPadding + | Error::InvalidSettingValue + | Error::InvalidWindowUpdateValue + | Error::InvalidStreamId + | Error::MalformedMessage + | Error::InvalidDependencyId => Reason::PROTOCOL_ERROR, + } + } +} diff --git a/src/frame/settings.rs b/src/frame/settings.rs index 484498a9d..900d5f535 100644 --- a/src/frame/settings.rs +++ b/src/frame/settings.rs @@ -174,7 +174,7 @@ impl Settings { } Some(InitialWindowSize(val)) => { if val as usize > MAX_INITIAL_WINDOW_SIZE { - return Err(Error::InvalidSettingValue); + return Err(Error::InvalidInitialWindowSize); } else { settings.initial_window_size = Some(val); } diff --git a/tests/h2-support/src/frames.rs b/tests/h2-support/src/frames.rs index 5454ebd79..6ba652d20 100644 --- a/tests/h2-support/src/frames.rs +++ b/tests/h2-support/src/frames.rs @@ -290,6 +290,10 @@ impl Mock { self.reason(frame::Reason::FRAME_SIZE_ERROR) } + pub fn compression(self) -> Self { + self.reason(frame::Reason::COMPRESSION_ERROR) + } + pub fn calm(self) -> Self { self.reason(frame::Reason::ENHANCE_YOUR_CALM) } diff --git a/tests/h2-tests/tests/server.rs b/tests/h2-tests/tests/server.rs index 2a7a9508c..47abd8e11 100644 --- a/tests/h2-tests/tests/server.rs +++ b/tests/h2-tests/tests/server.rs @@ -1700,3 +1700,120 @@ async fn init_window_size_smaller_than_default_should_use_default_before_ack() { join(client, h2).await; } + +/// Malformed frames with wrong payload lengths must produce +/// GOAWAY(FRAME_SIZE_ERROR). +#[tokio::test] +async fn frame_size_errors_use_correct_error_code() { + h2_support::trace_init!(); + + // Each entry: raw frame bytes with an invalid payload length. + let cases: &[(&str, &[u8])] = &[ + // PING with 4 bytes instead of 8 + ("PING", &[0, 0, 4, 6, 0, 0, 0, 0, 0, 1, 2, 3, 4]), + // WINDOW_UPDATE with 2 bytes instead of 4 + ("WINDOW_UPDATE", &[0, 0, 2, 8, 0, 0, 0, 0, 0, 0, 1]), + // RST_STREAM with 2 bytes instead of 4 on stream 1 + ("RST_STREAM", &[0, 0, 2, 3, 0, 0, 0, 0, 1, 0, 1]), + // SETTINGS with 5 bytes, not a multiple of 6 + ("SETTINGS", &[0, 0, 5, 4, 0, 0, 0, 0, 0, 1, 2, 3, 4, 5]), + // SETTINGS ACK with 1 byte payload instead of 0 + ("SETTINGS_ACK", &[0, 0, 1, 4, 1, 0, 0, 0, 0, 0]), + // PRIORITY with 3 bytes instead of 5 on stream 1 + ("PRIORITY", &[0, 0, 3, 2, 0, 0, 0, 0, 1, 0, 0, 0]), + // GOAWAY with 4 bytes instead of ≥8 + ("GOAWAY", &[0, 0, 4, 7, 0, 0, 0, 0, 0, 0, 0, 0, 0]), + ]; + + for (name, bad_frame) in cases { + let (io, mut client) = mock::new(); + + let client = async move { + let settings = client.assert_server_handshake().await; + assert_default_settings!(settings); + + // Open stream 1 so RST_STREAM/PRIORITY have a valid target. + client + .send_frame( + frames::headers(1) + .request("GET", "https://example.com/") + .eos(), + ) + .await; + client + .recv_frame(frames::headers(1).response(200).eos()) + .await; + client.send_bytes(bad_frame).await; + client.recv_frame(frames::go_away(1).frame_size()).await; + }; + + let srv = async move { + let mut srv = server::handshake(io).await.expect("handshake"); + let (_req, mut respond) = srv.next().await.unwrap().unwrap(); + let rsp = http::Response::builder().status(200).body(()).unwrap(); + respond.send_response(rsp, true).unwrap(); + let err = srv.next().await.unwrap().expect_err(name); + assert!(err.is_go_away()); + assert!(err.is_library()); + assert_eq!(err.reason(), Some(Reason::FRAME_SIZE_ERROR)); + }; + + join(client, srv).await; + } +} + +/// A SETTINGS frame with INITIAL_WINDOW_SIZE exceeding 2^31-1 must +/// produce GOAWAY(FLOW_CONTROL_ERROR). +#[tokio::test] +async fn settings_initial_window_size_overflow_is_flow_control_error() { + h2_support::trace_init!(); + let (io, mut client) = mock::new(); + + let client = async move { + let settings = client.assert_server_handshake().await; + assert_default_settings!(settings); + + client + .send_frame(frames::settings().initial_window_size(0xFFFFFFFF)) + .await; + client.recv_frame(frames::go_away(0).flow_control()).await; + }; + + let srv = async move { + let mut srv = server::handshake(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::FLOW_CONTROL_ERROR)); + }; + + join(client, srv).await; +} + +/// A HEADERS frame with an invalid HPACK block must produce +/// GOAWAY(COMPRESSION_ERROR). +#[tokio::test] +async fn hpack_error_is_compression_error() { + h2_support::trace_init!(); + let (io, mut client) = mock::new(); + + let client = async move { + let settings = client.assert_server_handshake().await; + assert_default_settings!(settings); + + // HEADERS(stream 1, END_HEADERS|END_STREAM) with 0xFE: + // HPACK indexed reference to index 126, which doesn't exist. + client.send_bytes(&[0, 0, 1, 1, 5, 0, 0, 0, 1, 0xFE]).await; + client.recv_frame(frames::go_away(0).compression()).await; + }; + + let srv = async move { + let mut srv = server::handshake(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::COMPRESSION_ERROR)); + }; + + join(client, srv).await; +}