From 88dd4f2a6206401af2f4f98c8703947f196ce971 Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Tue, 11 Aug 2026 16:26:29 +0700 Subject: [PATCH] fix(proto): accept upstream midpoint echo replies Upstream irtt 0.9.1 always emits both midpoint timestamp fields (wall, then monotonic) in an ECHO reply for single-clock StampAt::Midpoint negotiations, even though only one clock was negotiated, so its header is one timestamp longer than the negotiated header. The resulting datagram length is not simply negotiated_len + 8. Both forms are max(negotiated_length, header), so the extra field lengthens the datagram only while the larger header still exceeds the negotiated length, and displaces payload beyond that; the difference is 8, then 7..1, then 0. decode_echo_reply now accepts the compatibility length wherever it is strictly longer than the negotiated length, parsing both wire fields and exposing only the negotiated clock's value. Where the two lengths coincide the forms are indistinguishable from the packet alone, so the reply is accepted with normal negotiated parsing rather than a heuristic - a monotonic-only midpoint negotiation therefore cannot always be corrected, which is documented and tested. Every other length stays strictly rejected. Echo request decoding and encode_echo_reply are unchanged: the encoder must keep emitting the sane negotiated layout. --- crates/irtt-proto/src/echo.rs | 125 ++++++-- crates/irtt-proto/tests/directional_codecs.rs | 275 +++++++++++++++++- 2 files changed, 377 insertions(+), 23 deletions(-) diff --git a/crates/irtt-proto/src/echo.rs b/crates/irtt-proto/src/echo.rs index c924551..f6e9a5f 100644 --- a/crates/irtt-proto/src/echo.rs +++ b/crates/irtt-proto/src/echo.rs @@ -2,10 +2,38 @@ use crate::{ envelope::{self, FlagRule}, flags::{FLAG_CLOSE, FLAG_OPEN, FLAG_REPLY}, layout::{echo_packet_len, PacketLayout}, - params::Params, + params::{Clock, Params, StampAt}, ProtoError, Result, RECV_COUNT_SIZE, RECV_WINDOW_SIZE, SEQ_SIZE, TIMESTAMP_SIZE, TOKEN_SIZE, }; +/// Upstream irtt 0.9.1 always emits both midpoint timestamp fields (wall, +/// then monotonic) in an ECHO reply for single-clock `StampAt::Midpoint` +/// negotiations, even though only one clock was negotiated. That makes the +/// compatibility header exactly one timestamp longer than the negotiated +/// header. +const MIDPOINT_COMPAT_EXTRA: usize = TIMESTAMP_SIZE; + +fn is_midpoint_single_clock(params: &Params) -> bool { + params.stamp_at == StampAt::Midpoint && !matches!(params.clock, Clock::Both) +} + +/// Total datagram length of the upstream dual-midpoint compatibility form. +/// +/// Both lengths are `max(negotiated_length, header)`, so this is *not* +/// `normal_len + TIMESTAMP_SIZE`. The extra midpoint field only lengthens the +/// datagram while the larger header is still pushing past the negotiated +/// length; beyond that it displaces payload instead, and the two forms have +/// identical length. The difference is therefore 8, 7..1, or 0 depending on +/// the negotiated length. +/// +/// Deriving this from `normal_len` keeps the identity exact — +/// `max(negotiated_length, compat_header) == max(compat_header, normal_len)` — +/// and cannot overflow, because `compat_header` is a small bounded header +/// size rather than an attacker-influenced packet length. +fn midpoint_compat_packet_len(layout: PacketLayout, normal_len: usize) -> usize { + (layout.header_len() + MIDPOINT_COMPAT_EXTRA).max(normal_len) +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct EchoRequest { pub token: u64, @@ -83,7 +111,7 @@ pub fn decode_echo_request( ], )?; let layout = PacketLayout::echo(hmac_key.is_some(), params); - validate_echo_length(packet, params, layout)?; + validate_echo_length(packet, params, layout, false)?; envelope::verify(packet, hmac_key)?; let mut pos = envelope.body_offset; @@ -128,6 +156,22 @@ pub fn encode_echo_reply( envelope::finish(out, hmac_key) } +/// Decodes an ECHO reply, accepting the upstream irtt 0.9.1 dual-midpoint +/// compatibility form when it is identifiable by datagram length. +/// +/// For single-clock `StampAt::Midpoint` negotiations upstream emits both +/// midpoint fields (wall, then monotonic). When that larger header makes the +/// datagram longer than the negotiated length, the compatibility form is +/// unambiguous and both fields are parsed, exposing only the negotiated +/// clock's value. +/// +/// When the negotiated length already covers the larger header the two forms +/// have identical length and are indistinguishable from the packet alone. The +/// reply is still accepted, parsed against the negotiated layout. For a +/// monotonic-only negotiation that means the value exposed as `midpoint_mono` +/// is whatever occupies the negotiated field position, which for an upstream +/// peer is its wall timestamp — this cannot be corrected deterministically +/// without out-of-band knowledge of the peer, so no heuristic is applied. pub fn decode_echo_reply( packet: &[u8], params: &Params, @@ -139,23 +183,40 @@ pub fn decode_echo_reply( &[FlagRule::Reject(FLAG_OPEN), FlagRule::Require(FLAG_REPLY)], )?; let layout = PacketLayout::echo(hmac_key.is_some(), params); - validate_echo_length(packet, params, layout)?; + let midpoint_compat = validate_echo_length(packet, params, layout, true)?; envelope::verify(packet, hmac_key)?; - let header_len = layout.header_len(); + let header_len = layout.header_len() + + if midpoint_compat { + MIDPOINT_COMPAT_EXTRA + } else { + 0 + }; let mut pos = envelope.body_offset; let token = read_u64(packet, &mut pos); let sequence = read_u32(packet, &mut pos); let recv_count = layout.recv_count.then(|| read_u32(packet, &mut pos)); let recv_window = layout.recv_window.then(|| read_u64(packet, &mut pos)); - let timestamps = TimestampFields { - recv_wall: layout.recv_wall.then(|| read_i64(packet, &mut pos)), - recv_mono: layout.recv_mono.then(|| read_i64(packet, &mut pos)), - midpoint_wall: layout.midpoint_wall.then(|| read_i64(packet, &mut pos)), - midpoint_mono: layout.midpoint_mono.then(|| read_i64(packet, &mut pos)), - send_wall: layout.send_wall.then(|| read_i64(packet, &mut pos)), - send_mono: layout.send_mono.then(|| read_i64(packet, &mut pos)), + let recv_wall = layout.recv_wall.then(|| read_i64(packet, &mut pos)); + let recv_mono = layout.recv_mono.then(|| read_i64(packet, &mut pos)); + let (midpoint_wall, midpoint_mono) = if midpoint_compat { + let wall = read_i64(packet, &mut pos); + let mono = read_i64(packet, &mut pos); + match params.clock { + Clock::Wall => (Some(wall), None), + Clock::Monotonic => (None, Some(mono)), + Clock::Both => { + unreachable!("midpoint compat only applies to single-clock negotiations") + } + } + } else { + ( + layout.midpoint_wall.then(|| read_i64(packet, &mut pos)), + layout.midpoint_mono.then(|| read_i64(packet, &mut pos)), + ) }; + let send_wall = layout.send_wall.then(|| read_i64(packet, &mut pos)); + let send_mono = layout.send_mono.then(|| read_i64(packet, &mut pos)); Ok(EchoReply { flags: envelope.flags, @@ -163,12 +224,32 @@ pub fn decode_echo_reply( sequence, recv_count, recv_window, - timestamps, + timestamps: TimestampFields { + recv_wall, + recv_mono, + midpoint_wall, + midpoint_mono, + send_wall, + send_mono, + }, payload: packet[header_len..].to_vec(), }) } -fn validate_echo_length(packet: &[u8], params: &Params, layout: PacketLayout) -> Result<()> { +/// Validates the packet length against the negotiated layout. Returns +/// `Ok(true)` when the packet matched the upstream midpoint compatibility +/// length rather than the exact negotiated length (only possible when +/// `allow_midpoint_compat` is set). +/// +/// The negotiated length is always checked first, so when the compatibility +/// form is not longer than the negotiated form the two are indistinguishable +/// by length and normal negotiated parsing wins. See [`decode_echo_reply`]. +fn validate_echo_length( + packet: &[u8], + params: &Params, + layout: PacketLayout, + allow_midpoint_compat: bool, +) -> Result { let header_len = layout.header_len(); if packet.len() < header_len { return Err(ProtoError::PacketTooShort { @@ -177,13 +258,19 @@ fn validate_echo_length(packet: &[u8], params: &Params, layout: PacketLayout) -> }); } let expected_len = echo_packet_len(layout.hmac, params)?; - if packet.len() != expected_len { - return Err(ProtoError::PacketLengthMismatch { - expected: expected_len, - actual: packet.len(), - }); + if packet.len() == expected_len { + return Ok(false); } - Ok(()) + if allow_midpoint_compat && is_midpoint_single_clock(params) { + let compat_len = midpoint_compat_packet_len(layout, expected_len); + if compat_len > expected_len && packet.len() == compat_len { + return Ok(true); + } + } + Err(ProtoError::PacketLengthMismatch { + expected: expected_len, + actual: packet.len(), + }) } fn push_zeroed_layout_tail(layout: PacketLayout, out: &mut Vec) { diff --git a/crates/irtt-proto/tests/directional_codecs.rs b/crates/irtt-proto/tests/directional_codecs.rs index c8828e6..58587f7 100644 --- a/crates/irtt-proto/tests/directional_codecs.rs +++ b/crates/irtt-proto/tests/directional_codecs.rs @@ -1,11 +1,16 @@ use irtt_proto::{ decode_close_request, decode_echo_reply, decode_echo_request, decode_open_reply, - decode_open_request, encode_close_request, encode_echo_reply, encode_echo_request, - encode_open_reply, encode_open_request, Clock, CloseRequest, EchoReply, EchoRequest, OpenReply, - OpenRequest, PacketLayout, Params, ProtoError, ReceivedStats, StampAt, TimestampFields, - FLAG_CLOSE, FLAG_HMAC, FLAG_OPEN, FLAG_REPLY, MAGIC, + decode_open_request, echo_packet_len, encode_close_request, encode_echo_reply, + encode_echo_request, encode_open_reply, encode_open_request, hmac::compute_hmac_in_place, + Clock, CloseRequest, EchoReply, EchoRequest, OpenReply, OpenRequest, PacketLayout, Params, + ProtoError, ReceivedStats, StampAt, TimestampFields, FLAG_CLOSE, FLAG_HMAC, FLAG_OPEN, + FLAG_REPLY, MAGIC, }; +/// Wire offset of the HMAC digest within an ECHO reply: the fixed 4-byte +/// header (magic + flags) always precedes it. +const HMAC_OFFSET: usize = 4; + const KEY: &[u8] = b"testkey"; const TOKEN: u64 = 0x7896_b6ab_8771_5213; @@ -359,3 +364,265 @@ fn echo_payload_is_copied_zero_filled_and_bounded_in_both_directions() { }) ); } + +// ---------- Upstream midpoint ECHO-reply compatibility ---------- +// +// Verified upstream irtt 0.9.1 behavior: for single-clock +// `StampAt::Midpoint` negotiations (`Clock::Wall` or `Clock::Monotonic`) +// upstream always emits BOTH midpoint timestamp fields (wall, then +// monotonic), so its header is one timestamp longer than the negotiated +// header. +// +// Crucially the datagram length is NOT `normal_len + 8`. Both forms are +// `max(negotiated_length, header)`, so the extra field lengthens the +// datagram only while the larger header still exceeds the negotiated +// length, and displaces payload beyond that: +// +// compat_len - normal_len == 8, then 7..1, then 0 +// +// `decode_echo_reply` must accept the longer form wherever it is +// identifiable by length, keep rejecting unrelated lengths, and fall back +// to plain negotiated parsing once the two forms are the same length. +// +// `encode_echo_reply` must never produce the dual-field shape, so these +// tests hand-build the wire form directly and narrowly. + +/// Midpoint params whose negotiated length sits `payload_space` bytes past +/// the normal (single-midpoint) header. +fn midpoint_params(clock: Clock, payload_space: usize) -> Params { + let base = Params { + protocol_version: 1, + received_stats: ReceivedStats::Count, + stamp_at: StampAt::Midpoint, + clock, + ..Params::default() + }; + let header_len = PacketLayout::echo(false, &base).header_len(); + Params { + length: (header_len + payload_space) as i64, + ..base + } +} + +fn midpoint_compat_len(params: &Params, hmac: bool) -> usize { + let compat_header = PacketLayout::echo(hmac, params).header_len() + 8; + compat_header.max(echo_packet_len(hmac, params).unwrap()) +} + +/// Hand-builds upstream's dual-midpoint ECHO reply: both midpoint fields on +/// the wire, total length `max(negotiated_length, normal_header + 8)`. +fn build_midpoint_dual_field_reply( + params: &Params, + flags: u8, + seq: u32, + midpoint_wire_fields: (i64, i64), + hmac_key: Option<&[u8]>, +) -> Vec { + let (wall, mono) = midpoint_wire_fields; + let hmac = hmac_key.is_some(); + let layout = PacketLayout::echo(hmac, params); + let compat_header_len = layout.header_len() + 8; + let compat_len = midpoint_compat_len(params, hmac); + + let mut packet = MAGIC.to_vec(); + packet.push(flags); + if hmac { + packet.extend_from_slice(&[0u8; 16]); + } + packet.extend_from_slice(&TOKEN.to_le_bytes()); + packet.extend_from_slice(&seq.to_le_bytes()); + if layout.recv_count { + packet.extend_from_slice(&7_u32.to_le_bytes()); + } + if layout.recv_window { + packet.extend_from_slice(&11_u64.to_le_bytes()); + } + packet.extend_from_slice(&wall.to_le_bytes()); + packet.extend_from_slice(&mono.to_le_bytes()); + assert_eq!(packet.len(), compat_header_len); + + packet.resize(compat_len, 0); + for (index, byte) in packet[compat_header_len..].iter_mut().enumerate() { + *byte = (index as u8).wrapping_add(1); + } + + if let Some(key) = hmac_key { + compute_hmac_in_place(key, &mut packet, HMAC_OFFSET).unwrap(); + } + packet +} + +#[test] +fn midpoint_dual_field_reply_is_accepted_wherever_length_identifies_it() { + // payload_space -> how much longer the compatibility datagram is. + // Covers the full +8 regime and the intermediate +7..+1 regimes. + for (payload_space, expected_extra) in [(0, 8), (1, 7), (4, 4), (7, 1)] { + for clock in [Clock::Wall, Clock::Monotonic] { + let params = midpoint_params(clock, payload_space); + let normal_len = echo_packet_len(false, ¶ms).unwrap(); + let packet = build_midpoint_dual_field_reply(¶ms, FLAG_REPLY, 3, (111, 222), None); + assert_eq!( + packet.len(), + normal_len + expected_extra, + "payload_space {payload_space} clock {clock:?}" + ); + + let reply = decode_echo_reply(&packet, ¶ms, None).unwrap(); + let (expected_wall, expected_mono) = match clock { + Clock::Wall => (Some(111), None), + // The first wire field must not be mistaken for the + // negotiated monotonic value. + Clock::Monotonic => (None, Some(222)), + Clock::Both => unreachable!(), + }; + assert_eq!( + reply.timestamps.midpoint_wall, expected_wall, + "payload_space {payload_space} clock {clock:?}" + ); + assert_eq!( + reply.timestamps.midpoint_mono, expected_mono, + "payload_space {payload_space} clock {clock:?}" + ); + // Payload begins after BOTH compatibility timestamp fields. + let compat_header_len = PacketLayout::echo(false, ¶ms).header_len() + 8; + assert_eq!(reply.payload, packet[compat_header_len..].to_vec()); + } + } +} + +#[test] +fn midpoint_dual_field_reply_of_equal_length_is_accepted_but_not_correctable() { + // Once the negotiated length covers the larger header, the conforming + // and upstream forms have identical length. The reply must still be + // accepted, parsed against the negotiated layout. + for payload_space in [8, 64] { + for clock in [Clock::Wall, Clock::Monotonic] { + let params = midpoint_params(clock, payload_space); + let normal_len = echo_packet_len(false, ¶ms).unwrap(); + let packet = build_midpoint_dual_field_reply(¶ms, FLAG_REPLY, 4, (111, 222), None); + assert_eq!( + packet.len(), + normal_len, + "compat and negotiated lengths coincide for payload_space {payload_space}" + ); + + let reply = decode_echo_reply(&packet, ¶ms, None).unwrap(); + match clock { + // Wall-only happens to line up: the negotiated field + // position holds upstream's wall timestamp. + Clock::Wall => { + assert_eq!(reply.timestamps.midpoint_wall, Some(111)); + assert_eq!(reply.timestamps.midpoint_mono, None); + } + // Monotonic-only cannot be corrected deterministically from + // the packet alone: the negotiated field position holds + // upstream's WALL timestamp, and nothing in the datagram + // distinguishes that from a conforming reply. Documented + // limitation - we accept rather than guess. + Clock::Monotonic => { + assert_eq!(reply.timestamps.midpoint_wall, None); + assert_eq!(reply.timestamps.midpoint_mono, Some(111)); + } + Clock::Both => unreachable!(), + } + } + } +} + +#[test] +fn midpoint_dual_field_reply_authenticates_over_full_packet() { + let params = midpoint_params(Clock::Wall, 0); + let packet = + build_midpoint_dual_field_reply(¶ms, FLAG_REPLY | FLAG_HMAC, 9, (555, 666), Some(KEY)); + assert_eq!(packet.len(), midpoint_compat_len(¶ms, true)); + + let reply = decode_echo_reply(&packet, ¶ms, Some(KEY)).unwrap(); + assert_eq!(reply.timestamps.midpoint_wall, Some(555)); + + let mut corrupted = packet; + corrupted[HMAC_OFFSET] ^= 0xFF; + assert_eq!( + decode_echo_reply(&corrupted, ¶ms, Some(KEY)), + Err(ProtoError::BadHmac) + ); +} + +#[test] +fn only_the_exact_compat_length_is_accepted_as_an_extension() { + let params = midpoint_params(Clock::Wall, 0); + let packet = build_midpoint_dual_field_reply(¶ms, FLAG_REPLY, 1, (10, 20), None); + let expected_len = echo_packet_len(false, ¶ms).unwrap(); + assert_eq!(packet.len(), expected_len + 8); + + // Every other length around the compatibility size stays rejected, + // including the intermediate +1..+7 sizes that are only valid under a + // different negotiated length. + for delta in [-7_i64, -1, 1, 92] { + let target_len = (packet.len() as i64 + delta) as usize; + let mut malformed = packet.clone(); + malformed.resize(target_len, 0); + assert_eq!( + decode_echo_reply(&malformed, ¶ms, None), + Err(ProtoError::PacketLengthMismatch { + expected: expected_len, + actual: target_len, + }), + "delta {delta} (target length {target_len}) must still be rejected" + ); + } +} + +#[test] +fn midpoint_both_clock_reply_has_no_additional_extension() { + let params = midpoint_params(Clock::Both, 0); + let reply = echo_reply(¶ms, FLAG_REPLY); + let packet = encode_echo_reply(&reply, ¶ms, None).unwrap(); + + let mut too_long = packet.clone(); + too_long.extend_from_slice(&[0; 8]); + assert_eq!( + decode_echo_reply(&too_long, ¶ms, None), + Err(ProtoError::PacketLengthMismatch { + expected: packet.len(), + actual: too_long.len(), + }) + ); +} + +#[test] +fn non_midpoint_stamp_at_has_no_longer_form_exception() { + let params = Params { + received_stats: ReceivedStats::None, + stamp_at: StampAt::Receive, + clock: Clock::Wall, + ..Params::default() + }; + let reply = echo_reply(¶ms, FLAG_REPLY); + let packet = encode_echo_reply(&reply, ¶ms, None).unwrap(); + + let mut too_long = packet.clone(); + too_long.extend_from_slice(&[0; 8]); + assert_eq!( + decode_echo_reply(&too_long, ¶ms, None), + Err(ProtoError::PacketLengthMismatch { + expected: packet.len(), + actual: too_long.len(), + }) + ); +} + +#[test] +fn echo_request_decoding_rejects_the_compat_length() { + let params = midpoint_params(Clock::Wall, 0); + let packet = encode_echo_request(&echo_request(Vec::new()), ¶ms, None).unwrap(); + + let mut too_long = packet.clone(); + too_long.extend_from_slice(&[0; 8]); + assert_eq!( + decode_echo_request(&too_long, ¶ms, None), + Err(ProtoError::PacketLengthMismatch { + expected: packet.len(), + actual: too_long.len(), + }) + ); +} -- 2.54.0