fix(server): require an executable open before acknowledging it #77

Merged
Lochnair merged 4 commits from fix/server-open-executable-params into main 2026-08-12 13:58:26 +00:00
11 changed files with 102 additions and 42 deletions
Showing only changes of commit 3845142d30 - Show all commits

No files matched your search

+2 -2
View File
@@ -778,7 +778,7 @@ fn start_synchronized_peer_close_server(
); );
assert_eq!( assert_eq!(
packet.len(), packet.len(),
echo_packet_len(false, &params), echo_packet_len(false, &params).unwrap(),
"expected a complete echo probe before synchronized peer-close reply" "expected a complete echo probe before synchronized peer-close reply"
); );
let seq = u32::from_le_bytes(packet[12..16].try_into().unwrap()); let seq = u32::from_le_bytes(packet[12..16].try_into().unwrap());
@@ -829,7 +829,7 @@ fn start_gated_peer_close_server(
} }
assert_eq!( assert_eq!(
packet.len(), packet.len(),
echo_packet_len(false, &params), echo_packet_len(false, &params).unwrap(),
"expected a complete echo probe before peer-close reply" "expected a complete echo probe before peer-close reply"
); );
let seq = u32::from_le_bytes(packet[12..16].try_into().unwrap()); let seq = u32::from_le_bytes(packet[12..16].try_into().unwrap());
+7 -2
View File
@@ -246,7 +246,7 @@ impl AsyncClient {
machine, machine,
schedule: None, schedule: None,
remote, remote,
recv_buffer: vec![0_u8; recv_buffer_size(false, None)], recv_buffer: vec![0_u8; recv_buffer_size(false, None)?],
applied_traffic_class: None, applied_traffic_class: None,
prepared_open: Some(prepared_open), prepared_open: Some(prepared_open),
prepared_probe: None, prepared_probe: None,
@@ -820,7 +820,12 @@ impl AsyncClient {
return Err(Box::new(PreparedAsyncOpenFailure { primary, machine })); return Err(Box::new(PreparedAsyncOpenFailure { primary, machine }));
} }
}; };
let recv_buffer_len = recv_buffer_size(self.machine.has_hmac(), Some(negotiated)); let recv_buffer_len = match recv_buffer_size(self.machine.has_hmac(), Some(negotiated)) {
Ok(size) => size,
Err(primary) => {
return Err(Box::new(PreparedAsyncOpenFailure { primary, machine }));
}
};
let negotiated_traffic_class = match u8::try_from(negotiated.params.dscp) { let negotiated_traffic_class = match u8::try_from(negotiated.params.dscp) {
Ok(traffic_class) => traffic_class, Ok(traffic_class) => traffic_class,
Err(_) => { Err(_) => {
+5 -2
View File
@@ -131,7 +131,7 @@ impl Client {
schedule: None, schedule: None,
socket, socket,
remote, remote,
recv_buffer: vec![0_u8; recv_buffer_size(false, None)], recv_buffer: vec![0_u8; recv_buffer_size(false, None)?],
applied_traffic_class: None, applied_traffic_class: None,
#[cfg(test)] #[cfg(test)]
test_hooks: ClientTestHooks::default(), test_hooks: ClientTestHooks::default(),
@@ -530,7 +530,10 @@ impl Client {
Ok(schedule) => schedule, Ok(schedule) => schedule,
Err(primary) => return Err(Box::new(PreparedClientOpenFailure { primary, machine })), Err(primary) => return Err(Box::new(PreparedClientOpenFailure { primary, machine })),
}; };
let recv_buffer_len = recv_buffer_size(self.runtime.has_hmac(), Some(negotiated)); let recv_buffer_len = match recv_buffer_size(self.runtime.has_hmac(), Some(negotiated)) {
Ok(size) => size,
Err(primary) => return Err(Box::new(PreparedClientOpenFailure { primary, machine })),
};
#[cfg(test)] #[cfg(test)]
let recv_buffer_len = self let recv_buffer_len = self
.test_hooks .test_hooks
@@ -1,8 +1,8 @@
use super::*; use super::*;
pub(crate) use irtt_proto::echo_packet_len;
use irtt_proto::{ use irtt_proto::{
encode_echo_reply, encode_open_reply, flags::FLAG_OPEN, flags::FLAG_REPLY, echo_packet_len as checked_echo_packet_len, encode_echo_reply, encode_open_reply,
layout::PacketLayout, Clock, EchoReply, OpenReply, ReceivedStats, StampAt, flags::FLAG_OPEN, flags::FLAG_REPLY, layout::PacketLayout, Clock, EchoReply, OpenReply,
ReceivedStats, StampAt,
}; };
use std::{ use std::{
net::UdpSocket, net::UdpSocket,
@@ -72,6 +72,12 @@ pub(super) fn open_reply(
.unwrap() .unwrap()
} }
/// Test params always name a representable length, so the platform
/// representability check cannot fire here.
pub(crate) fn echo_packet_len(hmac: bool, params: &Params) -> usize {
checked_echo_packet_len(hmac, params).expect("test params must have a representable length")
}
pub(super) fn echo_reply_packet( pub(super) fn echo_reply_packet(
token: u64, token: u64,
seq: u32, seq: u32,
+14 -4
View File
@@ -823,13 +823,23 @@ impl SessionMachine {
} }
} }
pub(crate) fn recv_buffer_size(has_hmac: bool, negotiated: Option<&NegotiatedParams>) -> usize { /// Sizes the receive buffer for the negotiated layout.
match negotiated { ///
Some(negotiated) => echo_packet_len(has_hmac, &negotiated.params) /// Fallible only because a negotiated length wider than `usize` cannot name a
/// buffer at all. Negotiation already rejects a returned length that is
/// negative or larger than this client requested, and the requested length is a
/// `u32`, so the error is unreachable here in practice — it is propagated
/// rather than asserted away so that no 64-bit assumption is baked in.
pub(crate) fn recv_buffer_size(
has_hmac: bool,
negotiated: Option<&NegotiatedParams>,
) -> Result<usize, ClientError> {
Ok(match negotiated {
Some(negotiated) => echo_packet_len(has_hmac, &negotiated.params)?
.saturating_add(1) .saturating_add(1)
.max(MIN_RECV_BUFFER_SIZE), .max(MIN_RECV_BUFFER_SIZE),
None => MIN_RECV_BUFFER_SIZE, None => MIN_RECV_BUFFER_SIZE,
} })
} }
pub(crate) fn params_from_config(config: &ClientConfig) -> Result<Params, ClientError> { pub(crate) fn params_from_config(config: &ClientConfig) -> Result<Params, ClientError> {
+6 -3
View File
@@ -12,13 +12,12 @@ use std::{
use irtt_client::{Client, ClientConfig, ClientEvent, NegotiatedParams, OpenOutcome, SocketConfig}; use irtt_client::{Client, ClientConfig, ClientEvent, NegotiatedParams, OpenOutcome, SocketConfig};
use irtt_proto::{ use irtt_proto::{
compute_hmac_in_place, decode_request, encode_echo_reply, encode_open_reply, flags, compute_hmac_in_place, decode_request, echo_packet_len, encode_echo_reply, encode_open_reply,
verify_packet_hmac, Clock, DecodedRequest, DecodedRequestKind, EchoReply, OpenReply, flags, verify_packet_hmac, Clock, DecodedRequest, DecodedRequestKind, EchoReply, OpenReply,
PacketLayout, Params, ProtoError, ReceivedStats, ServerFill, StampAt, TimestampFields, PacketLayout, Params, ProtoError, ReceivedStats, ServerFill, StampAt, TimestampFields,
HMAC_SIZE, MAGIC, PROTOCOL_VERSION, HMAC_SIZE, MAGIC, PROTOCOL_VERSION,
}; };
pub(super) use irtt_proto::echo_packet_len as test_echo_packet_len;
pub use real_irtt::RealIrtServer; pub use real_irtt::RealIrtServer;
const HMAC_OFFSET: usize = 4; const HMAC_OFFSET: usize = 4;
@@ -534,6 +533,10 @@ fn open_reply(flags: u8, token: u64, params: &Params, hmac_key: Option<&[u8]>) -
.unwrap() .unwrap()
} }
pub(super) fn test_echo_packet_len(hmac: bool, params: &Params) -> usize {
echo_packet_len(hmac, params).expect("test params must have a representable length")
}
fn echo_reply_packet( fn echo_reply_packet(
token: u64, token: u64,
seq: u32, seq: u32,
+3 -3
View File
@@ -66,7 +66,7 @@ pub fn encode_echo_reply(
hmac_key: Option<&[u8]>, hmac_key: Option<&[u8]>,
) -> Result<Vec<u8>> { ) -> Result<Vec<u8>> {
let layout = PacketLayout::echo(hmac_key.is_some(), params); let layout = PacketLayout::echo(hmac_key.is_some(), params);
let len = echo_packet_len(hmac_key.is_some(), params); let len = echo_packet_len(hmac_key.is_some(), params)?;
let payload_offset = layout.header_len(); let payload_offset = layout.header_len();
let available_payload_len = len.saturating_sub(payload_offset); let available_payload_len = len.saturating_sub(payload_offset);
if reply.payload.len() > available_payload_len { if reply.payload.len() > available_payload_len {
@@ -186,7 +186,7 @@ fn validate_echo_length(packet: &[u8], params: &Params, layout: PacketLayout) ->
actual: packet.len(), actual: packet.len(),
}); });
} }
let expected_len = echo_packet_len(layout.hmac, params); let expected_len = echo_packet_len(layout.hmac, params)?;
if packet.len() == expected_len { if packet.len() == expected_len {
return Ok(false); return Ok(false);
} }
@@ -396,7 +396,7 @@ mod tests {
}; };
params.received_stats = ReceivedStats::Both; params.received_stats = ReceivedStats::Both;
let layout = PacketLayout::echo(true, &params); let layout = PacketLayout::echo(true, &params);
let mut packet = Vec::with_capacity(echo_packet_len(true, &params)); let mut packet = Vec::with_capacity(echo_packet_len(true, &params).unwrap());
write_header(&mut packet, FLAG_REPLY | FLAG_HMAC); write_header(&mut packet, FLAG_REPLY | FLAG_HMAC);
packet.extend_from_slice(&[0; HMAC_SIZE]); packet.extend_from_slice(&[0; HMAC_SIZE]);
packet.extend_from_slice(&0x7896_b6ab_8771_5213u64.to_le_bytes()); packet.extend_from_slice(&0x7896_b6ab_8771_5213u64.to_le_bytes());
+2
View File
@@ -30,6 +30,8 @@ pub enum ProtoError {
InvalidUtf8, InvalidUtf8,
#[error("invalid enum value {value} for {name}")] #[error("invalid enum value {value} for {name}")]
InvalidEnum { name: &'static str, value: i64 }, InvalidEnum { name: &'static str, value: i64 },
#[error("packet length {length} cannot be represented as a buffer length")]
PacketLengthUnrepresentable { length: i64 },
#[error("parameter {tag} length is too large: {length} bytes")] #[error("parameter {tag} length is too large: {length} bytes")]
ParameterLengthTooLarge { tag: u64, length: u64 }, ParameterLengthTooLarge { tag: u64, length: u64 },
#[error("open reply has zero token without close flag")] #[error("open reply has zero token without close flag")]
+44 -15
View File
@@ -1,7 +1,7 @@
use crate::{ use crate::{
params::{Params, StampAt}, params::{Params, StampAt},
HEADER_SIZE, HMAC_SIZE, RECV_COUNT_SIZE, RECV_WINDOW_SIZE, SEQ_SIZE, TIMESTAMP_SIZE, ProtoError, Result, HEADER_SIZE, HMAC_SIZE, RECV_COUNT_SIZE, RECV_WINDOW_SIZE, SEQ_SIZE,
TOKEN_SIZE, TIMESTAMP_SIZE, TOKEN_SIZE,
}; };
#[derive(Debug, Clone, Copy, PartialEq, Eq)] #[derive(Debug, Clone, Copy, PartialEq, Eq)]
3
@@ -123,18 +123,30 @@ pub fn echo_header_len(hmac: bool, params: &Params) -> usize {
/// therefore requests no space beyond the mandatory field block, exactly as /// therefore requests no space beyond the mandatory field block, exactly as
/// zero does. /// zero does.
/// ///
/// Only the negative end is bounded. An absurdly large positive length is /// # Errors
/// returned as asked, and encoding a packet that size fails in the allocator ///
/// rather than here — on a 64-bit target every positive `i64` converts, so that /// Returns [`ProtoError::PacketLengthUnrepresentable`] for a positive length
/// has always been true of a length beyond addressable memory. Capping a /// this platform's `usize` cannot hold. That is a representability check, not a
/// negotiated length to something a UDP datagram can actually be belongs with /// size policy: a wire value that cannot even name a local buffer must not be
/// the deferred maximum-packet-length policy, which would apply long before /// converted into one that can, because the result is handed to an allocator.
/// this conversion. /// It is unreachable on a 64-bit target, where every positive `i64` converts,
pub fn echo_packet_len(hmac: bool, params: &Params) -> usize { /// and is the reason this returns a [`Result`] at all.
// Saturating keeps the same answer on a target whose `usize` is narrower ///
// than the length: still unallocatable, just unrepresentable sooner. /// A ceiling on what a negotiated length may legitimately *be* — an MTU, a
let requested = usize::try_from(params.length.max(0)).unwrap_or(usize::MAX); /// resource bound, a maximum packet size — is deliberately not here. That is
echo_header_len(hmac, params).max(requested) /// server and runtime policy, and it belongs where the negotiation happens.
pub fn echo_packet_len(hmac: bool, params: &Params) -> Result<usize> {
let header_len = echo_header_len(hmac, params);
let requested = if params.length <= 0 {
// No datagram is shorter than none, so a negative length asks for
// nothing beyond the mandatory field block, exactly as zero does.
0
} else {
usize::try_from(params.length).map_err(|_| ProtoError::PacketLengthUnrepresentable {
length: params.length,
})?
};
Ok(header_len.max(requested))
} }
#[cfg(test)] #[cfg(test)]
@@ -304,9 +316,26 @@ mod tests {
p.length = length; p.length = length;
assert_eq!( assert_eq!(
echo_packet_len(false, &p), echo_packet_len(false, &p),
expected, Ok(expected),
"unexpected packet length for negotiated length {length}" "unexpected packet length for negotiated length {length}"
); );
} }
} }
/// A positive length wider than `usize` must stay an error rather than
/// becoming a saturated buffer size. Only reachable where `usize` is
/// narrower than `i64`; on a 64-bit target every positive `i64` converts.
#[cfg(target_pointer_width = "32")]
#[test]
fn a_length_wider_than_usize_is_rejected_rather_than_saturated() {
let mut p = params(ReceivedStats::Both, StampAt::Both, Clock::Both);
p.length = 5_000_000_000;
assert_eq!(
echo_packet_len(false, &p),
Err(ProtoError::PacketLengthUnrepresentable {
length: 5_000_000_000
})
);
}
} }
+5 -3
View File
@@ -84,8 +84,10 @@ pub enum DecodedRequestKind<'a> {
/// # Errors /// # Errors
/// ///
/// Returns [`ProtoError::PayloadTooLarge`] when an ECHO payload does not fit /// Returns [`ProtoError::PayloadTooLarge`] when an ECHO payload does not fit
/// the negotiated length. A negative negotiated length is not an error: see /// the negotiated length, and [`ProtoError::PacketLengthUnrepresentable`] when
/// [`echo_packet_len`], which floors it at the mandatory field block. /// the negotiated length is positive and wider than `usize`. A *negative*
/// negotiated length is not an error: see [`echo_packet_len`], which floors it
/// at the mandatory field block.
pub fn encode_request(request: RequestToEncode<'_>, hmac_key: Option<&[u8]>) -> Result<Vec<u8>> { pub fn encode_request(request: RequestToEncode<'_>, hmac_key: Option<&[u8]>) -> Result<Vec<u8>> {
match request { match request {
RequestToEncode::Open { params, no_test } => encode_open(params, no_test, hmac_key), RequestToEncode::Open { params, no_test } => encode_open(params, no_test, hmac_key),
@@ -205,7 +207,7 @@ fn encode_echo(
hmac_key: Option<&[u8]>, hmac_key: Option<&[u8]>,
) -> Result<Vec<u8>> { ) -> Result<Vec<u8>> {
let layout = PacketLayout::echo(hmac_key.is_some(), params); let layout = PacketLayout::echo(hmac_key.is_some(), params);
let len = echo_packet_len(hmac_key.is_some(), params); let len = echo_packet_len(hmac_key.is_some(), params)?;
let payload_offset = layout.header_len(); let payload_offset = layout.header_len();
let available_payload_len = len.saturating_sub(payload_offset); let available_payload_len = len.saturating_sub(payload_offset);
if payload.len() > available_payload_len { if payload.len() > available_payload_len {
@@ -316,7 +316,7 @@ fn a_negative_negotiated_length_still_encodes_in_both_directions() {
length: -4096, length: -4096,
..Params::default() ..Params::default()
}; };
assert_eq!(echo_packet_len(false, &params), 16); assert_eq!(echo_packet_len(false, &params), Ok(16));
let request = encode_request(echo_request(&params, &[]), None) let request = encode_request(echo_request(&params, &[]), None)
.expect("a negative negotiated length must still encode a request"); .expect("a negative negotiated length must still encode a request");
@@ -372,7 +372,7 @@ fn midpoint_params(clock: Clock, payload_space: usize) -> Params {
fn midpoint_compat_len(params: &Params, hmac: bool) -> usize { fn midpoint_compat_len(params: &Params, hmac: bool) -> usize {
let compat_header = PacketLayout::echo(hmac, params).header_len() + 8; let compat_header = PacketLayout::echo(hmac, params).header_len() + 8;
compat_header.max(echo_packet_len(hmac, params)) compat_header.max(echo_packet_len(hmac, params).unwrap())
} }
/// Hand-builds upstream's dual-midpoint ECHO reply: both midpoint fields on /// Hand-builds upstream's dual-midpoint ECHO reply: both midpoint fields on
@@ -425,7 +425,7 @@ fn midpoint_dual_field_reply_is_accepted_wherever_length_identifies_it() {
for (payload_space, expected_extra) in [(0, 8), (1, 7), (4, 4), (7, 1)] { for (payload_space, expected_extra) in [(0, 8), (1, 7), (4, 4), (7, 1)] {
for clock in [Clock::Wall, Clock::Monotonic] { for clock in [Clock::Wall, Clock::Monotonic] {
let params = midpoint_params(clock, payload_space); let params = midpoint_params(clock, payload_space);
let normal_len = echo_packet_len(false, &params); let normal_len = echo_packet_len(false, &params).unwrap();
let packet = build_midpoint_dual_field_reply(&params, FLAG_REPLY, 3, (111, 222), None); let packet = build_midpoint_dual_field_reply(&params, FLAG_REPLY, 3, (111, 222), None);
assert_eq!( assert_eq!(
packet.len(), packet.len(),
@@ -464,7 +464,7 @@ fn midpoint_dual_field_reply_of_equal_length_is_accepted_but_not_correctable() {
for payload_space in [8, 64] { for payload_space in [8, 64] {
for clock in [Clock::Wall, Clock::Monotonic] { for clock in [Clock::Wall, Clock::Monotonic] {
let params = midpoint_params(clock, payload_space); let params = midpoint_params(clock, payload_space);
let normal_len = echo_packet_len(false, &params); let normal_len = echo_packet_len(false, &params).unwrap();
let packet = build_midpoint_dual_field_reply(&params, FLAG_REPLY, 4, (111, 222), None); let packet = build_midpoint_dual_field_reply(&params, FLAG_REPLY, 4, (111, 222), None);
assert_eq!( assert_eq!(
packet.len(), packet.len(),
@@ -517,7 +517,7 @@ fn midpoint_dual_field_reply_authenticates_over_full_packet() {
fn only_the_exact_compat_length_is_accepted_as_an_extension() { fn only_the_exact_compat_length_is_accepted_as_an_extension() {
let params = midpoint_params(Clock::Wall, 0); let params = midpoint_params(Clock::Wall, 0);
let packet = build_midpoint_dual_field_reply(&params, FLAG_REPLY, 1, (10, 20), None); let packet = build_midpoint_dual_field_reply(&params, FLAG_REPLY, 1, (10, 20), None);
let expected_len = echo_packet_len(false, &params); let expected_len = echo_packet_len(false, &params).unwrap();
assert_eq!(packet.len(), expected_len + 8); assert_eq!(packet.len(), expected_len + 8);
// Every other length around the compatibility size stays rejected, // Every other length around the compatibility size stays rejected,