diff --git a/crates/irtt-client/src/client.rs b/crates/irtt-client/src/client.rs index 54bdbc1..b9b2c4f 100644 --- a/crates/irtt-client/src/client.rs +++ b/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, } } diff --git a/crates/irtt-client/src/client/tests/probes_replies.rs b/crates/irtt-client/src/client/tests/probes_replies.rs index bfa7232..c46656b 100644 --- a/crates/irtt-client/src/client/tests/probes_replies.rs +++ b/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, ¶ms, &TimestampFields::default(), None); + packet.truncate(echo_packet_len(false, ¶ms) - 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, ¶ms, &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, ¶ms, 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, ¶ms, &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, ¶ms) + 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, ¶ms, 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, ¶ms, &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, ¶ms) + )); + client.close(ClientTimestamp::now()).unwrap(); + server.join(); +} + #[test] fn recv_once_at_test_helper_provides_deterministic_timestamp() { let params = default_params(); diff --git a/crates/irtt-proto/src/echo.rs b/crates/irtt-proto/src/echo.rs index 5de3165..df2b02a 100644 --- a/crates/irtt-proto/src/echo.rs +++ b/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, ¶ms, 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, ¶ms, 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, ¶ms, 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, ¶ms); + let mut packet = Vec::with_capacity(try_echo_packet_len(true, ¶ms).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, ¶ms, 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, ¶ms, Some(b"testkey")), + Err(ProtoError::PacketLengthMismatch { + expected: 32, + actual: 33, + }) + ); + } } diff --git a/crates/irtt-proto/src/error.rs b/crates/irtt-proto/src/error.rs index 81fdd9e..cd30a0e 100644 --- a/crates/irtt-proto/src/error.rs +++ b/crates/irtt-proto/src/error.rs @@ -6,6 +6,8 @@ pub type Result = std::result::Result; 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}")]