diff --git a/zephcore/adapters/rng/ZephyrRNG.cpp b/zephcore/adapters/rng/ZephyrRNG.cpp index 2209df7..f95851a 100644 --- a/zephcore/adapters/rng/ZephyrRNG.cpp +++ b/zephcore/adapters/rng/ZephyrRNG.cpp @@ -10,6 +10,7 @@ #include #include #include +#include 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 */ diff --git a/zephcore/app/RepeaterMesh.cpp b/zephcore/app/RepeaterMesh.cpp index 6298d67..5724282 100644 --- a/zephcore/app/RepeaterMesh.cpp +++ b/zephcore/app/RepeaterMesh.cpp @@ -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; diff --git a/zephcore/include/mesh/Utils.h b/zephcore/include/mesh/Utils.h index b7d0daf..c4e50af 100644 --- a/zephcore/include/mesh/Utils.h +++ b/zephcore/include/mesh/Utils.h @@ -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); diff --git a/zephcore/src/Identity.cpp b/zephcore/src/Identity.cpp index 9bb2a29..4156413 100644 --- a/zephcore/src/Identity.cpp +++ b/zephcore/src/Identity.cpp @@ -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) diff --git a/zephcore/src/Utils.cpp b/zephcore/src/Utils.cpp index c59b55d..6b7c8bc 100644 --- a/zephcore/src/Utils.cpp +++ b/zephcore/src/Utils.cpp @@ -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) diff --git a/zephcore/src/main_companion.cpp b/zephcore/src/main_companion.cpp index 7f05eab..e98d9b2 100644 --- a/zephcore/src/main_companion.cpp +++ b/zephcore/src/main_companion.cpp @@ -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; diff --git a/zephcore/src/main_repeater.cpp b/zephcore/src/main_repeater.cpp index bf4609b..2e5f716 100644 --- a/zephcore/src/main_repeater.cpp +++ b/zephcore/src/main_repeater.cpp @@ -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;