fix(proto): require exact echo reply packet length

crates/irtt-client/src/client.rs

@@ -707,7 +707,9 @@ fn instant_abs_diff(left: Instant, right: Instant) -> Duration {
 
 fn recv_buffer_size(has_hmac: bool, negotiated: Option<&NegotiatedParams>) -> usize {
     match negotiated {
-        Some(negotiated) => echo_packet_len(has_hmac, &negotiated.params).max(MIN_RECV_BUFFER_SIZE),
+        Some(negotiated) => echo_packet_len(has_hmac, &negotiated.params)
+            .saturating_add(1)
+            .max(MIN_RECV_BUFFER_SIZE),
         None => MIN_RECV_BUFFER_SIZE,
     }
 }

crates/irtt-client/src/client/tests/probes_replies.rs

@@ -1897,14 +1897,172 @@ fn recv_buffer_uses_negotiated_packet_length() {
     assert_open_started(client.open(ClientTimestamp::now()).unwrap());
     let buf_size = client.recv_buffer_size();
     assert!(
-        buf_size >= 4096,
-        "recv buffer should be at least negotiated length, got {buf_size}"
+        buf_size > 4096,
+        "recv buffer should include truncation-detection byte, got {buf_size}"
     );
+    assert_eq!(buf_size, 4097);
     assert_eq!(client.recv_buffer.len(), buf_size);
     client.close(ClientTimestamp::now()).unwrap();
     server.join();
 }
 
+#[test]
+fn short_echo_reply_does_not_emit_echo_reply() {
+    let params = Params {
+        length: 64,
+        ..default_params()
+    };
+    let server = silent_open_server(params.clone());
+    let config = ClientConfig {
+        length: 64,
+        socket_config: crate::SocketConfig {
+            recv_timeout: Some(Duration::from_millis(200)),
+            ..Default::default()
+        },
+        ..default_test_config(server.addr)
+    };
+    let mut client = Client::connect(config).unwrap();
+    assert_open_started(client.open(ClientTimestamp::now()).unwrap());
+    client.send_probe().unwrap();
+
+    let mut packet = echo_reply_packet(TOKEN, 0, &params, &TimestampFields::default(), None);
+    packet.truncate(echo_packet_len(false, &params) - 1);
+    let events = client
+        .process_received_packet(&packet, ClientTimestamp::now(), ReceiveMeta::default())
+        .unwrap();
+
+    assert!(matches!(
+        events.as_slice(),
+        [ClientEvent::Warning {
+            kind: WarningKind::MalformedOrUnrelatedPacket,
+            ..
+        }]
+    ));
+    client.close(ClientTimestamp::now()).unwrap();
+    server.join();
+}
+
+#[test]
+fn overlong_echo_reply_does_not_emit_echo_reply() {
+    let params = default_params();
+    let server = silent_open_server(params.clone());
+    let config = ClientConfig {
+        socket_config: crate::SocketConfig {
+            recv_timeout: Some(Duration::from_millis(200)),
+            ..Default::default()
+        },
+        ..default_test_config(server.addr)
+    };
+    let mut client = Client::connect(config).unwrap();
+    assert_open_started(client.open(ClientTimestamp::now()).unwrap());
+    client.send_probe().unwrap();
+
+    let mut packet = echo_reply_packet(TOKEN, 0, &params, &TimestampFields::default(), None);
+    packet.push(0);
+    let events = client
+        .process_received_packet(&packet, ClientTimestamp::now(), ReceiveMeta::default())
+        .unwrap();
+
+    assert!(matches!(
+        events.as_slice(),
+        [ClientEvent::Warning {
+            kind: WarningKind::MalformedOrUnrelatedPacket,
+            ..
+        }]
+    ));
+    client.close(ClientTimestamp::now()).unwrap();
+    server.join();
+}
+
+#[test]
+fn overlong_datagram_detection_uses_extra_receive_byte() {
+    let params = Params {
+        length: 4096,
+        ..default_params()
+    };
+    let server = start_fake_server({
+        let params = params.clone();
+        move |socket, tx| {
+            let (_, peer) = recv_request(&socket, &tx);
+            let reply = open_reply(FLAG_OPEN | FLAG_REPLY, TOKEN, &params, None);
+            socket.send_to(&reply, peer).unwrap();
+            let (request, _) = recv_request(&socket, &tx);
+            let seq = u32::from_le_bytes(request[12..16].try_into().unwrap());
+            let mut reply =
+                echo_reply_packet(TOKEN, seq, &params, &TimestampFields::default(), None);
+            reply.push(0);
+            socket.send_to(&reply, peer).unwrap();
+            let _ = recv_request_timeout(&socket, &tx);
+        }
+    });
+    let config = ClientConfig {
+        length: 4096,
+        socket_config: crate::SocketConfig {
+            recv_timeout: Some(Duration::from_millis(200)),
+            ..Default::default()
+        },
+        ..default_test_config(server.addr)
+    };
+    let mut client = Client::connect(config).unwrap();
+    assert_open_started(client.open(ClientTimestamp::now()).unwrap());
+    assert_eq!(
+        client.recv_buffer.len(),
+        echo_packet_len(false, &params) + 1
+    );
+    client.send_probe().unwrap();
+
+    let events = client.recv_once().unwrap();
+    assert!(matches!(
+        events.as_slice(),
+        [ClientEvent::Warning {
+            kind: WarningKind::MalformedOrUnrelatedPacket,
+            ..
+        }]
+    ));
+    client.close(ClientTimestamp::now()).unwrap();
+    server.join();
+}
+
+#[test]
+fn exact_length_echo_reply_still_emits_echo_reply() {
+    let params = Params {
+        length: 4096,
+        ..default_params()
+    };
+    let server = start_fake_server({
+        let params = params.clone();
+        move |socket, tx| {
+            let (_, peer) = recv_request(&socket, &tx);
+            let reply = open_reply(FLAG_OPEN | FLAG_REPLY, TOKEN, &params, None);
+            socket.send_to(&reply, peer).unwrap();
+            let (request, _) = recv_request(&socket, &tx);
+            let seq = u32::from_le_bytes(request[12..16].try_into().unwrap());
+            let reply = echo_reply_packet(TOKEN, seq, &params, &TimestampFields::default(), None);
+            socket.send_to(&reply, peer).unwrap();
+            let _ = recv_request_timeout(&socket, &tx);
+        }
+    });
+    let config = ClientConfig {
+        length: 4096,
+        socket_config: crate::SocketConfig {
+            recv_timeout: Some(Duration::from_millis(200)),
+            ..Default::default()
+        },
+        ..default_test_config(server.addr)
+    };
+    let mut client = Client::connect(config).unwrap();
+    assert_open_started(client.open(ClientTimestamp::now()).unwrap());
+    client.send_probe().unwrap();
+
+    let events = client.recv_once().unwrap();
+    assert!(matches!(
+        events.as_slice(),
+        [ClientEvent::EchoReply { bytes, .. }] if *bytes == echo_packet_len(false, &params)
+    ));
+    client.close(ClientTimestamp::now()).unwrap();
+    server.join();
+}
+
 #[test]
 fn recv_once_at_test_helper_provides_deterministic_timestamp() {
     let params = default_params();

crates/irtt-proto/src/echo.rs

@@ -78,9 +78,6 @@ pub fn decode_echo_reply(
     reject(flags, FLAG_OPEN)?;
     crate::open::require(flags, FLAG_REPLY)?;
     check_hmac_presence(flags, hmac_key)?;
-    if let Some(key) = hmac_key {
-        hmac::verify_hmac(key, packet, hmac::hmac_offset())?;
-    }
 
     let layout = PacketLayout::echo(has(flags, FLAG_HMAC), params);
     let header_len = layout.header_len();
@@ -90,6 +87,17 @@ pub fn decode_echo_reply(
             actual: packet.len(),
         });
     }
+    let expected_len = try_echo_packet_len(has(flags, FLAG_HMAC), params)?;
+    if packet.len() != expected_len {
+        return Err(ProtoError::PacketLengthMismatch {
+            expected: expected_len,
+            actual: packet.len(),
+        });
+    }
+
+    if let Some(key) = hmac_key {
+        hmac::verify_hmac(key, packet, hmac::hmac_offset())?;
+    }
 
     let mut pos = HEADER_SIZE;
     if layout.hmac {
@@ -286,6 +294,54 @@ mod tests {
         assert!(reply.timestamps.midpoint_wall.is_none());
     }
 
+    #[test]
+    fn echo_reply_decodes_exact_negotiated_length_with_payload() {
+        let params = params_with_payload_space(4);
+        let mut packet = Vec::new();
+        write_header(&mut packet, FLAG_REPLY);
+        packet.extend_from_slice(&0x7896_b6ab_8771_5213u64.to_le_bytes());
+        packet.extend_from_slice(&2_u32.to_le_bytes());
+        packet.extend_from_slice(&[1, 2, 3, 4]);
+
+        let reply = decode_echo_reply(&packet, &params, None).unwrap();
+        assert_eq!(reply.payload, vec![1, 2, 3, 4]);
+    }
+
+    #[test]
+    fn echo_reply_rejects_shorter_than_negotiated_length() {
+        let params = params_with_payload_space(4);
+        let mut packet = Vec::new();
+        write_header(&mut packet, FLAG_REPLY);
+        packet.extend_from_slice(&0x7896_b6ab_8771_5213u64.to_le_bytes());
+        packet.extend_from_slice(&2_u32.to_le_bytes());
+
+        assert_eq!(
+            decode_echo_reply(&packet, &params, None),
+            Err(ProtoError::PacketLengthMismatch {
+                expected: 20,
+                actual: 16,
+            })
+        );
+    }
+
+    #[test]
+    fn echo_reply_rejects_longer_than_negotiated_length() {
+        let params = Params::default();
+        let mut packet = Vec::new();
+        write_header(&mut packet, FLAG_REPLY);
+        packet.extend_from_slice(&0x7896_b6ab_8771_5213u64.to_le_bytes());
+        packet.extend_from_slice(&2_u32.to_le_bytes());
+        packet.push(0);
+
+        assert_eq!(
+            decode_echo_reply(&packet, &params, None),
+            Err(ProtoError::PacketLengthMismatch {
+                expected: 16,
+                actual: 17,
+            })
+        );
+    }
+
     #[test]
     fn hmac_echo_reply_decodes_default_fields_after_hmac() {
         let params = default_params();
@@ -306,4 +362,45 @@ mod tests {
         assert_eq!(reply.recv_window, Some(0));
         assert_eq!(reply.payload.len(), 0);
     }
+
+    #[test]
+    fn hmac_echo_reply_decodes_exact_negotiated_length_with_payload() {
+        let mut params = Params {
+            length: 48,
+            ..Params::default()
+        };
+        params.received_stats = ReceivedStats::Both;
+        let layout = PacketLayout::echo(true, &params);
+        let mut packet = Vec::with_capacity(try_echo_packet_len(true, &params).unwrap());
+        write_header(&mut packet, FLAG_REPLY | FLAG_HMAC);
+        packet.extend_from_slice(&[0; HMAC_SIZE]);
+        packet.extend_from_slice(&0x7896_b6ab_8771_5213u64.to_le_bytes());
+        packet.extend_from_slice(&2_u32.to_le_bytes());
+        push_zeroed_layout_tail(layout, &mut packet);
+        packet.extend_from_slice(&[1, 2, 3, 4]);
+        hmac::compute_hmac_in_place(b"testkey", &mut packet, hmac::hmac_offset()).unwrap();
+
+        let reply = decode_echo_reply(&packet, &params, Some(b"testkey")).unwrap();
+        assert_eq!(reply.payload, vec![1, 2, 3, 4]);
+    }
+
+    #[test]
+    fn hmac_echo_reply_rejects_length_mismatch_before_hmac_verification() {
+        let params = Params::default();
+        let mut packet = Vec::new();
+        write_header(&mut packet, FLAG_REPLY | FLAG_HMAC);
+        packet.extend_from_slice(&[0; HMAC_SIZE]);
+        packet.extend_from_slice(&0x7896_b6ab_8771_5213u64.to_le_bytes());
+        packet.extend_from_slice(&2_u32.to_le_bytes());
+        hmac::compute_hmac_in_place(b"testkey", &mut packet, hmac::hmac_offset()).unwrap();
+        packet.push(0);
+
+        assert_eq!(
+            decode_echo_reply(&packet, &params, Some(b"testkey")),
+            Err(ProtoError::PacketLengthMismatch {
+                expected: 32,
+                actual: 33,
+            })
+        );
+    }
 }

crates/irtt-proto/src/error.rs

@@ -6,6 +6,8 @@ pub type Result<T> = std::result::Result<T, ProtoError>;
 pub enum ProtoError {
     #[error("packet is too short: needed {needed} bytes, got {actual}")]
     PacketTooShort { needed: usize, actual: usize },
+    #[error("bad packet length: expected {expected} bytes, got {actual}")]
+    PacketLengthMismatch { expected: usize, actual: usize },
     #[error("bad magic bytes")]
     BadMagic,
     #[error("reserved flag bits are set: 0x{0:02x}")]