mirror of
https://github.com/jmcorgan/fips.git
synced 2026-10-05 19:18:25 +00:00
fix(routing): filter coordinate-cache warm writes at the write site
The coordinate cache is warmed from plaintext session headers on datagrams this node is merely forwarding, and both the key and the value come off the wire unauthenticated. Nothing filtered those writes at all. Two checks now run at the single point every warm write passes through, and they are deliberately of different strengths. A coordinate under a root other than ours is refused. It can never route: find_next_hop returns None outright on a root mismatch, and the bloom fallback compares candidates against a my_distance of usize::MAX, so none is ever strictly closer. The cache already applies this invariant whenever our own tree position moves, via invalidate_other_roots; this applies it at write time instead of waiting for the next move. Refusing it also takes away a reflector. Warming runs ahead of the routing decision in the same call, so a foreign-root coordinate plants an entry that synth_routing_error then reads back to choose PathBroken over CoordsRequired, and the error is addressed to the datagram's own src_addr, which nothing binds to the peer that sent it. One packet therefore let an attacker choose both the victim and the more damaging of the two PDUs. The reflection itself is older than this change and survives it; what goes away is the attacker's control of which signal is emitted. A coordinate whose first element is not the address it is filed under is counted and not refused. It is wrong, but refusing it would also refuse a write honest nodes make: a sender whose own cache missed puts its own coordinates in SessionSetup.dest_coords, by way of get_dest_coords. What that would cost a transit node on first contact is not established, so this counts for now and the decision waits on the counter. It is not a security check either way, and the doc comment says so: an attacker satisfies it by naming the victim as its own child, which is the forgery worth making. Neither check is free. A node converging toward a root it has not yet adopted can no longer pre-cache coordinates under it, so it re-learns them after the switch rather than carrying them across. Five warming tests seeded coordinates under a root the node was not in, and so were asserting that a write we now refuse still lands. Their fixtures now share the node's own root, which is what they meant to test; every assertion is otherwise unchanged. Both new guards were break-checked: each new test goes red with its guard disabled, and the healthy path stays green. This is mitigation, not a fix. The primary defect stands: a same-root forgery is unaffected, and the root is public tree state.
This commit is contained in:
@@ -51,6 +51,8 @@
|
||||
"unbound_mtu": 0
|
||||
},
|
||||
"forwarding": {
|
||||
"coord_warm_foreign_root": 0,
|
||||
"coord_warm_key_mismatch": 0,
|
||||
"decode_error_bytes": 0,
|
||||
"decode_error_packets": 0,
|
||||
"delivered_bytes": 0,
|
||||
|
||||
@@ -6,6 +6,8 @@
|
||||
"estimated_mesh_size": null,
|
||||
"exe_path": "<redacted>",
|
||||
"forwarding": {
|
||||
"coord_warm_foreign_root": 0,
|
||||
"coord_warm_key_mismatch": 0,
|
||||
"decode_error_bytes": 0,
|
||||
"decode_error_packets": 0,
|
||||
"delivered_bytes": 0,
|
||||
|
||||
@@ -17,8 +17,9 @@ use crate::proto::fsp::wire::{
|
||||
use crate::proto::fsp::{SessionAck, SessionSetup};
|
||||
use crate::proto::link::{SessionDatagram, SessionDatagramRef};
|
||||
use crate::proto::routing::{DropReason, LimitVerdict, NextHop, RouteAction, RouteOutcome};
|
||||
use crate::proto::stp::TreeCoordinate;
|
||||
use std::time::{Duration, Instant};
|
||||
use tracing::{debug, warn};
|
||||
use tracing::{debug, trace, warn};
|
||||
|
||||
impl Node {
|
||||
/// Handle an incoming SessionDatagram from a peer.
|
||||
@@ -226,6 +227,45 @@ impl Node {
|
||||
/// reconstructed from the header size so the malformed-frame byte counter
|
||||
/// measures the same population as its siblings — which are charged the
|
||||
/// outer slice — instead of the inner FSP payload.
|
||||
/// Warm one coordinate-cache entry from a plaintext session header, after
|
||||
/// the two write-side sanity checks.
|
||||
///
|
||||
/// The key and the value both come off the wire unauthenticated, so this
|
||||
/// is the only place a warm write can be filtered at all. Two checks, and
|
||||
/// they are deliberately of different strengths:
|
||||
///
|
||||
/// **Foreign root: refused.** A coordinate under a root other than ours
|
||||
/// can never route. `StpState::find_next_hop` returns `None` outright on a
|
||||
/// root mismatch, and the bloom fallback compares against a `my_distance`
|
||||
/// of `usize::MAX`, so no candidate is ever strictly closer. Caching one
|
||||
/// therefore buys nothing and costs something real: the entry's presence
|
||||
/// is what `synth_routing_error` reads to choose `PathBroken` over
|
||||
/// `CoordsRequired`, so a foreign-root plant turns this node into a
|
||||
/// one-packet reflector aimed at whatever source the datagram claimed.
|
||||
/// `CoordCache::invalidate_other_roots` already applies this same
|
||||
/// invariant whenever our own tree position moves; this applies it at
|
||||
/// write time instead of waiting for the next move.
|
||||
///
|
||||
/// **Key mismatch: counted only.** A coordinate whose first element is not
|
||||
/// the address it is filed under is wrong, but refusing it here would also
|
||||
/// refuse a write honest nodes make: a sender whose own cache missed puts
|
||||
/// its *own* coordinates in `SessionSetup.dest_coords`, by way of
|
||||
/// `get_dest_coords`. What that costs a transit node on first contact is
|
||||
/// not established, so this counts and does not refuse. It is **not** a
|
||||
/// security check either way — an attacker satisfies it by naming the
|
||||
/// victim as its own child, which is the forgery worth making.
|
||||
fn warm_coord(&mut self, key: NodeAddr, coords: TreeCoordinate, now_ms: u64) {
|
||||
if coords.root_id() != self.tree_state.my_coords().root_id() {
|
||||
self.metrics().forwarding.record_warm_foreign_root();
|
||||
trace!(addr = %key, "Warm write names a foreign root; not caching");
|
||||
return;
|
||||
}
|
||||
if *coords.node_addr() != key {
|
||||
self.metrics().forwarding.record_warm_key_mismatch();
|
||||
}
|
||||
self.coord_cache_mut().insert(key, coords, now_ms);
|
||||
}
|
||||
|
||||
fn try_warm_coord_cache_ref(&mut self, datagram: &SessionDatagramRef<'_>, outer_len: usize) {
|
||||
let prefix = match FspCommonPrefix::parse(datagram.payload) {
|
||||
Some(p) => p,
|
||||
@@ -242,10 +282,8 @@ impl Node {
|
||||
match prefix.phase {
|
||||
FSP_PHASE_MSG1 => match SessionSetup::decode(inner) {
|
||||
Ok(setup) => {
|
||||
self.coord_cache_mut()
|
||||
.insert(datagram.src_addr, setup.src_coords, now_ms);
|
||||
self.coord_cache_mut()
|
||||
.insert(datagram.dest_addr, setup.dest_coords, now_ms);
|
||||
self.warm_coord(datagram.src_addr, setup.src_coords, now_ms);
|
||||
self.warm_coord(datagram.dest_addr, setup.dest_coords, now_ms);
|
||||
debug!(
|
||||
src = %datagram.src_addr,
|
||||
dest = %datagram.dest_addr,
|
||||
@@ -258,10 +296,8 @@ impl Node {
|
||||
},
|
||||
FSP_PHASE_MSG2 => match SessionAck::decode(inner) {
|
||||
Ok(ack) => {
|
||||
self.coord_cache_mut()
|
||||
.insert(datagram.src_addr, ack.src_coords, now_ms);
|
||||
self.coord_cache_mut()
|
||||
.insert(datagram.dest_addr, ack.dest_coords, now_ms);
|
||||
self.warm_coord(datagram.src_addr, ack.src_coords, now_ms);
|
||||
self.warm_coord(datagram.dest_addr, ack.dest_coords, now_ms);
|
||||
debug!(
|
||||
src = %datagram.src_addr,
|
||||
dest = %datagram.dest_addr,
|
||||
@@ -297,12 +333,10 @@ impl Node {
|
||||
match parse_encrypted_coords(coord_data) {
|
||||
Ok((src_coords, dest_coords, _bytes_consumed)) => {
|
||||
if let Some(coords) = src_coords {
|
||||
self.coord_cache_mut()
|
||||
.insert(datagram.src_addr, coords, now_ms);
|
||||
self.warm_coord(datagram.src_addr, coords, now_ms);
|
||||
}
|
||||
if let Some(coords) = dest_coords {
|
||||
self.coord_cache_mut()
|
||||
.insert(datagram.dest_addr, coords, now_ms);
|
||||
self.warm_coord(datagram.dest_addr, coords, now_ms);
|
||||
}
|
||||
debug!(
|
||||
src = %datagram.src_addr,
|
||||
|
||||
@@ -69,6 +69,18 @@ pub struct ForwardingMetrics {
|
||||
pub decode_error_bytes: Counter,
|
||||
pub warm_malformed_packets: Counter,
|
||||
pub warm_malformed_bytes: Counter,
|
||||
/// Coordinate-cache warm writes refused because the coordinate names a
|
||||
/// root other than this node's. Such an entry can never route — both
|
||||
/// selectors reject a foreign root — so a write of one is either tree
|
||||
/// churn or a plant, and the counter is the only place the difference
|
||||
/// shows.
|
||||
pub coord_warm_foreign_root: Counter,
|
||||
/// Coordinate-cache warm writes whose coordinate does not name the address
|
||||
/// it is filed under. Counted, not refused. **This is not a security
|
||||
/// signal**: an attacker forges a passing coordinate by naming the victim
|
||||
/// as its own child. It counts a defect honest nodes make, where a sender
|
||||
/// whose own cache missed sends its own coordinates as the destination's.
|
||||
pub coord_warm_key_mismatch: Counter,
|
||||
pub ttl_exhausted_packets: Counter,
|
||||
pub ttl_exhausted_bytes: Counter,
|
||||
pub delivered_packets: Counter,
|
||||
@@ -134,6 +146,16 @@ impl ForwardingMetrics {
|
||||
self.warm_malformed_bytes.add(bytes as u64);
|
||||
}
|
||||
|
||||
/// Record a warm write refused for naming a foreign root.
|
||||
pub fn record_warm_foreign_root(&self) {
|
||||
self.coord_warm_foreign_root.inc();
|
||||
}
|
||||
|
||||
/// Record a warm write whose coordinate does not name its own key.
|
||||
pub fn record_warm_key_mismatch(&self) {
|
||||
self.coord_warm_key_mismatch.inc();
|
||||
}
|
||||
|
||||
/// Record a forwarded (transit) packet of `bytes` payload.
|
||||
#[inline]
|
||||
pub fn record_forwarded(&self, bytes: usize) {
|
||||
@@ -202,6 +224,8 @@ impl ForwardingMetrics {
|
||||
decode_error_bytes: self.decode_error_bytes.get(),
|
||||
warm_malformed_packets: self.warm_malformed_packets.get(),
|
||||
warm_malformed_bytes: self.warm_malformed_bytes.get(),
|
||||
coord_warm_foreign_root: self.coord_warm_foreign_root.get(),
|
||||
coord_warm_key_mismatch: self.coord_warm_key_mismatch.get(),
|
||||
ttl_exhausted_packets: self.ttl_exhausted_packets.get(),
|
||||
ttl_exhausted_bytes: self.ttl_exhausted_bytes.get(),
|
||||
delivered_packets: self.delivered_packets.get(),
|
||||
|
||||
@@ -303,6 +303,8 @@ pub struct ForwardingStatsSnapshot {
|
||||
pub decode_error_bytes: u64,
|
||||
pub warm_malformed_packets: u64,
|
||||
pub warm_malformed_bytes: u64,
|
||||
pub coord_warm_foreign_root: u64,
|
||||
pub coord_warm_key_mismatch: u64,
|
||||
pub ttl_exhausted_packets: u64,
|
||||
pub ttl_exhausted_bytes: u64,
|
||||
pub delivered_packets: u64,
|
||||
|
||||
@@ -204,13 +204,103 @@ async fn test_forwarding_direct_peer() {
|
||||
// Coordinate Cache Warming Tests
|
||||
// ============================================================================
|
||||
|
||||
#[tokio::test]
|
||||
async fn warming_refuses_a_coordinate_rooted_in_a_tree_this_node_is_not_in() {
|
||||
let mut node = make_node();
|
||||
let from = make_node_addr(0xAA);
|
||||
let src_addr = make_node_addr(0x01);
|
||||
let dest_addr = make_node_addr(0x02);
|
||||
// Deliberately NOT this node's root. Such an entry can never route: both
|
||||
// selectors reject a foreign root, so caching it only occupies a slot and
|
||||
// flips the error-PDU choice in `synth_routing_error` from CoordsRequired
|
||||
// to PathBroken, which is the primitive this guard removes.
|
||||
let foreign_root = make_node_addr(0xF0);
|
||||
assert_ne!(
|
||||
&foreign_root,
|
||||
node.tree_state.my_coords().root_id(),
|
||||
"fixture must not accidentally share the node's root"
|
||||
);
|
||||
|
||||
let src_coords = TreeCoordinate::from_addrs(vec![src_addr, foreign_root]).unwrap();
|
||||
let dest_coords = TreeCoordinate::from_addrs(vec![dest_addr, foreign_root]).unwrap();
|
||||
let setup_payload = SessionSetup::new(src_coords, dest_coords).encode();
|
||||
let encoded = SessionDatagram::new(src_addr, dest_addr, setup_payload).encode();
|
||||
|
||||
let before = node.metrics().forwarding.coord_warm_foreign_root.get();
|
||||
node.handle_session_datagram(&from, &encoded[1..], false)
|
||||
.await;
|
||||
|
||||
let now_ms = std::time::SystemTime::now()
|
||||
.duration_since(std::time::UNIX_EPOCH)
|
||||
.unwrap()
|
||||
.as_millis() as u64;
|
||||
assert!(
|
||||
node.coord_cache().get(&src_addr, now_ms).is_none(),
|
||||
"a foreign-root src coordinate was cached"
|
||||
);
|
||||
assert!(
|
||||
node.coord_cache().get(&dest_addr, now_ms).is_none(),
|
||||
"a foreign-root dest coordinate was cached"
|
||||
);
|
||||
assert_eq!(
|
||||
node.metrics().forwarding.coord_warm_foreign_root.get(),
|
||||
before + 2,
|
||||
"both refusals should be counted"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn warming_counts_but_still_caches_a_coordinate_that_does_not_name_its_own_key() {
|
||||
let mut node = make_node();
|
||||
let from = make_node_addr(0xAA);
|
||||
let src_addr = make_node_addr(0x01);
|
||||
let dest_addr = make_node_addr(0x02);
|
||||
let root_addr = *node.tree_state.my_coords().root_id();
|
||||
let someone_else = make_node_addr(0x09);
|
||||
|
||||
// dest_coords names 0x09, not the 0x02 it will be filed under. This is the
|
||||
// shape an honest sender produces when its own cache missed and
|
||||
// `get_dest_coords` fell back to the sender's own coordinates, so it is
|
||||
// counted and NOT refused.
|
||||
let src_coords = TreeCoordinate::from_addrs(vec![src_addr, root_addr]).unwrap();
|
||||
let dest_coords = TreeCoordinate::from_addrs(vec![someone_else, root_addr]).unwrap();
|
||||
let setup_payload = SessionSetup::new(src_coords, dest_coords).encode();
|
||||
let encoded = SessionDatagram::new(src_addr, dest_addr, setup_payload).encode();
|
||||
|
||||
let before = node.metrics().forwarding.coord_warm_key_mismatch.get();
|
||||
node.handle_session_datagram(&from, &encoded[1..], false)
|
||||
.await;
|
||||
|
||||
let now_ms = std::time::SystemTime::now()
|
||||
.duration_since(std::time::UNIX_EPOCH)
|
||||
.unwrap()
|
||||
.as_millis() as u64;
|
||||
assert!(
|
||||
node.coord_cache().get(&dest_addr, now_ms).is_some(),
|
||||
"the mismatching entry should still be cached; this check counts only"
|
||||
);
|
||||
assert_eq!(
|
||||
node.metrics().forwarding.coord_warm_key_mismatch.get(),
|
||||
before + 1,
|
||||
"the mismatch should be counted exactly once"
|
||||
);
|
||||
assert_eq!(
|
||||
node.metrics().forwarding.coord_warm_key_mismatch.get() - before,
|
||||
1,
|
||||
"the well-formed src coordinate must not be counted as a mismatch"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_coord_cache_warming_session_setup() {
|
||||
let mut node = make_node();
|
||||
let from = make_node_addr(0xAA);
|
||||
let src_addr = make_node_addr(0x01);
|
||||
let dest_addr = make_node_addr(0x02);
|
||||
let root_addr = make_node_addr(0xF0);
|
||||
// The warming path refuses a coordinate under a root other than this
|
||||
// node's, so a fixture that wants the write to land has to share the
|
||||
// node's root. A fresh node is its own root.
|
||||
let root_addr = *node.tree_state.my_coords().root_id();
|
||||
|
||||
let src_coords = TreeCoordinate::from_addrs(vec![src_addr, root_addr]).unwrap();
|
||||
let dest_coords = TreeCoordinate::from_addrs(vec![dest_addr, root_addr]).unwrap();
|
||||
@@ -254,7 +344,10 @@ async fn test_coord_cache_warming_session_ack() {
|
||||
let from = make_node_addr(0xAA);
|
||||
let src_addr = make_node_addr(0x01);
|
||||
let dest_addr = make_node_addr(0x02);
|
||||
let root_addr = make_node_addr(0xF0);
|
||||
// The warming path refuses a coordinate under a root other than this
|
||||
// node's, so a fixture that wants the write to land has to share the
|
||||
// node's root. A fresh node is its own root.
|
||||
let root_addr = *node.tree_state.my_coords().root_id();
|
||||
|
||||
let src_coords = TreeCoordinate::from_addrs(vec![src_addr, root_addr]).unwrap();
|
||||
let dest_coords = TreeCoordinate::from_addrs(vec![dest_addr, root_addr]).unwrap();
|
||||
@@ -298,7 +391,10 @@ async fn test_coord_cache_warming_encrypted_msg_with_coords() {
|
||||
let from = make_node_addr(0xAA);
|
||||
let src_addr = make_node_addr(0x01);
|
||||
let dest_addr = make_node_addr(0x02);
|
||||
let root_addr = make_node_addr(0xF0);
|
||||
// The warming path refuses a coordinate under a root other than this
|
||||
// node's, so a fixture that wants the write to land has to share the
|
||||
// node's root. A fresh node is its own root.
|
||||
let root_addr = *node.tree_state.my_coords().root_id();
|
||||
|
||||
let src_coords = TreeCoordinate::from_addrs(vec![src_addr, root_addr]).unwrap();
|
||||
let dest_coords = TreeCoordinate::from_addrs(vec![dest_addr, root_addr]).unwrap();
|
||||
@@ -392,7 +488,10 @@ async fn test_coord_cache_warming_ttl_zero_local_delivery() {
|
||||
let from = make_node_addr(0xAA);
|
||||
let my_addr = *node.node_addr();
|
||||
let src_addr = make_node_addr(0x01);
|
||||
let root_addr = make_node_addr(0xF0);
|
||||
// The warming path refuses a coordinate under a root other than this
|
||||
// node's, so a fixture that wants the write to land has to share the
|
||||
// node's root. A fresh node is its own root.
|
||||
let root_addr = *node.tree_state.my_coords().root_id();
|
||||
|
||||
let src_coords = TreeCoordinate::from_addrs(vec![src_addr, root_addr]).unwrap();
|
||||
let dest_coords = TreeCoordinate::from_addrs(vec![my_addr, root_addr]).unwrap();
|
||||
@@ -438,7 +537,10 @@ async fn test_coord_cache_warming_ttl_zero_transit_drop() {
|
||||
let from = make_node_addr(0xAA);
|
||||
let src_addr = make_node_addr(0x01);
|
||||
let dest_addr = make_node_addr(0x02);
|
||||
let root_addr = make_node_addr(0xF0);
|
||||
// The warming path refuses a coordinate under a root other than this
|
||||
// node's, so a fixture that wants the write to land has to share the
|
||||
// node's root. A fresh node is its own root.
|
||||
let root_addr = *node.tree_state.my_coords().root_id();
|
||||
|
||||
let src_coords = TreeCoordinate::from_addrs(vec![src_addr, root_addr]).unwrap();
|
||||
let dest_coords = TreeCoordinate::from_addrs(vec![dest_addr, root_addr]).unwrap();
|
||||
|
||||
Reference in New Issue
Block a user