WIP Improved image, keygen, sign, test lib messages. Polish

Author: gojimmypi

.github/workflows/test-library.yml

@@ -20,7 +20,7 @@ jobs:
       fail-fast: false
 
       # Limit concurrent jobs for scheduling problem on GitHub's hosted runner pool.
-      max-parallel: 12
+      max-parallel: 10
 
       matrix:
         math:
@@ -31,7 +31,7 @@ jobs:
           - "SPMATH=0 SPMATHALL=0 WOLFBOOT_SMALL_STACK=0"
           - "SPMATH=0 SPMATHALL=0 WOLFBOOT_SMALL_STACK=1"
         asym: [ed25519, ecc256, ecc384, ecc521, rsa2048, rsa3072, rsa4096, ed448]
-        hash: [sha256, sha384, sha3]
+        hash: [sha256, sha384, sha3] # --sha256 for commandline, SHA256 for make
 
         # See https://github.com/wolfSSL/wolfBoot/issues/614 regarding exclusions:
         exclude:
@@ -41,41 +41,104 @@ jobs:
     steps:
       - uses: actions/checkout@v4
         with:
+          clean: true
           submodules: true
 
-      - name: make clean
+      - name: dist clean
         run: |
-          make keysclean && make -C tools/keytools clean && rm -f include/target.h
+          # Ensure parallel build did not leave behind any debris
+          make distclean
 
       - name: Build test-lib
         env:
           shell: bash
-          ASYM: ${{ matrix.asym }}
+          ASYM: ${{ matrix.asym }} # MAKE_SIGN in upper case
           HASH: ${{ matrix.hash }}
           MATH: ${{ matrix.math }}
         run: |
+          # Test various library parameters
+
+          export MAKE_SIGN="${ASYM^^}"
+          export MAKE_HASH="${HASH^^}"
+          export MAKE_MATH='${{ matrix.math }}' # e.g., "SPMATH=1 WOLFBOOT_SMALL_STACK=1"
+          export PRIVATE_KEY="wolfboot_signing_private_key.der"
+
+          echo "This MAKE_SIGN=$MAKE_SIGN"
+          echo "This MAKE_HASH=$MAKE_HASH"
+          echo "This MAKE_MATH=$MAKE_MATH"
+
           # Sample build
           build_once() {
               # Convert asym and hash to upper case, optionally add additional param
-              make -j test-lib SIGN=${ASYM^^} HASH=${HASH^^} ${MATH} "$@"
+              echo "Build test-lib..."
+              echo "make -j1 test-lib SIGN=${MAKE_SIGN} HASH=${MAKE_HASH} ${MATH} \"$@\""
+                    make -j1 test-lib SIGN=${MAKE_SIGN} HASH=${MAKE_HASH} ${MATH}  "$@"
           }
 
           set -euo pipefail
 
           # Get the reference config
           cp config/examples/library.config .config
 
+          # peek
+          echo "Existing files?"
+          if [ -f "src/keystore.c" ]; then
+            echo "WARNING: Found unexpected src/keystore.c"
+          fi
+          if [ -f "include/target.h" ]; then
+            echo "WARNING: Found unexpected include/target.h"
+          fi
+          if [ -f "keystore.der" ]; then
+            echo "WARNING: Found unexpected keystore.der"
+          fi
+          if [ -f "wolfboot_signing_private_key.der" ]; then
+            echo "WARNING: Found unexpected wolfboot_signing_private_key.der"
+          fi
+          if [ -f "./tools/keytools/keystore.der" ]; then
+            echo "WARNING: Found unexpected ./tools/keytools/keystore.der"
+          fi
+          if [ -f "./tools/keytools/wolfboot_signing_private_key.der" ]; then
+            echo "WARNING: Found unexpected ./tools/keytools/wolfboot_signing_private_key.der"
+          fi
+
           # Keytools
-          make keytools
-          ./tools/keytools/keygen --${ASYM} -g wolfboot_signing_private_key.der
+          echo ""
+          echo "make -j1 keytools SIGN=\"${MAKE_SIGN}\" HASH=\"${MAKE_HASH}\" $MATH"
+                make -j1 keytools SIGN="${MAKE_SIGN}"   HASH="${MAKE_HASH}"   $MATH
+
+          # Generate keys
+          echo ""
+          echo "./tools/keytools/keygen --${ASYM} -g wolfboot_signing_private_key.der"
+                ./tools/keytools/keygen --${ASYM} -g wolfboot_signing_private_key.der
+
+          # Force fresh files
+          # peek
+          echo "Existing files?"
+          if [ -f "src/keystore.c" ]; then
+            echo "Found unexpected src/keystore.c"
+          fi
+          if [ -f "include/target.h" ]; then
+            echo "Found unexpected include/target.h"
+          fi
+          if [ -f "keystore.der" ]; then
+            echo "Found unexpected keystore.der"
+          fi
+          if [ -f "wolfboot_signing_private_key.der" ]; then
+            echo "Found unexpected wolfboot_signing_private_key.der"
+          fi
 
           # Sign
+          echo ""
           echo "Test" > test.bin
