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.
niccellular