Harden XEdDSA unsigned-packet policy and add coverage - #10858
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughThis PR updates XEdDSA signing and receive-policy handling, applies the shared policy in Router and MQTT, tightens NodeInfo broadcast handling, expands regression tests, and repins the meshtastic/Crypto dependency across multiple variant configs. ChangesXEdDSA signing policy and enforcement
Crypto dependency refresh
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Sender
participant Router
participant CryptoEngine
participant MQTT
participant NodeInfoModule
Sender->>Router: perhapsEncode(packet)
Router->>Router: signedDataFits(data)
alt encoded+signature fits LoRa frame
Router->>CryptoEngine: xeddsa_sign(data)
CryptoEngine-->>Router: signature
else
Router->>Router: send unsigned
end
Router->>Router: perhapsDecode(packet)
Router->>Router: checkXeddsaReceivePolicy(p, size)
Router->>CryptoEngine: xeddsa_verify(signature)
CryptoEngine-->>Router: valid/invalid
Router-->>Router: accept / drop
MQTT->>MQTT: onReceiveProto(decoded packet)
MQTT->>Router: checkXeddsaReceivePolicy(p)
Router-->>MQTT: accept / drop
Router->>NodeInfoModule: handleReceivedProtobuf(mp)
NodeInfoModule->>NodeInfoModule: drop if broadcast && unsigned && previously signed
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
thebentern
force-pushed
the
fix/xeddsa-signing-policy
branch
from
July 2, 2026 14:20
d00e23a to
46708cb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/modules/NodeInfoModule.cpp (1)
54-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the policy backstop comment.
The behavior is clear; this can fit in one or two lines.
Suggested cleanup
- // Broadcasts only: unicast NodeInfo (want_response replies, directed exchanges) is never - // signed by the sender, so dropping it here would break exchanges with signer nodes. This - // check backstops ingress paths that skip Router's downgrade drop (e.g. decoded MQTT). + // Broadcasts only: unicast NodeInfo is never signed; this backstops decoded-ingress paths.As per coding guidelines, “Keep code comments minimal: one or two lines at most”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/NodeInfoModule.cpp` around lines 54 - 56, Shorten the existing policy backstop comment in NodeInfoModule so it fits within one or two lines while preserving the key point: only broadcasts are dropped, unicast NodeInfo must still pass for signer-node exchanges, and this backstops ingress paths that bypass Router’s downgrade drop. Update the comment near the NodeInfoModule logic to be more concise without changing the behavior description.Source: Coding guidelines
src/mqtt/MQTT.cpp (1)
136-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim the ingress-policy comment.
The code already shows the lock and policy call; keep only the non-obvious “why”.
Suggested cleanup
- // Already-decoded downlink skips perhapsDecode's crypto path entirely, so enforce the - // signature policy here: verify a carried signature and apply unsigned-downgrade - // protection for known signers. Without this, a peer on a plaintext broker could - // impersonate a signing node with unsigned broadcasts. Hold cryptLock like the RF path - // (perhapsDecode) does - checkXeddsaReceivePolicy -> xeddsa_verify mutates shared - // CryptoEngine cache state, and MQTT ingress can run on a different task. + // Decoded MQTT skips perhapsDecode; enforce signature policy under cryptLock because + // verification mutates CryptoEngine cache state and may run from another task.As per coding guidelines, “Keep code comments minimal: one or two lines at most”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mqtt/MQTT.cpp` around lines 136 - 141, The ingress-policy comment in MQTT ingress is too verbose and repeats what the code already shows. Trim the comment around the cryptLock and checkXeddsaReceivePolicy path in MQTT.cpp to only the non-obvious reason for the lock/policy check, keeping it to one or two short lines and removing the step-by-step explanation.Source: Coding guidelines
test/test_mqtt/MQTT.cpp (1)
675-678: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCondense the audit-context comment.
The test name covers the expectation; keep the threat-model note brief.
Suggested cleanup
-// Decoded (plaintext-broker) downlink skips perhapsDecode's crypto path, so MQTT applies -// checkXeddsaReceivePolicy at ingress. An unsigned broadcast claiming to come from a node that -// previously signed must be dropped - without this, a rogue broker peer could impersonate any -// signing node (audit F3). +// Decoded MQTT must apply receive policy so unsigned broadcasts cannot spoof known signers.As per coding guidelines, “Keep code comments minimal: one or two lines at most”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_mqtt/MQTT.cpp` around lines 675 - 678, Shorten the audit-context comment in MQTT.cpp to a brief one- or two-line note, since the test name already captures the expectation. Keep the key threat-model point only: that a decoded plaintext downlink still hits checkXeddsaReceivePolicy at ingress and must reject unsigned broadcasts claiming to be from a previously signing node.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/xeddsa-unsigned-packet-policy.md`:
- Around line 16-18: Update the prose in the xeddsa unsigned packet policy doc
to match the grammar/style suggestions from static analysis: in the paragraph
describing trust-on-first-use, replace “afterwards” with “afterward,” and in the
later sentence around the signing capability wording, change “is able to sign”
to “can sign” for conciseness. Keep the meaning unchanged and adjust the
surrounding sentence flow in the same section if needed.
---
Nitpick comments:
In `@src/modules/NodeInfoModule.cpp`:
- Around line 54-56: Shorten the existing policy backstop comment in
NodeInfoModule so it fits within one or two lines while preserving the key
point: only broadcasts are dropped, unicast NodeInfo must still pass for
signer-node exchanges, and this backstops ingress paths that bypass Router’s
downgrade drop. Update the comment near the NodeInfoModule logic to be more
concise without changing the behavior description.
In `@src/mqtt/MQTT.cpp`:
- Around line 136-141: The ingress-policy comment in MQTT ingress is too verbose
and repeats what the code already shows. Trim the comment around the cryptLock
and checkXeddsaReceivePolicy path in MQTT.cpp to only the non-obvious reason for
the lock/policy check, keeping it to one or two short lines and removing the
step-by-step explanation.
In `@test/test_mqtt/MQTT.cpp`:
- Around line 675-678: Shorten the audit-context comment in MQTT.cpp to a brief
one- or two-line note, since the test name already captures the expectation.
Keep the key threat-model point only: that a decoded plaintext downlink still
hits checkXeddsaReceivePolicy at ingress and must reject unsigned broadcasts
claiming to be from a previously signing node.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b2140ec-504f-4546-8c84-f1684c17f3af
📒 Files selected for processing (10)
docs/xeddsa-unsigned-packet-policy.mdsrc/mesh/CryptoEngine.cppsrc/mesh/CryptoEngine.hsrc/mesh/Router.cppsrc/mesh/Router.hsrc/modules/NodeInfoModule.cppsrc/mqtt/MQTT.cpptest/test_crypto/test_main.cpptest/test_mqtt/MQTT.cpptest/test_packet_signing/test_main.cpp
Contributor
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens Meshtastic’s XEdDSA packet-signing policy across both RF receive/decode and plaintext MQTT downlink, and adds regression coverage to prevent unsigned-packet acceptance and size-gate edge cases from reappearing.
Changes:
- Extracts XEdDSA receive-side policy into
checkXeddsaReceivePolicy()and applies it to decoded MQTT ingress (including holdingcryptLock). - Replaces the old payload-length signing heuristic with an exact encoded-size gate (
signedDataFits) and mirrors that logic on receive to avoid sign/TOO_LARGE dead bands. - Expands native tests to cover the receive/send policy matrix, NodeInfo broadcast-only rule, encoding invariants, decoded-ingress policy, and deterministic signing behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_packet_signing/test_main.cpp | Adds extensive policy regression tests (RF round-trips + direct helper coverage) including size-boundary and malformed-signature cases. |
| test/test_mqtt/MQTT.cpp | Adds plaintext-decoded MQTT downlink policy tests and resets shared MockNodeDB state between tests. |
| test/test_crypto/test_main.cpp | Pins deterministic XEdDSA signing behavior via signature equality assertions. |
| src/mqtt/MQTT.cpp | Enforces XEdDSA receive policy (with cryptLock) on decoded MQTT ingress before enqueueing. |
| src/modules/NodeInfoModule.cpp | Restricts “drop unsigned NodeInfo from known signer” backstop to broadcasts only. |
| src/mesh/Router.h | Declares checkXeddsaReceivePolicy() as a reusable policy helper. |
| src/mesh/Router.cpp | Implements checkXeddsaReceivePolicy(), integrates it into perhapsDecode(), and introduces signedDataFits() for exact send-side gating. |
| src/mesh/CryptoEngine.h | Adds XEDDSA_SIGNATURE_FIELD_BYTES constant (pinned by new tests). |
| src/mesh/CryptoEngine.cpp | Removes vestigial RNG prefill and documents determinism expectations. |
thebentern
force-pushed
the
fix/xeddsa-signing-policy
branch
from
July 2, 2026 14:58
46708cb to
c8b8b9a
Compare
jp-bennett
requested changes
Jul 2, 2026
thebentern
force-pushed
the
fix/xeddsa-signing-policy
branch
from
July 2, 2026 15:20
c8b8b9a to
efe4887
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mesh/CryptoEngine.cpp (1)
119-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCondense this comment to match the C++ comment guideline.
The RNG seeding rationale is useful, but this 8-line block exceeds the project’s “one or two lines” comment style. Keep the why and move protocol details to docs/tests if needed. As per coding guidelines, “Keep code comments minimal: one or two lines at most, only explain the why when it is not obvious, and avoid multi-paragraph explanatory comments.”
Suggested condensation
- // XEdDSA per the Signal spec is a *randomized* scheme: the nonce is r = hash1(a || M || Z), - // where Z is caller-supplied randomness, giving hedged signatures (defense against fault / - // bad-RNG / side-channel attacks). meshtastic/Crypto#3 made XEdDSA::sign spec-compliant - it - // now mixes the first 32 bytes of `signature` into r as Z - so we must seed those bytes with - // entropy before signing. Prefer the hardware RNG; fall back to the seeded software CSPRNG if - // it is unavailable. A weak or repeated Z still yields a valid signature that is safe against - // nonce reuse (Z is only defense-in-depth), so we never fail signing over it. XEdDSA::sign - // overwrites all 64 bytes with R||s on return. + // Seed XEdDSA's caller-supplied nonce randomness (`Z`) for hedged signatures. + // Fall back to the seeded software CSPRNG; `XEdDSA::sign` overwrites the buffer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/CryptoEngine.cpp` around lines 119 - 126, Condense the explanatory block in CryptoEngine::sign/XEdDSA setup to a one- or two-line C++ comment. Keep only the essential “why” about seeding the first 32 bytes of signature with entropy before XEdDSA::sign and the preference for hardware RNG with software CSPRNG fallback; move the detailed protocol/background notes out of the comment and keep the reference to XEdDSA::sign and the signature buffer initialization clear.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/mesh/CryptoEngine.cpp`:
- Around line 119-126: Condense the explanatory block in
CryptoEngine::sign/XEdDSA setup to a one- or two-line C++ comment. Keep only the
essential “why” about seeding the first 32 bytes of signature with entropy
before XEdDSA::sign and the preference for hardware RNG with software CSPRNG
fallback; move the detailed protocol/background notes out of the comment and
keep the reference to XEdDSA::sign and the signature buffer initialization
clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c95a7a0-1b59-4cce-8265-b6187a51b2d2
📒 Files selected for processing (19)
src/mesh/CryptoEngine.cppsrc/mesh/CryptoEngine.hsrc/mesh/Router.cppsrc/mesh/Router.hsrc/modules/NodeInfoModule.cppsrc/mqtt/MQTT.cpptest/test_crypto/test_main.cpptest/test_mqtt/MQTT.cpptest/test_packet_signing/test_main.cppvariants/esp32/esp32-common.inivariants/esp32/esp32.inivariants/esp32p4/esp32p4.inivariants/native/portduino.inivariants/native/portduino/platformio.inivariants/nrf52840/nrf52.inivariants/nrf54l15/nrf54l15.inivariants/rp2040/rp2040.inivariants/rp2350/rp2350.inivariants/stm32/stm32.ini
✅ Files skipped from review due to trivial changes (4)
- variants/rp2350/rp2350.ini
- variants/nrf54l15/nrf54l15.ini
- variants/nrf52840/nrf52.ini
- src/mesh/CryptoEngine.h
🚧 Files skipped from review as they are similar to previous changes (6)
- src/mesh/Router.h
- src/mqtt/MQTT.cpp
- src/modules/NodeInfoModule.cpp
- src/mesh/Router.cpp
- test/test_mqtt/MQTT.cpp
- test/test_packet_signing/test_main.cpp
jp-bennett
approved these changes
Jul 2, 2026
Audit of the XEdDSA packet-signing implementation (#10478) surfaced several issues in when unsigned packets are accepted on receive or emitted on send. This fixes them and adds regression coverage. - Unicast NodeInfo exchange no longer breaks against signer nodes: the NodeInfoModule downgrade drop is gated to broadcasts, since senders never sign unicast (want_response replies, directed exchanges). - Replace the payload-size sign heuristic with an exact encoded-size gate (signedDataFits) and mirror it on the receive side, removing a dead band where 167-168 B broadcasts were signed then failed TOO_LARGE. - Extract the receive policy into checkXeddsaReceivePolicy() and apply it to plaintext-MQTT decoded downlink, which previously skipped signature verification and downgrade protection entirely. - Reject signatures whose length is neither 0 nor 64 as malformed, so a crafted partial signature can't inflate the size estimate and dodge the unsigned-downgrade drop. - Hold cryptLock on the MQTT verify path (shared Ed25519 key cache). - Clear any client-preset signature on packets we originate, on all builds. - Randomized (hedged) signing per the Signal XEdDSA spec: bump the meshtastic/Crypto pin to the build where XEdDSA::sign mixes 32 bytes of caller randomness into the nonce as Z (meshtastic/Crypto#3), and seed those bytes in xeddsa_sign from HardwareRNG (checked, with a seeded-CSPRNG fallback). test_crypto pins that repeated signs differ and both verify. Adds test coverage: test_packet_signing groups A-E (receive matrix, send policy, NodeInfo backstop, encoding invariants, decoded-ingress policy), test_mqtt end-to-end downlink cases, and a test_crypto randomization check.
thebentern
force-pushed
the
fix/xeddsa-signing-policy
branch
from
July 2, 2026 15:58
efe4887 to
eb29e34
Compare
Contributor
Firmware Size Report22 targets | vs
Show 17 more target(s)
Updated for 341b167 |
This was referenced Jul 9, 2026
This was referenced Jul 24, 2026
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
Audit of the XEdDSA packet-signing implementation (#10478) turned up several issues in when unsigned packets are accepted on receive or emitted on send. This PR fixes them and adds regression coverage.
Fixes
NodeInfoModuledropped any unsigned NodeInfo from a node whose signer bit was set — but unicast NodeInfo (want_response replies, phone-initiated exchanges) is never signed by the sender, so request and reply both died. The drop is now gated to broadcasts.payload + 64 < DATA_PAYLOAD_LEN) admitted 167–168 B broadcasts whose signed encoding overflowed the LoRa frame, so they were signed and then failedTOO_LARGE— undeliverable, though they'd have been fine unsigned. Replaced with an exact encoded-size gate (signedDataFits), and the receive-side downgrade predicate now mirrors it byte-for-byte.perhapsDecode's crypto path, so signature verification and downgrade protection were skipped entirely on plaintext brokers — a rogue peer could impersonate a signing node. The policy was extracted intocheckXeddsaReceivePolicy()and is now applied at MQTT ingress.xeddsa_verifymutates a shared Ed25519 key cache; the RF path holdscryptLockbut the new MQTT path did not (races the BLE/proxy task on nRF52). Now locked.r = hash1(a ‖ M ‖ Z)withZcaller-supplied randomness, giving hedged signatures. The vendored library was previously a deterministic Ed25519-style variant that ignoredZ. This PR bumps themeshtastic/Cryptopin to meshtastic/Crypto#3 (which makesXEdDSA::signmix the first 32 bytes of the signature buffer into the nonce asZ) and seeds those bytes inxeddsa_signfromHardwareRNG::fillwith a checked return and a seeded-CSPRNG fallback. Signing the same content twice now yields different, both-valid signatures; a weak/repeatedZstill produces a signature safe against nonce reuse, so signing never fails over RNG quality.Tests
test_packet_signing: groups A (receive matrix), B (send policy incl. exhaustive payload-size sweeps over two Data shapes), C (NodeInfo backstop), D (wire-format invariants), E (decoded-ingress policy incl. malformed-signature cases).test_mqtt: four end-to-end plaintext-downlink cases (drop unsigned-from-signer, accept non-signer, verify + learn signer bit, drop bad signature).test_crypto: deterministic-nonce pin.All native suites pass locally: 521/521 in the Docker coverage/ASan environment and via the
native-macostoolchain.Notes
Summary by CodeRabbit