Addressed Fenrir's review comments + fixed more regressions

Author: danielinux

hal/stm32h5.c

@@ -42,7 +42,6 @@
 #elif defined(WOLFBOOT_HASH_SHA3_384)
 #include <wolfssl/wolfcrypt/sha3.h>
 #endif
-#include <wolfssl/wolfcrypt/memory.h>
 #endif
 
 #define PLL_SRC_HSE 1
@@ -247,6 +246,14 @@ static int buffer_is_all_value(const uint8_t *buf, size_t len, uint8_t value)
     return 1;
 }
 
+static NOINLINEFUNCTION void hal_secret_zeroize(void *ptr, size_t len)
+{
+    volatile uint8_t *p = (volatile uint8_t *)ptr;
+    while (len-- > 0U) {
+        *p++ = 0U;
+    }
+}
+
 int hal_uds_derive_key(uint8_t *out, size_t out_len)
 {
 #if defined(FLASH_OTP_KEYSTORE)
@@ -273,11 +280,11 @@ int hal_uds_derive_key(uint8_t *out, size_t out_len)
                 copy_len = out_len;
             }
             memcpy(out, uds, copy_len);
-            wc_ForceZero(uds, sizeof(uds));
+            hal_secret_zeroize(uds, sizeof(uds));
             return 0;
         }
     }
-    wc_ForceZero(uds, sizeof(uds));
+    hal_secret_zeroize(uds, sizeof(uds));
 #endif
 
 #ifdef WOLFBOOT_UDS_UID_FALLBACK_FORTEST

src/delta.c

@@ -185,7 +185,6 @@ int wb_patch(WB_PATCH_CTX *ctx, uint8_t *dst, uint32_t len)
 #include <stdio.h>
 #include <stdlib.h>
 #include <errno.h>
-#include <limits.h>     /* INT_MAX */
 #include <inttypes.h>   /* PRIu32  */
 
 static uint32_t wolfboot_sector_size = 0;
@@ -239,11 +238,6 @@ int wb_diff_get_sector_size(void)
             sec_sz);
         exit(6);
     }
-    if (sec_sz > (uint32_t)INT_MAX) {
-        fprintf(stderr, "WOLFBOOT_SECTOR_SIZE (%" PRIu32 ") exceeds INT_MAX (%d)\n",
-            sec_sz, INT_MAX);
-        exit(6);
-    }
     return (int)sec_sz;
 }
 

src/dice/dice.c

@@ -35,7 +35,6 @@
 #include <wolfssl/wolfcrypt/random.h>
 #include <wolfssl/wolfcrypt/sha256.h>
 #include <wolfssl/wolfcrypt/integer.h>
-#include <wolfssl/wolfcrypt/memory.h>
 
 #if defined(WOLFBOOT_HASH_SHA384)
 #include <wolfssl/wolfcrypt/sha512.h>
@@ -68,6 +67,14 @@
 #define WOLFBOOT_DICE_ERR_HW -3
 #define WOLFBOOT_DICE_ERR_CRYPTO -4
 
+static NOINLINEFUNCTION void wolfboot_dice_zeroize(void *ptr, size_t len)
+{
+    volatile uint8_t *p = (volatile uint8_t *)ptr;
+    while (len-- > 0U) {
+        *p++ = 0U;
+    }
+}
+
 #define COSE_LABEL_ALG 1
 #define COSE_ALG_ES256 (-7)
 
@@ -621,7 +628,7 @@ static int wolfboot_dice_derive_attestation_key(ecc_key *key,
         goto cleanup;
     }
     /* CDI is no longer needed once the seed has been derived. */
