diff --git a/src/node/handlers/session.rs b/src/node/handlers/session.rs index 5c709d2f..2f218f10 100644 --- a/src/node/handlers/session.rs +++ b/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; } diff --git a/src/node/tests/session.rs b/src/node/tests/session.rs index 36e7b3eb..a46bcea4 100644 --- a/src/node/tests/session.rs +++ b/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] diff --git a/src/noise/handshake.rs b/src/noise/handshake.rs index adf1c9bb..33d7bd6a 100644 --- a/src/noise/handshake.rs +++ b/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) // ======================================================================== diff --git a/src/noise/tests.rs b/src/noise/tests.rs index 1ff3f02e..9b718b0a 100644 --- a/src/noise/tests.rs +++ b/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) { + 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()); +}