mirror of
https://github.com/jmcorgan/fips.git
synced 2026-10-05 19:18:25 +00:00
fix(rekey): keep the rekey cycle when a msg2 fails to authenticate
The rekey initiator took its handshake off the peer before reading msg2 and abandoned the whole cycle when the read failed. Nothing authenticates a msg2 ahead of that read, and the index it is dispatched on travels in cleartext in rekey msg1, so anyone on the path who saw the msg1 could answer it first with a msg2 of the right size under that index. That costs more than one cycle. The IK responder commits its new keys when it answers msg1 and cuts over on its own next rekey tick, while the initiator, having abandoned, drops the responder's genuine msg2 at the dispatch lookup. Frames from the responder then miss the initiator's index table at once, frames to the responder fail once its drain window closes, and each end removes the other on the link-dead timeout. With the tick functions driven in-process at one-second steps, the responder removed the link at about 30 s and the initiator at about 31 s, and the initiator's last msg1 resend left the responder holding a peer the initiator no longer had, removed 30 s after that. Read msg2 through a rolling-back variant of the IK read, modelled on the existing XK one, and put the handshake back on the peer when it fails. The msg2 handler keeps the cycle and its dispatch entry in that case and still counts the reject, so the genuine msg2 completes the rekey. The other failures, a missing handshake or one after the read succeeded, still abandon, and if no readable msg2 ever arrives the msg1 resend budget abandons the cycle as before. Each forged msg2 carrying the live index now costs the initiator the msg2 key agreement until the cycle ends, where before only the first one did; the resend budget bounds that. No wire format changes. The new test ages a two-node link past both rekey gates, delivers the rekey msg1, holds the responder's msg2, injects a forged one under the live index, releases the real one, runs a rekey tick on both nodes and checks delivery in both directions. It fails on the unfixed tree at the responder-to-initiator delivery, which is the check that separates the two outcomes, and fails there again with each part of the fix reverted on its own: the handshake put-back, the symmetric-state rollback, and keeping the dispatch entry.
This commit is contained in:
Johnathan Corgan
@@ -30,6 +30,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
||||
interval instead. That retry interval gates only a peer whose last attempt
|
||||
failed, so it cannot clamp a `heartbeat_interval_secs` configured below it.
|
||||
|
||||
#### Link rekey
|
||||
|
||||
- A forged rekey msg2 no longer takes the link down. The rekey initiator gave
|
||||
up its handshake before reading msg2 and abandoned the cycle when the read
|
||||
failed, although nothing authenticates a msg2 ahead of that read. Anyone on
|
||||
the path who saw the rekey msg1 go out could answer first with a msg2 of the
|
||||
right size under the index msg1 carries in cleartext. The responder has
|
||||
already committed its new session by then and cuts over on its next tick, so
|
||||
the two ends were left on different keys: frames from the responder were
|
||||
dropped at once, frames to it failed once its drain window closed, and each
|
||||
end removed the other on the link-dead timeout about 30 s later. A msg2 that
|
||||
fails the read now leaves the handshake as it was before the read, along
|
||||
with the msg1 resend schedule and the msg2 dispatch entry, so the
|
||||
responder's genuine msg2 still completes the rekey. In exchange, every such
|
||||
forgery now costs the initiator the msg2 key agreement until the cycle ends,
|
||||
where before only the first one did; 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
|
||||
|
||||
@@ -1183,6 +1183,7 @@ impl Node {
|
||||
|
||||
// Complete the rekey handshake on the ActivePeer
|
||||
let mut rekey_completed = false;
|
||||
let mut cycle_kept = false;
|
||||
if let Some(peer) = self.peers.get_mut(&peer_node_addr) {
|
||||
match peer.complete_rekey_msg2(noise_msg2) {
|
||||
Ok((session, remote_epoch)) => {
|
||||
@@ -1222,6 +1223,26 @@ impl Node {
|
||||
);
|
||||
rekey_completed = true;
|
||||
}
|
||||
// Nothing authenticated this msg2 before the read, and the
|
||||
// index it names travels in cleartext in our msg1, so it
|
||||
// may be a forgery. The responder committed its new
|
||||
// session when it answered that msg1 and cuts over on its
|
||||
// own tick, so abandoning here would leave the two ends on
|
||||
// different keys. The read rolled the handshake back:
|
||||
// keep the cycle and its dispatch entry so the genuine
|
||||
// msg2 can still complete it. If no readable msg2 ever
|
||||
// arrives, the msg1 resend budget abandons the cycle as
|
||||
// it would for a lost one.
|
||||
Err(e) if peer.awaits_msg2() => {
|
||||
debug!(
|
||||
peer = %display_name,
|
||||
error = %e,
|
||||
"Rekey msg2 did not authenticate, keeping the rekey cycle"
|
||||
);
|
||||
cycle_kept = true;
|
||||
self.stats_mut()
|
||||
.record_reject(RejectReason::Handshake(HandshakeReject::BadState));
|
||||
}
|
||||
Err(e) => {
|
||||
warn!(
|
||||
peer = %display_name,
|
||||
@@ -1242,14 +1263,16 @@ impl Node {
|
||||
|
||||
// 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
|
||||
// untouched. The crypto effect already ran inline; this emits no
|
||||
// action.
|
||||
// the failure paths above either keep the cycle as it was or
|
||||
// revert it, and leave 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);
|
||||
if !cycle_kept {
|
||||
self.pending_outbound.remove(&key);
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
|
||||
@@ -1222,6 +1222,233 @@ async fn rekey_cutover_preserves_data_plane() {
|
||||
cleanup_nodes(&mut nodes).await;
|
||||
}
|
||||
|
||||
/// A forged rekey msg2 that carries the initiator's live rekey index must not
|
||||
/// split the link.
|
||||
///
|
||||
/// An on-path observer sees the rekey msg1 go out, reads the initiator's
|
||||
/// cleartext rekey index from it, and delivers a msg2 of the right size under
|
||||
/// that index ahead of the responder's real reply. Under IK the forgery cannot
|
||||
/// authenticate: only the responder's static key produces a msg2 the initiator
|
||||
/// can read. The IK responder has already committed its new session when it
|
||||
/// answered msg1 and cuts over on its own next rekey tick, so the initiator has
|
||||
/// to keep the cycle through the forgery and complete it on the real msg2. An
|
||||
/// initiator that gives the cycle up instead holds no session matching the one
|
||||
/// the responder now sends on.
|
||||
///
|
||||
/// Deterministic, no wall-clock wait: both sessions are backdated past node 0's
|
||||
/// time trigger and node 1's rekey-acceptance floor, node 1 never initiates,
|
||||
/// and every handshake message is delivered by hand.
|
||||
#[tokio::test]
|
||||
async fn forged_rekey_msg2_does_not_split_the_link() {
|
||||
use crate::noise::HANDSHAKE_MSG2_SIZE;
|
||||
use crate::proto::fmp::wire::{CommonPrefix, PHASE_MSG2, build_msg2};
|
||||
use crate::transport::ReceivedPacket;
|
||||
use crate::utils::index::SessionIndex;
|
||||
|
||||
const REKEY_AFTER_SECS: u64 = 60;
|
||||
|
||||
// node 0 rekeys on time; node 1 only ever responds.
|
||||
let mut cfg0 = crate::config::Config::new();
|
||||
cfg0.node.rekey.enabled = true;
|
||||
cfg0.node.rekey.after_secs = REKEY_AFTER_SECS;
|
||||
cfg0.node.rekey.after_messages = u64::MAX;
|
||||
let mut cfg1 = crate::config::Config::new();
|
||||
cfg1.node.rekey.enabled = true;
|
||||
cfg1.node.rekey.after_secs = u64::MAX;
|
||||
cfg1.node.rekey.after_messages = u64::MAX;
|
||||
|
||||
let mut nodes = vec![
|
||||
make_test_node_with_config(cfg0, 1280).await,
|
||||
make_test_node_with_config(cfg1, 1280).await,
|
||||
];
|
||||
|
||||
// FMP peering + FSP session between the two loopback nodes.
|
||||
initiate_handshake(&mut nodes, 0, 1).await;
|
||||
drain_all_packets(&mut nodes, false).await;
|
||||
let node0_addr = *nodes[0].node.node_addr();
|
||||
let node1_addr = *nodes[1].node.node_addr();
|
||||
assert!(nodes[0].node.get_peer(&node1_addr).is_some());
|
||||
assert!(nodes[1].node.get_peer(&node0_addr).is_some());
|
||||
populate_all_coord_caches(&mut nodes);
|
||||
|
||||
let node1_pubkey = nodes[1].node.identity().pubkey_full();
|
||||
nodes[0]
|
||||
.node
|
||||
.initiate_session(node1_addr, node1_pubkey)
|
||||
.await
|
||||
.unwrap();
|
||||
for _ in 0..4 {
|
||||
tokio::time::sleep(Duration::from_millis(10)).await;
|
||||
process_available_packets(&mut nodes).await;
|
||||
}
|
||||
for (i, remote) in [(0, node1_addr), (1, node0_addr)] {
|
||||
assert!(
|
||||
nodes[i]
|
||||
.node
|
||||
.get_session(&remote)
|
||||
.is_some_and(|s| s.state().is_established()),
|
||||
"node {i} session established"
|
||||
);
|
||||
}
|
||||
|
||||
// Each node's TUN receiver observes the plaintext the other one sent.
|
||||
let (tun0_tx, tun0_rx) = std::sync::mpsc::channel();
|
||||
nodes[0].node.supervisor.tun_tx = Some(tun0_tx);
|
||||
let (tun1_tx, tun1_rx) = std::sync::mpsc::channel();
|
||||
nodes[1].node.supervisor.tun_tx = Some(tun1_tx);
|
||||
let fips0 = crate::FipsAddress::from_node_addr(&node0_addr);
|
||||
let fips1 = crate::FipsAddress::from_node_addr(&node1_addr);
|
||||
|
||||
// Baseline: both directions decode before the rekey, so a failure below
|
||||
// is the rekey's and not the harness's.
|
||||
let pre_fwd = build_ipv6_packet(&fips0, &fips1, b"pre-rekey 0 to 1");
|
||||
let pre_rev = build_ipv6_packet(&fips1, &fips0, b"pre-rekey 1 to 0");
|
||||
nodes[0].node.handle_tun_outbound(pre_fwd.clone()).await;
|
||||
nodes[1].node.handle_tun_outbound(pre_rev.clone()).await;
|
||||
for _ in 0..50 {
|
||||
tokio::time::sleep(Duration::from_millis(10)).await;
|
||||
if process_available_packets(&mut nodes).await == 0 {
|
||||
break;
|
||||
}
|
||||
}
|
||||
let got: Vec<Vec<u8>> = std::iter::from_fn(|| tun1_rx.try_recv().ok()).collect();
|
||||
assert_eq!(got, vec![pre_fwd], "baseline node 0 to node 1 must decode");
|
||||
let got: Vec<Vec<u8>> = std::iter::from_fn(|| tun0_rx.try_recv().ok()).collect();
|
||||
assert_eq!(got, vec![pre_rev], "baseline node 1 to node 0 must decode");
|
||||
|
||||
// Age both sessions past both rekey gates: node 0's jittered time trigger,
|
||||
// and node 1's 30 s floor below which a msg1 is a duplicate, not a rekey.
|
||||
let age = Duration::from_secs(REKEY_AFTER_SECS + crate::node::REKEY_JITTER_SECS as u64 + 1);
|
||||
nodes[0]
|
||||
.node
|
||||
.get_peer_mut(&node1_addr)
|
||||
.unwrap()
|
||||
.test_backdate_session_established(age);
|
||||
nodes[1]
|
||||
.node
|
||||
.get_peer_mut(&node0_addr)
|
||||
.unwrap()
|
||||
.test_backdate_session_established(age);
|
||||
let node0_idx_before = nodes[0].node.get_peer(&node1_addr).unwrap().our_index();
|
||||
let node1_idx_before = nodes[1].node.get_peer(&node0_addr).unwrap().our_index();
|
||||
|
||||
// node 0 starts the rekey; its msg1 lands in node 1's queue.
|
||||
nodes[0].node.check_rekey().await;
|
||||
let rekey_idx = nodes[0]
|
||||
.node
|
||||
.get_peer(&node1_addr)
|
||||
.unwrap()
|
||||
.rekey_our_index()
|
||||
.expect("node 0 must have started a rekey");
|
||||
|
||||
// Deliver the msg1 to node 1 only. node 1 answers as the rekey responder
|
||||
// and commits its new session at once.
|
||||
assert_eq!(
|
||||
process_available_packets(&mut nodes[1..]).await,
|
||||
1,
|
||||
"node 1 must have exactly node 0's rekey msg1 queued"
|
||||
);
|
||||
assert!(
|
||||
nodes[1]
|
||||
.node
|
||||
.get_peer(&node0_addr)
|
||||
.unwrap()
|
||||
.pending_new_session()
|
||||
.is_some(),
|
||||
"node 1 must answer the msg1 as a rekey and hold its new session"
|
||||
);
|
||||
|
||||
// Hold node 1's real msg2 back.
|
||||
let mut held: Vec<ReceivedPacket> =
|
||||
std::iter::from_fn(|| nodes[0].packet_rx.try_recv().ok()).collect();
|
||||
assert_eq!(held.len(), 1, "node 0 must have only node 1's msg2 queued");
|
||||
let real_msg2 = held.remove(0);
|
||||
assert_eq!(
|
||||
CommonPrefix::parse(&real_msg2.data).map(|p| p.phase),
|
||||
Some(PHASE_MSG2),
|
||||
"the held packet must be node 1's msg2"
|
||||
);
|
||||
|
||||
// The forgery: a well-formed header naming node 0's live rekey index, a
|
||||
// valid curve point as the ephemeral so the read gets as far as mixing it
|
||||
// into the handshake, and an epoch ciphertext that cannot authenticate.
|
||||
// The source is node 1's address, as a spoofed UDP source would be.
|
||||
let mut forged_noise = Identity::generate().pubkey_full().serialize().to_vec();
|
||||
forged_noise.resize(HANDSHAKE_MSG2_SIZE, 0xA5);
|
||||
let forged = ReceivedPacket::new(
|
||||
nodes[0].transport_id,
|
||||
nodes[1].addr.clone(),
|
||||
build_msg2(SessionIndex::new(0x5EED_F00D), rekey_idx, &forged_noise),
|
||||
);
|
||||
nodes[0].node.handle_msg2(forged).await;
|
||||
|
||||
// Release the real msg2, then run one rekey tick on each node.
|
||||
nodes[0].node.handle_msg2(real_msg2).await;
|
||||
nodes[0].node.check_rekey().await;
|
||||
nodes[1].node.check_rekey().await;
|
||||
for _ in 0..50 {
|
||||
tokio::time::sleep(Duration::from_millis(10)).await;
|
||||
if process_available_packets(&mut nodes).await == 0 {
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
// node 1 cut over to the session it committed at msg1. Without this the
|
||||
// delivery checks below could pass because no rekey happened at all.
|
||||
assert_ne!(
|
||||
nodes[1].node.get_peer(&node0_addr).unwrap().our_index(),
|
||||
node1_idx_before,
|
||||
"node 1 must have cut over to its new session"
|
||||
);
|
||||
|
||||
let post_fwd = build_ipv6_packet(&fips0, &fips1, b"post-rekey 0 to 1");
|
||||
let post_rev = build_ipv6_packet(&fips1, &fips0, b"post-rekey 1 to 0");
|
||||
nodes[0].node.handle_tun_outbound(post_fwd.clone()).await;
|
||||
nodes[1].node.handle_tun_outbound(post_rev.clone()).await;
|
||||
for _ in 0..50 {
|
||||
tokio::time::sleep(Duration::from_millis(10)).await;
|
||||
if process_available_packets(&mut nodes).await == 0 {
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
let got: Vec<Vec<u8>> = std::iter::from_fn(|| tun1_rx.try_recv().ok()).collect();
|
||||
assert_eq!(
|
||||
got,
|
||||
vec![post_fwd],
|
||||
"node 0 to node 1 must decode after the forged msg2"
|
||||
);
|
||||
|
||||
// This is the assertion that tells the two outcomes apart; keep it. node 0
|
||||
// to node 1 passes either way inside this test, because node 1 keeps its
|
||||
// previous session through the drain window and still decrypts node 0's
|
||||
// old-session frames. node 1 to node 0 fails exactly when node 0 lost the
|
||||
// cycle to the forgery: node 1 now sends on its new session, addressed to
|
||||
// node 0's rekey index, and node 0 has no session registered under it.
|
||||
let handshake = &nodes[0].node.stats().handshake;
|
||||
let (bad_state, unknown) = (handshake.bad_state, handshake.unknown_connection);
|
||||
let got: Vec<Vec<u8>> = std::iter::from_fn(|| tun0_rx.try_recv().ok()).collect();
|
||||
assert_eq!(
|
||||
got,
|
||||
vec![post_rev],
|
||||
"node 1 to node 0 must decode after the forged msg2 \
|
||||
(node 0 handshake rejects: bad_state={bad_state}, unknown_connection={unknown})"
|
||||
);
|
||||
|
||||
let peer = nodes[0].node.get_peer(&node1_addr).unwrap();
|
||||
assert_ne!(
|
||||
peer.our_index(),
|
||||
node0_idx_before,
|
||||
"node 0 must have completed the rekey on the real msg2 and cut over"
|
||||
);
|
||||
assert!(
|
||||
!peer.rekey_in_progress(),
|
||||
"node 0 must not be left mid-rekey"
|
||||
);
|
||||
|
||||
cleanup_nodes(&mut nodes).await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_tun_outbound_triggers_session_initiation() {
|
||||
// Two connected nodes, no session yet.
|
||||
|
||||
+39
-3
@@ -15,9 +15,10 @@ use zeroize::{Zeroize, ZeroizeOnDrop};
|
||||
///
|
||||
/// Maintains the chaining key (ck), handshake hash (h), and current cipher.
|
||||
///
|
||||
/// `Clone` exists for [`HandshakeState::try_read_xk_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_xk_message_2`], which have to put the pre-read
|
||||
/// state back after a message that mixed material in 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
|
||||
@@ -636,6 +637,41 @@ impl HandshakeState {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Read message 2, leaving the handshake untouched when the message
|
||||
/// does not authenticate.
|
||||
///
|
||||
/// `read_message_2` mixes the sender's ephemeral into the symmetric state,
|
||||
/// and both DH results into the key, before it authenticates the encrypted
|
||||
/// epoch, so a message that fails partway leaves a handshake that can
|
||||
/// never read the genuine msg2 afterwards. A caller that keeps its rekey
|
||||
/// cycle across a failed read — because the message may be a forgery
|
||||
/// rather than the responder's corrupt reply — needs the pre-read state
|
||||
/// back.
|
||||
///
|
||||
/// The saved set is exactly what `read_message_2` writes: `symmetric`,
|
||||
/// `remote_ephemeral`, `remote_epoch` and `progress`. `remote_static` is
|
||||
/// not in it, because IK pins the responder's static before msg1 and the
|
||||
/// read only uses it. **That mirror is manual.** A later edit that adds a
|
||||
/// write to `read_message_2` without adding it here silently reintroduces
|
||||
/// the poisoning, and no caller can detect it.
|
||||
pub fn try_read_message_2(&mut self, message: &[u8]) -> Result<(), NoiseError> {
|
||||
let symmetric = self.symmetric.clone();
|
||||
let remote_ephemeral = self.remote_ephemeral;
|
||||
let remote_epoch = self.remote_epoch;
|
||||
let progress = self.progress;
|
||||
|
||||
match self.read_message_2(message) {
|
||||
Ok(()) => Ok(()),
|
||||
Err(e) => {
|
||||
self.symmetric = symmetric;
|
||||
self.remote_ephemeral = remote_ephemeral;
|
||||
self.remote_epoch = remote_epoch;
|
||||
self.progress = progress;
|
||||
Err(e)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// ========================================================================
|
||||
// XK Pattern Methods (Session Layer)
|
||||
// ========================================================================
|
||||
|
||||
+21
-2
@@ -1246,11 +1246,27 @@ impl ActivePeer {
|
||||
self.rekey_our_index
|
||||
}
|
||||
|
||||
/// Whether this peer still holds its rekey initiator handshake, waiting
|
||||
/// on msg2.
|
||||
///
|
||||
/// [`complete_rekey_msg2`](Self::complete_rekey_msg2) keeps the handshake
|
||||
/// when a msg2 fails to authenticate, so after a failed call this tells
|
||||
/// the caller the cycle is still intact.
|
||||
pub fn awaits_msg2(&self) -> bool {
|
||||
self.rekey_handshake.is_some()
|
||||
}
|
||||
|
||||
/// Complete the rekey by processing msg2 (initiator side).
|
||||
///
|
||||
/// Takes the stored handshake state, reads msg2, and returns the
|
||||
/// Reads msg2 against the stored handshake state and returns the
|
||||
/// completed NoiseSession. Clears the handshake-related fields but
|
||||
/// leaves rekey_our_index for set_pending_session to use.
|
||||
///
|
||||
/// A msg2 that fails the read changes nothing: the handshake goes back
|
||||
/// in its pre-read state, and the msg1 resend schedule stays as it was.
|
||||
/// Nothing authenticates a msg2 before this read, so the message may be a
|
||||
/// forgery naming our rekey index, and the responder's genuine msg2 has
|
||||
/// to remain readable when it arrives.
|
||||
pub fn complete_rekey_msg2(
|
||||
&mut self,
|
||||
msg2_bytes: &[u8],
|
||||
@@ -1263,7 +1279,10 @@ impl ActivePeer {
|
||||
got: "no handshake state".to_string(),
|
||||
})?;
|
||||
|
||||
hs.read_message_2(msg2_bytes)?;
|
||||
if let Err(e) = hs.try_read_message_2(msg2_bytes) {
|
||||
self.rekey_handshake = Some(hs);
|
||||
return Err(e);
|
||||
}
|
||||
let remote_epoch = hs.remote_epoch();
|
||||
let session = hs.into_session()?;
|
||||
|
||||
|
||||
Reference in New Issue
Block a user