From 044d4673870f363772d4cd264bb9487c0e3dd5f2 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Thu, 1 Oct 2026 14:21:27 +0000 Subject: [PATCH] Keep a rekey responder's handshake through a forged msg3 or setup A session rekey responder that has sent its SessionAck holds the only handshake the initiator's msg3 can complete: by then the initiator has read the SessionAck and holds the new keys. Two unauthenticated messages could discard that handshake inside the window. The responder arm of the SessionMsg3 handler abandoned it when the msg3 read failed, though nothing authenticates a msg3 before that read beyond a source address the sender chooses. And a setup message naming the peer while the handshake was armed ran the dual-initiation tie-break, which at the larger address abandoned the handshake to answer the setup. Either way the genuine msg3 and its resends found no handshake, the initiator cut over to keys this node never derived, and frames from it stopped decoding until a later rekey completed. The initial-handshake arm had the same flaw: it removed the half-open entry to read msg3 and did not put it back when the read failed, so a garbage msg3 naming the initiator, arriving ahead of the genuine one, left the genuine msg3 to an unknown session. The Noise handshake gains a rolling-back msg3 read, shaped like the msg2 one, and both msg3 arms use it and put the handshake back on a failed read. The rollback matters because the read advances the cipher nonce before it opens the first AEAD, so a handshake put back after a plain read could never read the genuine msg3. The restore leaves the rekey deadline, which runs from the peer's setup, and the half-open entry's activity stamp unchanged, so a spray cannot hold a handshake open. The abandons and drops after a successful read stay: each follows a read that authenticated the sender. A setup message is now dropped, at either address, while a handshake the peer armed awaits its msg3, and counted as rekey_held; the tie-break runs only when this node initiated the armed handshake. The cost is that a genuine retry from a peer that abandoned the rekey this node answered completes one handshake timeout later, once the held handshake expires, as it already did at the smaller address. New tests drive a genuine rekey to the point where the initiator has read the SessionAck, deliver a garbage msg3 or, at the larger-address end, a forged setup, and follow the genuine msg3, the cutover, the msg3 resend budget and the next rekey cycle, asserting the forged setup was held off by the armed handshake. Unit tests check that a forged msg3 leaves a stranger-armed handshake able to read the msg3 that finishes it and a peer-armed one with its deadline unchanged, that a second setup leaves a peer-armed handshake and the completed epoch intact, and that a forged initial msg3 leaves the half-open entry and its activity stamp untouched. Noise tests cover the rolling-back read against a msg3 failing at either AEAD, with a control showing the plain read cannot recover. The retry tests for a responder holding a stale handshake assert rekey_held rather than the tie-break counters. No wire format change. --- src/node/handlers/session.rs | 104 ++++--- src/node/reject.rs | 5 + src/node/stats.rs | 17 +- src/node/tests/session.rs | 527 +++++++++++++++++++++++++++++------ src/noise/handshake.rs | 41 ++- src/noise/tests.rs | 61 ++++ 6 files changed, 636 insertions(+), 119 deletions(-) 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)