diff --git a/CHANGELOG.md b/CHANGELOG.md index 25304e74..6efb336f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -743,6 +743,18 @@ with v0.5.x or earlier peers. socket is adopted, after its bind, so the traversal bind still receives a port no other socket holds. +#### Link rekey + +- A forged rekey msg2 no longer ends the rekey cycle. The initiator matches + msg2 to the rekey by an index that rekey msg1 carries in cleartext, so anyone + on the path could answer first, either with a msg2 that does not authenticate + or with a valid one under another static key, and either one abandoned the + cycle, so rotation could be suppressed for as long as the forgeries + continued. The initiator now restores its handshake after such a msg2 and + keeps the cycle, and the peer's own msg2 completes the rekey. Each forgery + costs the initiator the msg2 key agreement until the cycle ends; the msg1 + resend budget bounds that. The wire format is unchanged. + #### Control socket - `show_links` (`fipsctl show links`) now reports the traffic a link has diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index d922e3ff..5d301e38 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -12,17 +12,16 @@ use crate::node::dataplane::PeerActionCtx; use crate::node::rate_limit::Msg1Class; use crate::node::reject::{HandshakeReject, RejectReason}; use crate::node::{Node, NodeError}; -use crate::peer::ActivePeer; use crate::peer::machine::{ CrossConnOutcome, HandshakeCrypto, PeerAction, PeerEvent, PeerMachine, TimerKind, }; +use crate::peer::{ActivePeer, RekeyMsg2Step}; use crate::proto::fmp::wire::{Msg1Header, Msg2Header, Msg3Header, build_msg2, build_msg3}; use crate::proto::fmp::{ DialMsg2Decision, DialMsg2Reject, DialMsg2Snapshot, Disconnect, DisconnectReason, EPOCH_RESTART_MIN_INTERVAL_SECS, EstablishSnapshot, InboundDecision, InboundReject, - NegotiationPayload, OutboundSnapshot, PromotionResult, RekeyClaim, RekeyMsg2Decision, - RekeyMsg2Reject, RekeyMsg2Snapshot, WireOutcome, cross_connection_winner, - decide_fmp_negotiation, + NegotiationPayload, OutboundSnapshot, PromotionResult, RekeyClaim, RekeyMsg2Reject, + RekeyMsg2Snapshot, WireOutcome, cross_connection_winner, decide_fmp_negotiation, }; use crate::transport::{Link, LinkDirection, LinkId, ReceivedPacket}; use crate::utils::index::SessionIndex; @@ -583,32 +582,33 @@ impl Node { let mut rekey_completed = false; let our_profile = self.node_profile(); + // Static-key continuity gate. The rekey msg2 was matched to this + // peer by the session index WE allocated, which travels in the + // cleartext rekey msg1 header and is observable on path; under + // XX the responder's static is learned from msg2 rather than + // pinned a priori, so crypto success alone does not prove the + // peer already holding this link is the one that answered. The + // core decides, inside complete_rekey_msg2 and before the + // handshake is consumed. A reject costs the established session + // nothing: its send/recv cipher state is never touched here and + // set_remote_epoch is confined to the install arm. It costs the + // rekey cycle nothing either: the handshake is restored to its + // pre-read state and the msg1 resend schedule is untouched, so + // the peer's genuine msg2 can still complete the cycle. + let fmp = &self.fmp; + let continuity = |learned_peer| { + fmp.rekey_outbound(&RekeyMsg2Snapshot { + established_peer: peer_node_addr, + learned_peer, + }) + }; if let Some(peer) = self.peers.get_mut(&peer_node_addr) { - match peer.complete_rekey_msg2(noise_msg2, our_profile) { - Ok((msg3_bytes, session, remote_epoch, learned_peer)) => { - // Static-key continuity gate. The rekey msg2 was - // matched to this peer by the session index WE - // allocated, which travels in the cleartext rekey - // msg1 header and is observable on path; under XX - // the responder's static is learned from msg2 - // rather than pinned a priori, so crypto success - // alone does not prove the peer already holding - // this link is the one that answered. The core - // decides. A Reject costs the established session - // nothing: its send/recv cipher state is never - // touched here and set_remote_epoch is confined to - // the Install arm, so the working session survives - // intact and usable. The rekey cycle, by contrast, - // is already gone — complete_rekey_msg2 above - // consumed the handshake state and cleared the - // msg1-resend fields — which is why the reject arm - // must abandon the cycle rather than retry it. - let continuity = self.fmp.rekey_outbound(&RekeyMsg2Snapshot { - established_peer: peer_node_addr, - learned_peer, - }); - match continuity { - RekeyMsg2Decision::Install => { + match peer.complete_rekey_msg2(noise_msg2, our_profile, continuity) { + Ok(step) => { + match step { + RekeyMsg2Step::Installed(completion) => { + let (msg3_bytes, session, remote_epoch, _learned_peer) = + *completion; let our_index = peer.rekey_our_index().unwrap_or(header.receiver_idx); // Detect a peer restart: the epoch carried in this @@ -716,34 +716,47 @@ impl Node { HandshakeReject::BadState, )); } + self.pending_outbound.remove(&key); } - RekeyMsg2Decision::Reject { + RekeyMsg2Step::Rejected { reason: RekeyMsg2Reject::StaticMismatch, + learned_peer, } => { - // Not our peer: the freshly derived session - // is never installed (it falls out of - // scope here), this rekey cycle is - // abandoned, and the current session, its + // Not our peer: no session was derived, no + // msg3 is sent, and the current session, its // indices and its recorded epoch are left - // exactly as they were. No msg3 is sent, - // so the impostor learns nothing beyond - // what it already observed on the wire. + // exactly as they were, so the impostor + // learns nothing beyond what it already + // observed on the wire. The rekey cycle and + // its dispatch entry stay for the peer's own + // msg2. warn!( peer = %display_name, established = %peer_node_addr, learned = %learned_peer, "rekey-msg2 initiator: learned static is not the established peer, keeping current session" ); - if let Some(idx) = peer.abandon_rekey() { - if let Some(tid) = peer.transport_id() { - self.peers_by_index.remove(&(tid, idx.as_u32())); - } - let _ = self.index_allocator.free(idx); - } self.stats_mut().record_reject(RejectReason::Handshake( HandshakeReject::RekeyStaticMismatch, )); } + RekeyMsg2Step::Unreadable(e) => { + // The msg2 may be a forgery naming our + // cleartext rekey index. The handshake was + // restored, so keep the cycle and its + // dispatch entry for the genuine msg2. If + // none arrives, the msg1 resend budget + // abandons the cycle as it would for a lost + // one. + debug!( + peer = %display_name, + error = %e, + "Rekey msg2 did not authenticate, keeping the rekey cycle" + ); + self.stats_mut().record_reject(RejectReason::Handshake( + HandshakeReject::BadState, + )); + } } } Err(e) => { @@ -760,20 +773,23 @@ impl Node { } self.stats_mut() .record_reject(RejectReason::Handshake(HandshakeReject::BadState)); + self.pending_outbound.remove(&key); } } + } else { + self.pending_outbound.remove(&key); } // Feed the control machine the completed-rekey observation so its - // shadow index and rekey phase stay coherent. Only on success — - // the failure path above reverts the rekey and leaves the machine + // shadow index and rekey phase stay coherent. Only on an install + // with its msg3 sent — every other path above either keeps the + // cycle as it was or abandons it, and leaves the machine // untouched. The crypto effect already ran inline; this emits no // action. if rekey_completed { self.observe_rekey_msg2(&peer_node_addr, header.sender_idx); } - self.pending_outbound.remove(&key); return; } diff --git a/src/node/reject.rs b/src/node/reject.rs index 89287a55..2bd46dbd 100644 --- a/src/node/reject.rs +++ b/src/node/reject.rs @@ -210,11 +210,11 @@ pub enum HandshakeReject { /// Initiator-side rekey msg2 completed the Noise read, but the static /// key it revealed is not the key the established session is bound to /// — the msg2 did not come from the peer we are rekeying with. The - /// rekey cycle is abandoned and the working session is left untouched. - /// Unlike the rest of the cluster this arm cannot be reached by - /// routine handshake noise: the rekey msg1 header travels in the - /// clear, so a sustained rate here means an on-path attacker is - /// forging rekey msg2 and suppressing key rotation. Tracked via + /// msg2 is dropped, the rekey cycle is kept for the peer's own msg2, and + /// the working session is left untouched. Unlike the rest of the cluster + /// this arm cannot be reached by routine handshake noise: the rekey msg1 + /// header travels in the clear, so a sustained rate here means an on-path + /// attacker is forging rekey msg2. Tracked via /// [`HandshakeStats::rekey_static_mismatch`](crate::node::stats::HandshakeStats). RekeyStaticMismatch, } diff --git a/src/node/stats.rs b/src/node/stats.rs index 5fc4c702..b39903b3 100644 --- a/src/node/stats.rs +++ b/src/node/stats.rs @@ -157,9 +157,9 @@ pub struct HandshakeStats { pub unknown_connection: u64, /// Initiator-side rekey msg2 read cleanly but revealed a static key /// other than the one the established session is bound to, so the - /// rekey cycle was abandoned and the working session kept. A - /// sustained rate here is an on-path attacker forging rekey msg2 and - /// suppressing key rotation, not routine handshake noise. + /// msg2 was dropped, and the rekey cycle and the working session were + /// kept. A sustained rate here is an on-path attacker forging rekey + /// msg2, not routine handshake noise. pub rekey_static_mismatch: u64, } diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index 9814785e..473010ef 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -2951,8 +2951,9 @@ async fn test_anonymous_self_connect_drop_disposes_machine() { /// Establish initiator↔responder, start a real rekey on the initiator, then /// let a third node answer the rekey msg1 with a valid XX msg2 built from its -/// OWN static. The initiator must reject it and keep the established session -/// live and usable. +/// OWN static. The initiator must reject it, keep the established session live +/// and usable, and keep the rekey cycle so the real responder's msg2, arriving +/// second, still completes it. #[tokio::test] async fn test_rekey_msg2_foreign_static_rejected() { let mut rekey_config = Config::new(); @@ -3009,9 +3010,10 @@ async fn test_rekey_msg2_foreign_static_rejected() { ); // The attacker observes the cleartext rekey msg1 on path and answers it - // first, under its own static. The real responder never sees it. + // first, under its own static. The real responder gets the same msg1 and + // answers it second. let rekey_msg1 = recv_phase(&mut responder.packet_rx, 1, "rekey msg1").await; - attacker.node.handle_msg1(rekey_msg1).await; + attacker.node.handle_msg1(rekey_msg1.clone()).await; let forged_msg2 = recv_phase(&mut initiator.packet_rx, 2, "forged rekey msg2").await; initiator.node.handle_msg2(forged_msg2).await; @@ -3026,10 +3028,7 @@ async fn test_rekey_msg2_foreign_static_rejected() { peer.pending_new_session().is_none(), "a foreign static must not be installed as the pending session" ); - assert!( - !peer.rekey_in_progress(), - "the rejected rekey cycle is abandoned" - ); + assert!(peer.rekey_in_progress(), "the rejected rekey cycle is kept"); assert_eq!(peer.link_id(), peer_link, "the peer keeps its link"); // The established session is byte-for-byte the one we started with, still @@ -3045,19 +3044,40 @@ async fn test_rekey_msg2_foreign_static_rejected() { "the established session stays bound to the real peer" ); - // The rekey index is returned and its msg2 dispatch entry is gone, so a - // late (or replayed) msg2 on that index cannot re-enter the dead cycle. + // The rekey keeps its index and its msg2 dispatch entry, so the real + // responder's msg2 can still reach the cycle. assert_eq!( initiator.node.index_allocator.count(), - baseline, - "the rejected rekey must free its index" + baseline + 1, + "the kept rekey cycle keeps its index" + ); + assert!( + initiator + .node + .pending_outbound + .contains_key(&(initiator.transport_id, rekey_index.as_u32())), + "the kept rekey cycle keeps its dispatch entry" + ); + initiator.node.debug_assert_peer_maps_coherent(); + + // The real responder's msg2 completes the kept cycle. + responder.node.handle_msg1(rekey_msg1).await; + let genuine_msg2 = recv_phase(&mut initiator.packet_rx, 2, "genuine rekey msg2").await; + initiator.node.handle_msg2(genuine_msg2).await; + let peer = initiator.node.get_peer(&responder_addr).expect("kept"); + assert_eq!( + peer.pending_new_session() + .expect("the genuine msg2 installs the pending session") + .remote_static_xonly(), + responder.node.identity().pubkey(), + "the pending session is bound to the real peer" ); assert!( !initiator .node .pending_outbound .contains_key(&(initiator.transport_id, rekey_index.as_u32())), - "the rejected rekey's dispatch entry must not survive" + "the completed rekey's dispatch entry is gone" ); initiator.node.debug_assert_peer_maps_coherent(); @@ -3160,12 +3180,12 @@ async fn test_forged_rekey_msg2_is_counted_as_rekey_static_mismatch_not_bad_stat // dropped earlier for some unrelated reason. assert_eq!(initiator.node.peer_count(), 1, "peer set unchanged"); assert!( - !initiator + initiator .node .get_peer(&responder_addr) .unwrap() .rekey_in_progress(), - "the rejected rekey cycle is abandoned" + "the rejected rekey cycle is kept" ); assert_eq!( @@ -3184,6 +3204,211 @@ async fn test_forged_rekey_msg2_is_counted_as_rekey_static_mismatch_not_bad_stat stop_hs(&mut attacker).await; } +/// A rekey msg2 that names the live rekey index but does not authenticate +/// leaves the rekey cycle in place, and the responder's genuine msg2 then +/// completes it on both ends. +/// +/// Nothing authenticates a msg2 before the handshake reads it, and the index it +/// names travels in cleartext in the rekey msg1, so anyone on the path can send +/// one first. The forgery here carries a valid curve point as its ephemeral, so +/// the read gets as far as mixing it into the handshake before failing. +#[tokio::test] +async fn test_rekey_msg2_that_fails_to_authenticate_keeps_the_cycle() { + use crate::noise::HANDSHAKE_MSG2_SIZE; + use crate::proto::fmp::wire::build_msg2; + + let mut rekey_config = Config::new(); + rekey_config.node.rekey.enabled = true; + rekey_config.node.rekey.after_secs = 30; + + let mut initiator = make_hs_node(rekey_config).await; + let mut responder = make_hs_node(Config::new()).await; + + let responder_addr = + *PeerIdentity::from_pubkey_full(responder.node.identity().pubkey_full()).node_addr(); + let initiator_addr = + *PeerIdentity::from_pubkey_full(initiator.node.identity().pubkey_full()).node_addr(); + + let msg3 = drive_to_msg3(&mut initiator, &mut responder, 1000).await; + responder.node.handle_msg3(msg3).await; + assert_eq!(initiator.node.peer_count(), 1); + assert_eq!( + initiator.node.stats().handshake.bad_state, + 0, + "the clean handshake charges no catch-all reject" + ); + + initiator + .node + .get_peer_mut(&responder_addr) + .unwrap() + .test_backdate_session_established(std::time::Duration::from_secs(120)); + initiator.node.check_rekey().await; + let rekey_index = initiator + .node + .get_peer(&responder_addr) + .unwrap() + .rekey_our_index() + .expect("cadence started a rekey"); + + // The responder answers the rekey msg1; its msg2 is held back. + let rekey_msg1 = recv_phase(&mut responder.packet_rx, 1, "rekey msg1").await; + responder.node.handle_msg1(rekey_msg1).await; + let genuine_msg2 = recv_phase(&mut initiator.packet_rx, 2, "genuine rekey msg2").await; + + // The forgery arrives first, from the responder's address, as a spoofed + // source would. + let mut forged_noise = Identity::generate().pubkey_full().serialize().to_vec(); + forged_noise.resize(HANDSHAKE_MSG2_SIZE, 0xA5); + let forged = ReceivedPacket::new( + initiator.transport_id, + responder.addr.clone(), + build_msg2(SessionIndex::new(0x5EED_F00D), rekey_index, &forged_noise), + ); + initiator.node.handle_msg2(forged).await; + + assert_eq!( + initiator.node.stats().handshake.bad_state, + 1, + "the forged msg2 reached the rekey read and was charged once" + ); + let peer = initiator.node.get_peer(&responder_addr).unwrap(); + assert!( + peer.rekey_in_progress(), + "a msg2 that fails to authenticate keeps the rekey cycle" + ); + assert!(peer.pending_new_session().is_none()); + assert!( + initiator + .node + .pending_outbound + .contains_key(&(initiator.transport_id, rekey_index.as_u32())), + "the kept rekey cycle keeps its dispatch entry" + ); + + // The genuine msg2 completes the cycle, and its msg3 reaches the responder. + initiator.node.handle_msg2(genuine_msg2).await; + let rekey_msg3 = recv_phase(&mut responder.packet_rx, 3, "rekey msg3").await; + responder.node.handle_msg3(rekey_msg3).await; + + // Both ends hold a pending session, and each end's indices are the other's + // crossed: an initiator that lost the cycle, or completed it against a + // poisoned handshake, cannot pair with the responder this way. + let ours = initiator.node.get_peer(&responder_addr).unwrap(); + let theirs = responder.node.get_peer(&initiator_addr).unwrap(); + assert!(ours.pending_new_session().is_some(), "initiator installed"); + assert!( + theirs.pending_new_session().is_some(), + "responder installed" + ); + assert_eq!( + (ours.pending_our_index(), ours.pending_their_index()), + (theirs.pending_their_index(), theirs.pending_our_index()), + "the two ends of the rekey pair up" + ); + initiator.node.debug_assert_peer_maps_coherent(); + + stop_hs(&mut initiator).await; + stop_hs(&mut responder).await; +} + +/// A rekey msg2 whose base message reads but whose negotiation payload does not +/// decrypt leaves the rekey cycle in place, and the genuine msg2 still installs. +/// +/// The base read succeeding is what separates this from a msg2 that fails to +/// authenticate outright: the handshake has already taken the sender's +/// ephemeral and static when the payload fails, so it has to be put back. +#[tokio::test] +async fn test_rekey_msg2_with_a_corrupt_negotiation_payload_keeps_the_cycle() { + use crate::noise::HANDSHAKE_MSG2_SIZE; + use crate::proto::fmp::wire::Msg2Header; + + let mut rekey_config = Config::new(); + rekey_config.node.rekey.enabled = true; + rekey_config.node.rekey.after_secs = 30; + + let mut initiator = make_hs_node(rekey_config).await; + let mut responder = make_hs_node(Config::new()).await; + let mut attacker = make_hs_node(Config::new()).await; + + let responder_addr = + *PeerIdentity::from_pubkey_full(responder.node.identity().pubkey_full()).node_addr(); + + let msg3 = drive_to_msg3(&mut initiator, &mut responder, 1000).await; + responder.node.handle_msg3(msg3).await; + assert_eq!(initiator.node.peer_count(), 1); + assert_eq!(initiator.node.stats().handshake.bad_state, 0); + + initiator + .node + .get_peer_mut(&responder_addr) + .unwrap() + .test_backdate_session_established(std::time::Duration::from_secs(120)); + initiator.node.check_rekey().await; + let rekey_index = initiator + .node + .get_peer(&responder_addr) + .unwrap() + .rekey_our_index() + .expect("cadence started a rekey"); + + // The attacker answers the rekey msg1 with a valid msg2 of its own, and one + // byte of the trailing negotiation payload is flipped on the way. + let rekey_msg1 = recv_phase(&mut responder.packet_rx, 1, "rekey msg1").await; + attacker.node.handle_msg1(rekey_msg1.clone()).await; + let mut corrupt_msg2 = recv_phase(&mut initiator.packet_rx, 2, "attacker rekey msg2").await; + let header = Msg2Header::parse(&corrupt_msg2.data).expect("msg2 header"); + assert!( + corrupt_msg2.data.len() > header.noise_msg2_offset + HANDSHAKE_MSG2_SIZE, + "the msg2 must carry a negotiation payload past the base message" + ); + let last = corrupt_msg2.data.len() - 1; + corrupt_msg2.data[last] ^= 0x01; + initiator.node.handle_msg2(corrupt_msg2).await; + + assert_eq!( + initiator.node.stats().handshake.bad_state, + 1, + "the corrupt msg2 reached the rekey read and was charged once" + ); + assert_eq!( + initiator.node.stats().handshake.rekey_static_mismatch, + 0, + "the payload failed before the static-key gate" + ); + let peer = initiator.node.get_peer(&responder_addr).unwrap(); + assert!( + peer.rekey_in_progress(), + "a msg2 whose payload fails to decrypt keeps the rekey cycle" + ); + assert!(peer.pending_new_session().is_none()); + assert!( + initiator + .node + .pending_outbound + .contains_key(&(initiator.transport_id, rekey_index.as_u32())), + "the kept rekey cycle keeps its dispatch entry" + ); + + // The real responder's msg2 installs. + responder.node.handle_msg1(rekey_msg1).await; + let genuine_msg2 = recv_phase(&mut initiator.packet_rx, 2, "genuine rekey msg2").await; + initiator.node.handle_msg2(genuine_msg2).await; + let peer = initiator.node.get_peer(&responder_addr).unwrap(); + assert_eq!( + peer.pending_new_session() + .expect("the genuine msg2 installs the pending session") + .remote_static_xonly(), + responder.node.identity().pubkey(), + "the pending session is bound to the real peer" + ); + initiator.node.debug_assert_peer_maps_coherent(); + + stop_hs(&mut initiator).await; + stop_hs(&mut responder).await; + stop_hs(&mut attacker).await; +} + /// The same cadence-driven rekey, answered by the REAL peer, still installs the /// pending session — the gate must be invisible on the legitimate path. #[tokio::test] diff --git a/src/peer/active.rs b/src/peer/active.rs index 090e2b97..20dd0fef 100644 --- a/src/peer/active.rs +++ b/src/peer/active.rs @@ -7,7 +7,7 @@ use crate::config::MmpConfig; use crate::node::REKEY_JITTER_SECS; use crate::noise::{HandshakeState as NoiseHandshakeState, NoiseError, NoiseSession}; use crate::proto::bloom::BloomFilter; -use crate::proto::fmp::{NegotiationPayload, NodeProfile}; +use crate::proto::fmp::{NegotiationPayload, NodeProfile, RekeyMsg2Decision, RekeyMsg2Reject}; use crate::proto::mmp::MmpPeerState; use crate::proto::stp::{ParentDeclaration, TreeCoordinate}; use crate::transport::{LinkId, LinkStats, TransportAddr, TransportId}; @@ -30,6 +30,26 @@ use std::time::Instant; /// link. type RekeyMsg2Completion = (Vec, NoiseSession, Option<[u8; 8]>, NodeAddr); +/// How an initiator-side rekey msg2 ended, when the rekey cycle can continue. +/// +/// Only [`Installed`](Self::Installed) consumes the rekey handshake. The other +/// two leave it exactly as it was before the msg2 was read, so the peer's own +/// msg2 can still complete the cycle. +#[derive(Debug)] +pub(crate) enum RekeyMsg2Step { + /// The static-key gate allowed the install: msg3 is written and the + /// handshake has become a session. + Installed(Box), + /// The msg2 read and its static-key gate rejected it. `learned_peer` is the + /// address its static derives to. + Rejected { + reason: RekeyMsg2Reject, + learned_peer: NodeAddr, + }, + /// The msg2 did not read or its negotiation payload did not decrypt. + Unreadable(NoiseError), +} + /// Draw a fresh per-session rekey jitter from `[-REKEY_JITTER_SECS, +REKEY_JITTER_SECS]`. fn draw_rekey_jitter() -> i64 { rand::rng().random_range(-REKEY_JITTER_SECS..=REKEY_JITTER_SECS) @@ -1300,29 +1320,39 @@ impl ActivePeer { /// Complete the rekey by processing msg2 (initiator side, XX pattern). /// - /// Takes the stored handshake state, reads XX msg2, generates XX msg3, and - /// returns (msg3_bytes, completed NoiseSession, remote startup epoch, - /// learned peer node address). Clears the handshake-related fields but - /// leaves rekey_our_index for set_pending_session to use. The remote epoch - /// is surfaced so the caller can detect a peer restart (changed epoch) - /// during recovery rekey; the learned node address is surfaced so the - /// caller can gate the install on static-key continuity. + /// Reads XX msg2 against the stored handshake state, decrypts its + /// negotiation payload, and asks `decide` whether the static key it learned + /// may replace this peer's session. Only on + /// [`Install`](RekeyMsg2Decision::Install) is the handshake taken: msg3 is + /// written, the session is completed, and the msg1 resend fields are + /// cleared, leaving rekey_our_index for set_pending_session to use. The + /// remote epoch is surfaced so the caller can detect a peer restart + /// (changed epoch) during recovery rekey. /// - /// Completing the handshake here is deliberately identity-agnostic: this - /// is the crypto leaf, and whether the learned identity may replace the - /// peer's session is a decision, taken by the caller against the FMP core. - pub fn complete_rekey_msg2( + /// Nothing authenticates a msg2 before this read, and the index it names + /// travels in cleartext in the rekey msg1, so the message may be a forgery. + /// A msg2 that does not read, whose payload does not decrypt, or that the + /// gate rejects puts the handshake back in its pre-read state and returns + /// [`Unreadable`](RekeyMsg2Step::Unreadable) or + /// [`Rejected`](RekeyMsg2Step::Rejected), with the msg1 resend schedule + /// untouched. `Err` means the cycle cannot continue: there was no rekey + /// handshake, or completing an allowed install failed after the handshake + /// was taken. + /// + /// The peer does not decide whether the learned identity may replace its + /// session; the caller supplies that decision, taken against the FMP core, + /// and this runs it before anything is consumed. + pub(crate) fn complete_rekey_msg2( &mut self, msg2_bytes: &[u8], our_profile: NodeProfile, - ) -> Result { - let mut hs = self - .rekey_handshake - .take() - .ok_or_else(|| NoiseError::WrongState { - expected: "rekey handshake in progress".to_string(), - got: "no handshake state".to_string(), - })?; + decide: impl FnOnce(NodeAddr) -> RekeyMsg2Decision, + ) -> Result { + let no_handshake = || NoiseError::WrongState { + expected: "rekey handshake in progress".to_string(), + got: "no handshake state".to_string(), + }; + let hs = self.rekey_handshake.as_mut().ok_or_else(no_handshake)?; // Split msg2 into base XX part and any extra (negotiation payload) let base_size = crate::noise::HANDSHAKE_MSG2_SIZE; @@ -1332,18 +1362,44 @@ impl ActivePeer { (msg2_bytes, None) }; - hs.read_message_2(base_msg2)?; + // A failed read restores the handshake itself. + let rollback = match hs.try_read_message_2(base_msg2) { + Ok(rollback) => rollback, + Err(e) => return Ok(RekeyMsg2Step::Unreadable(e)), + }; + + // Must decrypt negotiation payload (if present) to keep hash chain + // in sync, even though rekey doesn't use the negotiation result. + if let Some(encrypted_neg) = extra + && let Err(e) = hs.decrypt_payload(encrypted_neg) + { + hs.restore_message_2(rollback); + return Ok(RekeyMsg2Step::Unreadable(e)); + } + + let Some(remote_static) = hs.remote_static() else { + hs.restore_message_2(rollback); + return Ok(RekeyMsg2Step::Unreadable(NoiseError::WrongState { + expected: "remote static learned from msg2".to_string(), + got: "no remote static".to_string(), + })); + }; + let learned_peer = NodeAddr::from_pubkey(&remote_static.x_only_public_key().0); + + if let RekeyMsg2Decision::Reject { reason } = decide(learned_peer) { + hs.restore_message_2(rollback); + return Ok(RekeyMsg2Step::Rejected { + reason, + learned_peer, + }); + } + + let mut hs = self.rekey_handshake.take().ok_or_else(no_handshake)?; // The remote static identity (and its startup epoch) is available once // msg2 has been read; capture it for peer-restart detection. let remote_epoch = hs.remote_epoch(); - // Must decrypt negotiation payload (if present) to keep hash chain - // in sync, even though rekey doesn't use the negotiation result. - if let Some(encrypted_neg) = extra { - let _ = hs.decrypt_payload(encrypted_neg)?; - } - // Declare this handshake a rekey of the session already installed, naming // the index the RESPONDER receives on (our `their_index`) so it can match // the marker against its own `our_index`. Without it the responder cannot @@ -1377,17 +1433,26 @@ impl ActivePeer { } let session = hs.into_session()?; - // Derive the learned identity from the session rather than the consumed - // handshake so the address returned is provably the one the session - // about to be installed is bound to. - let learned_peer = NodeAddr::from_pubkey(&session.remote_static_xonly()); + // The gate read the static from the handshake; the session copies the + // same key, so the address the gate allowed is the one the session about + // to be installed is bound to. + debug_assert_eq!( + NodeAddr::from_pubkey(&session.remote_static_xonly()), + learned_peer, + "the installed session is bound to the identity the gate allowed" + ); // Clear msg1 resend state self.rekey_msg1 = None; self.rekey_msg1_next_resend = 0; self.rekey_msg1_resend_count = 0; - Ok((msg3, session, remote_epoch, learned_peer)) + Ok(RekeyMsg2Step::Installed(Box::new(( + msg3, + session, + remote_epoch, + learned_peer, + )))) } /// Complete the rekey by processing msg3 (responder side, XX pattern). diff --git a/src/peer/mod.rs b/src/peer/mod.rs index bd186174..fda1bc8a 100644 --- a/src/peer/mod.rs +++ b/src/peer/mod.rs @@ -8,6 +8,7 @@ mod active; pub(crate) mod machine; +pub(crate) use active::RekeyMsg2Step; pub use active::{ActivePeer, ConnectivityState}; use crate::NodeAddr; diff --git a/src/proto/fmp/core.rs b/src/proto/fmp/core.rs index 8ca5db61..893199dd 100644 --- a/src/proto/fmp/core.rs +++ b/src/proto/fmp/core.rs @@ -557,9 +557,9 @@ pub(crate) enum RekeyMsg2Decision { /// cutover, the unchanged pre-existing behaviour. Install, /// The rekey was answered by some other identity: drop this `msg2` with a - /// handshake reject, abandon the rekey cycle, and leave the established - /// session completely undisturbed. `reason` selects only the diagnostic - /// log line. + /// handshake reject, keep the rekey cycle so the established peer's own + /// `msg2` can still complete it, and leave the established session + /// completely undisturbed. `reason` selects only the diagnostic log line. Reject { reason: RekeyMsg2Reject }, } @@ -939,8 +939,8 @@ impl Fmp { /// A rekey may only replace the session of the peer that already holds the /// link, so the identity learned from `msg2` must derive to the established /// peer's node address. Anything else is - /// [`Reject`](RekeyMsg2Decision::Reject) — the shell abandons the rekey and - /// keeps the current session. A match is + /// [`Reject`](RekeyMsg2Decision::Reject) — the shell drops the `msg2`, keeps + /// the rekey cycle and keeps the current session. A match is /// [`Install`](RekeyMsg2Decision::Install), the unchanged legitimate path. pub(crate) fn rekey_outbound(&self, snap: &RekeyMsg2Snapshot) -> RekeyMsg2Decision { if snap.learned_peer != snap.established_peer {