mirror of
https://github.com/jmcorgan/fips.git
synced 2026-08-10 00:26:59 +00:00
Bind the FSP session address to the authenticated peer key, on both the initial and rekey paths
The responder recorded a session under the source address carried in the datagram without ever checking that address against the static key the Noise handshake had just authenticated. A peer could therefore complete a genuine handshake while claiming another node's address, and the identity cache, the session map and the address the IPv6 shim reconstructs on delivery would all attribute its traffic to the node it named. Derive the address from the authenticated key at the point the key first becomes available in msg3, and reject the handshake when it does not match the claimed source. The entry has already been removed by that point, so returning drops the half-open session and neither the identity nor the session is recorded. The rekey responder path needed its own check rather than inheriting that one. It returns before the initial path's code is reached, and it never read the peer's static key at all, so a rekey could complete under an established session with a different key than the one that opened it. It now requires the key to be unchanged, which is the stronger comparison available there, and abandons the rekey while keeping the existing session intact on mismatch. Tearing the session down instead would have handed an attacker a way to kill established sessions. Both comparisons are on x-only keys. A stored peer key may carry a synthesized even parity because npubs encode no parity, while the handshake learns the true point, so comparing full keys would reject roughly half of legitimate peers on every rekey. Both rejections are counted separately in the session reject statistics. The tests drive real Noise handshakes through the datagram entry point and construct the mismatch rather than asserting the comparison exists.
This commit is contained in:
@@ -835,6 +835,31 @@ impl Node {
|
||||
return;
|
||||
}
|
||||
|
||||
// The rekey must come from the peer the session was established
|
||||
// with. Compare x-only keys: the stored key's parity may be a
|
||||
// synthesized even parity (npubs carry no parity), while the
|
||||
// handshake learns the true point.
|
||||
let rekey_pubkey = match handshake.remote_static() {
|
||||
Some(pk) => *pk,
|
||||
None => {
|
||||
debug!("No remote static key after processing rekey XK msg3");
|
||||
entry.abandon_rekey();
|
||||
self.sessions.insert(*src_addr, entry);
|
||||
return;
|
||||
}
|
||||
};
|
||||
if rekey_pubkey.x_only_public_key().0 != entry.remote_pubkey().x_only_public_key().0 {
|
||||
warn!(
|
||||
src = %self.peer_display_name(src_addr),
|
||||
"FSP rekey: initiator static key differs from the established peer key"
|
||||
);
|
||||
entry.abandon_rekey();
|
||||
self.sessions.insert(*src_addr, entry);
|
||||
self.stats_mut()
|
||||
.record_reject(RejectReason::Session(SessionReject::RekeyKeyMismatch));
|
||||
return;
|
||||
}
|
||||
|
||||
// Complete the handshake → store as pending new session
|
||||
let session = match handshake.into_session() {
|
||||
Ok(s) => s,
|
||||
@@ -884,6 +909,21 @@ impl Node {
|
||||
}
|
||||
};
|
||||
|
||||
// The claimed source address must be derivable from the key we just
|
||||
// authenticated, or the peer is opening a session under another
|
||||
// node's address.
|
||||
let derived_addr = NodeAddr::from_pubkey(&remote_pubkey.x_only_public_key().0);
|
||||
if derived_addr != *src_addr {
|
||||
warn!(
|
||||
src = %self.peer_display_name(src_addr),
|
||||
derived = %derived_addr,
|
||||
"SessionMsg3 source address does not match the authenticated static key"
|
||||
);
|
||||
self.stats_mut()
|
||||
.record_reject(RejectReason::Session(SessionReject::AddrMismatch));
|
||||
return; // Entry was already removed
|
||||
}
|
||||
|
||||
// Register the initiator's identity for future TUN → session routing
|
||||
self.register_identity(*src_addr, remote_pubkey);
|
||||
|
||||
|
||||
+14
-1
@@ -177,7 +177,8 @@ pub enum HandshakeReject {
|
||||
/// FSP session rejection reasons.
|
||||
///
|
||||
/// `UnknownSession` and `BadState` cover the session unknown-session
|
||||
/// and state-machine cluster.
|
||||
/// and state-machine cluster. `AddrMismatch` and `RekeyKeyMismatch`
|
||||
/// cover the peer-identity binding checks on the XK msg3 receive path.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
|
||||
#[non_exhaustive]
|
||||
pub enum SessionReject {
|
||||
@@ -196,6 +197,18 @@ pub enum SessionReject {
|
||||
/// is not `AwaitingMsg3`. Tracked via
|
||||
/// [`SessionStats::bad_state`](crate::node::stats::SessionStats).
|
||||
BadState,
|
||||
/// Inbound XK msg3 completed the handshake, but the initiator's
|
||||
/// static key does not derive the source address the datagram
|
||||
/// claimed — the peer is opening a session under another node's
|
||||
/// address. Tracked via
|
||||
/// [`SessionStats::addr_mismatch`](crate::node::stats::SessionStats).
|
||||
AddrMismatch,
|
||||
/// Inbound XK msg3 completed a responder-side rekey, but the
|
||||
/// initiator's static key differs from the key the session was
|
||||
/// established with — the rekey is not from the established peer.
|
||||
/// Tracked via
|
||||
/// [`SessionStats::rekey_key_mismatch`](crate::node::stats::SessionStats).
|
||||
RekeyKeyMismatch,
|
||||
}
|
||||
|
||||
/// MMP rejection reasons.
|
||||
|
||||
@@ -32,6 +32,14 @@ pub struct SessionStats {
|
||||
/// before Established; SessionAck outside Initiating; SessionMsg3
|
||||
/// outside AwaitingMsg3).
|
||||
pub bad_state: u64,
|
||||
/// Inbound XK msg3 whose initiator static key does not derive the
|
||||
/// source address the datagram claimed. The half-open session is
|
||||
/// dropped and no identity is registered.
|
||||
pub addr_mismatch: u64,
|
||||
/// Inbound rekey XK msg3 whose initiator static key differs from
|
||||
/// the key the session was established with. The rekey is
|
||||
/// abandoned and the existing session is left intact.
|
||||
pub rekey_key_mismatch: u64,
|
||||
}
|
||||
|
||||
impl SessionStats {
|
||||
@@ -39,6 +47,8 @@ impl SessionStats {
|
||||
SessionStatsSnapshot {
|
||||
unknown_session: self.unknown_session,
|
||||
bad_state: self.bad_state,
|
||||
addr_mismatch: self.addr_mismatch,
|
||||
rekey_key_mismatch: self.rekey_key_mismatch,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -46,6 +56,8 @@ impl SessionStats {
|
||||
match reason {
|
||||
SessionReject::UnknownSession => self.unknown_session += 1,
|
||||
SessionReject::BadState => self.bad_state += 1,
|
||||
SessionReject::AddrMismatch => self.addr_mismatch += 1,
|
||||
SessionReject::RekeyKeyMismatch => self.rekey_key_mismatch += 1,
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -303,6 +315,8 @@ pub struct BloomStatsSnapshot {
|
||||
pub struct SessionStatsSnapshot {
|
||||
pub unknown_session: u64,
|
||||
pub bad_state: u64,
|
||||
pub addr_mismatch: u64,
|
||||
pub rekey_key_mismatch: u64,
|
||||
}
|
||||
|
||||
#[derive(Clone, Debug, Default, Serialize)]
|
||||
@@ -359,6 +373,35 @@ mod tests {
|
||||
assert_eq!(stats.unknown_session, 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn session_stats_record_reject_addr_mismatch() {
|
||||
let mut stats = SessionStats::default();
|
||||
stats.record_reject(SessionReject::AddrMismatch);
|
||||
stats.record_reject(SessionReject::AddrMismatch);
|
||||
assert_eq!(stats.addr_mismatch, 2);
|
||||
assert_eq!(stats.rekey_key_mismatch, 0);
|
||||
assert_eq!(stats.unknown_session, 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn session_stats_record_reject_rekey_key_mismatch() {
|
||||
let mut stats = SessionStats::default();
|
||||
stats.record_reject(SessionReject::RekeyKeyMismatch);
|
||||
assert_eq!(stats.rekey_key_mismatch, 1);
|
||||
assert_eq!(stats.addr_mismatch, 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn session_stats_snapshot_carries_identity_binding_counters() {
|
||||
let mut stats = SessionStats::default();
|
||||
stats.record_reject(SessionReject::AddrMismatch);
|
||||
stats.record_reject(SessionReject::RekeyKeyMismatch);
|
||||
stats.record_reject(SessionReject::RekeyKeyMismatch);
|
||||
let snap = stats.snapshot();
|
||||
assert_eq!(snap.addr_mismatch, 1);
|
||||
assert_eq!(snap.rekey_key_mismatch, 2);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn node_stats_record_reject_dispatches_to_session() {
|
||||
let mut stats = NodeStats::new();
|
||||
|
||||
+224
-1
@@ -6,7 +6,7 @@ use crate::node::tests::spanning_tree::{
|
||||
TestNode, cleanup_nodes, generate_random_edges, lock_large_network_test,
|
||||
process_available_packets, run_tree_test, run_tree_test_with_mtus, verify_tree_convergence,
|
||||
};
|
||||
use crate::protocol::{SessionAck, SessionDatagram};
|
||||
use crate::protocol::{SessionAck, SessionDatagram, SessionMsg3};
|
||||
|
||||
/// Populate all nodes' coordinate caches with each other's coords.
|
||||
///
|
||||
@@ -2391,3 +2391,226 @@ fn test_handle_path_mtu_notification_no_session_no_op() {
|
||||
"PathMtuNotification with no session must not touch path_mtu_lookup"
|
||||
);
|
||||
}
|
||||
|
||||
// ============================================================================
|
||||
// Session identity binding: XK msg3 source address / static key
|
||||
// ============================================================================
|
||||
|
||||
/// Helper: drive a full XK exchange against `responder_identity` and return
|
||||
/// the responder's half-completed handshake plus the initiator's msg3.
|
||||
///
|
||||
/// The msg3 is cryptographically valid for the responder and carries
|
||||
/// `initiator_identity`'s static key, which is exactly the shape of the
|
||||
/// defect: a peer that completes a real handshake while the datagram claims
|
||||
/// somebody else's source address.
|
||||
fn drive_xk_to_msg3(
|
||||
initiator_identity: &Identity,
|
||||
responder_identity: &Identity,
|
||||
) -> (crate::noise::HandshakeState, Vec<u8>) {
|
||||
use crate::noise::HandshakeState;
|
||||
|
||||
let mut initiator = HandshakeState::new_xk_initiator(
|
||||
initiator_identity.keypair(),
|
||||
responder_identity.pubkey_full(),
|
||||
);
|
||||
let mut responder = HandshakeState::new_xk_responder(responder_identity.keypair());
|
||||
|
||||
let mut init_epoch = [0u8; 8];
|
||||
rand::Rng::fill_bytes(&mut rand::rng(), &mut init_epoch);
|
||||
initiator.set_local_epoch(init_epoch);
|
||||
let mut resp_epoch = [0u8; 8];
|
||||
rand::Rng::fill_bytes(&mut rand::rng(), &mut resp_epoch);
|
||||
responder.set_local_epoch(resp_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)
|
||||
}
|
||||
|
||||
/// Helper: generate an identity whose full public key has odd parity.
|
||||
fn generate_odd_parity_identity() -> Identity {
|
||||
loop {
|
||||
let id = Identity::generate();
|
||||
if id.pubkey_full().serialize()[0] == 0x03 {
|
||||
return id;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_session_msg3_rejects_spoofed_source_address() {
|
||||
let mut node = make_node();
|
||||
let attacker = Identity::generate();
|
||||
let victim = Identity::generate();
|
||||
let victim_addr = *victim.node_addr();
|
||||
|
||||
let (responder, msg3) = drive_xk_to_msg3(&attacker, node.identity());
|
||||
|
||||
// Half-open session recorded under the victim's address, as
|
||||
// handle_session_setup would have done from the claimed source.
|
||||
let entry = crate::node::session::SessionEntry::new(
|
||||
victim_addr,
|
||||
node.identity().pubkey_full(),
|
||||
EndToEndState::AwaitingMsg3(responder),
|
||||
1000,
|
||||
false,
|
||||
);
|
||||
node.sessions.insert(victim_addr, entry);
|
||||
|
||||
node.handle_session_payload(&victim_addr, &SessionMsg3::new(msg3).encode(), 1280, false)
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
node.session_count(),
|
||||
0,
|
||||
"session must not be installed under an address the peer's key does not derive"
|
||||
);
|
||||
assert_eq!(
|
||||
node.identity_cache_len(),
|
||||
0,
|
||||
"identity cache must not be poisoned with the spoofed address"
|
||||
);
|
||||
assert_eq!(node.stats().session.addr_mismatch, 1);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_session_msg3_accepts_matching_source_address() {
|
||||
let mut node = make_node();
|
||||
let peer = Identity::generate();
|
||||
let peer_addr = *peer.node_addr();
|
||||
|
||||
let (responder, msg3) = drive_xk_to_msg3(&peer, node.identity());
|
||||
|
||||
let entry = crate::node::session::SessionEntry::new(
|
||||
peer_addr,
|
||||
node.identity().pubkey_full(),
|
||||
EndToEndState::AwaitingMsg3(responder),
|
||||
1000,
|
||||
false,
|
||||
);
|
||||
node.sessions.insert(peer_addr, entry);
|
||||
|
||||
node.handle_session_payload(&peer_addr, &SessionMsg3::new(msg3).encode(), 1280, false)
|
||||
.await;
|
||||
|
||||
assert!(
|
||||
node.sessions
|
||||
.get(&peer_addr)
|
||||
.is_some_and(|e| e.is_established()),
|
||||
"an honest initiator using its own address must still establish"
|
||||
);
|
||||
assert_eq!(node.identity_cache_len(), 1);
|
||||
assert_eq!(node.stats().session.addr_mismatch, 0);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_rekey_msg3_rejects_different_static_key() {
|
||||
let mut node = make_node();
|
||||
let legit = Identity::generate();
|
||||
let attacker = Identity::generate();
|
||||
let peer_addr = *legit.node_addr();
|
||||
|
||||
let session = make_noise_session(node.identity(), &legit);
|
||||
let mut entry = crate::node::session::SessionEntry::new(
|
||||
peer_addr,
|
||||
legit.pubkey_full(),
|
||||
EndToEndState::Established(session),
|
||||
1000,
|
||||
true,
|
||||
);
|
||||
entry.mark_established(1000);
|
||||
|
||||
// Responder-side rekey armed, but driven by a different identity.
|
||||
let (responder, msg3) = drive_xk_to_msg3(&attacker, node.identity());
|
||||
entry.set_rekey_state(responder, false);
|
||||
node.sessions.insert(peer_addr, entry);
|
||||
|
||||
node.handle_session_payload(&peer_addr, &SessionMsg3::new(msg3).encode(), 1280, false)
|
||||
.await;
|
||||
|
||||
let entry = node
|
||||
.sessions
|
||||
.get(&peer_addr)
|
||||
.expect("existing session must survive a spoofed rekey");
|
||||
assert!(entry.is_established());
|
||||
assert!(
|
||||
entry.pending_new_session().is_none(),
|
||||
"a rekey from a different static key must not become the pending session"
|
||||
);
|
||||
assert!(!entry.has_rekey_in_progress());
|
||||
assert_eq!(node.stats().session.rekey_key_mismatch, 1);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_rekey_msg3_accepts_established_peer_key() {
|
||||
let mut node = make_node();
|
||||
let legit = Identity::generate();
|
||||
let peer_addr = *legit.node_addr();
|
||||
|
||||
let session = make_noise_session(node.identity(), &legit);
|
||||
let mut entry = crate::node::session::SessionEntry::new(
|
||||
peer_addr,
|
||||
legit.pubkey_full(),
|
||||
EndToEndState::Established(session),
|
||||
1000,
|
||||
true,
|
||||
);
|
||||
entry.mark_established(1000);
|
||||
|
||||
let (responder, msg3) = drive_xk_to_msg3(&legit, node.identity());
|
||||
entry.set_rekey_state(responder, false);
|
||||
node.sessions.insert(peer_addr, entry);
|
||||
|
||||
node.handle_session_payload(&peer_addr, &SessionMsg3::new(msg3).encode(), 1280, false)
|
||||
.await;
|
||||
|
||||
let entry = node.sessions.get(&peer_addr).expect("session present");
|
||||
assert!(entry.pending_new_session().is_some());
|
||||
assert_eq!(node.stats().session.rekey_key_mismatch, 0);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_rekey_msg3_accepts_odd_parity_peer_stored_as_even() {
|
||||
let mut node = make_node();
|
||||
let legit = generate_odd_parity_identity();
|
||||
let peer_addr = *legit.node_addr();
|
||||
|
||||
// The stored key is the even-parity synthesis an npub-sourced peer
|
||||
// identity produces; the peer's real key has odd parity. This must
|
||||
// still be accepted, or every peer-initiated rekey against roughly
|
||||
// half of all peers would be rejected.
|
||||
let stored_pubkey = crate::identity::PeerIdentity::from_pubkey(legit.pubkey()).pubkey_full();
|
||||
assert_ne!(
|
||||
stored_pubkey,
|
||||
legit.pubkey_full(),
|
||||
"test fixture must actually differ in parity"
|
||||
);
|
||||
|
||||
let session = make_noise_session(node.identity(), &legit);
|
||||
let mut entry = crate::node::session::SessionEntry::new(
|
||||
peer_addr,
|
||||
stored_pubkey,
|
||||
EndToEndState::Established(session),
|
||||
1000,
|
||||
true,
|
||||
);
|
||||
entry.mark_established(1000);
|
||||
|
||||
let (responder, msg3) = drive_xk_to_msg3(&legit, node.identity());
|
||||
entry.set_rekey_state(responder, false);
|
||||
node.sessions.insert(peer_addr, entry);
|
||||
|
||||
node.handle_session_payload(&peer_addr, &SessionMsg3::new(msg3).encode(), 1280, false)
|
||||
.await;
|
||||
|
||||
let entry = node.sessions.get(&peer_addr).expect("session present");
|
||||
assert!(
|
||||
entry.pending_new_session().is_some(),
|
||||
"a parity-normalized stored key must not reject a legitimate rekey"
|
||||
);
|
||||
assert_eq!(node.stats().session.rekey_key_mismatch, 0);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user