Bound the coordinate-cache warm path by the encrypted-header parser

A SessionDatagram carrying a truncated inner FSP payload panicked the
forwarding path. The warm path sliced the inner payload at the full
12-byte header offset while guarding only with the 4-byte common prefix
parser, so an inner payload of 4 to 11 bytes with phase 0x0 and the
Coords Present flag set indexed past the end of the slice. The receive
loop is the process's main future, so the panic terminated the daemon
rather than a task, and under the packaged systemd unit the node
restarted into the same frame.

Apply the same FspEncryptedHeader guard the local-delivery path already
used. That also drops a malformed frame carrying a non-zero protocol
version or the Unencrypted flag alongside Coords Present, rather than
reading its body as coordinates.

Any peer that had completed a link handshake could trigger this, and
admission is default-open.

Count the frames the coordinate-cache warm path abandons

The warm path drops a malformed encrypted frame silently apart from one
debug line, so a node being probed with them looks identical to a quiet
one. Add a counter pair for the abandoned frame and surface it.

The counter is deliberately not a forwarding rejection. The warm helper
returns nothing and runs ahead of both the delivery and TTL decisions, so
the frame goes on to be delivered or forwarded exactly as before; routing
it through the reject path would book a packet as dropped that was not.
The debug line now also carries the version and flags it parsed, which is
what distinguishes a truncated frame from one with an unexpected header.

