From 032873e1cf5ec771fbdf6d8929c370d15ba51c33 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Fri, 2 Oct 2026 16:02:30 +0000 Subject: [PATCH] Keep a pending inbound handshake through an unreadable msg3 The link msg3 handler took the pending inbound entry out before reading msg3, and when the read failed it removed the leg's link, machine and session index. Nothing ties a msg3 to the leg before that read except the receiver index, which travels in cleartext in msg2's header, so anyone who saw msg2 could send garbage under it and discard the leg. On an initial handshake the initiator has already promoted on sending msg3, so its genuine msg3 then found no leg and the two ends stayed split until link-dead. On a rekey the responder never installed the new session, and the initiator's msg3 resends also found no leg. The responder now reads msg3 with the rolling-back read already used by the session layer, and a base msg3 that does not read leaves the leg as it was: the handshake goes back rolled back to its pre-read state, the pending entry goes back, and the leg's activity stamp is not touched, so a spray cannot extend its reap deadline. The failure is reported as its own error variant, logged at debug and counted as before. Failures after a successful read keep their teardown, since only the initiator's keys could have produced that read; a corrupt negotiation payload is still torn down, as its existing test shows. Tests deliver a forged msg3 (the genuine header over a zeroed base message) ahead of the genuine one, on an initial handshake and on a rekey, where the initiator's msg3 resend then completes the rekey on the kept leg. Both fail without the fix. No wire format change. --- src/node/handlers/handshake.rs | 19 +++- src/node/tests/handshake.rs | 174 ++++++++++++++++++++++++++++++++- src/peer/machine.rs | 39 +++++++- 3 files changed, 225 insertions(+), 7 deletions(-) diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index fabadc48..8523dc3f 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -13,7 +13,7 @@ use crate::node::rate_limit::Msg1Class; use crate::node::reject::{HandshakeReject, RejectReason}; use crate::node::{Node, NodeError}; use crate::peer::machine::{ - CrossConnOutcome, HandshakeCrypto, PeerAction, PeerEvent, PeerMachine, TimerKind, + CrossConnOutcome, HandshakeCrypto, Msg3Error, PeerAction, PeerEvent, PeerMachine, TimerKind, }; use crate::peer::{ActivePeer, RekeyMsg2Step}; use crate::proto::fmp::wire::{Msg1Header, Msg2Header, Msg3Header, build_msg2, build_msg3}; @@ -1452,7 +1452,22 @@ impl Node { let received_negotiation = match machine.complete_handshake_msg3(noise_msg3, packet.timestamp_ms) { Ok(neg) => neg, - Err(e) => { + Err(Msg3Error::Unreadable(e)) => { + // The leg is untouched; put its pending-inbound entry + // back so the genuine msg3, or the initiator's resend, + // still finds it. Debug, not warn: anyone who saw + // msg2's cleartext header can send these. + debug!( + link_id = %link_id, + error = %e, + "Unreadable msg3, keeping the pending leg" + ); + self.pending_inbound.insert(key, link_id); + self.stats_mut() + .record_reject(RejectReason::Handshake(HandshakeReject::BadState)); + return; + } + Err(e @ Msg3Error::Failed(_)) => { warn!( link_id = %link_id, error = %e, diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index 4e5ce71f..f3447743 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -2178,7 +2178,18 @@ async fn test_msg3_crypto_fail_disposes_leg_machine() { assert_eq!(responder.node.peer_machines.len(), 1, "msg1-born machine"); assert_eq!(responder.node.index_allocator.count(), 1); - // Corrupt the Noise payload so `complete_handshake_msg3` fails. + // Corrupt the last byte so `complete_handshake_msg3` fails. It lands in + // the negotiation payload, after a base read that succeeds, so this is + // the authenticated-but-bad msg3 that still tears the leg down; an + // unreadable base msg3 keeps it (see + // `an_unreadable_msg3_leaves_the_pending_leg_for_the_genuine_msg3`). + let offset = crate::proto::fmp::wire::Msg3Header::parse(&msg3.data) + .unwrap() + .noise_msg3_offset; + assert!( + msg3.data.len() - offset > crate::noise::HANDSHAKE_MSG3_SIZE, + "msg3 carries a negotiation payload after the base message" + ); let last = msg3.data.len() - 1; msg3.data[last] ^= 0xFF; responder.node.handle_msg3(msg3).await; @@ -5053,3 +5064,164 @@ async fn a_dampened_epoch_restart_leaves_the_peer_address_naming_the_peer_link() stop_hs(&mut initiator).await; stop_hs(&mut responder).await; } + +// =========================================================================== +// An unreadable msg3 must not cost the pending inbound leg. Nothing ties a +// msg3 to the leg before the read except the receiver index, which travels in +// cleartext in msg2's header, so anyone who saw msg2 can send garbage under +// it; the genuine msg3 (or its resend) must still find the leg. +// =========================================================================== + +/// `msg3` with its Noise part replaced by zeros of the base msg3 length, so +/// the read fails at its first AEAD. Flipping a trailing byte does not do +/// this: the flip lands in the negotiation payload, the read succeeds, and +/// the post-read teardown runs instead. The forgery is stamped later than +/// the genuine msg3, so a path that touched the leg on it would show. +fn unreadable_msg3(msg3: &ReceivedPacket) -> ReceivedPacket { + use crate::proto::fmp::wire::Msg3Header; + + let offset = Msg3Header::parse(&msg3.data) + .expect("genuine msg3 header") + .noise_msg3_offset; + let mut data = msg3.data[..offset].to_vec(); + data.extend(std::iter::repeat_n(0u8, crate::noise::HANDSHAKE_MSG3_SIZE)); + ReceivedPacket { + transport_id: msg3.transport_id, + remote_addr: msg3.remote_addr.clone(), + data, + timestamp_ms: msg3.timestamp_ms + 10_000, + } +} + +/// Assert `leg` is still a pending inbound leg awaiting msg3: its link, its +/// machine parked at `SentMsg2` with `index`, its `pending_inbound` entry and +/// its allocated index, with its activity stamp unchanged. +fn assert_leg_pending(node: &HsNode, leg: LinkId, index: SessionIndex, activity: u64) { + use crate::peer::machine::{HandshakePhase, PeerState}; + + assert!(node.node.links.contains_key(&leg), "the leg's link stays"); + let machine = node + .node + .peer_machines + .get(&leg) + .expect("the leg's machine stays"); + assert!(matches!( + machine.state(), + PeerState::Handshaking { + phase: HandshakePhase::SentMsg2, + .. + } + )); + assert_eq!(machine.our_index(), Some(index)); + assert_eq!( + node.node + .pending_inbound + .get(&(node.transport_id, index.as_u32())), + Some(&leg), + "the leg's pending-inbound entry goes back" + ); + assert!(node.node.index_allocator.is_allocated(index)); + assert_eq!( + node.node.connection_last_activity(leg), + activity, + "an unreadable msg3 must not extend the leg's reap deadline" + ); +} + +#[tokio::test] +async fn an_unreadable_msg3_leaves_the_pending_leg_for_the_genuine_msg3() { + let mut initiator = make_hs_node(Config::new()).await; + let mut responder = make_hs_node(Config::new()).await; + + let msg3 = drive_to_msg3(&mut initiator, &mut responder, 1000).await; + let leg = responder.node.connections().next().unwrap().1.link_id(); + let index = responder + .node + .peer_machines + .get(&leg) + .and_then(|m| m.our_index()) + .unwrap(); + let activity = responder.node.connection_last_activity(leg); + + responder.node.handle_msg3(unreadable_msg3(&msg3)).await; + assert_eq!(responder.node.peer_count(), 0); + assert_leg_pending(&responder, leg, index, activity); + #[cfg(debug_assertions)] + responder.node.debug_assert_peer_maps_coherent(); + + responder.node.handle_msg3(msg3).await; + let peer_addr = + *PeerIdentity::from_pubkey_full(initiator.node.identity().pubkey_full()).node_addr(); + let peer = responder + .node + .get_peer(&peer_addr) + .expect("the genuine msg3 must still promote the leg"); + assert_eq!(peer.link_id(), leg); + assert_eq!(peer.our_index(), Some(index)); + + stop_hs(&mut initiator).await; + stop_hs(&mut responder).await; +} + +#[tokio::test] +async fn an_unreadable_rekey_msg3_leaves_the_rekey_leg_for_the_resent_msg3() { + let mut initiator = make_hs_node(rekey_config()).await; + let mut responder = make_hs_node(rekey_config()).await; + let (peer_addr, _peer_link, leg) = park_rekey_leg(&mut initiator, &mut responder).await; + let index = responder + .node + .peer_machines + .get(&leg) + .and_then(|m| m.our_index()) + .unwrap(); + let activity = responder.node.connection_last_activity(leg); + + let msg2 = recv_phase(&mut initiator.packet_rx, 2, "rekey msg2").await; + initiator.node.handle_msg2(msg2).await; + let msg3 = recv_phase(&mut responder.packet_rx, 3, "rekey msg3").await; + + responder.node.handle_msg3(unreadable_msg3(&msg3)).await; + assert_leg_pending(&responder, leg, index, activity); + assert!( + responder + .node + .get_peer(&peer_addr) + .unwrap() + .pending_new_session() + .is_none() + ); + #[cfg(debug_assertions)] + responder.node.debug_assert_peer_maps_coherent(); + + // The initiator's own msg3 resend, not the held original, completes it. + drop(msg3); + let responder_addr = + *PeerIdentity::from_pubkey_full(responder.node.identity().pubkey_full()).node_addr(); + let resend_at = initiator + .node + .get_peer(&responder_addr) + .unwrap() + .rekey_msg3_next_resend_ms(); + initiator + .node + .resend_pending_fmp_rekey_msg3(resend_at) + .await; + let resent = recv_phase(&mut responder.packet_rx, 3, "resent rekey msg3").await; + responder.node.handle_msg3(resent).await; + assert!( + responder + .node + .get_peer(&peer_addr) + .unwrap() + .pending_new_session() + .is_some(), + "the resent msg3 must complete the rekey on the kept leg" + ); + assert!( + !responder.node.links.contains_key(&leg), + "the leg is consumed" + ); + + stop_hs(&mut initiator).await; + stop_hs(&mut responder).await; +} diff --git a/src/peer/machine.rs b/src/peer/machine.rs index fa4b2bf2..131b441e 100644 --- a/src/peer/machine.rs +++ b/src/peer/machine.rs @@ -96,6 +96,20 @@ use crate::utils::index::{IndexAllocator, SessionIndex}; use crate::{NodeAddr, PeerIdentity}; use secp256k1::Keypair; +/// Why [`PeerMachine::complete_handshake_msg3`] did not complete the leg. +#[derive(Debug, thiserror::Error)] +pub(crate) enum Msg3Error { + /// The base msg3 did not authenticate. The leg is unchanged: its + /// handshake is back, rolled back to its pre-read state, for the genuine + /// msg3. + #[error("unreadable msg3: {0}")] + Unreadable(NoiseError), + /// The leg had no handshake to read with, or a step after a successful + /// read failed. The leg's handshake is consumed. + #[error(transparent)] + Failed(#[from] NoiseError), +} + // ============================================================================ // Timing placeholders // @@ -955,11 +969,17 @@ impl PeerMachine { /// /// If the msg3 contains a negotiation payload (bytes beyond base XX msg3), /// it is decrypted and returned. + /// + /// A base msg3 that does not authenticate returns + /// [`Msg3Error::Unreadable`] and leaves the leg as it was: the handshake + /// goes back rolled back to its pre-read state and the carrier is not + /// touched, so the genuine msg3 can still complete the leg and a spray + /// cannot push out its reap deadline. pub(crate) fn complete_handshake_msg3( &mut self, message: &[u8], current_time_ms: u64, - ) -> Result>, NoiseError> { + ) -> Result>, Msg3Error> { let (received_negotiation, learned_identity, remote_epoch) = { let leg = self.leg.as_mut().ok_or_else(no_pending_connection)?; @@ -972,7 +992,8 @@ impl PeerMachine { return Err(NoiseError::WrongState { expected: "received_msg1 state".to_string(), got: "no active handshake".to_string(), - }); + } + .into()); } // Cleared as on the initiator's msg2 path: under XX the responder @@ -991,8 +1012,18 @@ impl PeerMachine { (message, None) }; - // Process XX msg3 (learns initiator identity + epoch) - hs.read_message_3(base_msg3)?; + // Process XX msg3 (learns initiator identity + epoch). Nothing + // ties this msg3 to the leg before the read except the receiver + // index, which travels in cleartext in msg2's header, so a msg3 + // that does not read proves nothing about the initiator. The + // handshake goes back, rolled back because the read advances the + // cipher nonce before it authenticates. Failures after a + // successful read keep their teardown: only the initiator's keys + // could have produced that read. + if let Err(e) = hs.try_read_message_3(base_msg3) { + leg.noise_handshake = Some(hs); + return Err(Msg3Error::Unreadable(e)); + } // Decrypt negotiation payload from msg3 if present let received_negotiation = if let Some(encrypted) = extra {