From c9fcdddefb221f399a70dc0158e4900361a92a1c Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Mon, 24 Aug 2026 18:00:41 +0200 Subject: [PATCH] F-11024: pass full-width fields to wc_ecc_rs_raw_to_sig in the wolfHSM verify path The wolfHSM verify path of wolfBoot_verify_signature_ecc() converts the fixed-width raw R||S signature to DER with wc_ecc_rs_raw_to_sig(). It passed the minimal field sizes (mp_unsigned_bin_size) while leaving the pointers at the start of each fixed-width field, so whenever R or S had a leading zero byte the conversion encoded a zero-padded integer with the low bytes truncated, and wc_ecc_verify_hash() rejected an otherwise valid signature (each component has roughly a 1 in 256 chance of a leading zero). Pass the full-width fields (point_sz for both): the raw signature is fixed-width and left-zero-padded, and the conversion strips the padding itself. The multiprecision sizing only existed to compute the minimal lengths and is dropped with the fix. unit-ecc-raw-der signs until a leading-zero signature shows up, then checks that the minimal-size pattern rejects it while the full-width pattern accepts it, and that both patterns agree for leading-zero-free signatures. Verification: - Built: src/image.c syntax-clean with WOLFBOOT_ENABLE_WOLFHSM_CLIENT (gcc -fsyntax-only, __WOLFBOOT, ECC256/SHA256 config, partition stubs); normal library build via make test-lib. - Tested: unit-ecc-raw-der 3/3: the minimal pattern rejects the leading-zero signature the full-width pattern accepts; both agree on leading-zero-free signatures. - Pitfalls: no key material touched; the dropped mp values had no matching mp_clear before the fix either (stack variables). - Style: cstyle-check.sh on src/image.c flags two pre-existing violations (L1869 anonymous union, L2079 C99 declaration) outside this change; the new test trips the same uncrustify pointer-alignment class the unit-stm32l5/u5-write twins trip and matches their local style. - Message: F-11024: prefix, no co-author trailers. - Unverified: no wolfHSM target builds in CI; the HSM branch was checked by syntax-only compile, not a full target build. --- src/image.c | 15 +- tools/unit-tests/Makefile | 36 ++++ tools/unit-tests/unit-ecc-raw-der.c | 289 ++++++++++++++++++++++++++++ 3 files changed, 331 insertions(+), 9 deletions(-) create mode 100644 tools/unit-tests/unit-ecc-raw-der.c diff --git a/src/image.c b/src/image.c index 062bbcf7..8b210be5 100644 --- a/src/image.c +++ b/src/image.c @@ -382,15 +382,12 @@ static void wolfBoot_verify_signature_ecc(uint8_t key_slot, #endif /* WOLFBOOT_ENABLE_WOLFHSM_CLIENT || (SERVER && CERT_CHAIN) */ /* wc_ecc_verify_hash_ex() doesn't trigger a crypto callback, so we need to use wc_ecc_verify_hash instead. Unfortunately, that requires - converting the signature to intermediate DER format first */ - mp_init(&r); - mp_init(&s); - mp_read_unsigned_bin(&r, sig, point_sz); - mp_read_unsigned_bin(&s, sig + point_sz, point_sz); - uint32_t rSz = mp_unsigned_bin_size(&r); - uint32_t sSz = mp_unsigned_bin_size(&s); - ret = wc_ecc_rs_raw_to_sig(sig, rSz, &sig[point_sz], sSz, - (byte*)&tmpSigBuf, (word32*)&tmpSigSz); + converting the signature to intermediate DER format first. Both + fields are passed at full width: the raw signature is fixed-width + and left-zero-padded, and the conversion strips the padding. */ + ret = wc_ecc_rs_raw_to_sig(sig, (word32)point_sz, &sig[point_sz], + (word32)point_sz, + (byte*)&tmpSigBuf, (word32*)&tmpSigSz); /* Verify the (temporary) DER representation of the signature */ if (ret == 0) { VERIFY_FN(img, &verify_res, wc_ecc_verify_hash, tmpSigBuf, tmpSigSz, diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index 1e204bae..a8dcb019 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -115,6 +115,7 @@ TESTS+=unit-t10xx-qe-firmware TESTS+=unit-aurix-erased-fill TESTS+=unit-aurix-erased-fill-invert TESTS+=unit-t2080-fman-loader +TESTS+=unit-ecc-raw-der TESTS+=unit-stm32g4-write TESTS+=unit-stm32l5-write TESTS+=unit-stm32u5-write @@ -1003,6 +1004,41 @@ stm32g4_write_extract.h: ../../hal/stm32g4.c unit-stm32g4-write: unit-stm32g4-write.c stm32g4_write_extract.h gcc -o $@ unit-stm32g4-write.c $(CFLAGS) $(LDFLAGS) +# unit-ecc-raw-der runs the real wolfCrypt raw-to-DER conversion and +# verification (F-11024: the wolfHSM verify path in src/image.c passed +# minimal field sizes with field-start pointers to +# wc_ecc_rs_raw_to_sig, corrupting signatures with leading zeros). It +# links the same wolfSSL sources the sign tool uses. +unit-ecc-raw-der: unit-ecc-raw-der.c + # WOLFSSL_NO_DER_TO_PEM: the test config's NO_CODING drops base64, which + # DER-to-PEM (pulled in by WOLFSSL_KEY_GEN) needs; the test uses no PEM. + gcc -o $@ unit-ecc-raw-der.c $(CFLAGS) -DWOLFCRYPT_TEST -DWOLFSSL_NO_DER_TO_PEM \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/aes.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/asn.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/chacha.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/coding.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/ecc.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/ed25519.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/ed448.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/fe_448.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/fe_operations.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/ge_448.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/ge_operations.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/hash.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/logging.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/memory.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/random.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/rsa.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/sha256.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/sha3.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/sha512.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/sp_c32.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/sp_c64.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/sp_int.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/tfm.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/wc_port.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/wolfmath.c $(LDFLAGS) + # unit-stm32l5-write runs the real hal_flash_write() from hal/stm32l5.c # (an 8-byte program unit was read whole even when len left a # partial unit, over-reading the caller's buffer and writing the excess diff --git a/tools/unit-tests/unit-ecc-raw-der.c b/tools/unit-tests/unit-ecc-raw-der.c new file mode 100644 index 00000000..36df3269 --- /dev/null +++ b/tools/unit-tests/unit-ecc-raw-der.c @@ -0,0 +1,289 @@ +/* unit-ecc-raw-der.c + * + * Regression test for F-11024: the wolfHSM verify path of + * wolfBoot_verify_signature_ecc() (src/image.c) converts the fixed-width + * raw R||S signature to DER with wc_ecc_rs_raw_to_sig(). It passed the + * minimal field sizes (mp_unsigned_bin_size) while leaving the pointers + * at the start of each fixed-width field, so whenever R or S had a + * leading zero byte the conversion encoded a zero-padded integer with + * the low bytes truncated, and wc_ecc_verify_hash() rejected an + * otherwise valid signature. + * + * The fix passes the full-width fields (point_sz for both) and drops + * the now-unused multiprecision sizing. This test runs the real + * wolfCrypt conversion and verification: it signs until a signature + * with a leading zero shows up, then asserts that the minimal-size + * pattern rejects it while the full-width pattern accepts the same + * signature. Both patterns must also accept leading-zero-free + * signatures, so the fix does not change the common case. + * + * Notes on the build: WOLFCRYPT_TEST pulls in the full ECC (sign and + * verify) and the deterministic-k mode (WOLFSSL_ECDSA_SET_K), so the + * digest varies per iteration to get independent signatures, and + * wc_ecc_sign_hash() emits DER here and is decoded back to the + * fixed-width raw layout wolfBoot stores in the image header. The + * check harness does not keep globals between tests, so the search + * runs in every setup. + * Copyright (C) 2026 wolfSSL Inc. + * + * This file is part of wolfBoot. + * + * wolfBoot is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfBoot is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA + */ + +#include +#include +#include +#include + +#include +#include + +/* The test build config wires the RNG block generator to this hook + * (include/user_settings.h, CUSTOM_RAND_GENERATE_BLOCK); it only feeds + * the DRBG seed, never the signature bytes. */ +int my_rng_seed_gen(unsigned char* output, unsigned int sz) +{ + int i; + + for (i = 0; i < (int)sz; i++) + output[i] = (unsigned char)(i * 37 + 11); + return 0; +} + +#define POINT_SZ 32 +#define DIGEST_SZ 32 +#define MAX_SIGS 1000 + +static WC_RNG g_rng; +static ecc_key g_ecc; +static byte g_digest[DIGEST_SZ]; +static byte g_raw_sig[2 * POINT_SZ]; +static byte g_search_digest[DIGEST_SZ]; +static int g_have_leading_zero_sig; + +static void find_leading_zero_sig(void); +static int sign_raw(const byte* digest); +static int leading_zeros(const byte* field); +static int convert_and_verify(const byte* raw, const byte* digest, + int full_width, int* valid); + +/* Sign the digest and pack the result as fixed-width raw R||S, the + * layout wolfBoot stores in the image header. wc_ecc_sign_hash() emits + * DER in this build, so the components are decoded and re-padded. */ +static int sign_raw(const byte* digest) +{ + byte der[255]; + byte r[POINT_SZ]; + byte s[POINT_SZ]; + word32 derSz = sizeof(der); + word32 rSz = sizeof(r); + word32 sSz = sizeof(s); + int ret; + + ret = wc_ecc_sign_hash(digest, DIGEST_SZ, der, &derSz, &g_rng, &g_ecc); + if (ret != 0) + return ret; + ret = wc_ecc_sig_to_rs(der, derSz, r, &rSz, s, &sSz); + if (ret != 0) + return ret; + memset(g_raw_sig, 0, sizeof(g_raw_sig)); + memcpy(g_raw_sig + (POINT_SZ - rSz), r, rSz); + memcpy(g_raw_sig + POINT_SZ + (POINT_SZ - sSz), s, sSz); + return 0; +} + +/* Number of leading zero bytes of a fixed-width field. */ +static int leading_zeros(const byte* field) +{ + int n = 0; + + while ((n < POINT_SZ) && (field[n] == 0)) + n++; + return n; +} + +/* Convert raw R||S to DER and verify it against the digest the + * signature was made over. + * full_width: pass point_sz for both fields (the fix); + * otherwise pass the minimal sizes with the field-start pointers + * (the pre-fix pattern). */ +static int convert_and_verify(const byte* raw, const byte* digest, + int full_width, int* valid) +{ + int ret; + int res = 0; + word32 rSz = POINT_SZ; + word32 sSz = POINT_SZ; + word32 derSz = 255; + byte der[255]; + + if (!full_width) { + rSz = (word32)(POINT_SZ - leading_zeros(raw)); + sSz = (word32)(POINT_SZ - leading_zeros(raw + POINT_SZ)); + } + ret = wc_ecc_rs_raw_to_sig(raw, rSz, raw + POINT_SZ, sSz, der, &derSz); + if (ret != 0) + return ret; + + ret = wc_ecc_verify_hash(der, derSz, digest, DIGEST_SZ, &res, &g_ecc); + *valid = res; + return ret; +} + +/* Sign until a signature has a leading zero in R or S (the trigger for + * the pre-fix bug); keep the signature in g_raw_sig and its digest in + * g_search_digest. The build uses deterministic k (WOLFSSL_ECDSA_SET_K), + * so the digest varies per iteration to get independent signatures. + * With a ~1/128 chance per signature, 1000 iterations come up short + * ~0.04% of the time; the dependent test skips itself in that case. */ +static void find_leading_zero_sig(void) +{ + byte digest[DIGEST_SZ]; + int i; + + g_have_leading_zero_sig = 0; + for (i = 0; i < MAX_SIGS; i++) { + memcpy(digest, g_digest, sizeof(digest)); + digest[DIGEST_SZ - 1] = (byte)i; + if (sign_raw(digest) != 0) + return; + if ((g_raw_sig[0] == 0) || (g_raw_sig[POINT_SZ] == 0)) { + g_have_leading_zero_sig = 1; + memcpy(g_search_digest, digest, sizeof(digest)); + return; + } + } +} + +static void setup(void) +{ + int i; + + for (i = 0; i < DIGEST_SZ; i++) + g_digest[i] = (byte)(i + 1); + + ck_assert_int_eq(wc_InitRng(&g_rng), 0); + ck_assert_int_eq(wc_ecc_init(&g_ecc), 0); + ck_assert_int_eq(wc_ecc_make_key(&g_rng, POINT_SZ, &g_ecc), 0); + find_leading_zero_sig(); +} + +static void teardown(void) +{ + wc_ecc_free(&g_ecc); + wc_FreeRng(&g_rng); +} + +/* The full-width conversion verifies every signature, including the + * leading-zero one: the fix is correct. */ +START_TEST(test_full_width_always_verifies) +{ + int i; + int valid; + + /* First: the search signature from setup, before the loop below + * overwrites g_raw_sig. */ + if (g_have_leading_zero_sig) { + ck_assert_int_eq(convert_and_verify(g_raw_sig, g_search_digest, 1, + &valid), 0); + ck_assert_int_eq(valid, 1); + } + + for (i = 0; i < 16; i++) { + ck_assert_int_eq(sign_raw(g_digest), 0); + ck_assert_int_eq(convert_and_verify(g_raw_sig, g_digest, 1, + &valid), 0); + ck_assert_int_eq(valid, 1); + } +} +END_TEST + +/* The pre-fix pattern (minimal sizes, field-start pointers) rejects + * the same leading-zero signature that the full-width pattern + * accepts: the signature is valid, the conversion was wrong. */ +START_TEST(test_minimal_pattern_rejects_leading_zero_sig) +{ + int valid; + + if (!g_have_leading_zero_sig) { + printf("no leading-zero signature found, skipping\n"); + return; + } + + ck_assert_int_eq(convert_and_verify(g_raw_sig, g_search_digest, 1, + &valid), 0); + ck_assert_int_eq(valid, 1); + + ck_assert_int_eq(convert_and_verify(g_raw_sig, g_search_digest, 0, + &valid), 0); + ck_assert_int_eq(valid, 0); +} +END_TEST + +/* Without leading zeros both patterns agree and the signature + * verifies: the common case is untouched by the fix. */ +START_TEST(test_no_leading_zero_both_patterns_agree) +{ + byte digest[DIGEST_SZ]; + int valid; + int i; + int checked; + + for (i = 0, checked = 0; i < 8 && checked < 4; i++) { + memcpy(digest, g_digest, sizeof(digest)); + digest[DIGEST_SZ - 1] = (byte)(0xC0 + i); + ck_assert_int_eq(sign_raw(digest), 0); + if ((g_raw_sig[0] == 0) || (g_raw_sig[POINT_SZ] == 0)) + continue; + + ck_assert_int_eq(convert_and_verify(g_raw_sig, digest, 1, + &valid), 0); + ck_assert_int_eq(valid, 1); + ck_assert_int_eq(convert_and_verify(g_raw_sig, digest, 0, + &valid), 0); + ck_assert_int_eq(valid, 1); + checked++; + } + ck_assert_int_gt(checked, 0); +} +END_TEST + +Suite *ecc_raw_der_suite(void) +{ + Suite *s = suite_create("ecc-raw-der"); + TCase *tc = tcase_create("ecc-raw-der"); + + tcase_add_checked_fixture(tc, setup, teardown); + tcase_add_test(tc, test_full_width_always_verifies); + tcase_add_test(tc, test_minimal_pattern_rejects_leading_zero_sig); + tcase_add_test(tc, test_no_leading_zero_both_patterns_agree); + suite_add_tcase(s, tc); + + return s; +} + +int main(void) +{ + int fails; + Suite *s = ecc_raw_der_suite(); + SRunner *sr = srunner_create(s); + + srunner_run_all(sr, CK_NORMAL); + fails = srunner_ntests_failed(sr); + srunner_free(sr); + + return fails; +}