Strip inner-message padding when sizing unsigned broadcasts#11083
Conversation
The XEdDSA unsigned-downgrade check sized the decoded Data, but Data.payload is an opaque bytes field. Unknown fields inside the inner message inflated payload.size past the signable budget while still decoding, skipping the drop. Size the canonical re-encoding of the inner message for POSITION_APP, TELEMETRY_APP, WAYPOINT_APP and NODEINFO_APP. The canonical size is used only when it is no larger than the wire size, and the original payload is restored for downstream modules.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds canonical inner-payload sizing for XEdDSA receive-policy decisions and tests unsigned broadcasts containing padded Position, Waypoint, and User payloads or oversized Telemetry. ChangesXEdDSA canonical sizing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/mesh/Router.cpp (1)
455-476: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring exceeds the 1-2 line comment guideline.
This 22-line, 4-paragraph docstring is valuable content (it explains a subtle security property), but it directly contradicts the repo's explicit comment-length rule. Consider condensing to the essential "why" (canonical re-encoding strips padding to prevent budget-inflation bypass) and moving the deeper forward-compat rationale to a design doc/ADR referenced by a short pointer comment.
As per coding guidelines, "Keep code comments minimal—one or two lines maximum—and comment only when the reason is not obvious; do not restate straightforward code or add multi-paragraph explanatory blocks."
🤖 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/Router.cpp` around lines 455 - 476, Condense the docstring above the decoded Data sizing helper to one or two lines, retaining only that canonical re-encoding strips attacker-controlled inner-message padding to prevent signable-budget bypass. Remove the extended implementation and forward-compatibility discussion from the inline comment; reference the appropriate design documentation only if an established ADR or design document exists.Source: Coding guidelines
test/test_packet_signing/test_main.cpp (2)
780-831: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWaypoint/NodeInfo canonical-sizing paths remain untested.
canonicalSignableSize()explicitly recognizes 4 port types (Position, Telemetry, Waypoint, NodeInfo/User), but E10 only exercises the Position drop path and E11 only exercises the Telemetry accept path. The Waypoint and NodeInfo branches of the new switch statement have no equivalent padding-rejection coverage, leaving two of the four recognized inner types unverified against the same attack this PR targets.Consider mirroring E10's pattern for
WAYPOINT_APPandNODEINFO_APP/meshtastic_Userpayloads.🤖 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_packet_signing/test_main.cpp` around lines 780 - 831, Add tests covering the Waypoint and NodeInfo/User branches of canonicalSignableSize() using the E10 pattern: construct valid encoded payloads, append an unknown field inside the payload, assert the padded frame remains radio-sendable but no longer fits signed encoding, and verify checkXeddsaReceivePolicy() rejects unsigned packets from a signer. Use WAYPOINT_APP and NODEINFO_APP with meshtastic_User payloads, while preserving E11’s oversized-message acceptance behavior.
775-779: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame comment-length guideline violation as the new Router.cpp docstring.
These 4-5 line explanatory blocks preceding the E10/E11 tests are useful but exceed the documented 1-2 line comment limit.
As per coding guidelines, "Keep code comments minimal—one or two lines—and comment only when the reason is not obvious."
Also applies to: 807-810
🤖 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_packet_signing/test_main.cpp` around lines 775 - 779, Shorten the explanatory comment blocks preceding the E10 and E11 tests to one or two concise lines each, preserving only the non-obvious reason for the test behavior. Update the comments near the E10 and E11 test sections without changing the tests or their logic.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/Router.cpp`:
- Around line 455-476: Condense the docstring above the decoded Data sizing
helper to one or two lines, retaining only that canonical re-encoding strips
attacker-controlled inner-message padding to prevent signable-budget bypass.
Remove the extended implementation and forward-compatibility discussion from the
inline comment; reference the appropriate design documentation only if an
established ADR or design document exists.
In `@test/test_packet_signing/test_main.cpp`:
- Around line 780-831: Add tests covering the Waypoint and NodeInfo/User
branches of canonicalSignableSize() using the E10 pattern: construct valid
encoded payloads, append an unknown field inside the payload, assert the padded
frame remains radio-sendable but no longer fits signed encoding, and verify
checkXeddsaReceivePolicy() rejects unsigned packets from a signer. Use
WAYPOINT_APP and NODEINFO_APP with meshtastic_User payloads, while preserving
E11’s oversized-message acceptance behavior.
- Around line 775-779: Shorten the explanatory comment blocks preceding the E10
and E11 tests to one or two concise lines each, preserving only the non-obvious
reason for the test behavior. Update the comments near the E10 and E11 test
sections without changing the tests or their logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d0a9d3da-6706-46e0-8ab6-c27bd32ff5ec
📒 Files selected for processing (2)
src/mesh/Router.cpptest/test_packet_signing/test_main.cpp
caveman99
added
bugfix
Shorten the new comment blocks to match the repo comment guideline. Add E12 and E13 covering the Waypoint and NodeInfo branches of the canonical-sizing switch, factoring the padded-broadcast construction into a shared helper. Suppress the cppcheck unusedStructMember warnings on the scratch union.
1 participant
The XEdDSA unsigned-downgrade check in
checkXeddsaReceivePolicysized the decodedData, butData.payloadis an opaque bytes field. Unknown fields inside the inner message inflatedpayload.sizepast the signable budget while the inner message still decoded, so the drop was skipped.canonicalSignableSize()now measures the canonical re-encoding of the inner message forPOSITION_APP,TELEMETRY_APP,WAYPOINT_APPandNODEINFO_APP. The canonical size is substituted only when it is no larger than the wire size, and the original payload is restored for downstream modules. Other portnums keep their wire size.Waypoint (199 bytes max) and Telemetry (272 bytes max) can legitimately exceed the signable budget, so the size exemption is retained rather than requiring a signature unconditionally.
Canonical sizing measures what the current schema can decode. Adding a field to
Dataor to any of these four inner types shrinks the measurement, so their legitimate maximums need re-checking against the boundary when the schema grows. This is documented on the helper.Tests:
test_packet_signingE10 (payload-padded unsigned Position from a known signer is dropped) and E11 (honest oversized HostMetrics from a known signer is accepted).Summary by CodeRabbit