diff --git a/src/node/handlers/session.rs b/src/node/handlers/session.rs index 15aad913..cb343bce 100644 --- a/src/node/handlers/session.rs +++ b/src/node/handlers/session.rs @@ -684,15 +684,30 @@ impl Node { // the veto returns without touching them, and the // fall-through only arms a handshake beside them. Adopting // them is still an authenticated msg3's job alone. No arm - // below drops one either: the dual-initiation arm did until - // it was narrowed to abandon only the handshake, for the - // reason recorded at that site. + // below drops one either. let pending_outranks = existing.pending_new_session().is_some() && !existing.pending_stale( Self::now_ms(), self.config().node.session.idle_timeout_secs * 1000, ); + // A handshake the peer armed is held until its msg3 or its + // timeout. Once the peer has read our SessionAck it holds the + // new keys, so this handshake is the only one its msg3 can + // complete, and discarding it for an unauthenticated setup + // splits the session's epochs. A genuine retry that meets a + // stale handshake here completes one handshake timeout + // later, once the handshake has expired. + if rekey_in_progress && !existing.is_rekey_initiator() { + debug!( + src = %self.peer_display_name(src_addr), + "FSP rekey msg1 received while the peer's handshake awaits msg3, dropping" + ); + self.stats_mut() + .record_reject(RejectReason::Session(SessionReject::RekeyHeld)); + return; + } + // Dual-initiation detection: both sides sent SessionSetup // simultaneously. Apply tie-breaker — smaller NodeAddr // wins as initiator (same as initial session setup). @@ -707,22 +722,16 @@ impl Node { .record_reject(RejectReason::Session(SessionReject::RekeyTiebreak)); return; } - // We lose — abandon the armed handshake, become responder - // below. + // We lose — abandon our armed handshake, become + // responder below. // - // `abandon_handshake`, not `abandon_rekey`: the gate - // above is `has_rekey_in_progress`, which says only that - // *some* handshake is armed, not that we armed it. A - // handshake the peer armed carries `rekey_initiator == - // false` and can sit beside a completed epoch that a - // stale `pending_outranks` no longer vetoes, so a - // stranger reaches this line with two unauthenticated - // setup messages: one to arm the handshake, one to lose - // the tie-break against it. Dropping the pending session - // there kills the epoch the peer may already have cut - // over to. Only the handshake is ours to discard, and - // discarding it costs nothing, since an armed handshake - // holds no key material either endpoint is using. + // `abandon_handshake`, not `abandon_rekey`, although the + // arm above leaves only handshakes this node initiated, + // and an initiator handshake never sits beside a pending + // session (see the SessionAck handler). Only the handshake + // is ours to discard, and discarding it costs nothing: + // the peer has not answered it, so it holds no key + // material either endpoint is using. debug!( src = %self.peer_display_name(src_addr), "Dual FSP rekey initiation: we lose (larger addr), abandoning ours" @@ -1132,15 +1141,18 @@ impl Node { // Rekey path: entry is Established with rekey_state (responder side) // - // Every failure below abandons only the handshake. Nothing in a msg3 - // is authenticated until `read_xk_message_3` has both succeeded and - // produced a static key matching this session's peer, so a failure - // here proves nothing about the sender and must not cost the entry - // anything it would miss. A `pending_new_session` beside the - // handshake is the epoch the real peer may already have cut over to, - // and dropping it kills the reverse direction on two unauthenticated - // messages: a forged msg1 to arm the handshake, then any garbage - // msg3. `abandon_handshake` keeps it; `abandon_rekey` does not. + // Nothing in a msg3 is authenticated until the read has both + // succeeded and produced a static key matching this session's peer, + // so a failure here proves nothing about the sender and must not cost + // the entry anything it would miss. An unreadable msg3 therefore + // costs nothing at all: the handshake goes back, rolled back, for the + // genuine msg3. Every later failure follows a read that + // authenticated its sender and abandons only the handshake. A + // `pending_new_session` beside the handshake is the epoch the real + // peer may already have cut over to, and dropping it kills the + // reverse direction on two unauthenticated messages: a forged msg1 to + // arm the handshake, then any garbage msg3. `abandon_handshake` keeps + // it; `abandon_rekey` does not. // // What `abandon_handshake` leaves behind, and why each is safe here: // `rekey_completed_ms` must survive, since `pending_stale` reads it @@ -1158,14 +1170,23 @@ impl Node { } }; - // Process XK msg3 - if let Err(e) = handshake.read_xk_message_3(&msg3.handshake_payload) { + // Process XK msg3. The only tie between this msg3 and the + // handshake is the datagram's source address, which the sender + // chooses, and the peer that read our SessionAck may already hold + // the new keys. Abandoning here would let anyone able to name the + // session discard the handshake the peer's genuine msg3 needs, + // splitting the session's epochs. So the handshake goes back, + // rolled back to its pre-read state, because the read advances + // the cipher nonce before it authenticates. The restore leaves + // the deadline alone, which runs from the peer's setup, so a + // spray cannot hold the handshake open. + if let Err(e) = handshake.try_read_xk_message_3(&msg3.handshake_payload) { debug!( src = %self.peer_display_name(src_addr), error = %e, - "Failed to process rekey XK msg3" + "Failed to process rekey XK msg3, keeping the handshake" ); - entry.abandon_handshake(); + entry.set_rekey_state(handshake, false); self.sessions.insert(*src_addr, entry); return; } @@ -1252,10 +1273,23 @@ impl Node { _ => unreachable!("checked is_awaiting_msg3 above"), }; - // Process XK msg3: read_xk_message_3 (extracts initiator's static key and epoch) - if let Err(e) = handshake.read_xk_message_3(&msg3.handshake_payload) { - debug!(error = %e, "Failed to process Noise XK msg3"); - return; // Entry was already removed + // Process XK msg3 (extracts the initiator's static key and epoch). + // + // Nothing here has been authenticated: the only tie to this half-open + // entry is the datagram's source address, which the sender chooses, + // and the initiator considers the session established once it has + // sent msg3. Dropping the entry would let anyone able to name the + // initiator discard the handshake its genuine msg3 and resends need, + // so the entry goes back with the handshake rolled back to its + // pre-read state, as the SessionAck arm does. `touch()` is + // deliberately not called, so a spray cannot push the handshake + // sweep's deadline out. The drops below stay drops: each follows a + // read that authenticated the sender. + if let Err(e) = handshake.try_read_xk_message_3(&msg3.handshake_payload) { + debug!(error = %e, "Failed to process Noise XK msg3, keeping the handshake"); + entry.set_state(EndToEndState::AwaitingMsg3(handshake)); + self.sessions.insert(*src_addr, entry); + return; } // Extract the initiator's static public key (now available after msg3) diff --git a/src/node/reject.rs b/src/node/reject.rs index 40bfbd9d..34ed5e32 100644 --- a/src/node/reject.rs +++ b/src/node/reject.rs @@ -251,6 +251,11 @@ pub enum SessionReject { /// is being suppressed. Tracked via /// [`SessionStats::rekey_yielded`](crate::node::stats::SessionStats). RekeyYielded, + /// A setup message named an established peer while a handshake that + /// peer armed was still waiting for its msg3, so the message was dropped + /// and the handshake kept. Tracked via + /// [`SessionStats::rekey_held`](crate::node::stats::SessionStats). + RekeyHeld, /// A setup message named an established peer that already holds a /// completed rekey awaiting cut-over, so the message was dropped /// rather than arming a second handshake. Tracked via diff --git a/src/node/stats.rs b/src/node/stats.rs index a638f131..e4fd7d04 100644 --- a/src/node/stats.rs +++ b/src/node/stats.rs @@ -56,6 +56,12 @@ pub struct SessionStats { /// abandoned our own rekey and answered as responder. A sustained /// rate here means local key rotation is being suppressed. pub rekey_yielded: u64, + /// A setup message named an established peer while a handshake that + /// peer armed was still waiting for its msg3, so the message was dropped + /// and the handshake kept. A genuine retry lands here when the peer + /// abandoned a rekey this node answered; a sustained rate means setups + /// are being sprayed at the session. + pub rekey_held: u64, /// A setup message named an established peer that already holds a /// completed rekey awaiting cut-over, so the message was dropped /// rather than arming a second handshake. @@ -105,6 +111,7 @@ impl SessionStats { rekey_armed: self.rekey_armed, rekey_tiebreak: self.rekey_tiebreak, rekey_yielded: self.rekey_yielded, + rekey_held: self.rekey_held, rekey_pending: self.rekey_pending, rekey_expired: self.rekey_expired, rekey_unanswered: self.rekey_unanswered, @@ -124,6 +131,7 @@ impl SessionStats { SessionReject::RekeyKeyMismatch => self.rekey_key_mismatch += 1, SessionReject::RekeyTiebreak => self.rekey_tiebreak += 1, SessionReject::RekeyYielded => self.rekey_yielded += 1, + SessionReject::RekeyHeld => self.rekey_held += 1, SessionReject::RekeyPending => self.rekey_pending += 1, SessionReject::AckHandshakeFailed => self.ack_handshake_failed += 1, SessionReject::SetupRateLimited => self.setup_rate_limited += 1, @@ -411,6 +419,7 @@ pub struct SessionStatsSnapshot { pub rekey_armed: u64, pub rekey_tiebreak: u64, pub rekey_yielded: u64, + pub rekey_held: u64, pub rekey_pending: u64, pub rekey_expired: u64, pub rekey_unanswered: u64, @@ -538,14 +547,18 @@ mod tests { } #[test] - fn session_stats_record_reject_separates_the_three_rekey_arming_refusals() { + fn session_stats_record_reject_separates_the_four_rekey_arming_refusals() { let mut stats = SessionStats::default(); stats.record_reject(SessionReject::RekeyTiebreak); stats.record_reject(SessionReject::RekeyYielded); stats.record_reject(SessionReject::RekeyYielded); + stats.record_reject(SessionReject::RekeyHeld); + stats.record_reject(SessionReject::RekeyHeld); + stats.record_reject(SessionReject::RekeyHeld); stats.record_reject(SessionReject::RekeyPending); assert_eq!(stats.rekey_tiebreak, 1); assert_eq!(stats.rekey_yielded, 2); + assert_eq!(stats.rekey_held, 3); assert_eq!(stats.rekey_pending, 1); assert_eq!(stats.rekey_armed, 0); } @@ -554,11 +567,13 @@ mod tests { fn session_stats_snapshot_carries_the_rekey_arming_counters() { let mut stats = SessionStats::default(); stats.record_reject(SessionReject::RekeyTiebreak); + stats.record_reject(SessionReject::RekeyHeld); stats.rekey_armed = 7; stats.rekey_expired = 3; stats.pending_replaced = 2; let snap = stats.snapshot(); assert_eq!(snap.rekey_tiebreak, 1); + assert_eq!(snap.rekey_held, 1); assert_eq!(snap.rekey_armed, 7); assert_eq!(snap.rekey_expired, 3); assert_eq!(snap.pending_replaced, 2); diff --git a/src/node/tests/session.rs b/src/node/tests/session.rs index e484be83..aba0d69a 100644 --- a/src/node/tests/session.rs +++ b/src/node/tests/session.rs @@ -5511,8 +5511,15 @@ fn arm_stranger_handshake_beside_stale_pending( async fn test_forged_msg3_against_a_peer_armed_handshake_leaves_the_completed_epoch_intact() { let peer = Identity::generate(); let stranger = Identity::generate(); - let (mut node, peer_addr, _valid_msg3) = + let (mut node, peer_addr, valid_msg3) = install_stale_pending_beside_a_stranger_armed_handshake(&peer, &stranger); + // Backdate the stamp the handshake's deadline runs from, so a restore + // that restamped it to the current time could not match by accident. + let armed_at = wall_clock_ms() - 5_000; + node.sessions + .get_mut(&peer_addr) + .unwrap() + .record_peer_rekey(armed_at); // Garbage of the right length: `read_xk_message_3` fails on the AEAD. let forged = SessionMsg3::new(vec![0u8; crate::noise::XK_HANDSHAKE_MSG3_SIZE]).encode(); @@ -5520,19 +5527,44 @@ async fn test_forged_msg3_against_a_peer_armed_handshake_leaves_the_completed_ep .await; let entry = node.sessions.get(&peer_addr).expect("session present"); + assert_eq!( + entry.last_peer_rekey_ms(), + armed_at, + "the restore must not restamp the handshake's deadline, or a spray \ + would hold it open" + ); assert!( entry.pending_new_session().is_some(), "an unauthenticated msg3 must not discard the key epoch the peer may \ - already have cut over to; only the handshake it failed belongs to it" + already have cut over to" ); assert!( entry.is_established(), "the running session must be left intact alongside the pending one" ); assert!( - !entry.has_rekey_in_progress(), - "the handshake the msg3 failed against must still be abandoned" + entry.has_rekey_in_progress() && !entry.is_rekey_initiator(), + "an unreadable msg3 must not discard the handshake it failed against" ); + + // The handshake went back rolled back: the msg3 that genuinely finishes + // it still reads, and the key-mismatch arm, not the read, refuses it. + node.handle_session_payload( + &peer_addr, + &stub_link_peer(), + &SessionMsg3::new(valid_msg3).encode(), + 1280, + false, + ) + .await; + assert_eq!( + node.stats().session.rekey_key_mismatch, + 1, + "the kept handshake must still read the msg3 that finishes it" + ); + let entry = node.sessions.get(&peer_addr).expect("session present"); + assert!(entry.pending_new_session().is_some()); + assert!(!entry.has_rekey_in_progress()); } #[tokio::test] @@ -6345,6 +6377,75 @@ async fn test_forged_session_ack_leaves_the_initiation_able_to_complete_on_the_g cleanup_nodes(&mut nodes).await; } +/// A garbage msg3 that reaches the responder of an initial handshake after +/// the initiator has sent its genuine msg3 must not cost the genuine one the +/// half-open entry it completes. +#[tokio::test] +async fn test_forged_initial_msg3_leaves_the_responder_able_to_complete_on_the_genuine_msg3() { + let mut nodes = make_rekey_disabled_pair().await; + let node0_addr = *nodes[0].node.node_addr(); + let node1_addr = *nodes[1].node.node_addr(); + let node1_pubkey = nodes[1].node.identity().pubkey_full(); + + nodes[0] + .node + .initiate_session(node1_addr, node1_pubkey) + .await + .expect("initiate_session failed"); + tokio::time::sleep(Duration::from_millis(20)).await; + process_available_packets(&mut nodes[1..]).await; + let activity_before = nodes[1] + .node + .get_session(&node0_addr) + .filter(|e| e.is_awaiting_msg3()) + .expect("node 1 must be awaiting msg3 after answering the setup") + .last_activity(); + tokio::time::sleep(Duration::from_millis(20)).await; + process_available_packets(&mut nodes[..1]).await; + assert!( + nodes[0] + .node + .get_session(&node1_addr) + .is_some_and(|e| e.is_established()), + "node 0 must be established once it has sent msg3" + ); + + // Hold node 0's genuine msg3 and deliver the forgery first. + tokio::time::sleep(Duration::from_millis(20)).await; + let held: Vec<_> = std::iter::from_fn(|| nodes[1].packet_rx.try_recv().ok()).collect(); + assert!(!held.is_empty(), "node 0's msg3 must be queued at node 1"); + let forged = SessionMsg3::new(vec![0u8; crate::noise::XK_HANDSHAKE_MSG3_SIZE]).encode(); + nodes[1] + .node + .handle_session_payload(&node0_addr, &node0_addr, &forged, 1280, false) + .await; + let entry = nodes[1] + .node + .get_session(&node0_addr) + .expect("an unauthenticated msg3 must not destroy the half-open entry"); + assert!(entry.is_awaiting_msg3()); + assert_eq!( + entry.last_activity(), + activity_before, + "the reinsert must not push the handshake sweep's deadline out, or a \ + spray would keep a dead entry alive" + ); + + for packet in held { + nodes[1].node.handle_encrypted_frame(packet).await; + } + pump_until_quiet(&mut nodes).await; + assert!( + nodes[1] + .node + .get_session(&node0_addr) + .is_some_and(|e| e.is_established()), + "the genuine msg3 must still complete the session at node 1" + ); + + cleanup_nodes(&mut nodes).await; +} + // ============================================================================ // Integration tests: a lost initial msg3 // ============================================================================ @@ -7418,25 +7519,23 @@ async fn test_setup_naming_a_peer_whose_address_sorts_below_ours_yields_our_reke } #[tokio::test] -async fn test_losing_the_tiebreak_against_a_peer_armed_handshake_keeps_the_completed_epoch() { +async fn test_a_setup_against_a_peer_armed_handshake_is_dropped_and_keeps_the_handshake_and_the_completed_epoch() + { let mut config = Config::new(); config.node.rekey.enabled = false; let mut node = make_node_with(config); - // Our address sorts larger, so the second setup loses the tie-break. - // Which side of it a given pair lands on is fixed by the two addresses, - // not chosen by the sender, so this is half of all peers rather than - // something an attacker selects. + // Our address sorts larger, which is the end that used to yield: the + // tie-break would make us the responder to the second setup. let peer = peer_identity_sorting_below(node.node_addr()); let peer_addr = install_established_peer(&mut node, &peer); - // What a first forged setup leaves: a handshake the *stranger* armed, - // beside a completed epoch too stale for `pending_outranks` to veto. The - // tie-break arm gates on `has_rekey_in_progress`, which this satisfies, - // so a second forged setup reaches the yield with a pending session - // present. Nothing here required us to be the rekey initiator. + // What a first setup leaves: a handshake the sender armed, beside a + // completed epoch too stale for `pending_outranks` to veto, so only the + // armed handshake stands between a second setup and the arming path. let stranger = Identity::generate(); - arm_stranger_handshake_beside_stale_pending(&mut node, &peer_addr, &peer, &stranger); + let valid_msg3 = + arm_stranger_handshake_beside_stale_pending(&mut node, &peer_addr, &peer, &stranger); assert!( !node.sessions.get(&peer_addr).unwrap().is_rekey_initiator(), "the state under test is a handshake we did not arm" @@ -7446,26 +7545,37 @@ async fn test_losing_the_tiebreak_against_a_peer_armed_handshake_keeps_the_compl node.handle_session_payload(&peer_addr, &stub_link_peer(), &forged, 1280, false) .await; - let entry = node.sessions.get(&peer_addr).expect("session present"); + let stats = &node.stats().session; assert_eq!( - node.stats().session.rekey_yielded, - 1, - "the test must actually reach the yield arm, or it proves nothing" + stats.rekey_held, 1, + "a setup meeting a peer-armed handshake must be dropped and counted" + ); + assert_eq!( + (stats.rekey_yielded, stats.rekey_tiebreak, stats.rekey_armed), + (0, 0, 0), + "the tie-break is for two initiators; nothing may yield or arm here" + ); + let entry = node.sessions.get(&peer_addr).expect("session present"); + assert!( + entry.pending_new_session().is_some() && entry.is_established(), + "the completed epoch and the running session must be left intact" ); assert!( - entry.pending_new_session().is_some(), - "yielding a tie-break to an unauthenticated setup must not discard \ - the key epoch the peer may already have cut over to; two forged \ - setups would otherwise kill the reverse direction" - ); - assert!( - entry.is_established(), - "the running session must be left intact alongside the pending one" - ); - assert!( - !entry.has_rekey_in_progress(), - "the handshake we yielded must still be abandoned" + entry.has_rekey_in_progress() && !entry.is_rekey_initiator(), + "the handshake the peer armed must survive the setup, since a peer \ + that read our SessionAck needs it for its msg3" ); + + // The kept handshake is the same one: its own msg3 still reads. + node.handle_session_payload( + &peer_addr, + &stub_link_peer(), + &SessionMsg3::new(valid_msg3).encode(), + 1280, + false, + ) + .await; + assert_eq!(node.stats().session.rekey_key_mismatch, 1); } #[tokio::test] @@ -7952,19 +8062,19 @@ async fn test_a_rekey_whose_session_ack_was_lost_is_retired_after_the_handshake_ /// /// Both nodes would rekey after one message; the initiator is picked at run /// time so that the responder holds the smaller address when -/// `responder_wins`, and the larger otherwise. Only the initiator's tick is -/// run before the responder has armed, after which the responder is +/// `responder_smaller`, and the larger otherwise. Only the initiator's tick +/// is run before the responder has armed, after which the responder is /// dampened and cannot start a rekey of its own inside the test. /// -/// A responder that wins the tie-break drops the retry before arming -/// anything, so both handshakes expire and the next retry completes one -/// timeout later. A responder that loses yields and answers the retry at -/// once. -async fn lostack_retry(responder_wins: bool) { +/// A responder holding a handshake its peer armed drops the retry before +/// arming anything, at either address, since that handshake is the only one +/// a peer that read the SessionAck could finish. Both handshakes then +/// expire, and the next retry completes one timeout later. +async fn lostack_retry(responder_smaller: bool) { let mut nodes = rekey_pair([true, true], Some(1)).await; let node0_smaller = crate::proto::fsp::initiation_winner(nodes[0].node.node_addr(), nodes[1].node.node_addr()); - let resp = if responder_wins == node0_smaller { + let resp = if responder_smaller == node0_smaller { 0 } else { 1 @@ -8009,56 +8119,52 @@ async fn lostack_retry(responder_wins: bool) { tokio::time::sleep(Duration::from_millis(20)).await; process_available_packets(&mut nodes[resp..=resp]).await; let stats = &nodes[resp].node.stats().session; - let (tiebreak, yielded) = (stats.rekey_tiebreak, stats.rekey_yielded); assert_eq!( - tiebreak + yielded, - 1, + stats.rekey_held, 1, "the retry must have met the responder's stale handshake" ); - if responder_wins { - assert_eq!(tiebreak, 1, "the smaller responder must win"); - } else { - assert_eq!(yielded, 1, "the larger responder must yield"); - } + assert_eq!( + (stats.rekey_tiebreak, stats.rekey_yielded), + (0, 0), + "a handshake the peer armed is not a dual initiation at either address" + ); pump_all(&mut nodes).await; - if responder_wins { - assert!( - !holds_pending(&nodes[init], &resp_addr) && !holds_pending(&nodes[resp], &init_addr), - "a retry the responder dropped completes nothing" - ); - tokio::time::sleep(Duration::from_millis(1200)).await; - nodes[resp].node.check_session_rekey().await; - assert_eq!( - nodes[resp].node.stats().session.rekey_expired, - 1, - "the responder's stale handshake must expire on its own rule" - ); - nodes[init].node.check_session_rekey().await; - assert!( - !nodes[init] - .node - .get_session(&resp_addr) - .unwrap() - .has_rekey_in_progress(), - "the retry the responder dropped must itself be retired" - ); - nodes[init].node.check_session_rekey().await; - assert!( - rekey_initiated(&nodes[init], &resp_addr), - "the trigger must start a second retry" - ); - pump_all(&mut nodes).await; - } + assert!( + !holds_pending(&nodes[init], &resp_addr) && !holds_pending(&nodes[resp], &init_addr), + "a retry the responder dropped completes nothing" + ); + tokio::time::sleep(Duration::from_millis(1200)).await; + nodes[resp].node.check_session_rekey().await; + assert_eq!( + nodes[resp].node.stats().session.rekey_expired, + 1, + "the responder's stale handshake must expire on its own rule" + ); + nodes[init].node.check_session_rekey().await; + assert!( + !nodes[init] + .node + .get_session(&resp_addr) + .unwrap() + .has_rekey_in_progress(), + "the retry the responder dropped must itself be retired" + ); + nodes[init].node.check_session_rekey().await; + assert!( + rekey_initiated(&nodes[init], &resp_addr), + "the trigger must start a second retry" + ); + pump_all(&mut nodes).await; assert!( holds_pending(&nodes[init], &resp_addr) && holds_pending(&nodes[resp], &init_addr), "the retried rekey must complete on both nodes" ); - // The first handshake always; the retry too when the responder dropped it. + // The first handshake and the retry the responder dropped. assert_eq!( nodes[init].node.stats().session.rekey_unanswered, - if responder_wins { 2 } else { 1 }, + 2, "every retired handshake must be counted once" ); assert_eq!(nodes[resp].node.stats().session.rekey_unanswered, 0); @@ -8074,13 +8180,274 @@ async fn test_a_retry_dropped_by_a_smaller_responders_stale_handshake_completes_ lostack_retry(true).await; } -/// A retry that a larger responder, still holding its stale handshake, -/// yields to completes at once. +/// A retry dropped by a larger responder still holding its stale handshake +/// completes once both handshakes have expired, as at a smaller one: the +/// larger end no longer yields a handshake its peer armed. #[tokio::test] -async fn test_a_retry_that_a_larger_responder_yields_to_completes_at_once() { +async fn test_a_retry_dropped_by_a_larger_responders_stale_handshake_completes_one_timeout_later() { lostack_retry(false).await; } +// ============================================================================ +// Integration tests: a forgery inside a rekey responder's msg2-to-msg3 window +// ============================================================================ + +/// An unauthenticated message delivered to a rekey responder after the +/// initiator has read its SessionAck and before the initiator's msg3 arrives. +#[derive(Clone, Copy, Debug)] +enum WindowForgery { + /// Nothing is forged: the control the other two are measured against. + Nothing, + /// A SessionMsg3 of the right size whose first AEAD cannot open. + Msg3, + /// A SessionSetup carrying a stranger's ephemeral under the initiator's + /// address, delivered at the larger address, where the dual-initiation + /// tie-break would yield to it. + Setup, +} + +/// What a rekey left behind once a forgery may have reached its window. +#[derive(Debug, PartialEq, Eq)] +struct WindowOutcome { + /// The responder completed the handshake on the initiator's genuine msg3. + responder_completed: bool, + /// The initiator cut over on its liveness timer. + initiator_cut_over: bool, + /// A frame the initiator sent after its cutover and after its whole msg3 + /// resend budget decoded at the responder. + initiator_to_responder: bool, + /// A frame the responder sent decoded at the initiator. + responder_to_initiator: bool, + /// After the responder's own next rekey, frames decode both ways. + next_cycle_carries_both_ways: bool, +} + +/// The outcome of a rekey nothing interfered with. +const WINDOW_HEALTHY: WindowOutcome = WindowOutcome { + responder_completed: true, + initiator_cut_over: true, + initiator_to_responder: true, + responder_to_initiator: true, + next_cycle_carries_both_ways: true, +}; + +/// Send one data frame from `nodes[from]` to `nodes[to]`, deliver it, and +/// report whether it decoded at `nodes[to]`. +async fn session_frame_decodes(nodes: &mut [TestNode], from: usize, to: usize) -> bool { + let from_addr = *nodes[from].node.node_addr(); + let to_addr = *nodes[to].node.node_addr(); + let received = |nodes: &[TestNode]| { + nodes[to] + .node + .get_session(&from_addr) + .expect("the session must survive") + .traffic_counters() + .1 + }; + let before = received(nodes); + nodes[from] + .node + .send_session_data(&to_addr, 0, 0, b"window forgery probe") + .await + .expect("send_session_data failed"); + pump_all(nodes).await; + received(nodes) == before + 1 +} + +/// Cut `nodes[init]` over to the rekey it initiated by running its tick with +/// the liveness timer already elapsed, and report whether it did. +async fn cut_over_initiator(nodes: &mut [TestNode], init: usize, resp_addr: &NodeAddr) -> bool { + nodes[init] + .node + .sessions + .get_mut(resp_addr) + .unwrap() + .set_rekey_completed_ms(wall_clock_ms() - 10_000); + nodes[init].node.check_session_rekey().await; + let entry = nodes[init].node.get_session(resp_addr).unwrap(); + entry.pending_new_session().is_none() && !entry.has_rekey_in_progress() +} + +/// Drive a genuine FSP rekey to the point where the initiator has read the +/// SessionAck and derived the new session, deliver `forgery` to the +/// responder, then release the initiator's genuine msg3 and follow the cycle +/// through the initiator's cutover, its msg3 resend budget, and one data frame +/// each way. Then let the responder start the next rekey and check that it +/// carries frames both ways. +/// +/// The responder is chosen at run time to hold the larger address, the end +/// at which the dual-initiation tie-break yields; a smaller responder wins it +/// and drops the forged setup regardless. The forged msg3 reaches the same +/// arm at either end. +/// +/// Returns the outcome and a note of the responder's counters for the +/// failure message: msg3s refused for arriving with no handshake to read +/// them, and dual-initiation yields. +async fn rekey_with_window_forgery(forgery: WindowForgery) -> (WindowOutcome, String) { + use crate::transport::ReceivedPacket; + + let mut nodes = rekey_pair([true, true], None).await; + let node0_smaller = + crate::proto::fsp::initiation_winner(nodes[0].node.node_addr(), nodes[1].node.node_addr()); + let (init, resp) = if node0_smaller { (0, 1) } else { (1, 0) }; + let init_addr = *nodes[init].node.node_addr(); + let resp_addr = *nodes[resp].node.node_addr(); + assert!( + !crate::proto::fsp::initiation_winner(&resp_addr, &init_addr), + "the responder must hold the larger address" + ); + + // The setup reaches the responder only; it arms and answers. + start_rekey(&mut nodes, init).await; + tokio::time::sleep(Duration::from_millis(20)).await; + process_available_packets(&mut nodes[resp..=resp]).await; + assert_eq!( + nodes[resp].node.stats().session.rekey_armed, + 1, + "the responder must have armed" + ); + + // The SessionAck reaches the initiator only; it derives the new session + // and sends msg3, which waits in the responder's queue. + tokio::time::sleep(Duration::from_millis(20)).await; + process_available_packets(&mut nodes[init..=init]).await; + assert!( + holds_pending(&nodes[init], &resp_addr) + && !nodes[init] + .node + .get_session(&resp_addr) + .unwrap() + .has_rekey_in_progress(), + "the initiator must have read the SessionAck and hold the new session" + ); + assert!( + nodes[resp] + .node + .get_session(&init_addr) + .is_some_and(|e| e.has_rekey_in_progress() && !e.is_rekey_initiator()), + "the responder must still be waiting for msg3" + ); + tokio::time::sleep(Duration::from_millis(20)).await; + let held: Vec = + std::iter::from_fn(|| nodes[resp].packet_rx.try_recv().ok()).collect(); + assert!( + !held.is_empty(), + "the initiator's msg3 must be queued at the responder" + ); + + match forgery { + WindowForgery::Nothing => {} + WindowForgery::Msg3 => { + let forged = SessionMsg3::new(vec![0u8; crate::noise::XK_HANDSHAKE_MSG3_SIZE]).encode(); + nodes[resp] + .node + .handle_session_payload(&init_addr, &init_addr, &forged, 1280, false) + .await; + } + WindowForgery::Setup => { + let forged = forge_setup_for(&nodes[resp].node); + nodes[resp] + .node + .handle_session_payload(&init_addr, &init_addr, &forged, 1280, false) + .await; + assert_eq!( + nodes[resp].node.stats().session.rekey_held, + 1, + "the forged setup must have reached the armed handshake and \ + been held off, or the test measures nothing" + ); + } + } + + // Release the genuine msg3. + for packet in held { + nodes[resp].node.handle_encrypted_frame(packet).await; + } + pump_all(&mut nodes).await; + let responder_completed = holds_pending(&nodes[resp], &init_addr); + + // The initiator cuts over on its timer, then spends its msg3 resend + // budget: a resend is what recovers a msg3 that was merely lost. + let initiator_cut_over = cut_over_initiator(&mut nodes, init, &resp_addr).await; + let max_resends = nodes[init] + .node + .config() + .node + .rate_limit + .handshake_max_resends; + let base_ms = Node::now_ms(); + for step in 1..=u64::from(max_resends) + 1 { + nodes[init] + .node + .resend_pending_session_msg3(base_ms + step * 64_000) + .await; + pump_all(&mut nodes).await; + } + + let initiator_to_responder = session_frame_decodes(&mut nodes, init, resp).await; + let responder_to_initiator = session_frame_decodes(&mut nodes, resp, init).await; + + // The responder's own next rekey, once past the dampening that the + // initiator's setup started. Its data frame above crossed its trigger. + nodes[resp] + .node + .sessions + .get_mut(&init_addr) + .unwrap() + .record_peer_rekey(wall_clock_ms() - 60_000); + nodes[resp].node.check_session_rekey().await; + assert!( + rekey_initiated(&nodes[resp], &init_addr), + "the responder must start the next rekey" + ); + pump_all(&mut nodes).await; + let next_cycle_carries_both_ways = cut_over_initiator(&mut nodes, resp, &init_addr).await + && session_frame_decodes(&mut nodes, resp, init).await + && session_frame_decodes(&mut nodes, init, resp).await; + + let stats = &nodes[resp].node.stats().session; + let note = format!( + "the responder refused {} msg3s and yielded {} times", + stats.bad_state, stats.rekey_yielded + ); + cleanup_nodes(&mut nodes).await; + ( + WindowOutcome { + responder_completed, + initiator_cut_over, + initiator_to_responder, + responder_to_initiator, + next_cycle_carries_both_ways, + }, + note, + ) +} + +/// The control: with nothing forged, the procedure that measures the two +/// forgeries completes the rekey and carries frames both ways on both cycles. +#[tokio::test] +async fn a_rekey_with_nothing_forged_in_the_responders_window_completes_and_carries_traffic_both_ways() + { + let (outcome, note) = rekey_with_window_forgery(WindowForgery::Nothing).await; + assert_eq!(outcome, WINDOW_HEALTHY, "{note}"); +} + +/// A garbage msg3 that reaches a rekey responder after the initiator has read +/// the SessionAck must not cost the genuine msg3 its handshake. +#[tokio::test] +async fn a_forged_msg3_in_the_responders_window_does_not_split_the_rekey() { + let (outcome, note) = rekey_with_window_forgery(WindowForgery::Msg3).await; + assert_eq!(outcome, WINDOW_HEALTHY, "{note}"); +} + +/// A forged setup at a larger rekey responder, after the initiator has read +/// the SessionAck, must not cost the genuine msg3 its handshake. +#[tokio::test] +async fn a_forged_setup_at_the_larger_responder_in_its_window_does_not_split_the_rekey() { + let (outcome, note) = rekey_with_window_forgery(WindowForgery::Setup).await; + assert_eq!(outcome, WINDOW_HEALTHY, "{note}"); +} + /// A forged SessionAck arriving midway through an unanswered rekey must not /// restart its deadline: the rekey is retired on the timeout measured from /// the setup this node sent. diff --git a/src/noise/handshake.rs b/src/noise/handshake.rs index 51dcbd49..035d4b4a 100644 --- a/src/noise/handshake.rs +++ b/src/noise/handshake.rs @@ -15,9 +15,10 @@ use zeroize::{Zeroize, ZeroizeOnDrop}; /// /// Maintains the chaining key (ck), handshake hash (h), and current cipher. /// -/// `Clone` exists for [`HandshakeState::try_read_message_2`] and -/// [`HandshakeState::try_read_xk_message_2`], which have to put the pre-read -/// state back after a message that mixed material in before failing to +/// `Clone` exists for [`HandshakeState::try_read_message_2`], +/// [`HandshakeState::try_read_xk_message_2`] and +/// [`HandshakeState::try_read_xk_message_3`], which have to put the pre-read +/// state back after a message that advanced it before failing to /// authenticate. /// /// `ck` and `h` are cleared on drop, including on the clone above once it @@ -1018,6 +1019,40 @@ impl HandshakeState { Ok(()) } + /// Read XK message 3, leaving the handshake untouched when the message + /// does not authenticate. + /// + /// `read_xk_message_3` advances the symmetric state's nonce before the + /// first AEAD opens, and a message that fails later has already mixed a + /// DH result into the key, so a failed read leaves a handshake that can + /// never read the genuine msg3 afterwards. A responder that keeps its + /// handshake across a failed read, because the message may be a forgery + /// rather than the initiator's corrupt msg3, needs the pre-read state + /// back. + /// + /// The saved set is exactly what `read_xk_message_3` writes: + /// `symmetric`, `remote_static`, `remote_epoch` and `progress`. **That + /// mirror is manual.** A later edit that adds a write to + /// `read_xk_message_3` without adding it here silently reintroduces the + /// poisoning, and no caller can detect it. + pub fn try_read_xk_message_3(&mut self, message: &[u8]) -> Result<(), NoiseError> { + let symmetric = self.symmetric.clone(); + let remote_static = self.remote_static; + let remote_epoch = self.remote_epoch; + let progress = self.progress; + + match self.read_xk_message_3(message) { + Ok(()) => Ok(()), + Err(e) => { + self.symmetric = symmetric; + self.remote_static = remote_static; + self.remote_epoch = remote_epoch; + self.progress = progress; + Err(e) + } + } + } + /// Complete the handshake and return a NoiseSession. /// /// Must be called after the handshake is complete. diff --git a/src/noise/tests.rs b/src/noise/tests.rs index baabfc04..4c5e86b3 100644 --- a/src/noise/tests.rs +++ b/src/noise/tests.rs @@ -693,6 +693,67 @@ fn test_xk_wrong_state_errors() { ); } +/// Drive an XK handshake to the point where the responder is waiting for +/// msg3, and return the initiator's genuine msg3 with the responder. +fn xk_responder_awaiting_msg3() -> (HandshakeState, Vec) { + let initiator_keypair = generate_keypair(); + let responder_keypair = generate_keypair(); + let mut initiator = + HandshakeState::new_xk_initiator(initiator_keypair, responder_keypair.public_key()); + initiator.set_local_epoch(generate_epoch()); + let mut responder = HandshakeState::new_xk_responder(responder_keypair); + responder.set_local_epoch(generate_epoch()); + + let msg1 = initiator.write_xk_message_1().unwrap(); + responder.read_xk_message_1(&msg1).unwrap(); + let msg2 = responder.write_xk_message_2().unwrap(); + initiator.read_xk_message_2(&msg2).unwrap(); + let msg3 = initiator.write_xk_message_3().unwrap(); + (responder, msg3) +} + +#[test] +fn test_a_failed_rolling_back_xk_msg3_read_still_reads_the_genuine_msg3() { + let (mut responder, msg3) = xk_responder_awaiting_msg3(); + + // Fails at the first AEAD, after the nonce has advanced. + assert!( + responder + .try_read_xk_message_3(&[0u8; XK_HANDSHAKE_MSG3_SIZE]) + .is_err() + ); + // Fails at the epoch AEAD, after the static was learned and the se DH + // mixed into the key. + let mut tampered = msg3.clone(); + let last = tampered.len() - 1; + tampered[last] ^= 0x01; + assert!(responder.try_read_xk_message_3(&tampered).is_err()); + assert!( + responder.remote_static().is_none(), + "a failed read must not leave the static it decrypted behind" + ); + assert!(!responder.is_complete()); + + responder + .try_read_xk_message_3(&msg3) + .expect("the genuine msg3 must still read after two failed reads"); + assert!(responder.is_complete()); + assert!(responder.into_session().is_ok()); +} + +#[test] +fn test_a_failed_plain_xk_msg3_read_cannot_read_the_genuine_msg3() { + // The control for the rolling-back read: without the rollback, the + // failed read leaves a handshake the genuine msg3 no longer opens. + let (mut responder, msg3) = xk_responder_awaiting_msg3(); + assert!( + responder + .read_xk_message_3(&[0u8; XK_HANDSHAKE_MSG3_SIZE]) + .is_err() + ); + assert!(responder.read_xk_message_3(&msg3).is_err()); +} + #[test] fn test_xk_handshake_hash_differs_from_ik() { // XK and IK should produce different handshake hashes (different protocol names)