-          ./tools/keytools/sign --${ASYM} --${HASH} test.bin wolfboot_signing_private_key.der 1
+          echo "Sign test.bin"
+          echo "./tools/keytools/sign --${ASYM} --${HASH} test.bin wolfboot_signing_private_key.der 1"
+                ./tools/keytools/sign --${ASYM} --${HASH} test.bin wolfboot_signing_private_key.der 1
 
           # First attempt
           if build_once >build.out 2>build.err; then
             echo "Success on first attempt, WOLFBOOT_HUGE_STACK not applied."
+            cat build.out
+            cat build.err
             exit 0
           fi
 
@@ -90,12 +153,16 @@ jobs:
             build_once WOLFBOOT_HUGE_STACK=1
           else
             echo "Build failed for another reason:"
+            cat build.out
             cat build.err
             exit 1
           fi
 
       - name: Run test-lib
         run: |
+          # Check test_v1_signed.bin
+
+          echo "./test-lib test_v1_signed.bin"
           ./test-lib test_v1_signed.bin
           ./test-lib test_v1_signed.bin 2>&1 | grep "Firmware Valid"
 
@@ -104,5 +171,25 @@ jobs:
           # Corrupt signed binary
           truncate -s -1 test_v1_signed.bin
           echo "A" >> test_v1_signed.bin
-          ./test-lib test_v1_signed.bin
-          ./test-lib test_v1_signed.bin 2>&1 | grep "Failure"
+
+          # Run once, capture output and status
+          set +e
+          output=$(./test-lib test_v1_signed.bin 2>&1)
+          status=$?
+          set -e
+
+          echo "$output"
+
+          # Must have failed (non-zero exit)
+          if [ $status -eq 0 ]; then
+            echo "Expected failure, but exit code was 0"
+            exit 1
+          fi
+
+          # Must include the expected Failure message
+          echo "$output" | grep -F "Failure" >/dev/null || {
+            echo "Expected 'Failure' not found in output"
+            exit 1
+          }
+
+          echo "Got expected non-zero exit and 'Failure' message."

hal/library.c

@@ -130,14 +130,17 @@ int wolfBoot_start(void)
     os_image.hdr = (uint8_t*)gImage;
 
     if ((ret = wolfBoot_open_image_address(&os_image, (uint8_t*)gImage)) < 0) {
+        wolfBoot_printf("Failed to open image address.\n");
         goto exit;
     }
 
     if ((ret = wolfBoot_verify_integrity(&os_image)) < 0) {
+        wolfBoot_printf("Failed to verify integrity.\n");
         goto exit;
     }
 
     if ((ret = wolfBoot_verify_authenticity(&os_image)) < 0) {
+        wolfBoot_printf("Failed to verify authenticity.\n");
         goto exit;
     }
 
@@ -148,11 +151,13 @@ int wolfBoot_start(void)
  exit:
     if (ret < 0) {
         wolfBoot_printf("Failure %d: Hdr %d, Hash %d, Sig %d\n", ret,
-                        (int)os_image.hdr_ok, (int)os_image.sha_ok,
-                        (int)os_image.signature_ok);
+                        os_image.hdr_ok, os_image.sha_ok, os_image.signature_ok);
+        return -1;
+    }
+    else {
+        return 0;
     }
 
-    return 0;
 }
 
 
@@ -161,39 +166,62 @@ int main(int argc, const char* argv[])
     int ret = 0;
 
 #ifdef NO_FILESYSTEM
+    wolfBoot_printf("NO_FILESYSTEM is defined, looking at test_img");
     gImage = (uintptr_t)test_img;
 #else
