fix(keystore): fail closed when the OTP trust anchor cannot be read

core/keystore.c

@@ -16,9 +16,17 @@
 #include <string.h>
 
 /* 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;
 }
 
 

tests/unit/test_keystore.c

@@ -8,10 +8,73 @@
  */
 
 #include "eos_keystore.h"
+#include "eos_hal.h"
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
 
+/* ---- 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;
 }