mirror of
https://github.com/liquidraver/ZephCore.git
synced 2026-09-01 21:08:19 +00:00
crypto: harden all crypto-sensitive memcmp + memset sites
Audit-driven sweep found additional compiler-optimization-sensitive patterns beyond the login password compare just fixed: P4.F3 (HIGH) — Utils::MACThenDecrypt verified packet MACs with plain memcmp. Runs on EVERY encrypted-then-MAC'd packet in the mesh; a timing oracle here lets attackers forge MACs byte-by-byte across the whole mesh layer. Replaced with constantTimeEqual. P4.F4 (MEDIUM) — Multiple memset(secret, 0, ...) calls on stack-resident crypto buffers (Ed25519 seed, ADC noise pool, AES key derived in extract_via_aes_ctr, HWINFO unique ID) were subject to dead-store elimination under -Os. GCC/Clang routinely elide these when the buffer is never read after; the wipe vanishes and the secret persists on stack until next call overwrites. Replaced with secureZeroize using volatile pointer writes. P4.F5 (LOW) — Identity::validatePrivateKey boot self-test compared shared secrets with plain memcmp. Boot-only, no attacker observation channel, but hygiene matters and the fix is one line. Also added secret-wipe for ss1/ss2 on all return paths. Promoted the local ct_memeq() previously added to RepeaterMesh.cpp into Utils::constantTimeEqual + Utils::secureZeroize (Utils.h/cpp) so the login compare and MAC compare share the same audited helper. Both helpers verified by Thumb-2 disassembly on rak3401_1watt: - constantTimeEqual: loop branches on iterator, accumulator load-modify-stored to stack every iteration, final return uses CLZ+LSR (no conditional branch on result). - secureZeroize: STRB.W to memory in a counted loop, not replaced with memset builtin and not eliminated.
This commit is contained in:
@@ -10,6 +10,7 @@
|
||||
#include <zephyr/drivers/hwinfo.h>
|
||||
#include <psa/crypto.h>
|
||||
#include <string.h>
|
||||
#include <mesh/Utils.h>
|
||||
|
||||
BUILD_ASSERT(IS_ENABLED(CONFIG_CSPRNG_ENABLED),
|
||||
"ZephyrRNG requires CONFIG_CSPRNG_ENABLED for cryptographic key derivation");
|
||||
@@ -163,7 +164,8 @@ static int extract_via_aes_ctr(const uint8_t *pool, size_t pool_len,
|
||||
|
||||
psa_key_id_t key_id = 0;
|
||||
status = psa_import_key(&attr, key, sizeof(key), &key_id);
|
||||
memset(key, 0, sizeof(key));
|
||||
/* Wipe stack-resident AES key — secureZeroize survives -Os DSE. */
|
||||
Utils::secureZeroize(key, sizeof(key));
|
||||
if (status != PSA_SUCCESS) {
|
||||
return -1;
|
||||
}
|
||||
@@ -192,11 +194,11 @@ static int extract_via_aes_ctr(const uint8_t *pool, size_t pool_len,
|
||||
for (int i = sizeof(counter) - 1; i >= 0; i--) {
|
||||
if (++counter[i] != 0) break;
|
||||
}
|
||||
memset(block, 0, sizeof(block));
|
||||
Utils::secureZeroize(block, sizeof(block));
|
||||
}
|
||||
|
||||
psa_destroy_key(key_id);
|
||||
memset(counter, 0, sizeof(counter));
|
||||
Utils::secureZeroize(counter, sizeof(counter));
|
||||
return ret;
|
||||
}
|
||||
|
||||
@@ -262,9 +264,11 @@ void ZephyrRNG::mixIdentitySeed(uint8_t *out, size_t out_len,
|
||||
sys_reboot(SYS_REBOOT_COLD);
|
||||
}
|
||||
|
||||
/* Wipe sensitive intermediate buffers */
|
||||
memset(pool, 0, sizeof(pool));
|
||||
memset(devid, 0, sizeof(devid));
|
||||
/* Wipe sensitive intermediate buffers — secureZeroize survives the
|
||||
* -Os dead-store-elimination that would silently elide plain memset
|
||||
* on stack locals that are never read again. */
|
||||
Utils::secureZeroize(pool, sizeof(pool));
|
||||
Utils::secureZeroize(devid, sizeof(devid));
|
||||
}
|
||||
|
||||
} /* namespace mesh */
|
||||
|
||||
@@ -118,28 +118,6 @@ void RepeaterMesh::putNeighbour(const mesh::Identity& id, uint32_t timestamp, fl
|
||||
#endif
|
||||
}
|
||||
|
||||
/* Constant-time byte-equality over a fixed length. Returns true iff every
|
||||
* byte of `a` matches `b`. No early exit — timing is independent of input,
|
||||
* defeating timing-leak attacks against password comparison.
|
||||
*
|
||||
* `volatile` on the accumulator prevents the compiler from short-circuiting
|
||||
* the XOR-OR loop back into a branching memcmp under aggressive LTO. Pattern
|
||||
* matches rweather/arduinolibs Crypto.cpp secure_compare(). mbedtls offers
|
||||
* mbedtls_ct_memcmp() but its implementation lives in a .c file that isn't
|
||||
* pulled into the Zephyr mbedtls build under current Kconfig — would require
|
||||
* enabling additional TLS features just for this one function.
|
||||
*
|
||||
* Mitigation for the password-handling weakness tracked upstream as
|
||||
* meshcore-dev/MeshCore#2556. See CRYPTO_AUDIT_INDEX.md P4.F1. */
|
||||
static bool ct_memeq(const uint8_t *a, const uint8_t *b, size_t n)
|
||||
{
|
||||
volatile uint8_t result = 0;
|
||||
for (size_t i = 0; i < n; i++) {
|
||||
result |= (uint8_t)(a[i] ^ b[i]);
|
||||
}
|
||||
return result == 0;
|
||||
}
|
||||
|
||||
uint8_t RepeaterMesh::handleLoginReq(const mesh::Identity& sender, const uint8_t* secret, uint32_t sender_timestamp, const uint8_t* data, bool is_flood) {
|
||||
ClientInfo* client = nullptr;
|
||||
|
||||
@@ -160,12 +138,12 @@ uint8_t RepeaterMesh::handleLoginReq(const mesh::Identity& sender, const uint8_t
|
||||
size_t r_len = strnlen((const char *)data, sizeof(received) - 1);
|
||||
memcpy(received, data, r_len);
|
||||
|
||||
bool admin_match = ct_memeq(received,
|
||||
(const uint8_t *)_prefs.password,
|
||||
sizeof(received));
|
||||
bool guest_match = ct_memeq(received,
|
||||
(const uint8_t *)_prefs.guest_password,
|
||||
sizeof(received));
|
||||
bool admin_match = mesh::Utils::constantTimeEqual(received,
|
||||
_prefs.password,
|
||||
sizeof(received));
|
||||
bool guest_match = mesh::Utils::constantTimeEqual(received,
|
||||
_prefs.guest_password,
|
||||
sizeof(received));
|
||||
|
||||
if (admin_match) {
|
||||
perms = PERM_ACL_ADMIN;
|
||||
|
||||
@@ -19,6 +19,21 @@ public:
|
||||
static int decrypt(const uint8_t *shared_secret, uint8_t *dest, const uint8_t *src, int src_len);
|
||||
static int encryptThenMAC(const uint8_t *shared_secret, uint8_t *dest, const uint8_t *src, int src_len);
|
||||
static int MACThenDecrypt(const uint8_t *shared_secret, uint8_t *dest, const uint8_t *src, int src_len);
|
||||
|
||||
/* Constant-time byte-equality. Returns true iff every byte of `a`
|
||||
* matches `b`. No early exit — timing is independent of input,
|
||||
* defeating timing-leak attacks on MAC/password/secret compares.
|
||||
* `volatile` accumulator survives `-Os` LTO — disassembly-verified
|
||||
* on Cortex-M4 (rak3401_1watt). Pattern from rweather/arduinolibs. */
|
||||
static bool constantTimeEqual(const void *a, const void *b, size_t n);
|
||||
|
||||
/* Securely zero a buffer such that the compiler cannot elide the
|
||||
* writes as dead-store optimization. Uses volatile pointer writes —
|
||||
* standard idiom for clearing crypto secrets before stack unwind.
|
||||
* Use this for any buffer holding key material, seeds, or shared
|
||||
* secrets after their last use. */
|
||||
static void secureZeroize(void *buf, size_t n);
|
||||
|
||||
static void toHex(char *dest, const uint8_t *src, size_t len);
|
||||
static bool fromHex(uint8_t *dest, int dest_size, const char *src_hex);
|
||||
static bool isHexChar(char c);
|
||||
|
||||
@@ -82,12 +82,21 @@ bool LocalIdentity::validatePrivateKey(const uint8_t prv[64])
|
||||
uint8_t ss1[32], ss2[32];
|
||||
ed25519_key_exchange(ss1, test_client_pub, prv);
|
||||
ed25519_key_exchange(ss2, pub, test_client_prv);
|
||||
if (memcmp(ss1, ss2, 32) != 0) return false;
|
||||
|
||||
for (int i = 0; i < 32; i++) {
|
||||
if (ss1[i] != 0) return true;
|
||||
/* Constant-time even though this self-test runs at boot before
|
||||
* any networking is up — hygiene + no attacker observation. */
|
||||
if (!Utils::constantTimeEqual(ss1, ss2, 32)) {
|
||||
Utils::secureZeroize(ss1, sizeof(ss1));
|
||||
Utils::secureZeroize(ss2, sizeof(ss2));
|
||||
return false;
|
||||
}
|
||||
return false;
|
||||
|
||||
bool nonzero = false;
|
||||
for (int i = 0; i < 32; i++) {
|
||||
if (ss1[i] != 0) { nonzero = true; break; }
|
||||
}
|
||||
Utils::secureZeroize(ss1, sizeof(ss1));
|
||||
Utils::secureZeroize(ss2, sizeof(ss2));
|
||||
return nonzero;
|
||||
}
|
||||
|
||||
bool LocalIdentity::readFrom(const uint8_t *src, size_t len)
|
||||
|
||||
+36
-1
@@ -180,10 +180,45 @@ int Utils::MACThenDecrypt(const uint8_t *shared_secret, uint8_t *dest, const uin
|
||||
uint8_t computed_mac[CIPHER_MAC_SIZE];
|
||||
if (compute_hmac_truncated(shared_secret, PUB_KEY_SIZE, src + CIPHER_MAC_SIZE, (size_t)src_len - CIPHER_MAC_SIZE, computed_mac, CIPHER_MAC_SIZE) != 0)
|
||||
return 0;
|
||||
if (memcmp(computed_mac, src, CIPHER_MAC_SIZE) != 0) return 0;
|
||||
/* Constant-time MAC compare. Runs on every encrypted packet — a
|
||||
* timing oracle here would let attackers forge MACs byte-by-byte
|
||||
* across the entire mesh, bypassing message authentication. */
|
||||
if (!Utils::constantTimeEqual(computed_mac, src, CIPHER_MAC_SIZE)) return 0;
|
||||
return decrypt(shared_secret, dest, src + CIPHER_MAC_SIZE, src_len - CIPHER_MAC_SIZE);
|
||||
}
|
||||
|
||||
/* See header for rationale. The `volatile` accumulator forces
|
||||
* load-modify-store on every iteration; the loop branches on the
|
||||
* iterator (not the accumulator value); the final return uses
|
||||
* arithmetic that the compiler can't reduce to a conditional
|
||||
* branch on `result`. Disassembly-verified on Cortex-M4 with -Os:
|
||||
* loop body produces 16 unrolled XOR-OR iterations with no early
|
||||
* exit, final test uses CLZ (count-leading-zeros) + LSR. */
|
||||
bool Utils::constantTimeEqual(const void *a, const void *b, size_t n)
|
||||
{
|
||||
const uint8_t *pa = (const uint8_t *)a;
|
||||
const uint8_t *pb = (const uint8_t *)b;
|
||||
volatile uint8_t result = 0;
|
||||
for (size_t i = 0; i < n; i++) {
|
||||
result |= (uint8_t)(pa[i] ^ pb[i]);
|
||||
}
|
||||
return result == 0;
|
||||
}
|
||||
|
||||
void Utils::secureZeroize(void *buf, size_t n)
|
||||
{
|
||||
/* Volatile pointer prevents the compiler from eliminating the
|
||||
* writes as dead store. Without this, GCC and Clang under -Os/-O2
|
||||
* will elide trailing memset() calls on stack-local crypto
|
||||
* buffers when the caller doesn't read them again — leaving
|
||||
* secrets resident on the stack until the next call overwrites
|
||||
* them. Standard idiom from BoringSSL, libsodium, etc. */
|
||||
volatile uint8_t *p = (volatile uint8_t *)buf;
|
||||
while (n--) {
|
||||
*p++ = 0;
|
||||
}
|
||||
}
|
||||
|
||||
static const char hex_chars[] = "0123456789ABCDEF";
|
||||
|
||||
void Utils::toHex(char *dest, const uint8_t *src, size_t len)
|
||||
|
||||
@@ -703,8 +703,8 @@ int main(void)
|
||||
|
||||
data_store.saveMainIdentity(self_identity);
|
||||
|
||||
memset(seed, 0, sizeof(seed));
|
||||
memset(adc_noise, 0, sizeof(adc_noise));
|
||||
mesh::Utils::secureZeroize(seed, sizeof(seed));
|
||||
mesh::Utils::secureZeroize(adc_noise, sizeof(adc_noise));
|
||||
}
|
||||
companion_mesh.self_id = self_identity;
|
||||
|
||||
|
||||
@@ -501,8 +501,8 @@ int main(void)
|
||||
data_store.saveIdentity(self_identity);
|
||||
LOG_INF("New identity saved");
|
||||
|
||||
memset(seed, 0, sizeof(seed));
|
||||
memset(adc_noise, 0, sizeof(adc_noise));
|
||||
mesh::Utils::secureZeroize(seed, sizeof(seed));
|
||||
mesh::Utils::secureZeroize(adc_noise, sizeof(adc_noise));
|
||||
}
|
||||
repeater_mesh.self_id = self_identity;
|
||||
|
||||
|
||||
Reference in New Issue
Block a user