enhancement: Add rak4631_lockdown hardened build variant #9771
enhancement: Add rak4631_lockdown hardened build variant #9771niccellular wants to merge 5 commits into
Conversation
Introduces a new RAK4631 build variant (-DMESHTASTIC_LOCKDOWN=1) with encrypted flash storage, BLE/USB config access control, and hardened defaults. New files: - variants/nrf52840/rak4631_lockdown/ — PlatformIO env, variant header - src/security/EncryptedStorage.h/.cpp — AES-128-CTR + HMAC-SHA256 encrypted proto storage using CC310 hardware, passphrase-gated DEK, TTL/boot-count unlock token, backoff on failed attempts - src/security/APProtect.h/.cpp — UICR APPROTECT (SWD lockout) Modified files: - configuration.h — MESHTASTIC_LOCKDOWN flag derives PHONEAPI_ACCESS_CONTROL, ENCRYPTED_STORAGE, ENABLE_APPROTECT, HARDENED_DEFAULTS (nRF52 only) - NodeDB.cpp/.h — encrypted loadProto/saveProto, locked-boot early return, migration of plaintext files to MENC, hardened defaults, saveToDiskNoRetry guard prevents FSCom.format() when storage locked - PhoneAPI.cpp/.h — per-connection auth reset, redacts channel PSKs and security keys for unauthorized clients, revokeAllAuth() for Lock Now, post-config TAK_LOCKED/TAK_NEEDS_PROVISION notification - AdminModule.cpp — passphrase delivery via set_config(security), provision/unlock/re-verify flow, Lock Now sentinel (0xFF), PKC admin auth callback to PhoneAPI - PowerFSM.cpp — guard serialEnter() BLE disable on nRF52
|
What does the hardened mode do? |
Basically, increases the security posture of a physical node by encrypting the sensitive files on the filesystem, gates access to PhoneAPI based off passphrase, and disables logs and debug port. The intent was to make it harder for unauthorized people to do anything with an unattended node. |
There was a problem hiding this comment.
Pull request overview
Adds a hardened “lockdown” build flavor for the RAK4631 (nRF52840) that enables encrypted on-device protobuf storage, tighter local PhoneAPI access control with secret redaction, and optional SWD lockout (APPROTECT), all gated behind -DMESHTASTIC_LOCKDOWN=1.
Changes:
- Introduces a new
rak4631_lockdownPlatformIO env + variant header with hardened build flags and reduced feature surface. - Adds an encrypted storage layer (
EncryptedStorage) using CC310 crypto and integrates it intoNodeDBload/save and boot flow. - Adds connection-level PhoneAPI gating/redaction hooks and AdminModule flows for provisioning/unlocking/“Lock Now”.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| variants/nrf52840/rak4631_lockdown/variant.h | New lockdown variant header inheriting RAK4631 and disabling ethernet. |
| variants/nrf52840/rak4631_lockdown/platformio.ini | New PlatformIO env enabling MESHTASTIC_LOCKDOWN, muting logs, excluding modules. |
| src/security/EncryptedStorage.h | Public API + format documentation for encrypted proto storage, DEK/token scheme. |
| src/security/EncryptedStorage.cpp | CC310-backed AES-CTR + HMAC implementation, token/backoff logic, file I/O. |
| src/security/APProtect.h | Declares enableAPProtect() for SWD lockout builds. |
| src/security/APProtect.cpp | Implements UICR APPROTECT enablement on nRF52. |
| src/modules/AdminModule.cpp | Adds managed-mode gating, passphrase transport via security config, Lock Now flow. |
| src/mesh/PhoneAPI.h | Adds access-control flags/APIs for per-connection authorization + global authorization helper. |
| src/mesh/PhoneAPI.cpp | Enforces unauthorized-client gating, redacts secrets for unauthorized clients, sends lock/provision notifications. |
| src/mesh/NodeDB.h | Adds reloadFromDisk() for post-unlock runtime reload. |
| src/mesh/NodeDB.cpp | Encrypt/decrypt proto load/save paths; locked-boot behavior; plaintext→encrypted migration; save skip when locked. |
| src/main.cpp | Calls enableAPProtect() and EncryptedStorage::initLocked() at boot (when enabled). |
| src/configuration.h | Adds MESHTASTIC_LOCKDOWN macro derivations for nRF52. |
| src/PowerFSM.cpp | Avoids disabling BLE in serialEnter() on nRF52. |
| // private_key.bytes/size — raw passphrase (1–64 bytes) | ||
| // size==1, bytes[0]==0xFF — LOCK NOW sentinel | ||
| // admin_key[1].bytes[0] — boots_remaining for new token (0 → default) | ||
| // admin_key[2].bytes[0..3] LE u32 — valid_until_epoch (absolute Unix timestamp; 0 → no time limit) | ||
| { | ||
| const auto &sec = c.payload_variant.security; | ||
| const uint8_t *pp = sec.private_key.bytes; | ||
| size_t ppLen = sec.private_key.size; | ||
|
|
||
| // LOCK NOW sentinel — always honoured regardless of lock state | ||
| if (ppLen == 1 && pp[0] == 0xFF) { |
There was a problem hiding this comment.
Using ppLen==1 && pp[0]==0xFF as a LOCK NOW sentinel means a 1-byte passphrase value of 0xFF can never be used (even though passphrases are treated as raw bytes). Consider moving the sentinel to a separate field or using a multi-byte/structured marker to avoid colliding with valid passphrases.
| // private_key.bytes/size — raw passphrase (1–64 bytes) | |
| // size==1, bytes[0]==0xFF — LOCK NOW sentinel | |
| // admin_key[1].bytes[0] — boots_remaining for new token (0 → default) | |
| // admin_key[2].bytes[0..3] LE u32 — valid_until_epoch (absolute Unix timestamp; 0 → no time limit) | |
| { | |
| const auto &sec = c.payload_variant.security; | |
| const uint8_t *pp = sec.private_key.bytes; | |
| size_t ppLen = sec.private_key.size; | |
| // LOCK NOW sentinel — always honoured regardless of lock state | |
| if (ppLen == 1 && pp[0] == 0xFF) { | |
| // private_key.bytes/size — raw passphrase (1–64 bytes) | |
| // size==0 & admin_key[0].bytes[0]==0xFF | |
| // — LOCK NOW sentinel (uses admin_key[0] as a structured flag) | |
| // admin_key[1].bytes[0] — boots_remaining for new token (0 → default) | |
| // admin_key[2].bytes[0..3] LE u32 — valid_until_epoch (absolute Unix timestamp; 0 → no time limit) | |
| { | |
| const auto &sec = c.payload_variant.security; | |
| const uint8_t *pp = sec.private_key.bytes; | |
| size_t ppLen = sec.private_key.size; | |
| // LOCK NOW sentinel — always honoured regardless of lock state | |
| const auto &lockCmd = sec.admin_key[0]; | |
| if (ppLen == 0 && lockCmd.size == 1 && lockCmd.bytes[0] == 0xFF) { |
| memcpy(&plaintextLen, fileBuf + pos, 4); | ||
| pos += 4; | ||
|
|
||
| size_t ciphertextLen = fileSize - HEADER_SIZE - HMAC_SIZE; |
There was a problem hiding this comment.
plaintextLen is read from the header but never validated against the actual ciphertext length (fileSize - HEADER_SIZE - HMAC_SIZE). Because downstream code trusts outLen = plaintextLen, a tampered header could make callers read beyond decrypted data (uninitialized bytes) or accept truncated data. Validate plaintextLen == ciphertextLen (or at least plaintextLen <= ciphertextLen and decrypt only plaintextLen) before returning.
| size_t ciphertextLen = fileSize - HEADER_SIZE - HMAC_SIZE; | |
| size_t ciphertextLen = fileSize - HEADER_SIZE - HMAC_SIZE; | |
| if (plaintextLen == 0 || plaintextLen > ciphertextLen) { | |
| LOG_ERROR("EncryptedStorage: Invalid plaintext length %u (ciphertext length %u) in %s", | |
| (unsigned)plaintextLen, (unsigned)ciphertextLen, filename); | |
| memset(fileBuf, 0, fileSize); | |
| delete[] fileBuf; | |
| return false; | |
| } |
| #ifdef MESHTASTIC_PHONEAPI_ACCESS_CONTROL | ||
| // Authorize local PhoneAPI connections to dump full config | ||
| PhoneAPI::authorizeLocalAdmin(); | ||
| #endif |
There was a problem hiding this comment.
PhoneAPI::authorizeLocalAdmin() is triggered for any PKC admin payload with an authorized sender key, which sets a global flag affecting local PhoneAPI redaction/gating. That means a remote PKC admin message (or any other context that passes this check) can unintentionally authorize local config dumps/injection without a passphrase. Consider scoping this authorization to the originating connection/session (or at least gating this call so it only applies to truly local authorization events).
| #ifdef MESHTASTIC_PHONEAPI_ACCESS_CONTROL | |
| // Authorize local PhoneAPI connections to dump full config | |
| PhoneAPI::authorizeLocalAdmin(); | |
| #endif | |
| // Note: Do NOT authorize local PhoneAPI admin sessions based solely on a remote PKC admin payload. | |
| // Remote possession of an admin key should not globally relax local PhoneAPI redaction/gating. |
| LOG_ERROR("Error: can't encode protobuf %s", PB_GET_ERROR(&stream)); | ||
| delete[] pbBuf; | ||
| return false; | ||
| } | ||
|
|
||
| size_t encodedSize = stream.bytes_written; | ||
| bool ok = EncryptedStorage::encryptAndWrite(filename, pbBuf, encodedSize, fullAtomic); |
There was a problem hiding this comment.
pbBuf holds plaintext (including keys/PSKs) and is freed without being wiped after encryptAndWrite(). Consider zeroing pbBuf (at least the encoded bytes) before delete[] to avoid leaving sensitive material in heap memory.
| LOG_ERROR("Error: can't encode protobuf %s", PB_GET_ERROR(&stream)); | |
| delete[] pbBuf; | |
| return false; | |
| } | |
| size_t encodedSize = stream.bytes_written; | |
| bool ok = EncryptedStorage::encryptAndWrite(filename, pbBuf, encodedSize, fullAtomic); | |
| LOG_ERROR("Error: can't encode protobuf %s", PB_GET_ERROR(&stream)); | |
| // Wipe potentially sensitive data before freeing | |
| volatile uint8_t *pbBufVolatile = pbBuf; | |
| for (size_t i = 0; i < protoSize; ++i) { | |
| pbBufVolatile[i] = 0; | |
| } | |
| delete[] pbBuf; | |
| return false; | |
| } | |
| size_t encodedSize = stream.bytes_written; | |
| bool ok = EncryptedStorage::encryptAndWrite(filename, pbBuf, encodedSize, fullAtomic); | |
| // Wipe plaintext protobuf contents before freeing buffer | |
| volatile uint8_t *pbBufVolatile = pbBuf; | |
| for (size_t i = 0; i < encodedSize; ++i) { | |
| pbBufVolatile[i] = 0; | |
| } |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ions and logic bugs Three-agent security audit of the LOCKDOWN build identified race conditions, incomplete key zeroing, and missing memory hygiene. All findings except HIGH-1 are addressed here. HIGH-1 (revokeAllAuth not resetting per-instance isAdminAuthorized) was reviewed and closed as wontfix: PKC admin key holders have persistent access regardless, so the 7-second pre-reboot window changes nothing about the threat model. HIGH-2: lockNow() previously only zeroed dek[]. Now also zeroes kek[], ephemeralKek[], and their derived flags so no key material survives a lock event in a RAM dump. HIGH-3: isAdminAuthorized (per-instance) and s_localAdminAuthorized (static) changed from plain bool to std::atomic<bool>. Both flags are written from the NimBLE FreeRTOS task and read from the main loop — non-atomic reads were a data race. MED-1: readAndDecrypt() and encryptAndWrite() now snapshot dek[] into a stack- local dekSnapshot[] on entry before acquiring any lock. A concurrent lockNow() that zeros dek[] mid-operation either zeroes the snapshot (HMAC fails securely) or has no effect on an already-captured copy. dekSnapshot is zeroed on all exit paths. MED-2: close() and handleStartConfig() no longer clear s_localAdminAuthorized. Resetting it on every disconnect was an auth DoS — any new client connection would silently revoke the passphrase-authenticated state, forcing the operator to re-enter the passphrase on every reconnect. Device-level auth now persists until explicit Lock Now (revokeAllAuth()). MED-3: readAndDecrypt() scoped the spiLock LockGuard to a nested block covering only the FSCom.open/read/close sequence. The lock is released before any HMAC or AES-CTR work. spiLock is a non-recursive binary semaphore; holding it across CC310 calls created a re-entry deadlock risk. MED-4: AdminModule zeros the passphrase bytes in toRadioScratch via a volatile pointer cast before returning from both the Lock Now and provision/unlock paths. Prevents the passphrase lingering in the scratch buffer until the next ToRadio message overwrites it. MED-5: readAndDecrypt() was missing memset(fileBuf) on the hmacData OOM early- exit path, leaving ciphertext in the heap until the allocator reuses it. MED-6: readAndDecrypt() now sets outLen=0 on entry so callers never observe a stale length value on any early failure return. MED-7: readAndDecrypt() calls memset(outBuf, 0, ciphertextLen) on AES-CTR failure to clear partial plaintext that may have been written to the caller's buffer before the error was detected. MED-8: Passphrase length limit corrected from 64 to 32 bytes throughout (EncryptedStorage.cpp provisionPassphrase/unlockWithPassphrase, AdminModule.cpp ppLen range check, EncryptedStorage.h doc comments). The proto private_key field is 32 bytes; accepting 33-64 silently failed inside EncryptedStorage while appearing to succeed at the AdminModule gate. L-1: readAndDecrypt() now rejects files larger than 65536+54 bytes before allocating the heap buffer, preventing OOM or integer overflow on corrupt or attacker-supplied oversized files. L-2: migrateFile() has a precondition comment that spiLock must not be held by the caller; both isEncrypted() and encryptAndWrite() acquire it internally.
|
IMHO: Gating it to just nRF and just rak4631 doesn't feel like the right thing to do. AFAICT your problem statement is not well-defined; as @NomDeTom said:
Meshtastic devices have various access vectors: Serial, Bluetooth, Web Client, etc. and they all actually use a pretty common bunch of code. If lockdown mode is important to you, and you have a well-defined problem, securing the various device admin vectors (edit: and storage drivers, mainly LittleFS) seems like a worthy cause. Since it looks like most/all of the code is AI-generated, you may as well burn your credits/allowance on properly analyzing the existing code and coding a feature that is universal, and make it modular enough and well-tested enough that it can just be baked in by default (or with a build flag that is applicable across all variants). Several ESP32, STM32 spins also have hardware crypto blocks. If the code was actually created in a modular, OOP-style way, this could be very cleanly implemented on a per-arch basis then picked up appropriately by the build system. Personally, I have nothing against the intent of the PR, but I don't think it helps the Meshtastic devs to have to review and then have to request changes when the overall PR does not even try to sell itself as a feature someone might plausibly use. |
|
Secondly, "All changes are guarded by #ifdef — zero impact on any existing build." sounds like a sycophantic response intended to assure you; I don't think this is something that belongs in a PR description. I would suggest reviewing your workflow or prompt to provide actual summary and justification for your changes. I'm not related to Meshtastic devs, just a community contributor, but I think putting a bit more thought into your PR would help to get it in a shape that'll be much less painful to review, because as an outside observer, it was painful to read the diffs. |
Hello, It is implemented only for rak4631 due to that is the hardware I have on hand to test. My goal is to apply this to all nrf52 targets. Regarding the various access vectors; yes this address serial/usb and bluetooth. I forgot about TCP and will look into securing that as well. A "lockdown" mode is important to a lot of people not just me. All of this is AI-generated. I've read every line and the implementation passes the sniff test. The point of this PR is to bring in other experts to review what I believe is a worthy contribution. I've already gone through with multiple models using "agentic" passes to perform auditing focused on race conditions. I've tested this enough that I feel comfortable presenting and defending it to our community. Hence the PR. As mentioned in the discord, the plan here is NRF52 first, then work up an ESP32 solution. ESP32 has a secure boot/enclave that appears to be more disruptive to integrate which is why I held off. NRF52 was a lot simpler. For example, my understand is that ESP32 will require a new bootloader to leverage their hardware backed crypto. Also requires an ESP32-S3 or newer (which I need to acquire) I am a Meshtastic dev, I've already reviewed it. The point again is to get some other expert feedback. Cheers, |
Thanks again. This is great advice. |
|
Fair enough, there’s definitely some reviewing coming in.
Still not in favor of gating to a specific variant since all nRF52840 have
the encryption hardware.
The community can test other variants for you if needed, within the scope
of this PR.
…On Tue, Mar 3, 2026 at 00:12 Nick ***@***.***> wrote:
*niccellular* left a comment (meshtastic/firmware#9771)
<#9771 (comment)>
Secondly, "All changes are guarded by #ifdef — zero impact on any existing
build." sounds like a sycophantic response intended to assure you; I don't
think this is something that belongs in a PR description.
I would suggest reviewing your workflow or prompt to provide actual
summary and justification for your changes.
I'm not related to Meshtastic devs, just a community contributor, but I
think putting a bit more thought into your PR would help to get it in a
shape that'll be much less painful to review, because as an outside
observer, it was painful to read the diffs.
Secondly, "All changes are guarded by #ifdef — zero impact on any existing
build." sounds like a sycophantic response intended to assure you; I don't
think this is something that belongs in a PR description.
I would suggest reviewing your workflow or prompt to provide actual
summary and justification for your changes.
I'm not related to Meshtastic devs, just a community contributor, but I
think putting a bit more thought into your PR would help to get it in a
shape that'll be much less painful to review, because as an outside
observer, it was painful to read the diffs.
Thanks again. This is great advice.
—
Reply to this email directly, view it on GitHub
<#9771 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACAFAEM7UJ7S3G4BWGVUPL4OWXFLAVCNFSM6AAAAACWBXPDKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTSOBVGMZTIMBQG4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Yeah I figure baby steps. IMO this stuff should be core in all platforms. But you gotta start somewhere. |
| // Verify HMAC-SHA256(KEK, DEK_AUTH_LABEL || nonce || encDEK) | ||
| size_t authLabelLen = strlen(DEK_AUTH_LABEL); | ||
| size_t hmacInputLen = authLabelLen + NONCE_SIZE + AES_KEY_SIZE; | ||
| uint8_t *hmacInput = new uint8_t[hmacInputLen]; |
| } | ||
|
|
||
| size_t ciphertextLen = plaintextLen; | ||
| uint8_t *ciphertext = new uint8_t[ciphertextLen > 0 ? ciphertextLen : 1]; |
There was a problem hiding this comment.
You should make a template class that memset zeros in the destructor rather than memseting by hand.
Also std::unique_ptr
| * | ||
| * WARNING: This is a one-way operation. Once APPROTECT is enabled, | ||
| * the debug port is permanently disabled. The device can still be | ||
| * re-flashed via the bootloader (USB/DFU), but SWD/JTAG access is blocked. |
There was a problem hiding this comment.
I can't read this any other way than SWD is permanently disabled.
| memset(fromRadioScratch.config.payload_variant.network.wifi_psk, 0, | ||
| sizeof(fromRadioScratch.config.payload_variant.network.wifi_psk)); |
There was a problem hiding this comment.
| memset(fromRadioScratch.config.payload_variant.network.wifi_psk, 0, | |
| sizeof(fromRadioScratch.config.payload_variant.network.wifi_psk)); | |
| fromRadioScratch.config.payload_variant.network.wifi_psk = { 0 }; |
There was a problem hiding this comment.
Not an expert but doesn't that only zero out the first element (char?) not the entire array?
There was a problem hiding this comment.
to zero out the first element you would need to index into the left hand side like ...wifi_psk[0].
The part that I always find weird is that afaik { 0 } expands to the left-hand size's array size ? I think
There was a problem hiding this comment.
Oh i've only seen the = {0}; used in declarations is why I said something.
Summary
Adds a new hardened build variant for the RAK4631 (nRF52840): rak4631_lockdown, enabled by a single compile flag -DMESHTASTIC_LOCKDOWN=1.
New files:
Modified files:
All changes are guarded by #ifdef — zero impact on any existing build.
🤝 Attestations