mirror of
https://github.com/jmcorgan/fips.git
synced 2026-10-05 11:08:25 +00:00
Refuse a ceiling frame counter and saturate the gap tracker
An authenticated peer sending a frame counter of u64::MAX pinned its own replay window's high-water mark at the ceiling, after which every later counter it sent fell more than a window below `highest` and was dropped, wedging that peer's receive path until a rekey replaced the session. The same counter overflowed the MMP gap tracker's expected-counter add, which wraps to zero in a release build and aborts under a build with overflow checks on. Refuse the value in ReplayWindow::check, which covers the FSP session paths and the off-task FMP decrypt worker in one place, and advance the gap tracker with a saturating add. Neither send path in this tree can emit u64::MAX, so no conforming peer notices; u64::MAX - 1, the highest counter an honest peer can send, is unaffected.
This commit is contained in:
@@ -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
|
||||
|
||||
+27
-2
@@ -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();
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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();
|
||||
|
||||
Reference in New Issue
Block a user