-    wc_ForceZero(cdi, sizeof(cdi));
+    wolfboot_dice_zeroize(cdi, sizeof(cdi));
 
     if (wolfboot_dice_hkdf(seed, sizeof(seed),
                            (const uint8_t *)"WOLFBOOT-IAK", 12,
@@ -630,7 +637,7 @@ static int wolfboot_dice_derive_attestation_key(ecc_key *key,
         goto cleanup;
     }
     /* Seed is no longer needed once the private key material is derived. */
-    wc_ForceZero(seed, sizeof(seed));
+    wolfboot_dice_zeroize(seed, sizeof(seed));
 
     if (wolfboot_dice_fixup_priv(priv, sizeof(priv)) != 0) {
         goto cleanup;
@@ -644,9 +651,9 @@ static int wolfboot_dice_derive_attestation_key(ecc_key *key,
     ret = 0;
 
 cleanup:
-    wc_ForceZero(priv, sizeof(priv));
-    wc_ForceZero(seed, sizeof(seed));
-    wc_ForceZero(cdi, sizeof(cdi));
+    wolfboot_dice_zeroize(priv, sizeof(priv));
+    wolfboot_dice_zeroize(seed, sizeof(seed));
+    wolfboot_dice_zeroize(cdi, sizeof(cdi));
     return ret;
 }
 
@@ -675,7 +682,7 @@ static int wolfboot_attest_get_private_key(ecc_key *key,
         ret = 0;
 
 cleanup:
-        wc_ForceZero(priv, sizeof(priv));
+        wolfboot_dice_zeroize(priv, sizeof(priv));
         return ret;
     }
 #else
@@ -684,7 +691,7 @@ static int wolfboot_attest_get_private_key(ecc_key *key,
     if (hal_uds_derive_key(uds, uds_len) == 0) {
         ret = wolfboot_dice_derive_attestation_key(key, uds, uds_len, claims);
     }
-    wc_ForceZero(uds, sizeof(uds));
+    wolfboot_dice_zeroize(uds, sizeof(uds));
     return ret;
 #endif
 }
@@ -870,10 +877,10 @@ static int wolfboot_dice_sign_tbs(const uint8_t *tbs,
     }
     if (key_inited) {
         wc_ecc_free(&key);
-        wc_ForceZero(&key, sizeof(key));
+        wolfboot_dice_zeroize(&key, sizeof(key));
     }
-    wc_ForceZero(hash, sizeof(hash));
-    wc_ForceZero(der_sig, sizeof(der_sig));
+    wolfboot_dice_zeroize(hash, sizeof(hash));
+    wolfboot_dice_zeroize(der_sig, sizeof(der_sig));
     return ret;
 }
 

src/tpm.c

@@ -44,17 +44,17 @@ WOLFTPM2_KEY     wolftpm_srk;
 #endif
 
 #if defined(WOLFBOOT_TPM_SEAL) || defined(WOLFBOOT_TPM_KEYSTORE)
-int wolfBoot_constant_compare(const uint8_t* a, const uint8_t* b,
+int NOINLINEFUNCTION wolfBoot_constant_compare(const uint8_t* a, const uint8_t* b,
     uint32_t len)
 {
     uint32_t i;
-    volatile uint8_t diff = 0;
+    volatile uint32_t diff = 0U;
 
     for (i = 0; i < len; i++) {
-        diff |= a[i] ^ b[i];
+        diff |= (uint32_t)(a[i] ^ b[i]);
     }
 
-    return diff;
+    return (int)diff;
 }
 
 void wolfBoot_print_hexstr(const unsigned char* bin, unsigned long sz,

src/update_flash.c

@@ -567,7 +567,7 @@ static int RAMFUNCTION wolfBoot_swap_and_final_erase(int resume)
     #   define DELTA_BLOCK_SIZE 1024
     #endif
 
-static inline uint32_t im2n(uint32_t val)
+static inline uint32_t wb_delta_im2n(uint32_t val)
 {
 #ifdef BIG_ENDIAN_ORDER
     return (((val & 0x000000FFU) << 24) |
@@ -631,9 +631,9 @@ static int wolfBoot_delta_update(struct wolfBoot_image *boot,
                 &delta_base_hash, &delta_base_hash_sz) < 0) {
         return -1;
     }
-    delta_img_size = im2n(*img_size);
+    delta_img_size = wb_delta_im2n(*img_size);
     if (inverse)
-        delta_img_offset = im2n(*img_offset);
+        delta_img_offset = wb_delta_im2n(*img_offset);
     cur_v = wolfBoot_current_firmware_version();
     upd_v = wolfBoot_update_firmware_version();
     delta_base_v = wolfBoot_get_diffbase_version(PART_UPDATE);

tools/keytools/otp/Makefile

@@ -19,7 +19,6 @@ CFLAGS+=-I. -I../../../ -I../../../include -I$(WOLFBOOT_LIB_WOLFSSL) \
 CFLAGS+=-I./wcs
 CFLAGS+=-DFLASH_OTP_KEYSTORE -D__FLASH_OTP_PRIMER -DWOLFSSL_USER_SETTINGS -DWOLFCRYPT_SECURE_MODE
 PRI_KS_OBJS+=startup.o otp-keystore-primer.o ../../../src/keystore.o
-PRI_KS_OBJS+=$(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/memory.o
 
 ifeq ($(HASH),SHA256)
     PRI_KS_OBJS+=$(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/sha256.o

tools/keytools/otp/target.ld

@@ -15,6 +15,7 @@ SECTIONS
         *(.fini)
         *(.text*)
         *(.rodata*)
+        KEEP(*(.keystore*))
         . = ALIGN(4);
         _end_text = .;
     } > FLASH

tools/unit-tests/unit-update-flash.c

@@ -573,6 +573,38 @@ START_TEST (test_empty_panic)
 }
 END_TEST
 
+START_TEST (test_part_sanity_check_panics_on_sha_mismatch)
+{
+    struct wolfBoot_image img;
+
+    memset(&img, 0, sizeof(img));
+    img.hdr_ok = 1;
+    img.sha_ok = 0;
+    img.signature_ok = 1;
+    wolfBoot_panicked = 0;
+
+    PART_SANITY_CHECK(&img);
+    ck_assert_int_eq(wolfBoot_panicked, 1);
+    wolfBoot_panicked = 0;
+}
+END_TEST
+
+START_TEST (test_part_sanity_check_panics_on_signature_mismatch)
+{
+    struct wolfBoot_image img;
+
+    memset(&img, 0, sizeof(img));
+    img.hdr_ok = 1;
+    img.sha_ok = 1;
+    img.signature_ok = 0;
+    wolfBoot_panicked = 0;
+
+    PART_SANITY_CHECK(&img);
+    ck_assert_int_eq(wolfBoot_panicked, 1);
+    wolfBoot_panicked = 0;
+}
+END_TEST
+
 #ifdef EXT_ENCRYPTED
 START_TEST (test_fallback_image_verification_rejects_corruption)
 {
@@ -1297,6 +1329,8 @@ Suite *wolfboot_suite(void)
     return s;
 #else
     tcase_add_test(empty_panic, test_empty_panic);
+    tcase_add_test(empty_panic, test_part_sanity_check_panics_on_sha_mismatch);
+    tcase_add_test(empty_panic, test_part_sanity_check_panics_on_signature_mismatch);
     tcase_add_test(sunnyday_noupdate, test_sunnyday_noupdate);
     tcase_add_test(forward_update_samesize, test_forward_update_samesize);
     tcase_add_test(forward_update_tolarger, test_forward_update_tolarger);