diff --git a/crates/kerykeion/src/collector.rs b/crates/kerykeion/src/collector.rs index d2df6bb..8fdaa72 100644 --- a/crates/kerykeion/src/collector.rs +++ b/crates/kerykeion/src/collector.rs @@ -30,7 +30,7 @@ use crate::router::MeshRouter; use crate::store_forward::StoreForward; use crate::topology::MeshTopology; use crate::transport::{self, ConnectionHandle}; -use crate::types::NodeNum; +use crate::types::ClaimedNodeNum; // Historical default (1 s) now lives in [`CollectorConfig::default`]. @@ -152,8 +152,23 @@ impl MeshCollector { } /// Handles a received mesh packet by updating the node database. + /// + /// `mesh_packet.from` is the sender this layer actually received the + /// packet attributed to — the strongest identity signal available here — + /// but it is an unauthenticated claim, not a verified fact (see + /// [`ClaimedNodeNum`]). A packet claiming the non-node sentinels (`0` or + /// broadcast) is dropped before it can create or update a node-DB entry + /// (#246). async fn handle_mesh_packet(&self, mesh_packet: &crate::proto::MeshPacket) { - let node_num = NodeNum(mesh_packet.from); + let Some(node_num) = + ClaimedNodeNum::from_wire(mesh_packet.from).map(ClaimedNodeNum::accept_unauthenticated) + else { + tracing::trace!( + from = mesh_packet.from, + "ignoring mesh packet with sentinel `from` (unset or broadcast)" + ); + return; + }; let snr = if mesh_packet.rx_snr == 0.0 { None } else { @@ -675,3 +690,7 @@ where #[cfg(test)] #[path = "collector_tests.rs"] mod tests; + +#[cfg(test)] +#[path = "collector_tests_attribution.rs"] +mod tests_attribution; diff --git a/crates/kerykeion/src/collector_tests_attribution.rs b/crates/kerykeion/src/collector_tests_attribution.rs new file mode 100644 index 0000000..e30930b --- /dev/null +++ b/crates/kerykeion/src/collector_tests_attribution.rs @@ -0,0 +1,129 @@ +//! Tests for [`super`]'s unauthenticated-attribution guard (#246); split out +//! from `collector_tests.rs` rather than added to it, which is already at the +//! RUST/file-too-long 800-line threshold. + +use super::*; +use crate::config::{ConnectionConfig, MeshConfig, StoreForwardConfig, TopologyConfig}; + +fn make_config(connections: Vec) -> MeshConfig { + MeshConfig { + connections, + store_forward: StoreForwardConfig::default(), + topology: TopologyConfig::default(), + ..MeshConfig::default() + } +} + +#[test] +fn claimed_node_num_rejects_zero_and_broadcast() { + assert!( + ClaimedNodeNum::from_wire(0).is_none(), + "from == 0 must be rejected" + ); + assert!( + ClaimedNodeNum::from_wire(0xFFFF_FFFF).is_none(), + "from == broadcast must be rejected" + ); +} + +#[test] +fn claimed_node_num_accepts_a_real_value() { + // WHY: the falsifiable half of the sentinel-rejection test above -- + // without this, a guard that rejected EVERY `from` value (not just the + // two sentinels) would also pass it. + assert_eq!( + ClaimedNodeNum::from_wire(0xDEAD_BEEF).map(ClaimedNodeNum::accept_unauthenticated), + Some(crate::types::NodeNum(0xDEAD_BEEF)) + ); +} + +#[tokio::test] +async fn process_packet_ignores_zero_from_sentinel() { + // WHY(#246): pre-fix, `mesh_packet.from == 0` created a node-DB entry + // keyed on a value that is never a real node -- ANY node on the mesh + // could spoof `from: 0` and still land in the DB. + let c = MeshCollector::new(make_config(vec![])); + let pkt = FromRadio { + id: 1, + payload_variant: Some(from_radio::PayloadVariant::Packet( + crate::proto::MeshPacket { + from: 0, + to: 0xFFFF_FFFF, + rx_snr: 5.0, + hop_start: 3, + hop_limit: 1, + ..Default::default() + }, + )), + }; + + c.process_packet(&pkt).await; + + let db = c.node_db().lock().await; + let has_entry = db.get(crate::types::NodeNum(0)).is_some(); + let db_is_empty = db.is_empty(); + drop(db); + assert!(!has_entry, "from == 0 must never create a node-DB entry"); + assert!(db_is_empty, "from == 0 must not touch the node DB at all"); +} + +#[tokio::test] +async fn process_packet_ignores_broadcast_from_sentinel() { + // WHY(#246): pre-fix, a spoofed `from == 0xFFFF_FFFF` (broadcast) would + // insert/update a node-DB entry keyed on the broadcast address, feeding + // a phantom entry into topology/discovery. + let c = MeshCollector::new(make_config(vec![])); + let pkt = FromRadio { + id: 1, + payload_variant: Some(from_radio::PayloadVariant::Packet( + crate::proto::MeshPacket { + from: 0xFFFF_FFFF, + to: 0x1234, + rx_snr: 5.0, + hop_start: 3, + hop_limit: 1, + ..Default::default() + }, + )), + }; + + c.process_packet(&pkt).await; + + let db = c.node_db().lock().await; + let has_entry = db.get(crate::types::NodeNum(0xFFFF_FFFF)).is_some(); + let db_is_empty = db.is_empty(); + drop(db); + assert!( + !has_entry, + "from == broadcast must never create a node-DB entry" + ); + assert!(db_is_empty); +} + +#[tokio::test] +async fn process_packet_still_admits_a_real_node_num() { + // WHY: the falsifiable half of the two sentinel-rejection tests above -- + // without this, a guard that rejected EVERY packet (not just the + // sentinels) would also pass them. + let c = MeshCollector::new(make_config(vec![])); + let pkt = FromRadio { + id: 1, + payload_variant: Some(from_radio::PayloadVariant::Packet( + crate::proto::MeshPacket { + from: 0xDEAD, + to: 0xFFFF_FFFF, + rx_snr: 5.0, + hop_start: 3, + hop_limit: 1, + ..Default::default() + }, + )), + }; + + c.process_packet(&pkt).await; + + let db = c.node_db().lock().await; + let has_entry = db.get(crate::types::NodeNum(0xDEAD)).is_some(); + drop(db); + assert!(has_entry); +} diff --git a/crates/kerykeion/src/error.rs b/crates/kerykeion/src/error.rs index a13c3a6..00232fb 100644 --- a/crates/kerykeion/src/error.rs +++ b/crates/kerykeion/src/error.rs @@ -233,6 +233,21 @@ pub enum Error { location: snafu::Location, }, + /// Position lat/lon is non-finite or outside Meshtastic's valid + /// geographic range, which the fixed-point wire conversion assumes holds. + #[snafu(display( + "invalid position: lat={lat}, lon={lon} (must be finite, lat ∈ [-90, 90], lon ∈ [-180, 180])" + ))] + InvalidPosition { + /// The rejected latitude in decimal degrees. + lat: f64, + /// The rejected longitude in decimal degrees. + lon: f64, + /// Source location for diagnostics. + #[snafu(implicit)] + location: snafu::Location, + }, + /// A store-forward snapshot has more distinct destinations than the /// running cap allows; loading it verbatim would silently disable the /// per-destination resource-exhaustion protection for every over-cap @@ -316,4 +331,14 @@ mod tests { }; assert!(err.to_string().contains("0xdeadbeef")); } + + #[test] + fn invalid_position_message() { + let err = Error::InvalidPosition { + lat: f64::NAN, + lon: 200.0, + location: snafu::location!(), + }; + assert!(err.to_string().contains("invalid position")); + } } diff --git a/crates/kerykeion/src/message.rs b/crates/kerykeion/src/message.rs index 0866207..08dbbab 100644 --- a/crates/kerykeion/src/message.rs +++ b/crates/kerykeion/src/message.rs @@ -4,7 +4,7 @@ use prost::Message as _; use crate::config::MessageConfig; use crate::crypto; -use crate::error::Error; +use crate::error::{Error, InvalidPositionSnafu}; use crate::packet_id::PacketIdCounter; use crate::proto::mesh_packet::Priority; use crate::proto::{AdminMessage, Data, MeshPacket, PortNum, Position, mesh_packet}; @@ -22,6 +22,7 @@ use crate::types::{ChannelIndex, MAX_HOP_LIMIT, NodeNum}; /// .with_ack() /// .build(NodeNum(0xABCD), &[0x01], &mut packet_ids)?; /// ``` +#[derive(Debug)] pub struct MessageBuilder { dest: NodeNum, portnum: PortNum, @@ -57,25 +58,53 @@ impl MessageBuilder { /// /// Latitude and longitude are in decimal degrees; they are converted to /// Meshtastic's `i32` representation (`value * 1e7`). - #[must_use] - pub fn position(dest: NodeNum, lat: f64, lon: f64) -> Self { + /// + /// # Errors + /// + /// Returns [`Error::InvalidPosition`] if `lat` or `lon` is not finite, or + /// outside `[-90, 90]` / `[-180, 180]` respectively (#247). + pub fn position(dest: NodeNum, lat: f64, lon: f64) -> Result { Self::position_with_config(dest, lat, lon, &MessageConfig::default()) } /// Build a position message with a caller-supplied [`MessageConfig`]. - #[must_use] - pub fn position_with_config(dest: NodeNum, lat: f64, lon: f64, config: &MessageConfig) -> Self { + /// + /// # Errors + /// + /// Returns [`Error::InvalidPosition`] if `lat` or `lon` is not finite, or + /// outside `[-90, 90]` / `[-180, 180]` respectively (#247). + pub fn position_with_config( + dest: NodeNum, + lat: f64, + lon: f64, + config: &MessageConfig, + ) -> Result { + // WHY: the `as i32` cast below never panics — Rust saturates NaN to 0 + // and ±Inf to the i32 extremes — so an unchecked cast turns a failed + // GPS read (NaN) or a caller bug (Inf, out-of-range degrees) into a + // plausible-looking wire coordinate instead of a build-time error. + // Validate BEFORE the cast so the invariant the cast relies on + // (finite, in-range degrees) is actually established, not merely + // asserted (#247). + if !lat.is_finite() + || !(-90.0..=90.0).contains(&lat) + || !lon.is_finite() + || !(-180.0..=180.0).contains(&lon) + { + return InvalidPositionSnafu { lat, lon }.fail(); + } + // WHY: Meshtastic firmware stores lat/lon as fixed-point i32 = degrees * 1e7. #[expect( clippy::as_conversions, reason = "f64→i32 via multiplication is the Meshtastic wire format convention" )] let pos = Position { - latitude_i: (lat * 1e7) as i32, // SAFETY: lat ∈ [-90, 90] so lat*1e7 ∈ [-9e8, 9e8] which fits i32 (±2.1e9) - longitude_i: (lon * 1e7) as i32, // SAFETY: lon ∈ [-180, 180] so lon*1e7 ∈ [-1.8e9, 1.8e9] which fits i32 + latitude_i: (lat * 1e7) as i32, // SAFETY: lat validated finite + ∈ [-90, 90] above, so lat*1e7 ∈ [-9e8, 9e8] which fits i32 (±2.1e9) + longitude_i: (lon * 1e7) as i32, // SAFETY: lon validated finite + ∈ [-180, 180] above, so lon*1e7 ∈ [-1.8e9, 1.8e9] which fits i32 ..Default::default() }; - Self { + Ok(Self { dest, portnum: PortNum::PositionApp, payload: pos.encode_to_vec(), @@ -83,7 +112,7 @@ impl MessageBuilder { want_ack: false, hop_limit: config.default_hop_limit.min(MAX_HOP_LIMIT), priority: Priority::Default, - } + }) } /// Build an admin message (`ADMIN_APP`) with the default hop limit. @@ -310,6 +339,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) + .unwrap() .build(FROM_NODE, &[], &mut PacketIdCounter::resume(0)) .unwrap(); @@ -327,6 +357,47 @@ mod tests { ); } + #[test] + fn position_rejects_nan_latitude() { + // WHY(#247): pre-fix, `NaN as i32` saturates to 0 rather than + // panicking, so a failed GPS read silently produced a packet at + // (0, lon) instead of failing the build. + let result = MessageBuilder::position(DEST, f64::NAN, -122.4194); + assert!( + matches!(result, Err(Error::InvalidPosition { .. })), + "NaN latitude must be rejected, got {result:?}" + ); + } + + #[test] + fn position_rejects_infinite_longitude() { + let result = MessageBuilder::position(DEST, 37.7749, f64::INFINITY); + assert!( + matches!(result, Err(Error::InvalidPosition { .. })), + "infinite longitude must be rejected, got {result:?}" + ); + } + + #[test] + fn position_rejects_out_of_range_latitude() { + // WHY: distinguishes the range check from the finiteness check -- + // 91.0 is finite but not a valid latitude. + let result = MessageBuilder::position(DEST, 91.0, 0.0); + assert!( + matches!(result, Err(Error::InvalidPosition { .. })), + "out-of-range latitude must be rejected, got {result:?}" + ); + } + + #[test] + fn position_accepts_boundary_coordinates() { + // WHY: the falsifiable half of the range checks above -- without + // this, an inverted comparison (e.g. `>` instead of `>=`) that + // rejects everything would also pass the rejection tests. + assert!(MessageBuilder::position(DEST, 90.0, 180.0).is_ok()); + assert!(MessageBuilder::position(DEST, -90.0, -180.0).is_ok()); + } + #[test] fn admin_message_sets_reliable_priority() { let admin = AdminMessage { diff --git a/crates/kerykeion/src/processor.rs b/crates/kerykeion/src/processor.rs index 1dc4deb..0841e34 100644 --- a/crates/kerykeion/src/processor.rs +++ b/crates/kerykeion/src/processor.rs @@ -11,7 +11,7 @@ use crate::proto::mesh_packet::PayloadVariant; use crate::proto::{MeshPacket, Routing, routing}; use crate::signals::{MeshEvent, mesh_event_to_signal}; use crate::topology::MeshTopology; -use crate::types::{NodeNum, PacketId}; +use crate::types::{ClaimedNodeNum, NodeNum, PacketId}; /// `NeighborInfo` protobuf (portnum 71) - not in vendored protos, decoded manually. #[derive(prost::Message)] @@ -93,12 +93,30 @@ impl PacketProcessor { /// /// Dispatches based on portnum and updates internal state. Returns any /// produced events for external handling. + /// + /// `packet.from` is an unauthenticated wire claim (see [`ClaimedNodeNum`]). + /// A packet claiming the non-node sentinels (`0` or broadcast) is dropped + /// HERE, before any node-DB write, topology write, or event emission — + /// every write this function (and everything it calls) makes is keyed on + /// `from`, so this is the single point that derives `from` FROM the wire + /// for `PacketProcessor`. Gating only some of the downstream writes would + /// resurface #246 against `PacketProcessor`'s own `NodeDb` + topology + /// graph, which are SEPARATE from `MeshCollector`'s display-only copy + /// that #246's original fix guards. pub fn process_mesh_packet(&mut self, packet: &crate::proto::MeshPacket) -> Vec { let mut events = Vec::new(); - let from = NodeNum(packet.from); + let Some(from) = + ClaimedNodeNum::from_wire(packet.from).map(ClaimedNodeNum::accept_unauthenticated) + else { + tracing::trace!( + from = packet.from, + "ignoring mesh packet with sentinel `from` (unset or broadcast)" + ); + return events; + }; // WHY: passive learning - every received packet provides link metadata. - self.apply_passive_learning(packet); + self.apply_passive_learning(packet, from); let Some(crate::proto::mesh_packet::PayloadVariant::Decoded(decoded)) = &packet.payload_variant @@ -117,7 +135,7 @@ impl PacketProcessor { self.handle_telemetry(from, &decoded.payload, &mut events); } p if p == portnum::NEIGHBORINFO_APP => { - self.handle_neighborinfo(&decoded.payload, &mut events); + self.handle_neighborinfo(from, &decoded.payload, &mut events); } p if p == portnum::TRACEROUTE_APP => { self.handle_traceroute(from, packet, &decoded.payload, &mut events); @@ -138,9 +156,10 @@ impl PacketProcessor { } // WHY: an event names its own subject, which is not always the packet - // sender. NEIGHBORINFO reports carry a reporter id from the payload, so - // locating every signal at `from` puts a relayed report at the relay - // rather than at the node the report is about. + // sender. TRACEROUTE reports name every hop along the discovered + // route, so locating every signal at `from` would put each + // intermediate hop's link at the packet's own sender instead of the + // hop it actually describes. for event in &events { let position = event .subject() @@ -157,8 +176,14 @@ impl PacketProcessor { } /// Infer link quality FROM packet metadata without explicit topology messages. - fn apply_passive_learning(&mut self, packet: &crate::proto::MeshPacket) { - let from = NodeNum(packet.from); + /// + /// `from` must already be a [`ClaimedNodeNum`]-accepted identity — + /// this function performs no sentinel check of its own; `from` is + /// threaded in from [`Self::process_mesh_packet`]'s single validated + /// derivation rather than re-read from `packet.from` here, so there is + /// exactly one place in `PacketProcessor` that turns a raw wire `from` + /// into a [`NodeNum`]. + fn apply_passive_learning(&mut self, packet: &crate::proto::MeshPacket, from: NodeNum) { let snr = if packet.rx_snr == 0.0 { None } else { @@ -366,7 +391,20 @@ impl PacketProcessor { } } - fn handle_neighborinfo(&mut self, payload: &[u8], events: &mut Vec) { + /// Handles a `NEIGHBORINFO_APP` payload, attributing the reported links + /// to `from` (the packet's actual sender) rather than the payload's own + /// `node_id` claim. + /// + /// The payload's `node_id` is a SECOND, independently forgeable identity + /// assertion embedded inside the packet body — Meshtastic gives no + /// channel-level guarantee it agrees with `from`. Trusting it + /// unconditionally lets any sender attribute fabricated neighbor links + /// to whichever victim node number it names, on that victim's behalf + /// (#207). When the two disagree the report is dropped rather than + /// silently reattributed to `from`: a mismatch does not distinguish a + /// forged claim from a payload describing a genuinely different node, so + /// neither identity can be trusted for this report. + fn handle_neighborinfo(&mut self, from: NodeNum, payload: &[u8], events: &mut Vec) { let ni = match NeighborInfo::decode(payload) { Ok(n) => n, Err(e) => { @@ -375,15 +413,23 @@ impl PacketProcessor { } }; - let reporter = NodeNum(ni.node_id); - self.topology.add_node(reporter); + let claimed = NodeNum(ni.node_id); + if claimed != from { + tracing::warn!( + claimed = claimed.0, + sender = from.0, + "NEIGHBORINFO payload node_id disagrees with packet sender; dropping report" + ); + return; + } + + self.topology.add_node(from); for neighbor in &ni.neighbors { let neighbor_num = NodeNum(neighbor.node_id); - self.topology - .update_link(reporter, neighbor_num, neighbor.snr); + self.topology.update_link(from, neighbor_num, neighbor.snr); events.push(MeshEvent::TopologyChange { - from: reporter, + from, to: neighbor_num, snr: neighbor.snr, }); @@ -618,3 +664,11 @@ impl RoutingProcessor { )] #[path = "processor_tests.rs"] mod tests; + +#[cfg(test)] +#[expect( + clippy::unwrap_used, + reason = "test code: panics and unwraps acceptable in assertions" +)] +#[path = "processor_tests_attribution.rs"] +mod tests_attribution; diff --git a/crates/kerykeion/src/processor_tests.rs b/crates/kerykeion/src/processor_tests.rs index 44ec6c1..01dad99 100644 --- a/crates/kerykeion/src/processor_tests.rs +++ b/crates/kerykeion/src/processor_tests.rs @@ -713,19 +713,24 @@ fn node_at(num: u32, latitude: f64, longitude: f64) -> MeshNode { } #[tokio::test] -async fn neighborinfo_signal_is_located_at_the_reporter_not_the_packet_sender() { - // WHY: NEIGHBORINFO carries its reporter id in the payload, so a relayed - // report describes links the relay is not an endpoint of. Locating every - // signal at the packet sender puts those links at the relay's coordinates. +async fn neighborinfo_spoofed_node_id_is_dropped_not_attributed_to_the_victim() { + // WHY(#207): NEIGHBORINFO's `node_id` is a claim embedded in the payload + // ITSELF, independent of `packet.from`. Pre-fix, node 0x1111 could + // broadcast a NEIGHBORINFO packet claiming `node_id: 0x2222` and have the + // resulting link/signal attributed to 0x2222 -- a victim who never sent + // anything -- rather than to 0x1111, the node that actually transmitted + // it. The fix requires `node_id == packet.from`; on mismatch the report + // is dropped, producing no event and no topology mutation under either + // identity. let (tx, mut rx) = broadcast::channel(64); let mut node_db = NodeDb::new(); node_db.set_my_node(NodeNum(0xAAAA)); - node_db.insert(node_at(0x1111, 10.0, 10.0)); // the relay that transmitted - node_db.insert(node_at(0x2222, 50.0, 60.0)); // the node the report is about + node_db.insert(node_at(0x1111, 10.0, 10.0)); // the actual sender + node_db.insert(node_at(0x2222, 50.0, 60.0)); // the claimed/victim node_id let mut proc = PacketProcessor::new(node_db, MeshTopology::new(), tx); let ni = NeighborInfo { - node_id: 0x2222, + node_id: 0x2222, // spoofed: does not match the packet's `from` last_sent_by_id: 0, node_broadcast_interval_secs: 0, neighbors: vec![Neighbor { @@ -738,7 +743,48 @@ async fn neighborinfo_signal_is_located_at_the_reporter_not_the_packet_sender() let packet = make_mesh_packet(0x1111, portnum::NEIGHBORINFO_APP, payload); let events = proc.process_mesh_packet(&packet); + + assert!( + events.is_empty(), + "a spoofed node_id must yield no events, got {events:?}" + ); + assert!( + !proc.topology().contains_node(NodeNum(0x2222)), + "the victim node must gain no topology entry from a report it never sent" + ); + assert!( + rx.try_recv().is_err(), + "no signal should be broadcast for a dropped report" + ); +} + +#[tokio::test] +async fn neighborinfo_agreeing_node_id_is_located_at_the_reporter() { + // WHY: the falsifiable half of the spoof-rejection test above -- without + // this, a handler that dropped EVERY NEIGHBORINFO report (not just + // mismatched ones) would also pass it. + let (tx, mut rx) = broadcast::channel(64); + let mut node_db = NodeDb::new(); + node_db.set_my_node(NodeNum(0xAAAA)); + node_db.insert(node_at(0x2222, 50.0, 60.0)); // sender == claimed node_id + let mut proc = PacketProcessor::new(node_db, MeshTopology::new(), tx); + + let ni = NeighborInfo { + node_id: 0x2222, // agrees with packet.from below + last_sent_by_id: 0, + node_broadcast_interval_secs: 0, + neighbors: vec![Neighbor { + node_id: 0x3333, + snr: 4.0, + }], + }; + let mut payload = Vec::new(); + ni.encode(&mut payload).unwrap(); + + let packet = make_mesh_packet(0x2222, portnum::NEIGHBORINFO_APP, payload); + let events = proc.process_mesh_packet(&packet); assert_eq!(events.len(), 1, "one neighbor should yield one event"); + assert!(proc.topology().contains_node(NodeNum(0x2222))); let signal = rx.recv().await.unwrap(); #[expect(clippy::expect_used, reason = "test-only")] @@ -748,7 +794,7 @@ async fn neighborinfo_signal_is_located_at_the_reporter_not_the_packet_sender() assert!( (coords.latitude - 50.0).abs() < f64::EPSILON && (coords.longitude - 60.0).abs() < f64::EPSILON, - "signal should be located at reporter 0x2222 (50, 60), got ({}, {})", + "signal should be located at 0x2222 (50, 60), got ({}, {})", coords.latitude, coords.longitude ); diff --git a/crates/kerykeion/src/processor_tests_attribution.rs b/crates/kerykeion/src/processor_tests_attribution.rs new file mode 100644 index 0000000..5024898 --- /dev/null +++ b/crates/kerykeion/src/processor_tests_attribution.rs @@ -0,0 +1,154 @@ +//! Tests for [`super`]'s unauthenticated-attribution guard on `PacketProcessor`'s +//! OWN `NodeDb` + `MeshTopology` -- SEPARATE state from `MeshCollector`'s +//! display-only `NodeDb`, which `collector_tests_attribution.rs` covers. +//! `#246`'s original fix guarded only the collector's copy; this file +//! guards against the same defect resurfacing here. Split out rather than +//! added to `processor_tests.rs`, which is already at the +//! RUST/file-too-long 800-line threshold. + +use super::*; + +fn make_processor() -> PacketProcessor { + let (tx, _rx) = broadcast::channel(64); + let mut node_db = NodeDb::new(); + node_db.set_my_node(NodeNum(0xAAAA)); + PacketProcessor::new(node_db, MeshTopology::new(), tx) +} + +/// A packet shaped to exercise the passive-learning direct-link write: +/// `hop_start == hop_limit` (`hop_count` 0) with a non-zero `rx_snr` is +/// exactly what makes pre-fix `apply_passive_learning` call +/// `topology.update_link` -- the reviewer's crafted attack packet for the +/// blocking finding on #381. +fn sentinel_packet(from: u32) -> crate::proto::MeshPacket { + crate::proto::MeshPacket { + from, + to: 0xFFFF_FFFF, + rx_snr: 5.0, + hop_start: 1, + hop_limit: 1, + ..Default::default() + } +} + +#[test] +fn process_mesh_packet_ignores_zero_from_sentinel() { + // WHY(#381): pre-fix, `PacketProcessor::apply_passive_learning` read + // `packet.from` directly with no sentinel check, unlike the guarded + // `MeshCollector::handle_mesh_packet` path -- a spoofed `from: 0` + // created a node-DB entry in the processor's OWN NodeDb and, because + // this packet's hop_count is 0, fabricated a direct topology edge FROM + // the sentinel identity to `my_node`. + let mut proc = make_processor(); + let packet = sentinel_packet(0); + + let events = proc.process_mesh_packet(&packet); + + assert!(events.is_empty(), "sentinel `from` must emit no events"); + assert!( + proc.node_db().get(NodeNum(0)).is_none(), + "from == 0 must never create a node-DB entry" + ); + assert!( + proc.node_db().is_empty(), + "from == 0 must not touch the node DB at all" + ); + assert!( + !proc.topology().contains_node(NodeNum(0)), + "from == 0 must never appear in the topology graph" + ); + assert_eq!( + proc.topology().edge_count(), + 0, + "from == 0 must never create a topology edge" + ); +} + +#[test] +fn process_mesh_packet_ignores_broadcast_from_sentinel() { + // WHY(#381): same as the zero case, for the broadcast sentinel. + let mut proc = make_processor(); + let packet = sentinel_packet(0xFFFF_FFFF); + + let events = proc.process_mesh_packet(&packet); + + assert!(events.is_empty(), "sentinel `from` must emit no events"); + assert!( + proc.node_db().get(NodeNum(0xFFFF_FFFF)).is_none(), + "from == broadcast must never create a node-DB entry" + ); + assert!(proc.node_db().is_empty()); + assert!( + !proc.topology().contains_node(NodeNum(0xFFFF_FFFF)), + "from == broadcast must never appear in the topology graph" + ); + assert_eq!(proc.topology().edge_count(), 0); +} + +#[test] +fn process_mesh_packet_still_admits_a_real_node_num() { + // WHY: the falsifiable half of the two sentinel-rejection tests above -- + // without this, a guard that rejected EVERY `from` value (not just the + // two sentinels) would also pass them. + let mut proc = make_processor(); + let packet = sentinel_packet(0xDEAD); + + proc.process_mesh_packet(&packet); + + let my_node = proc.node_db().my_node().unwrap(); + assert!(proc.node_db().get(NodeNum(0xDEAD)).is_some()); + let neighbors = proc.topology().neighbors(NodeNum(0xDEAD)); + assert!( + neighbors.iter().any(|(n, _)| *n == my_node), + "a real node number must still create the direct topology link" + ); +} + +#[test] +fn process_mesh_packet_sentinel_from_blocks_nodeinfo_dispatch_too() { + // WHY(#381): the fix gates once, at `process_mesh_packet`'s single + // derivation of `from`, rather than inside `apply_passive_learning` + // alone -- this proves the portnum-dispatched handlers (`handle_nodeinfo` + // here) are ALSO unreachable for a sentinel `from`, not merely the + // passive-learning write the reviewer's citation named. A fix mirroring + // only that literal citation (processor.rs:161-206) would have left + // this second path exploitable -- the same "protects the wrong copy" + // shape one level deeper in the same file. + let mut proc = make_processor(); + let user = crate::proto::User { + id: "!deadbeef".into(), + long_name: "Spoofed Node".into(), + short_name: "SPF".into(), + macaddr: vec![], + hw_model: 9, + is_licensed: false, + role: 0, + }; + let mut payload = Vec::new(); + user.encode(&mut payload).unwrap(); + + let mut packet = sentinel_packet(0); + packet.payload_variant = Some(crate::proto::mesh_packet::PayloadVariant::Decoded( + crate::proto::Data { + portnum: portnum::NODEINFO_APP, + payload, + want_response: false, + dest: 0, + source: 0, + request_id: 0, + reply_id: 0, + emoji: vec![], + }, + )); + + let events = proc.process_mesh_packet(&packet); + + assert!( + events.is_empty(), + "sentinel `from` must emit no NodeDiscovered event" + ); + assert!( + proc.node_db().is_empty(), + "sentinel `from` must not reach handle_nodeinfo's insert" + ); +} diff --git a/crates/kerykeion/src/router.rs b/crates/kerykeion/src/router.rs index 2199ed1..aa4bf6e 100644 --- a/crates/kerykeion/src/router.rs +++ b/crates/kerykeion/src/router.rs @@ -252,6 +252,15 @@ impl MeshRouter { /// each `packet_bytes` back into the original [`MeshPacket`] so the /// re-enqueued message carries its real payload and header fields /// (`from`, `hop_limit`, `hop_start`, ...) rather than a synthetic shell. + /// + /// WARNING: `dest` drives WHERE a store-and-forward queue's contents get + /// re-sent, so callers must reach this ONLY from an authenticated + /// reachability/topology event, never from a raw per-packet `from` + /// attribution (an unauthenticated claim per #246) — that coupling would + /// let a spoofed `from` redirect a queued message toward an + /// attacker-named destination. No call site in this crate currently + /// drives this from packet receipt (verified: only `MeshRouter`'s own + /// tests call it); keep it that way. pub fn node_came_online(&mut self, dest: NodeNum) { let stored = self.store_forward.drain_for(dest); for msg in stored { diff --git a/crates/kerykeion/src/signals.rs b/crates/kerykeion/src/signals.rs index 5e03e25..7e4ff63 100644 --- a/crates/kerykeion/src/signals.rs +++ b/crates/kerykeion/src/signals.rs @@ -89,8 +89,10 @@ impl MeshEvent { /// The node this event is about, when it names exactly one. /// /// This is the node whose position locates the emitted signal. It is not - /// the packet sender: `NEIGHBORINFO` carries a reporter id in its payload, - /// so a relayed report describes links the relay is not an endpoint of. + /// always the packet's transmitting sender: a `TRACEROUTE` report names + /// every hop along the discovered route, most of which are not the + /// sender. (`NEIGHBORINFO`'s reporter id is required to match the + /// sender to be admitted at all — see `PacketProcessor::handle_neighborinfo`.) /// /// Returns `None` for the partition events, which describe a set of nodes /// rather than one; their conversions carry no location either. diff --git a/crates/kerykeion/src/types.rs b/crates/kerykeion/src/types.rs index 857933e..6a3b186 100644 --- a/crates/kerykeion/src/types.rs +++ b/crates/kerykeion/src/types.rs @@ -89,6 +89,45 @@ impl NodeNum { } } +/// A node number as CLAIMED by a raw, over-the-air `MeshPacket.from` field. +/// +/// Meshtastic carries no cryptographic sender binding at this layer in this +/// proto subset (no signature, no `relay_node`) — any node holding the +/// channel key can set `from` to any value, including another node's number. +/// This wrapper is the SINGLE gate every raw wire header must pass through +/// before it becomes a [`NodeNum`] used to CREATE or UPDATE a node-DB or +/// topology entry, so the conversion reads as a stated trust decision rather +/// than a bare cast that looks like an established fact. `MeshCollector` and +/// `PacketProcessor` each own a SEPARATE `NodeDb` (and `PacketProcessor` also +/// owns the `MeshTopology` the collector's display DB does not touch) — this +/// type is shared rather than reimplemented at each site precisely so both +/// write paths stay gated by the identical rule instead of drifting apart. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) struct ClaimedNodeNum(NodeNum); + +impl ClaimedNodeNum { + /// Wraps a raw wire `from` value, rejecting the two values that are + /// never a real originating node: `0` (unset) and the broadcast address + /// `0xFFFF_FFFF`. A packet claiming either must be dropped before it + /// reaches ANY node-DB or topology write, at every call site that + /// derives an identity from a packet's `from` field. + pub(crate) fn from_wire(raw: u32) -> Option { + let candidate = NodeNum(raw); + (raw != 0 && !candidate.is_broadcast()).then_some(Self(candidate)) + } + + /// Accepts the claim as a [`NodeNum`] for node-DB / topology attribution. + /// + /// Named explicitly rather than via `From`/`Into` so every call site + /// states, in its own name, that it is accepting an UNAUTHENTICATED + /// identity claim, not a verified fact — akroasis has no channel key or + /// out-of-band anchor to check `from` against at this layer, so this is + /// the strongest attribution available, not proof. + pub(crate) const fn accept_unauthenticated(self) -> NodeNum { + self.0 + } +} + impl ChannelIndex { /// Constructs a `ChannelIndex`, returning an error if `index >= MAX_CHANNELS`. ///