-    if (argc > 1) {
-        size_t sz = 0, bread;
+    if (argc == 2) {
+        size_t sz = 0, bread = 0;
         FILE* img = fopen(argv[1], "rb");
         if (img == NULL) {
-            wolfBoot_printf("failed to open %s!\n", argv[1]);
+            wolfBoot_printf("Failed to open file: %s!\n\n", argv[1]);
+            wolfBoot_printf("Usage: %s image_file.bin\n", argv[0]);
             return -3;
         }
-        fseek(img, 0, SEEK_END);
-        sz = ftell(img);
-        fseek(img, 0, SEEK_SET);
+        else {
+            wolfBoot_printf("Looking at image file: %s\n", argv[1]);
+            fseek(img, 0, SEEK_END);
+            sz = ftell(img);
+            fseek(img, 0, SEEK_SET);
+
+            gImage = (uintptr_t)malloc(sz);
+        }
 
-        gImage = (uintptr_t)malloc(sz);
         if (((void*)gImage) == NULL) {
-            wolfBoot_printf("failed to malloc %zu bytes for image\n", sz);
+            wolfBoot_printf("Failed to malloc %zu bytes for image.\n", sz);
             ret = -1;
         }
+        else {
+            /* check the image */
+            bread = fread((void*)gImage, 1, sz, img);
+        }
 
-        bread = fread((void*)gImage, 1, sz, img);
-        if (bread != sz) {
+        if (bread == sz) {
+            wolfBoot_printf("Confirmed expected size: %zu bytes.\n", bread);
+        }
+        else {
             ret = -2;
-            wolfBoot_printf("read %zu of %zu bytes from %s\n", bread, sz, argv[1]);
+            wolfBoot_printf("Read %zu of %zu bytes from %s\n", bread, sz, argv[1]);
         }
         fclose(img);
-    } else {
+    }
+    else {
         wolfBoot_printf("usage: %s image_file.bin\n", argv[0]);
         ret = 255;
     }
 #endif
     if (ret == 0) {
+        wolfBoot_printf("Checking image... ");
         ret = wolfBoot_start();
     }
+    if (ret == 0) {
+        wolfBoot_printf("Success!\n");
+    }
+    else {
+        if (ret != 255) {
+            /* Only show error if we actually processed file, not missing params */
+            wolfBoot_printf("Failed to verify with wolfBoot_start\n");
+        }
+    }
 
 #ifndef NO_FILESYSTEM
     if ((void*)gImage != NULL)

include/printf.h

@@ -66,6 +66,9 @@
 #   elif defined(WOLFBOOT_LOG_PRINTF)
         /* allow output to stdout */
 #       define wolfBoot_printf(_f_, ...) printf(_f_, ##__VA_ARGS__)
+#   elif defined(_MSC_VER)
+#       include <stdio.h>
+#       define wolfBoot_printf(...) do { fprintf(stderr, __VA_ARGS__); } while (0)
 #   else
         /* use stderr by default */
 #       define wolfBoot_printf(_f_, ...) fprintf(stderr, _f_, ##__VA_ARGS__)

src/image.c

@@ -946,7 +946,7 @@ static int header_sha256(wc_Sha256 *sha256_ctx, struct wolfBoot_image *img)
     while (p < end_sha) {
         blksz = WOLFBOOT_SHA_BLOCK_SIZE;
         if (end_sha - p < blksz)
-            blksz = end_sha - p;
+            blksz = (int)(end_sha - p);
         wc_Sha256Update(sha256_ctx, p, blksz);
         p += blksz;
     }
@@ -993,20 +993,28 @@ static int image_sha256(struct wolfBoot_image *img, uint8_t *hash)
  * @param key_slot The key slot ID to calculate the hash for.
  * @param hash A pointer to store the resulting SHA256 hash.
  */
-static void key_sha256(uint8_t key_slot, uint8_t *hash)
+static int key_sha256(uint8_t key_slot, uint8_t *hash)
 {
     uint8_t *pubkey = keystore_get_buffer(key_slot);
     int pubkey_sz = keystore_get_size(key_slot);
     wc_Sha256 sha256_ctx;
+    int ret = 0;
+
+    if (!pubkey || (pubkey_sz < 0)) {
+        return -1;
+    }
 
     memset(hash, 0, SHA256_DIGEST_SIZE);
-    if (!pubkey || (pubkey_sz < 0))
-        return;
 
-    wc_InitSha256(&sha256_ctx);
-    wc_Sha256Update(&sha256_ctx, pubkey, (word32)pubkey_sz);
-    wc_Sha256Final(&sha256_ctx, hash);
+    ret = wc_InitSha256(&sha256_ctx);
+    if (ret == 0) {
+        ret = wc_Sha256Update(&sha256_ctx, pubkey, (word32)pubkey_sz);
+    }
+    if (ret == 0) {
+        ret = wc_Sha256Final(&sha256_ctx, hash);
+    }
     wc_Sha256Free(&sha256_ctx);
+    return ret;
 }
 #endif /* WOLFBOOT_NO_SIGN */
 #endif /* SHA2-256 */
@@ -2205,18 +2213,34 @@ uint8_t* wolfBoot_peek_image(struct wolfBoot_image *img, uint32_t offset,
  *
  * @param hint The SHA hash of the public key to search for.
  * @return The key slot ID if found, -1 if the key was not found.
+ *         Other negative values if the key_hash function failed.
  */
+
 int keyslot_id_by_sha(const uint8_t *hint)
 {
     int id;
+    int ret = -1;
+    int ct = 0;
+    if (hint == NULL) {
+        return -1;
+    }
 
     for (id = 0; id < keystore_num_pubkeys(); id++) {
+        ct++;
         key_hash(id, digest);
-        if (memcmp(digest, hint, WOLFBOOT_SHA_DIGEST_SIZE) == 0) {
+        if ((ret == 0) && memcmp(digest, hint, WOLFBOOT_SHA_DIGEST_SIZE) == 0) {
+            wolfBoot_printf("Found matching digest in slot %d\n", id);
             return id;
         }
     }
-    return -1;
+
+    if (ret == 0) {
+        /* Calls to key_hash were successful, but we did not find one. Fail: */
+        wolfBoot_printf("No matching key hash found. Looked at %d slot(s)", ct);
+        ret = -1;
+    }
+    /* Reminder: zero based slot array. */
+    return ret;
 }
 #endif /* !WOLFBOOT_NO_SIGN && !WOLFBOOT_RENESAS_SCEPROTECT */