From ada5acad138cc4cd41039947fd9bb6b2a4a1bf7b Mon Sep 17 00:00:00 2001 From: Aidan Garske Date: Tue, 6 Oct 2026 09:57:04 -0700 Subject: [PATCH 01/13] F-4262 - Reject negative stored array lengths when loading token objects --- src/internal.c | 6 + tests/include.am | 7 + tests/object_store_decode_test.c | 436 +++++++++++++++++++++++++++++++ 3 files changed, 449 insertions(+) create mode 100644 tests/object_store_decode_test.c diff --git a/src/internal.c b/src/internal.c index 7c61a3f6..db6e1ad0 100644 --- a/src/internal.c +++ b/src/internal.c @@ -2462,6 +2462,7 @@ static int wp11_storage_write_array(void* storage, * @param [in] len Length of data to read. * @return 0 on success. * @return BUFFER_E to indicate failure. + * @return ASN_PARSE_E when the stored length is negative. * @return MEMORY_E when dynamic memory allocation fails. */ static int wp11_storage_read_alloc_array(void* storage, @@ -2471,6 +2472,11 @@ static int wp11_storage_read_alloc_array(void* storage, /* Read length of array. */ ret = wp11_storage_read_int(storage, len); + if (ret == 0 && *len < 0) { + *len = 0; + /* Not BUFFER_E, which callers accept as an older record's end. */ + ret = ASN_PARSE_E; + } if (ret == 0 && *len > 0) { /* Allocate buffer to hold data. */ *buffer = (unsigned char*)XMALLOC(*len, NULL, DYNAMIC_TYPE_TMP_BUFFER); diff --git a/tests/include.am b/tests/include.am index 584136e7..ed95d9e3 100644 --- a/tests/include.am +++ b/tests/include.am @@ -320,6 +320,11 @@ noinst_PROGRAMS += tests/login_pin_state_test tests_login_pin_state_test_SOURCES = tests/login_pin_state_test.c tests_login_pin_state_test_LDADD = +check_PROGRAMS += tests/object_store_decode_test +noinst_PROGRAMS += tests/object_store_decode_test +tests_object_store_decode_test_SOURCES = tests/object_store_decode_test.c +tests_object_store_decode_test_LDADD = + if BUILD_STATIC tests_pkcs11test_LDADD += src/libwolfpkcs11.la tests_pkcs11mtt_LDADD += src/libwolfpkcs11.la @@ -385,6 +390,7 @@ tests_set_pin_rekey_test_LDADD += src/libwolfpkcs11.la tests_nss_configdir_prefix_test_LDADD += src/libwolfpkcs11.la tests_logout_retire_test_LDADD += src/libwolfpkcs11.la tests_login_pin_state_test_LDADD += src/libwolfpkcs11.la +tests_object_store_decode_test_LDADD += src/libwolfpkcs11.la else tests_object_id_uniqueness_test_LDADD += src/libwolfpkcs11.la tests_empty_pin_store_test_LDADD += src/libwolfpkcs11.la @@ -442,6 +448,7 @@ tests_set_pin_rekey_test_LDADD += src/libwolfpkcs11.la tests_nss_configdir_prefix_test_LDADD += src/libwolfpkcs11.la tests_logout_retire_test_LDADD += src/libwolfpkcs11.la tests_login_pin_state_test_LDADD += src/libwolfpkcs11.la +tests_object_store_decode_test_LDADD += src/libwolfpkcs11.la endif EXTRA_DIST += tests/unit.h \ diff --git a/tests/object_store_decode_test.c b/tests/object_store_decode_test.c new file mode 100644 index 00000000..b240c46f --- /dev/null +++ b/tests/object_store_decode_test.c @@ -0,0 +1,436 @@ +/* object_store_decode_test.c + * + * Copyright (C) 2026 wolfSSL Inc. + * + * This file is part of wolfPKCS11. + * + * wolfPKCS11 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. + * + * wolfPKCS11 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 + * + * Stored token objects whose recorded lengths do not describe valid data must + * be rejected cleanly when the token is loaded or logged into. Each reload runs + * in a child process so a failure is reported instead of ending the test. + */ + +#ifdef HAVE_CONFIG_H + #include +#endif + +#include +#include +#include + +#ifndef WOLFSSL_USER_SETTINGS + #include +#endif +#include +#include + +#ifndef WOLFPKCS11_USER_SETTINGS + #include +#endif +#include + +#ifndef HAVE_PKCS11_STATIC + #include +#endif + +#include "testdata.h" +#include "pkcs11_test_util.h" + +#if !defined(WOLFPKCS11_NO_STORE) && !defined(WOLFPKCS11_TPM_STORE) && \ + !defined(WOLFPKCS11_CUSTOM_STORE) && !defined(WOLFPKCS11_NO_ENV) && \ + !defined(_WIN32) + +#include +#include +#include + +#define TEST_DIR "./store/object_store_decode_test" +#define MAX_OBJ_ID 8 +#define MAX_FILE_SZ 16384 + +/* Offset of the first variable-length attribute in a stored object record. */ +#define OBJ_ID_LEN_OFF (12 + 3 * sizeof(CK_ULONG) + 1 + 1 + 4 + 4 + 8 + 8) +#define OBJ_LABEL_LEN_OFF (OBJ_ID_LEN_OFF + 4) +#define OBJ_ISSUER_LEN_OFF (OBJ_LABEL_LEN_OFF + 4) + +#define CHILD_PASS 0 +#define CHILD_FAIL 1 +#define CHILD_SETUP 2 + +static byte soPin[] = "password123456"; +static byte userPin[] = "wolfpkcs11-test"; +static CK_BBOOL ckTrue = CK_TRUE; +static CK_BBOOL ckFalse = CK_FALSE; + +static const char* storeKinds[] = { + "obj", "data", "symmkey", "rsakey_priv", "rsakey_pub", "ecckey_priv", + "ecckey_pub", "dhkey_priv", "dhkey_pub", "cert", "trust" +}; + +static void store_path(char* path, size_t sz, const char* kind, int objId) +{ + if (objId < 0) { + (void)snprintf(path, sz, "%s/wp11_%s_%016lx", TEST_DIR, kind, 1UL); + } + else { + (void)snprintf(path, sz, "%s/wp11_%s_%016lx_%016lx", TEST_DIR, kind, + 1UL, (unsigned long)objId); + } +} + +static void cleanup_test_files(void) +{ + char path[256]; + size_t k; + int i; + + for (k = 0; k < sizeof(storeKinds) / sizeof(*storeKinds); k++) { + for (i = 0; i < MAX_OBJ_ID; i++) { + store_path(path, sizeof(path), storeKinds[k], i); + (void)remove(path); + } + } + store_path(path, sizeof(path), "token", -1); + (void)remove(path); +} + +/* Locate the stored file of the given kind for the single object of that kind. */ +static int find_store_file(const char* kind, char* path, size_t sz) +{ + FILE* f; + int i; + + for (i = 0; i < MAX_OBJ_ID; i++) { + store_path(path, sz, kind, i); + f = fopen(path, "rb"); + if (f != NULL) { + fclose(f); + return 0; + } + } + return -1; +} + +static int read_file(const char* path, byte* data, size_t sz, size_t* readSz) +{ + FILE* f = fopen(path, "rb"); + + if (f == NULL) + return -1; + *readSz = fread(data, 1, sz, f); + if (ferror(f) || !feof(f)) { + fclose(f); + return -1; + } + fclose(f); + return 0; +} + +static int write_file(const char* path, const byte* data, size_t sz) +{ + FILE* f = fopen(path, "wb"); + size_t written; + + if (f == NULL) + return -1; + written = fwrite(data, 1, sz, f); + if (fclose(f) != 0 || written != sz) + return -1; + return 0; +} + +static word32 get_be32(const byte* p) +{ + return ((word32)p[0] << 24) | ((word32)p[1] << 16) | + ((word32)p[2] << 8) | (word32)p[3]; +} + +static void put_be32(byte* p, word32 v) +{ + p[0] = (byte)(v >> 24); + p[1] = (byte)(v >> 16); + p[2] = (byte)(v >> 8); + p[3] = (byte)v; +} + +/* Overwrite a 4-byte length field that is expected to hold 'expect'. */ +static int patch_length(const char* path, size_t off, word32 expect, + word32 value) +{ + static byte data[MAX_FILE_SZ]; + size_t sz = 0; + + if (read_file(path, data, sizeof(data), &sz) != 0 || sz < off + 4) + return -1; + if (get_be32(data + off) != expect) + return -1; + put_be32(data + off, value); + return write_file(path, data, sz); +} + +static CK_RV init_slot(CK_SLOT_ID* slot) +{ + CK_RV rv; + CK_C_INITIALIZE_ARGS args; + CK_SLOT_ID slots[16]; + CK_ULONG count = sizeof(slots) / sizeof(*slots); + + XMEMSET(&args, 0, sizeof(args)); + args.flags = CKF_OS_LOCKING_OK; + rv = funcList->C_Initialize(&args); + if (rv == CKR_OK) + rv = funcList->C_GetSlotList(CK_TRUE, slots, &count); + if (rv == CKR_OK && count == 0) + rv = CKR_TOKEN_NOT_PRESENT; + if (rv == CKR_OK) + *slot = slots[0]; + return rv; +} + +static CK_RV provision_token(CK_SLOT_ID slot) +{ + CK_RV rv; + CK_SESSION_HANDLE session = CK_INVALID_HANDLE; + CK_UTF8CHAR label[32]; + CK_FLAGS flags = CKF_SERIAL_SESSION | CKF_RW_SESSION; + + XMEMSET(label, ' ', sizeof(label)); + XMEMCPY(label, "wolfpkcs11", 10); + rv = funcList->C_InitToken(slot, soPin, sizeof(soPin) - 1, label); + if (rv == CKR_OK) + rv = funcList->C_OpenSession(slot, flags, NULL, NULL, &session); + if (rv == CKR_OK) + rv = funcList->C_Login(session, CKU_SO, soPin, sizeof(soPin) - 1); + if (rv == CKR_OK) + rv = funcList->C_InitPIN(session, userPin, sizeof(userPin) - 1); + if (session != CK_INVALID_HANDLE) { + (void)funcList->C_Logout(session); + (void)funcList->C_CloseSession(session); + } + return rv; +} + +static CK_RV open_session(CK_SLOT_ID slot, int login, + CK_SESSION_HANDLE* session) +{ + CK_RV rv; + CK_FLAGS flags = CKF_SERIAL_SESSION | CKF_RW_SESSION; + + rv = funcList->C_OpenSession(slot, flags, NULL, NULL, session); + if (rv == CKR_OK && login) { + rv = funcList->C_Login(*session, CKU_USER, userPin, + sizeof(userPin) - 1); + } + return rv; +} + +typedef CK_RV (*create_fn)(CK_SESSION_HANDLE session); + +/* Provision a fresh token, create objects with 'create' and persist them. */ +static CK_RV store_objects(create_fn create) +{ + CK_RV rv; + CK_SLOT_ID slot = 0; + CK_SESSION_HANDLE session = CK_INVALID_HANDLE; + + cleanup_test_files(); + rv = pkcs11_load(); + if (rv != CKR_OK) + return rv; + rv = init_slot(&slot); + if (rv == CKR_OK) + rv = provision_token(slot); + if (rv == CKR_OK) + rv = open_session(slot, 1, &session); + if (rv == CKR_OK) + rv = create(session); + if (session != CK_INVALID_HANDLE) + (void)funcList->C_CloseSession(session); + (void)funcList->C_Finalize(NULL); + pkcs11_unload(); + return rv; +} + +typedef int (*child_fn)(void); + +/* Run fn in a child process. Returns CHILD_PASS only when it exits normally + * reporting that the invariant held. */ +static int run_in_child(child_fn fn) +{ + pid_t pid; + int status = 0; + + fflush(stdout); + fflush(stderr); + pid = fork(); + if (pid < 0) + return CHILD_SETUP; + if (pid == 0) { + int res = fn(); + fflush(stdout); + fflush(stderr); + _exit(res); + } + if (waitpid(pid, &status, 0) != pid) + return CHILD_SETUP; + if (WIFSIGNALED(status)) { + fprintf(stderr, " reload terminated by signal %d\n", + WTERMSIG(status)); + return CHILD_FAIL; + } + if (!WIFEXITED(status)) + return CHILD_FAIL; + return WEXITSTATUS(status); +} + +static CK_RV create_data_object(CK_SESSION_HANDLE session) +{ + CK_OBJECT_CLASS dataClass = CKO_DATA; + CK_OBJECT_HANDLE obj = CK_INVALID_HANDLE; + static byte app[] = "wolfpkcs11"; + static byte value[] = "stored data object value"; + CK_ATTRIBUTE tmpl[] = { + { CKA_CLASS, &dataClass, sizeof(dataClass) }, + { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, + { CKA_PRIVATE, &ckFalse, sizeof(ckFalse) }, + { CKA_APPLICATION, app, sizeof(app) - 1 }, + { CKA_VALUE, value, sizeof(value) - 1 }, + }; + + return funcList->C_CreateObject(session, tmpl, + sizeof(tmpl) / sizeof(*tmpl), &obj); +} + +/* Either the token refuses to load or the data object reports a real label + * length. */ +static int child_data_label_length(void) +{ + CK_RV rv; + CK_SLOT_ID slot = 0; + CK_SESSION_HANDLE session = CK_INVALID_HANDLE; + CK_OBJECT_CLASS dataClass = CKO_DATA; + CK_OBJECT_HANDLE obj = CK_INVALID_HANDLE; + CK_ULONG count = 0; + CK_ATTRIBUTE findTmpl[] = { + { CKA_CLASS, &dataClass, sizeof(dataClass) }, + }; + CK_ATTRIBUTE label = { CKA_LABEL, NULL, 0 }; + int res = CHILD_SETUP; + + if (pkcs11_load() != CKR_OK) + return CHILD_SETUP; + rv = init_slot(&slot); + if (rv != CKR_OK) + return CHILD_PASS; + + rv = open_session(slot, 0, &session); + if (rv == CKR_OK) + rv = funcList->C_FindObjectsInit(session, findTmpl, 1); + if (rv == CKR_OK) + rv = funcList->C_FindObjects(session, &obj, 1, &count); + if (rv == CKR_OK) + rv = funcList->C_FindObjectsFinal(session); + if (rv == CKR_OK && count == 1) + rv = funcList->C_GetAttributeValue(session, obj, &label, 1); + if (rv == CKR_OK && count == 1) { + res = (label.ulValueLen == CK_UNAVAILABLE_INFORMATION || + label.ulValueLen > 64) ? CHILD_FAIL : CHILD_PASS; + if (res != CHILD_PASS) { + fprintf(stderr, " loaded label length 0x%lx\n", + (unsigned long)label.ulValueLen); + } + } + (void)funcList->C_Finalize(NULL); + return res; +} + +/* A negative length must not pass as the end of an older record format. */ +static int child_token_not_loaded(void) +{ + CK_SLOT_ID slot = 0; + int res; + + if (pkcs11_load() != CKR_OK) + return CHILD_SETUP; + res = (init_slot(&slot) != CKR_OK) ? CHILD_PASS : CHILD_FAIL; + (void)funcList->C_Finalize(NULL); + return res; +} + +static void test_negative_optional_length(void) +{ + CK_RV rv; + char path[256]; + int res = CHILD_SETUP; + + printf("\n--- stored issuer length must not be negative ---\n"); + rv = store_objects(create_data_object); + CHECK_RV(rv, "store data object", CKR_OK); + if (rv != CKR_OK) + return; + if (find_store_file("obj", path, sizeof(path)) == 0 && + patch_length(path, OBJ_ISSUER_LEN_OFF, 0, 0xFFFFFFFFUL) == 0) { + res = run_in_child(child_token_not_loaded); + } + CHECK_TRUE(res == CHILD_PASS, + "negative stored issuer length is not loaded"); + cleanup_test_files(); +} + +static void test_negative_attribute_length(void) +{ + CK_RV rv; + char path[256]; + int res = CHILD_SETUP; + + printf("\n--- stored attribute length must not be negative ---\n"); + rv = store_objects(create_data_object); + CHECK_RV(rv, "store data object", CKR_OK); + if (rv != CKR_OK) + return; + if (find_store_file("obj", path, sizeof(path)) == 0 && + patch_length(path, OBJ_LABEL_LEN_OFF, 0, 0xFFFFFFFFUL) == 0) { + res = run_in_child(child_data_label_length); + } + CHECK_TRUE(res == CHILD_PASS, + "negative stored attribute length is not loaded"); + cleanup_test_files(); +} + +int main(int argc, char* argv[]) +{ + (void)argc; + (void)argv; + XSETENV("WOLFPKCS11_TOKEN_PATH", TEST_DIR, 1); + + printf("=== wolfPKCS11 stored object decode test ===\n"); + test_negative_optional_length(); + test_negative_attribute_length(); + + return pkcs11_test_summary(); +} + +#else + +int main(int argc, char* argv[]) +{ + (void)argc; + (void)argv; + printf("File-backed token storage not available, skipping test\n"); + return 77; +} + +#endif From 8437bbfc1dff4c515979224f402085b3457005ac Mon Sep 17 00:00:00 2001 From: Aidan Garske Date: Tue, 6 Oct 2026 10:16:19 -0700 Subject: [PATCH 02/13] F-5078 - Grow stored array buffers only as data is read --- src/internal.c | 118 +++++++++++++++-- tests/object_store_decode_test.c | 219 +++++++++++++++++++++++++++++++ 2 files changed, 327 insertions(+), 10 deletions(-) diff --git a/src/internal.c b/src/internal.c index db6e1ad0..1461aedf 100644 --- a/src/internal.c +++ b/src/internal.c @@ -147,6 +147,11 @@ #define WP11_MAX_CERT_SZ 4096 #endif +/* Initial allocation when reading a variable-length array from storage. */ +#ifndef WP11_STORE_READ_CHUNK_SZ +#define WP11_STORE_READ_CHUNK_SZ 4096 +#endif + #define PKCS11_CHECK_VALUE_SIZE 3 /* Sizes for storage. */ @@ -1979,6 +1984,36 @@ int wolfPKCS11_Store_Read(void* store, unsigned char* buffer, int len) return ret; } +#ifndef WOLFPKCS11_TPM_STORE +/** + * Get the number of bytes left to read from a store file. + * + * @param [in] store Context for operation. + * @param [out] left Number of bytes after the read position. + * @return 0 on success. + * @return BUFFER_E when the position cannot be worked out or restored. + */ +static int wolfPKCS11_Store_Left(void* store, long* left) +{ + WP11_FileStoreCtx* ctx = (WP11_FileStoreCtx*)store; + long pos = -1; + long end = -1; + + /* stdio directly: the file store's handles are FILE* (see fdopen()). */ + if (ctx != NULL && ctx->file != XBADFILE && ctx->file != NULL) { + pos = ftell(ctx->file); + if (pos >= 0 && fseek(ctx->file, 0, SEEK_END) == 0) + end = ftell(ctx->file); + if (pos >= 0 && fseek(ctx->file, pos, SEEK_SET) != 0) + end = -1; + } + if (pos < 0 || end < pos) + return BUFFER_E; + *left = end - pos; + return 0; +} +#endif + /** * Writes a specific number of bytes from buffer. * @@ -2080,6 +2115,26 @@ static void wp11_storage_close(void* storage) wolfPKCS11_Store_Close(storage); } +/** + * Get the number of bytes left to read, when the store can tell. + * + * @param [in] storage Context for operation. + * @param [out] left Number of bytes left to read. + * @return 0 on success. + * @return NOT_AVAILABLE_E when the store cannot tell. + * @return Other value on failure. + */ +static int wp11_storage_left(void* storage, long* left) +{ +#if !defined(WOLFPKCS11_CUSTOM_STORE) && !defined(WOLFPKCS11_TPM_STORE) + return wolfPKCS11_Store_Left(storage, left); +#else + (void)storage; + (void)left; + return NOT_AVAILABLE_E; +#endif +} + /** * Reads a specific number of bytes into buffer. * @@ -2469,6 +2524,13 @@ static int wp11_storage_read_alloc_array(void* storage, unsigned char** buffer, int* len) { int ret; + int have = 0; + int bufSz = 0; + int newSz; + int first = WP11_STORE_READ_CHUNK_SZ; + long left = 0; + unsigned char* buf = NULL; + unsigned char* tmp; /* Read length of array. */ ret = wp11_storage_read_int(storage, len); @@ -2478,21 +2540,57 @@ static int wp11_storage_read_alloc_array(void* storage, ret = ASN_PARSE_E; } if (ret == 0 && *len > 0) { - /* Allocate buffer to hold data. */ - *buffer = (unsigned char*)XMALLOC(*len, NULL, DYNAMIC_TYPE_TMP_BUFFER); - if (*buffer == NULL) - ret = MEMORY_E; + /* When the store knows its size, allocate once if the data is there. */ + ret = wp11_storage_left(storage, &left); + if (ret == 0 && (long)*len > left) + ret = BUFFER_E; + else if (ret == 0) + first = *len; + else if (ret == NOT_AVAILABLE_E) + ret = 0; + } + /* Otherwise grow the buffer only as stored data is read, so a corrupt + * length cannot drive an allocation larger than the data present. */ + while (ret == 0 && have < *len) { + if (bufSz == 0) + newSz = first; + else if (bufSz <= *len / 2) + newSz = bufSz * 2; + else + newSz = *len; + if (newSz > *len) + newSz = *len; - if (ret == 0) { - /* Read array data into allocated buffer. */ - ret = wp11_storage_read(storage, *buffer, *len); - if (ret != 0) { - XFREE(*buffer, NULL, DYNAMIC_TYPE_TMP_BUFFER); - *buffer = NULL; + tmp = (unsigned char*)XMALLOC(newSz, NULL, DYNAMIC_TYPE_TMP_BUFFER); + if (tmp == NULL) { + ret = MEMORY_E; + } + else { + if (buf != NULL) { + XMEMCPY(tmp, buf, have); + wc_ForceZero(buf, (word32)have); + XFREE(buf, NULL, DYNAMIC_TYPE_TMP_BUFFER); } + buf = tmp; + bufSz = newSz; + /* Read array data into the newly allocated space. */ + ret = wp11_storage_read(storage, buf + have, bufSz - have); + if (ret == 0) + have = bufSz; } } + if (ret != 0 && (buf != NULL || ret == MEMORY_E)) { + if (buf != NULL) { + wc_ForceZero(buf, (word32)bufSz); + XFREE(buf, NULL, DYNAMIC_TYPE_TMP_BUFFER); + } + *buffer = NULL; + } + else if (buf != NULL) { + *buffer = buf; + } + return ret; } diff --git a/tests/object_store_decode_test.c b/tests/object_store_decode_test.c index b240c46f..e26332f6 100644 --- a/tests/object_store_decode_test.c +++ b/tests/object_store_decode_test.c @@ -36,6 +36,7 @@ #endif #include #include +#include #ifndef WOLFPKCS11_USER_SETTINGS #include @@ -80,6 +81,74 @@ static const char* storeKinds[] = { "ecckey_pub", "dhkey_priv", "dhkey_pub", "cert", "trust" }; +#if defined(USE_WOLFSSL_MEMORY) && !defined(WOLFSSL_STATIC_MEMORY) && \ + !defined(WOLFSSL_DEBUG_MEMORY) +#define HAVE_ALLOC_TRACKING +#define ALLOC_REFUSE_SZ ((size_t)64 * 1024 * 1024) + +#define TRACK_HDR_SZ 16 + +static size_t largestAlloc = 0; +/* Live bytes in blocks no larger than peakCap, and their peak. */ +static size_t peakCap = 0; +static size_t liveBytes = 0; +static size_t peakLive = 0; + +static void* track_malloc(size_t sz) +{ + unsigned char* p; + + if (sz > largestAlloc) + largestAlloc = sz; + if (sz >= ALLOC_REFUSE_SZ) + return NULL; + p = (unsigned char*)malloc(sz + TRACK_HDR_SZ); + if (p == NULL) + return NULL; + XMEMCPY(p, &sz, sizeof(sz)); + if (sz <= peakCap) { + liveBytes += sz; + if (liveBytes > peakLive) + peakLive = liveBytes; + } + return p + TRACK_HDR_SZ; +} + +static void track_free(void* ptr) +{ + unsigned char* p; + size_t sz; + + if (ptr == NULL) + return; + p = (unsigned char*)ptr - TRACK_HDR_SZ; + XMEMCPY(&sz, p, sizeof(sz)); + if (sz <= peakCap) + liveBytes -= sz; + free(p); +} + +static void* track_realloc(void* ptr, size_t sz) +{ + void* n; + size_t old; + + if (ptr == NULL) + return track_malloc(sz); + if (sz == 0) { + track_free(ptr); + return NULL; + } + n = track_malloc(sz); + if (n == NULL) + return NULL; + XMEMCPY(&old, (unsigned char*)ptr - TRACK_HDR_SZ, sizeof(old)); + XMEMCPY(n, ptr, (old < sz) ? old : sz); + track_free(ptr); + return n; +} +#endif + static void store_path(char* path, size_t sz, const char* kind, int objId) { if (objId < 0) { @@ -410,6 +479,149 @@ static void test_negative_attribute_length(void) cleanup_test_files(); } +#define LARGE_VALUE_SZ 300001 +static byte largeValue[LARGE_VALUE_SZ]; +static byte largeRead[LARGE_VALUE_SZ]; + +static CK_RV create_large_data_object(CK_SESSION_HANDLE session) +{ + CK_OBJECT_CLASS dataClass = CKO_DATA; + CK_OBJECT_HANDLE obj = CK_INVALID_HANDLE; + CK_ATTRIBUTE tmpl[] = { + { CKA_CLASS, &dataClass, sizeof(dataClass) }, + { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, + { CKA_PRIVATE, &ckFalse, sizeof(ckFalse) }, + { CKA_VALUE, largeValue, sizeof(largeValue) }, + }; + size_t i; + + for (i = 0; i < sizeof(largeValue); i++) + largeValue[i] = (byte)(i * 7 + (i >> 8)); + return funcList->C_CreateObject(session, tmpl, + sizeof(tmpl) / sizeof(*tmpl), &obj); +} + +static void test_large_stored_value_round_trip(void) +{ + CK_RV rv; + CK_SLOT_ID slot = 0; + CK_SESSION_HANDLE session = CK_INVALID_HANDLE; + CK_OBJECT_CLASS dataClass = CKO_DATA; + CK_OBJECT_HANDLE obj = CK_INVALID_HANDLE; + CK_ULONG count = 0; + CK_ATTRIBUTE findTmpl[] = { + { CKA_CLASS, &dataClass, sizeof(dataClass) }, + }; + CK_ATTRIBUTE value = { CKA_VALUE, largeRead, sizeof(largeRead) }; + + printf("\n--- large stored value reloads intact ---\n"); + rv = store_objects(create_large_data_object); + CHECK_RV(rv, "store large data object", CKR_OK); + if (rv != CKR_OK) + return; + rv = pkcs11_load(); + if (rv == CKR_OK) { + rv = init_slot(&slot); + if (rv == CKR_OK) + rv = open_session(slot, 0, &session); + if (rv == CKR_OK) + rv = funcList->C_FindObjectsInit(session, findTmpl, 1); + if (rv == CKR_OK) + rv = funcList->C_FindObjects(session, &obj, 1, &count); + if (rv == CKR_OK) + rv = funcList->C_FindObjectsFinal(session); + if (rv == CKR_OK && count != 1) + rv = CKR_GENERAL_ERROR; + if (rv == CKR_OK) + rv = funcList->C_GetAttributeValue(session, obj, &value, 1); + (void)funcList->C_Finalize(NULL); + pkcs11_unload(); + } + CHECK_RV(rv, "reload large data object", CKR_OK); + CHECK_TRUE(rv == CKR_OK && value.ulValueLen == sizeof(largeValue) && + XMEMCMP(largeRead, largeValue, sizeof(largeValue)) == 0, + "large stored value is unchanged after reload"); + cleanup_test_files(); +} + +#ifdef HAVE_ALLOC_TRACKING +/* Loading must not size an allocation from a length the data cannot back. */ +static int child_load_allocation_bound(void) +{ + CK_SLOT_ID slot = 0; + + if (wolfSSL_SetAllocators(track_malloc, track_free, track_realloc) != 0) + return CHILD_SETUP; + if (pkcs11_load() != CKR_OK) + return CHILD_SETUP; + if (init_slot(&slot) == CKR_OK) + (void)funcList->C_Finalize(NULL); + if (largestAlloc >= ALLOC_REFUSE_SZ) { + fprintf(stderr, " largest allocation while loading: %lu\n", + (unsigned long)largestAlloc); + return CHILD_FAIL; + } + return CHILD_PASS; +} + +static void test_oversized_attribute_length(void) +{ + CK_RV rv; + char path[256]; + int res = CHILD_SETUP; + + printf("\n--- stored attribute length must be bounded ---\n"); + rv = store_objects(create_data_object); + CHECK_RV(rv, "store data object", CKR_OK); + if (rv != CKR_OK) + return; + if (find_store_file("obj", path, sizeof(path)) == 0 && + patch_length(path, OBJ_LABEL_LEN_OFF, 0, 0x7FFFFFF8UL) == 0) { + res = run_in_child(child_load_allocation_bound); + } + CHECK_TRUE(res == CHILD_PASS, + "oversized stored attribute length is not allocated"); + cleanup_test_files(); +} + +/* Loading a large stored value holds about one copy of it, not two. Larger + * blocks cannot be partial copies of it, so they are not counted. */ +static int child_large_value_peak(void) +{ + CK_SLOT_ID slot = 0; + + peakCap = LARGE_VALUE_SZ; + if (wolfSSL_SetAllocators(track_malloc, track_free, track_realloc) != 0) + return CHILD_SETUP; + if (pkcs11_load() != CKR_OK) + return CHILD_SETUP; + if (init_slot(&slot) == CKR_OK) + (void)funcList->C_Finalize(NULL); + if (peakLive >= (size_t)LARGE_VALUE_SZ + LARGE_VALUE_SZ / 2) { + fprintf(stderr, " peak live bytes while loading: %lu\n", + (unsigned long)peakLive); + return CHILD_FAIL; + } + return CHILD_PASS; +} + +static void test_large_stored_value_peak(void) +{ + CK_RV rv; + int res; + + printf("\n--- large stored value loads without a second copy ---\n"); + rv = store_objects(create_large_data_object); + CHECK_RV(rv, "store large data object", CKR_OK); + if (rv != CKR_OK) + return; + res = run_in_child(child_large_value_peak); + CHECK_TRUE(res == CHILD_PASS, + "peak memory while loading stays near one copy"); + cleanup_test_files(); +} +#endif + int main(int argc, char* argv[]) { (void)argc; @@ -419,6 +631,13 @@ int main(int argc, char* argv[]) printf("=== wolfPKCS11 stored object decode test ===\n"); test_negative_optional_length(); test_negative_attribute_length(); +#ifdef HAVE_ALLOC_TRACKING + test_oversized_attribute_length(); +#endif + test_large_stored_value_round_trip(); +#ifdef HAVE_ALLOC_TRACKING + test_large_stored_value_peak(); +#endif return pkcs11_test_summary(); } From 8427035a6461d3fd3fb60d289848a67873c6b5bf Mon Sep 17 00:00:00 2001 From: Aidan Garske Date: Tue, 6 Oct 2026 10:25:41 -0700 Subject: [PATCH 03/13] F-13814 - Set data object lengths only after a successful storage read --- src/internal.c | 11 +++-- tests/object_store_decode_test.c | 75 +++++++++++++++++++++++++++++++- 2 files changed, 80 insertions(+), 6 deletions(-) diff --git a/src/internal.c b/src/internal.c index 1461aedf..3a1a1e04 100644 --- a/src/internal.c +++ b/src/internal.c @@ -3596,7 +3596,7 @@ static int wp11_Object_Load_Data(WP11_Object* object, int tokenId, int objId) { int ret; void* storage = NULL; - int tempLen; + int tempLen = 0; /* Open access to data. */ ret = wp11_storage_open_readonly(WOLFPKCS11_STORE_DATA, tokenId, objId, @@ -3605,7 +3605,8 @@ static int wp11_Object_Load_Data(WP11_Object* object, int tokenId, int objId) ret = wp11_storage_read_alloc_array(storage, &object->data.genericData.data, &tempLen); - object->data.genericData.dataLen = (word32)tempLen; + if (ret == 0) + object->data.genericData.dataLen = (word32)tempLen; } /* Read application length and application. */ @@ -3613,7 +3614,8 @@ static int wp11_Object_Load_Data(WP11_Object* object, int tokenId, int objId) ret = wp11_storage_read_alloc_array(storage, &object->data.genericData.application, &tempLen); - object->data.genericData.applicationLen = (word32)tempLen; + if (ret == 0) + object->data.genericData.applicationLen = (word32)tempLen; } /* Read object ID length and object ID. */ @@ -3621,7 +3623,8 @@ static int wp11_Object_Load_Data(WP11_Object* object, int tokenId, int objId) ret = wp11_storage_read_alloc_array(storage, &object->data.genericData.objectId, &tempLen); - object->data.genericData.objectIdLen = (word32)tempLen; + if (ret == 0) + object->data.genericData.objectIdLen = (word32)tempLen; } wp11_storage_close(storage); diff --git a/tests/object_store_decode_test.c b/tests/object_store_decode_test.c index e26332f6..d90771cc 100644 --- a/tests/object_store_decode_test.c +++ b/tests/object_store_decode_test.c @@ -75,6 +75,7 @@ static byte soPin[] = "password123456"; static byte userPin[] = "wolfpkcs11-test"; static CK_BBOOL ckTrue = CK_TRUE; static CK_BBOOL ckFalse = CK_FALSE; +static byte dataValue[] = "stored data object value"; static const char* storeKinds[] = { "obj", "data", "symmkey", "rsakey_priv", "rsakey_pub", "ecckey_priv", @@ -370,13 +371,12 @@ static CK_RV create_data_object(CK_SESSION_HANDLE session) CK_OBJECT_CLASS dataClass = CKO_DATA; CK_OBJECT_HANDLE obj = CK_INVALID_HANDLE; static byte app[] = "wolfpkcs11"; - static byte value[] = "stored data object value"; CK_ATTRIBUTE tmpl[] = { { CKA_CLASS, &dataClass, sizeof(dataClass) }, { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, { CKA_PRIVATE, &ckFalse, sizeof(ckFalse) }, { CKA_APPLICATION, app, sizeof(app) - 1 }, - { CKA_VALUE, value, sizeof(value) - 1 }, + { CKA_VALUE, dataValue, sizeof(dataValue) - 1 }, }; return funcList->C_CreateObject(session, tmpl, @@ -479,6 +479,76 @@ static void test_negative_attribute_length(void) cleanup_test_files(); } +static int truncate_file(const char* path, size_t keep) +{ + static byte data[MAX_FILE_SZ]; + size_t sz = 0; + + if (read_file(path, data, sizeof(data), &sz) != 0 || sz <= keep) + return -1; + return write_file(path, data, keep); +} + +/* Either the token refuses to load or the data object reads back its full + * stored value. */ +static int child_data_value_intact(void) +{ + CK_RV rv; + CK_SLOT_ID slot = 0; + CK_SESSION_HANDLE session = CK_INVALID_HANDLE; + CK_OBJECT_CLASS dataClass = CKO_DATA; + CK_OBJECT_HANDLE obj = CK_INVALID_HANDLE; + CK_ULONG count = 0; + CK_ATTRIBUTE findTmpl[] = { + { CKA_CLASS, &dataClass, sizeof(dataClass) }, + }; + byte value[64]; + CK_ATTRIBUTE attr = { CKA_VALUE, value, sizeof(value) }; + int res = CHILD_FAIL; + + if (pkcs11_load() != CKR_OK) + return CHILD_SETUP; + rv = init_slot(&slot); + if (rv != CKR_OK) + return CHILD_PASS; + + rv = open_session(slot, 0, &session); + if (rv == CKR_OK) + rv = funcList->C_FindObjectsInit(session, findTmpl, 1); + if (rv == CKR_OK) + rv = funcList->C_FindObjects(session, &obj, 1, &count); + if (rv == CKR_OK) + rv = funcList->C_FindObjectsFinal(session); + if (rv == CKR_OK && count == 1) + rv = funcList->C_GetAttributeValue(session, obj, &attr, 1); + if (rv == CKR_OK && count == 1 && + attr.ulValueLen == sizeof(dataValue) - 1 && + XMEMCMP(value, dataValue, sizeof(dataValue) - 1) == 0) { + res = CHILD_PASS; + } + (void)funcList->C_Finalize(NULL); + return res; +} + +static void test_truncated_data_record(void) +{ + CK_RV rv; + char path[256]; + int res = CHILD_SETUP; + + printf("\n--- truncated data object record is not loaded ---\n"); + rv = store_objects(create_data_object); + CHECK_RV(rv, "store data object", CKR_OK); + if (rv != CKR_OK) + return; + if (find_store_file("data", path, sizeof(path)) == 0 && + truncate_file(path, 2) == 0) { + res = run_in_child(child_data_value_intact); + } + CHECK_TRUE(res == CHILD_PASS, "truncated data object record is rejected"); + cleanup_test_files(); +} + #define LARGE_VALUE_SZ 300001 static byte largeValue[LARGE_VALUE_SZ]; static byte largeRead[LARGE_VALUE_SZ]; @@ -638,6 +708,7 @@ int main(int argc, char* argv[]) #ifdef HAVE_ALLOC_TRACKING test_large_stored_value_peak(); #endif + test_truncated_data_record(); return pkcs11_test_summary(); } From 651c6c8bb39e87769b8edecb1d48ec5bc618477f Mon Sep 17 00:00:00 2001 From: Aidan Garske Date: Tue, 6 Oct 2026 10:45:11 -0700 Subject: [PATCH 04/13] F-4264 - Reject stored private key records shorter than the tag --- src/internal.c | 44 ++++--- tests/object_store_decode_test.c | 189 ++++++++++++++++++++++++++++++- 2 files changed, 218 insertions(+), 15 deletions(-) diff --git a/src/internal.c b/src/internal.c index 3a1a1e04..77960a7b 100644 --- a/src/internal.c +++ b/src/internal.c @@ -4230,11 +4230,14 @@ static int wp11_Object_Decode_RsaKey(WP11_Object* object) else #endif if (object->objClass == CKO_PRIVATE_KEY) { - unsigned char* der; + unsigned char* der = NULL; int len = object->keyDataLen - AES_BLOCK_SIZE; - der = (unsigned char*)XMALLOC(len, NULL, DYNAMIC_TYPE_TMP_BUFFER); - if (der == NULL) { + if (object->keyDataLen <= AES_BLOCK_SIZE) + ret = BAD_FUNC_ARG; + if (ret == 0) + der = (unsigned char*)XMALLOC(len, NULL, DYNAMIC_TYPE_TMP_BUFFER); + if (ret == 0 && der == NULL) { ret = MEMORY_E; } if (ret == 0) { @@ -4578,11 +4581,14 @@ static int wp11_Object_Decode_EccKey(WP11_Object* object) else #endif if (object->objClass == CKO_PRIVATE_KEY) { - unsigned char* der; + unsigned char* der = NULL; int len = object->keyDataLen - AES_BLOCK_SIZE; - der = (unsigned char*)XMALLOC(len, NULL, DYNAMIC_TYPE_TMP_BUFFER); - if (der == NULL) { + if (object->keyDataLen <= AES_BLOCK_SIZE) + ret = BAD_FUNC_ARG; + if (ret == 0) + der = (unsigned char*)XMALLOC(len, NULL, DYNAMIC_TYPE_TMP_BUFFER); + if (ret == 0 && der == NULL) { ret = MEMORY_E; } if (ret == 0) { @@ -4818,11 +4824,14 @@ static int wp11_Object_Decode_MldsaKey(WP11_Object* object) int ret = 0; if (object->objClass == CKO_PRIVATE_KEY) { - unsigned char* der; + unsigned char* der = NULL; int len = object->keyDataLen - AES_BLOCK_SIZE; - der = (unsigned char*)XMALLOC(len, NULL, DYNAMIC_TYPE_TMP_BUFFER); - if (der == NULL) { + if (object->keyDataLen <= AES_BLOCK_SIZE) + ret = BAD_FUNC_ARG; + if (ret == 0) + der = (unsigned char*)XMALLOC(len, NULL, DYNAMIC_TYPE_TMP_BUFFER); + if (ret == 0 && der == NULL) { ret = MEMORY_E; } if (ret == 0) { @@ -5035,11 +5044,18 @@ static int wp11_Object_Decode_DhKey(WP11_Object* object) int ret = 0; if (object->objClass == CKO_PRIVATE_KEY) { - ret = wp11_DecryptData(object->data.dhKey->key, object->keyData, - object->keyDataLen - AES_BLOCK_SIZE, - object->slot->token.key, - sizeof(object->slot->token.key), object->iv, - sizeof(object->iv), object->devId); + if (object->keyDataLen <= AES_BLOCK_SIZE) + ret = BAD_FUNC_ARG; + else if (object->keyDataLen - AES_BLOCK_SIZE > WP11_MAX_DH_KEY_SZ) + ret = BUFFER_E; + if (ret == 0) { + ret = wp11_DecryptData(object->data.dhKey->key, object->keyData, + object->keyDataLen - AES_BLOCK_SIZE, + object->slot->token.key, + sizeof(object->slot->token.key), + object->iv, sizeof(object->iv), + object->devId); + } if (ret == 0) object->data.dhKey->len = object->keyDataLen - AES_BLOCK_SIZE; } diff --git a/tests/object_store_decode_test.c b/tests/object_store_decode_test.c index d90771cc..963e34b6 100644 --- a/tests/object_store_decode_test.c +++ b/tests/object_store_decode_test.c @@ -79,7 +79,8 @@ static byte dataValue[] = "stored data object value"; static const char* storeKinds[] = { "obj", "data", "symmkey", "rsakey_priv", "rsakey_pub", "ecckey_priv", - "ecckey_pub", "dhkey_priv", "dhkey_pub", "cert", "trust" + "ecckey_pub", "dhkey_priv", "dhkey_pub", "cert", "trust", "mldsakey_priv", + "mldsakey_pub" }; #if defined(USE_WOLFSSL_MEMORY) && !defined(WOLFSSL_STATIC_MEMORY) && \ @@ -549,6 +550,191 @@ static void test_truncated_data_record(void) cleanup_test_files(); } +/* Replace the first length-prefixed array in a stored key record with newLen + * bytes, keeping the rest of the record unchanged. */ +static int rewrite_first_array(const char* path, word32 newLen) +{ + static byte data[MAX_FILE_SZ]; + static byte out[MAX_FILE_SZ]; + size_t sz = 0; + word32 oldLen; + size_t tail; + + if (read_file(path, data, sizeof(data), &sz) != 0 || sz < 4) + return -1; + oldLen = get_be32(data); + if (oldLen > sz - 4 || 4 + (size_t)newLen + (sz - 4 - oldLen) > sizeof(out)) + return -1; + tail = sz - 4 - oldLen; + put_be32(out, newLen); + XMEMSET(out + 4, 0x5A, newLen); + XMEMCPY(out + 4, data + 4, (newLen < oldLen) ? newLen : oldLen); + XMEMCPY(out + 4 + newLen, data + 4 + oldLen, tail); + return write_file(path, out, 4 + newLen + tail); +} + +/* Loading may succeed but logging in must reject the damaged key record. */ +static int child_login_rejected(void) +{ + CK_RV rv; + CK_SLOT_ID slot = 0; + CK_SESSION_HANDLE session = CK_INVALID_HANDLE; + int res = CHILD_FAIL; + + if (pkcs11_load() != CKR_OK) + return CHILD_SETUP; + rv = init_slot(&slot); + if (rv != CKR_OK) + return CHILD_PASS; + rv = open_session(slot, 1, &session); + if (rv != CKR_OK) + res = CHILD_PASS; + (void)funcList->C_Finalize(NULL); + return res; +} + +static void check_damaged_key_record(const char* name, create_fn create, + const char* kind, word32 newLen) +{ + CK_RV rv; + char path[256]; + char msg[128]; + int res = CHILD_SETUP; + + rv = store_objects(create); + (void)snprintf(msg, sizeof(msg), "store %s", name); + CHECK_RV(rv, msg, CKR_OK); + if (rv != CKR_OK) + return; + if (find_store_file(kind, path, sizeof(path)) == 0 && + rewrite_first_array(path, newLen) == 0) { + res = run_in_child(child_login_rejected); + } + (void)snprintf(msg, sizeof(msg), "damaged %s record is rejected", name); + CHECK_TRUE(res == CHILD_PASS, msg); + cleanup_test_files(); +} + +#ifndef NO_DH +static CK_RV create_dh_private_key(CK_SESSION_HANDLE session) +{ + CK_OBJECT_CLASS keyClass = CKO_PRIVATE_KEY; + CK_KEY_TYPE keyType = CKK_DH; + CK_OBJECT_HANDLE obj = CK_INVALID_HANDLE; + CK_ATTRIBUTE tmpl[] = { + { CKA_CLASS, &keyClass, sizeof(keyClass) }, + { CKA_KEY_TYPE, &keyType, sizeof(keyType) }, + { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, + { CKA_PRIVATE, &ckTrue, sizeof(ckTrue) }, + { CKA_DERIVE, &ckTrue, sizeof(ckTrue) }, + { CKA_PRIME, dh_ffdhe2048_p, sizeof(dh_ffdhe2048_p) }, + { CKA_BASE, dh_ffdhe2048_g, sizeof(dh_ffdhe2048_g) }, + { CKA_VALUE, dh_2048_priv, sizeof(dh_2048_priv) }, + }; + + return funcList->C_CreateObject(session, tmpl, + sizeof(tmpl) / sizeof(*tmpl), &obj); +} +#endif + +#ifdef HAVE_ECC +static CK_RV create_ecc_private_key(CK_SESSION_HANDLE session) +{ + CK_OBJECT_CLASS keyClass = CKO_PRIVATE_KEY; + CK_KEY_TYPE keyType = CKK_EC; + CK_OBJECT_HANDLE obj = CK_INVALID_HANDLE; + CK_ATTRIBUTE tmpl[] = { + { CKA_CLASS, &keyClass, sizeof(keyClass) }, + { CKA_KEY_TYPE, &keyType, sizeof(keyType) }, + { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, + { CKA_PRIVATE, &ckTrue, sizeof(ckTrue) }, + { CKA_SIGN, &ckTrue, sizeof(ckTrue) }, + { CKA_EC_PARAMS, ecc_p256_params, sizeof(ecc_p256_params) }, + { CKA_VALUE, ecc_p256_priv, sizeof(ecc_p256_priv) }, + }; + + return funcList->C_CreateObject(session, tmpl, + sizeof(tmpl) / sizeof(*tmpl), &obj); +} +#endif + +#ifndef NO_RSA +static CK_RV create_rsa_private_key(CK_SESSION_HANDLE session) +{ + CK_OBJECT_CLASS keyClass = CKO_PRIVATE_KEY; + CK_KEY_TYPE keyType = CKK_RSA; + CK_OBJECT_HANDLE obj = CK_INVALID_HANDLE; + CK_ATTRIBUTE tmpl[] = { + { CKA_CLASS, &keyClass, sizeof(keyClass) }, + { CKA_KEY_TYPE, &keyType, sizeof(keyType) }, + { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, + { CKA_PRIVATE, &ckTrue, sizeof(ckTrue) }, + { CKA_SIGN, &ckTrue, sizeof(ckTrue) }, + { CKA_MODULUS, rsa_2048_modulus, sizeof(rsa_2048_modulus) }, + { CKA_PUBLIC_EXPONENT, rsa_2048_pub_exp, sizeof(rsa_2048_pub_exp) }, + { CKA_PRIVATE_EXPONENT, rsa_2048_priv_exp, sizeof(rsa_2048_priv_exp) }, + { CKA_PRIME_1, rsa_2048_p, sizeof(rsa_2048_p) }, + { CKA_PRIME_2, rsa_2048_q, sizeof(rsa_2048_q) }, + { CKA_EXPONENT_1, rsa_2048_dP, sizeof(rsa_2048_dP) }, + { CKA_EXPONENT_2, rsa_2048_dQ, sizeof(rsa_2048_dQ) }, + { CKA_COEFFICIENT, rsa_2048_u, sizeof(rsa_2048_u) }, + }; + + return funcList->C_CreateObject(session, tmpl, + sizeof(tmpl) / sizeof(*tmpl), &obj); +} +#endif + +#ifdef WOLFPKCS11_MLDSA +static CK_RV create_mldsa_key_pair(CK_SESSION_HANDLE session) +{ + CK_MECHANISM mech = { CKM_ML_DSA_KEY_PAIR_GEN, NULL, 0 }; + CK_ULONG paramSet = CKP_ML_DSA_44; + CK_OBJECT_HANDLE pub = CK_INVALID_HANDLE; + CK_OBJECT_HANDLE priv = CK_INVALID_HANDLE; + CK_ATTRIBUTE pubTmpl[] = { + { CKA_PARAMETER_SET, ¶mSet, sizeof(paramSet) }, + { CKA_VERIFY, &ckTrue, sizeof(ckTrue) }, + { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, + }; + CK_ATTRIBUTE privTmpl[] = { + { CKA_SIGN, &ckTrue, sizeof(ckTrue) }, + { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, + }; + + return funcList->C_GenerateKeyPair(session, &mech, pubTmpl, + sizeof(pubTmpl) / sizeof(*pubTmpl), privTmpl, + sizeof(privTmpl) / sizeof(*privTmpl), &pub, &priv); +} +#endif + +/* A stored private key record shorter than its authentication tag, or + * longer than the key it decrypts into. */ +static void test_short_private_key_record(void) +{ + printf("\n--- short private key record is rejected ---\n"); +#ifndef NO_DH + check_damaged_key_record("DH private key", create_dh_private_key, + "dhkey_priv", 8); + check_damaged_key_record("tag-only DH private key", create_dh_private_key, + "dhkey_priv", 16); + check_damaged_key_record("oversized DH private key", + create_dh_private_key, "dhkey_priv", 4096 + 16); +#endif +#ifdef HAVE_ECC + check_damaged_key_record("tag-only ECC private key", + create_ecc_private_key, "ecckey_priv", 16); +#endif +#ifndef NO_RSA + check_damaged_key_record("tag-only RSA private key", + create_rsa_private_key, "rsakey_priv", 16); +#endif +#ifdef WOLFPKCS11_MLDSA + check_damaged_key_record("tag-only ML-DSA private key", + create_mldsa_key_pair, "mldsakey_priv", 16); +#endif +} + #define LARGE_VALUE_SZ 300001 static byte largeValue[LARGE_VALUE_SZ]; static byte largeRead[LARGE_VALUE_SZ]; @@ -709,6 +895,7 @@ int main(int argc, char* argv[]) test_large_stored_value_peak(); #endif test_truncated_data_record(); + test_short_private_key_record(); return pkcs11_test_summary(); } From 2982321af146b93b9e05b773d4ef054addcff6da Mon Sep 17 00:00:00 2001 From: Aidan Garske Date: Tue, 6 Oct 2026 10:53:42 -0700 Subject: [PATCH 05/13] F-4733 - Bound stored DH public key length to the key buffer --- src/internal.c | 3 +++ tests/object_store_decode_test.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/src/internal.c b/src/internal.c index 77960a7b..70a522de 100644 --- a/src/internal.c +++ b/src/internal.c @@ -5059,6 +5059,9 @@ static int wp11_Object_Decode_DhKey(WP11_Object* object) if (ret == 0) object->data.dhKey->len = object->keyDataLen - AES_BLOCK_SIZE; } + else if (object->keyDataLen > WP11_MAX_DH_KEY_SZ) { + ret = BUFFER_E; + } else { XMEMCPY(object->data.dhKey->key, object->keyData, object->keyDataLen); object->data.dhKey->len = object->keyDataLen; diff --git a/tests/object_store_decode_test.c b/tests/object_store_decode_test.c index 963e34b6..15a57d96 100644 --- a/tests/object_store_decode_test.c +++ b/tests/object_store_decode_test.c @@ -735,6 +735,34 @@ static void test_short_private_key_record(void) #endif } +#ifndef NO_DH +static CK_RV create_dh_public_key(CK_SESSION_HANDLE session) +{ + CK_OBJECT_CLASS keyClass = CKO_PUBLIC_KEY; + CK_KEY_TYPE keyType = CKK_DH; + CK_OBJECT_HANDLE obj = CK_INVALID_HANDLE; + CK_ATTRIBUTE tmpl[] = { + { CKA_CLASS, &keyClass, sizeof(keyClass) }, + { CKA_KEY_TYPE, &keyType, sizeof(keyType) }, + { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, + { CKA_PRIME, dh_ffdhe2048_p, sizeof(dh_ffdhe2048_p) }, + { CKA_BASE, dh_ffdhe2048_g, sizeof(dh_ffdhe2048_g) }, + { CKA_VALUE, dh_2048_pub, sizeof(dh_2048_pub) }, + }; + + return funcList->C_CreateObject(session, tmpl, + sizeof(tmpl) / sizeof(*tmpl), &obj); +} + +/* A stored public key record longer than the largest supported key. */ +static void test_oversized_public_key_record(void) +{ + printf("\n--- oversized public key record is rejected ---\n"); + check_damaged_key_record("oversized DH public key", create_dh_public_key, + "dhkey_pub", 4096); +} +#endif + #define LARGE_VALUE_SZ 300001 static byte largeValue[LARGE_VALUE_SZ]; static byte largeRead[LARGE_VALUE_SZ]; @@ -896,6 +924,9 @@ int main(int argc, char* argv[]) #endif test_truncated_data_record(); test_short_private_key_record(); +#ifndef NO_DH + test_oversized_public_key_record(); +#endif return pkcs11_test_summary(); } From 4a951dfdf53b6f216715801481a7b5e3aa28a1cd Mon Sep 17 00:00:00 2001 From: Aidan Garske Date: Tue, 6 Oct 2026 11:06:36 -0700 Subject: [PATCH 06/13] F-3641 - Reject stored trust records that are not the trust size --- src/internal.c | 28 ++++++++++++++++++------ tests/object_store_decode_test.c | 37 ++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 7 deletions(-) diff --git a/src/internal.c b/src/internal.c index 70a522de..37a9c6b3 100644 --- a/src/internal.c +++ b/src/internal.c @@ -3511,14 +3511,25 @@ static void wp11_Object_Decode_Cert(WP11_Object* object) * Trust is not encrypted. * * @param [in, out] object Trust object. + * @return 0 on success. + * @return BUFFER_E when the stored trust data is not the expected size. */ -static void wp11_Object_Decode_Trust(WP11_Object* object) +static int wp11_Object_Decode_Trust(WP11_Object* object) { + int ret = 0; + if (object->keyData != NULL) { - XMEMCPY((unsigned char*)&object->data.trust, object->keyData, - object->keyDataLen); + if (object->keyDataLen != (int)sizeof(WP11_Trust)) { + ret = BUFFER_E; + } + else { + XMEMCPY((unsigned char*)&object->data.trust, object->keyData, + object->keyDataLen); + } } - object->encoded = 0; + object->encoded = (ret != 0); + + return ret; } #endif @@ -3585,7 +3596,11 @@ static int wp11_Object_Load_Trust(WP11_Object* object, int tokenId, int objId) ret = wp11_storage_read_alloc_array(storage, &object->keyData, &object->keyDataLen); wp11_storage_close(storage); - wp11_Object_Decode_Trust(object); + /* An empty record would leave the trust zeroed but decoded. */ + if (ret == 0 && object->keyData == NULL) + ret = BUFFER_E; + if (ret == 0) + ret = wp11_Object_Decode_Trust(object); } return ret; @@ -6960,8 +6975,7 @@ static int wp11_Object_Decode(WP11_Object* object) } #ifdef WOLFPKCS11_NSS else if (object->objClass == CKO_NSS_TRUST) { - wp11_Object_Decode_Trust(object); - ret = 0; + ret = wp11_Object_Decode_Trust(object); } #endif else if (object->objClass == CKO_DATA) { diff --git a/tests/object_store_decode_test.c b/tests/object_store_decode_test.c index 15a57d96..43d034be 100644 --- a/tests/object_store_decode_test.c +++ b/tests/object_store_decode_test.c @@ -763,6 +763,40 @@ static void test_oversized_public_key_record(void) } #endif +#ifdef WOLFPKCS11_NSS +static CK_RV create_trust_object(CK_SESSION_HANDLE session) +{ + CK_OBJECT_CLASS trustClass = CKO_NSS_TRUST; + CK_OBJECT_HANDLE obj = CK_INVALID_HANDLE; + static byte issuer[] = "CN=Test,O=wolfSSL,C=US"; + static byte serial[] = { 0x02, 0x05, 0x00, 0xC6, 0xA7, 0x91, 0x84 }; + static byte sha1Hash[20] = { 0 }; + static byte md5Hash[16] = { 0 }; + CK_ULONG trustValue = 0xCE534352; + CK_ATTRIBUTE tmpl[] = { + { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, + { CKA_CLASS, &trustClass, sizeof(trustClass) }, + { CKA_ISSUER, issuer, sizeof(issuer) - 1 }, + { CKA_SERIAL_NUMBER, serial, sizeof(serial) }, + { CKA_CERT_SHA1_HASH, sha1Hash, sizeof(sha1Hash) }, + { CKA_CERT_MD5_HASH, md5Hash, sizeof(md5Hash) }, + { CKA_TRUST_SERVER_AUTH, &trustValue, sizeof(trustValue) }, + }; + + return funcList->C_CreateObject(session, tmpl, + sizeof(tmpl) / sizeof(*tmpl), &obj); +} + +/* Trust records have a fixed size and are stored unencrypted. */ +static void test_trust_record_length(void) +{ + printf("\n--- trust record length must match ---\n"); + check_damaged_key_record("oversized trust", create_trust_object, "trust", + 1024); + check_damaged_key_record("empty trust", create_trust_object, "trust", 0); +} +#endif + #define LARGE_VALUE_SZ 300001 static byte largeValue[LARGE_VALUE_SZ]; static byte largeRead[LARGE_VALUE_SZ]; @@ -927,6 +961,9 @@ int main(int argc, char* argv[]) #ifndef NO_DH test_oversized_public_key_record(); #endif +#ifdef WOLFPKCS11_NSS + test_trust_record_length(); +#endif return pkcs11_test_summary(); } From 4ab2ef30d6eb0fb7bee5f5635b34df8c31d7d05e Mon Sep 17 00:00:00 2001 From: Aidan Garske Date: Tue, 6 Oct 2026 11:14:03 -0700 Subject: [PATCH 07/13] F-3404 - Write the encoded TPM key into the caller's buffer --- src/internal.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/internal.c b/src/internal.c index 37a9c6b3..9b803a83 100644 --- a/src/internal.c +++ b/src/internal.c @@ -4150,15 +4150,15 @@ static int WP11_Object_EncodeTpmKey(WP11_Object* object, byte* keyData, if (keyData != NULL) { if (ret <= keyDataLen) { /* Write size marker for the public part */ - XMEMCPY(object->keyData, &pubAreaSize, + XMEMCPY(keyData, &pubAreaSize, sizeof(UINT16)); idx += sizeof(UINT16); /* Write the public part with bytes aligned */ - XMEMCPY(object->keyData + idx, pubAreaBuffer, + XMEMCPY(keyData + idx, pubAreaBuffer, sizeof(UINT16) + pubAreaSize); idx += sizeof(UINT16) + pubAreaSize; /* Write the private part, size marker is included */ - XMEMCPY(object->keyData + idx, &object->tpmKey->priv, + XMEMCPY(keyData + idx, &object->tpmKey->priv, sizeof(UINT16) + object->tpmKey->priv.size); idx += sizeof(UINT16) + object->tpmKey->priv.size; From af7c5da2ed97aaf4b0a4331c83c2634fa9df55f2 Mon Sep 17 00:00:00 2001 From: Aidan Garske Date: Tue, 6 Oct 2026 11:20:21 -0700 Subject: [PATCH 08/13] F-8643 - Keep key buffer allocation errors in TPM RSA and ECC encoders --- src/internal.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/internal.c b/src/internal.c index 9b803a83..578ccf85 100644 --- a/src/internal.c +++ b/src/internal.c @@ -4336,7 +4336,10 @@ static int wp11_Object_Encode_RsaKey(WP11_Object* object) } #ifdef WOLFPKCS11_TPM - ret = WP11_Object_EncodeTpmKey(object, object->keyData, object->keyDataLen); + if (ret == 0) { + ret = WP11_Object_EncodeTpmKey(object, object->keyData, + object->keyDataLen); + } if (ret > 0) { ret = 0; } @@ -4678,7 +4681,10 @@ static int wp11_Object_Encode_EccKey(WP11_Object* object) } #ifdef WOLFPKCS11_TPM - ret = WP11_Object_EncodeTpmKey(object, object->keyData, object->keyDataLen); + if (ret == 0) { + ret = WP11_Object_EncodeTpmKey(object, object->keyData, + object->keyDataLen); + } if (ret > 0) { ret = 0; } From 004016c8cfc9e272da271cdc4628bf3e2b5f84cf Mon Sep 17 00:00:00 2001 From: Aidan Garske Date: Tue, 6 Oct 2026 11:27:25 -0700 Subject: [PATCH 09/13] F-13813 - Release DH and symmetric key data when encoding fails --- src/internal.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/internal.c b/src/internal.c index 578ccf85..1256dfcb 100644 --- a/src/internal.c +++ b/src/internal.c @@ -5130,6 +5130,14 @@ static int wp11_Object_Encode_DhKey(WP11_Object* object) } } + if (ret != 0) { + if (object->keyData != NULL) + wc_ForceZero(object->keyData, object->keyDataLen); + XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); + object->keyData = NULL; + object->keyDataLen = 0; + } + return ret; } @@ -6507,6 +6515,14 @@ static int wp11_Object_Encode_SymmKey(WP11_Object* object) object->keyDataLen = object->data.symmKey->len + AES_BLOCK_SIZE; } + if (ret != 0) { + if (object->keyData != NULL) + wc_ForceZero(object->keyData, object->keyDataLen); + XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); + object->keyData = NULL; + object->keyDataLen = 0; + } + return ret; } From c240e05f3bc45a89cb4b9f4db4645c594ec2d3f7 Mon Sep 17 00:00:00 2001 From: Aidan Garske Date: Tue, 6 Oct 2026 11:33:20 -0700 Subject: [PATCH 10/13] F-3017 - Zeroize encoded private keys when RSA, ECC or ML-DSA encoding fails --- src/internal.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/internal.c b/src/internal.c index 1256dfcb..a760ef73 100644 --- a/src/internal.c +++ b/src/internal.c @@ -4370,6 +4370,8 @@ static int wp11_Object_Encode_RsaKey(WP11_Object* object) } if (ret != 0) { + if (object->keyData != NULL) + wc_ForceZero(object->keyData, object->keyDataLen); XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); object->keyData = NULL; object->keyDataLen = 0; @@ -4711,6 +4713,8 @@ static int wp11_Object_Encode_EccKey(WP11_Object* object) } if (ret != 0) { + if (object->keyData != NULL) + wc_ForceZero(object->keyData, object->keyDataLen); XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); object->keyData = NULL; object->keyDataLen = 0; @@ -4961,6 +4965,8 @@ static int wp11_Object_Encode_MldsaKey(WP11_Object* object) } if (ret != 0) { + if (object->keyData != NULL) + wc_ForceZero(object->keyData, object->keyDataLen); XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); object->keyData = NULL; object->keyDataLen = 0; From bb0085e11e172b05102ac892abc4656b4a0cd490 Mon Sep 17 00:00:00 2001 From: Aidan Garske Date: Tue, 6 Oct 2026 11:47:05 -0700 Subject: [PATCH 11/13] F-3413 - Zeroize decrypted private key buffers on every decode path --- src/internal.c | 21 ++-- tests/object_store_decode_test.c | 170 +++++++++++++++++++++++++++++++ 2 files changed, 185 insertions(+), 6 deletions(-) diff --git a/src/internal.c b/src/internal.c index a760ef73..ea0f1950 100644 --- a/src/internal.c +++ b/src/internal.c @@ -4264,16 +4264,20 @@ static int wp11_Object_Decode_RsaKey(WP11_Object* object) if (ret == 0) { /* Decode RSA private key. */ ret = wc_RsaPrivateKeyDecode(der, &idx, key, len); - wc_ForceZero(der, len); } - if (der != NULL) + if (der != NULL) { + wc_ForceZero(der, len); XFREE(der, NULL, DYNAMIC_TYPE_TMP_BUFFER); + } } else { /* Decode RSA public key. */ ret = wc_RsaPublicKeyDecode(object->keyData, &idx, key, object->keyDataLen); } + /* Drop partially decoded components and the init before a retry. */ + if (ret != 0) + wc_FreeRsaKey(key); object->encoded = (ret != 0); return ret; @@ -4620,16 +4624,20 @@ static int wp11_Object_Decode_EccKey(WP11_Object* object) if (ret == 0) { /* Decode ECC private key. */ ret = wc_EccPrivateKeyDecode(der, &idx, key, len); - wc_ForceZero(der, len); } - if (der != NULL) + if (der != NULL) { + wc_ForceZero(der, len); XFREE(der, NULL, DYNAMIC_TYPE_TMP_BUFFER); + } } else { /* Decode ECC public key. */ ret = wc_EccPublicKeyDecode(object->keyData, &idx, key, object->keyDataLen); } + /* Drop partially decoded components and the init before a retry. */ + if (ret != 0) + wc_ecc_free(key); object->encoded = (ret != 0); return ret; @@ -4877,10 +4885,11 @@ static int wp11_Object_Decode_MldsaKey(WP11_Object* object) ret = MldsaKeyTryDecode(object->data.mldsaKey, WC_ML_DSA_87, der, len, object->objClass); } - wc_ForceZero(der, len); } - if (der != NULL) + if (der != NULL) { + wc_ForceZero(der, len); XFREE(der, NULL, DYNAMIC_TYPE_TMP_BUFFER); + } } else { /* Decode ML-DSA public key. */ diff --git a/tests/object_store_decode_test.c b/tests/object_store_decode_test.c index 43d034be..6432fc66 100644 --- a/tests/object_store_decode_test.c +++ b/tests/object_store_decode_test.c @@ -940,6 +940,173 @@ static void test_large_stored_value_peak(void) } #endif +#ifdef HAVE_ALLOC_TRACKING +#define MAX_TRACKED 8192 + +typedef struct TrackedAlloc { + void* ptr; + size_t sz; +} TrackedAlloc; + +static TrackedAlloc tracked[MAX_TRACKED]; +static const byte* secretNeedle = NULL; +static size_t secretNeedleSz = 0; +static int secretFreed = 0; + +static void track_add(void* ptr, size_t sz) +{ + int i; + + for (i = 0; ptr != NULL && i < MAX_TRACKED; i++) { + if (tracked[i].ptr == NULL) { + tracked[i].ptr = ptr; + tracked[i].sz = sz; + break; + } + } +} + +static int track_find(void* ptr) +{ + int i; + + for (i = 0; ptr != NULL && i < MAX_TRACKED; i++) { + if (tracked[i].ptr == ptr) + return i; + } + return -1; +} + +static int holds_secret(const byte* buf, size_t sz) +{ + size_t i; + + for (i = 0; sz >= secretNeedleSz && i <= sz - secretNeedleSz; i++) { + if (XMEMCMP(buf + i, secretNeedle, secretNeedleSz) == 0) + return 1; + } + return 0; +} + +static void* scan_malloc(size_t sz) +{ + void* ptr = malloc(sz); + + /* Zeroed so a scan only sees bytes the library wrote. */ + if (ptr != NULL) + XMEMSET(ptr, 0, sz); + track_add(ptr, sz); + return ptr; +} + +static void scan_free(void* ptr) +{ + int i = track_find(ptr); + + if (i >= 0) { + if (holds_secret((const byte*)ptr, tracked[i].sz)) + secretFreed++; + tracked[i].ptr = NULL; + } + free(ptr); +} + +static void* scan_realloc(void* ptr, size_t sz) +{ + int i = track_find(ptr); + int oldKnown = (ptr == NULL || i >= 0); + size_t oldSz = (i >= 0) ? tracked[i].sz : 0; + void* newPtr = realloc(ptr, sz); + + if (newPtr != NULL) { + if (i >= 0) + tracked[i].ptr = NULL; + if (oldKnown && sz > oldSz) + XMEMSET((byte*)newPtr + oldSz, 0, sz - oldSz); + track_add(newPtr, sz); + } + return newPtr; +} + +/* Logging in with a stored private key that fails authentication must not + * release decrypted key material to the heap. */ +static int child_no_key_material_freed(void) +{ + CK_SLOT_ID slot = 0; + CK_SESSION_HANDLE session = CK_INVALID_HANDLE; + + if (wolfSSL_SetAllocators(scan_malloc, scan_free, scan_realloc) != 0) + return CHILD_SETUP; + if (pkcs11_load() != CKR_OK) + return CHILD_SETUP; + if (init_slot(&slot) == CKR_OK) { + (void)open_session(slot, 1, &session); + (void)funcList->C_Finalize(NULL); + } + if (secretFreed != 0) { + fprintf(stderr, " %d freed buffer(s) held key material\n", + secretFreed); + return CHILD_FAIL; + } + return CHILD_PASS; +} + +/* Flip the last byte of the first length-prefixed array (the GCM tag). */ +static int corrupt_first_array_tag(const char* path) +{ + static byte data[MAX_FILE_SZ]; + size_t sz = 0; + word32 len; + + if (read_file(path, data, sizeof(data), &sz) != 0 || sz < 4) + return -1; + len = get_be32(data); + if (len == 0 || len > sz - 4) + return -1; + data[4 + len - 1] ^= 0x01; + return write_file(path, data, sz); +} + +static void check_failed_decrypt_scrubbed(const char* name, create_fn create, + const char* kind, + const byte* secret, size_t secretSz) +{ + CK_RV rv; + char path[256]; + char msg[128]; + int res = CHILD_SETUP; + + rv = store_objects(create); + (void)snprintf(msg, sizeof(msg), "store %s", name); + CHECK_RV(rv, msg, CKR_OK); + if (rv != CKR_OK) + return; + secretNeedle = secret; + secretNeedleSz = secretSz; + if (find_store_file(kind, path, sizeof(path)) == 0 && + corrupt_first_array_tag(path) == 0) { + res = run_in_child(child_no_key_material_freed); + } + (void)snprintf(msg, sizeof(msg), + "%s buffer is cleared after failed decrypt", name); + CHECK_TRUE(res == CHILD_PASS, msg); + cleanup_test_files(); +} + +static void test_failed_decrypt_scrubbed(void) +{ + printf("\n--- private key buffer cleared after failed decrypt ---\n"); +#ifdef HAVE_ECC + check_failed_decrypt_scrubbed("ECC private key", create_ecc_private_key, + "ecckey_priv", ecc_p256_priv, sizeof(ecc_p256_priv)); +#endif +#ifndef NO_RSA + check_failed_decrypt_scrubbed("RSA private key", create_rsa_private_key, + "rsakey_priv", rsa_2048_priv_exp, 32); +#endif +} +#endif + int main(int argc, char* argv[]) { (void)argc; @@ -964,6 +1131,9 @@ int main(int argc, char* argv[]) #ifdef WOLFPKCS11_NSS test_trust_record_length(); #endif +#ifdef HAVE_ALLOC_TRACKING + test_failed_decrypt_scrubbed(); +#endif return pkcs11_test_summary(); } From 808664451bd7a26279c50beb2abe4a9db618f3d3 Mon Sep 17 00:00:00 2001 From: Aidan Garske Date: Tue, 6 Oct 2026 11:53:02 -0700 Subject: [PATCH 12/13] F-8667 - Scrub symmetric and DH key buffers after a failed decrypt --- src/internal.c | 8 +++++++ tests/object_store_decode_test.c | 41 ++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/src/internal.c b/src/internal.c index ea0f1950..0bff0a83 100644 --- a/src/internal.c +++ b/src/internal.c @@ -5091,6 +5091,10 @@ static int wp11_Object_Decode_DhKey(WP11_Object* object) sizeof(object->slot->token.key), object->iv, sizeof(object->iv), object->devId); + if (ret != 0) { + wc_ForceZero(object->data.dhKey->key, + (word32)(object->keyDataLen - AES_BLOCK_SIZE)); + } } if (ret == 0) object->data.dhKey->len = object->keyDataLen - AES_BLOCK_SIZE; @@ -6493,6 +6497,10 @@ static int wp11_Object_Decode_SymmKey(WP11_Object* object) object->slot->token.key, sizeof(object->slot->token.key), object->iv, sizeof(object->iv), object->devId); + if (ret != 0) { + wc_ForceZero(object->data.symmKey->data, + (word32)(object->keyDataLen - AES_BLOCK_SIZE)); + } } if (ret == 0) object->data.symmKey->len = object->keyDataLen - AES_BLOCK_SIZE; diff --git a/tests/object_store_decode_test.c b/tests/object_store_decode_test.c index 6432fc66..dc3f434b 100644 --- a/tests/object_store_decode_test.c +++ b/tests/object_store_decode_test.c @@ -1105,6 +1105,46 @@ static void test_failed_decrypt_scrubbed(void) "rsakey_priv", rsa_2048_priv_exp, 32); #endif } + +#ifndef NO_AES +static byte aesKeyValue[32] = { + 0x5b, 0x1e, 0xc4, 0x27, 0x9a, 0x30, 0xe8, 0x71, + 0x0d, 0xb6, 0x43, 0xf9, 0x82, 0x2c, 0x95, 0x6e, + 0xa7, 0x14, 0xd0, 0x3b, 0x68, 0xcf, 0x51, 0x0a, + 0xe3, 0x9d, 0x26, 0x7c, 0xb1, 0x48, 0xf5, 0x13 +}; + +static CK_RV create_aes_key(CK_SESSION_HANDLE session) +{ + CK_OBJECT_CLASS keyClass = CKO_SECRET_KEY; + CK_KEY_TYPE keyType = CKK_AES; + CK_OBJECT_HANDLE obj = CK_INVALID_HANDLE; + CK_ATTRIBUTE tmpl[] = { + { CKA_CLASS, &keyClass, sizeof(keyClass) }, + { CKA_KEY_TYPE, &keyType, sizeof(keyType) }, + { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, + { CKA_PRIVATE, &ckTrue, sizeof(ckTrue) }, + { CKA_ENCRYPT, &ckTrue, sizeof(ckTrue) }, + { CKA_VALUE, aesKeyValue, sizeof(aesKeyValue) }, + }; + + return funcList->C_CreateObject(session, tmpl, + sizeof(tmpl) / sizeof(*tmpl), &obj); +} +#endif + +static void test_failed_key_decrypt_scrubbed(void) +{ + printf("\n--- secret key storage cleared after failed decrypt ---\n"); +#ifndef NO_AES + check_failed_decrypt_scrubbed("AES key", create_aes_key, "symmkey", + aesKeyValue, sizeof(aesKeyValue)); +#endif +#ifndef NO_DH + check_failed_decrypt_scrubbed("DH private key value", create_dh_private_key, + "dhkey_priv", dh_2048_priv, sizeof(dh_2048_priv)); +#endif +} #endif int main(int argc, char* argv[]) @@ -1133,6 +1173,7 @@ int main(int argc, char* argv[]) #endif #ifdef HAVE_ALLOC_TRACKING test_failed_decrypt_scrubbed(); + test_failed_key_decrypt_scrubbed(); #endif return pkcs11_test_summary(); From 0d8ae911e4ee33195de4503b4366e9fddd7255fb Mon Sep 17 00:00:00 2001 From: Aidan Garske Date: Tue, 13 Oct 2026 13:32:45 -0700 Subject: [PATCH 13/13] Keep the old key record when encoding fails --- src/internal.c | 144 +++++++++++++++---------------- tests/object_store_decode_test.c | 90 ++++++++++++++++++- 2 files changed, 157 insertions(+), 77 deletions(-) diff --git a/src/internal.c b/src/internal.c index 0bff0a83..96a6bfed 100644 --- a/src/internal.c +++ b/src/internal.c @@ -3488,6 +3488,33 @@ static int wp11_DecryptData(byte* out, byte* data, int len, byte* key, return ret; } +/** + * Finish encoding key data into a new buffer: on success release the previous + * record, on failure drop the new buffer and put the previous record back so + * the key still decodes at the next login. + * + * @param [in, out] object Key object. + * @param [in] oldData Key data buffer before the encode. + * @param [in] oldLen Length of key data before the encode. + * @param [in] newLen Size of the buffer allocated for the encode. + * @param [in] ret Result of the encode. + */ +static void wp11_Object_EncodeDone(WP11_Object* object, unsigned char* oldData, + int oldLen, word32 newLen, int ret) +{ + if (ret == 0) { + XFREE(oldData, NULL, DYNAMIC_TYPE_TMP_BUFFER); + } + else { + if (object->keyData != oldData && object->keyData != NULL) { + wc_ForceZero(object->keyData, newLen); + XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); + } + object->keyData = oldData; + object->keyDataLen = oldLen; + } +} + /** * "Decode" the certificate. * @@ -4295,6 +4322,8 @@ static int wp11_Object_Decode_RsaKey(WP11_Object* object) static int wp11_Object_Encode_RsaKey(WP11_Object* object) { int ret; + unsigned char* oldData = object->keyData; + int oldLen = object->keyDataLen; #ifdef WOLFPKCS11_TPM ret = WP11_Object_EncodeTpmKey(object, NULL, 0); @@ -4331,7 +4360,6 @@ static int wp11_Object_Encode_RsaKey(WP11_Object* object) } if (ret == 0) { - XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); /* Allocate buffer to hold encoded key. */ object->keyData = (unsigned char*)XMALLOC(object->keyDataLen, NULL, DYNAMIC_TYPE_TMP_BUFFER); @@ -4373,13 +4401,8 @@ static int wp11_Object_Encode_RsaKey(WP11_Object* object) } } - if (ret != 0) { - if (object->keyData != NULL) - wc_ForceZero(object->keyData, object->keyDataLen); - XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); - object->keyData = NULL; - object->keyDataLen = 0; - } + wp11_Object_EncodeDone(object, oldData, oldLen, (word32)object->keyDataLen, + ret); return ret; } @@ -4655,6 +4678,8 @@ static int wp11_Object_Decode_EccKey(WP11_Object* object) static int wp11_Object_Encode_EccKey(WP11_Object* object) { int ret; + unsigned char* oldData = object->keyData; + int oldLen = object->keyDataLen; #ifdef WOLFPKCS11_TPM ret = WP11_Object_EncodeTpmKey(object, NULL, 0); @@ -4682,7 +4707,6 @@ static int wp11_Object_Encode_EccKey(WP11_Object* object) } if (ret == 0) { - XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); /* Allocate buffer to hold encoded key. */ object->keyData = (unsigned char*)XMALLOC(object->keyDataLen, NULL, DYNAMIC_TYPE_TMP_BUFFER); @@ -4720,13 +4744,8 @@ static int wp11_Object_Encode_EccKey(WP11_Object* object) } } - if (ret != 0) { - if (object->keyData != NULL) - wc_ForceZero(object->keyData, object->keyDataLen); - XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); - object->keyData = NULL; - object->keyDataLen = 0; - } + wp11_Object_EncodeDone(object, oldData, oldLen, (word32)object->keyDataLen, + ret); return ret; } @@ -4924,6 +4943,8 @@ static int wp11_Object_Decode_MldsaKey(WP11_Object* object) static int wp11_Object_Encode_MldsaKey(WP11_Object* object) { int ret; + unsigned char* oldData = object->keyData; + int oldLen = object->keyDataLen; if (object->objClass == CKO_PRIVATE_KEY) { /* Get length of encoded private key. */ @@ -4943,7 +4964,6 @@ static int wp11_Object_Encode_MldsaKey(WP11_Object* object) } if (ret == 0) { - XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); /* Allocate buffer to hold encoded key. */ object->keyData = (unsigned char*)XMALLOC(object->keyDataLen, NULL, DYNAMIC_TYPE_TMP_BUFFER); @@ -4973,13 +4993,8 @@ static int wp11_Object_Encode_MldsaKey(WP11_Object* object) } } - if (ret != 0) { - if (object->keyData != NULL) - wc_ForceZero(object->keyData, object->keyDataLen); - XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); - object->keyData = NULL; - object->keyDataLen = 0; - } + wp11_Object_EncodeDone(object, oldData, oldLen, (word32)object->keyDataLen, + ret); return ret; } @@ -5123,12 +5138,14 @@ static int wp11_Object_Decode_DhKey(WP11_Object* object) static int wp11_Object_Encode_DhKey(WP11_Object* object) { int ret = 0; + unsigned char* oldData = object->keyData; + int oldLen = object->keyDataLen; + word32 newLen = object->data.dhKey->len + AES_BLOCK_SIZE; - object->keyDataLen = object->data.dhKey->len + AES_BLOCK_SIZE; - XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); + object->keyDataLen = (int)newLen; /* Allocate buffer to hold encoded key. */ - object->keyData = (unsigned char*)XMALLOC(object->keyDataLen, NULL, - DYNAMIC_TYPE_TMP_BUFFER); + object->keyData = (unsigned char*)XMALLOC(newLen, NULL, + DYNAMIC_TYPE_TMP_BUFFER); if (object->keyData == NULL) ret = MEMORY_E; @@ -5148,14 +5165,7 @@ static int wp11_Object_Encode_DhKey(WP11_Object* object) object->keyDataLen = object->data.dhKey->len; } } - - if (ret != 0) { - if (object->keyData != NULL) - wc_ForceZero(object->keyData, object->keyDataLen); - XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); - object->keyData = NULL; - object->keyDataLen = 0; - } + wp11_Object_EncodeDone(object, oldData, oldLen, newLen, ret); return ret; } @@ -5527,6 +5537,8 @@ static int wp11_Object_Encode_MlKemKey(WP11_Object* object) { int ret; word32 keyLen = 0; + unsigned char* oldData = object->keyData; + int oldLen = object->keyDataLen; if (object->objClass == CKO_PRIVATE_KEY) { ret = wc_MlKemKey_PrivateKeySize(object->data.mlKemKey, &keyLen); @@ -5542,7 +5554,6 @@ static int wp11_Object_Encode_MlKemKey(WP11_Object* object) } if (ret == 0) { - XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); object->keyData = (unsigned char*)XMALLOC(object->keyDataLen, NULL, DYNAMIC_TYPE_TMP_BUFFER); if (object->keyData == NULL) @@ -5564,13 +5575,8 @@ static int wp11_Object_Encode_MlKemKey(WP11_Object* object) object->keyData, keyLen); } - if (ret != 0) { - if (object->keyData != NULL) - wc_ForceZero(object->keyData, object->keyDataLen); - XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); - object->keyData = NULL; - object->keyDataLen = 0; - } + wp11_Object_EncodeDone(object, oldData, oldLen, (word32)object->keyDataLen, + ret); return ret; } @@ -5749,13 +5755,14 @@ static int wp11_Object_Encode_HssKey(WP11_Object* object) { int ret; word32 pubLen = 0; + unsigned char* oldData = object->keyData; + int oldLen = object->keyDataLen; if (object->objClass != CKO_PUBLIC_KEY) return NOT_AVAILABLE_E; /* verify-only: no private-key objects */ ret = wc_LmsKey_GetPubLen(object->data.lmsKey, &pubLen); if (ret == 0) { - XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); object->keyData = (unsigned char*)XMALLOC(pubLen, NULL, DYNAMIC_TYPE_TMP_BUFFER); if (object->keyData == NULL) @@ -5767,14 +5774,7 @@ static int wp11_Object_Encode_HssKey(WP11_Object* object) if (ret == 0) object->keyDataLen = (int)pubLen; } - /* On failure release the buffer so a later store does not skip re-encoding - * (keyData != NULL) and persist an uninitialised buffer with a stale - * length. This is public key material only, so no zeroization is needed. */ - if (ret != 0) { - XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); - object->keyData = NULL; - object->keyDataLen = 0; - } + wp11_Object_EncodeDone(object, oldData, oldLen, pubLen, ret); return ret; } @@ -6194,13 +6194,14 @@ static int wp11_Object_Encode_XmssKey(WP11_Object* object) { int ret; word32 pubLen = 0; + unsigned char* oldData = object->keyData; + int oldLen = object->keyDataLen; if (object->objClass != CKO_PUBLIC_KEY) return NOT_AVAILABLE_E; ret = wc_XmssKey_GetPubLen(object->data.xmssKey, &pubLen); if (ret == 0) { - XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); object->keyData = (unsigned char*)XMALLOC(pubLen, NULL, DYNAMIC_TYPE_TMP_BUFFER); if (object->keyData == NULL) @@ -6212,14 +6213,7 @@ static int wp11_Object_Encode_XmssKey(WP11_Object* object) if (ret == 0) object->keyDataLen = (int)pubLen; } - /* On failure release the buffer so a later store does not skip re-encoding - * (keyData != NULL) and persist an uninitialised buffer with a stale - * length. This is public key material only, so no zeroization is needed. */ - if (ret != 0) { - XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); - object->keyData = NULL; - object->keyDataLen = 0; - } + wp11_Object_EncodeDone(object, oldData, oldLen, pubLen, ret); return ret; } @@ -6519,12 +6513,14 @@ static int wp11_Object_Decode_SymmKey(WP11_Object* object) static int wp11_Object_Encode_SymmKey(WP11_Object* object) { int ret = 0; + unsigned char* oldData = object->keyData; + int oldLen = object->keyDataLen; + word32 newLen = object->data.symmKey->len + AES_BLOCK_SIZE; - object->keyDataLen = object->data.symmKey->len + AES_BLOCK_SIZE; - XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); + object->keyDataLen = (int)newLen; /* Allocate buffer to hold encoded key. */ - object->keyData = (unsigned char*)XMALLOC(object->keyDataLen, NULL, - DYNAMIC_TYPE_TMP_BUFFER); + object->keyData = (unsigned char*)XMALLOC(newLen, NULL, + DYNAMIC_TYPE_TMP_BUFFER); if (object->keyData == NULL) ret = MEMORY_E; @@ -6534,17 +6530,8 @@ static int wp11_Object_Encode_SymmKey(WP11_Object* object) object->slot->token.key, sizeof(object->slot->token.key), object->iv, sizeof(object->iv), object->devId); - if (ret == 0) - object->keyDataLen = object->data.symmKey->len + AES_BLOCK_SIZE; - } - - if (ret != 0) { - if (object->keyData != NULL) - wc_ForceZero(object->keyData, object->keyDataLen); - XFREE(object->keyData, NULL, DYNAMIC_TYPE_TMP_BUFFER); - object->keyData = NULL; - object->keyDataLen = 0; } + wp11_Object_EncodeDone(object, oldData, oldLen, newLen, ret); return ret; } @@ -7211,10 +7198,12 @@ static int wp11_Object_Encode(WP11_Object* object, int protect) { int ret = 0; int encrypt = wp11_Object_IsEncrypted(object); + byte oldIv[GCM_NONCE_MID_SZ]; /* Every AES-GCM encryption under the token key needs a fresh nonce. Do * this immediately before encoding, while the plaintext is still the * source of the ciphertext that will be persisted. */ + XMEMCPY(oldIv, object->iv, sizeof(oldIv)); if (encrypt) { WP11_Lock_LockRW(&object->slot->token.rngLock); ret = wc_RNG_GenerateBlock(&object->slot->token.rng, object->iv, @@ -7223,6 +7212,9 @@ static int wp11_Object_Encode(WP11_Object* object, int protect) } if (ret == 0) ret = wp11_Object_EncodeData(object, protect); + /* The previous record stays in use when the encode fails. */ + if (ret != 0) + XMEMCPY(object->iv, oldIv, sizeof(object->iv)); return ret; } diff --git a/tests/object_store_decode_test.c b/tests/object_store_decode_test.c index dc3f434b..74fcdb2f 100644 --- a/tests/object_store_decode_test.c +++ b/tests/object_store_decode_test.c @@ -95,6 +95,7 @@ static size_t largestAlloc = 0; static size_t peakCap = 0; static size_t liveBytes = 0; static size_t peakLive = 0; +static int refuseAllocs = 0; static void* track_malloc(size_t sz) { @@ -102,7 +103,7 @@ static void* track_malloc(size_t sz) if (sz > largestAlloc) largestAlloc = sz; - if (sz >= ALLOC_REFUSE_SZ) + if (sz >= ALLOC_REFUSE_SZ || refuseAllocs) return NULL; p = (unsigned char*)malloc(sz + TRACK_HDR_SZ); if (p == NULL) @@ -1133,6 +1134,90 @@ static CK_RV create_aes_key(CK_SESSION_HANDLE session) } #endif +#ifndef NO_AES +/* Count the AES keys visible after a login and check one still encrypts. */ +static CK_RV use_stored_aes_key(CK_SESSION_HANDLE session) +{ + CK_RV rv; + CK_OBJECT_CLASS keyClass = CKO_SECRET_KEY; + CK_KEY_TYPE keyType = CKK_AES; + CK_ATTRIBUTE tmpl[] = { + { CKA_CLASS, &keyClass, sizeof(keyClass) }, + { CKA_KEY_TYPE, &keyType, sizeof(keyType) }, + }; + CK_OBJECT_HANDLE key = CK_INVALID_HANDLE; + CK_ULONG count = 0; + byte iv[16]; + byte plain[16]; + byte enc[32]; + CK_ULONG encLen = sizeof(enc); + CK_MECHANISM mech = { CKM_AES_CBC, iv, sizeof(iv) }; + + XMEMSET(iv, 0, sizeof(iv)); + XMEMSET(plain, 0, sizeof(plain)); + rv = funcList->C_FindObjectsInit(session, tmpl, + sizeof(tmpl) / sizeof(*tmpl)); + if (rv == CKR_OK) { + rv = funcList->C_FindObjects(session, &key, 1, &count); + (void)funcList->C_FindObjectsFinal(session); + } + if (rv == CKR_OK && count != 1) + rv = CKR_OBJECT_HANDLE_INVALID; + if (rv == CKR_OK) + rv = funcList->C_EncryptInit(session, &mech, key); + if (rv == CKR_OK) + rv = funcList->C_Encrypt(session, plain, sizeof(plain), enc, &encLen); + return rv; +} + +/* A logout whose re-encryption of a stored key fails must leave the stored + * record in place, so the next login still decodes it. */ +static int child_login_after_failed_encode(void) +{ + CK_RV rv; + CK_SLOT_ID slot = 0; + CK_SESSION_HANDLE session = CK_INVALID_HANDLE; + int res = CHILD_FAIL; + + if (wolfSSL_SetAllocators(track_malloc, track_free, track_realloc) != 0) + return CHILD_SETUP; + if (pkcs11_load() != CKR_OK) + return CHILD_SETUP; + rv = init_slot(&slot); + if (rv == CKR_OK) + rv = open_session(slot, 1, &session); + if (rv != CKR_OK) + return CHILD_SETUP; + refuseAllocs = 1; + (void)funcList->C_Logout(session); + refuseAllocs = 0; + rv = funcList->C_Login(session, CKU_USER, userPin, sizeof(userPin) - 1); + if (rv == CKR_OK) + rv = use_stored_aes_key(session); + if (rv == CKR_OK) + res = CHILD_PASS; + else + fprintf(stderr, " login or key use after failed encode: 0x%lx\n", + (unsigned long)rv); + (void)funcList->C_Finalize(NULL); + return res; +} + +static void test_login_after_failed_encode(void) +{ + CK_RV rv; + int res = CHILD_SETUP; + + printf("\n--- stored key survives a failed re-encryption at logout ---\n"); + rv = store_objects(create_aes_key); + CHECK_RV(rv, "store AES key", CKR_OK); + if (rv == CKR_OK) + res = run_in_child(child_login_after_failed_encode); + CHECK_TRUE(res == CHILD_PASS, "login decodes the key after a failed encode"); + cleanup_test_files(); +} +#endif + static void test_failed_key_decrypt_scrubbed(void) { printf("\n--- secret key storage cleared after failed decrypt ---\n"); @@ -1174,6 +1259,9 @@ int main(int argc, char* argv[]) #ifdef HAVE_ALLOC_TRACKING test_failed_decrypt_scrubbed(); test_failed_key_decrypt_scrubbed(); +#ifndef NO_AES + test_login_after_failed_encode(); +#endif #endif return pkcs11_test_summary();