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..27c6556 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! {