PhoneAPI: gate local admin on the connection, not the wire from#11033
PhoneAPI: gate local admin on the connection, not the wire from#11033caveman99 wants to merge 3 commits into
Conversation
The lockdown admin check in handleToRadioPacket only ran when p.from == 0. from is a client-supplied wire field, and MeshService::handleToRadio rewrites it to 0 before AdminModule sees the packet. A client could therefore set from != 0 to skip the !getAdminAuthorized() drop, then have the packet normalized back to a local-admin identity and executed - unauthorized admin from an unauthorized connection. Every packet in handleToRadioPacket already comes from the local connection, so locality is a property of the connection, not of from. Move the decision into classifyLocalAdminPacket(), which ignores from and keys only on the admin variant and the connection's authorization: lockdown_auth is delivered inline, any other admin from an unauthorized connection is dropped, authorized admin passes through. The classifier is compiled unconditionally and unit-tested; the guarded caller (built only in the nRF52 lockdown config) calls it. Test: an unauthorized connection's ADMIN_APP packet with from != 0 is classified DropUnauthorized.
|
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)
📝 WalkthroughWalkthroughPhoneAPI now classifies phone-to-radio admin packets using connection authorization instead of the packet’s wire sender, handles lockdown authentication inline, drops unauthorized admin traffic, and adds tests covering spoofed sender IDs and non-admin packets. ChangesLocal admin authorization
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/mesh/PhoneAPI.h (1)
332-338: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the docstring to comply with comment length guidelines.
The documentation comment on this function spans 5 lines, which exceeds the repository's guideline of one or two lines maximum. 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."
♻️ Proposed refactor
- /// Classify a phone->radio packet for the lockdown admin gate. Deliberately independent of the - /// wire `from` field: every packet here originates from the local connection and the phone does - /// not own `from` (MeshService rewrites it), so keying the gate on `from` lets a client bypass - /// it by setting `from != 0`. Static and free of connection state so it is unit-testable; the - /// caller supplies the connection's authorization and consumes `outAdmin` for the lockdown case. + /// Classify a phone->radio packet for the lockdown admin gate (independent of the wire `from` + /// field to prevent bypass). static LocalAdminGate classifyLocalAdminPacket(const meshtastic_MeshPacket &p, bool adminAuthorized, meshtastic_AdminMessage &outAdmin);🤖 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/PhoneAPI.h` around lines 332 - 338, Shorten the documentation comment for classifyLocalAdminPacket to one or two concise lines, retaining only the non-obvious security rationale that classification must ignore the packet’s from field and use the connection’s authorization.Source: Coding guidelines
src/mesh/PhoneAPI.cpp (1)
1737-1739: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the comment to comply with comment length guidelines.
This comment spans 3 lines, which exceeds the repository's guideline of one or two lines maximum. 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."
♻️ Proposed refactor
- // Gate on the connection, not the wire `from`. `from` is attacker-controlled and MeshService - // rewrites it to 0 downstream, so keying this on `from == 0` let a client bypass the check by - // sending `from != 0` and have AdminModule treat it as authorized local admin. + // Gate on the connection, not the wire `from`, because `from` can be bypassed by an + // attacker setting it to a non-zero value prior to normalization.🤖 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/PhoneAPI.cpp` around lines 1737 - 1739, Shorten the comment above the connection gate to no more than two lines while preserving only the essential rationale: authorization must depend on the connection because the attacker-controlled from value is rewritten downstream and can bypass the check.Source: Coding guidelines
test/test_stream_api/test_main.cpp (1)
423-425: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the comment to comply with comment length guidelines.
This comment spans 3 lines, which exceeds the repository's guideline of one or two lines maximum. As per coding guidelines, "Keep comments minimal—one or two lines—and comment only when the reason is not obvious."
♻️ Proposed refactor
-// The lockdown admin gate must decide on the connection's authorization, not the wire `from`. A -// client that sets from != 0 previously skipped the gate, so an unauthorized connection could run -// admin. classifyLocalAdminPacket ignores `from`, so the same spoofed packet is still dropped. +// The lockdown admin gate must decide on the connection's authorization, not the wire `from`. +// classifyLocalAdminPacket ignores `from`, ensuring a spoofed packet is still dropped.🤖 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_stream_api/test_main.cpp` around lines 423 - 425, Shorten the comment describing the lockdown admin authorization behavior to no more than two lines, preserving only the essential reason: authorization must use the connection rather than the spoofable wire from value.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 `@src/mesh/PhoneAPI.cpp`:
- Around line 1745-1750: Update the cleanup block around
handleLockdownAuthInline to explicitly wipe the encoded passphrase stored in
p.decoded.payload.bytes before returning true. Keep the existing
admin.lockdown_auth.passphrase.bytes cleanup, and ensure the MeshPacket payload
bytes are cleared using the same secure volatile wipe approach.
---
Nitpick comments:
In `@src/mesh/PhoneAPI.cpp`:
- Around line 1737-1739: Shorten the comment above the connection gate to no
more than two lines while preserving only the essential rationale: authorization
must depend on the connection because the attacker-controlled from value is
rewritten downstream and can bypass the check.
In `@src/mesh/PhoneAPI.h`:
- Around line 332-338: Shorten the documentation comment for
classifyLocalAdminPacket to one or two concise lines, retaining only the
non-obvious security rationale that classification must ignore the packet’s from
field and use the connection’s authorization.
In `@test/test_stream_api/test_main.cpp`:
- Around line 423-425: Shorten the comment describing the lockdown admin
authorization behavior to no more than two lines, preserving only the essential
reason: authorization must use the connection rather than the spoofable wire
from value.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eb6c8b76-4c41-4aba-832e-62863050598b
📒 Files selected for processing (3)
src/mesh/PhoneAPI.cppsrc/mesh/PhoneAPI.htest/test_stream_api/test_main.cpp
|
All three addressed in 6bcc699. Encoded passphrase wipe (Major): correct - Two comment-length nitpicks: shortened the Note this whole block is in the |
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
2 participants
Build config affected: nRF52 with
MESHTASTIC_ENABLE_LOCKDOWN=1(which co-definesMESHTASTIC_PHONEAPI_ACCESS_CONTROLandMESHTASTIC_ENCRYPTED_STORAGE).The per-connection admin check in
handleToRadioPacketwas guarded byif (p.from == 0 && ...):p.fromis a client-supplied wire field. The outer lockdown gate lets a decodedADMIN_APPpacket addressed to us through without checking
from, andMeshService::handleToRadiorewrites
p.from = 0beforeAdminModuleruns. So a client that setsfrom != 0skips the!getAdminAuthorized()drop entirely, the packet is normalized back to a local identity, andAdminModuletreats it as authorized local admin. An unauthorized local connection can thenrun arbitrary admin once the device has been unlocked.
Every packet reaching
handleToRadioPacketalready originates from the local connection, sowhether it is "local admin" is a property of the connection, not of the wire
from. Thedecision now lives in
classifyLocalAdminPacket(), which ignoresfromand keys only on theadmin variant and the connection's authorization:
lockdown_authis delivered inline (it isitself the authentication), any other admin from an unauthorized connection is dropped, and an
authorized connection passes through. Authorized flows are unchanged.
The classifier is a static method compiled unconditionally, so it is unit-testable; the guarded
caller (built only in the lockdown config) calls it. No CI target builds
MESHTASTIC_ENABLE_LOCKDOWN=1, so the guarded caller is verified by inspection while thesecurity-critical decision is covered by the test.
Test (test_stream_api): an unauthorized connection's
ADMIN_APPpacket withfrom != 0isclassified
DropUnauthorized; an authorized connection's identical packet passes through;lockdown_authis delivered inline regardless offrom; a non-admin packet is outside the gate.Summary by CodeRabbit