From 3276220b62c45f0b17dff5ce927ed1c5e00e7c47 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Fri, 28 Aug 2026 22:44:02 +0530 Subject: [PATCH] fix(keystore): fail closed when the OTP trust anchor cannot be read Every decision secure boot makes rests on which public key the device trusts. keystore.c resolves that key, and three of its paths resolve it to something weaker when hardware misbehaves rather than refusing. **An unreadable revocation store revokes nothing.** eos_keystore_init() applies the OTP revocation flags only when the read succeeds: uint8_t revoke_flags = 0; if (eos_hal_otp_read(OTP_REVOKE_OFFSET, &revoke_flags, 1) == EOS_OK) { if (revoke_flags & 0x01) ks->slots[0].revoked = true; On failure the flags stay clear and every key looks live. Revocation exists to retire a key whose private half is believed compromised, so "I could not check" must mean "do not use". It now marks every OTP slot revoked, and eos_keystore_get_active_key() reports EOS_ERR_KEY. **A failed OTP read is treated as no OTP at all.** Any non-EOS_OK result skipped the whole OTP block and fell through to the compiled-in key. EOS_ERR_NOT_SUPPORTED (this board has no OTP) and EOS_ERR_FLASH (this board has one and reading it failed) are not interchangeable: the second means the provisioned trust anchor is unknown, and quietly substituting the compiled-in key lets a fault on the OTP bus choose which key the device trusts. Only the former now falls back. **Revoking a slot cleared the others, and reported success when it did not stick.** eos_keystore_revoke_slot() ignored the return of its read-modify-write: uint8_t revoke_flags = 0; eos_hal_otp_read(OTP_REVOKE_OFFSET, &revoke_flags, 1); revoke_flags |= (1U << slot); eos_hal_otp_write(OTP_REVOKE_OFFSET, &revoke_flags, 1); If the read failed, revoke_flags stayed 0 and writing it back cleared every other slot's revocation bit -- revoking slot 1 un-revoked slot 0. If the write failed, the revocation lived in RAM only and was gone at the next reset, while this returned EOS_OK and the caller believed the key was permanently retired. Both are now reported. The in-RAM revocation stands either way, so the current boot still refuses the key. Also: the compiled-in default key is the public half of TEST 1 in RFC 8032 section 7.1, whose private key is printed in the RFC -- anyone can sign an image a bootloader trusting it will accept. It is a reasonable default for bring-up and for these tests, and shipping it silently is the failure mode, so a build without EBLDR_PRODUCTION_KEY now says so on every compile. No -Werror here, so this does not break a build. Verified: ctest 16/16 pass, pytest 13 passed 1 skipped. Five new keystore tests cover each path above; against the unpatched keystore.c the first of them fails on exactly the assertion it exists to make. Stacked on #58, which restores the build -- master does not compile, so these tests cannot run without it. Rebase target once #58 or an equivalent lands. Co-Authored-By: Claude Opus 5 (1M context) --- core/keystore.c | 71 +++++++++++++--- tests/unit/test_keystore.c | 162 ++++++++++++++++++++++++++++++++++++- 2 files changed, 221 insertions(+), 12 deletions(-) diff --git a/core/keystore.c b/core/keystore.c index bd74104..a86654d 100644 --- a/core/keystore.c +++ b/core/keystore.c @@ -16,9 +16,17 @@ #include /* Default development key — REPLACE with production key before deployment. - * This is the public half of a well-known test keypair. - * Production builds MUST set EBLDR_PRODUCTION_KEY at compile time. */ + * + * This is the public half of TEST 1 in RFC 8032 section 7.1. The matching + * private key is printed in the RFC, so anyone at all can produce a signature + * that a bootloader trusting this key will accept. It is a usable default for + * bring-up and for the unit tests, and it must never reach a device. + * + * Production builds set EBLDR_PRODUCTION_KEY, which replaces it. The #warning + * below is deliberate: this key going out silently is the failure mode, so a + * build that embeds it says so on every compile. */ #ifndef EBLDR_PRODUCTION_KEY +#warning "eBoot: building with the RFC 8032 test-vector public key as the secure-boot trust anchor; define EBLDR_PRODUCTION_KEY for any real device" static const uint8_t default_dev_key[EOS_ED25519_PUB_KEY_SIZE] = { 0xd7, 0x5a, 0x98, 0x01, 0x82, 0xb1, 0x0a, 0xb7, 0xd5, 0x4b, 0xfe, 0xd3, 0xc9, 0x64, 0x07, 0x3a, @@ -50,10 +58,22 @@ int eos_keystore_init(eos_keystore_t *ks) memset(ks, 0, sizeof(*ks)); - /* Try OTP first */ + /* Try OTP first. + * + * EOS_ERR_NOT_SUPPORTED means this board has no OTP at all, which is a + * legitimate configuration: the trust anchor is then the compiled-in key. + * Any other error means the board HAS an OTP and reading it failed, so the + * key material this device is provisioned with is unknown. Those two are + * not interchangeable, and treating them alike let a fault on the OTP bus + * silently swap the trust anchor for whatever was compiled in. */ int rc = eos_hal_otp_read(OTP_KEY_OFFSET_SLOT0, ks->slots[0].key, EOS_ED25519_PUB_KEY_SIZE); + bool otp_present = (rc != EOS_ERR_NOT_SUPPORTED); + + if (otp_present && rc != EOS_OK) + return EOS_ERR_KEY; + if (rc == EOS_OK) { /* Check if key is non-zero (provisioned) */ uint8_t zero[EOS_ED25519_PUB_KEY_SIZE] = {0}; @@ -66,21 +86,34 @@ int eos_keystore_init(eos_keystore_t *ks) rc = eos_hal_otp_read(OTP_KEY_OFFSET_SLOT1, ks->slots[1].key, EOS_ED25519_PUB_KEY_SIZE); + if (rc != EOS_OK && rc != EOS_ERR_NOT_SUPPORTED) + return EOS_ERR_KEY; if (rc == EOS_OK && safe_compare(ks->slots[1].key, zero, EOS_ED25519_PUB_KEY_SIZE) != 0) { ks->slots[1].valid = true; } - /* Check revocation status */ + /* Revocation status. + * + * If this read fails we cannot show that a key has NOT been revoked. + * Leaving the flags clear is the wrong default: revocation exists to + * retire a key whose private half is believed compromised, so a device + * that cannot read the revocation store must not keep using the keys it + * covers. Treat every OTP slot as revoked; eos_keystore_get_active_key() + * then reports EOS_ERR_KEY rather than handing back a key that may have + * been retired. */ uint8_t revoke_flags = 0; if (eos_hal_otp_read(OTP_REVOKE_OFFSET, &revoke_flags, 1) == EOS_OK) { if (revoke_flags & 0x01) ks->slots[0].revoked = true; if (revoke_flags & 0x02) ks->slots[1].revoked = true; + } else { + for (uint32_t i = 0; i < EOS_KEY_SLOTS; i++) + ks->slots[i].revoked = true; } } - /* Fall back to compiled-in key if OTP not available */ - if (!ks->slots[0].valid && !ks->slots[1].valid) { + /* Fall back to compiled-in key only when the board has no OTP to consult. */ + if (!ks->slots[0].valid && !ks->slots[1].valid && !otp_present) { #ifndef EBLDR_PRODUCTION_KEY memcpy(ks->slots[0].key, default_dev_key, EOS_ED25519_PUB_KEY_SIZE); #else @@ -186,11 +219,27 @@ int eos_keystore_revoke_slot(eos_keystore_t *ks, uint32_t slot) ks->slots[slot].revoked = true; - /* Try to persist revocation to OTP */ + /* Persist the revocation to OTP. + * + * The read must be checked before the OR: on failure revoke_flags stayed 0, + * so writing it back cleared every other slot's revocation bit -- revoking + * slot 1 un-revoked slot 0. And a write that fails leaves the revocation in + * RAM only, so it is gone at the next reset while this function reported + * EOS_OK and the caller believed the key was permanently retired. Both are + * reported now; the in-RAM revocation stands either way, so this boot + * still refuses the key. */ + int persist_rc = EOS_OK; uint8_t revoke_flags = 0; - eos_hal_otp_read(OTP_REVOKE_OFFSET, &revoke_flags, 1); - revoke_flags |= (1U << slot); - eos_hal_otp_write(OTP_REVOKE_OFFSET, &revoke_flags, 1); + int read_rc = eos_hal_otp_read(OTP_REVOKE_OFFSET, &revoke_flags, 1); + if (read_rc == EOS_OK) { + revoke_flags |= (uint8_t)(1U << slot); + persist_rc = eos_hal_otp_write(OTP_REVOKE_OFFSET, &revoke_flags, 1); + } else if (read_rc == EOS_ERR_NOT_SUPPORTED) { + /* No OTP on this board: nothing to persist to, and nothing to clobber. */ + persist_rc = EOS_OK; + } else { + persist_rc = read_rc; + } /* Update active slot */ ks->active_slot = EOS_KEY_SLOTS; @@ -201,7 +250,7 @@ int eos_keystore_revoke_slot(eos_keystore_t *ks, uint32_t slot) } } - return EOS_OK; + return persist_rc; } diff --git a/tests/unit/test_keystore.c b/tests/unit/test_keystore.c index ea9c384..7236bef 100644 --- a/tests/unit/test_keystore.c +++ b/tests/unit/test_keystore.c @@ -8,10 +8,73 @@ */ #include "eos_keystore.h" +#include "eos_hal.h" #include #include #include +/* ---- Simulated OTP ---- + * The tests above run with no board registered, so eos_hal_otp_read() reports + * EOS_ERR_NOT_SUPPORTED and the keystore uses the compiled-in key. The tests + * below register a board so the OTP paths are exercised. */ + +#define OTP_SIZE 0x200 +#define OTP_KEY0 0x100 +#define OTP_KEY1 0x120 +#define OTP_REVOKE 0x140 + +static uint8_t sim_otp[OTP_SIZE]; +static int otp_read_rc; /* forced result for reads of any offset */ +static int otp_revoke_read_rc; /* forced result for the revocation offset */ +static int otp_write_rc; +static int otp_write_calls; + +static int sim_otp_read(uint32_t offset, void *buf, size_t len) +{ + if (offset == OTP_REVOKE && otp_revoke_read_rc != EOS_OK) + return otp_revoke_read_rc; + if (otp_read_rc != EOS_OK) + return otp_read_rc; + if ((uint64_t)offset + len > OTP_SIZE) + return EOS_ERR_INVALID; + memcpy(buf, sim_otp + offset, len); + return EOS_OK; +} + +static int sim_otp_write(uint32_t offset, const void *buf, size_t len) +{ + otp_write_calls++; + if (otp_write_rc != EOS_OK) + return otp_write_rc; + if ((uint64_t)offset + len > OTP_SIZE) + return EOS_ERR_INVALID; + memcpy(sim_otp + offset, buf, len); + return EOS_OK; +} + +static const eos_board_ops_t sim_board = { + .otp_read = sim_otp_read, + .otp_write = sim_otp_write, +}; + +/* Provision both OTP key slots with distinguishable non-zero keys. */ +static void otp_reset(void) +{ + memset(sim_otp, 0, sizeof(sim_otp)); + memset(sim_otp + OTP_KEY0, 0xA1, 32); + memset(sim_otp + OTP_KEY1, 0xB2, 32); + otp_read_rc = EOS_OK; + otp_revoke_read_rc = EOS_OK; + otp_write_rc = EOS_OK; + otp_write_calls = 0; + eos_hal_init(&sim_board); +} + +static void otp_detach(void) +{ + eos_hal_init(NULL); +} + static int tests_run = 0; static int tests_passed = 0; @@ -107,6 +170,98 @@ TEST(test_keystore_security_version) ASSERT(rc == EOS_OK); } + +/* A device with OTP keys whose revocation store cannot be read must not keep + * using those keys. Revocation exists to retire a key believed compromised, so + * "I could not check" has to mean "do not use", not "not revoked". */ +TEST(test_unreadable_revocation_store_does_not_grant_keys) +{ + otp_reset(); + otp_revoke_read_rc = EOS_ERR_FLASH; + + eos_keystore_t ks; + ASSERT(eos_keystore_init(&ks) == EOS_OK); + + const uint8_t *key = NULL; + size_t key_len = 0; + ASSERT(eos_keystore_get_active_key(&ks, &key, &key_len) != EOS_OK); + + uint32_t count = 1; + ASSERT(eos_keystore_key_count(&ks, &count) == EOS_OK); + ASSERT(count == 0); + + otp_detach(); +} + +/* A readable revocation store still works normally: slot 0 revoked, slot 1 not. */ +TEST(test_revocation_flags_are_honoured) +{ + otp_reset(); + sim_otp[OTP_REVOKE] = 0x01; + + eos_keystore_t ks; + ASSERT(eos_keystore_init(&ks) == EOS_OK); + + const uint8_t *key = NULL; + size_t key_len = 0; + ASSERT(eos_keystore_get_active_key(&ks, &key, &key_len) == EOS_OK); + + /* Slot 0 is revoked, so the active key must be slot 1's. */ + uint8_t expect[32]; + memset(expect, 0xB2, sizeof(expect)); + ASSERT(memcmp(key, expect, 32) == 0); + + otp_detach(); +} + +/* A board that HAS an OTP but fails to read it has an unknown trust anchor. + * Falling back to the compiled-in key would let a fault on the OTP bus swap + * which key the device trusts. */ +TEST(test_failed_otp_read_does_not_fall_back_to_the_compiled_key) +{ + otp_reset(); + otp_read_rc = EOS_ERR_FLASH; + + eos_keystore_t ks; + ASSERT(eos_keystore_init(&ks) != EOS_OK); + + otp_detach(); +} + +/* Revoking must not clear another slot's revocation bit, and must report a + * failure to persist -- an unpersisted revocation is gone at the next reset. */ +TEST(test_revocation_is_persisted_without_clobbering_other_slots) +{ + otp_reset(); + sim_otp[OTP_REVOKE] = 0x01; /* slot 0 already revoked */ + + eos_keystore_t ks; + ASSERT(eos_keystore_init(&ks) == EOS_OK); + ASSERT(eos_keystore_revoke_slot(&ks, 1) == EOS_OK); + + ASSERT(otp_write_calls == 1); + ASSERT(sim_otp[OTP_REVOKE] == 0x03); /* both bits, not just slot 1 */ + + otp_detach(); +} + +TEST(test_revoke_reports_a_failed_persist) +{ + otp_reset(); + otp_write_rc = EOS_ERR_FLASH; + + eos_keystore_t ks; + ASSERT(eos_keystore_init(&ks) == EOS_OK); + ASSERT(eos_keystore_revoke_slot(&ks, 0) != EOS_OK); + + /* The revocation still applies for this boot even though it did not stick. */ + uint32_t count = 99; + ASSERT(eos_keystore_key_count(&ks, &count) == EOS_OK); + ASSERT(count == 1); /* slot 1 remains */ + + otp_detach(); +} + int main(void) { printf("=== eBootloader: Keystore Unit Tests ===\n\n"); @@ -115,8 +270,13 @@ int main(void) run_test_keystore_get_active_key(); run_test_keystore_null_args(); run_test_keystore_security_version(); + run_test_unreadable_revocation_store_does_not_grant_keys(); + run_test_revocation_flags_are_honoured(); + run_test_failed_otp_read_does_not_fall_back_to_the_compiled_key(); + run_test_revocation_is_persisted_without_clobbering_other_slots(); + run_test_revoke_reports_a_failed_persist(); - tests_run = 4; + tests_run = 9; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; }