The counter is tested by driving the drop and asserting it moved, and the
status pane row is tested the same way.
This commit is contained in:
Johnathan Corgan
2026-08-13 21:13:10 +00:00
parent 92efe094fd
commit 1d589f61e1
9 changed files with 177 additions and 6 deletions
+22
View File
@@ -90,6 +90,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed
- A SessionDatagram carrying a truncated inner FSP payload no longer panics the
forwarding path. The coordinate-cache warm path sliced the inner payload at
the full 12-byte header offset while guarding only with the 4-byte common
prefix parser, so an inner payload of 4 to 11 bytes with phase 0x0 and the
Coords Present flag set indexed past the end of the slice. Because the
receive loop is the process's main future, the panic terminated the daemon
rather than a task, and under the packaged systemd unit the node restarted
into the same frame. The warm path now applies the same
`FspEncryptedHeader` guard the local-delivery path already used, which
additionally means a malformed frame carrying a non-zero protocol version or
the Unencrypted flag alongside Coords Present is dropped rather than having
its body read as coordinates. Any peer that had completed a link handshake
could trigger this, and admission is default-open. Frames rejected by that
guard are now counted in the forwarding statistics as
`warm_malformed_packets` and `warm_malformed_bytes`, visible over the control
socket and on the fipstop Routing State pane, so a node being fed malformed
frames is distinguishable from a quiet one at the default log level. The
count is not a packet drop: the frame is still delivered or forwarded, and
only the coordinate-cache warm attempt is abandoned. The existing debug log
now also carries the frame's protocol version and flags, which separate a
short frame from a bad-version or Unencrypted-flagged one.
- The maintainer address published in package metadata no longer bounces. The
crate authors field, the Debian package maintainer and upstream contact, and
both AUR PKGBUILD maintainer lines carried an address that no longer accepts
+7
View File
@@ -59,6 +59,13 @@ fn draw_routing_state(
"Recent Requests",
helpers::u64_field(data, "recent_requests"),
),
// Not a drop: the frame is still delivered or forwarded, only the
// coordinate-cache warm attempt was abandoned. It belongs here beside
// the cache it failed to warm, not in the Dropped section.
(
"Warm Malformed",
fwd_value(data, "warm_malformed_packets", "warm_malformed_bytes"),
),
]);
let block = helpers::pane_block(" Routing State ", focused);
+13 -1
View File
@@ -908,7 +908,7 @@ fn routing_state_values_aligned() {
"identity_cache_entries": 5,
"pending_lookups": [],
"recent_requests": 7,
"forwarding": {},
"forwarding": { "warm_malformed_packets": 5, "warm_malformed_bytes": 640 },
"discovery": {},
"error_signals": {},
"congestion": {}
@@ -932,6 +932,18 @@ fn routing_state_values_aligned() {
coord_val, ident_val,
"routing state values share a column: {coord:?} vs {ident:?}"
);
// The forwarding helpers fall back to 0 on a missing key, so a mistyped
// key would render "0 pkts" forever without failing anything. Assert the
// fixture's nonzero value actually reaches the row.
let warm = lines
.iter()
.find(|r| r.contains("Warm Malformed"))
.expect("routing state shows the abandoned-warm row");
assert!(
warm.contains("5 pkts"),
"warm-malformed row reads its counter keys: {warm:?}"
);
}
/// Graphs by-peer summary list: the min/max/last numeric columns are
+3 -1
View File
@@ -60,7 +60,9 @@
"route_tree_down_cross": 0,
"route_tree_up": 0,
"ttl_exhausted_bytes": 0,
"ttl_exhausted_packets": 0
"ttl_exhausted_packets": 0,
"warm_malformed_bytes": 0,
"warm_malformed_packets": 0
},
"identity_cache_entries": 0,
"pending_lookups": [],
+3 -1
View File
@@ -29,7 +29,9 @@
"route_tree_down_cross": 0,
"route_tree_up": 0,
"ttl_exhausted_bytes": 0,
"ttl_exhausted_packets": 0
"ttl_exhausted_packets": 0,
"warm_malformed_bytes": 0,
"warm_malformed_packets": 0
},
"ipv6_addr": "fd1b:4788:b7ab:7a43:6a61:1fc5:9fb1:e34c",
"is_leaf_only": false,
+20 -3
View File
@@ -10,7 +10,7 @@ use crate::NodeAddr;
use crate::node::reject::ForwardingReject;
use crate::node::session_wire::{
FSP_COMMON_PREFIX_SIZE, FSP_HEADER_SIZE, FSP_PHASE_ESTABLISHED, FSP_PHASE_MSG1, FSP_PHASE_MSG2,
FspCommonPrefix, parse_encrypted_coords,
FspCommonPrefix, FspEncryptedHeader, parse_encrypted_coords,
};
use crate::node::{Node, NodeError};
use crate::protocol::{
@@ -230,8 +230,25 @@ impl Node {
FSP_PHASE_ESTABLISHED if prefix.has_coords() => {
// CP flag set: coords in cleartext between header and ciphertext.
// Parse coords from the cleartext section after the 12-byte header.
// inner starts after the 4-byte prefix, so we need 8 more bytes
// for the counter (header is 12 total = 4 prefix + 8 counter).
// Re-parse with the encrypted-header parser — the same guard the
// local-delivery path uses — so the slice below is bounded by
// FSP_ENCRYPTED_MIN_SIZE and not by the 4-byte prefix check.
if FspEncryptedHeader::parse(datagram.payload).is_none() {
// Counter is the always-on surface; the debug fields are the
// drill-down that separates a short frame from a bad version
// or a U-flagged one. The level stays at debug: any peer past
// the handshake can drive this at line rate.
self.metrics()
.forwarding
.record_warm_malformed(datagram.payload.len());
debug!(
len = datagram.payload.len(),
version = prefix.version,
flags = prefix.flags,
"Not a well-formed encrypted FSP message; not warming coords"
);
return;
}
let coord_data = &datagram.payload[FSP_HEADER_SIZE..];
match parse_encrypted_coords(coord_data) {
Ok((src_coords, dest_coords, _bytes_consumed)) => {
+20
View File
@@ -67,6 +67,8 @@ pub struct ForwardingMetrics {
pub received_bytes: Counter,
pub decode_error_packets: Counter,
pub decode_error_bytes: Counter,
pub warm_malformed_packets: Counter,
pub warm_malformed_bytes: Counter,
pub ttl_exhausted_packets: Counter,
pub ttl_exhausted_bytes: Counter,
pub delivered_packets: Counter,
@@ -143,6 +145,22 @@ impl ForwardingMetrics {
self.delivered_bytes.add(bytes as u64);
}
/// Record a coordinate-cache warm attempt abandoned because the frame was
/// not a well-formed encrypted FSP message.
///
/// This is **not** a packet drop. The frame is still delivered or
/// forwarded by the normal path; only the opportunistic warm attempt was
/// abandoned, so this must never be folded into the rejection family or
/// rendered as dropped traffic. `bytes` is the payload size of the frame
/// whose warm attempt was abandoned, not volume dropped; it is carried so
/// the counter can be rendered as a packets-and-bytes pair like its
/// siblings.
#[inline]
pub fn record_warm_malformed(&self, bytes: usize) {
self.warm_malformed_packets.inc();
self.warm_malformed_bytes.add(bytes as u64);
}
/// Record a forwarded (transit) packet of `bytes` payload.
#[inline]
pub fn record_forwarded(&self, bytes: usize) {
@@ -209,6 +227,8 @@ impl ForwardingMetrics {
received_bytes: self.received_bytes.get(),
decode_error_packets: self.decode_error_packets.get(),
decode_error_bytes: self.decode_error_bytes.get(),
warm_malformed_packets: self.warm_malformed_packets.get(),
warm_malformed_bytes: self.warm_malformed_bytes.get(),
ttl_exhausted_packets: self.ttl_exhausted_packets.get(),
ttl_exhausted_bytes: self.ttl_exhausted_bytes.get(),
delivered_packets: self.delivered_packets.get(),
+2
View File
@@ -227,6 +227,8 @@ pub struct ForwardingStatsSnapshot {
pub received_bytes: u64,
pub decode_error_packets: u64,
pub decode_error_bytes: u64,
pub warm_malformed_packets: u64,
pub warm_malformed_bytes: u64,
pub ttl_exhausted_packets: u64,
pub ttl_exhausted_bytes: u64,
pub delivered_packets: u64,
+87
View File
@@ -366,6 +366,93 @@ async fn test_coord_cache_warming_encrypted_msg_no_coords() {
);
}
/// Acceptance: an inner FSP payload of 4 to 11 bytes with phase 0x0 and the
/// CP flag set is dropped rather than panicking the forwarding path. That
/// window sits between the common prefix parser's 4-byte floor and the
/// 12-byte header slice the warm path takes, so before the fix the first
/// iteration panicked with a range start index out of range.
#[tokio::test]
async fn test_coord_cache_warming_short_inner_payload_is_dropped_not_panic() {
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 now_ms = std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
.unwrap()
.as_millis() as u64;
for extra in 0..=7 {
let mut data_payload = vec![0x00, FSP_FLAG_CP, 0x00, 0x00];
data_payload.resize(4 + extra, 0x00);
let dg = SessionDatagram::new(src_addr, dest_addr, data_payload).with_ttl(1);
let encoded = dg.encode();
node.handle_session_datagram(&from, &encoded[1..], false)
.await;
}
assert!(
node.coord_cache().get(&src_addr, now_ms).is_none(),
"Short inner payload must not warm src coords"
);
assert!(
node.coord_cache().get(&dest_addr, now_ms).is_none(),
"Short inner payload must not warm dest coords"
);
// Anti-vacuity: only a datagram that ran past the warm call reaches the
// TTL gate. `received_packets` is charged before decode and so would
// count a datagram rejected earlier.
assert_eq!(
node.metrics().forwarding.ttl_exhausted_packets.get(),
8,
"each short-inner-payload datagram must run past the warm call to the TTL gate"
);
// Discriminating: separates "the guard fired" from "coords parsed and
// yielded nothing", which the cache assertions above cannot tell apart.
assert_eq!(
node.metrics().forwarding.warm_malformed_packets.get(),
8,
"each short-inner-payload datagram must be counted as an abandoned warm attempt"
);
// Inner lengths 12 to 27 document the new 28-byte floor: they do not
// panic today either, so this half is not discriminating.
for len in 12..=27 {
let mut data_payload = vec![0x00, FSP_FLAG_CP, 0x00, 0x00];
data_payload.resize(len, 0x00);
let dg = SessionDatagram::new(src_addr, dest_addr, data_payload).with_ttl(1);
let encoded = dg.encode();
node.handle_session_datagram(&from, &encoded[1..], false)
.await;
}
assert!(
node.coord_cache().get(&src_addr, now_ms).is_none(),
"Payload below the encrypted minimum must not warm src coords"
);
assert!(
node.coord_cache().get(&dest_addr, now_ms).is_none(),
"Payload below the encrypted minimum must not warm dest coords"
);
assert_eq!(
node.metrics().forwarding.ttl_exhausted_packets.get(),
24,
"every datagram in both loops must reach the TTL gate"
);
assert_eq!(
node.metrics().forwarding.warm_malformed_packets.get(),
24,
"every datagram in both loops must be counted as an abandoned warm attempt"
);
assert!(
node.metrics().forwarding.warm_malformed_bytes.get() > 0,
"the byte counter must move alongside the packet counter"
);
}
// ============================================================================
// Integration Tests
// ============================================================================