diff --git a/crates/kerykeion/src/delivery.rs b/crates/kerykeion/src/delivery.rs index 676fa96..62796d1 100644 --- a/crates/kerykeion/src/delivery.rs +++ b/crates/kerykeion/src/delivery.rs @@ -44,6 +44,15 @@ pub enum DeliveryFailure { MaxRetries, /// Explicit NAK with a routing error code. Nak(routing::Error), + /// NAK carrying a routing error code this build does not recognize. + /// + // WHY a distinct variant rather than folding into `Nak` (#208): + // `routing::Error` cannot represent "unrecognized" without reusing an + // existing, semantically-wrong code — see `RoutingResult::UnknownError`. + UnknownNak { + /// The raw wire code that did not match any known `routing::Error` variant. + code: i32, + }, /// Message TTL expired. Ttl, /// Destination node is offline and S&F is not available. diff --git a/crates/kerykeion/src/error.rs b/crates/kerykeion/src/error.rs index 5929121..a13c3a6 100644 --- a/crates/kerykeion/src/error.rs +++ b/crates/kerykeion/src/error.rs @@ -249,6 +249,21 @@ pub enum Error { #[snafu(implicit)] location: snafu::Location, }, + + /// Outbound packet-id space exhausted for a [`crate::packet_id::PacketIdCounter`]. + /// + /// The AES-CTR nonce is derived FROM `packet_id` (`crypto::build_nonce`), + /// so this counter never wraps past `u32::MAX` back toward values it may + /// already have issued this run — see `PacketIdCounter::next_id`. Recovery + /// requires the caller to rotate to a new key/PSK; retrying does not help. + #[snafu(display( + "outbound packet-id space exhausted (u32::MAX reached) -- rotate the channel PSK" + ))] + PacketIdSpaceExhausted { + /// Source location for diagnostics. + #[snafu(implicit)] + location: snafu::Location, + }, } // WHY: tokio_util::codec::Decoder::Error and Encoder::Error both require diff --git a/crates/kerykeion/src/lib.rs b/crates/kerykeion/src/lib.rs index 706ff00..40bf567 100644 --- a/crates/kerykeion/src/lib.rs +++ b/crates/kerykeion/src/lib.rs @@ -22,6 +22,7 @@ //! - Gateway detection: [`gateway::GatewayDetector`] //! - Signal production: [`signals::MeshEvent`] //! - Message construction: [`message::MessageBuilder`] +//! - Monotonic outbound nonce sequencing: [`packet_id::PacketIdCounter`] //! - Outbound queue: [`outbound::OutboundQueue`] //! - Message routing: [`router::MeshRouter`] //! - Delivery tracking: [`delivery::DeliveryTracker`] @@ -43,6 +44,7 @@ pub mod message; pub mod mqtt; pub mod node_db; pub mod outbound; +pub mod packet_id; pub mod processor; pub mod router; pub mod signals; @@ -78,6 +80,7 @@ pub use message::MessageBuilder; pub use mqtt::{GatewayInfo, ParsedMapReport}; pub use node_db::{DeviceMetrics, MeshNode, NodeDb, NodePosition, UserInfo}; pub use outbound::{InflightMessage, OutboundQueue, PendingMessage}; +pub use packet_id::PacketIdCounter; pub use processor::{PacketProcessor, RoutingProcessor, RoutingResult}; pub use proto::{FromRadio, ToRadio}; pub use router::{MeshRouter, SendOptions}; @@ -85,8 +88,8 @@ pub use signals::{MeshEvent, mesh_event_to_signal}; pub use store_forward::{StoreForward, StoredMessage}; pub use topology::{LinkQuality, MeshTopology, TopologySnapshot}; pub use types::{ - BROADCAST_ADDR, ChannelIndex, FRAME_MAGIC, MAX_CHANNELS, MAX_HOP_LIMIT, MAX_PACKET_SIZE, - MeshChannelId, NodeIdStr, NodeNum, PacketId, + BROADCAST_ADDR, ChannelIndex, FRAME_MAGIC, MAX_CHANNELS, MAX_HOP_LIMIT, MAX_LIVE_LINKS, + MAX_LIVE_NODES, MAX_PACKET_SIZE, MeshChannelId, NodeIdStr, NodeNum, PacketId, }; #[cfg(test)] diff --git a/crates/kerykeion/src/message.rs b/crates/kerykeion/src/message.rs index 410c50d..0866207 100644 --- a/crates/kerykeion/src/message.rs +++ b/crates/kerykeion/src/message.rs @@ -1,11 +1,11 @@ //! Outbound message construction for Meshtastic mesh packets. use prost::Message as _; -use rand_core::{OsRng, RngCore as _}; use crate::config::MessageConfig; use crate::crypto; use crate::error::Error; +use crate::packet_id::PacketIdCounter; use crate::proto::mesh_packet::Priority; use crate::proto::{AdminMessage, Data, MeshPacket, PortNum, Position, mesh_packet}; use crate::types::{ChannelIndex, MAX_HOP_LIMIT, NodeNum}; @@ -17,9 +17,10 @@ use crate::types::{ChannelIndex, MAX_HOP_LIMIT, NodeNum}; /// # Examples /// /// ```ignore +/// let mut packet_ids = PacketIdCounter::resume(persisted_last_id); /// let packet = MessageBuilder::text(NodeNum(0x1234), "hello") /// .with_ack() -/// .build(NodeNum(0xABCD), &[0x01])?; +/// .build(NodeNum(0xABCD), &[0x01], &mut packet_ids)?; /// ``` pub struct MessageBuilder { dest: NodeNum, @@ -157,14 +158,23 @@ impl MessageBuilder { /// Consume the builder and produce an encrypted [`MeshPacket`]. /// - /// A random `packet_id` is assigned. The payload is encrypted using - /// AES-CTR with the provided PSK. + /// `packet_id` is drawn FROM `packet_ids` (see [`PacketIdCounter`] — it + /// doubles as the AES-CTR nonce counter for this PSK, so its + /// non-repetition guarantee is what keeps the nonce from repeating; + /// see #209). The payload is encrypted using AES-CTR with the provided PSK. /// /// # Errors /// - /// Returns [`Error::Encryption`] if encryption fails (e.g. invalid PSK length). - pub fn build(self, from: NodeNum, psk: &[u8]) -> Result { - let packet_id = OsRng.next_u32(); + /// Returns [`Error::Encryption`] if encryption fails (e.g. invalid PSK + /// length). Returns [`Error::PacketIdSpaceExhausted`] if `packet_ids` + /// has issued every value in its space — see [`PacketIdCounter::next_id`]. + pub fn build( + self, + from: NodeNum, + psk: &[u8], + packet_ids: &mut PacketIdCounter, + ) -> Result { + let packet_id = packet_ids.next_id()?; let data = Data { portnum: i32::from(self.portnum), @@ -206,7 +216,10 @@ impl MessageBuilder { /// Consume the builder and produce an encrypted [`MeshPacket`] with a deterministic packet ID. /// - /// Useful for testing. Production code should use [`build`](Self::build). + /// Test-only: deliberately bypasses [`PacketIdCounter`] to let a test + /// pick an exact `packet_id`/nonce. Production code MUST use + /// [`build`](Self::build) — a caller reachable outside `#[cfg(test)]` + /// has no such bypass. /// /// # Errors /// @@ -267,7 +280,7 @@ mod tests { fn text_message_sets_portnum_and_encrypts() { #[expect(clippy::unwrap_used, reason = "test-only")] let pkt = MessageBuilder::text(DEST, "hello mesh") - .build(FROM_NODE, &[0x01]) + .build(FROM_NODE, &[0x01], &mut PacketIdCounter::resume(0)) .unwrap(); assert_eq!(pkt.from, FROM_NODE.0); @@ -284,7 +297,7 @@ mod tests { fn text_message_unencrypted_when_empty_psk() { #[expect(clippy::unwrap_used, reason = "test-only")] let pkt = MessageBuilder::text(DEST, "cleartext") - .build(FROM_NODE, &[]) + .build(FROM_NODE, &[], &mut PacketIdCounter::resume(0)) .unwrap(); assert!( @@ -297,7 +310,7 @@ mod tests { fn position_message_encodes_lat_lon() { #[expect(clippy::unwrap_used, reason = "test-only")] let pkt = MessageBuilder::position(DEST, 37.7749, -122.4194) - .build(FROM_NODE, &[]) + .build(FROM_NODE, &[], &mut PacketIdCounter::resume(0)) .unwrap(); let Some(PayloadVariant::Decoded(data)) = &pkt.payload_variant else { @@ -321,7 +334,7 @@ mod tests { }; #[expect(clippy::unwrap_used, reason = "test-only")] let pkt = MessageBuilder::admin(DEST, &admin) - .build(FROM_NODE, &[]) + .build(FROM_NODE, &[], &mut PacketIdCounter::resume(0)) .unwrap(); assert_eq!(pkt.priority, i32::from(Priority::Reliable)); assert!(pkt.want_ack, "admin messages should request ACK"); @@ -331,7 +344,7 @@ mod tests { fn traceroute_uses_max_hop_limit() { #[expect(clippy::unwrap_used, reason = "test-only")] let pkt = MessageBuilder::traceroute(DEST) - .build(FROM_NODE, &[]) + .build(FROM_NODE, &[], &mut PacketIdCounter::resume(0)) .unwrap(); assert_eq!(pkt.hop_limit, u32::from(MAX_HOP_LIMIT)); assert!(pkt.want_ack, "traceroute should request ACK"); @@ -345,7 +358,7 @@ mod tests { .with_ack() .hop_limit(5) .priority(Priority::Reliable) - .build(FROM_NODE, &[]) + .build(FROM_NODE, &[], &mut PacketIdCounter::resume(0)) .unwrap(); assert_eq!(pkt.channel, 2); @@ -359,7 +372,7 @@ mod tests { #[expect(clippy::unwrap_used, reason = "test-only")] let pkt = MessageBuilder::text(DEST, "test") .hop_limit(100) - .build(FROM_NODE, &[]) + .build(FROM_NODE, &[], &mut PacketIdCounter::resume(0)) .unwrap(); assert_eq!(pkt.hop_limit, u32::from(MAX_HOP_LIMIT)); } @@ -374,7 +387,7 @@ mod tests { }; #[expect(clippy::unwrap_used, reason = "test-only")] let pkt = MessageBuilder::text_with_config(DEST, "test", &cfg) - .build(FROM_NODE, &[]) + .build(FROM_NODE, &[], &mut PacketIdCounter::resume(0)) .unwrap(); assert_eq!(pkt.hop_limit, 1); assert_eq!(pkt.hop_start, 1); @@ -390,7 +403,7 @@ mod tests { }; #[expect(clippy::unwrap_used, reason = "test-only")] let pkt = MessageBuilder::text_with_config(DEST, "test", &cfg) - .build(FROM_NODE, &[]) + .build(FROM_NODE, &[], &mut PacketIdCounter::resume(0)) .unwrap(); assert_eq!(pkt.hop_limit, u32::from(MAX_HOP_LIMIT)); } @@ -410,7 +423,11 @@ mod tests { // 1..=10 channel index resolves to itself, so a 3-byte PSK reaches // AES-CTR as an invalid key length. `build` must surface that as // Error::Encryption rather than emitting an unencrypted packet. - let result = MessageBuilder::text(DEST, "test").build(FROM_NODE, &[0xAA, 0xBB, 0xCC]); + let result = MessageBuilder::text(DEST, "test").build( + FROM_NODE, + &[0xAA, 0xBB, 0xCC], + &mut PacketIdCounter::resume(0), + ); assert!( matches!(result, Err(Error::Encryption { .. })), @@ -425,7 +442,7 @@ mod tests { // Without this the test above would pass even if `build` always failed. #[expect(clippy::unwrap_used, reason = "test-only")] let pkt = MessageBuilder::text(DEST, "test") - .build(FROM_NODE, &[0x11; 16]) + .build(FROM_NODE, &[0x11; 16], &mut PacketIdCounter::resume(0)) .unwrap(); assert!(matches!( @@ -433,4 +450,41 @@ mod tests { Some(mesh_packet::PayloadVariant::Encrypted(_)) )); } + + #[test] + fn sequential_builds_never_share_a_packet_id() { + // WHY(#209): the issue's literal Done-when — `build` is the shipped + // production entry point, and it must draw `packet_id` from a + // shared, advancing counter rather than an independent random draw + // per call, or two packets can carry the same AES-CTR nonce. + let mut ids = PacketIdCounter::resume(0); + #[expect(clippy::unwrap_used, reason = "test-only")] + let first = MessageBuilder::text(DEST, "one") + .build(FROM_NODE, &[0x01], &mut ids) + .unwrap(); + #[expect(clippy::unwrap_used, reason = "test-only")] + let second = MessageBuilder::text(DEST, "two") + .build(FROM_NODE, &[0x01], &mut ids) + .unwrap(); + + assert_ne!( + first.id, second.id, + "two builds sharing one counter must not share a packet_id/nonce" + ); + assert_eq!(second.id, first.id + 1); + } + + #[test] + fn build_surfaces_packet_id_space_exhaustion() { + // WHY(#209): `build` must propagate the counter's refusal rather + // than silently wrapping the nonce — see `packet_id::tests::next_refuses_to_wrap_past_u32_max` + // for the underlying counter behavior this exercises through the + // production entry point. + let mut ids = PacketIdCounter::resume(u32::MAX); + let result = MessageBuilder::text(DEST, "test").build(FROM_NODE, &[0x01], &mut ids); + assert!( + matches!(result, Err(Error::PacketIdSpaceExhausted { .. })), + "build must surface exhaustion rather than emit a wrapped packet_id, got {result:?}" + ); + } } diff --git a/crates/kerykeion/src/node_db.rs b/crates/kerykeion/src/node_db.rs index 173525b..c3eccd5 100644 --- a/crates/kerykeion/src/node_db.rs +++ b/crates/kerykeion/src/node_db.rs @@ -6,7 +6,7 @@ use std::time::Duration; use jiff::Timestamp; use serde::{Deserialize, Serialize}; -use crate::types::{NodeIdStr, NodeNum}; +use crate::types::{MAX_LIVE_NODES, NodeIdStr, NodeNum}; /// In-memory store of all mesh nodes seen during a session. #[derive(Debug, Default, Clone)] @@ -105,10 +105,47 @@ impl NodeDb { } /// Inserts or replaces a node record. + /// + /// If `node.num` is not already tracked and the table is at + /// [`MAX_LIVE_NODES`], the least-recently-heard tracked node is evicted + /// first (#204) — `from` on an inbound frame is unauthenticated, so an + /// OTA peer can announce unbounded distinct identities without this. pub fn insert(&mut self, node: MeshNode) { + if !self.nodes.contains_key(&node.num) && self.nodes.len() >= MAX_LIVE_NODES { + self.evict_stalest(); + } self.nodes.insert(node.num, node); } + /// Remove the least-recently-heard tracked node to make room for an + /// insertion, protecting [`Self::my_node`] — the local radio's own + /// identity — from eviction. + /// + // WHY least-recently-heard rather than insertion order or a + // hash/id-derived victim (#204): both of those give an attacker a + // predictable target — flood enough distinct fake identities and the + // Nth-inserted (or lowest-hashing) *real* node is evicted on schedule. + // Staleness-by-`last_heard` means only entries an attacker themselves + // stopped refreshing become evictable; a sustained flood of one-shot + // identities degrades to evicting the flood's own earlier entries, and + // any legitimate node that keeps transmitting keeps refreshing its + // `last_heard` and stays out of eviction range. + // + // WARNING: if every tracked entry is protected (`my_node` is the only + // entry) this is a no-op and `insert` grows one past the cap — not + // reachable in practice since MAX_LIVE_NODES is far above a table of one. + fn evict_stalest(&mut self) { + let victim = self + .nodes + .iter() + .filter(|&(&num, _)| Some(num) != self.my_node) + .min_by_key(|&(_, node)| node.last_heard.map_or(i64::MIN, Timestamp::as_millisecond)) + .map(|(&num, _)| num); + if let Some(victim) = victim { + self.nodes.remove(&victim); + } + } + /// Returns a reference to the node with the given number, if present. #[must_use] pub fn get(&self, num: NodeNum) -> Option<&MeshNode> { @@ -223,4 +260,73 @@ mod tests { assert_eq!(db.len(), 1); assert_eq!(db.get(NodeNum(1)).and_then(|n| n.snr), Some(4.5)); } + + fn make_node_heard(num: u32, secs: i64) -> MeshNode { + let mut node = make_node(num); + #[expect(clippy::unwrap_used, reason = "test-only: secs is a small fixed value")] + { + node.last_heard = Some(Timestamp::from_second(secs).unwrap()); + } + node + } + + #[test] + fn insert_bounds_live_cardinality_at_the_cap() { + // WHY(#204): `from` on an inbound frame is unauthenticated, so an + // OTA peer announcing MAX_LIVE_NODES+N distinct identities must + // never grow the table past the cap. + let mut db = NodeDb::new(); + for i in 0..(MAX_LIVE_NODES as u32 + 500) { + db.insert(make_node(i)); + } + assert!( + db.len() <= MAX_LIVE_NODES, + "len()={} exceeds MAX_LIVE_NODES={MAX_LIVE_NODES}", + db.len() + ); + } + + #[test] + fn insert_evicts_the_stalest_node_and_protects_my_node() { + let mut db = NodeDb::new(); + let my_num = NodeNum(0xAAAA); + db.set_my_node(my_num); + // my_node is the freshest entry — if eviction ever picked it despite + // that, this test still would not catch a staleness-ordering bug; + // the explicit `my_node` protection is what's under test here. + db.insert(make_node_heard(my_num.0, 1_000_000_000)); + + // Fill to the cap with MAX_LIVE_NODES-1 more distinct, strictly + // increasing-freshness nodes (node `i` has timestamp `i`), so node 0 + // is the single stalest entry and no eviction has fired yet. + for i in 0..(MAX_LIVE_NODES as u32 - 1) { + db.insert(make_node_heard(i + 1, i64::from(i))); + } + assert_eq!(db.len(), MAX_LIVE_NODES, "setup must reach the cap"); + assert!( + db.get(NodeNum(1)).is_some(), + "setup must not have evicted node 1 yet" + ); + + db.insert(make_node_heard(0xFFFF, 2_000_000_000)); + + assert!( + db.get(NodeNum(1)).is_none(), + "the stalest node (timestamp 0) must be the one evicted" + ); + assert_eq!( + db.my_node(), + Some(my_num), + "my_node identity must survive eviction pressure" + ); + assert!( + db.get(my_num).is_some(), + "my_node's own record must survive" + ); + assert!( + db.get(NodeNum(0xFFFF)).is_some(), + "the new node must be present" + ); + assert_eq!(db.len(), MAX_LIVE_NODES); + } } diff --git a/crates/kerykeion/src/packet_id.rs b/crates/kerykeion/src/packet_id.rs new file mode 100644 index 0000000..0b126a8 --- /dev/null +++ b/crates/kerykeion/src/packet_id.rs @@ -0,0 +1,164 @@ +//! Monotonic outbound packet-id sequencing for AES-CTR nonce uniqueness (#209). +//! +//! [`crate::message::MessageBuilder::build`] feeds `packet_id` straight into +//! `crypto::build_nonce` as the AES-CTR nonce material for the sender's PSK +//! (`crypto.rs`'s module doc has the exact byte layout). A value drawn +//! independently per packet gives no non-repetition guarantee — two packets +//! sharing a `(packet_id, from)` pair XOR their plaintexts together under +//! CTR's keystream reuse. [`PacketIdCounter`] replaces that with a value +//! that only ever increases for the lifetime of one instance. + +use rand_core::{OsRng, RngCore as _}; + +use crate::Error; +use crate::error::PacketIdSpaceExhaustedSnafu; + +/// Monotonic sequence generator for outbound `packet_id` / AES-CTR nonce values. +/// +/// # Persistence — read before wiring this into a long-running radio +/// +/// WARNING: [`Self::seed_random`] draws a fresh random starting point and +/// carries no memory of ids a prior instance issued. Calling it on every +/// process start reproduces the exact defect this type exists to close: a +/// sequence that is monotonic *within one run* but restarts at an +/// independent random point on every reboot gives the same non-guarantee +/// the bare `OsRng.next_u32()`-per-packet call did (#209), just redrawn once +/// per process instead of once per packet. Call `seed_random` only when no +/// persisted value exists for this identity/PSK — typically first-ever +/// provisioning. On every subsequent start, persist [`Self::current`] after +/// each [`Self::next`] and reconstruct via [`Self::resume`] instead. +#[derive(Debug)] +pub struct PacketIdCounter { + /// The most recently issued id, or the resume point if none has been + /// issued by this instance yet. `None` only before the very first + /// `seed_random`/`resume` call — the public constructors never leave it + /// unset, so [`Self::current`] is total once a counter exists. + last_issued: u32, +} + +impl PacketIdCounter { + /// Start a fresh sequence from a random 32-bit seed. + /// + /// WARNING: valid only when no persisted counter value exists for this + /// identity/PSK — see the type-level docs. + #[must_use] + pub fn seed_random() -> Self { + Self { + last_issued: OsRng.next_u32(), + } + } + + /// Resume a sequence from the last id a prior instance issued, as + /// returned by [`Self::current`] and persisted by the caller (e.g. on + /// process shutdown, or after every send if crash-safety across an + /// unclean exit matters more than the write cost). + /// + /// The first subsequent [`Self::next`] call returns `last_used + 1`, + /// never `last_used` itself or a fresh random value — this is what + /// prevents a restart from repeating a nonce this key has already used. + #[must_use] + pub const fn resume(last_used: u32) -> Self { + Self { + last_issued: last_used, + } + } + + /// The most recently issued id. Callers persist this after every + /// [`Self::next_id`] and pass it to [`Self::resume`] on the next start. + #[must_use] + pub const fn current(&self) -> u32 { + self.last_issued + } + + /// Issue the next id in the sequence. + /// + /// # Errors + /// + /// Returns [`Error::PacketIdSpaceExhausted`] instead of wrapping past + /// `u32::MAX` back toward `0`. A silent wrap would revisit a value this + /// instance (or, via `resume`, a prior one) may already have issued + /// under the same key — reproducing #209 at the 32-bit boundary instead + /// of the birthday bound the raw-random design failed at. Exhaustion + /// means the PSK must rotate; retrying `next_id` cannot recover. + // + // WHY not named `next`: `clippy::should_implement_trait` -- a + // `Result`-returning `next(&mut self)` reads as `Iterator::next` + // (`Option`-returning) and invites exactly that confusion. + pub fn next_id(&mut self) -> Result { + let next = self + .last_issued + .checked_add(1) + .ok_or_else(|| PacketIdSpaceExhaustedSnafu.build())?; + self.last_issued = next; + Ok(next) + } +} + +#[cfg(test)] +mod tests { + use std::collections::HashSet; + + use super::*; + + #[test] + fn next_is_strictly_increasing_and_never_repeats() { + let mut counter = PacketIdCounter::resume(0); + let mut seen = HashSet::new(); + let mut prev = 0u32; + for i in 0..10_000u32 { + #[expect(clippy::unwrap_used, reason = "test-only: far from u32::MAX")] + let id = counter.next_id().unwrap(); + assert!(seen.insert(id), "packet id {id} repeated at iteration {i}"); + if i > 0 { + assert_eq!(id, prev + 1, "counter must advance by exactly 1 per call"); + } + prev = id; + } + } + + #[test] + fn resume_continues_from_the_persisted_value_not_from_zero() { + // WHY(#209): this is the restart-repro case named in the issue — a + // counter that forgets what it already issued and restarts at zero + // (or anywhere below the persisted point) can reissue an id, and + // hence a nonce, this key has already used. + let persisted_last_used = 4_242u32; + let mut resumed = PacketIdCounter::resume(persisted_last_used); + #[expect(clippy::unwrap_used, reason = "test-only")] + let first_after_restart = resumed.next_id().unwrap(); + assert_eq!( + first_after_restart, + persisted_last_used + 1, + "resume must continue past the persisted value, never reissue it or reset" + ); + assert_ne!( + first_after_restart, 0, + "resume must not behave like a fresh/unpersisted seed" + ); + } + + #[test] + fn next_refuses_to_wrap_past_u32_max() { + // WHY(#209): the other failure mode named in the issue. Wrapping + // silently back to 0 would reissue the lowest ids this instance + // already used under the same key. + let mut counter = PacketIdCounter::resume(u32::MAX - 1); + #[expect(clippy::unwrap_used, reason = "test-only: one below the ceiling")] + let one_below_ceiling = counter.next_id().unwrap(); + assert_eq!(one_below_ceiling, u32::MAX); + assert!( + matches!(counter.next_id(), Err(Error::PacketIdSpaceExhausted { .. })), + "next_id() must refuse rather than wrap to a low, already-issued value" + ); + } + + #[test] + fn seed_random_two_instances_start_from_different_points() { + // WHY: not a strong cryptographic proof (a collision is astronomically + // unlikely, not impossible) — a weak smoke check that seed_random is + // actually drawing from the RNG rather than a fixed constant. + let a = PacketIdCounter::seed_random(); + let b = PacketIdCounter::seed_random(); + assert_ne!(a.current(), b.current()); + } +} diff --git a/crates/kerykeion/src/processor.rs b/crates/kerykeion/src/processor.rs index 80470a0..1dc4deb 100644 --- a/crates/kerykeion/src/processor.rs +++ b/crates/kerykeion/src/processor.rs @@ -474,6 +474,23 @@ pub enum RoutingResult { /// The routing error code. error: routing::Error, }, + /// The `ROUTING_APP` packet decoded and carried an `error_reason`, but + /// its wire value is not among the `routing::Error` variants this build + /// knows. + /// + // WHY a distinct variant rather than folding into `Nak` or `Ack` (#208): + // `routing::Error` cannot represent "unrecognized" without reusing an + // existing code, which would misreport the failure reason — and reusing + // `Error::None` (the pre-fix `unwrap_or` fallback) is exactly the + // fail-open defect this variant exists to prevent. MUST NEVER be treated + // as delivery confirmation: an out-of-enum code is exactly what a + // forged NAK or an unrecognized future firmware code looks like. + UnknownError { + /// The packet ID the unrecognized error was reported against. + request_id: PacketId, + /// The raw wire code that did not match any known `routing::Error` variant. + code: i32, + }, /// The packet was not a routing packet or had no actionable variant. NotRouting, } @@ -514,19 +531,31 @@ impl RoutingProcessor { }; match routing_msg.variant { - Some(routing::Variant::ErrorReason(code)) => { - let error = routing::Error::try_from(code).unwrap_or(routing::Error::None); - if error == routing::Error::None { - RoutingResult::Ack { - request_id: PacketId(request_id), - } - } else { - RoutingResult::Nak { + // WHY match on the decode result directly rather than + // `unwrap_or(routing::Error::None)` (#208): the prior fallback + // read ANY unrecognized code as `Error::None`, i.e. delivery + // confirmed. Only an EXPLICITLY decoded `Error::None` may ACK; + // an out-of-enum code falls through to `UnknownError`, never `Ack`. + Some(routing::Variant::ErrorReason(code)) => match routing::Error::try_from(code) { + Ok(routing::Error::None) => RoutingResult::Ack { + request_id: PacketId(request_id), + }, + Ok(error) => RoutingResult::Nak { + request_id: PacketId(request_id), + error, + }, + Err(_) => { + tracing::warn!( + packet_id = packet.id, + code, + "unrecognized routing error code; treating as undelivered, not ACK" + ); + RoutingResult::UnknownError { request_id: PacketId(request_id), - error, + code, } } - } + }, _ => RoutingResult::NotRouting, } } @@ -556,6 +585,25 @@ impl RoutingProcessor { delivery.mark_failed(*request_id, DeliveryFailure::Nak(*error)); } } + RoutingResult::UnknownError { request_id, code } => { + // WHY the same retry/fail pipeline as `Nak`, not a silent + // drop (#208): an unrecognized code is still evidence the + // packet was NOT delivered — treating it as inert would + // leave `outbound`'s inflight slot and `delivery`'s record + // stuck until TTL/timeout instead of retrying or failing + // promptly, and would never surface the unrecognized code. + tracing::debug!( + packet_id = %request_id, + code, + "delivery NAK received (unrecognized routing error code)" + ); + let retried = outbound.handle_nak(*request_id); + if retried { + delivery.record_retry(*request_id); + } else { + delivery.mark_failed(*request_id, DeliveryFailure::UnknownNak { code: *code }); + } + } RoutingResult::NotRouting => {} } } diff --git a/crates/kerykeion/src/processor_tests.rs b/crates/kerykeion/src/processor_tests.rs index 5cc0177..44ec6c1 100644 --- a/crates/kerykeion/src/processor_tests.rs +++ b/crates/kerykeion/src/processor_tests.rs @@ -318,6 +318,63 @@ fn nak_max_retransmit_detected() { ); } +#[test] +fn unrecognized_error_code_is_never_classified_as_ack() { + // WHY(#208): the fail-open defect. Neither `NOT_AUTHORIZED` (33) nor any + // code above it is defined in the vendored routing::Error enum, so 999 + // is guaranteed out-of-enum without depending on the proto staying + // fixed at its current variant count. The pre-fix + // `unwrap_or(routing::Error::None)` fallback read this as `Error::None` + // -> Ack, i.e. delivery confirmed for a code the build never decoded. + let pkt = make_routing_packet(0x9999, 999); + let result = RoutingProcessor::process_routing(&pkt); + assert_ne!( + result, + RoutingResult::Ack { + request_id: PacketId(0x9999) + }, + "an unrecognized routing error code must never read as delivery confirmation" + ); + assert_eq!( + result, + RoutingResult::UnknownError { + request_id: PacketId(0x9999), + code: 999, + } + ); +} + +#[test] +fn apply_unknown_error_marks_failed_when_no_inflight_and_never_acks() { + // WHY(#208): exercises the write path, not just the classification — + // `apply_routing_result` must route `UnknownError` through the same + // not-delivered pipeline as `Nak`, never call `mark_acknowledged`. + let mut delivery = DeliveryTracker::new(); + let mut outbound = OutboundQueue::new(); + let id = PacketId(0xBEEF); + + delivery.track(id, 0x5678); + delivery.mark_sent(id); + + let result = RoutingResult::UnknownError { + request_id: id, + code: 999, + }; + RoutingProcessor::apply_routing_result(&result, &mut delivery, &mut outbound); + + assert!( + !matches!( + delivery.delivery_status(id), + Some(crate::delivery::DeliveryStatus::Acknowledged { .. }) + ), + "an unrecognized routing error code must never mark delivery acknowledged" + ); + assert!(matches!( + delivery.delivery_status(id), + Some(crate::delivery::DeliveryStatus::Failed { .. }) + )); +} + #[test] fn non_routing_packet_ignored() { let data = Data { diff --git a/crates/kerykeion/src/topology.rs b/crates/kerykeion/src/topology.rs index 5df4e0f..6d5f5a2 100644 --- a/crates/kerykeion/src/topology.rs +++ b/crates/kerykeion/src/topology.rs @@ -12,22 +12,10 @@ use serde::{Deserialize, Serialize}; use tokio::time::Instant; use crate::config::TopologyConfig; -use crate::types::NodeNum; +use crate::types::{MAX_LIVE_LINKS, MAX_LIVE_NODES, NodeNum}; // Historical default (30.0) now lives in [`TopologyConfig::default`]. -/// Maximum nodes accepted from a persisted topology snapshot. -/// -// WHY: `load_from_bytes` allocates one graph node per entry from a file that -// may be truncated, corrupt or attacker-written. A Meshtastic node DB holds low -// hundreds of nodes, so this ceiling is far above any real mesh while still -// bounding the allocation. Exceeding it is recoverable: passive learning -// re-observes live links, so a truncated restore self-heals. -const MAX_SNAPSHOT_NODES: usize = 4096; - -/// Maximum links accepted from a persisted topology snapshot. -const MAX_SNAPSHOT_LINKS: usize = 16384; - /// Directed edge weight representing radio link quality between two nodes. #[derive(Debug, Clone)] pub struct LinkQuality { @@ -64,15 +52,89 @@ impl MeshTopology { } /// Insert a node or return its existing index. + /// + /// If `node` is not already tracked and the graph is at + /// [`MAX_LIVE_NODES`], the coldest tracked node is evicted first (#204). pub fn add_node(&mut self, node: NodeNum) -> NodeIndex { + self.add_node_protecting(node, &[]) + } + + /// [`Self::add_node`], excluding `protect` from eviction candidacy. + /// + // WHY this split exists (#204 self-eviction, caught in review before + // shipping): `update_link` must add TWO nodes (`from` and `to`) before + // it can create their edge. A node that was JUST inserted by the first + // call has zero edges yet — `freshness` reports `None`, the coldest + // possible key — so a second, independent `add_node` call for the + // other endpoint could evict the first one before the edge is ever + // created, leaving a dangling `NodeIndex` and panicking + // `StableGraph::add_edge`. Protecting the sibling endpoint closes that. + // See `update_link_never_evicts_its_own_two_new_endpoints`. + fn add_node_protecting(&mut self, node: NodeNum, protect: &[NodeNum]) -> NodeIndex { if let Some(&idx) = self.node_index.get(&node) { return idx; } + if self.node_index.len() >= MAX_LIVE_NODES { + self.evict_coldest_node(protect); + } let idx = self.graph.add_node(node); self.node_index.insert(node, idx); idx } + /// Most recent `last_observed` across all of `node`'s edges (either + /// direction), or `None` if it has none. + /// + // WHY `None` sorts coldest via `Option`'s derived `Ord` (`None < Some(_)`): + // this matches `remove_stale_nodes`'s existing "no edges is stale" rule + // rather than introducing a second policy for the same question. + fn freshness(&self, idx: NodeIndex) -> Option { + self.graph + .edges_directed(idx, Direction::Incoming) + .chain(self.graph.edges_directed(idx, Direction::Outgoing)) + .map(|e| e.weight().last_observed) + .max() + } + + /// Remove the coldest tracked node not in `protect` to make room for an insertion. + /// + // WHY freshness-by-edge-activity rather than insertion order (#204): an + // attacker who knows the eviction policy could target a specific real + // node by insertion position; picking the coldest node instead means an + // attacker can only ever evict entries THEY stopped refreshing (their + // own flood, once it exceeds the cap) or a real node that has + // genuinely gone quiet — the same tradeoff `remove_stale_nodes` already + // makes on a timer. No explicit "protect my own identity" field exists + // on `MeshTopology` (unlike `NodeDb::my_node`): the local radio's own + // node is the target of every direct-neighbor `update_link` call + // (`processor::apply_passive_learning`), so its edges are refreshed on + // essentially every received packet and it naturally stays warm. + fn evict_coldest_node(&mut self, protect: &[NodeNum]) { + let victim = self + .node_index + .iter() + .filter(|&(num, _)| !protect.contains(num)) + .min_by_key(|&(_, &idx)| self.freshness(idx)) + .map(|(&num, &idx)| (num, idx)); + if let Some((num, idx)) = victim { + self.node_index.remove(&num); + self.graph.remove_node(idx); + } + } + + /// Remove the coldest tracked edge to make room for a new one. + fn evict_coldest_edge(&mut self) { + let victim = self + .graph + .edge_indices() + .filter_map(|idx| self.graph.edge_weight(idx).map(|w| (idx, w.last_observed))) + .min_by_key(|&(_, last_observed)| last_observed) + .map(|(idx, _)| idx); + if let Some(idx) = victim { + self.graph.remove_edge(idx); + } + } + /// Add or update a directed edge from `from` to `to` with the given SNR. pub fn update_link(&mut self, from: NodeNum, to: NodeNum, snr: f32) { // WHY: a non-finite SNR (NaN/Inf from OTA protobuf) corrupts astar @@ -90,8 +152,11 @@ impl MeshTopology { return; } - let from_idx = self.add_node(from); - let to_idx = self.add_node(to); + // WHY `add_node_protecting` (not `add_node`) with each other as the + // protected node: see the WHY on `add_node_protecting` — the second + // call must not evict the node the first call just inserted. + let from_idx = self.add_node_protecting(from, &[to]); + let to_idx = self.add_node_protecting(to, &[from]); // WHY: search existing edges to update rather than create duplicates. let existing = self @@ -107,6 +172,12 @@ impl MeshTopology { weight.packet_count = weight.packet_count.saturating_add(1); } } else { + // WHY: a NEW edge is what grows cardinality (#204) — an update to + // an existing edge (the branch above) never does, so the cap + // check belongs only here. + if self.graph.edge_count() >= MAX_LIVE_LINKS { + self.evict_coldest_edge(); + } self.graph.add_edge( from_idx, to_idx, @@ -386,7 +457,10 @@ impl MeshTopology { /// Restore topology from a serialized snapshot. All links are marked as observed now. /// /// Repeated `(from, to)` pairs are folded into a single edge, and the - /// restore is bounded at [`MAX_SNAPSHOT_NODES`] / [`MAX_SNAPSHOT_LINKS`]. + /// restore is bounded at [`MAX_LIVE_NODES`] / [`MAX_LIVE_LINKS`] — the + /// same live-cardinality ceiling [`Self::add_node`] / [`Self::update_link`] + /// enforce (#204), so a restored topology can never exceed what the live + /// insertion path would ever admit. /// /// # Errors /// @@ -400,26 +474,26 @@ impl MeshTopology { // WHY: never truncate silently - a restore that dropped half the mesh // without saying so reads as a small mesh rather than a bad snapshot. - if snapshot.nodes.len() > MAX_SNAPSHOT_NODES { + if snapshot.nodes.len() > MAX_LIVE_NODES { tracing::warn!( present = snapshot.nodes.len(), - cap = MAX_SNAPSHOT_NODES, + cap = MAX_LIVE_NODES, "topology snapshot exceeds node cap; restoring a prefix" ); } - if snapshot.links.len() > MAX_SNAPSHOT_LINKS { + if snapshot.links.len() > MAX_LIVE_LINKS { tracing::warn!( present = snapshot.links.len(), - cap = MAX_SNAPSHOT_LINKS, + cap = MAX_LIVE_LINKS, "topology snapshot exceeds link cap; restoring a prefix" ); } let mut topo = Self::new(); - for node in snapshot.nodes.iter().take(MAX_SNAPSHOT_NODES) { + for node in snapshot.nodes.iter().take(MAX_LIVE_NODES) { topo.add_node(*node); } - for link in snapshot.links.iter().take(MAX_SNAPSHOT_LINKS) { + for link in snapshot.links.iter().take(MAX_LIVE_LINKS) { let from_idx = topo.add_node(link.from); let to_idx = topo.add_node(link.to); diff --git a/crates/kerykeion/src/topology_tests.rs b/crates/kerykeion/src/topology_tests.rs index e0c62bb..0e9e01f 100644 --- a/crates/kerykeion/src/topology_tests.rs +++ b/crates/kerykeion/src/topology_tests.rs @@ -319,7 +319,7 @@ fn load_from_bytes_round_trips_without_multiplying_edges() { #[test] fn load_from_bytes_caps_nodes_and_links() { - let over = MAX_SNAPSHOT_NODES + 10; + let over = MAX_LIVE_NODES + 10; #[expect( clippy::cast_possible_truncation, reason = "test-only: indices are far below u32::MAX" @@ -335,7 +335,194 @@ fn load_from_bytes_caps_nodes_and_links() { assert_eq!( topo.node_count(), - MAX_SNAPSHOT_NODES, + MAX_LIVE_NODES, "restore must stop at the node cap" ); } + +#[test] +fn add_node_bounds_live_cardinality_at_the_cap() { + // WHY(#204): `from` on an inbound frame is unauthenticated, so a hostile + // peer announcing MAX_LIVE_NODES+N distinct identities via `add_node` + // (the LIVE ingestion path, not the snapshot-restore path the sibling + // test above already covered) must never grow the graph past the cap. + let mut topo = MeshTopology::new(); + for i in 0..(MAX_LIVE_NODES + 500) as u32 { + topo.add_node(n(i)); + } + assert!( + topo.node_count() <= MAX_LIVE_NODES, + "node_count()={} exceeds MAX_LIVE_NODES={MAX_LIVE_NODES}", + topo.node_count() + ); +} + +#[test] +fn update_link_bounds_live_edge_cardinality_at_the_cap() { + // WHY(#204): one NEIGHBORINFO frame lets an attacker assert edges + // between arbitrary node-id pairs it invents; distinct pairs must not + // grow the edge set past the cap. Uses a small, FIXED node set (K*K + // ordered pairs give far more than MAX_LIVE_LINKS distinct edges) so + // the independent, much stricter node cap never triggers and this test + // isolates the edge cap specifically. + let mut topo = MeshTopology::new(); + let k: u32 = 200; // 200*199 ordered pairs (39_800) >> MAX_LIVE_LINKS+500 + let mut created = 0usize; + 'outer: for i in 0..k { + for j in 0..k { + if i == j { + continue; + } + topo.update_link(n(i), n(j), 1.0); + created += 1; + if created >= MAX_LIVE_LINKS + 500 { + break 'outer; + } + } + } + assert!( + topo.node_count() <= k as usize, + "node set must stay well under its own cap for this test to isolate the edge cap" + ); + assert!( + topo.edge_count() <= MAX_LIVE_LINKS, + "edge_count()={} exceeds MAX_LIVE_LINKS={MAX_LIVE_LINKS}", + topo.edge_count() + ); +} + +#[test] +fn add_node_evicts_the_coldest_node_not_the_newest() { + // WHY(#204): the eviction policy must not simply refuse growth (which + // would let a flood permanently lock out real, later-observed nodes) — + // it must make room by removing the entry with the OLDEST activity, so + // a node that keeps transmitting is never the one an attacker's flood + // pushes out. + let mut topo = MeshTopology::new(); + let hub = n(u32::MAX); + // WHY exactly MAX_LIVE_NODES-1 iterations: each touches one cold node + // plus the shared hub, so this fills to EXACTLY the cap (cold nodes + + // hub) with no eviction yet triggered — verified below — leaving n(0) + // as the single oldest-touched entry and `hub` as the freshest. + for i in 0..(MAX_LIVE_NODES as u32 - 1) { + topo.update_link(n(i), hub, 1.0); + } + assert_eq!( + topo.node_count(), + MAX_LIVE_NODES, + "setup must reach the cap with no eviction yet" + ); + assert!(topo.contains_node(n(0)), "setup must not have evicted n(0)"); + + topo.add_node(n(MAX_LIVE_NODES as u32)); + + assert!( + !topo.contains_node(n(0)), + "the coldest (least-recently touched) node must be the one evicted" + ); + assert!( + topo.contains_node(hub), + "the freshest hub node must survive" + ); + assert!( + topo.contains_node(n(MAX_LIVE_NODES as u32)), + "the newly inserted node must be present" + ); + assert_eq!(topo.node_count(), MAX_LIVE_NODES); +} + +#[test] +fn update_link_never_evicts_its_own_two_new_endpoints() { + // WHY: regression, caught in review before shipping — `update_link` + // must insert TWO nodes (`from` and `to`) before their edge can exist. + // A node the FIRST insertion just added has zero edges yet + // (freshness=None, the coldest possible key), so an eviction triggered + // by the SECOND, independent insertion could pick the node the first + // one just added — before the edge between them is ever created — + // leaving a dangling index and panicking `StableGraph::add_edge`. + let mut topo = MeshTopology::new(); + let hub = n(u32::MAX); + for i in 0..(MAX_LIVE_NODES as u32 - 1) { + topo.update_link(n(i), hub, 1.0); + } + assert_eq!(topo.node_count(), MAX_LIVE_NODES); + + let from = n(MAX_LIVE_NODES as u32); + let to = n(MAX_LIVE_NODES as u32 + 1); + topo.update_link(from, to, 1.0); // must not panic + + assert!( + topo.contains_node(from), + "the edge's own source must survive its own insertion call" + ); + assert!(topo.contains_node(to), "the edge's own target must survive"); + assert!( + topo.neighbors(from).iter().any(|&(num, _)| num == to), + "the new edge between the two brand-new endpoints must exist" + ); + assert_eq!(topo.node_count(), MAX_LIVE_NODES); +} + +#[test] +fn load_from_bytes_never_evicts_its_own_two_new_endpoints() { + // WHY: regression sibling to `update_link_never_evicts_its_own_two_new_endpoints` + // (#204). `load_from_bytes`'s link-restore loop predates `add_node`'s + // eviction capability and was not re-audited when that capability was + // added: it called plain `add_node` for both of a link's endpoints + // instead of `add_node_protecting`, so the second `add_node` call for + // `to` could evict the node the first call just inserted for `from` -- + // before their edge exists -- leaving a dangling `NodeIndex` and + // panicking `StableGraph::add_edge`. + // + // Construction: a chain of `MAX_LIVE_NODES - 1` links spanning node ids + // `0..MAX_LIVE_NODES` fills the graph to exactly the cap with every + // node warmed by an edge (`freshness = Some(_)`), zero `None`-freshness + // entries. One more link between two brand-new ids then forces two + // evictions back to back while restoring: the first `add_node` call for + // that link evicts some warm chain node and inserts `from` -- now the + // UNIQUE `None`-freshness (zero-edge) entry, hence deterministically + // the coldest. The second `add_node` call for `to` then evicts `from` + // itself, and `add_edge` on the now-stale `from_idx` panics. + #[expect( + clippy::cast_possible_truncation, + reason = "test-only: indices are far below u32::MAX" + )] + let cap = MAX_LIVE_NODES as u32; + let mut links: Vec = (0..cap - 1) + .map(|i| LinkSnapshot { + from: n(i), + to: n(i + 1), + snr: 1.0, + packet_count: 1, + }) + .collect(); + links.push(LinkSnapshot { + from: n(cap), + to: n(cap + 1), + snr: 1.0, + packet_count: 1, + }); + let snapshot = TopologySnapshot { + nodes: Vec::new(), + links, + }; + let bytes = serde_json::to_vec(&snapshot).unwrap(); + + let topo = MeshTopology::load_from_bytes(&bytes).unwrap(); // must not panic + + assert_eq!(topo.node_count(), MAX_LIVE_NODES); + assert!( + topo.contains_node(n(cap)), + "the new link's own source must survive its own restore" + ); + assert!( + topo.contains_node(n(cap + 1)), + "the new link's own target must survive its own restore" + ); + assert!( + topo.neighbors(n(cap)) + .iter() + .any(|&(num, _)| num == n(cap + 1)), + "the new edge between the two brand-new endpoints must exist" + ); +} diff --git a/crates/kerykeion/src/types.rs b/crates/kerykeion/src/types.rs index 59f2b3c..857933e 100644 --- a/crates/kerykeion/src/types.rs +++ b/crates/kerykeion/src/types.rs @@ -149,6 +149,20 @@ pub const MAX_HOP_LIMIT: u8 = 7; /// Maximum protobuf payload size enforced by Meshtastic firmware. pub const MAX_PACKET_SIZE: usize = 512; +/// Hard ceiling on live-tracked node identities, shared by [`crate::node_db::NodeDb`] +/// and [`crate::topology::MeshTopology`] for both OTA-learned insertion and +/// persisted-snapshot restore. +/// +// WHY one shared constant rather than one per call site (#204): `from` on an +// inbound frame is unauthenticated, so nothing stops a hostile peer from +// announcing distinct node identities without bound; a real Meshtastic mesh +// runs low hundreds of nodes, so this ceiling is far above any real mesh +// while still bounding worst-case memory. Single fact, both structures derive. +pub const MAX_LIVE_NODES: usize = 4096; + +/// Hard ceiling on live-tracked topology links. See [`MAX_LIVE_NODES`]. +pub const MAX_LIVE_LINKS: usize = 16384; + /// Two-byte magic header that begins every Meshtastic serial frame. pub const FRAME_MAGIC: [u8; 2] = [0x94, 0xC3];