fix(lockdown): re-lock serial console admin auth on USB link drop#11028
Open
thebentern wants to merge 1 commit into
Open
fix(lockdown): re-lock serial console admin auth on USB link drop#11028thebentern wants to merge 1 commit into
thebentern wants to merge 1 commit into
Conversation
On lockdown builds (MESHTASTIC_PHONEAPI_ACCESS_CONTROL, nRF52) the SerialConsole is a process-lifetime singleton, so the per-connection admin-auth slot keyed by its inherited PhoneAPI* is reused for every USB/serial client for the whole boot. An operator's admin unlock stayed latched across serial client swaps: an attacker plugging into the USB/serial port before the prior session's 15-minute inactivity timeout inherited admin authorization -- the serial analog of the BLE stale-session reuse bug closed by resetting state in onConnect()/onDisconnect(). Sample the USB-CDC host link (DTR/mount) state each runOnce(); on the link-drop edge call close(), which frees the auth slot and resets PhoneAPI state so whoever connects next re-locks via handleStartConfig()'s !isConnected() branch on their first want_config -- the same physical-link boundary BLE enforces in onConnect(). On the nRF52 TinyUSB (Adafruit) core, (bool)Port == tud_cdc_n_connected(), which goes false on cable unplug or host port-close. Console transports without a real DTR line fall back to the existing inactivity timeout, no worse than before. Entirely nRF52-lockdown-gated; non-lockdown builds are byte-identical.
Contributor
📝 WalkthroughWalkthrough
ChangesSerial link session reset
Estimated code review effort: 2 (Simple) | ~10 minutes 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}
Contributor
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/SerialConsole.cpp`:
- Around line 34-40: The comments in src/SerialConsole.cpp lines 34-40 and
102-118 are overly verbose; reduce each explanatory block to no more than one or
two lines. Keep the file-scope comment focused on s_serialLinkUp and its
purpose, and keep the runOnce() comment focused on calling close() to re-lock
admin authentication after the USB-CDC link drops.
🪄 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: d12873eb-f049-494f-93fb-b19e17610596
📒 Files selected for processing (1)
src/SerialConsole.cpp
Comment on lines
+34
to
+40
| // Last-seen USB-CDC host link (DTR/mount) state, sampled each runOnce() so a | ||
| // physical unplug/replug re-locks the per-connection admin auth (see runOnce()). | ||
| // Kept at file scope rather than as a member both because there is exactly one | ||
| // console singleton and because adding per-instance members to the PhoneAPI | ||
| // hierarchy has historically perturbed nRF52 USB-CDC enumeration (see PhoneAPI.h). | ||
| // Only compiled on lockdown (nRF52) builds. | ||
| static bool s_serialLinkUp = false; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep code comments minimal.
Both of these locations add multi-paragraph explanatory blocks, which violates the coding guidelines for C++ files requiring comments to be one or two lines maximum. As per coding guidelines, do not restate straightforward code or add multi-paragraph explanatory blocks.
src/SerialConsole.cpp#L34-L40: reduce this file-scope rationale to one or two lines (e.g.,// Last-seen USB-CDC host link state; sampled each runOnce() to re-lock admin auth on unplug. Kept at file scope to avoid perturbing nRF52 USB-CDC enumeration with per-instance members.).src/SerialConsole.cpp#L102-L118: reduce this detailed behavior explanation insiderunOnce()to one or two lines (e.g.,// Re-lock admin auth when the physical USB-CDC link drops by calling close(). This frees the auth slot and resets the session for the next client.).
📍 Affects 1 file
src/SerialConsole.cpp#L34-L40(this comment)src/SerialConsole.cpp#L102-L118
🤖 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/SerialConsole.cpp` around lines 34 - 40, The comments in
src/SerialConsole.cpp lines 34-40 and 102-118 are overly verbose; reduce each
explanatory block to no more than one or two lines. Keep the file-scope comment
focused on s_serialLinkUp and its purpose, and keep the runOnce() comment
focused on calling close() to re-lock admin authentication after the USB-CDC
link drops.
Source: Coding guidelines
Contributor
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (29)
Build artifacts expire on 2026-08-15. Updated for |
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
1 participant
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.
The bug
On lockdown builds (
MESHTASTIC_PHONEAPI_ACCESS_CONTROL+MESHTASTIC_ENCRYPTED_STORAGE, nRF52 — seeconfiguration.h), per-connection admin authorization persists across serial-console client swaps.SerialConsoleis a process-lifetime singleton (src/SerialConsole.cpp, globalconsole) and inheritsPhoneAPIviaStreamAPI, so the samePhoneAPIobject services every USB/serial client for the whole boot. Admin auth lives in the file-staticg_authSlotstable keyed by thePhoneAPI*this. Because the console'sthisnever changes, an operator's unlock stays latched in that slot. The only re-lock paths for the serial console wereclose()(15-min inactivity timeout / explicit disconnect) and the!isConnected()branch ofhandleStartConfig().Impact: an attacker who plugs into the USB/serial port before the prior authenticated session times out inherits admin authorization — the same stale-state-on-reuse class the BLE
onConnect()/resetBleSessionStatefix just closed, with no serial equivalent until now.The fix
Give the serial console a per-physical-link re-lock. Each
runOnce()samples the USB-CDC host link state; on the link-drop edge it callsclose(), which frees the auth slot (clearAuthSlot_LH) and resetsPhoneAPIstate toSTATE_SEND_NOTHING. Whoever connects next then re-locks through the existing!isConnected()branch ofhandleStartConfig()on their firstwant_config— the same physical-link boundary BLE enforces inonConnect().On the nRF52 TinyUSB (Adafruit) core,
(bool)Port == tud_cdc_n_connected(): true while a host holds the CDC port open with DTR asserted, false on cable unplug or host port-close.Design notes
#ifdef MESHTASTIC_PHONEAPI_ACCESS_CONTROL-gated (nRF52-only). Non-lockdown builds compile byte-identically; when access control is off,getAdminAuthorized()returnstrueunconditionally so the gates are already no-ops.close()only.close()already clears the slot and resets state, so a rising-edgesetAdminAuthorized(false)would be redundant. (setAdminAuthorizedis also declared under the access-control#ifdef, and in this TUPhoneAPI.his parsed beforeconfiguration.hdefines the macro —close()is unconditionally declared and is the right primitive here.)static, not a member — per the note atPhoneAPI.hthat per-instance members on the PhoneAPI hierarchy have perturbed nRF52 USB-CDC enumeration. Safe becauseconsoleis a singleton.onConnectevent, so it relies on the disconnect edge + the new client'shandleStartConfigre-lock rather than an unconditional connect-time clear. Reaching an un-cleared authorized slot requires the passphrase (everysetAdminAuthorized(true)path is passphrase-gated) and the next client re-locks before any config is emitted, so the gap isn't exploitable.runOncepoll on a battery-powered device is beyond "reconnect before timeout."PhoneAPItransports — BLE (already fixed) and the serial console (this PR).Testing
rak4631_ctf_dev, real nRF52 toolchain):SerialConsole.cppcompiles + links, firmware.hex/OTA produced.clang-format16.0.3 (repo config) — no changes.#ifdef).🤖 Generated with Claude Code
Summary by CodeRabbit