From 307c2e85489dd3450628ae33e9a821a02e77c59e Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Tue, 4 Aug 2026 07:02:53 +0200 Subject: [PATCH 1/5] F-7389: arm_tee: reject Secure .base in zero-length PSA iovecs arm_tee_psa_call() only ran cmse_check_address_range() on descriptors whose .len was non-zero, so a non-secure caller could pass an outvec of {Secure address, 0} and skip attribution checking entirely. Several dispatch handlers write a fixed-size object through out_vec[0].base without consulting out_vec[0].len (ARM_TEE_PS_GET_SUPPORT, ARM_TEE_CRYPTO_OPEN_KEY/IMPORT_KEY/GENERATE_KEY and GET_KEY_ATTRIBUTES), which turned that into an arbitrary write into Secure memory from the non-secure world. Check every non-NULL .base with at least one byte regardless of the declared length, and require the handlers that write a fixed-size object to be given a large enough output descriptor. Adds unit tests covering the zero-length Secure outvec and the PS_GET_SUPPORT length check; the CMSE stub is now a test-provided function so it can model a Secure region. --- src/arm_tee_psa_ipc.c | 39 ++++-- tools/unit-tests/arm_cmse.h | 7 +- tools/unit-tests/unit-arm-tee-psa-ipc.c | 156 ++++++++++++++++++++++++ 3 files changed, 188 insertions(+), 14 deletions(-) diff --git a/src/arm_tee_psa_ipc.c b/src/arm_tee_psa_ipc.c index aaf752673d..f624458582 100644 --- a/src/arm_tee_psa_ipc.c +++ b/src/arm_tee_psa_ipc.c @@ -395,7 +395,8 @@ static psa_status_t wolfboot_crypto_dispatch(const psa_invec *in_vec, return psa_generate_random((uint8_t *)out_vec[0].base, out_vec[0].len); case ARM_TEE_CRYPTO_OPEN_KEY_SID: - if (out_vec == NULL || out_len < 1) { + if (out_vec == NULL || out_len < 1 || out_vec[0].base == NULL || + out_vec[0].len < sizeof(psa_key_id_t)) { return PSA_ERROR_INVALID_ARGUMENT; } return wolfboot_psa_open_key(iov->key_id, @@ -405,7 +406,9 @@ static psa_status_t wolfboot_crypto_dispatch(const psa_invec *in_vec, return wolfboot_psa_close_key(iov->key_id); case ARM_TEE_CRYPTO_IMPORT_KEY_SID: - if (in_len < 3 || out_vec == NULL || out_len < 1) { + if (in_len < 3 || out_vec == NULL || out_len < 1 || + out_vec[0].base == NULL || + out_vec[0].len < sizeof(psa_key_id_t)) { return PSA_ERROR_INVALID_ARGUMENT; } if (in_vec[1].base == NULL || @@ -425,7 +428,9 @@ static psa_status_t wolfboot_crypto_dispatch(const psa_invec *in_vec, } case ARM_TEE_CRYPTO_GENERATE_KEY_SID: - if (in_len < 2 || out_vec == NULL || out_len < 1) { + if (in_len < 2 || out_vec == NULL || out_len < 1 || + out_vec[0].base == NULL || + out_vec[0].len < sizeof(psa_key_id_t)) { return PSA_ERROR_INVALID_ARGUMENT; } if (in_vec[1].base == NULL || @@ -478,7 +483,8 @@ static psa_status_t wolfboot_crypto_dispatch(const psa_invec *in_vec, } case ARM_TEE_CRYPTO_GET_KEY_ATTRIBUTES_SID: - if (out_vec == NULL || out_len < 1) { + if (out_vec == NULL || out_len < 1 || out_vec[0].base == NULL || + out_vec[0].len < sizeof(psa_key_attributes_t)) { return PSA_ERROR_INVALID_ARGUMENT; } return psa_get_key_attributes(iov->key_id, @@ -1006,11 +1012,13 @@ static int32_t arm_tee_psa_ps_dispatch(int32_t type, const psa_invec *in_vec, return PSA_SUCCESS; } if (type == ARM_TEE_PS_GET_SUPPORT) { - if (out_vec != NULL && out_len >= 1 && out_vec[0].base != NULL) { - uint32_t support = 0; - XMEMCPY(out_vec[0].base, &support, sizeof(support)); - out_vec[0].len = sizeof(support); + uint32_t support = 0; + if (out_vec == NULL || out_len < 1 || out_vec[0].base == NULL || + out_vec[0].len < sizeof(support)) { + return PSA_ERROR_INVALID_ARGUMENT; } + XMEMCPY(out_vec[0].base, &support, sizeof(support)); + out_vec[0].len = sizeof(support); return PSA_SUCCESS; } return PSA_ERROR_NOT_SUPPORTED; @@ -1067,13 +1075,19 @@ int32_t arm_tee_psa_call(psa_handle_t handle, int32_t type, out_vec_s[i] = out_vec[i]; } + /* Every non-NULL .base must pass the non-secure attribution check, even + * when the declared .len is zero: a descriptor is not guaranteed to be + * accessed only within .len, so a zero-length descriptor would otherwise + * smuggle a Secure pointer past validation. At least one byte is always + * checked. */ for (i = 0; i < in_len; i++) { if (in_vec_s[i].len > 0 && in_vec_s[i].base == NULL) { return PSA_ERROR_INVALID_ARGUMENT; } - if (in_vec_s[i].len > 0 && + if (in_vec_s[i].base != NULL && cmse_check_address_range((void *)in_vec_s[i].base, - in_vec_s[i].len, + in_vec_s[i].len > 0 ? + in_vec_s[i].len : 1, CMSE_NONSECURE) == NULL) { return PSA_ERROR_INVALID_ARGUMENT; } @@ -1082,9 +1096,10 @@ int32_t arm_tee_psa_call(psa_handle_t handle, int32_t type, if (out_vec_s[i].len > 0 && out_vec_s[i].base == NULL) { return PSA_ERROR_INVALID_ARGUMENT; } - if (out_vec_s[i].len > 0 && + if (out_vec_s[i].base != NULL && cmse_check_address_range(out_vec_s[i].base, - out_vec_s[i].len, + out_vec_s[i].len > 0 ? + out_vec_s[i].len : 1, CMSE_NONSECURE) == NULL) { return PSA_ERROR_INVALID_ARGUMENT; } diff --git a/tools/unit-tests/arm_cmse.h b/tools/unit-tests/arm_cmse.h index 7423acb491..1fe8446ce9 100644 --- a/tools/unit-tests/arm_cmse.h +++ b/tools/unit-tests/arm_cmse.h @@ -1,10 +1,13 @@ #ifndef UNIT_TEST_ARM_CMSE_H #define UNIT_TEST_ARM_CMSE_H +#include #include #define CMSE_NONSECURE 0 -#define cmse_check_address_range(ptr, size, flags) \ - ((void *)(uintptr_t)(ptr)) + +/* Provided by the unit test, so it can model a Secure region that must never + * pass a non-secure attribution check. */ +void *cmse_check_address_range(void *ptr, size_t size, int flags); #endif diff --git a/tools/unit-tests/unit-arm-tee-psa-ipc.c b/tools/unit-tests/unit-arm-tee-psa-ipc.c index d0e560a10d..8b4fd04bf1 100644 --- a/tools/unit-tests/unit-arm-tee-psa-ipc.c +++ b/tools/unit-tests/unit-arm-tee-psa-ipc.c @@ -17,8 +17,99 @@ void wc_ForceZero(void *mem, size_t len) ForceZero(mem, len); } +/* Simulated Secure SRAM: any pointer landing in here is rejected by the CMSE + * stub below, exactly like a real Secure address fails CMSE_NONSECURE. */ +static uint8_t secure_mem[64]; + +void *cmse_check_address_range(void *ptr, size_t size, int flags) +{ + uint8_t *start = (uint8_t *)ptr; + uint8_t *end; + + (void)flags; + if (size == 0) { + size = 1; + } + end = start + size; + if (end > secure_mem && start < secure_mem + sizeof(secure_mem)) { + return NULL; + } + return ptr; +} + #include "../../src/arm_tee_psa_ipc.c" +/* Backend stubs: the tests below only exercise the IPC argument validation, + * never the crypto/attestation back ends. */ +psa_status_t psa_crypto_init(void) { return PSA_SUCCESS; } +psa_status_t psa_generate_random(uint8_t *o, size_t s) +{ (void)o; (void)s; return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_get_key_attributes(psa_key_id_t k, psa_key_attributes_t *a) +{ (void)k; (void)a; return PSA_ERROR_NOT_SUPPORTED; } +void psa_reset_key_attributes(psa_key_attributes_t *a) { (void)a; } +psa_status_t psa_destroy_key(psa_key_id_t k) +{ (void)k; return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_import_key(const psa_key_attributes_t *a, const uint8_t *d, + size_t dl, psa_key_id_t *k) +{ (void)a; (void)d; (void)dl; (void)k; return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_generate_key(const psa_key_attributes_t *a, psa_key_id_t *k) +{ (void)a; (void)k; return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_export_key(psa_key_id_t k, uint8_t *d, size_t ds, size_t *dl) +{ (void)k; (void)d; (void)ds; (void)dl; return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_export_public_key(psa_key_id_t k, uint8_t *d, size_t ds, + size_t *dl) +{ (void)k; (void)d; (void)ds; (void)dl; return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_hash_compute(psa_algorithm_t alg, const uint8_t *i, size_t il, + uint8_t *h, size_t hs, size_t *hl) +{ (void)alg; (void)i; (void)il; (void)h; (void)hs; (void)hl; + return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_hash_setup(psa_hash_operation_t *op, psa_algorithm_t alg) +{ (void)op; (void)alg; return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_hash_update(psa_hash_operation_t *op, const uint8_t *i, + size_t il) +{ (void)op; (void)i; (void)il; return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_hash_finish(psa_hash_operation_t *op, uint8_t *h, size_t hs, + size_t *hl) +{ (void)op; (void)h; (void)hs; (void)hl; return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_hash_clone(const psa_hash_operation_t *s, + psa_hash_operation_t *t) +{ (void)s; (void)t; return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_hash_abort(psa_hash_operation_t *op) +{ (void)op; return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_cipher_encrypt_setup(psa_cipher_operation_t *op, + psa_key_id_t k, psa_algorithm_t alg) +{ (void)op; (void)k; (void)alg; return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_cipher_decrypt_setup(psa_cipher_operation_t *op, + psa_key_id_t k, psa_algorithm_t alg) +{ (void)op; (void)k; (void)alg; return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_cipher_set_iv(psa_cipher_operation_t *op, const uint8_t *iv, + size_t ivl) +{ (void)op; (void)iv; (void)ivl; return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_cipher_update(psa_cipher_operation_t *op, const uint8_t *i, + size_t il, uint8_t *o, size_t os, size_t *ol) +{ (void)op; (void)i; (void)il; (void)o; (void)os; (void)ol; + return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_cipher_finish(psa_cipher_operation_t *op, uint8_t *o, + size_t os, size_t *ol) +{ (void)op; (void)o; (void)os; (void)ol; return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_cipher_abort(psa_cipher_operation_t *op) +{ (void)op; return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_sign_hash(psa_key_id_t k, psa_algorithm_t alg, + const uint8_t *h, size_t hl, uint8_t *s, size_t ss, size_t *sl) +{ (void)k; (void)alg; (void)h; (void)hl; (void)s; (void)ss; (void)sl; + return PSA_ERROR_NOT_SUPPORTED; } +psa_status_t psa_verify_hash(psa_key_id_t k, psa_algorithm_t alg, + const uint8_t *h, size_t hl, const uint8_t *s, size_t sl) +{ (void)k; (void)alg; (void)h; (void)hl; (void)s; (void)sl; + return PSA_ERROR_NOT_SUPPORTED; } +int wolfBoot_dice_get_token(const uint8_t *c, size_t cs, uint8_t *t, size_t ts, + size_t *tl) +{ (void)c; (void)cs; (void)t; (void)ts; (void)tl; return -1; } +int wolfBoot_dice_get_token_size(size_t cs, size_t *ts) +{ (void)cs; (void)ts; return -1; } +int wolfBoot_dice_get_attest_pubkey(uint8_t *b, size_t *l) +{ (void)b; (void)l; return -1; } + static void reset_ps_state(void) { memset(g_ps_entries, 0, sizeof(g_ps_entries)); @@ -164,6 +255,68 @@ START_TEST(test_ps_set_get_info_remove_success_path) } END_TEST +START_TEST(test_psa_call_rejects_secure_zero_len_outvec) +{ + psa_outvec out_vec[1]; + size_t i; + + reset_ps_state(); + memset(secure_mem, 0xA5, sizeof(secure_mem)); + + /* A zero-length descriptor pointing at Secure memory must not pass + * validation: ARM_TEE_PS_GET_SUPPORT writes through .base regardless of + * the declared length. */ + out_vec[0].base = secure_mem; + out_vec[0].len = 0; + + ck_assert_int_eq( + arm_tee_psa_call((psa_handle_t)ARM_TEE_PROTECTED_STORAGE_HANDLE, + ARM_TEE_PS_GET_SUPPORT, NULL, 0, out_vec, 1), + PSA_ERROR_INVALID_ARGUMENT); + + for (i = 0; i < sizeof(secure_mem); i++) { + ck_assert_uint_eq(secure_mem[i], 0xA5); + } +} +END_TEST + +START_TEST(test_ps_get_support_rejects_short_outvec) +{ + uint8_t buf[sizeof(uint32_t)]; + psa_outvec out_vec[1]; + + reset_ps_state(); + memset(buf, 0xA5, sizeof(buf)); + + out_vec[0].base = buf; + out_vec[0].len = sizeof(uint32_t) - 1; + + ck_assert_int_eq( + arm_tee_psa_test_ps_dispatch(ARM_TEE_PS_GET_SUPPORT, NULL, 0, + out_vec, 1), + PSA_ERROR_INVALID_ARGUMENT); + ck_assert_uint_eq(buf[0], 0xA5); +} +END_TEST + +START_TEST(test_ps_get_support_success_path) +{ + uint32_t support = 0xFFFFFFFFU; + psa_outvec out_vec[1]; + + reset_ps_state(); + out_vec[0].base = &support; + out_vec[0].len = sizeof(support); + + ck_assert_int_eq( + arm_tee_psa_call((psa_handle_t)ARM_TEE_PROTECTED_STORAGE_HANDLE, + ARM_TEE_PS_GET_SUPPORT, NULL, 0, out_vec, 1), + PSA_SUCCESS); + ck_assert_uint_eq(support, 0); + ck_assert_uint_eq(out_vec[0].len, sizeof(support)); +} +END_TEST + Suite *arm_tee_psa_ipc_suite(void) { Suite *s = suite_create("arm-tee-psa-ipc"); @@ -174,6 +327,9 @@ Suite *arm_tee_psa_ipc_suite(void) tcase_add_test(tc, test_ps_get_info_rejects_short_uid_vector); tcase_add_test(tc, test_ps_remove_rejects_short_uid_vector); tcase_add_test(tc, test_ps_set_get_info_remove_success_path); + tcase_add_test(tc, test_psa_call_rejects_secure_zero_len_outvec); + tcase_add_test(tc, test_ps_get_support_rejects_short_outvec); + tcase_add_test(tc, test_ps_get_support_success_path); suite_add_tcase(s, tc); return s; From 0e53cac36c8b4ae6b6ea5f94c20f717cf6baa492 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Tue, 4 Aug 2026 08:07:18 +0200 Subject: [PATCH 2/5] F-7065: tpm: snapshot NS length in wolfBoot_tpm2_read_cert veneer wolfBoot_tpm2_read_cert() is a cmse_nonsecure_entry veneer. It dereferenced the caller-supplied 'certSz' to bound-check 'cert' with cmse_check_address_range(), then passed the same non-secure pointer to wolfTPM2_NVReadCert(), which re-reads '*len' as the destination capacity before copying the NV data (lib/wolfTPM/src/tpm2_wrap.c:7221). The length was therefore fetched twice from non-secure memory with no snapshot in between, so a racing non-secure agent could present a small capacity to pass the CMSE check and enlarge it before wolfTPM's own check, making the secure world write the certificate past the validated range and into adjacent Secure SRAM. Single-fetch the capacity into a secure local before validating, hand wolfTPM the local, and copy the result back, matching ns_outlen_begin() in src/pkcs11_callable.c and the rsp_capacity handling in src/wolfhsm_callable.c and src/fwtpm_callable.c. Add unit-tpm-nsc-cert, which drives the veneer through a CMSE stub that models Secure SRAM immediately after the validated non-secure buffer and a wolfTPM stub that enlarges the non-secure length word in the race window. The out-of-bounds write test fails before this fix and passes after it. --- .gitignore | 1 + src/tpm.c | 17 ++- tools/unit-tests/Makefile | 7 + tools/unit-tests/unit-tpm-nsc-cert.c | 213 +++++++++++++++++++++++++++ 4 files changed, 236 insertions(+), 2 deletions(-) create mode 100644 tools/unit-tests/unit-tpm-nsc-cert.c diff --git a/.gitignore b/.gitignore index 8884036888..0b2ec23612 100644 --- a/.gitignore +++ b/.gitignore @@ -203,6 +203,7 @@ tools/unit-tests/unit-mpusize tools/unit-tests/unit-otp-keystore tools/unit-tests/unit-otp-keystore-gen-zeroize tools/unit-tests/unit-tpm-api-names +tools/unit-tests/unit-tpm-nsc-cert tools/unit-tests/unit-elf-bss-guard tools/unit-tests/unit-fit-fpga tools/unit-tests/unit-flash-erase-c0 diff --git a/src/tpm.c b/src/tpm.c index efb783f85b..08285bee23 100644 --- a/src/tpm.c +++ b/src/tpm.c @@ -1334,14 +1334,27 @@ int CSME_NSE_API wolfBoot_tpm2_read_pcr(uint8_t pcrIndex, uint8_t* digest, int* int CSME_NSE_API wolfBoot_tpm2_read_cert(uint32_t handle, uint8_t* cert, uint32_t* certSz) { + uint32_t certCapacity; + int rc; + if (WOLFBOOT_TPM_NS_RW(certSz, sizeof(*certSz)) == NULL) { return BAD_FUNC_ARG; } - if (WOLFBOOT_TPM_NS_RW(cert, *certSz) == NULL) { + /* single-fetch *certSz so it cannot be re-read after validation: wolfTPM + * checks the capacity again before filling 'cert', and a racing non-secure + * agent would otherwise enlarge it in between to reopen the write past the + * range validated here */ + certCapacity = *(volatile const uint32_t*)certSz; + if (certCapacity == 0) { + return BAD_FUNC_ARG; + } + if (WOLFBOOT_TPM_NS_RW(cert, certCapacity) == NULL) { return BAD_FUNC_ARG; } wolfTPM2_SetAuthPassword(&wolftpm_dev, 0, NULL); - return wolfTPM2_NVReadCert(&wolftpm_dev, handle, cert, certSz); + rc = wolfTPM2_NVReadCert(&wolftpm_dev, handle, cert, &certCapacity); + *certSz = certCapacity; + return rc; } #ifdef WOLFTPM_MFG_IDENTITY diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index ea64692297..a011ebc579 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -64,6 +64,7 @@ TESTS:=unit-parser unit-fdt unit-extflash unit-string unit-spi-flash unit-aes128 unit-keygen-xmss-params TESTS+=unit-tpm-check-rot-auth TESTS+=unit-tpm-api-names +TESTS+=unit-tpm-nsc-cert TESTS+=unit-diagnostics TESTS+=unit-diagnostics-256 TESTS+=unit-fit-gzip unit-fit-nogzip @@ -280,6 +281,12 @@ unit-tpm-api-names: ../../include/target.h unit-tpm-api-names.c ../../src/string -DWOLFBOOT_HASH_SHA256 \ -ffunction-sections -fdata-sections $(LDFLAGS) -Wl,--gc-sections +unit-tpm-nsc-cert: ../../include/target.h unit-tpm-nsc-cert.c ../../src/string.c + gcc -o $@ $^ $(CFLAGS) -I$(WOLFBOOT_LIB_WOLFTPM) -DWOLFBOOT_TPM \ + -DWOLFTPM_USER_SETTINGS -DWOLFBOOT_SIGN_RSA2048 \ + -DWOLFBOOT_HASH_SHA256 -D__ARM_FEATURE_CMSE=3U -DCSME_NSE_API= \ + -ffunction-sections -fdata-sections $(LDFLAGS) -Wl,--gc-sections + unit-fwtpm-stub: ../../include/target.h unit-fwtpm-stub.c gcc -o $@ $^ $(CFLAGS) -I$(WOLFBOOT_LIB_WOLFTPM) \ -DWOLFTPM_USER_SETTINGS -ffunction-sections -fdata-sections \ diff --git a/tools/unit-tests/unit-tpm-nsc-cert.c b/tools/unit-tests/unit-tpm-nsc-cert.c new file mode 100644 index 0000000000..06e0970af1 --- /dev/null +++ b/tools/unit-tests/unit-tpm-nsc-cert.c @@ -0,0 +1,213 @@ +/* unit-tpm-nsc-cert.c + * + * Unit tests for the wolfBoot_tpm2_read_cert() non-secure entry veneer. + * + * The veneer validates the caller-supplied output buffer against the length + * word the non-secure caller points at. That length must be snapshotted into + * Secure memory before validation, otherwise a racing non-secure agent (a + * second NS thread, an NS interrupt, or NS-programmed DMA) can enlarge it + * between the veneer's cmse_check_address_range() and wolfTPM's own capacity + * check, and the secure world writes NV data past the validated range. + */ + +#include +#include +#include +#include + +#ifndef SPI_CS_TPM +#define SPI_CS_TPM 1 +#endif + +#include "tpm.h" + +/* Simulated non-secure region followed by the Secure SRAM adjacent to it. Any + * range touching 'secure' is rejected by the CMSE stub below, exactly like a + * real Secure address fails CMSE_NONSECURE. */ +#define NS_CERT_CAP 16 +static struct { + uint8_t cert[NS_CERT_CAP]; + uint8_t secure[64]; +} ns_edge; + +/* Non-secure length word handed to the veneer, and the value a racing + * non-secure agent stores into it once the veneer has validated it. */ +static uint32_t ns_cert_sz; +static uint32_t ns_race_value; + +/* Size of the certificate the TPM reports for the requested NV index. */ +static uint32_t nv_cert_size; + +void *cmse_check_address_range(void *ptr, size_t size, int flags) +{ + uint8_t *start = (uint8_t *)ptr; + uint8_t *end; + + (void)flags; + if (start == NULL) { + return NULL; + } + if (size == 0) { + size = 1; + } + end = start + size; + if (end > ns_edge.secure && + start < ns_edge.secure + sizeof(ns_edge.secure)) { + return NULL; + } + return ptr; +} + +int wolfBoot_printf(const char* fmt, ...) +{ + (void)fmt; + return 0; +} + +const char* TPM2_GetAlgName(TPM_ALG_ID alg) +{ + (void)alg; + return NULL; +} + +const char* TPM2_GetRCString(int rc) +{ + (void)rc; + return NULL; +} + +int wolfTPM2_SetAuthPassword(WOLFTPM2_DEV* dev, int index, + const TPM2B_AUTH* auth) +{ + (void)dev; + (void)index; + (void)auth; + return 0; +} + +/* Faithful stand-in for wolfTPM2_NVReadCert() (lib/wolfTPM/src/tpm2_wrap.c): + * '*len' is an input capacity, re-read after the veneer's own validation, and + * the NV data is copied into 'buffer' only if it fits. The racing non-secure + * write is modelled here because that is exactly the window it occupies. */ +int wolfTPM2_NVReadCert(WOLFTPM2_DEV* dev, TPM_HANDLE handle, + uint8_t* buffer, uint32_t* len) +{ + (void)dev; + (void)handle; + + if (len == NULL) { + return BAD_FUNC_ARG; + } + if (ns_race_value != 0) { + ns_cert_sz = ns_race_value; /* NS agent enlarges the length word */ + } + if (nv_cert_size > *len) { + return BUFFER_E; + } + *len = nv_cert_size; + memset(buffer, 0xA5, nv_cert_size); + return 0; +} + +#include "../../src/tpm.c" + +static void setup_ns_edge(uint32_t certSz, uint32_t race, uint32_t nvSize) +{ + memset(&ns_edge, 0xEE, sizeof(ns_edge)); + ns_cert_sz = certSz; + ns_race_value = race; + nv_cert_size = nvSize; +} + +static int secure_untouched(void) +{ + unsigned int i; + + for (i = 0; i < sizeof(ns_edge.secure); i++) { + if (ns_edge.secure[i] != 0xEE) { + return 0; + } + } + return 1; +} + +/* A non-secure caller presents a 16-byte capacity, so only 16 bytes of NS + * memory are validated, then enlarges the length word. The 48-byte NV + * certificate must not be written, because 32 of those bytes land in the + * Secure SRAM following the validated range. */ +START_TEST(test_read_cert_ns_length_race) +{ + int rc; + + setup_ns_edge(NS_CERT_CAP, sizeof(ns_edge), 48); + + rc = wolfBoot_tpm2_read_cert(0x01C00002, ns_edge.cert, &ns_cert_sz); + + ck_assert_int_eq(secure_untouched(), 1); + ck_assert_int_ne(rc, 0); +} +END_TEST + +/* Without a race the veneer must still behave as documented: the certificate + * is copied and the non-secure length word receives the actual size. */ +START_TEST(test_read_cert_normal) +{ + uint8_t cert[128]; + uint32_t certSz = (uint32_t)sizeof(cert); + int rc; + + setup_ns_edge(0, 0, 48); + memset(cert, 0xEE, sizeof(cert)); + + rc = wolfBoot_tpm2_read_cert(0x01C00002, cert, &certSz); + + ck_assert_int_eq(rc, 0); + ck_assert_uint_eq(certSz, 48); + ck_assert_int_eq(cert[0], 0xA5); + ck_assert_int_eq(cert[47], 0xA5); + ck_assert_int_eq(cert[48], 0xEE); +} +END_TEST + +/* A length word pointing into Secure memory must be rejected outright. */ +START_TEST(test_read_cert_secure_length_pointer) +{ + int rc; + + setup_ns_edge(NS_CERT_CAP, 0, 48); + + rc = wolfBoot_tpm2_read_cert(0x01C00002, ns_edge.cert, + (uint32_t*)ns_edge.secure); + + ck_assert_int_eq(rc, BAD_FUNC_ARG); + ck_assert_int_eq(secure_untouched(), 1); +} +END_TEST + +static Suite* tpm_nsc_cert_suite(void) +{ + Suite* s; + TCase* tc; + + s = suite_create("TPM NSC read cert"); + tc = tcase_create("ns_bounds"); + tcase_add_test(tc, test_read_cert_ns_length_race); + tcase_add_test(tc, test_read_cert_normal); + tcase_add_test(tc, test_read_cert_secure_length_pointer); + suite_add_tcase(s, tc); + return s; +} + +int main(void) +{ + Suite* s; + SRunner* sr; + int failed; + + s = tpm_nsc_cert_suite(); + sr = srunner_create(s); + srunner_run_all(sr, CK_NORMAL); + failed = srunner_ntests_failed(sr); + srunner_free(sr); + return failed == 0 ? 0 : 1; +} From 675a927fa20295b7485762781b6f3980ff013d87 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Tue, 4 Aug 2026 08:13:36 +0200 Subject: [PATCH 3/5] F-7054: sign: fail when the hybrid secondary key cannot be loaded main() checked load_key() for the primary key but not for the hybrid secondary key, and load_key() left *pubkey/*pubkey_sz untouched (or dangling, after the ED25519/ED448 free(*pubkey)) on its failure paths. With a missing or undecodable secondary key file the sign tool therefore either silently emitted a manifest with no secondary public key hashed, dereferenced a freed pubkey buffer and double-freed it, or crashed on the uninitialized pubkey_sz2 stack value. Clear *pubkey/*pubkey_sz on every load_key() failure path, initialize pubkey_sz2, and exit(1) when the secondary key fails to load. Add tools/unit-tests/unit-sign-hybrid-keyload, covering the missing-file and decode-failure contracts of load_key() plus the end-to-end exit status of the sign tool. --- tools/keytools/sign.c | 20 ++- tools/unit-tests/Makefile | 9 ++ tools/unit-tests/unit-sign-hybrid-keyload.c | 161 ++++++++++++++++++++ 3 files changed, 187 insertions(+), 3 deletions(-) create mode 100644 tools/unit-tests/unit-sign-hybrid-keyload.c diff --git a/tools/keytools/sign.c b/tools/keytools/sign.c index 320c396414..d7335940b8 100644 --- a/tools/keytools/sign.c +++ b/tools/keytools/sign.c @@ -639,6 +639,8 @@ static uint8_t *load_key(uint8_t **key_buffer, uint32_t *key_buffer_sz, /* open and load key buffer */ *key_buffer = NULL; + *pubkey = NULL; + *pubkey_sz = 0; if (secondary) { key_file = CMD.secondary_key_file; sign = CMD.secondary_sign; @@ -722,8 +724,10 @@ static uint8_t *load_key(uint8_t **key_buffer, uint32_t *key_buffer_sz, wc_ed25519_free(&key.ed); } - if (ret != 0) + if (ret != 0) { free(*pubkey); + *pubkey = NULL; + } /* break if we succeed or are not using auto */ if (ret == 0 || sign != SIGN_AUTO) { @@ -789,8 +793,10 @@ static uint8_t *load_key(uint8_t **key_buffer, uint32_t *key_buffer_sz, wc_ed448_free(&key.ed4); } - if (ret != 0) + if (ret != 0) { free(*pubkey); + *pubkey = NULL; + } /* break if we succeed or are not using auto */ if (ret == 0 || sign != SIGN_AUTO) { @@ -1051,6 +1057,11 @@ static uint8_t *load_key(uint8_t **key_buffer, uint32_t *key_buffer_sz, zero_and_free(*key_buffer, *key_buffer_sz); *key_buffer = NULL; } + if (*pubkey != NULL) { + free(*pubkey); + *pubkey = NULL; + } + *pubkey_sz = 0; return NULL; } @@ -3728,9 +3739,12 @@ int main(int argc, char** argv) if (CMD.hybrid) { uint8_t *kbuf2 = NULL; uint8_t *pubkey2 = NULL; - uint32_t pubkey_sz2; + uint32_t pubkey_sz2 = 0; DEBUG_PRINT("Loading secondary key\n"); kbuf2 = load_key(&key_buffer2, &key_buffer_sz2, &pubkey2, &pubkey_sz2, 1); + if (!kbuf2) { + exit(1); + } printf("Creating hybrid signature\n"); make_hybrid_header(pubkey, pubkey_sz, CMD.image_file, CMD.output_image_file, pubkey2, pubkey_sz2); diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index a011ebc579..0af9c2c22a 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -61,6 +61,7 @@ TESTS:=unit-parser unit-fdt unit-extflash unit-string unit-spi-flash unit-aes128 unit-image-nopart unit-image-sha384 unit-image-sha3-384 unit-store-sbrk \ unit-tpm-blob unit-policy-create unit-policy-sign unit-rot-auth unit-sdhci-response-bits \ unit-sdhci-disk-unaligned unit-sign-encrypted-output \ + unit-sign-hybrid-keyload \ unit-keygen-xmss-params TESTS+=unit-tpm-check-rot-auth TESTS+=unit-tpm-api-names @@ -323,6 +324,14 @@ unit-sign-encrypted-output: ../../include/target.h unit-sign-encrypted-output.c -ffunction-sections -fdata-sections \ $(LDFLAGS) -Wl,--gc-sections +unit-sign-hybrid-keyload: ../../include/target.h unit-sign-hybrid-keyload.c \ + $(KEYTOOLS_SIGN_SRCS) + gcc -o $@ $^ -I../keytools $(CFLAGS) -DML_DSA_LEVEL=2 -DDELTA_UPDATES \ + -D"LMS_LEVELS=1" -D"LMS_HEIGHT=10" -D"LMS_WINTERNITZ=8" \ + -DWOLFBOOT_XMSS_PARAMS=\"XMSS-SHA2_10_256\" \ + -ffunction-sections -fdata-sections \ + $(LDFLAGS) -Wl,--gc-sections + unit-keygen-xmss-params: ../../include/target.h unit-keygen-xmss-params.c gcc -o $@ $^ -I../keytools $(CFLAGS) -DML_DSA_LEVEL=2 \ -D"LMS_LEVELS=1" -D"LMS_HEIGHT=10" -D"LMS_WINTERNITZ=8" \ diff --git a/tools/unit-tests/unit-sign-hybrid-keyload.c b/tools/unit-tests/unit-sign-hybrid-keyload.c new file mode 100644 index 0000000000..461dd495f5 --- /dev/null +++ b/tools/unit-tests/unit-sign-hybrid-keyload.c @@ -0,0 +1,161 @@ +/* unit-sign-hybrid-keyload.c + * + * Unit test for sign tool secondary (hybrid) key load error handling. + */ + +#include +#include +#include +#include +#include +#include +#include + +#define WOLFBOOT_HASH_SHA256 +#define IMAGE_HEADER_SIZE 512 + +#define main wolfboot_sign_main +#include "../keytools/sign.c" +#undef main + +static const char missing_key[] = "/nonexistent/wolfboot-secondary-key.der"; + +static int write_file(const char *path, const void *buf, size_t len) +{ + FILE *f = fopen(path, "wb"); + size_t written; + + if (f == NULL) { + return -1; + } + + written = fwrite(buf, 1, len, f); + fclose(f); + + return written == len ? 0 : -1; +} + +static void reset_cmd_defaults(void) +{ + memset(&CMD, 0, sizeof(CMD)); + CMD.sign = NO_SIGN; + CMD.hash_algo = HASH_SHA256; + CMD.partition_id = HDR_IMG_TYPE_APP; + CMD.header_sz = IMAGE_HEADER_SIZE; + CMD.fw_version = "1"; + CMD.no_ts = 1; +} + +/* load_key() must not hand back an unset public key when the key file + * cannot be opened at all. */ +START_TEST(test_load_key_clears_pubkey_when_file_missing) +{ + uint8_t *key_buffer = NULL; + uint32_t key_buffer_sz = 0; + uint8_t sentinel = 0xA5; + uint8_t *pubkey = &sentinel; + uint32_t pubkey_sz = 0xDEADBEEFU; + + reset_cmd_defaults(); + CMD.hybrid = 1; + CMD.secondary_sign = SIGN_ML_DSA; + CMD.secondary_key_file = missing_key; + + ck_assert_ptr_null(load_key(&key_buffer, &key_buffer_sz, &pubkey, + &pubkey_sz, 1)); + ck_assert_ptr_null(pubkey); + ck_assert_uint_eq(pubkey_sz, 0); +} +END_TEST + +/* load_key() must not hand back a dangling public key pointer when the key + * file is readable but cannot be decoded. */ +START_TEST(test_load_key_clears_pubkey_when_decode_fails) +{ + char tempdir[] = "/tmp/wolfboot-sign-XXXXXX"; + char key_path[PATH_MAX]; + uint8_t garbage[7]; + uint8_t *key_buffer = NULL; + uint32_t key_buffer_sz = 0; + uint8_t *pubkey = NULL; + uint32_t pubkey_sz = 0; + + ck_assert_ptr_nonnull(mkdtemp(tempdir)); + snprintf(key_path, sizeof(key_path), "%s/secondary.der", tempdir); + + memset(garbage, 0x5A, sizeof(garbage)); + ck_assert_int_eq(write_file(key_path, garbage, sizeof(garbage)), 0); + + reset_cmd_defaults(); + CMD.hybrid = 1; + CMD.secondary_sign = SIGN_ED25519; + CMD.secondary_key_file = key_path; + + ck_assert_ptr_null(load_key(&key_buffer, &key_buffer_sz, &pubkey, + &pubkey_sz, 1)); + ck_assert_ptr_null(pubkey); + ck_assert_uint_eq(pubkey_sz, 0); + + unlink(key_path); + rmdir(tempdir); +} +END_TEST + +/* The sign tool must fail when the hybrid secondary key cannot be loaded, + * instead of building a manifest out of an unset secondary public key. */ +START_TEST(test_sign_main_fails_when_secondary_key_missing) +{ + char tempdir[] = "/tmp/wolfboot-sign-XXXXXX"; + char image_path[PATH_MAX]; + char key_path[PATH_MAX]; + uint8_t image_buf[] = { 0x01, 0x02, 0x03, 0x04 }; + uint8_t raw_pubkey[64]; /* ECC256 raw Qx + Qy */ + char *argv[8]; + + ck_assert_ptr_nonnull(mkdtemp(tempdir)); + snprintf(image_path, sizeof(image_path), "%s/image.bin", tempdir); + snprintf(key_path, sizeof(key_path), "%s/ecc256.raw", tempdir); + + memset(raw_pubkey, 0x11, sizeof(raw_pubkey)); + ck_assert_int_eq(write_file(image_path, image_buf, sizeof(image_buf)), 0); + ck_assert_int_eq(write_file(key_path, raw_pubkey, sizeof(raw_pubkey)), 0); + + argv[0] = "sign"; + argv[1] = "--sha-only"; + argv[2] = "--ecc256"; + argv[3] = "--ml_dsa"; + argv[4] = image_path; + argv[5] = key_path; + argv[6] = (char *)missing_key; + argv[7] = "1"; + + exit(wolfboot_sign_main(8, argv)); +} +END_TEST + +Suite *wolfboot_suite(void) +{ + Suite *s = suite_create("sign-hybrid-keyload"); + TCase *tcase = tcase_create("load-key"); + + tcase_add_test(tcase, test_load_key_clears_pubkey_when_file_missing); + tcase_add_test(tcase, test_load_key_clears_pubkey_when_decode_fails); + tcase_add_exit_test(tcase, test_sign_main_fails_when_secondary_key_missing, + 1); + suite_add_tcase(s, tcase); + + return s; +} + +int main(void) +{ + int failed; + Suite *s = wolfboot_suite(); + SRunner *runner = srunner_create(s); + + srunner_run_all(runner, CK_NORMAL); + failed = srunner_ntests_failed(runner); + srunner_free(runner); + + return failed == 0 ? 0 : 1; +} From dfdcf7eeb589a32a84b7e040e45868612ab2a435 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Tue, 4 Aug 2026 08:16:53 +0200 Subject: [PATCH 4/5] F-7053: sign: propagate make_header() failure to the exit status main() called make_header()/make_hybrid_header() and discarded their return value. Both are wrappers around make_header_ex(), which returns -1 on every "goto failure" path (image file not openable, header malloc failure, firmware version out of range, certificate chain errors, signing and output write errors). Since ret is initialized to 0 and is only reassigned by the optional base_diff() delta step, a signing run that produced no output image still terminated with status 0, so Makefile recipes and CI treated the failure as success and moved on with a missing or stale *_v_signed.bin. This was also asymmetric with the key loading path just above, which exits on failure. Capture the return value of both header helpers, skip the delta step when header generation failed, and let main() return it. Add tools/unit-tests/unit-sign-header-failure, covering the exit status of both the plain and the hybrid signing path when the input image cannot be opened. --- tools/keytools/sign.c | 12 +- tools/unit-tests/Makefile | 9 ++ tools/unit-tests/unit-sign-header-failure.c | 127 ++++++++++++++++++++ 3 files changed, 143 insertions(+), 5 deletions(-) create mode 100644 tools/unit-tests/unit-sign-header-failure.c diff --git a/tools/keytools/sign.c b/tools/keytools/sign.c index d7335940b8..4f722b5647 100644 --- a/tools/keytools/sign.c +++ b/tools/keytools/sign.c @@ -3746,8 +3746,8 @@ int main(int argc, char** argv) exit(1); } printf("Creating hybrid signature\n"); - make_hybrid_header(pubkey, pubkey_sz, CMD.image_file, CMD.output_image_file, - pubkey2, pubkey_sz2); + ret = make_hybrid_header(pubkey, pubkey_sz, CMD.image_file, + CMD.output_image_file, pubkey2, pubkey_sz2); DEBUG_PRINT("Signature size: %u\n", CMD.signature_sz); DEBUG_PRINT("Secondary signature size: %u\n", CMD.secondary_signature_sz); DEBUG_PRINT("Header size: %u\n", CMD.header_sz); @@ -3756,11 +3756,13 @@ int main(int argc, char** argv) if (pubkey2) free(pubkey2); } else { - make_header(pubkey, pubkey_sz, CMD.image_file, CMD.output_image_file); + ret = make_header(pubkey, pubkey_sz, CMD.image_file, + CMD.output_image_file); } - - if (CMD.delta) { + /* Skip the delta step and propagate the failure to the caller if the + * signed image could not be created. */ + if ((ret == 0) && CMD.delta) { if (CMD.encrypt) ret = base_diff(CMD.delta_base_file, pubkey, pubkey_sz, 64); else diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index 0af9c2c22a..546e745dc4 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -62,6 +62,7 @@ TESTS:=unit-parser unit-fdt unit-extflash unit-string unit-spi-flash unit-aes128 unit-tpm-blob unit-policy-create unit-policy-sign unit-rot-auth unit-sdhci-response-bits \ unit-sdhci-disk-unaligned unit-sign-encrypted-output \ unit-sign-hybrid-keyload \ + unit-sign-header-failure \ unit-keygen-xmss-params TESTS+=unit-tpm-check-rot-auth TESTS+=unit-tpm-api-names @@ -332,6 +333,14 @@ unit-sign-hybrid-keyload: ../../include/target.h unit-sign-hybrid-keyload.c \ -ffunction-sections -fdata-sections \ $(LDFLAGS) -Wl,--gc-sections +unit-sign-header-failure: ../../include/target.h unit-sign-header-failure.c \ + $(KEYTOOLS_SIGN_SRCS) + gcc -o $@ $^ -I../keytools $(CFLAGS) -DML_DSA_LEVEL=2 -DDELTA_UPDATES \ + -D"LMS_LEVELS=1" -D"LMS_HEIGHT=10" -D"LMS_WINTERNITZ=8" \ + -DWOLFBOOT_XMSS_PARAMS=\"XMSS-SHA2_10_256\" \ + -ffunction-sections -fdata-sections \ + $(LDFLAGS) -Wl,--gc-sections + unit-keygen-xmss-params: ../../include/target.h unit-keygen-xmss-params.c gcc -o $@ $^ -I../keytools $(CFLAGS) -DML_DSA_LEVEL=2 \ -D"LMS_LEVELS=1" -D"LMS_HEIGHT=10" -D"LMS_WINTERNITZ=8" \ diff --git a/tools/unit-tests/unit-sign-header-failure.c b/tools/unit-tests/unit-sign-header-failure.c new file mode 100644 index 0000000000..e159fab43f --- /dev/null +++ b/tools/unit-tests/unit-sign-header-failure.c @@ -0,0 +1,127 @@ +/* unit-sign-header-failure.c + * + * Unit test for sign tool exit status when the manifest cannot be created. + */ + +#include +#include +#include +#include +#include +#include +#include + +#define WOLFBOOT_HASH_SHA256 +#define IMAGE_HEADER_SIZE 512 + +#define main wolfboot_sign_main +#include "../keytools/sign.c" +#undef main + +static const char missing_image[] = "/nonexistent/wolfboot-image.bin"; + +static int write_file(const char *path, const void *buf, size_t len) +{ + FILE *f = fopen(path, "wb"); + size_t written; + + if (f == NULL) { + return -1; + } + + written = fwrite(buf, 1, len, f); + fclose(f); + + return written == len ? 0 : -1; +} + +/* The sign tool must report a failure to the caller (make, CI) when + * make_header() could not produce the output image. */ +START_TEST(test_sign_main_fails_when_image_missing) +{ + char tempdir[] = "/tmp/wolfboot-sign-XXXXXX"; + char key_path[PATH_MAX]; + uint8_t raw_pubkey[64]; /* ECC256 raw Qx + Qy */ + char *argv[6]; + + ck_assert_ptr_nonnull(mkdtemp(tempdir)); + snprintf(key_path, sizeof(key_path), "%s/ecc256.raw", tempdir); + + memset(raw_pubkey, 0x11, sizeof(raw_pubkey)); + ck_assert_int_eq(write_file(key_path, raw_pubkey, sizeof(raw_pubkey)), 0); + + argv[0] = "sign"; + argv[1] = "--sha-only"; + argv[2] = "--ecc256"; + argv[3] = (char *)missing_image; + argv[4] = key_path; + argv[5] = "1"; + + ck_assert_int_ne(wolfboot_sign_main(6, argv), 0); + + unlink(key_path); + rmdir(tempdir); +} +END_TEST + +/* Same contract for the hybrid path, which uses make_hybrid_header(). */ +START_TEST(test_sign_main_fails_when_image_missing_hybrid) +{ + char tempdir[] = "/tmp/wolfboot-sign-XXXXXX"; + char key_path[PATH_MAX]; + char key2_path[PATH_MAX]; + uint8_t raw_pubkey[64]; /* ECC256 raw Qx + Qy */ + uint8_t raw_pubkey2[32]; /* ED25519 raw public key */ + char *argv[8]; + + ck_assert_ptr_nonnull(mkdtemp(tempdir)); + snprintf(key_path, sizeof(key_path), "%s/ecc256.raw", tempdir); + snprintf(key2_path, sizeof(key2_path), "%s/ed25519.raw", tempdir); + + memset(raw_pubkey, 0x11, sizeof(raw_pubkey)); + memset(raw_pubkey2, 0x22, sizeof(raw_pubkey2)); + ck_assert_int_eq(write_file(key_path, raw_pubkey, sizeof(raw_pubkey)), 0); + ck_assert_int_eq(write_file(key2_path, raw_pubkey2, sizeof(raw_pubkey2)), + 0); + + argv[0] = "sign"; + argv[1] = "--sha-only"; + argv[2] = "--ecc256"; + argv[3] = "--ed25519"; + argv[4] = (char *)missing_image; + argv[5] = key_path; + argv[6] = key2_path; + argv[7] = "1"; + + ck_assert_int_ne(wolfboot_sign_main(8, argv), 0); + + unlink(key_path); + unlink(key2_path); + rmdir(tempdir); +} +END_TEST + +Suite *wolfboot_suite(void) +{ + Suite *s = suite_create("sign-header-failure"); + TCase *tcase = tcase_create("make-header"); + + tcase_add_test(tcase, test_sign_main_fails_when_image_missing); + tcase_add_test(tcase, test_sign_main_fails_when_image_missing_hybrid); + suite_add_tcase(s, tcase); + + return s; +} + +int main(void) +{ + int failed; + Suite *s = wolfboot_suite(); + SRunner *runner = srunner_create(s); + + srunner_run_all(runner, CK_NORMAL); + failed = srunner_ntests_failed(runner); + srunner_free(runner); + + return failed == 0 ? 0 : 1; +} From 35a23bf0ef355456faecd1e456bf73d16f4effd6 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Tue, 4 Aug 2026 08:26:17 +0200 Subject: [PATCH 5/5] F-6875: pkcs11: zeroize NSC bounce buffers before freeing them The PKCS#11 non-secure-callable veneers deep-copy every NS attribute value and every mechanism parameter into secure-world heap. On key import (C_CreateObject/C_UnwrapKey/C_CopyObject/C_SetAttributeValue carrying CKA_VALUE or the RSA private components) and on password-based derivation (CKM_PKCS5_PBKD2 pPassword) those bounce buffers hold plaintext secrets, but nsc_tmpl_free() and nsc_mech_free() released them with a bare XFREE(), leaving the material in the freed secure heap block until something else happens to overwrite it. Scrub each block with wc_ForceZero() before releasing it. The template values use the prepare-time snapshot length, since wolfPKCS11 rewrites work[].ulValueLen on the C_GetAttributeValue path; nsc_alloc() now records the length of each mechanism allocation for the same reason. Adds unit-pkcs11-nsc-zeroize, which drives C_CreateObject_nsc_call and C_DeriveKey_nsc_call over a secure-heap stand-in that is never cleared, and fails if the imported key or the PBKDF2 password survives the free. --- src/pkcs11_callable.c | 24 +- tools/unit-tests/Makefile | 12 + tools/unit-tests/unit-pkcs11-nsc-zeroize.c | 269 +++++++++++++++++++++ 3 files changed, 301 insertions(+), 4 deletions(-) create mode 100644 tools/unit-tests/unit-pkcs11-nsc-zeroize.c diff --git a/src/pkcs11_callable.c b/src/pkcs11_callable.c index dd9868d6fc..323c929a5b 100644 --- a/src/pkcs11_callable.c +++ b/src/pkcs11_callable.c @@ -29,6 +29,7 @@ #include #include /* offsetof */ #include /* XMALLOC/XFREE/XMEMCPY, DYNAMIC_TYPE_* */ +#include /* wc_ForceZero */ /* * TrustZone-M PKCS#11 non-secure-callable (NSC) layer with pointer @@ -130,6 +131,7 @@ static int ns_outlen_begin(const volatile void *pBuf, CK_ULONG_PTR pulLen, struct nsc_mech { CK_MECHANISM mech; /* secure mechanism passed to wolfPKCS11 */ void *alloc[NSC_MECH_MAX_ALLOC]; + CK_ULONG allocLen[NSC_MECH_MAX_ALLOC]; int nAlloc; struct { void *dst; /* NS destination */ @@ -148,8 +150,10 @@ static void *nsc_alloc(struct nsc_mech *m, CK_ULONG len) if (m->nAlloc >= NSC_MECH_MAX_ALLOC) return NULL; p = XMALLOC((size_t)len, NULL, DYNAMIC_TYPE_TMP_BUFFER); - if (p != NULL) + if (p != NULL) { + m->allocLen[m->nAlloc] = len; m->alloc[m->nAlloc++] = p; + } return p; } @@ -205,13 +209,17 @@ static CK_RV nsc_inout(struct nsc_mech *m, CK_VOID_PTR dst, CK_ULONG len, return CKR_OK; } -/* Free all secure allocations without copying anything back (error path). */ +/* Free all secure allocations without copying anything back (error path). + * Parameter blobs can carry secrets (CKM_PKCS5_PBKD2 pPassword, HKDF salt, + * ...), so scrub every block before it goes back to the secure heap. */ static void nsc_mech_free(struct nsc_mech *m) { int i; - for (i = 0; i < m->nAlloc; i++) + for (i = 0; i < m->nAlloc; i++) { + wc_ForceZero(m->alloc[i], (size_t)m->allocLen[i]); XFREE(m->alloc[i], NULL, DYNAMIC_TYPE_TMP_BUFFER); + } m->nAlloc = 0; m->nCback = 0; } @@ -551,8 +559,16 @@ static void nsc_tmpl_free(struct nsc_tmpl *t) if (t->work != NULL) { for (i = 0; i < t->count; i++) { - if (t->work[i].pValue != NULL) + if (t->work[i].pValue != NULL) { + /* Value buffers hold imported key material (CKA_VALUE, the RSA + * private components, ...). Scrub before releasing, using the + * snapshot length: that is what was allocated, and wolfPKCS11 + * rewrites work[].ulValueLen on the C_GetAttributeValue path. */ + if (t->snap != NULL) + wc_ForceZero(t->work[i].pValue, + (size_t)t->snap[i].ulValueLen); XFREE(t->work[i].pValue, NULL, DYNAMIC_TYPE_TMP_BUFFER); + } } XFREE(t->work, NULL, DYNAMIC_TYPE_TMP_BUFFER); t->work = NULL; diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index 546e745dc4..f0756ea927 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -67,6 +67,7 @@ TESTS:=unit-parser unit-fdt unit-extflash unit-string unit-spi-flash unit-aes128 TESTS+=unit-tpm-check-rot-auth TESTS+=unit-tpm-api-names TESTS+=unit-tpm-nsc-cert +TESTS+=unit-pkcs11-nsc-zeroize TESTS+=unit-diagnostics TESTS+=unit-diagnostics-256 TESTS+=unit-fit-gzip unit-fit-nogzip @@ -289,6 +290,17 @@ unit-tpm-nsc-cert: ../../include/target.h unit-tpm-nsc-cert.c ../../src/string.c -DWOLFBOOT_HASH_SHA256 -D__ARM_FEATURE_CMSE=3U -DCSME_NSE_API= \ -ffunction-sections -fdata-sections $(LDFLAGS) -Wl,--gc-sections +# The PKCS#11 NSC veneers are exercised here through C_CreateObject_nsc_call +# and C_DeriveKey_nsc_call only; --gc-sections drops the remaining veneers so +# just those two wolfPKCS11 entry points need a stub. +unit-pkcs11-nsc-zeroize: ../../include/target.h unit-pkcs11-nsc-zeroize.c + gcc -o $@ unit-pkcs11-nsc-zeroize.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/memory.c \ + $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/misc.c \ + $(CFLAGS) -I$(WOLFBOOT_LIB_WOLFPKCS11) -DSECURE_PKCS11 \ + -DWOLFPKCS11_USER_SETTINGS -DWOLFCRYPT_SECURE_MODE \ + -ffunction-sections -fdata-sections $(LDFLAGS) -Wl,--gc-sections + unit-fwtpm-stub: ../../include/target.h unit-fwtpm-stub.c gcc -o $@ $^ $(CFLAGS) -I$(WOLFBOOT_LIB_WOLFTPM) \ -DWOLFTPM_USER_SETTINGS -ffunction-sections -fdata-sections \ diff --git a/tools/unit-tests/unit-pkcs11-nsc-zeroize.c b/tools/unit-tests/unit-pkcs11-nsc-zeroize.c new file mode 100644 index 0000000000..04f95b5413 --- /dev/null +++ b/tools/unit-tests/unit-pkcs11-nsc-zeroize.c @@ -0,0 +1,269 @@ +/* unit-pkcs11-nsc-zeroize.c + * + * Unit test for the PKCS#11 non-secure-callable bounce buffers. + * + * The NSC veneers deep-copy every non-secure CK_ATTRIBUTE value and every + * CK_MECHANISM parameter into secure-world heap before calling wolfPKCS11. + * For key import (C_CreateObject with CKA_VALUE, or the RSA private + * components) and for password-based derivation (CKM_PKCS5_PBKD2 pPassword) + * those bounce buffers hold plaintext secrets, so they must be scrubbed before + * being released back to the secure heap. + * + * 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 + +/* + * Secure-world heap stand-in: a bump allocator whose backing store is never + * reused or cleared, so whatever a veneer leaves behind on XFREE() is still + * observable afterwards. That is exactly what a released heap block looks like + * to the next allocation that recycles it, to a secure-world debugger, or to a + * cold-boot dump. + */ +static uint8_t sec_pool[4096]; +static size_t sec_pool_used; + +static void *sec_malloc(size_t n) +{ + void *p; + + if (n == 0) + n = 1; + n = (n + 7U) & ~(size_t)7U; /* keep allocations aligned */ + if (sec_pool_used + n > sizeof(sec_pool)) + return NULL; + p = &sec_pool[sec_pool_used]; + sec_pool_used += n; + return p; +} + +#define XMALLOC_OVERRIDE +#define XMALLOC(n, h, t) sec_malloc((size_t)(n)) +#define XFREE(p, h, t) do { (void)(p); } while (0) +#define XREALLOC(p, n, h, t) NULL + +#include "user_settings.h" +#include "wolfboot/wc_secure.h" +#include "wolfpkcs11/pkcs11.h" +#include "wolfboot/wcs_pkcs11.h" + +/* + * Simulated non-secure RAM. Only pointers fully inside this object pass the + * CMSE attribution stub, like real NS memory on a TrustZone-M part. + */ +static union { + uint8_t bytes[1024]; + CK_ATTRIBUTE tmpl[8]; + struct { + CK_MECHANISM mech; + CK_PKCS5_PBKD2_PARAMS2 params; + struct C_DeriveKey_nsc_args args; + CK_OBJECT_HANDLE hKey; + CK_UTF8CHAR password[16]; + } derive; +} ns_mem; + +void *cmse_check_address_range(void *ptr, size_t size, int flags) +{ + uint8_t *start = (uint8_t *)ptr; + + (void)flags; + if (start == NULL) + return NULL; + if (size == 0) + size = 1; + if (start < ns_mem.bytes || + start + size > ns_mem.bytes + sizeof(ns_mem.bytes)) + return NULL; + return ptr; +} + +/* The 32-byte AES key the non-secure client imports. */ +static const uint8_t secret_key[32] = { + 0x53, 0x45, 0x43, 0x52, 0x45, 0x54, 0x4b, 0x30, + 0x53, 0x45, 0x43, 0x52, 0x45, 0x54, 0x4b, 0x31, + 0x53, 0x45, 0x43, 0x52, 0x45, 0x54, 0x4b, 0x32, + 0x53, 0x45, 0x43, 0x52, 0x45, 0x54, 0x4b, 0x33 +}; + +/* The PBKDF2 password the non-secure client derives from. */ +static const uint8_t secret_pwd[16] = { + 0x50, 0x41, 0x53, 0x53, 0x77, 0x30, 0x72, 0x64, + 0x50, 0x41, 0x53, 0x53, 0x77, 0x30, 0x72, 0x65 +}; + +/* Set when the wolfPKCS11 stub actually saw the secret in the secure copy. */ +static int stub_saw_secret; + +CK_RV C_CreateObject(CK_SESSION_HANDLE hSession, CK_ATTRIBUTE_PTR pTemplate, + CK_ULONG ulCount, CK_OBJECT_HANDLE_PTR phObject) +{ + CK_ULONG i; + + (void)hSession; + for (i = 0; i < ulCount; i++) { + if (pTemplate[i].type == CKA_VALUE && + pTemplate[i].ulValueLen == sizeof(secret_key) && + memcmp(pTemplate[i].pValue, secret_key, + sizeof(secret_key)) == 0) { + stub_saw_secret = 1; + } + } + if (phObject != NULL) + *phObject = 1; + return CKR_OK; +} + +CK_RV C_DeriveKey(CK_SESSION_HANDLE hSession, CK_MECHANISM_PTR pMechanism, + CK_OBJECT_HANDLE hBaseKey, CK_ATTRIBUTE_PTR pTemplate, + CK_ULONG ulAttributeCount, CK_OBJECT_HANDLE_PTR phKey) +{ + CK_PKCS5_PBKD2_PARAMS2 *p; + + (void)hSession; + (void)hBaseKey; + (void)pTemplate; + (void)ulAttributeCount; + p = (CK_PKCS5_PBKD2_PARAMS2 *)pMechanism->pParameter; + if (p->ulPasswordLen == sizeof(secret_pwd) && + memcmp(p->pPassword, secret_pwd, sizeof(secret_pwd)) == 0) { + stub_saw_secret = 1; + } + if (phKey != NULL) + *phKey = 1; + return CKR_OK; +} + +#include "../../src/pkcs11_callable.c" + +static void reset_state(void) +{ + memset(sec_pool, 0, sizeof(sec_pool)); + sec_pool_used = 0; + stub_saw_secret = 0; + memset(&ns_mem, 0, sizeof(ns_mem)); +} + +/* Return 1 if the released secure heap still contains 'secret'. */ +static int pool_has(const uint8_t *secret, size_t len) +{ + size_t i; + + for (i = 0; i + len <= sec_pool_used; i++) { + if (memcmp(&sec_pool[i], secret, len) == 0) + return 1; + } + return 0; +} + +/* + * A non-secure client imports a symmetric key with C_CreateObject. The veneer + * bounce-buffers CKA_VALUE into the secure heap; once the call completes that + * buffer is freed and must no longer hold the key bytes. + */ +START_TEST(test_create_object_value_zeroized) +{ + CK_ATTRIBUTE *tmpl = ns_mem.tmpl; + uint8_t *nsKey = ns_mem.bytes + 3 * sizeof(CK_ATTRIBUTE); + CK_OBJECT_HANDLE *nsHandle; + CK_OBJECT_CLASS *nsClass; + CK_KEY_TYPE *nsType; + CK_RV rv; + + reset_state(); + memcpy(nsKey, secret_key, sizeof(secret_key)); + nsClass = (CK_OBJECT_CLASS *)(nsKey + sizeof(secret_key)); + nsType = (CK_KEY_TYPE *)(nsClass + 1); + nsHandle = (CK_OBJECT_HANDLE *)(nsType + 1); + *nsClass = CKO_SECRET_KEY; + *nsType = CKK_AES; + + tmpl[0].type = CKA_CLASS; + tmpl[0].pValue = nsClass; + tmpl[0].ulValueLen = sizeof(*nsClass); + tmpl[1].type = CKA_KEY_TYPE; + tmpl[1].pValue = nsType; + tmpl[1].ulValueLen = sizeof(*nsType); + tmpl[2].type = CKA_VALUE; + tmpl[2].pValue = nsKey; + tmpl[2].ulValueLen = sizeof(secret_key); + + rv = C_CreateObject_nsc_call(1, tmpl, 3, nsHandle); + ck_assert_int_eq((int)rv, (int)CKR_OK); + /* The secure copy really was made, so the pool did hold the key... */ + ck_assert_int_eq(stub_saw_secret, 1); + /* ...and it must not survive the free. */ + ck_assert_int_eq(pool_has(secret_key, sizeof(secret_key)), 0); +} +END_TEST + +/* + * Same for the mechanism parameter path: CKM_PKCS5_PBKD2 carries the caller's + * password, which nsc_mech_prepare() copies into its own secure buffer. + */ +START_TEST(test_mech_password_zeroized) +{ + CK_RV rv; + + reset_state(); + memcpy(ns_mem.derive.password, secret_pwd, sizeof(secret_pwd)); + ns_mem.derive.params.saltSource = CKZ_DATA_SPECIFIED; + ns_mem.derive.params.iterations = 1000; + ns_mem.derive.params.prf = CKP_PKCS5_PBKD2_HMAC_SHA256; + ns_mem.derive.params.pPassword = ns_mem.derive.password; + ns_mem.derive.params.ulPasswordLen = sizeof(secret_pwd); + ns_mem.derive.mech.mechanism = CKM_PKCS5_PBKD2; + ns_mem.derive.mech.pParameter = &ns_mem.derive.params; + ns_mem.derive.mech.ulParameterLen = sizeof(ns_mem.derive.params); + ns_mem.derive.args.hSession = 1; + ns_mem.derive.args.pMechanism = &ns_mem.derive.mech; + ns_mem.derive.args.phKey = &ns_mem.derive.hKey; + + rv = C_DeriveKey_nsc_call(&ns_mem.derive.args); + ck_assert_int_eq((int)rv, (int)CKR_OK); + ck_assert_int_eq(stub_saw_secret, 1); + ck_assert_int_eq(pool_has(secret_pwd, sizeof(secret_pwd)), 0); +} +END_TEST + +Suite *pkcs11_nsc_suite(void) +{ + Suite *s = suite_create("pkcs11-nsc-zeroize"); + TCase *tc = tcase_create("bounce-buffers"); + + tcase_add_test(tc, test_create_object_value_zeroized); + tcase_add_test(tc, test_mech_password_zeroized); + suite_add_tcase(s, tc); + return s; +} + +int main(void) +{ + int fails; + SRunner *sr = srunner_create(pkcs11_nsc_suite()); + + srunner_run_all(sr, CK_NORMAL); + fails = srunner_ntests_failed(sr); + srunner_free(sr); + return (fails == 0) ? 0 : 1; +}