fix(server): require an executable open before acknowledging it #77
No files matched your search
@@ -778,7 +778,7 @@ fn start_synchronized_peer_close_server(
|
||||
);
|
||||
assert_eq!(
|
||||
packet.len(),
|
||||
echo_packet_len(false, ¶ms).unwrap(),
|
||||
echo_packet_len(false, ¶ms),
|
||||
"expected a complete echo probe before synchronized peer-close reply"
|
||||
);
|
||||
let seq = u32::from_le_bytes(packet[12..16].try_into().unwrap());
|
||||
@@ -829,7 +829,7 @@ fn start_gated_peer_close_server(
|
||||
}
|
||||
assert_eq!(
|
||||
packet.len(),
|
||||
echo_packet_len(false, ¶ms).unwrap(),
|
||||
echo_packet_len(false, ¶ms),
|
||||
"expected a complete echo probe before peer-close reply"
|
||||
);
|
||||
let seq = u32::from_le_bytes(packet[12..16].try_into().unwrap());
|
||||
|
||||
@@ -246,7 +246,7 @@ impl AsyncClient {
|
||||
machine,
|
||||
schedule: None,
|
||||
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,
|
||||
prepared_open: Some(prepared_open),
|
||||
prepared_probe: None,
|
||||
@@ -820,12 +820,7 @@ impl AsyncClient {
|
||||
return Err(Box::new(PreparedAsyncOpenFailure { primary, machine }));
|
||||
}
|
||||
};
|
||||
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 recv_buffer_len = recv_buffer_size(self.machine.has_hmac(), Some(negotiated));
|
||||
let negotiated_traffic_class = match u8::try_from(negotiated.params.dscp) {
|
||||
Ok(traffic_class) => traffic_class,
|
||||
Err(_) => {
|
||||
|
||||
@@ -131,7 +131,7 @@ impl Client {
|
||||
schedule: None,
|
||||
socket,
|
||||
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,
|
||||
#[cfg(test)]
|
||||
test_hooks: ClientTestHooks::default(),
|
||||
@@ -530,10 +530,7 @@ impl Client {
|
||||
Ok(schedule) => schedule,
|
||||
Err(primary) => return Err(Box::new(PreparedClientOpenFailure { primary, machine })),
|
||||
};
|
||||
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 })),
|
||||
};
|
||||
let recv_buffer_len = recv_buffer_size(self.runtime.has_hmac(), Some(negotiated));
|
||||
#[cfg(test)]
|
||||
let recv_buffer_len = self
|
||||
.test_hooks
|
||||
|
||||
@@ -1,8 +1,8 @@
|
||||
use super::*;
|
||||
pub(crate) use irtt_proto::echo_packet_len;
|
||||
use irtt_proto::{
|
||||
echo_packet_len as checked_echo_packet_len, encode_echo_reply, encode_open_reply,
|
||||
flags::FLAG_OPEN, flags::FLAG_REPLY, layout::PacketLayout, Clock, EchoReply, OpenReply,
|
||||
ReceivedStats, StampAt,
|
||||
encode_echo_reply, encode_open_reply, flags::FLAG_OPEN, flags::FLAG_REPLY,
|
||||
layout::PacketLayout, Clock, EchoReply, OpenReply, ReceivedStats, StampAt,
|
||||
};
|
||||
use std::{
|
||||
net::UdpSocket,
|
||||
@@ -72,11 +72,6 @@ pub(super) fn open_reply(
|
||||
.unwrap()
|
||||
}
|
||||
|
||||
pub(crate) fn echo_packet_len(hmac: bool, params: &Params) -> usize {
|
||||
checked_echo_packet_len(hmac, params)
|
||||
.expect("test params must have a non-negative packet length")
|
||||
}
|
||||
|
||||
pub(super) fn echo_reply_packet(
|
||||
token: u64,
|
||||
seq: u32,
|
||||
|
||||
@@ -235,7 +235,6 @@ impl SessionMachine {
|
||||
| irtt_proto::ProtoError::VarintOverflow
|
||||
| irtt_proto::ProtoError::InvalidUtf8
|
||||
| irtt_proto::ProtoError::InvalidEnum { .. }
|
||||
| irtt_proto::ProtoError::NegativePacketLength { .. }
|
||||
| irtt_proto::ProtoError::ParameterLengthTooLarge { .. }
|
||||
| irtt_proto::ProtoError::MalformedParams),
|
||||
) => Err(ClientError::Protocol(error)),
|
||||
@@ -824,16 +823,13 @@ impl SessionMachine {
|
||||
}
|
||||
}
|
||||
|
||||
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)?
|
||||
pub(crate) fn recv_buffer_size(has_hmac: bool, negotiated: Option<&NegotiatedParams>) -> usize {
|
||||
match negotiated {
|
||||
Some(negotiated) => echo_packet_len(has_hmac, &negotiated.params)
|
||||
.saturating_add(1)
|
||||
.max(MIN_RECV_BUFFER_SIZE),
|
||||
None => MIN_RECV_BUFFER_SIZE,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn params_from_config(config: &ClientConfig) -> Result<Params, ClientError> {
|
||||
|
||||
@@ -12,12 +12,13 @@ use std::{
|
||||
|
||||
use irtt_client::{Client, ClientConfig, ClientEvent, NegotiatedParams, OpenOutcome, SocketConfig};
|
||||
use irtt_proto::{
|
||||
compute_hmac_in_place, decode_request, echo_packet_len, encode_echo_reply, encode_open_reply,
|
||||
flags, verify_packet_hmac, Clock, DecodedRequest, DecodedRequestKind, EchoReply, OpenReply,
|
||||
compute_hmac_in_place, decode_request, encode_echo_reply, encode_open_reply, flags,
|
||||
verify_packet_hmac, Clock, DecodedRequest, DecodedRequestKind, EchoReply, OpenReply,
|
||||
PacketLayout, Params, ProtoError, ReceivedStats, ServerFill, StampAt, TimestampFields,
|
||||
HMAC_SIZE, MAGIC, PROTOCOL_VERSION,
|
||||
};
|
||||
|
||||
pub(super) use irtt_proto::echo_packet_len as test_echo_packet_len;
|
||||
pub use real_irtt::RealIrtServer;
|
||||
|
||||
const HMAC_OFFSET: usize = 4;
|
||||
@@ -533,11 +534,6 @@ fn open_reply(flags: u8, token: u64, params: &Params, hmac_key: Option<&[u8]>) -
|
||||
.unwrap()
|
||||
}
|
||||
|
||||
pub(super) fn test_echo_packet_len(hmac: bool, params: &Params) -> usize {
|
||||
echo_packet_len(hmac, params)
|
||||
.expect("managed runner test params must have a non-negative packet length")
|
||||
}
|
||||
|
||||
fn echo_reply_packet(
|
||||
token: u64,
|
||||
seq: u32,
|
||||
|
||||
@@ -66,7 +66,7 @@ pub fn encode_echo_reply(
|
||||
hmac_key: Option<&[u8]>,
|
||||
) -> Result<Vec<u8>> {
|
||||
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 available_payload_len = len.saturating_sub(payload_offset);
|
||||
if reply.payload.len() > available_payload_len {
|
||||
@@ -186,7 +186,7 @@ fn validate_echo_length(packet: &[u8], params: &Params, layout: PacketLayout) ->
|
||||
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 {
|
||||
return Ok(false);
|
||||
}
|
||||
@@ -396,7 +396,7 @@ mod tests {
|
||||
};
|
||||
params.received_stats = ReceivedStats::Both;
|
||||
let layout = PacketLayout::echo(true, ¶ms);
|
||||
let mut packet = Vec::with_capacity(echo_packet_len(true, ¶ms).unwrap());
|
||||
let mut packet = Vec::with_capacity(echo_packet_len(true, ¶ms));
|
||||
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());
|
||||
|
||||
@@ -30,8 +30,6 @@ pub enum ProtoError {
|
||||
InvalidUtf8,
|
||||
#[error("invalid enum value {value} for {name}")]
|
||||
InvalidEnum { name: &'static str, value: i64 },
|
||||
#[error("packet length must be non-negative, got {length}")]
|
||||
NegativePacketLength { length: i64 },
|
||||
#[error("parameter {tag} length is too large: {length} bytes")]
|
||||
ParameterLengthTooLarge { tag: u64, length: u64 },
|
||||
#[error("open reply has zero token without close flag")]
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
use crate::{
|
||||
params::{Params, StampAt},
|
||||
ProtoError, Result, HEADER_SIZE, HMAC_SIZE, RECV_COUNT_SIZE, RECV_WINDOW_SIZE, SEQ_SIZE,
|
||||
TIMESTAMP_SIZE, TOKEN_SIZE,
|
||||
HEADER_SIZE, HMAC_SIZE, RECV_COUNT_SIZE, RECV_WINDOW_SIZE, SEQ_SIZE, TIMESTAMP_SIZE,
|
||||
TOKEN_SIZE,
|
||||
};
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
@@ -111,13 +111,23 @@ pub fn echo_header_len(hmac: bool, params: &Params) -> usize {
|
||||
PacketLayout::echo(hmac, params).header_len()
|
||||
|
|
||||
}
|
||||
|
||||
pub fn echo_packet_len(hmac: bool, params: &Params) -> Result<usize> {
|
||||
let header_len = echo_header_len(hmac, params);
|
||||
let requested =
|
||||
usize::try_from(params.length).map_err(|_| ProtoError::NegativePacketLength {
|
||||
length: params.length,
|
||||
})?;
|
||||
Ok(header_len.max(requested))
|
||||
/// The datagram length of an ECHO request or reply under `params`.
|
||||
///
|
||||
/// The negotiated length is a floor a peer asks for, never a ceiling: a packet
|
||||
/// can never be shorter than the field block its negotiated layout requires, so
|
||||
/// the result is `max(header, requested)`.
|
||||
///
|
||||
/// A negative negotiated length is a legitimate input rather than an encoder
|
||||
/// error. It is accepted during open and echoed back unchanged, so a session
|
||||
/// can genuinely carry one, and there is no datagram shorter than none — it
|
||||
/// therefore requests no space beyond the mandatory field block, exactly as
|
||||
/// zero does.
|
||||
pub fn echo_packet_len(hmac: bool, params: &Params) -> usize {
|
||||
// Saturating rather than wrapping: on a target whose `usize` is narrower
|
||||
// than the requested length, the largest representable packet is the
|
||||
// closest honest answer, and the encoder's own limits reject it from there.
|
||||
let requested = usize::try_from(params.length.max(0)).unwrap_or(usize::MAX);
|
||||
echo_header_len(hmac, params).max(requested)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
@@ -278,32 +288,18 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn negotiated_length_never_smaller_than_header() {
|
||||
fn negotiated_length_is_floored_at_the_required_field_block() {
|
||||
// The 60-byte header of this layout is the floor. A negative negotiated
|
||||
// length is not an error: it asks for nothing beyond the mandatory
|
||||
// fields, exactly as zero does.
|
||||
let mut p = params(ReceivedStats::Both, StampAt::Both, Clock::Both);
|
||||
p.length = 20;
|
||||
assert_eq!(echo_packet_len(false, &p).unwrap(), 60);
|
||||
p.length = 92;
|
||||
assert_eq!(echo_packet_len(false, &p).unwrap(), 92);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn checked_packet_len_rejects_negative_requested_length() {
|
||||
let mut p = params(ReceivedStats::Both, StampAt::Both, Clock::Both);
|
||||
p.length = -1;
|
||||
|
||||
assert_eq!(
|
||||
echo_packet_len(false, &p),
|
||||
Err(ProtoError::NegativePacketLength { length: -1 })
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn checked_packet_len_preserves_non_negative_header_floor() {
|
||||
let mut p = params(ReceivedStats::Both, StampAt::Both, Clock::Both);
|
||||
p.length = 0;
|
||||
assert_eq!(echo_packet_len(false, &p), Ok(60));
|
||||
|
||||
p.length = 92;
|
||||
assert_eq!(echo_packet_len(false, &p), Ok(92));
|
||||
for (length, expected) in [(-4096, 60), (-1, 60), (0, 60), (20, 60), (92, 92)] {
|
||||
p.length = length;
|
||||
assert_eq!(
|
||||
echo_packet_len(false, &p),
|
||||
expected,
|
||||
"unexpected packet length for negotiated length {length}"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -84,8 +84,8 @@ pub enum DecodedRequestKind<'a> {
|
||||
/// # Errors
|
||||
///
|
||||
/// Returns [`ProtoError::PayloadTooLarge`] when an ECHO payload does not fit
|
||||
/// the negotiated length, and [`ProtoError::NegativePacketLength`] when the
|
||||
/// negotiated length is negative.
|
||||
/// the negotiated length. 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>> {
|
||||
match request {
|
||||
RequestToEncode::Open { params, no_test } => encode_open(params, no_test, hmac_key),
|
||||
@@ -205,7 +205,7 @@ fn encode_echo(
|
||||
hmac_key: Option<&[u8]>,
|
||||
) -> Result<Vec<u8>> {
|
||||
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 available_payload_len = len.saturating_sub(payload_offset);
|
||||
if payload.len() > available_payload_len {
|
||||
|
||||
@@ -305,6 +305,32 @@ fn echo_payload_is_copied_zero_filled_and_bounded_in_both_directions() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_negative_negotiated_length_still_encodes_in_both_directions() {
|
||||
// A server accepts a negative negotiated length during open and returns it
|
||||
// unchanged, so a live session can hold one and must stay executable. There
|
||||
// is no datagram shorter than none, so both directions fall back to the
|
||||
// mandatory field block — header (4) + token (8) + sequence (4) — exactly
|
||||
// as a zero length would.
|
||||
let params = Params {
|
||||
length: -4096,
|
||||
..Params::default()
|
||||
};
|
||||
assert_eq!(echo_packet_len(false, ¶ms), 16);
|
||||
|
||||
let request = encode_request(echo_request(¶ms, &[]), None)
|
||||
.expect("a negative negotiated length must still encode a request");
|
||||
assert_eq!(request.len(), 16);
|
||||
|
||||
let reply = encode_echo_reply(&echo_reply(¶ms, FLAG_REPLY), ¶ms, None)
|
||||
.expect("a negative negotiated length must still encode a reply");
|
||||
assert_eq!(reply.len(), 16);
|
||||
assert_eq!(
|
||||
decode_echo_reply(&reply, ¶ms, None).unwrap().token,
|
||||
TOKEN
|
||||
);
|
||||
}
|
||||
|
||||
// ---------- Upstream midpoint ECHO-reply compatibility ----------
|
||||
//
|
||||
// Verified upstream irtt 0.9.1 behavior: for single-clock
|
||||
@@ -346,7 +372,7 @@ fn midpoint_params(clock: Clock, payload_space: usize) -> Params {
|
||||
|
||||
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())
|
||||
compat_header.max(echo_packet_len(hmac, params))
|
||||
}
|
||||
|
||||
/// Hand-builds upstream's dual-midpoint ECHO reply: both midpoint fields on
|
||||
@@ -399,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 clock in [Clock::Wall, Clock::Monotonic] {
|
||||
let params = midpoint_params(clock, payload_space);
|
||||
let normal_len = echo_packet_len(false, ¶ms).unwrap();
|
||||
let normal_len = echo_packet_len(false, ¶ms);
|
||||
let packet = build_midpoint_dual_field_reply(¶ms, FLAG_REPLY, 3, (111, 222), None);
|
||||
assert_eq!(
|
||||
packet.len(),
|
||||
@@ -438,7 +464,7 @@ fn midpoint_dual_field_reply_of_equal_length_is_accepted_but_not_correctable() {
|
||||
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 normal_len = echo_packet_len(false, ¶ms);
|
||||
let packet = build_midpoint_dual_field_reply(¶ms, FLAG_REPLY, 4, (111, 222), None);
|
||||
assert_eq!(
|
||||
packet.len(),
|
||||
@@ -491,7 +517,7 @@ fn midpoint_dual_field_reply_authenticates_over_full_packet() {
|
||||
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();
|
||||
let expected_len = echo_packet_len(false, ¶ms);
|
||||
assert_eq!(packet.len(), expected_len + 8);
|
||||
|
||||
// Every other length around the compatibility size stays rejected,
|
||||
|
||||
@@ -450,21 +450,34 @@ fn encoded_requests_decode_back_to_their_sender_side_identity() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn echo_encoding_rejects_a_negative_negotiated_length() {
|
||||
fn echo_encoding_floors_a_negative_negotiated_length_at_the_field_block() {
|
||||
// A negative negotiated length is accepted during open and echoed back
|
||||
// unchanged, so a real session can carry one. It must therefore stay
|
||||
// encodable: it asks for no space beyond the mandatory fields.
|
||||
let params = Params {
|
||||
length: -1,
|
||||
..Params::default()
|
||||
};
|
||||
let packet = encode_request(
|
||||
RequestToEncode::Echo {
|
||||
token: TOKEN,
|
||||
sequence: SEQUENCE,
|
||||
params: ¶ms,
|
||||
payload: &[],
|
||||
},
|
||||
None,
|
||||
)
|
||||
.expect("a negative negotiated length must still encode");
|
||||
|
||||
// header (4) + token (8) + sequence (4).
|
||||
assert_eq!(packet.len(), 16);
|
||||
let request = decode_request(&packet).unwrap();
|
||||
assert_eq!(
|
||||
encode_request(
|
||||
RequestToEncode::Echo {
|
||||
token: TOKEN,
|
||||
sequence: SEQUENCE,
|
||||
params: ¶ms,
|
||||
payload: &[],
|
||||
},
|
||||
None,
|
||||
),
|
||||
Err(ProtoError::NegativePacketLength { length: -1 })
|
||||
request.kind,
|
||||
DecodedRequestKind::Echo {
|
||||
token: TOKEN,
|
||||
sequence: SEQUENCE,
|
||||
tail: &[],
|
||||
}
|
||||
);
|
||||
}
|
||||
Reference in new issue
Block a user
On 32-bit targets, a positive wire length above
u32::MAXnow becomesusize::MAX;encode_requestandencode_echo_replypass that value toVec::with_capacity, which panics with a capacity overflow instead of returning a protocol error as the previous checked conversion did. This can terminate a process encoding untrusted or otherwise oversizedParams, so only negative values should be floored while positive values that do not fit the platform should remain fallible.AGENTS.md reference: AGENTS.md:L76-L78
Useful? React with 👍 / 👎.
Corrected the misleading comment in
c833b06, but kept the saturating behavior — the premise that the old conversion guarded oversized lengths only holds on 32-bit. On 64-bit (every target indist-workspace.toml)usize::try_fromsucceeds for all positivei64, somainalready reachedVec::with_capacitywith an absurd length and panicked identically; the old error was incidental to the negative check and even reported "must be non-negative" for a positive value. It is also not attacker-reachable: the client rejects a negotiated length that is negative or increased above its own request (negotiation.rs:178),ClientConfig::lengthisu32, and the server does not encode ECHO replies yet. ReintroducingResultto guard that would undo this PR's API cleanup for a case no shipped target and no untrusted input can reach — a real cap belongs with the deferred maximum-packet-length policy. What the comment claimed was false, though: there are no "encoder's own limits", so it now says the allocation is where it fails.🤖 Addressed by Claude Code
Reopened and fixed properly in
3845142— my earlier reply argued the narrow facts correctly but drew the wrong conclusion, so this now does what you asked for.echo_packet_lenis fallible again, but negatives stay floored and the error is a newPacketLengthUnrepresentable { length: i64 }rather than a resurrectedNegativePacketLength, whose name would have been nonsense here:Representability only — no MTU, maximum packet size, or allocation bound; that stays with the deferred policy slice, as does the pre-existing 64-bit
Vec::with_capacityhazard for absurd-but-representable lengths.recv_buffer_sizeand the client call sites go back toResultwith it; propagating an error that negotiation makes unreachable beats baking a 64-bit assumption into shared protocol code.Verified on an actual 32-bit target rather than by argument:
cargo check -p irtt-proto --target i686-unknown-linux-gnu --all-targetspasses, and a temporary deliberate type error inside the new#[cfg(target_pointer_width = "32")]test failed the 32-bit check while the 64-bit check stayed green — so the test really is compiled where the branch exists. Workspacefmt/check/test/clippy -D warningsandRUSTDOCFLAGS="-D warnings" cargo docare all green. PR description updated, since it still claimed the API was infallible.🤖 Addressed by Claude Code