diff --git a/CHANGELOG.md b/CHANGELOG.md index 51947372..d08a2786 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -529,6 +529,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 #### FMP/FSP session integrity +- A frame whose counter is `u64::MAX` is now refused by the replay window + instead of being accepted as a new high-water mark. Accepting it pinned + `highest` at the ceiling, after which every subsequent counter from that peer + fell more than a replay window below it and was rejected, wedging that peer's + own receive path until a rekey replaced the session. The send side already + refuses to emit that counter (`take_send_counter` and `advance_nonce` both + return a nonce-overflow error), so no conforming peer can produce it and the + refusal is invisible on the wire; the highest counter an honest peer can send, + `u64::MAX - 1`, is still accepted. Reaching this required an + already-authenticated peer running modified code, and the damage was confined + to that peer's own session. + +- The MMP gap tracker advances its expected-counter state with a saturating add, + so a received counter of `u64::MAX` no longer overflows it. The wrap silently + reset the expectation to zero in a release build and aborted the task under a + build with overflow checks on, such as the test harness. Behaviour is + unchanged for every counter an honest peer can emit. + - A session setup message naming an already-established peer no longer replaces that peer's session. The handler did this whenever `node.rekey.enabled` was false: it ran a fresh responder handshake and overwrote the entry, discarding diff --git a/src/mmp/receiver.rs b/src/mmp/receiver.rs index 3c3b5e0e..3cbfee1b 100644 --- a/src/mmp/receiver.rs +++ b/src/mmp/receiver.rs @@ -62,7 +62,7 @@ impl GapTracker { fn observe(&mut self, counter: u64) -> u64 { let Some(expected) = self.expected_next else { // First frame: initialize - self.expected_next = Some(counter + 1); + self.expected_next = Some(counter.saturating_add(1)); return 0; }; @@ -91,7 +91,7 @@ impl GapTracker { // Update expected (always advance to counter+1 or keep expected if // this was a late/reordered frame) if counter >= expected { - self.expected_next = Some(counter + 1); + self.expected_next = Some(counter.saturating_add(1)); } lost @@ -560,6 +560,31 @@ mod tests { assert_eq!(mean, 0); } + #[test] + fn test_gap_tracker_saturates_on_first_frame_at_max_counter() { + // Pre-fix this aborts the test process at the unchecked `counter + 1` + // under the dev profile's overflow checks rather than failing an + // assertion; the abort is the red, not a harness fault. + let mut g = GapTracker::new(); + assert_eq!(g.observe(u64::MAX), 0); + // A saturated expectation must simply stop the tracker advancing, and + // every later counter then takes the in-order branch. + assert_eq!(g.observe(u64::MAX), 0); + let (count, max, _mean) = g.take_interval_stats(); + assert_eq!(count, 0); + assert_eq!(max, 0); + } + + #[test] + fn test_gap_tracker_saturates_on_advance_at_max_counter() { + // Primes the tracker first so the advance branch, not the first-frame + // branch, is the site that would overflow. + let mut g = GapTracker::new(); + g.observe(10); + g.observe(u64::MAX); + assert_eq!(g.observe(u64::MAX), 0); + } + #[test] fn test_gap_tracker_single_burst() { let mut g = GapTracker::new(); diff --git a/src/noise/replay.rs b/src/noise/replay.rs index 2e4e6443..99217e05 100644 --- a/src/noise/replay.rs +++ b/src/noise/replay.rs @@ -31,6 +31,14 @@ impl ReplayWindow { /// Returns true if the counter is acceptable, false if it should be rejected. /// Does NOT update the window - call `accept` after successful decryption. pub fn check(&self, counter: u64) -> bool { + // The send side refuses to emit u64::MAX (`CipherState::advance_nonce`, + // `NoiseSession::take_send_counter`), so no conforming peer produces it. + // Refusing it here keeps `accept` from pinning `highest` at the ceiling, + // which would wedge the window against every later counter. + if counter == u64::MAX { + return false; + } + if counter > self.highest { // New highest - always acceptable return true; diff --git a/src/noise/tests.rs b/src/noise/tests.rs index 82cd7a93..baabfc04 100644 --- a/src/noise/tests.rs +++ b/src/noise/tests.rs @@ -387,6 +387,41 @@ fn test_replay_window_reset() { assert!(window.check(100)); } +#[test] +fn test_replay_window_max_counter_does_not_wedge_the_window() { + let mut window = ReplayWindow::new(); + + window.accept(100); + // Mirror the real receive path: check, and accept only if the check passed. + if window.check(u64::MAX) { + window.accept(u64::MAX); + } + + // The honest peer's next frame must still be acceptable. + assert!(window.check(101), "ceiling frame wedged the window"); +} + +#[test] +fn test_replay_window_rejects_max_counter() { + let window = ReplayWindow::new(); + assert!(!window.check(u64::MAX)); +} + +#[test] +fn test_replay_window_accepts_highest_counter_an_honest_peer_can_send() { + // take_send_counter refuses u64::MAX, so u64::MAX - 1 is the highest + // counter a conforming peer emits. The ceiling guard must be exactly one + // value wide and leave that one alone. + let mut window = ReplayWindow::new(); + assert!(window.check(u64::MAX - 1)); + + window.accept(u64::MAX - 1); + assert!( + !window.check(u64::MAX - 1), + "replay should still be rejected" + ); +} + #[test] fn test_session_replay_protection() { let keypair1 = generate_keypair();