diff --git a/CHANGELOG.md b/CHANGELOG.md index 8267495e..47b3a1a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/src/bin/fipstop/ui/routing.rs b/src/bin/fipstop/ui/routing.rs index d03cbab4..e3b9a7b1 100644 --- a/src/bin/fipstop/ui/routing.rs +++ b/src/bin/fipstop/ui/routing.rs @@ -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); diff --git a/src/bin/fipstop/ui/snapshots.rs b/src/bin/fipstop/ui/snapshots.rs index f9f4fe85..8df2fc8c 100644 --- a/src/bin/fipstop/ui/snapshots.rs +++ b/src/bin/fipstop/ui/snapshots.rs @@ -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 diff --git a/src/control/snapshots/show_routing.json b/src/control/snapshots/show_routing.json index 52c3f5a5..83be9d2d 100644 --- a/src/control/snapshots/show_routing.json +++ b/src/control/snapshots/show_routing.json @@ -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": [], diff --git a/src/control/snapshots/show_status.json b/src/control/snapshots/show_status.json index a67008ae..fd2efe5f 100644 --- a/src/control/snapshots/show_status.json +++ b/src/control/snapshots/show_status.json @@ -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, diff --git a/src/node/handlers/forwarding.rs b/src/node/handlers/forwarding.rs index a14e1e37..d2cd9ae3 100644 --- a/src/node/handlers/forwarding.rs +++ b/src/node/handlers/forwarding.rs @@ -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)) => { diff --git a/src/node/metrics.rs b/src/node/metrics.rs index 948cb83b..e2eb813a 100644 --- a/src/node/metrics.rs +++ b/src/node/metrics.rs @@ -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(), diff --git a/src/node/stats.rs b/src/node/stats.rs index 5d84ce8b..1d29c6f3 100644 --- a/src/node/stats.rs +++ b/src/node/stats.rs @@ -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, diff --git a/src/node/tests/forwarding.rs b/src/node/tests/forwarding.rs index 4378512f..3855c729 100644 --- a/src/node/tests/forwarding.rs +++ b/src/node/tests/forwarding.rs @@ -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 // ============================================================================