Keep a session responder's handshake through an unreadable XX msg3

Both msg3 arms of the session handler gave up their handshake when the
msg3 read failed, though nothing authenticates a msg3 before that read
beyond a source address the sender chooses. In the rekey responder arm,
a garbage msg3 delivered under the peer's address after the peer had
read this node's SessionAck abandoned the handshake the peer's genuine
msg3 needed; the peer cut over to keys this node never derived, its
msg3 resends found no handshake, and frames from it stopped decoding
until a later rekey. The initial-handshake arm removed the half-open
entry to read msg3 and did not put it back, so a garbage msg3 naming the
initiator, 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 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 and the half-open entry's activity stamp unchanged, so a spray
cannot hold a handshake open. The failures after a successful read keep
their abandons and drops: each follows a read only the initiator's
ephemeral key could have produced.

Tests drive a genuine rekey to the point where the initiator has read
the SessionAck, deliver a garbage msg3, and follow the genuine msg3, the
cutover, the msg3 resend budget and the next rekey cycle; a unit test
checks a forged msg3 leaves a stranger-armed handshake able to read the
msg3 that finishes it with its deadline unchanged; another holds an
initial msg3 back behind a forgery. Noise tests cover the rolling-back
read against a msg3 failing at either AEAD, with a control showing the
plain read cannot recover. No wire format change.

src/node/handlers/session.rs

@@ -1296,15 +1296,17 @@ 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_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 `read_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. An unreadable msg3 therefore
+        // costs nothing at all: the handshake goes back, rolled back, for the
+        // genuine msg3. Every later failure 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
@@ -1341,14 +1343,23 @@ impl Node {
                 (msg3.handshake_payload.as_slice(), None)
             };
 
-            // Process XX msg3
-            if let Err(e) = handshake.read_message_3(base_msg3) {
+            // Process XX 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_message_3(base_msg3) {
                 debug!(
                     src = %self.peer_display_name(src_addr),
                     error = %e,
-                    "Failed to process rekey XX msg3"
+                    "Failed to process rekey XX msg3, keeping the handshake"
                 );
-                entry.abandon_handshake();
+                entry.set_rekey_state(handshake, false);
                 self.sessions.insert(*src_addr, entry);
                 return;
             }
@@ -1461,9 +1472,22 @@ impl Node {
             (msg3.handshake_payload.as_slice(), None)
         };
 
-        // Process XX msg3 (learns initiator's identity and epoch)
-        if let Err(e) = handshake.read_message_3(base_msg3) {
-            debug!(error = %e, "Failed to process Noise XX msg3");
+        // Process XX msg3 (learns initiator's identity 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_message_3(base_msg3) {
+            debug!(error = %e, "Failed to process Noise XX msg3, keeping the handshake");
+            entry.set_state(EndToEndState::AwaitingMsg3(handshake));
+            self.sessions.insert(*src_addr, entry);
             return;
         }
 

src/node/tests/session.rs

@@ -4499,8 +4499,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_message_3` fails on the AEAD.
     let forged = SessionMsg3::new(vec![0u8; crate::noise::HANDSHAKE_MSG3_SIZE]).encode();
@@ -4508,19 +4515,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]
@@ -5601,6 +5633,75 @@ async fn test_a_session_ack_under_the_wrong_static_key_leaves_the_initiation_ali
     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::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
 // ============================================================================
@@ -7883,6 +7984,8 @@ async fn test_a_retry_dropped_by_a_larger_responders_stale_handshake_completes_o
 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.
@@ -7960,7 +8063,8 @@ async fn cut_over_initiator(nodes: &mut [TestNode], init: usize, resp_addr: &Nod
 ///
 /// 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.
+/// 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
@@ -8019,6 +8123,13 @@ async fn rekey_with_window_forgery(forgery: WindowForgery) -> (WindowOutcome, St
 
     match forgery {
         WindowForgery::Nothing => {}
+        WindowForgery::Msg3 => {
+            let forged = SessionMsg3::new(vec![0u8; crate::noise::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]
@@ -8107,6 +8218,14 @@ async fn a_rekey_with_nothing_forged_in_the_responders_window_completes_and_carr
     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]

src/noise/handshake.rs

@@ -14,9 +14,10 @@ use zeroize::{Zeroize, ZeroizeOnDrop};
 ///
 /// Maintains the chaining key (ck), handshake hash (h), and current cipher.
 ///
-/// `Clone` exists for [`HandshakeState::try_read_message_2`], which has to put
-/// the pre-read state back after a message that mixed material in before
-/// failing to authenticate.
+/// `Clone` exists for [`HandshakeState::try_read_message_2`] and
+/// [`HandshakeState::try_read_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
 /// goes out of scope. `cipher` is skipped because [`CipherState`] clears its
@@ -687,6 +688,40 @@ impl HandshakeState {
         Ok(())
     }
 
+    /// Read message 3, leaving the handshake untouched when the message does
+    /// not authenticate.
+    ///
+    /// `read_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_message_3` writes: `symmetric`,
+    /// `remote_static`, `remote_epoch` and `progress`. **That mirror is
+    /// manual.** A later edit that adds a write to `read_message_3` without
+    /// adding it here silently reintroduces the poisoning, and no caller can
+    /// detect it.
+    pub fn try_read_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_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)
+            }
+        }
+    }
+
     // ========================================================================
     // Payload Encryption (for negotiation payload in msg2/msg3)
     // ========================================================================

src/noise/tests.rs

@@ -597,3 +597,61 @@ fn test_session_replay_protection() {
     // Check method alone also detects replay
     assert!(receiver.check_replay(counter).is_err());
 }
+
+/// Drive an XX handshake to the point where the responder is waiting for
+/// msg3, and return the initiator's genuine msg3 with the responder.
+fn responder_awaiting_msg3() -> (HandshakeState, Vec<u8>) {
+    let mut initiator = HandshakeState::new_initiator(generate_keypair());
+    initiator.set_local_epoch(generate_epoch());
+    let mut responder = HandshakeState::new_responder(generate_keypair());
+    responder.set_local_epoch(generate_epoch());
+
+    let msg1 = initiator.write_message_1().unwrap();
+    responder.read_message_1(&msg1).unwrap();
+    let msg2 = responder.write_message_2().unwrap();
+    initiator.read_message_2(&msg2).unwrap();
+    let msg3 = initiator.write_message_3().unwrap();
+    (responder, msg3)
+}
+
+#[test]
+fn test_a_failed_rolling_back_msg3_read_still_reads_the_genuine_msg3() {
+    let (mut responder, msg3) = responder_awaiting_msg3();
+
+    // Fails at the first AEAD, after the nonce has advanced.
+    assert!(
+        responder
+            .try_read_message_3(&[0u8; 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_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_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_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) = responder_awaiting_msg3();
+    assert!(
+        responder
+            .read_message_3(&[0u8; HANDSHAKE_MSG3_SIZE])
+            .is_err()
+    );
+    assert!(responder.read_message_3(&msg3).is_err());
+}