From ca2c3dcbd80694663299c9b6c84f0b161aa5b657 Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Wed, 12 Aug 2026 17:21:23 +0700 Subject: [PATCH] fix(server): reject unusable timestamp sessions --- crates/irtt-server/AGENTS.md | 24 +++++++++ crates/irtt-server/src/core.rs | 51 +++++++++++++++++-- crates/irtt-server/src/negotiate.rs | 6 +++ crates/irtt-server/src/tests/no_test.rs | 26 +++++++++- crates/irtt-server/src/tests/open.rs | 68 ++++++++++++++++++++++++- 5 files changed, 169 insertions(+), 6 deletions(-) diff --git a/crates/irtt-server/AGENTS.md b/crates/irtt-server/AGENTS.md index 3069944..e4d0dc0 100644 --- a/crates/irtt-server/AGENTS.md +++ b/crates/irtt-server/AGENTS.md @@ -60,6 +60,30 @@ Do not let reply behavior reveal which stage rejected a datagram — in particular, an authentication failure must be indistinguishable from an unknown token. +## An acknowledged open must be executable + +Validate the **final, restricted** parameters before acknowledging an open, not +just the requested ones. Once an open passes admission and negotiation, its +effective parameters must be safe for the server to run. + +- Effective non-none `StampAt` with `Clock::Unspecified` is silently rejected. + Timestamps from no clock lay out no timestamp field, so the session could not + be run. Do **not** synthesize a clock and do **not** rewrite `StampAt` to + none: either answers with a session the client did not ask for. This is + `irtt-rs` policy for nonconforming input, chosen because the clean evidence + records the reference server accepting this combination and then failing on + the first echo. A conforming client always sends a clock when it selects + timestamps, so conforming traffic is unaffected. +- `Clock::Unspecified` alone is valid — it is just an absent tag. Only the + combination is refused, and only an absent tag reaches it, since an explicit + zero Clock is already out of range for the decoder. +- Run the check after negotiation, so that a later policy restricting `StampAt` + to none makes an omitted clock safe without this rule changing. +- Negative `Length` stays an accepted negotiated value, returned unchanged in + the open reply and stored verbatim in the session. It needs no clamping to be + executable: `irtt-proto` floors actual echo packet sizing at the required + protocol field block, so a negative length behaves as zero does. + ## Resource policy Total session and resource state must stay bounded. A single unauthenticated diff --git a/crates/irtt-server/src/core.rs b/crates/irtt-server/src/core.rs index 7320c0e..0e28c6b 100644 --- a/crates/irtt-server/src/core.rs +++ b/crates/irtt-server/src/core.rs @@ -1,8 +1,8 @@ use std::{collections::HashMap, net::SocketAddr}; use irtt_proto::{ - decode_request, encode_open_reply, verify_packet_hmac, DecodedParams, DecodedRequestKind, - OpenReply, Params, FLAG_CLOSE, FLAG_OPEN, FLAG_REPLY, + decode_request, encode_open_reply, verify_packet_hmac, Clock, DecodedParams, + DecodedRequestKind, OpenReply, Params, StampAt, FLAG_CLOSE, FLAG_OPEN, FLAG_REPLY, }; use crate::{ @@ -95,8 +95,9 @@ impl ServerCore { /// datagram is discarded when it is structurally invalid, carries the reply /// flag, disagrees with this server's authentication configuration, fails /// authentication, carries a malformed or out-of-range open parameter - /// payload, would exceed the session bound, or is a request kind this slice - /// does not implement. None of that mutates any live session. + /// payload, negotiates to parameters the server could not execute, would + /// exceed the session bound, or is a request kind this slice does not + /// implement. None of that mutates any live session. /// /// `Ok(None)` is not only a rejection, though. A close request that does /// release its session is answered with nothing as well, because protocol @@ -161,6 +162,11 @@ impl ServerCore { return Ok(None); } let params = negotiate_params(decoded.params, &self.config); + // Deliberately after negotiation, and before either open path: what has + // to be executable is the effective session, not the request. + if !negotiated_params_are_admissible(¶ms) { + return Ok(None); + } if no_test { // No-test validates parameters without running a test: same @@ -284,6 +290,11 @@ fn same_endpoint(session: SocketAddr, peer: SocketAddr) -> bool { /// Applies the open parameter rules the decoder cannot express. /// +/// This judges the *request*, before negotiation, which is why it takes the +/// decoded form with its presence flags. Whether the resulting session is +/// coherent is a separate question, asked after negotiation by +/// [`negotiated_params_are_admissible`]. +/// /// Duration and Interval are accepted when absent, taking the wire default /// zero, and rejected when explicitly present and not positive. The presence /// distinction is why the decoder reports it: testing the value alone would @@ -301,3 +312,35 @@ fn open_params_are_admissible(decoded: &DecodedParams) -> bool { admissible(decoded.presence.duration_ns, decoded.params.duration_ns) && admissible(decoded.presence.interval_ns, decoded.params.interval_ns) } + +/// Whether the *effective* parameters describe a session this server could +/// actually run. +/// +/// This is the last gate before an open is acknowledged, and it asks a +/// different question from [`open_params_are_admissible`]: not whether the +/// request was well formed, but whether the parameters negotiation settled on +/// are coherent. It therefore runs on the negotiated params, after +/// [`negotiate_params`], and covers the no-test path too — a no-test open +/// exists to tell a client what the session would be, so it must not report a +/// session that could not exist. +/// +/// One rule so far: **selecting timestamps requires a specified clock.** A +/// non-none `stamp_at` with [`Clock::Unspecified`] asks for timestamp fields +/// from no clock at all, which lays out no timestamp field and leaves the +/// session with a request the server cannot honor. +/// +/// [`Clock::Unspecified`] means the Clock tag was absent — an explicit zero is +/// already out of range for the decoder — so this cannot be reached by a +/// conforming client, which sends a clock whenever it selects timestamps. The +/// combination is silently rejected rather than repaired: synthesizing a clock +/// would answer with a session the client never asked for, and rewriting +/// `stamp_at` to none would silently drop the measurement it did ask for. +/// Neither is the server's to decide. +/// +/// The check is deliberately *after* negotiation so that a later policy slice +/// restricting timestamps can restrict `stamp_at` to [`StampAt::None`], at +/// which point an absent clock is safe and this predicate holds without +/// changing. +fn negotiated_params_are_admissible(params: &Params) -> bool { + params.stamp_at == StampAt::None || params.clock != Clock::Unspecified +} diff --git a/crates/irtt-server/src/negotiate.rs b/crates/irtt-server/src/negotiate.rs index 6485153..486dfe6 100644 --- a/crates/irtt-server/src/negotiate.rs +++ b/crates/irtt-server/src/negotiate.rs @@ -18,6 +18,12 @@ use crate::config::ServerConfig; /// timestamp allowance, DSCP policy and server-fill allow-list. This function /// is where they belong, which is why the configuration is threaded through /// even though no knob reads it yet. +/// +/// Negotiation rewriting nothing but the version does not mean every open is +/// answered: the core validates what this function returns and silently +/// discards an open whose effective parameters it could not execute. That is a +/// separate step on purpose — this function decides what the session *would* +/// be, and the core decides whether that session can exist. pub(crate) fn negotiate_params(mut requested: Params, _config: &ServerConfig) -> Params { requested.protocol_version = PROTOCOL_VERSION; requested diff --git a/crates/irtt-server/src/tests/no_test.rs b/crates/irtt-server/src/tests/no_test.rs index ea4602d..ea89ca2 100644 --- a/crates/irtt-server/src/tests/no_test.rs +++ b/crates/irtt-server/src/tests/no_test.rs @@ -1,4 +1,4 @@ -use irtt_proto::{verify_packet_hmac, Params}; +use irtt_proto::{verify_packet_hmac, Clock, Params, StampAt}; use super::support::{ client_params, core_with_tokens, expect_no_test_reply, no_test_request, open_request, @@ -74,6 +74,30 @@ fn a_no_test_open_is_rejected_on_the_same_parameters_a_normal_open_is() { assert_eq!(core.session_count(), 0); } +#[test] +fn a_no_test_open_selecting_timestamps_without_a_clock_is_silently_refused() { + // No-test exists to tell a client what the session would be, so it must not + // report a session that could not exist. The effective-parameter check runs + // before the no-test path branches off, so both open forms refuse the same + // incoherent timestamp request. + let tokens = ScriptedTokens::new([TOKEN_A]); + let mut core = core_with_tokens(ServerConfig::default(), tokens.clone()); + + let requested = Params { + clock: Clock::Unspecified, + stamp_at: StampAt::Both, + ..client_params() + }; + + assert_eq!( + core.handle_datagram(peer(), &no_test_request(&requested, None)) + .expect("an unusable timestamp request is not a server error"), + None + ); + assert_eq!(core.session_count(), 0); + assert_eq!(tokens.remaining(), 1, "no token may be drawn"); +} + #[test] fn a_no_test_open_is_still_answered_when_no_session_could_be_created() { // No-test consumes no capacity, so it must keep working exactly where a diff --git a/crates/irtt-server/src/tests/open.rs b/crates/irtt-server/src/tests/open.rs index c2876d7..9a13509 100644 --- a/crates/irtt-server/src/tests/open.rs +++ b/crates/irtt-server/src/tests/open.rs @@ -104,6 +104,10 @@ fn an_empty_parameter_payload_is_answered_with_only_a_version() { ..Params::default() } ); + // An unspecified clock is not itself a defect. Only pairing it with a + // selected `stamp_at` is, so a session requesting no timestamps at all is + // fully valid with no Clock tag. + assert_eq!(reply.params.stamp_at, StampAt::None); assert_eq!(reply.params.clock, Clock::Unspecified); assert_eq!(core.session_count(), 1); } @@ -136,10 +140,61 @@ fn unknown_parameter_tags_are_ignored_and_not_reflected_in_the_reply() { } #[test] -fn this_slice_restricts_nothing_but_the_protocol_version() { +fn selecting_timestamps_without_a_clock_is_silently_refused() { + // Timestamps from no clock at all describe a session the server could not + // run: the negotiated layout carries no timestamp field to fill. The open + // is discarded rather than repaired — synthesizing a clock would answer + // with a session the client never asked for, and dropping `stamp_at` would + // silently discard the measurement it did ask for. + // + // Only an *absent* Clock tag reaches this: an explicit zero is already out + // of range for the decoder, and a conforming client always sends a clock + // when it selects timestamps. + for stamp_at in [ + StampAt::Send, + StampAt::Receive, + StampAt::Both, + StampAt::Midpoint, + ] { + let requested = Params { + clock: Clock::Unspecified, + stamp_at, + ..client_params() + }; + let tokens = ScriptedTokens::new([TOKEN_A]); + let mut core = core_with_tokens(ServerConfig::default(), tokens.clone()); + + let reply = core + .handle_datagram(peer(), &open_request(&requested, None)) + .expect("an unusable timestamp request is not a server error"); + + assert!( + reply.is_none(), + "{stamp_at:?} without a clock must be silent" + ); + assert_eq!( + core.session_count(), + 0, + "{stamp_at:?} without a clock must create no session" + ); + assert_eq!( + tokens.remaining(), + 1, + "{stamp_at:?} without a clock must not consume a token" + ); + } +} + +#[test] +fn negotiation_restricts_nothing_but_the_protocol_version() { // These are the values a later restriction-policy slice will deliberately // start changing. Pinning them now makes that a visible decision rather // than an accident. + // + // This is about what *negotiation* rewrites, which is a separate question + // from whether the core will acknowledge the result: an open whose + // effective parameters are incoherent is still discarded, as + // `selecting_timestamps_without_a_clock_is_silently_refused` shows. let unrestricted = Params { protocol_version: 2, duration_ns: 3_600_000_000_000, @@ -175,6 +230,11 @@ fn negative_and_out_of_range_length_and_dscp_survive_negotiation() { // Length and DSCP are accepted as decoded in this slice, including values a // socket could never carry. Restricting a DSCP the server cannot apply is a // later policy decision, not an admission rule. + // + // A negative length in particular stays negative in both the reply and the + // stored session. It does not need clamping here to be executable: echo + // packet sizing in `irtt-proto` floors it at the required field block, so + // the session this creates can be run as negotiated. for (length, dscp) in [(-1, -1), (0, 0), (1472, 184), (-4096, 512)] { let requested = Params { protocol_version: 1, @@ -193,5 +253,11 @@ fn negative_and_out_of_range_length_and_dscp_survive_negotiation() { assert_eq!(reply.params.length, length); assert_eq!(reply.params.dscp, dscp); assert_eq!(core.session_count(), 1); + let session = core.session(TOKEN_A).expect("the session must be live"); + assert_eq!( + session.params(), + &reply.params, + "the session must hold the negotiated values verbatim" + ); } }