AdminModule: only accept admin responses to requests we sent#11024
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesAdmin response solicitation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MeshService
participant AdminModule
participant Mesh
MeshService->>AdminModule: noteOutgoingAdminRequest(packet)
MeshService->>Mesh: forward admin packet
Mesh->>AdminModule: deliver admin response
AdminModule->>AdminModule: responseIsSolicited(packet, responseVariant)
AdminModule->>AdminModule: process solicited response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 2
🤖 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/modules/AdminModule.cpp`:
- Around line 1852-1853: Replace the raw sentAtSecs/millis-based expiry
arithmetic in the request response gate around slot->sentAtSecs and the related
lines 1864-1870 with the repository’s Throttle helper. Initialize and check
Throttle using elapsed-time semantics so the 300-second request expiry remains
correct across millis() rollover, while preserving the existing response
handling behavior.
In `@src/modules/AdminModule.h`:
- Around line 90-99: Update OutstandingAdminRequest and responseIsSolicited to
store and validate the expected response variant/tag alongside the destination
and key, so a request only authorizes its matching response type within the
existing timeout. Populate the expected tag when recording each getter request,
reject mismatched response tags, and add a test confirming an otherwise valid
response with the wrong variant is denied.
🪄 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: 8b8d9011-19d7-450e-bd7f-873840dc1d28
📒 Files selected for processing (5)
src/mesh/MeshService.cppsrc/modules/AdminModule.cppsrc/modules/AdminModule.htest/support/AdminModuleTestShim.htest/test_admin_session_repro/test_main.cpp
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
caveman99
added
bug
|
Both addressed in 21a8f96. millis() rollover: the expiry now uses Response-type binding: each outstanding record now carries a bitmask of the response variants its requests authorize ( |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/AdminModule.cpp (1)
1894-1911: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix rollover vulnerability in oldest-slot eviction and prevent resurrection of expired requests.
There are two logic issues in the slot allocation:
- Rollover hazard: As per coding guidelines, raw timestamp comparisons like
o.sentAtMs < slot->sentAtMsare vulnerable tomillis()rollover. Whenmillis()wraps, newer timestamps appear smaller than older pre-wrap timestamps, causing the newest pending request to be evicted instead of the oldest. Compare elapsed time instead.- Resurrection of expired requests: If an existing slot for the node is found but has already expired,
sameNoderemains true. This ORs the new request bit with the expiredexpectedResponsesmask, resetting the 5-minute timer and unintentionally resurrecting previously expired request types.♻️ Proposed fix
- // Reuse this remote's slot if it has one, else a free slot, else the oldest. - OutstandingAdminRequest *slot = nullptr; - for (auto &o : outstandingAdminRequests) - if (o.to == p.to) - slot = &o; - const bool sameNode = slot != nullptr; - if (!slot) - for (auto &o : outstandingAdminRequests) - if (o.to == 0) { - slot = &o; - break; - } - if (!slot) { - slot = &outstandingAdminRequests[0]; - for (auto &o : outstandingAdminRequests) - if (o.sentAtMs < slot->sentAtMs) - slot = &o; - } + // Reuse this remote's slot if it has one, else a free slot, else the oldest. + OutstandingAdminRequest *slot = nullptr; + for (auto &o : outstandingAdminRequests) { + if (o.to == p.to) { + slot = &o; + break; + } + } + + bool sameNode = slot != nullptr; + if (sameNode && !Throttle::isWithinTimespanMs(slot->sentAtMs, kOutstandingAdminRequestMs)) { + sameNode = false; // Expired; do not inherit stale expected responses + } + + if (!slot) { + for (auto &o : outstandingAdminRequests) { + if (o.to == 0) { + slot = &o; + break; + } + } + } + if (!slot) { + slot = &outstandingAdminRequests[0]; + uint32_t now = millis(); + for (auto &o : outstandingAdminRequests) { + if ((uint32_t)(now - o.sentAtMs) > (uint32_t)(now - slot->sentAtMs)) { + slot = &o; + } + } + }🤖 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/AdminModule.cpp` around lines 1894 - 1911, Update the slot allocation logic around outstandingAdminRequests to select the oldest slot using rollover-safe elapsed-time comparisons rather than raw sentAtMs ordering. When reusing a matching node slot, detect whether it has expired and clear/reset that slot’s prior request state so sameNode is false for the new request, preventing expired response bits from being resurrected or extending their timer.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.
Outside diff comments:
In `@src/modules/AdminModule.cpp`:
- Around line 1894-1911: Update the slot allocation logic around
outstandingAdminRequests to select the oldest slot using rollover-safe
elapsed-time comparisons rather than raw sentAtMs ordering. When reusing a
matching node slot, detect whether it has expired and clear/reset that slot’s
prior request state so sameNode is false for the new request, preventing expired
response bits from being resurrected or extending their timer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0efc028a-9f2e-4bec-a17e-c0c4be8fb11b
📒 Files selected for processing (3)
src/modules/AdminModule.cppsrc/modules/AdminModule.htest/test_admin_session_repro/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/modules/AdminModule.h
- test/test_admin_session_repro/test_main.cpp
thebentern
removed
bug
|
Definitely approve of the concept |
|
@copilot resolve the merge conflicts in this pull request |
Done — merged |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@bin/config.d/lora-ZebraHatDuo_R1_1W.yaml`:
- Around line 4-6: Correct the Meta.name value in the ZebraHatDuo_R1_1W
configuration from “Radio 0” to “Radio 1,” while leaving the other metadata
unchanged.
In `@variants/esp32s3/ELECROW-ThinkNode-M7/variant.cpp`:
- Around line 12-18: Remove the duplicate initVariant definition in
variants/esp32s3/ELECROW-ThinkNode-M7/variant.cpp, retaining the original
implementation. Also remove the duplicate getCurrentRSSI and enableInterrupt
definitions in src/mesh/SX126xInterface.cpp, preserving each original
implementation so every function has a single definition.
🪄 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: dfb4b76b-a733-4d87-8f26-eeb12c2e84b5
📒 Files selected for processing (14)
.github/workflows/nightly.yml.github/workflows/trunk_annotate_pr.yml.github/workflows/trunk_check.ymlbin/config.d/lora-ZebraHatDuo_R0_1W.yamlbin/config.d/lora-ZebraHatDuo_R1_1W.yamlbin/org.meshtastic.meshtasticd.metainfo.xmldebian/changelogsrc/detect/ScanI2CTwoWire.hsrc/graphics/draw/CompassRenderer.hsrc/mesh/SX126xInterface.cppsrc/platform/nrf52/NRF52Bluetooth.cppsrc/platform/nrf52/NRF52Bluetooth.hvariants/esp32s3/ELECROW-ThinkNode-M7/variant.cppvariants/esp32s3/m5stack_cardputer_adv/variant.h
💤 Files with no reviewable changes (1)
- src/graphics/draw/CompassRenderer.h
|
@copilot resolve the merge conflicts in this pull request |
An admin *_response short-circuited the auth and session-passkey checks that gate every other admin message, so any node could deliver one. On a channel the module listens to unauthenticated, a get_module_config_response drives the remote-hardware pin handler with attacker-supplied values. Track the destination of outgoing admin requests (per remote, with the pinned PKC key when there is one) and accept a response only from a node with a matching outstanding request, inside the same window as the session passkey. Local (from == 0) admin is unchanged; PhoneAPI already gates it. Also fix the response dispatch: get_module_config_response.which_payload_variant is a ModuleConfig oneof tag, but it was compared against the AdminMessage ModuleConfigType enum (different numbering), so the handler never ran. Compare against the oneof tag.
Two review refinements to the request/response pairing: Use Throttle::isWithinTimespanMs for the outstanding-request expiry instead of comparing millis()/1000 sums, which mis-expired across the millis() rollover. Bind each accepted response to a request type actually sent to that node. Each outstanding record now carries a bitmask of the response variants its requests authorize, so a get_owner request no longer admits a get_module_config response. The mask accumulates per remote, so a client may still pipeline several request types to one node and have every answer accepted.
|
I have no idea what happened here but i cleaned it up :-) |
c926cae to
74c85cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/modules/AdminModule.h`:
- Around line 90-95: Update OutstandingAdminRequest and its associated
request-tracking logic to store sentAtMs and PKC pinning metadata separately for
each response variant in expectedResponses, rather than sharing one timestamp
and key across the node. Ensure expiration and key validation use the metadata
for the specific variant, preserving older requests when later getters target
the same node; add pipeline coverage for staggered expiry and mixed
pinned/unpinned requests.
🪄 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: 18319b06-69d5-4787-ad86-630b01f1ac3a
📒 Files selected for processing (5)
src/mesh/MeshService.cppsrc/modules/AdminModule.cppsrc/modules/AdminModule.htest/support/AdminModuleTestShim.htest/test_admin_session_repro/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- test/support/AdminModuleTestShim.h
- src/mesh/MeshService.cpp
- src/modules/AdminModule.cpp
Reworks the outstanding-request table so each request is its own entry with its own expiry window and pinned key, replacing the per-node bitmask that shared one timestamp and one key across every response variant. That sharing let a later request to the same node extend an earlier one's window and, worse, clear its PKC pin: an unpinned request cleared keyValid, so a plaintext response to an earlier PKC-pinned request was then accepted. Per-request entries keep each pin intact. Identical requests are de-duplicated (a client may fetch several config subtypes, all answered by one response variant) and eviction compares elapsed time, which is rollover-safe. Test: a pinned request's response still requires its key after an unpinned request to the same node.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/modules/AdminModule.cpp`:
- Around line 1870-1904: Track the nested module-config request discriminator
alongside expectedResponse in the outstanding-request entries. Include it in
deduplication, and in handleGetModuleConfigResponse require
ModuleConfig.which_payload_variant to match the recorded discriminator before
accepting or mutating state; preserve the existing remote-admin key and
session-passkey authorization checks.
- Around line 1913-1925: Update the matching branch in the
outstanding-admin-request scan to consume the authorized entry before returning
true. After the checks in the loop over outstandingAdminRequests pass, clear or
remove the matched request so it cannot authorize another response during the
remaining timeout window; preserve the existing behavior for expired,
mismatched, or incorrectly pinned requests.
🪄 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: 8de0be28-3b71-49cf-b844-5e8da00e2f8a
📒 Files selected for processing (3)
src/modules/AdminModule.cppsrc/modules/AdminModule.htest/test_admin_session_repro/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/modules/AdminModule.h
- test/test_admin_session_repro/test_main.cpp
Two refinements to the request/response pairing: Only remote_hardware get_module_config_response mutates state (the pin table), so it must answer a request for that exact ModuleConfigType, not just any module-config request. Each entry records the requested subtype and the gate checks it. A matched request is now consumed on accept, so a node cannot replay a state-mutating response within the window. Because one request yields one response, request de-dup is dropped (a client's N indexed get_channel requests are N entries, each consumed once). Tests: a non-remote-hardware request does not admit a remote_hardware response, and a second copy of an answered response is rejected.
|
Both new findings addressed in 8b73e68 (and the per-variant/rollover ones stay fixed by the per-request model in d8e5dd4). Module-config subtype (AdminModule.cpp:1904): Consume on accept (AdminModule.cpp:1925): Tests: a non-remote-hardware request does not admit a |
Admin
*_responsemessages skip both the sender-authorization branch and thesession-passkey check that gate every other admin message:
The module is bound to no channel, so on any channel it can decode, a node can deliver
a
get_module_config_responseit never asked for.handleGetModuleConfigResponsethenwrites the responder's node_num and pin list into
devicestate.node_remote_hardware_pins.Accept a response only from a node we sent the matching request to.
MeshService::handleToRadionow records the destination of each outgoing admin request (per remote, with the pinned PKC key
when the client set one), and the response branch checks for a live matching request within the
same window as the session passkey. Local admin (
from == 0) is unchanged - PhoneAPI alreadygates that connection.
Second, smaller fix in the same path: the response dispatch compared
get_module_config_response.which_payload_variant(a ModuleConfig oneof tag,remote_hardware= 9)against
meshtastic_AdminMessage_ModuleConfigType_REMOTEHARDWARE_CONFIG(an AdminMessage enum, = 8).They never matched, so the handler was unreachable. It's compared against the oneof tag now, the
same way the request-side switch already builds the response.
Tests (test_admin_session_repro): the pairing gate directly - unsolicited response rejected,
solicited accepted, a request to one node does not admit another, and a setter does not arm the
gate - plus two end-to-end cases through handleReceivedProtobuf asserting the pin table is left
alone for an unsolicited response and updated for a solicited one (the latter also covering the
dispatch fix).
Note: touches AdminModule alongside the in-flight signing series (#10967, #10969, #10972); no
overlap on the response path, but they will need a trivial rebase around the switch.
Summary by CodeRabbit
Security Improvements
Bug Fixes
Tests