From a9509a235d85894b105c1194f4c70d68e525cdf3 Mon Sep 17 00:00:00 2001 From: ospab Date: Sat, 11 Jul 2026 21:25:36 +0300 Subject: [PATCH] fix: low-severity hardening (Karn RTT, 32-bit frame overflow, replay-cache DoS) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Karn's algorithm: drop_acked_frames no longer samples RTT from frames that were retransmitted (last_sent is bumped on each retransmit, so an ACK for the original transmission would measure a spuriously small RTT and drag SRTT/RTO down). Added CongestionController::on_ack_no_rtt for the case where every acked frame was ambiguous, so the window still advances without polluting the RTT estimator. Refactored the shared window-growth into grow_window. - Frame decode: header+payload+pad length now uses checked_add. payload_len is a u32 from the header and on 32-bit targets (MIPS/ARMv7 routers are supported) the sum could wrap usize and slip past the truncation check. - Replay cache: a full cache used to reject ALL new handshakes globally until the next tick, letting one flooding key-holder deny service to everyone. Now it reclaims expired entries and, if still full, evicts the single oldest — new handshakes always get in. Fixed the mislabelled "100000" log (cap is 50000) and named it REPLAY_CACHE_MAX. --- ostp-core/src/congestion.rs | 40 +++++++++++++++++++++++++++++++--- ostp-core/src/framing/frame.rs | 10 ++++++++- ostp-core/src/protocol.rs | 24 ++++++++++++++------ ostp-server/src/dispatcher.rs | 32 ++++++++++++++++++++++++--- 4 files changed, 92 insertions(+), 14 deletions(-) diff --git a/ostp-core/src/congestion.rs b/ostp-core/src/congestion.rs index 22758d0..9e790e1 100644 --- a/ostp-core/src/congestion.rs +++ b/ostp-core/src/congestion.rs @@ -144,6 +144,19 @@ impl CongestionController { self.bytes_in_flight = self.bytes_in_flight.saturating_add(bytes); } + /// Record that `bytes` were acknowledged but WITHOUT a usable RTT sample + /// (e.g. every acked frame was retransmitted, so Karn's algorithm forbids + /// measuring RTT from it). The window still advances; only the RTT estimator + /// is left untouched. + pub fn on_ack_no_rtt(&mut self, bytes: u64) { + let now = Instant::now(); + self.bytes_in_flight = self.bytes_in_flight.saturating_sub(bytes); + self.total_acked = self.total_acked.saturating_add(bytes); + self.grow_window(bytes); + self.update_pacing_rate(); + self.last_ack_time = now; + } + /// Record that `bytes` were acknowledged with the given RTT sample. pub fn on_ack(&mut self, bytes: u64, rtt: Duration) { let now = Instant::now(); @@ -153,6 +166,13 @@ impl CongestionController { // Update RTT measurements self.update_rtt(rtt, now); + self.grow_window(bytes); + self.update_pacing_rate(); + self.last_ack_time = now; + } + + /// Congestion-window growth shared by both ACK paths (slow start / probe). + fn grow_window(&mut self, bytes: u64) { // State machine match self.phase { Phase::SlowStart => { @@ -168,9 +188,6 @@ impl CongestionController { self.cwnd = self.cwnd.saturating_add(bytes * self.mtu / self.cwnd.max(1)); } } - - self.update_pacing_rate(); - self.last_ack_time = now; } /// Record a loss event. @@ -313,6 +330,23 @@ mod tests { assert_eq!(rto, Duration::from_millis(150)); } + #[test] + fn test_on_ack_no_rtt_grows_window_without_touching_srtt() { + let mut cc = CongestionController::new(1200); + // Establish a known SRTT with a real sample. + cc.on_send(1200); + cc.on_ack(1200, Duration::from_millis(40)); + let srtt_before = cc.smoothed_rtt(); + let cwnd_before = cc.cwnd(); + + // A Karn's-algorithm ACK (all acked frames were retransmitted): window + // must advance, RTT estimate must be untouched. + cc.on_send(1200); + cc.on_ack_no_rtt(1200); + assert!(cc.cwnd() > cwnd_before, "cwnd should still grow on a no-RTT ack"); + assert_eq!(cc.smoothed_rtt(), srtt_before, "SRTT must not move on a no-RTT ack"); + } + #[test] fn test_rto_clamp_min() { let cc = CongestionController::new(1200); diff --git a/ostp-core/src/framing/frame.rs b/ostp-core/src/framing/frame.rs index 74f6b18..370aa77 100644 --- a/ostp-core/src/framing/frame.rs +++ b/ostp-core/src/framing/frame.rs @@ -101,7 +101,15 @@ impl FramedPacket { let payload_len = header.payload_len as usize; let pad_len = header.pad_len as usize; - let expected = FRAME_HEADER_LEN + payload_len + pad_len; + // Use checked arithmetic: payload_len is a u32 from the (decrypted, but + // still to-be-trusted) header, and on 32-bit targets — MIPS/ARMv7 + // routers are supported build targets — header+payload+pad can overflow + // usize and wrap to a small value that spuriously passes the length + // check, causing an out-of-range slice below. + let expected = FRAME_HEADER_LEN + .checked_add(payload_len) + .and_then(|v| v.checked_add(pad_len)) + .ok_or_else(|| ProtocolError::Framing("frame length overflow".to_string()))?; if buf.len() < expected { return Err(ProtocolError::Framing("frame body truncated".to_string())); } diff --git a/ostp-core/src/protocol.rs b/ostp-core/src/protocol.rs index f1d0ba7..a1d1a5e 100644 --- a/ostp-core/src/protocol.rs +++ b/ostp-core/src/protocol.rs @@ -683,24 +683,34 @@ impl ProtocolMachine { fn drop_acked_frames(&mut self, ranges: &[(u64, u64)]) { let now = Instant::now(); let mut acked_bytes = 0u64; - let mut min_rtt = Duration::from_secs(60); + let mut min_rtt: Option = None; - // Compute RTT from the oldest acked frame's send timestamp for frame in self.sent_history.iter() { if nonce_in_ranges(frame.nonce, ranges) { acked_bytes += frame.bytes.len() as u64; - let rtt = now.duration_since(frame.last_sent); - if rtt < min_rtt { - min_rtt = rtt; + // Karn's algorithm: never take an RTT sample from a frame that + // was retransmitted. `last_sent` is bumped on every retransmit, + // so an ACK for the ORIGINAL transmission would be measured + // against the retransmit time, yielding a spuriously small RTT + // that drags SRTT/RTO down and triggers more spurious + // retransmits. Only unambiguous (never-retried) frames qualify. + if frame.retries == 0 { + let rtt = now.duration_since(frame.last_sent); + min_rtt = Some(min_rtt.map_or(rtt, |m| m.min(rtt))); } } } self.sent_history.retain(|frame| !nonce_in_ranges(frame.nonce, ranges)); - // Notify congestion controller + // Notify congestion controller. Feed an RTT sample only when we had at + // least one unambiguous ACK; otherwise update the window without + // polluting the RTT estimator. if acked_bytes > 0 { - self.cc.on_ack(acked_bytes, min_rtt); + match min_rtt { + Some(rtt) => self.cc.on_ack(acked_bytes, rtt), + None => self.cc.on_ack_no_rtt(acked_bytes), + } } } } diff --git a/ostp-server/src/dispatcher.rs b/ostp-server/src/dispatcher.rs index 8316ab4..20e144a 100644 --- a/ostp-server/src/dispatcher.rs +++ b/ostp-server/src/dispatcher.rs @@ -11,6 +11,11 @@ use portable_atomic::AtomicU64; /// Excess handshake attempts are silently dropped -- no response, no state allocated. const MAX_SESSIONS: usize = 1024; +/// Cap on the anti-replay handshake cache. When reached, expired entries are +/// reclaimed (and if needed the oldest is evicted) rather than rejecting new +/// handshakes globally — see the eviction logic in on_datagram. +const REPLAY_CACHE_MAX: usize = 50_000; + pub enum DispatchOutcome { Unauthorized, /// Packet matched a registered key's per-key junk marker — drop silently. @@ -457,9 +462,30 @@ impl Dispatcher { } if !self.replay_cache.contains_key(&payload.to_vec()) { - if self.replay_cache.len() >= 50_000 { - tracing::warn!("Replay cache full (100000 entries), rejecting handshake from {}", peer); - return Ok(DispatchOutcome::Unauthorized); + if self.replay_cache.len() >= REPLAY_CACHE_MAX { + // Don't globally reject new handshakes when full — + // that would let one flooding key-holder deny + // service to everyone. Reclaim space instead: + // first drop entries already past the drift + // window, then, if still full, evict the single + // oldest. A replay is still caught because it can + // only be accepted while within the 300s drift + // window, and an entry that young is never the + // one evicted before the cache genuinely holds + // 50k sub-300s handshakes. + self.replay_cache.retain(|_, &mut cached_ts| { + (now as i64 - cached_ts as i64).abs() <= 300 + }); + if self.replay_cache.len() >= REPLAY_CACHE_MAX { + if let Some(oldest) = self.replay_cache + .iter() + .min_by_key(|(_, &ts)| ts) + .map(|(k, _)| k.clone()) + { + self.replay_cache.remove(&oldest); + } + tracing::warn!("Replay cache full ({} entries), evicting oldest", REPLAY_CACHE_MAX); + } } if self.peer_machines.len() >= MAX_SESSIONS { tracing::warn!("Max sessions reached ({}), rejecting handshake from {}", MAX_SESSIONS, peer);