mirror of
https://github.com/alexhopeoconnor/firmware.git
synced 2026-10-17 09:01:30 +10:00
fix audit findings from Opus 4.6 specifically looking for race conditions 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.
This commit is contained in:
+13
-7
@@ -59,12 +59,14 @@ void PhoneAPI::handleStartConfig()
|
||||
observe(&xModem.packetReady);
|
||||
#endif
|
||||
#ifdef MESHTASTIC_PHONEAPI_ACCESS_CONTROL
|
||||
// New physical connection: reset auth so the client must present the passphrase.
|
||||
// New physical connection: reset per-connection PKC auth.
|
||||
// Do NOT reset here on re-requests (want_config_id sent again within the same
|
||||
// connection after auth) — that would strip auth from a client who just unlocked
|
||||
// and is re-fetching the full unredacted config.
|
||||
isAdminAuthorized = false;
|
||||
s_localAdminAuthorized = false;
|
||||
// MED-2: s_localAdminAuthorized (device-level passphrase auth) is intentionally
|
||||
// NOT reset here. It persists until an explicit Lock Now (revokeAllAuth()).
|
||||
// Resetting it on every new connection would cause an auth DoS.
|
||||
#endif
|
||||
}
|
||||
|
||||
@@ -140,7 +142,10 @@ void PhoneAPI::close()
|
||||
heartbeatReceived = false;
|
||||
#ifdef MESHTASTIC_PHONEAPI_ACCESS_CONTROL
|
||||
isAdminAuthorized = false;
|
||||
s_localAdminAuthorized = false;
|
||||
// MED-2: do NOT reset s_localAdminAuthorized on disconnect.
|
||||
// It represents device-level passphrase auth and must persist across connections
|
||||
// until an explicit Lock Now (revokeAllAuth()). Resetting it here would revoke
|
||||
// any passphrase-authenticated session the moment a second client connects.
|
||||
#endif
|
||||
}
|
||||
}
|
||||
@@ -978,22 +983,23 @@ int PhoneAPI::onNotify(uint32_t newValue)
|
||||
}
|
||||
|
||||
#ifdef MESHTASTIC_PHONEAPI_ACCESS_CONTROL
|
||||
bool PhoneAPI::s_localAdminAuthorized = false;
|
||||
// HIGH-3: static atomic so BLE-task writes and main-loop reads are race-free.
|
||||
std::atomic<bool> PhoneAPI::s_localAdminAuthorized{false};
|
||||
|
||||
void PhoneAPI::authorizeLocalAdmin()
|
||||
{
|
||||
s_localAdminAuthorized = true;
|
||||
s_localAdminAuthorized.store(true);
|
||||
LOG_INFO("TAK: Local admin authorized via PKC");
|
||||
}
|
||||
|
||||
bool PhoneAPI::isLocalAdminAuthorized()
|
||||
{
|
||||
return s_localAdminAuthorized;
|
||||
return s_localAdminAuthorized.load();
|
||||
}
|
||||
|
||||
void PhoneAPI::revokeAllAuth()
|
||||
{
|
||||
s_localAdminAuthorized = false;
|
||||
s_localAdminAuthorized.store(false);
|
||||
LOG_INFO("TAK: All connection auth revoked (Lock Now)");
|
||||
}
|
||||
#endif
|
||||
|
||||
Reference in New Issue
Block a user