fix: use FRAME_SIZE_ERROR and FLOW_CONTROL_ERROR for malformed control frames

src/codec/framed_read.rs

@@ -147,6 +147,21 @@ impl FrameDecoder {
 
 /// Decodes a frame.
 ///
+/// The connection error reason for a frame that failed to load.
+///
+/// A frame with the wrong length is a `FRAME_SIZE_ERROR` (RFC 9113, Section 4.2),
+/// and an oversized `SETTINGS_INITIAL_WINDOW_SIZE` is a `FLOW_CONTROL_ERROR`
+/// (Section 6.5.2). Anything else is a `PROTOCOL_ERROR`.
+fn load_error_reason(e: &frame::Error) -> Reason {
+    match e {
+        frame::Error::BadFrameSize
+        | frame::Error::InvalidPayloadLength
+        | frame::Error::InvalidPayloadAckSettings => Reason::FRAME_SIZE_ERROR,
+        frame::Error::InvalidInitialWindowSize => Reason::FLOW_CONTROL_ERROR,
+        _ => Reason::PROTOCOL_ERROR,
+    }
+}
+
 /// This function is intentionally de-generified and outlined because it is very large.
 fn decode_frame(decoder: &mut FrameDecoder, mut bytes: BytesMut) -> Result<Option<Frame>, Error> {
     let span = tracing::trace_span!("FramedRead::decode_frame", offset = bytes.len());
@@ -233,7 +248,7 @@ fn decode_frame(decoder: &mut FrameDecoder, mut bytes: BytesMut) -> Result<Optio
 
             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(load_error_reason(&e))
             })?
             .into()
         }
@@ -242,7 +257,7 @@ fn decode_frame(decoder: &mut FrameDecoder, mut bytes: BytesMut) -> Result<Optio
 
             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(load_error_reason(&e))
             })?
             .into()
         }
@@ -251,7 +266,7 @@ fn decode_frame(decoder: &mut FrameDecoder, mut bytes: BytesMut) -> Result<Optio
 
             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(load_error_reason(&e))
             })?
             .into()
         }
@@ -271,15 +286,15 @@ fn decode_frame(decoder: &mut FrameDecoder, mut bytes: BytesMut) -> Result<Optio
             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(load_error_reason(&e))
             })?
             .into()
         }
         Kind::GoAway => {
             let res = frame::GoAway::load(head, &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(load_error_reason(&e))
             })?
             .into()
         }

src/frame/mod.rs

@@ -141,6 +141,9 @@ pub enum Error {
     /// An invalid setting value was provided
     InvalidSettingValue,
 
+    /// A `SETTINGS_INITIAL_WINDOW_SIZE` above the maximum window size was provided
+    InvalidInitialWindowSize,
+
     /// An invalid window update value
     InvalidWindowUpdateValue,
 

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);
                     }

tests/h2-tests/tests/server.rs

@@ -1994,3 +1994,80 @@ async fn remote_reset_does_not_panic_connection_driver() {
         .expect("server task timed out")
         .expect("server task panicked");
 }
+
+async fn assert_malformed_frame_goes_away(frame: &'static [u8], reason: Reason) {
+    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_bytes(frame).await;
+        client.recv_frame(frames::go_away(0).reason(reason)).await;
+        client.recv_eof().await;
+    };
+
+    let srv = async move {
+        let mut srv = server::handshake(io).await.expect("handshake");
+        let err = poll_fn(move |cx| srv.poll_closed(cx))
+            .await
+            .expect_err("connection should error");
+        assert_eq!(err.reason(), Some(reason));
+    };
+
+    join(client, srv).await;
+}
+
+#[tokio::test]
+async fn settings_ack_with_payload_is_frame_size_error() {
+    assert_malformed_frame_goes_away(
+        &[0, 0, 6, 4, 1, 0, 0, 0, 0, 0, 3, 0, 0, 0, 100],
+        Reason::FRAME_SIZE_ERROR,
+    )
+    .await;
+}
+
+#[tokio::test]
+async fn settings_length_not_multiple_of_6_is_frame_size_error() {
+    assert_malformed_frame_goes_away(
+        &[0, 0, 5, 4, 0, 0, 0, 0, 0, 0, 3, 0, 0, 0],
+        Reason::FRAME_SIZE_ERROR,
+    )
+    .await;
+}
+
+#[tokio::test]
+async fn ping_wrong_length_is_frame_size_error() {
+    assert_malformed_frame_goes_away(
+        &[0, 0, 7, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
+        Reason::FRAME_SIZE_ERROR,
+    )
+    .await;
+}
+
+#[tokio::test]
+async fn window_update_wrong_length_is_frame_size_error() {
+    assert_malformed_frame_goes_away(
+        &[0, 0, 3, 8, 0, 0, 0, 0, 0, 0, 0, 1],
+        Reason::FRAME_SIZE_ERROR,
+    )
+    .await;
+}
+
+#[tokio::test]
+async fn goaway_too_short_is_frame_size_error() {
+    assert_malformed_frame_goes_away(
+        &[0, 0, 4, 7, 0, 0, 0, 0, 0, 0, 0, 0, 0],
+        Reason::FRAME_SIZE_ERROR,
+    )
+    .await;
+}
+
+#[tokio::test]
+async fn initial_window_size_too_large_is_flow_control_error() {
+    assert_malformed_frame_goes_away(
+        &[0, 0, 6, 4, 0, 0, 0, 0, 0, 0, 4, 0x80, 0, 0, 0],
+        Reason::FLOW_CONTROL_ERROR,
+    )
+    .await;
+}