From a9e4084519c8884002b75de75ffe08f4598a966d Mon Sep 17 00:00:00 2001 From: forkwright Date: Fri, 21 Aug 2026 00:47:00 -0500 Subject: [PATCH 1/2] fix(kerykeion): bound the wire fields that were trusted to bound themselves Two of #229's surviving clauses, both the same shape: a value read straight off the radio was trusted to satisfy a range that only Meshtastic firmware maintains. A neighbour that is hostile or merely wrong is not running that firmware. Positions were persisted unchecked. latitude_i is an i32, so its extremes scale to plus or minus 214.7 degrees -- a node could be placed off the planet, and the value flowed on into the topology and signal paths. koinon's Coordinates::new already owns this rule for the whole fleet, so it is reused rather than restated; it also rejects NaN, which a bare range comparison admits. Hop counts were cast to u8 under an expect whose reason cited the firmware bound. The cast did not hold that bound: a hop_start of 1000 truncates to 232, outside the very limit the justification relied on, and 256 truncates to zero. Both call sites now share one helper that rejects anything past MAX_HOP_LIMIT. The helper also rejects a hop_limit above hop_start rather than saturating it to zero, as the processor did. That pair describes no journey, and reporting zero hops for it invents a measurement the packet never carried. Each case has an anti-vacuity partner: ordinary hop fields still produce a count, and a position exactly at plus or minus 90 and 180 is still kept. Refs #229 --- crates/kerykeion/src/collector.rs | 15 ++---- crates/kerykeion/src/processor.rs | 25 ++++++---- crates/kerykeion/src/processor_tests.rs | 65 +++++++++++++++++++++++++ crates/kerykeion/src/types.rs | 61 +++++++++++++++++++++++ 4 files changed, 147 insertions(+), 19 deletions(-) diff --git a/crates/kerykeion/src/collector.rs b/crates/kerykeion/src/collector.rs index 8fdaa72..b5ba288 100644 --- a/crates/kerykeion/src/collector.rs +++ b/crates/kerykeion/src/collector.rs @@ -118,16 +118,11 @@ impl MeshCollector { } /// Computes hop count FROM packet hop fields. - #[expect( - clippy::cast_possible_truncation, - reason = "hop VALUES are bounded by MAX_HOP_LIMIT (7) in Meshtastic firmware" - )] - const fn compute_hop_count(hop_start: u32, hop_limit: u32) -> Option { - if hop_start > 0 && hop_limit <= hop_start { - Some((hop_start - hop_limit) as u8) // SAFETY: hop_start >= hop_limit is checked by caller; difference fits u8 - } else { - None - } + /// + /// Delegates to [`crate::types::hop_count_from_wire`] so the bound lives in + /// one place; see there for why the fields cannot be trusted to hold it. + fn compute_hop_count(hop_start: u32, hop_limit: u32) -> Option { + crate::types::hop_count_from_wire(hop_start, hop_limit) } /// Processes a single `FromRadio` message, updating the node database. diff --git a/crates/kerykeion/src/processor.rs b/crates/kerykeion/src/processor.rs index 0841e34..bbc9448 100644 --- a/crates/kerykeion/src/processor.rs +++ b/crates/kerykeion/src/processor.rs @@ -190,15 +190,7 @@ impl PacketProcessor { Some(packet.rx_snr) }; - let hop_count = if packet.hop_start > 0 { - #[expect( - clippy::cast_possible_truncation, - reason = "hop VALUES are bounded by MAX_HOP_LIMIT (7)" - )] - Some((packet.hop_start.saturating_sub(packet.hop_limit)) as u8) // SAFETY: saturating_sub result is bounded by hop_start (u8 domain) - } else { - None - }; + let hop_count = crate::types::hop_count_from_wire(packet.hop_start, packet.hop_limit); // WHY: UPDATE or CREATE the node record with latest packet metadata. let mut node = self.node_db.get(from).cloned().unwrap_or(MeshNode { @@ -302,6 +294,21 @@ impl PacketProcessor { // WHY: Meshtastic encodes lat/lon as integer degrees × 1e7. let lat = f64::from(pos_proto.latitude_i) * 1e-7; let lon = f64::from(pos_proto.longitude_i) * 1e-7; + + // WHY(#229): `latitude_i` is an i32 straight off the radio, so its full + // range scales to ±214.7 degrees — a neighbour that is hostile or simply + // wrong can place a node off the planet, and the value was persisted + // unchecked. `Coordinates::new` already owns this rule for the whole + // fleet, so it is reused rather than restated; it also rejects NaN, + // which a bare range comparison would silently admit. + if let Err(error) = koinon::Coordinates::new(lat, lon, None) { + tracing::warn!( + from = from.0, + %error, + "discarding POSITION_APP payload with out-of-range coordinates" + ); + return; + } let alt = if pos_proto.altitude != 0 { Some(pos_proto.altitude) } else { diff --git a/crates/kerykeion/src/processor_tests.rs b/crates/kerykeion/src/processor_tests.rs index ff97397..90d59a8 100644 --- a/crates/kerykeion/src/processor_tests.rs +++ b/crates/kerykeion/src/processor_tests.rs @@ -91,6 +91,71 @@ fn process_position_updates_node_and_emits_event() { ); } +/// WHY(#229): `latitude_i` is an i32 straight off the radio, so its extremes +/// scale to ±214.7 degrees. A neighbour that is hostile or simply wrong could +/// place a node off the planet and the value was stored unchecked, from where it +/// reaches the topology and signal paths. +#[test] +fn a_position_outside_the_planet_is_discarded() { + for (label, latitude_i, longitude_i) in [ + ("latitude past the pole", 900_000_001, 0), + ("latitude at the i32 extreme", i32::MAX, 0), + ("longitude past the meridian", 0, 1_800_000_001), + ("longitude at the i32 minimum", 0, i32::MIN), + ] { + let mut proc = make_processor(); + let pos = crate::proto::Position { + latitude_i, + longitude_i, + altitude: 0, + time: 1_700_000_000, + }; + let mut payload = Vec::new(); + pos.encode(&mut payload).unwrap(); + + let events = + proc.process_mesh_packet(&make_mesh_packet(0xBEEF, portnum::POSITION_APP, payload)); + + assert!( + !events + .iter() + .any(|e| matches!(e, MeshEvent::PositionUpdate { .. })), + "{label}: an out-of-range position must not emit an update" + ); + assert!( + proc.node_db() + .get(NodeNum(0xBEEF)) + .is_none_or(|node| node.position.is_none()), + "{label}: an out-of-range position must not be stored" + ); + } +} + +/// Anti-vacuity for the case above, distinct from the existing happy path: a +/// position exactly at the limits is valid and must survive. +#[test] +fn a_position_at_the_coordinate_limits_is_kept() { + let mut proc = make_processor(); + let pos = crate::proto::Position { + latitude_i: 900_000_000, // +90.0 + longitude_i: -1_800_000_000, // -180.0 + altitude: 0, + time: 1_700_000_000, + }; + let mut payload = Vec::new(); + pos.encode(&mut payload).unwrap(); + + let events = + proc.process_mesh_packet(&make_mesh_packet(0xBEEF, portnum::POSITION_APP, payload)); + + assert!( + events + .iter() + .any(|e| matches!(e, MeshEvent::PositionUpdate { .. })), + "the limits themselves are valid coordinates" + ); +} + #[test] fn process_telemetry_updates_metrics() { let mut proc = make_processor(); diff --git a/crates/kerykeion/src/types.rs b/crates/kerykeion/src/types.rs index 6a3b186..25fcff6 100644 --- a/crates/kerykeion/src/types.rs +++ b/crates/kerykeion/src/types.rs @@ -185,6 +185,28 @@ pub const MAX_CHANNELS: u8 = 8; /// Maximum hop limit for a mesh packet. pub const MAX_HOP_LIMIT: u8 = 7; +/// Hop count derived from a packet's wire hop fields, or `None` when they do +/// not describe a hop count this protocol can produce. +/// +/// WHY(#229) this is bounded rather than cast: both fields are `u32` read +/// straight off the radio. The call sites justified an unchecked `as u8` on the +/// grounds that Meshtastic firmware bounds them by [`MAX_HOP_LIMIT`] — true of +/// firmware, and not true of a neighbour that is hostile or merely wrong. A +/// `hop_start` of 1000 truncated to 232, so the claimed bound produced a value +/// outside it. +/// +/// `hop_limit` above `hop_start` is rejected rather than saturated: the +/// difference is meaningless, and reporting zero hops for it invents a +/// measurement the packet did not carry. +pub(crate) fn hop_count_from_wire(hop_start: u32, hop_limit: u32) -> Option { + if hop_start == 0 || hop_limit > hop_start { + return None; + } + u8::try_from(hop_start - hop_limit) + .ok() + .filter(|&hops| hops <= MAX_HOP_LIMIT) +} + /// Maximum protobuf payload size enforced by Meshtastic firmware. pub const MAX_PACKET_SIZE: usize = 512; @@ -257,6 +279,45 @@ mod tests { assert_eq!(MAX_HOP_LIMIT, 7); } + /// Anti-vacuity: ordinary packets must still yield a hop count, or the + /// rejection cases below would pass against a function returning None for + /// everything. + #[test] + fn ordinary_hop_fields_produce_a_count() { + assert_eq!(hop_count_from_wire(7, 7), Some(0)); + assert_eq!(hop_count_from_wire(7, 4), Some(3)); + assert_eq!(hop_count_from_wire(3, 0), Some(3)); + assert_eq!( + hop_count_from_wire(u32::from(MAX_HOP_LIMIT), 0), + Some(MAX_HOP_LIMIT) + ); + } + + /// WHY(#229): the call sites cast these straight to u8 on the grounds that + /// firmware bounds them. A hostile neighbour is not running that firmware, + /// and 1000 truncates to 232 — a value outside the very bound the cast + /// claimed to rely on. + #[test] + fn wire_hop_fields_beyond_the_protocol_bound_are_rejected() { + assert_eq!(hop_count_from_wire(1000, 0), None, "would truncate to 232"); + assert_eq!(hop_count_from_wire(u32::MAX, 0), None); + assert_eq!(hop_count_from_wire(256, 0), None, "would truncate to 0"); + assert_eq!( + hop_count_from_wire(u32::from(MAX_HOP_LIMIT) + 1, 0), + None, + "one hop past the protocol maximum is still past it" + ); + } + + /// A hop_limit above hop_start describes no journey. Saturating it to zero + /// would report a measurement the packet never carried. + #[test] + fn an_inverted_hop_pair_is_rejected_rather_than_saturated() { + assert_eq!(hop_count_from_wire(3, 9), None); + assert_eq!(hop_count_from_wire(0, 0), None); + assert_eq!(hop_count_from_wire(0, 5), None); + } + // ── Property tests ────────────────────────────────────────────────────── proptest::proptest! { From e33c17ad5aa4312343387bb7c02ed83d13639078 Mon Sep 17 00:00:00 2001 From: forkwright Date: Fri, 21 Aug 2026 00:52:09 -0500 Subject: [PATCH 2/2] docs(kerykeion): backtick the identifiers in the hop test's doc comment clippy::doc_markdown applies to doc comments on test functions too, not only on the public API. --- crates/kerykeion/src/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/kerykeion/src/types.rs b/crates/kerykeion/src/types.rs index 25fcff6..27c6556 100644 --- a/crates/kerykeion/src/types.rs +++ b/crates/kerykeion/src/types.rs @@ -309,7 +309,7 @@ mod tests { ); } - /// A hop_limit above hop_start describes no journey. Saturating it to zero + /// A `hop_limit` above `hop_start` describes no journey. Saturating it to zero /// would report a measurement the packet never carried. #[test] fn an_inverted_hop_pair_is_rejected_rather_than_saturated() {