fix(kerykeion): attribute mesh source to the verified sender, not the packet's claim - #381
Open
forkwright wants to merge 5 commits into
Open
fix(kerykeion): attribute mesh source to the verified sender, not the packet's claim#381forkwright wants to merge 5 commits into
forkwright wants to merge 5 commits into
Conversation
added 5 commits
August 16, 2026 22:02
…tinel from values mesh_packet.from feeds the node DB unconditionally, so any node on the mesh can claim any NodeNum -- including the two values that are never a real originating node (0, and the 0xFFFF_FFFF broadcast address) -- and create or update that entry. Meshtastic carries no cryptographic sender binding at this layer in this proto subset (no signature, no relay_node field), so `from` remains the strongest identity signal available but not a verified fact. Wrap it in ClaimedNodeNum at the one place it turns into a node-DB attribution, so the conversion reads as a stated trust decision rather than a bare cast, and reject the two non-node sentinels before they reach the DB. node_came_online (router.rs) moves store-and-forward messages toward a NodeNum and was flagged as reachable from raw packet attribution; it is not -- verified by repo-wide grep, only MeshRouter's own tests call it. Documented the invariant so it stays that way.
…t the payload claim handle_neighborinfo derived the reporting node's identity entirely from the protobuf body field ni.node_id, never consulting the packet's actual transmitting sender. Any mesh node could broadcast a NEIGHBORINFO_APP packet claiming an arbitrary node_id and have the resulting topology links, and the located signal, attributed to that victim rather than to itself. Pass the packet's sender into handle_neighborinfo and require ni.node_id == from; on mismatch the report is dropped rather than silently reattributed to the sender, since a mismatch does not distinguish a forged claim from a payload describing a genuinely different node. Updated the two doc comments that cited NEIGHBORINFO as the reason event subjects can differ from the packet sender -- that case is now enforced equal by construction. TRACEROUTE's intermediate hops are the still-live example and now stand alone.
position_with_config converted caller-supplied f64 lat/lon straight to i32 via an as cast with no finiteness or range check. Rust saturates an out-of-range as-cast rather than panicking, so a NaN latitude (e.g. from a failed GPS read) silently became 0 and an infinite value silently became an i32 extreme, producing a plausible-looking wire coordinate instead of an error -- exactly the silent corruption the removed SAFETY comment claimed could not happen. Validate finiteness and Meshtastic's geographic range before the cast and return Error::InvalidPosition on failure. position and position_with_config now return Result<Self, Error>; dropped their #[must_use] since Result already carries it at the type level (clippy::double_must_use under -D warnings, kanon#3473).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
One defect in two places: mesh-packet attribution trusted a field the sender controls, without verifying it against anything the receiving side actually observed.
handle_mesh_packetderived the node-DB key straight frommesh_packet.from, with no guard against the two values that are never a real originating node (0, and the0xFFFF_FFFFbroadcast address).handle_neighborinfoderived the topology "reporter" straight from the payload's ownni.node_idfield, never checking it against the packet's actual sender (packet.from), so a relay could attribute fabricated neighbor links to a victim node it never spoke for.MessageBuilder::position_with_configcast caller-suppliedf64lat/lon straight toi32with no finiteness/range check;as i32saturates rather than panics, soNaNsilently became0and±Infsilently became ani32extreme.Fix shape: attribute to what the receiving side can verify (the packet's actual sender), not to what the packet claims; where no stronger signal exists at that layer, make the unverified attribution visibly unverified in the type rather than a plain field that reads as fact.
Changes
#246 —
crates/kerykeion/src/collector.rs:75addsClaimedNodeNum, a private newtype wrapping the raw wirefromvalue.ClaimedNodeNum::from_wire(collector.rs:82) rejects the two non-node sentinels;accept_unauthenticated(collector.rs:94) is the one, explicitly-named place a raw wire claim becomes aNodeNumused for DB attribution — the name states the trust decision instead of a bare cast reading as fact.handle_mesh_packet(collector.rs:196) now drops a packet claiming a sentinelfrombefore it reaches the node DB.node_came_online(router.rs:255) is the mechanism the issue names as reachable "off raw packet attribution" (a spoofedfromredirecting a store-and-forward flush toward an attacker-named destination). Repo-wide grep confirms it is not currently wired to the packet-receive path at all — the only callers areMeshRouter's own tests (router.rs:444,506). Documented the invariant atrouter.rs:256(WARNING tag) so a future wiring attempt is warned rather than silent.#207 —
processor.rs:120now passesfrom(the packet's actual sender) intohandle_neighborinfo.handle_neighborinfo(processor.rs:383) computesclaimed = NodeNum(ni.node_id)(processor.rs:392) and requiresclaimed == from(processor.rs:393); on mismatch the report is dropped (logged, no topology mutation, no event) rather than silently reattributed tofrom— a mismatch doesn't distinguish a forged claim from a payload genuinely describing a different node, so neither identity is trustworthy for that report. On agreement, topology mutations usefromthroughout.Updated the two doc comments (
processor.rs:140,signals.rs:91) that cited NEIGHBORINFO as the reason an event's subject can differ from the packet sender — that case is now enforced equal by construction. TRACEROUTE's intermediate hops are the still-live example.#247 —
message.rs:65/message.rs:75:position/position_with_confignow returnResult<Self, Error>and validatelat/lon(message.rs:88) — finite, and within[-90, 90]/[-180, 180]— before theas i32cast, returning the newError::InvalidPosition(error.rs:241) on failure. Dropped#[must_use]on both (Result already carries it; an explicit one isclippy::double_must_useunder-D warnings, kanon#3473). Added#[derive(Debug)]toMessageBuilderso the new tests can format the rejectedResult.Done when: (from the issues)
#246
from == 0orfrom == 0xFFFF_FFFFneither creates nor updates a node DB entry" —collector.rs:196(ClaimedNodeNum::from_wireguard); proven bycollector_tests_attribution.rs:41and:71.node_came_onlinefires only from an authenticated reachability event rather than rawfromattribution" — already true onmain(verified: zero non-test call sites); now documented as an invariant atrouter.rs:256so it stays true.#207
processor.rs:392-393(claimed == fromgate; topology writes usefrom).node_idthat disagrees with the packet source is rejected or flagged" — rejected: early return atprocessor.rs:394-399.node_idframe does not create links attributed to the victim node" —processor_tests.rs:659.#247
f64::NANas lat returns anErrfrom the position builder rather than producing a packet" —message.rs:88-93; proven bymessage.rs:348.Negative fixtures
Verified on the build box (
verda-build), against the exact shipped code — not a re-implementation. All three fixtures were run against unmodifiedorigin/main(bebbcd1) first and observed to fail/demonstrate the defect, then run again against this branch's tip and observed to pass.Testing
cargo fmt --checkpassescargo clippy -D warningspassescargo testpassesVerification
cargo fmt -p kerykeion— clean (build box + locally).kanon lint <changed files> --all— clean except two pre-existing, unrelated findings, both confirmed outside this diff's hunks:RUST/no-arc-mutex-anti-patternatcollector.rs:296/298/381(unrelated functions, already onmain) andRUST/file-too-longoncollector_tests.rs(807 lines onmain, unchanged by this PR — the new Unauthenticated mesh source attribution lets any node spoof any NodeNum in the node DB #246 tests went into a new sibling file,collector_tests_attribution.rs, specifically so this PR doesn't make that pre-existing overage worse).cargo clippy -p kerykeion --all-targets -- -D warnings— clean on the build box.cargo test -p kerykeion— 285/285 passing on the build box, including all new tests.cargo fmt -p kerykeion --checkandcargo test/cargo clippy --workspace(full workspace, not just kerykeion) — clean, 987/987 tests passing.No
Gate-Passedstamp on this PR — explained, not silently missing.~/gate-repo.sh akroasis <branch>(kanon gate --tier full --stamp) fails at itskanon lintstage on 2 pre-existingOIKOS/private-contenterrors inCONTRIBUTING.md:8/:18(an internalkanon.lanforge hostname leaked in a public doc). This is unrelated to kerykeion/this diff, confirmed present on unmodifiedorigin/main, and already tracked as #377 (filed by a sibling agent gating a different PR in this same wave — its own text: "no PR can currently produce a clean local Gate-Passed trailer regardless of what it touches"). Fixing #377 is out of scope for this PR (a security-attribution fix touchingCONTRIBUTING.mdwould blur the diff for no reason connected to #246/#207/#247), so this PR was verified instead by running the actual gate stages directly against the shipped branch on the build box (listed above) — fmt, clippy-D warnings, and the full test suite, all green, all against real compiled+executed code rather than the stamp mechanism.Independent confirmation: real GitHub Actions CI (
gate / full-gate-build) ran on this PR and passed —fmt/check/clippy/nextest/doctest, 4m18s, green. Absence of aGate-Passedtrailer makes the hybrid-gate workflow (#262's partial fix, landed as #288) fall through to a real hosted build rather than trusting a stamp, so this is genuine independent verification, not the "checks that fail instantly with steps=0" no-op pattern.cargo-audit/cargo-deny/osv-scanner/ai-attribution/check-trailerall pass too.#262(no-CI) itself stays open for its documented residual gaps (dependabot bypass, no push-to-main check) — unrelated to this PR passing its own checks.Closes #246
Closes #207
Closes #247