From 54fb588d624c131e117113f3113adaebbeb6f188 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Fri, 20 Mar 2026 07:20:49 +0000 Subject: [PATCH 01/13] Remove wrong `#ifndef` F-817 --- src/slot.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/slot.c b/src/slot.c index edbaf6c8..9325d94d 100644 --- a/src/slot.c +++ b/src/slot.c @@ -579,12 +579,10 @@ static CK_MECHANISM_INFO rsaPssMechInfo = { 1024, 4096, CKF_SIGN | CKF_VERIFY }; #endif -#ifndef NO_SHA256 static CK_MECHANISM_INFO shaRsaPkcsMechInfo = { 1024, 4096, CKF_SIGN | CKF_VERIFY }; #endif -#endif #ifdef HAVE_ECC /* Info on EC key generation mechanism. */ static CK_MECHANISM_INFO ecKgMechInfo = { From 1de249231f8c9209205683d10a1379e02b123fef Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Fri, 20 Mar 2026 07:25:31 +0000 Subject: [PATCH 02/13] Add too short decryption protection If ciphertext is too short, it could cause an excessive malloc. F-312 --- src/crypto.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/crypto.c b/src/crypto.c index aa002a2e..c0924b02 100644 --- a/src/crypto.c +++ b/src/crypto.c @@ -3209,6 +3209,8 @@ CK_RV C_Decrypt(CK_SESSION_HANDLE hSession, CK_BYTE_PTR pEncryptedData, if (!WP11_Session_IsOpInitialized(session, WP11_INIT_AES_GCM_DEC)) return CKR_OPERATION_NOT_INITIALIZED; + if (ulEncryptedDataLen < (CK_ULONG)WP11_AesGcm_GetTagBits(session) / 8) + return CKR_ENCRYPTED_DATA_LEN_RANGE; decDataLen = (word32)ulEncryptedDataLen - WP11_AesGcm_GetTagBits(session) / 8; if (pData == NULL) { @@ -3230,6 +3232,8 @@ CK_RV C_Decrypt(CK_SESSION_HANDLE hSession, CK_BYTE_PTR pEncryptedData, if (!WP11_Session_IsOpInitialized(session, WP11_INIT_AES_CCM_DEC)) return CKR_OPERATION_NOT_INITIALIZED; + if (ulEncryptedDataLen < (CK_ULONG)WP11_AesCcm_GetMacLen(session)) + return CKR_ENCRYPTED_DATA_LEN_RANGE; decDataLen = (word32)ulEncryptedDataLen - WP11_AesCcm_GetMacLen(session); if (pData == NULL) { @@ -3297,6 +3301,8 @@ CK_RV C_Decrypt(CK_SESSION_HANDLE hSession, CK_BYTE_PTR pEncryptedData, /* AES Key Wrap unwrapping reduces the size by 8 bytes (the * integrity check value). If using padding then its even smaller * but we can't know the final size without decrypting first. */ + if (ulEncryptedDataLen < KEYWRAP_BLOCK_SIZE) + return CKR_ENCRYPTED_DATA_LEN_RANGE; decDataLen = (word32)(ulEncryptedDataLen - KEYWRAP_BLOCK_SIZE); if (pData == NULL) { *pulDataLen = decDataLen; @@ -3623,6 +3629,9 @@ CK_RV C_DecryptFinal(CK_SESSION_HANDLE hSession, CK_BYTE_PTR pLastPart, if (!WP11_Session_IsOpInitialized(session, WP11_INIT_AES_GCM_DEC)) return CKR_OPERATION_NOT_INITIALIZED; + if (WP11_AesGcm_EncDataLen(session) < + WP11_AesGcm_GetTagBits(session) / 8) + return CKR_ENCRYPTED_DATA_LEN_RANGE; decPartLen = WP11_AesGcm_EncDataLen(session) - WP11_AesGcm_GetTagBits(session) / 8; if (pLastPart == NULL) { From 06439030146f51bb04c015ba3d957a5af58e97f0 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Fri, 20 Mar 2026 07:31:37 +0000 Subject: [PATCH 03/13] Use ForceZero instead of XMEMSET F-313 --- src/internal.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/internal.c b/src/internal.c index f9d574e3..b5be8ba3 100644 --- a/src/internal.c +++ b/src/internal.c @@ -8285,8 +8285,7 @@ void WP11_Object_Free(WP11_Object* object) #endif if ((object->type == CKK_AES || object->type == CKK_GENERIC_SECRET || object->type == CKK_HKDF) && object->data.symmKey != NULL) { - /* TODO: ForceZero */ - XMEMSET(object->data.symmKey->data, 0, object->data.symmKey->len); + ForceZero(object->data.symmKey->data, object->data.symmKey->len); XFREE(object->data.symmKey, NULL, DYNAMIC_TYPE_AES); object->data.symmKey = NULL; } From d43ba514aa0cb62f57d98a402e7ea07839b3d87e Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Fri, 20 Mar 2026 07:33:53 +0000 Subject: [PATCH 04/13] Clear key buffers before free F-314 --- src/internal.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/internal.c b/src/internal.c index b5be8ba3..8654ea9d 100644 --- a/src/internal.c +++ b/src/internal.c @@ -2738,7 +2738,10 @@ int WP11_Object_Copy(WP11_Object *src, WP11_Object *dest) } } - XFREE(derBuf, NULL, DYNAMIC_TYPE_TMP_BUFFER); + if (derBuf != NULL) { + ForceZero(derBuf, derSz); + XFREE(derBuf, NULL, DYNAMIC_TYPE_TMP_BUFFER); + } /* Free destination key on failure */ if (ret != 0) { @@ -2811,7 +2814,7 @@ int WP11_Object_Copy(WP11_Object *src, WP11_Object *dest) /* Clean up */ if (derBuf != NULL) { - XMEMSET(derBuf, 0, derSz); /* Clear sensitive data */ + ForceZero(derBuf, derSz); XFREE(derBuf, NULL, DYNAMIC_TYPE_TMP_BUFFER); } From b1c5a1130773da64a1661bb25d850926b8b60c8e Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Fri, 20 Mar 2026 07:55:03 +0000 Subject: [PATCH 05/13] Fix ForceZero and use constant time Previous commit to use ForceZero() didn't work, it should be wc_ForceZero(). Also, the AES key wrap padding should be constant time. F-497 --- src/crypto.c | 29 ++++++++++++++++++++++++----- src/internal.c | 6 +++--- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/crypto.c b/src/crypto.c index c0924b02..f83e92ce 100644 --- a/src/crypto.c +++ b/src/crypto.c @@ -3318,12 +3318,31 @@ CK_RV C_Decrypt(CK_SESSION_HANDLE hSession, CK_BYTE_PTR pEncryptedData, if (mechanism == CKM_AES_KEY_WRAP_PAD) { int i; byte padValue = pData[decDataLen - 1]; - if (padValue > KEYWRAP_BLOCK_SIZE || padValue > decDataLen) - return CKR_ENCRYPTED_DATA_LEN_RANGE; - for (i = 0; i < padValue; i++) { - if (pData[decDataLen - 1 - i] != padValue) - return CKR_ENCRYPTED_DATA_INVALID; + unsigned int badPad = 0; + unsigned int inPad; + + /* Constant-time range check: padValue must be 1..KEYWRAP_BLOCK_SIZE + * and must not exceed decDataLen. */ + badPad |= ((unsigned int)padValue - 1) >> 31; + badPad |= ((unsigned int)KEYWRAP_BLOCK_SIZE - + (unsigned int)padValue) >> 31; + badPad |= ((unsigned int)decDataLen - + (unsigned int)padValue) >> 31; + + /* Constant-time padding byte verification. + * Always iterate KEYWRAP_BLOCK_SIZE times to avoid leaking + * padValue through iteration count. */ + for (i = 0; i < KEYWRAP_BLOCK_SIZE; i++) { + /* Full mask: all-ones when i < padValue, else 0 */ + inPad = 0 - (((unsigned int)i - + (unsigned int)padValue) >> 31); + badPad |= inPad & + ((unsigned int)pData[decDataLen - 1 - i] ^ + (unsigned int)padValue); } + + if (badPad != 0) + return CKR_ENCRYPTED_DATA_INVALID; decDataLen -= padValue; } *pulDataLen = decDataLen; diff --git a/src/internal.c b/src/internal.c index 8654ea9d..36de277a 100644 --- a/src/internal.c +++ b/src/internal.c @@ -2739,7 +2739,7 @@ int WP11_Object_Copy(WP11_Object *src, WP11_Object *dest) } if (derBuf != NULL) { - ForceZero(derBuf, derSz); + wc_ForceZero(derBuf, derSz); XFREE(derBuf, NULL, DYNAMIC_TYPE_TMP_BUFFER); } @@ -2814,7 +2814,7 @@ int WP11_Object_Copy(WP11_Object *src, WP11_Object *dest) /* Clean up */ if (derBuf != NULL) { - ForceZero(derBuf, derSz); + wc_ForceZero(derBuf, derSz); XFREE(derBuf, NULL, DYNAMIC_TYPE_TMP_BUFFER); } @@ -8288,7 +8288,7 @@ void WP11_Object_Free(WP11_Object* object) #endif if ((object->type == CKK_AES || object->type == CKK_GENERIC_SECRET || object->type == CKK_HKDF) && object->data.symmKey != NULL) { - ForceZero(object->data.symmKey->data, object->data.symmKey->len); + wc_ForceZero(object->data.symmKey->data, object->data.symmKey->len); XFREE(object->data.symmKey, NULL, DYNAMIC_TYPE_AES); object->data.symmKey = NULL; } From 0f35bc85d121f29fcd66a2197f98285c8d53c9cc Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Fri, 20 Mar 2026 08:43:51 +0000 Subject: [PATCH 06/13] Set CCM dataSz after buffer is cleared, not before F-820 --- src/internal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/internal.c b/src/internal.c index 36de277a..582fb6d2 100644 --- a/src/internal.c +++ b/src/internal.c @@ -7822,8 +7822,8 @@ int WP11_Session_SetCcmParams(WP11_Session* session, int dataSz, ret = BAD_FUNC_ARG; if (ret == 0) { - ccm->dataSz = dataSz; XMEMSET(ccm, 0, sizeof(*ccm)); + ccm->dataSz = dataSz; XMEMCPY(ccm->iv, iv, ivSz); ccm->ivSz = ivSz; if (aad != NULL) { From 87aace100337eb77c5d10c841bc21ccb1b8c4b94 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Fri, 20 Mar 2026 09:00:48 +0000 Subject: [PATCH 07/13] Validate PKCS#7 padding bytes in AES-CBC-PAD decrypt WP11_AesCbcPad_DecryptFinal only checked the last byte as the pad count without verifying all padding bytes matched. This allowed tampered ciphertext to decrypt without error. Add constant-time validation that padCnt is 1..AES_BLOCK_SIZE and all padding bytes equal padCnt, returning BAD_PADDING_E on failure. Add test exercising tampered ciphertext in both one-shot and multi-part decrypt paths. F-821 --- src/internal.c | 19 ++ tests/aes_cbc_pad_padding_test.c | 548 +++++++++++++++++++++++++++++++ tests/include.am | 7 + 3 files changed, 574 insertions(+) create mode 100644 tests/aes_cbc_pad_padding_test.c diff --git a/src/internal.c b/src/internal.c index 582fb6d2..53b79d1e 100644 --- a/src/internal.c +++ b/src/internal.c @@ -12993,7 +12993,26 @@ int WP11_AesCbcPad_DecryptFinal(unsigned char* dec, word32* decSz, ret = wc_AesCbcDecrypt(&cbc->aes, cbc->partial, cbc->partial, cbc->partialSz); if (ret == 0) { + byte padBad; + padCnt = cbc->partial[AES_BLOCK_SIZE-1]; + + /* Validate PKCS#7 padding in constant time: + * padCnt must be 1..AES_BLOCK_SIZE and all padding bytes must equal + * padCnt. */ + padBad = (byte)(0 - (padCnt == 0)); + padBad |= (byte)(0 - (padCnt > AES_BLOCK_SIZE)); + for (i = 0; i < AES_BLOCK_SIZE; i++) { + /* inPad is 0xFF when i is in the padding region, 0x00 otherwise */ + byte inPad = (byte)(0 - + ((unsigned)(AES_BLOCK_SIZE - 1 - i) < (unsigned)padCnt)); + padBad |= inPad & (cbc->partial[i] ^ padCnt); + } + if (padBad) { + ret = BAD_PADDING_E; + } + } + if (ret == 0) { outSz = AES_BLOCK_SIZE - (padCnt & (0 - (padCnt <= AES_BLOCK_SIZE))); for (i = 0; i < AES_BLOCK_SIZE; i++) { mask = (size_t)0 - (i != outSz); diff --git a/tests/aes_cbc_pad_padding_test.c b/tests/aes_cbc_pad_padding_test.c new file mode 100644 index 00000000..ccd3d3c4 --- /dev/null +++ b/tests/aes_cbc_pad_padding_test.c @@ -0,0 +1,548 @@ +/* aes_cbc_pad_padding_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 + * + * Test for AES-CBC-PAD PKCS#7 padding validation (bug #821). + * + * WP11_AesCbcPad_DecryptFinal does not validate that all padding bytes equal + * the pad count. Tampered ciphertext decrypts without error, silently returning + * incorrect plaintext. + */ + +#ifdef HAVE_CONFIG_H + #include +#endif + +#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" + +#if !defined(NO_AES) && !defined(NO_AES_CBC) + +#define CBC_PAD_TEST_DIR "./store/cbc_pad_padding_test" +#define WOLFPKCS11_TOKEN_FILENAME "wp11_token_0000000000000001" + +static int test_passed = 0; +static int test_failed = 0; + +#define CHECK_CKR(rv, op, expected) do { \ + if (rv != expected) { \ + fprintf(stderr, "FAIL: %s: expected %ld, got %ld\n", op, (long)expected, (long)rv); \ + test_failed++; \ + result = -1; \ + goto cleanup; \ + } else { \ + printf("PASS: %s\n", op); \ + test_passed++; \ + } \ +} while(0) + +#ifndef HAVE_PKCS11_STATIC +static void* dlib; +#endif +static CK_FUNCTION_LIST* funcList; +static CK_SLOT_ID slot = 0; +static const char* tokenName = "wolfpkcs11"; +static byte* soPin = (byte*)"password123456"; +static int soPinLen = 14; +static byte* userPin = (byte*)"someUserPin"; +static int userPinLen = 11; + +static CK_OBJECT_CLASS secretKeyClass = CKO_SECRET_KEY; +static CK_BBOOL ckTrue = CK_TRUE; +static CK_KEY_TYPE aesKeyType = CKK_AES; + +static CK_RV pkcs11_init(void) +{ + CK_RV ret; + CK_C_INITIALIZE_ARGS args; + CK_INFO info; + CK_SLOT_ID slotList[16]; + CK_ULONG slotCount = sizeof(slotList) / sizeof(slotList[0]); + +#ifndef HAVE_PKCS11_STATIC + CK_C_GetFunctionList func; + + dlib = dlopen(WOLFPKCS11_DLL_FILENAME, RTLD_NOW | RTLD_LOCAL); + if (dlib == NULL) { + fprintf(stderr, "dlopen error: %s\n", dlerror()); + return -1; + } + + func = (CK_C_GetFunctionList)dlsym(dlib, "C_GetFunctionList"); + if (func == NULL) { + fprintf(stderr, "Failed to get function list function\n"); + dlclose(dlib); + return -1; + } + + ret = func(&funcList); + if (ret != CKR_OK) { + fprintf(stderr, "Failed to get function list: 0x%lx\n", + (unsigned long)ret); + dlclose(dlib); + return ret; + } +#else + ret = C_GetFunctionList(&funcList); + if (ret != CKR_OK) { + fprintf(stderr, "Failed to get function list: 0x%lx\n", + (unsigned long)ret); + return ret; + } +#endif + + XMEMSET(&args, 0, sizeof(args)); + args.flags = CKF_OS_LOCKING_OK; + ret = funcList->C_Initialize(&args); + if (ret != CKR_OK) + return ret; + + ret = funcList->C_GetInfo(&info); + if (ret != CKR_OK) + return ret; + + ret = funcList->C_GetSlotList(CK_TRUE, slotList, &slotCount); + if (ret != CKR_OK) + return ret; + + if (slotCount > 0) { + slot = slotList[0]; + } else { + fprintf(stderr, "No slots available\n"); + return CKR_GENERAL_ERROR; + } + + return ret; +} + +static CK_RV pkcs11_final(void) +{ + if (funcList != NULL) { + funcList->C_Finalize(NULL); + funcList = NULL; + } +#ifndef HAVE_PKCS11_STATIC + if (dlib) { + dlclose(dlib); + dlib = NULL; + } +#endif + return CKR_OK; +} + +static CK_RV pkcs11_init_token(void) +{ + unsigned char label[32]; + + XMEMSET(label, ' ', sizeof(label)); + XMEMCPY(label, tokenName, XSTRLEN(tokenName)); + + return funcList->C_InitToken(slot, soPin, soPinLen, label); +} + +static CK_RV pkcs11_open_session(CK_SESSION_HANDLE* session) +{ + CK_RV ret; + int sessFlags = CKF_SERIAL_SESSION | CKF_RW_SESSION; + + ret = funcList->C_OpenSession(slot, sessFlags, NULL, NULL, session); + if (ret != CKR_OK) + return ret; + + ret = funcList->C_Login(*session, CKU_USER, userPin, userPinLen); + if (ret != CKR_OK) { + funcList->C_CloseSession(*session); + return ret; + } + + return CKR_OK; +} + +static CK_RV pkcs11_close_session(CK_SESSION_HANDLE session) +{ + funcList->C_Logout(session); + return funcList->C_CloseSession(session); +} + +static void cleanup_test_files(const char* dir) +{ + char filepath[512]; + + snprintf(filepath, sizeof(filepath), "%s" PATH_SEP "%s", dir, + WOLFPKCS11_TOKEN_FILENAME); + (void)remove(filepath); +} + +static CK_RV create_aes_128_key(CK_SESSION_HANDLE session, + CK_OBJECT_HANDLE* key) +{ + CK_ATTRIBUTE tmpl[] = { + { CKA_CLASS, &secretKeyClass, sizeof(secretKeyClass) }, + { CKA_KEY_TYPE, &aesKeyType, sizeof(aesKeyType) }, + { CKA_ENCRYPT, &ckTrue, sizeof(ckTrue) }, + { CKA_DECRYPT, &ckTrue, sizeof(ckTrue) }, + { CKA_VALUE, aes_128_key, sizeof(aes_128_key) }, + { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, + }; + CK_ULONG tmplCnt = sizeof(tmpl) / sizeof(*tmpl); + + return funcList->C_CreateObject(session, tmpl, tmplCnt, key); +} + +/* + * Test 1: Valid encrypt/decrypt roundtrip (baseline). + * 20-byte plaintext -> 32 bytes ciphertext (2 blocks, 12 bytes padding). + */ +static int test_valid_roundtrip(CK_SESSION_HANDLE session, + CK_OBJECT_HANDLE key, + unsigned char* cipherOut, + CK_ULONG* cipherOutLen) +{ + CK_RV ret; + CK_MECHANISM mech; + byte plain[20], dec[32], iv[16]; + CK_ULONG encSz, decSz; + int result = 0; + + memset(plain, 9, sizeof(plain)); + memset(iv, 9, sizeof(iv)); + + mech.mechanism = CKM_AES_CBC_PAD; + mech.ulParameterLen = sizeof(iv); + mech.pParameter = iv; + + /* Encrypt */ + ret = funcList->C_EncryptInit(session, &mech, key); + CHECK_CKR(ret, "Test1: C_EncryptInit", CKR_OK); + + encSz = *cipherOutLen; + ret = funcList->C_Encrypt(session, plain, sizeof(plain), cipherOut, &encSz); + CHECK_CKR(ret, "Test1: C_Encrypt", CKR_OK); + + if (encSz != 32) { + fprintf(stderr, "FAIL: Test1: expected 32 bytes ciphertext, got %lu\n", + (unsigned long)encSz); + test_failed++; + result = -1; + goto cleanup; + } + printf("PASS: Test1: ciphertext length is 32\n"); + test_passed++; + *cipherOutLen = encSz; + + /* Decrypt */ + ret = funcList->C_DecryptInit(session, &mech, key); + CHECK_CKR(ret, "Test1: C_DecryptInit", CKR_OK); + + decSz = sizeof(dec); + ret = funcList->C_Decrypt(session, cipherOut, encSz, dec, &decSz); + CHECK_CKR(ret, "Test1: C_Decrypt", CKR_OK); + + if (decSz != sizeof(plain) || memcmp(dec, plain, sizeof(plain)) != 0) { + fprintf(stderr, "FAIL: Test1: decrypted plaintext mismatch\n"); + test_failed++; + result = -1; + goto cleanup; + } + printf("PASS: Test1: plaintext roundtrip matches\n"); + test_passed++; + +cleanup: + return result; +} + +/* + * Test 2: Tamper last byte of ciphertext, one-shot C_Decrypt. + * Flipping a bit in the last block produces garbage padding after decryption. + * Expected: CKR_ENCRYPTED_DATA_INVALID. + */ +static int test_tampered_last_byte_oneshot(CK_SESSION_HANDLE session, + CK_OBJECT_HANDLE key, + unsigned char* cipher, + CK_ULONG cipherLen) +{ + CK_RV ret; + CK_MECHANISM mech; + byte dec[32], iv[16], tampered[32]; + CK_ULONG decSz; + int result = 0; + + memset(iv, 9, sizeof(iv)); + memcpy(tampered, cipher, cipherLen); + tampered[cipherLen - 1] ^= 0x01; /* flip one bit in last byte */ + + mech.mechanism = CKM_AES_CBC_PAD; + mech.ulParameterLen = sizeof(iv); + mech.pParameter = iv; + + ret = funcList->C_DecryptInit(session, &mech, key); + CHECK_CKR(ret, "Test2: C_DecryptInit", CKR_OK); + + decSz = sizeof(dec); + ret = funcList->C_Decrypt(session, tampered, cipherLen, dec, &decSz); + CHECK_CKR(ret, "Test2: C_Decrypt tampered last byte", + CKR_ENCRYPTED_DATA_INVALID); + +cleanup: + return result; +} + +/* + * Test 3: Tamper last byte of ciphertext, multi-part C_DecryptUpdate/Final. + * Expected: C_DecryptFinal returns CKR_FUNCTION_FAILED. + */ +static int test_tampered_last_byte_multipart(CK_SESSION_HANDLE session, + CK_OBJECT_HANDLE key, + unsigned char* cipher, + CK_ULONG cipherLen) +{ + CK_RV ret; + CK_MECHANISM mech; + byte dec[48], iv[16], tampered[32], lastPart[16]; + CK_ULONG decSz, lastPartLen; + int result = 0; + + memset(iv, 9, sizeof(iv)); + memcpy(tampered, cipher, cipherLen); + tampered[cipherLen - 1] ^= 0x01; + + mech.mechanism = CKM_AES_CBC_PAD; + mech.ulParameterLen = sizeof(iv); + mech.pParameter = iv; + + ret = funcList->C_DecryptInit(session, &mech, key); + CHECK_CKR(ret, "Test3: C_DecryptInit", CKR_OK); + + decSz = sizeof(dec); + ret = funcList->C_DecryptUpdate(session, tampered, cipherLen, dec, &decSz); + CHECK_CKR(ret, "Test3: C_DecryptUpdate", CKR_OK); + + lastPartLen = sizeof(lastPart); + ret = funcList->C_DecryptFinal(session, lastPart, &lastPartLen); + CHECK_CKR(ret, "Test3: C_DecryptFinal tampered last byte", + CKR_FUNCTION_FAILED); + +cleanup: + return result; +} + +/* + * Test 4: Tamper last byte of first block, one-shot C_Decrypt. + * Corrupting the first block changes the CBC chain for the second (padding) + * block, producing invalid padding. + * Expected: CKR_ENCRYPTED_DATA_INVALID. + */ +static int test_tampered_first_block_oneshot(CK_SESSION_HANDLE session, + CK_OBJECT_HANDLE key, + unsigned char* cipher, + CK_ULONG cipherLen) +{ + CK_RV ret; + CK_MECHANISM mech; + byte dec[32], iv[16], tampered[32]; + CK_ULONG decSz; + int result = 0; + + memset(iv, 9, sizeof(iv)); + memcpy(tampered, cipher, cipherLen); + tampered[15] ^= 0x01; /* flip one bit in last byte of first block */ + + mech.mechanism = CKM_AES_CBC_PAD; + mech.ulParameterLen = sizeof(iv); + mech.pParameter = iv; + + ret = funcList->C_DecryptInit(session, &mech, key); + CHECK_CKR(ret, "Test4: C_DecryptInit", CKR_OK); + + decSz = sizeof(dec); + ret = funcList->C_Decrypt(session, tampered, cipherLen, dec, &decSz); + CHECK_CKR(ret, "Test4: C_Decrypt tampered first block", + CKR_ENCRYPTED_DATA_INVALID); + +cleanup: + return result; +} + +static int aes_cbc_pad_padding_test(void) +{ + CK_RV ret; + CK_SESSION_HANDLE session = 0; + CK_OBJECT_HANDLE key; + unsigned char cipher[48]; + CK_ULONG cipherLen; + int result = 0; + + printf("\n=== Testing AES-CBC-PAD padding validation ===\n"); + + cleanup_test_files(CBC_PAD_TEST_DIR); + + ret = pkcs11_init(); + if (ret != CKR_OK) { + fprintf(stderr, "FAIL: pkcs11_init: 0x%lx\n", (unsigned long)ret); + test_failed++; + return -1; + } + + ret = pkcs11_init_token(); + if (ret != CKR_OK) { + fprintf(stderr, "FAIL: C_InitToken: 0x%lx\n", (unsigned long)ret); + test_failed++; + pkcs11_final(); + return -1; + } + + /* Set user PIN via SO session */ + { + CK_SESSION_HANDLE soSession; + int sessFlags = CKF_SERIAL_SESSION | CKF_RW_SESSION; + + ret = funcList->C_OpenSession(slot, sessFlags, NULL, NULL, &soSession); + if (ret != CKR_OK) { + fprintf(stderr, "FAIL: C_OpenSession (SO): 0x%lx\n", + (unsigned long)ret); + test_failed++; + pkcs11_final(); + return -1; + } + + ret = funcList->C_Login(soSession, CKU_SO, soPin, soPinLen); + if (ret != CKR_OK) { + fprintf(stderr, "FAIL: C_Login (SO): 0x%lx\n", + (unsigned long)ret); + test_failed++; + funcList->C_CloseSession(soSession); + pkcs11_final(); + return -1; + } + + ret = funcList->C_InitPIN(soSession, userPin, userPinLen); + if (ret != CKR_OK) { + fprintf(stderr, "FAIL: C_InitPIN: 0x%lx\n", (unsigned long)ret); + test_failed++; + funcList->C_Logout(soSession); + funcList->C_CloseSession(soSession); + pkcs11_final(); + return -1; + } + + funcList->C_Logout(soSession); + funcList->C_CloseSession(soSession); + } + + ret = pkcs11_open_session(&session); + if (ret != CKR_OK) { + fprintf(stderr, "FAIL: pkcs11_open_session: 0x%lx\n", + (unsigned long)ret); + test_failed++; + pkcs11_final(); + return -1; + } + + ret = create_aes_128_key(session, &key); + if (ret != CKR_OK) { + fprintf(stderr, "FAIL: create_aes_128_key: 0x%lx\n", + (unsigned long)ret); + test_failed++; + pkcs11_close_session(session); + pkcs11_final(); + return -1; + } + + /* Test 1: Valid roundtrip — also produces ciphertext for tamper tests */ + cipherLen = sizeof(cipher); + if (test_valid_roundtrip(session, key, cipher, &cipherLen) != 0) + result = -1; + + /* Tests 2-4 only run if test 1 produced valid ciphertext */ + if (result == 0) { + if (test_tampered_last_byte_oneshot(session, key, cipher, + cipherLen) != 0) + result = -1; + if (test_tampered_last_byte_multipart(session, key, cipher, + cipherLen) != 0) + result = -1; + if (test_tampered_first_block_oneshot(session, key, cipher, + cipherLen) != 0) + result = -1; + } + + pkcs11_close_session(session); + pkcs11_final(); + return result; +} + +static void print_results(void) +{ + printf("\n=== Test Results ===\n"); + printf("Tests passed: %d\n", test_passed); + printf("Tests failed: %d\n", test_failed); + + if (test_failed == 0) { + printf("ALL TESTS PASSED!\n"); + } else { + printf("SOME TESTS FAILED!\n"); + } +} + +int main(int argc, char* argv[]) +{ +#ifndef WOLFPKCS11_NO_ENV + XSETENV("WOLFPKCS11_TOKEN_PATH", CBC_PAD_TEST_DIR, 1); +#endif + + (void)argc; + (void)argv; + + printf("=== wolfPKCS11 AES-CBC-PAD Padding Validation Test ===\n"); + + (void)aes_cbc_pad_padding_test(); + + print_results(); + + return (test_failed == 0) ? 0 : 1; +} + +#else /* NO_AES || NO_AES_CBC */ + +int main(int argc, char* argv[]) +{ + (void)argc; + (void)argv; + + printf("AES-CBC not available, skipping padding validation test\n"); + return 0; +} + +#endif /* !NO_AES && !NO_AES_CBC */ diff --git a/tests/include.am b/tests/include.am index 857e4e12..60a86454 100644 --- a/tests/include.am +++ b/tests/include.am @@ -46,6 +46,11 @@ noinst_PROGRAMS += tests/find_objects_null_template_test tests_find_objects_null_template_test_SOURCES = tests/find_objects_null_template_test.c tests_find_objects_null_template_test_LDADD = +check_PROGRAMS += tests/aes_cbc_pad_padding_test +noinst_PROGRAMS += tests/aes_cbc_pad_padding_test +tests_aes_cbc_pad_padding_test_SOURCES = tests/aes_cbc_pad_padding_test.c +tests_aes_cbc_pad_padding_test_LDADD = + check_PROGRAMS += tests/pkcs11v3test noinst_PROGRAMS += tests/pkcs11v3test tests_pkcs11v3test_SOURCES = tests/pkcs11v3test.c @@ -61,11 +66,13 @@ tests_debug_test_LDADD += src/libwolfpkcs11.la tests_object_id_uniqueness_test_LDADD += src/libwolfpkcs11.la tests_empty_pin_store_test_LDADD += src/libwolfpkcs11.la tests_find_objects_null_template_test_LDADD += src/libwolfpkcs11.la +tests_aes_cbc_pad_padding_test_LDADD += src/libwolfpkcs11.la tests_pkcs11v3test_LDADD += src/libwolfpkcs11.la else tests_object_id_uniqueness_test_LDADD += src/libwolfpkcs11.la tests_empty_pin_store_test_LDADD += src/libwolfpkcs11.la tests_find_objects_null_template_test_LDADD += src/libwolfpkcs11.la +tests_aes_cbc_pad_padding_test_LDADD += src/libwolfpkcs11.la endif EXTRA_DIST += tests/unit.h \ From d8b35070ea2fe4302983bd76901418d67eb4c780 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Fri, 20 Mar 2026 09:02:59 +0000 Subject: [PATCH 08/13] Fix declaration of CKM_SHA1_RSA_PKCS_PSS F-494 --- src/slot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/slot.c b/src/slot.c index 9325d94d..bf99f8f0 100644 --- a/src/slot.c +++ b/src/slot.c @@ -328,7 +328,7 @@ static CK_MECHANISM_TYPE mechanismList[] = { #ifdef WC_RSA_PSS CKM_RSA_PKCS_PSS, #ifndef NO_SHA - CKM_SHA1_RSA_PKCS, + CKM_SHA1_RSA_PKCS_PSS, #endif #ifdef WOLFSSL_SHA224 CKM_SHA224_RSA_PKCS_PSS, From d3d6d259f0deb7b9d9ebf1ec70053f477e51117d Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Fri, 20 Mar 2026 09:09:14 +0000 Subject: [PATCH 09/13] Use XMEMCPY instead of memcpy F-495 --- src/wolfpkcs11.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wolfpkcs11.c b/src/wolfpkcs11.c index 71f276ac..01b64124 100644 --- a/src/wolfpkcs11.c +++ b/src/wolfpkcs11.c @@ -448,7 +448,7 @@ CK_RV C_GetInterfaceList(CK_INTERFACE_PTR pInterfacesList, CK_ULONG_PTR pulCount return CKR_BUFFER_TOO_SMALL; } - memcpy(pInterfacesList, interfaces, NUM_INTERFACES * sizeof(CK_INTERFACE)); + XMEMCPY(pInterfacesList, interfaces, NUM_INTERFACES * sizeof(CK_INTERFACE)); *pulCount = NUM_INTERFACES; return CKR_OK; From 5a8373963b1cf4c2937fb80b56def53e27824f83 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Fri, 20 Mar 2026 09:22:15 +0000 Subject: [PATCH 10/13] Fix AES-ECB encrypt failure error handling AES-ECB would return `CKR_OK` when encryption failed. Instead it should return an appropriate error. F-496 --- src/internal.c | 3 + tests/ecb_check_value_error_test.c | 449 +++++++++++++++++++++++++++++ tests/include.am | 7 + 3 files changed, 459 insertions(+) create mode 100644 tests/ecb_check_value_error_test.c diff --git a/src/internal.c b/src/internal.c index 53b79d1e..7e00c90b 100644 --- a/src/internal.c +++ b/src/internal.c @@ -9933,6 +9933,9 @@ static int GetEcbCheckValue(WP11_Object* secret, byte* dataOut, XFREE(hash, NULL, DYNAMIC_TYPE_TMP_BUFFER); XFREE(input, NULL, DYNAMIC_TYPE_TMP_BUFFER); + if (ret != 0) + return CKR_FUNCTION_FAILED; + return CKR_OK; } #endif diff --git a/tests/ecb_check_value_error_test.c b/tests/ecb_check_value_error_test.c new file mode 100644 index 00000000..fc564472 --- /dev/null +++ b/tests/ecb_check_value_error_test.c @@ -0,0 +1,449 @@ +/* ecb_check_value_error_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 + * + * Test for GetEcbCheckValue error propagation (bug #496). + * + * GetEcbCheckValue always returns CKR_OK even when WP11_AesEcb_Encrypt fails. + * A generic secret key with an invalid AES key length (e.g. 5 bytes) causes + * the encrypt to fail, but C_GetAttributeValue still returns CKR_OK with + * uninitialized output. + */ + +#ifdef HAVE_CONFIG_H + #include +#endif + +#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" + +#ifdef HAVE_AESECB + +#define ECB_CV_TEST_DIR "./store/ecb_check_value_error_test" +#define WOLFPKCS11_TOKEN_FILENAME "wp11_token_0000000000000001" + +static int test_passed = 0; +static int test_failed = 0; + +#define CHECK_CKR(rv, op, expected) do { \ + if (rv != expected) { \ + fprintf(stderr, "FAIL: %s: expected %ld, got %ld\n", op, (long)expected, (long)rv); \ + test_failed++; \ + result = -1; \ + goto cleanup; \ + } else { \ + printf("PASS: %s\n", op); \ + test_passed++; \ + } \ +} while(0) + +#ifndef HAVE_PKCS11_STATIC +static void* dlib; +#endif +static CK_FUNCTION_LIST* funcList; +static CK_SLOT_ID slot = 0; +static const char* tokenName = "wolfpkcs11"; +static byte* soPin = (byte*)"password123456"; +static int soPinLen = 14; +static byte* userPin = (byte*)"someUserPin"; +static int userPinLen = 11; + +static CK_OBJECT_CLASS secretKeyClass = CKO_SECRET_KEY; +static CK_BBOOL ckTrue = CK_TRUE; + +static CK_RV pkcs11_init(void) +{ + CK_RV ret; + CK_C_INITIALIZE_ARGS args; + CK_INFO info; + CK_SLOT_ID slotList[16]; + CK_ULONG slotCount = sizeof(slotList) / sizeof(slotList[0]); + +#ifndef HAVE_PKCS11_STATIC + CK_C_GetFunctionList func; + + dlib = dlopen(WOLFPKCS11_DLL_FILENAME, RTLD_NOW | RTLD_LOCAL); + if (dlib == NULL) { + fprintf(stderr, "dlopen error: %s\n", dlerror()); + return -1; + } + + func = (CK_C_GetFunctionList)dlsym(dlib, "C_GetFunctionList"); + if (func == NULL) { + fprintf(stderr, "Failed to get function list function\n"); + dlclose(dlib); + return -1; + } + + ret = func(&funcList); + if (ret != CKR_OK) { + fprintf(stderr, "Failed to get function list: 0x%lx\n", + (unsigned long)ret); + dlclose(dlib); + return ret; + } +#else + ret = C_GetFunctionList(&funcList); + if (ret != CKR_OK) { + fprintf(stderr, "Failed to get function list: 0x%lx\n", + (unsigned long)ret); + return ret; + } +#endif + + XMEMSET(&args, 0, sizeof(args)); + args.flags = CKF_OS_LOCKING_OK; + ret = funcList->C_Initialize(&args); + if (ret != CKR_OK) + return ret; + + ret = funcList->C_GetInfo(&info); + if (ret != CKR_OK) + return ret; + + ret = funcList->C_GetSlotList(CK_TRUE, slotList, &slotCount); + if (ret != CKR_OK) + return ret; + + if (slotCount > 0) { + slot = slotList[0]; + } else { + fprintf(stderr, "No slots available\n"); + return CKR_GENERAL_ERROR; + } + + return ret; +} + +static CK_RV pkcs11_final(void) +{ + if (funcList != NULL) { + funcList->C_Finalize(NULL); + funcList = NULL; + } +#ifndef HAVE_PKCS11_STATIC + if (dlib) { + dlclose(dlib); + dlib = NULL; + } +#endif + return CKR_OK; +} + +static CK_RV pkcs11_init_token(void) +{ + unsigned char label[32]; + + XMEMSET(label, ' ', sizeof(label)); + XMEMCPY(label, tokenName, XSTRLEN(tokenName)); + + return funcList->C_InitToken(slot, soPin, soPinLen, label); +} + +static CK_RV pkcs11_open_session(CK_SESSION_HANDLE* session) +{ + CK_RV ret; + int sessFlags = CKF_SERIAL_SESSION | CKF_RW_SESSION; + + ret = funcList->C_OpenSession(slot, sessFlags, NULL, NULL, session); + if (ret != CKR_OK) + return ret; + + ret = funcList->C_Login(*session, CKU_USER, userPin, userPinLen); + if (ret != CKR_OK) { + funcList->C_CloseSession(*session); + return ret; + } + + return CKR_OK; +} + +static CK_RV pkcs11_close_session(CK_SESSION_HANDLE session) +{ + funcList->C_Logout(session); + return funcList->C_CloseSession(session); +} + +static void cleanup_test_files(const char* dir) +{ + char filepath[512]; + + snprintf(filepath, sizeof(filepath), "%s" PATH_SEP "%s", dir, + WOLFPKCS11_TOKEN_FILENAME); + (void)remove(filepath); +} + +/* + * Test 1 (positive control): Create a 16-byte CKK_GENERIC_SECRET key and + * query CKA_CHECK_VALUE. Expect CKR_OK and a 3-byte result. + */ +static int test_valid_key_check_value(CK_SESSION_HANDLE session) +{ + CK_RV ret; + CK_OBJECT_HANDLE key; + CK_KEY_TYPE keyType = CKK_GENERIC_SECRET; + unsigned char keyData[16]; + unsigned char checkValue[3]; + CK_ULONG checkLen = sizeof(checkValue); + int result = 0; + + CK_ATTRIBUTE createTmpl[] = { + { CKA_CLASS, &secretKeyClass, sizeof(secretKeyClass) }, + { CKA_KEY_TYPE, &keyType, sizeof(keyType) }, + { CKA_VALUE, keyData, sizeof(keyData) }, + { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, + }; + CK_ULONG createTmplCnt = sizeof(createTmpl) / sizeof(*createTmpl); + + CK_ATTRIBUTE getTmpl[] = { + { CKA_CHECK_VALUE, checkValue, checkLen }, + }; + + XMEMSET(keyData, 0xAA, sizeof(keyData)); + XMEMSET(checkValue, 0, sizeof(checkValue)); + + ret = funcList->C_CreateObject(session, createTmpl, createTmplCnt, &key); + CHECK_CKR(ret, "Test1: C_CreateObject (16-byte generic secret)", CKR_OK); + + ret = funcList->C_GetAttributeValue(session, key, getTmpl, 1); + CHECK_CKR(ret, "Test1: C_GetAttributeValue CKA_CHECK_VALUE", CKR_OK); + + if (getTmpl[0].ulValueLen != 3) { + fprintf(stderr, + "FAIL: Test1: expected check value length 3, got %lu\n", + (unsigned long)getTmpl[0].ulValueLen); + test_failed++; + result = -1; + goto cleanup; + } + printf("PASS: Test1: check value length is 3\n"); + test_passed++; + + /* Verify not all zeros (encrypt of zero block should produce non-zero) */ + if (checkValue[0] == 0 && checkValue[1] == 0 && checkValue[2] == 0) { + fprintf(stderr, + "FAIL: Test1: check value is all zeros (likely not computed)\n"); + test_failed++; + result = -1; + goto cleanup; + } + printf("PASS: Test1: check value is non-zero: %02x%02x%02x\n", + checkValue[0], checkValue[1], checkValue[2]); + test_passed++; + +cleanup: + return result; +} + +/* + * Test 2 (bug demonstration): Create a 5-byte CKK_GENERIC_SECRET key and + * query CKA_CHECK_VALUE. The 5-byte key is not a valid AES key length, so + * WP11_AesEcb_Encrypt (called by GetEcbCheckValue) fails internally. + * + * BUG #496: GetEcbCheckValue always returns CKR_OK regardless of whether the + * encrypt succeeded. This test expects CKR_OK (proving the bug exists). + * Once the bug is fixed, this test should be updated to expect + * CKR_FUNCTION_FAILED. + */ +static int test_invalid_key_check_value(CK_SESSION_HANDLE session) +{ + CK_RV ret; + CK_OBJECT_HANDLE key; + CK_KEY_TYPE keyType = CKK_GENERIC_SECRET; + unsigned char keyData[5]; + unsigned char checkValue[3]; + CK_ULONG checkLen = sizeof(checkValue); + int result = 0; + + CK_ATTRIBUTE createTmpl[] = { + { CKA_CLASS, &secretKeyClass, sizeof(secretKeyClass) }, + { CKA_KEY_TYPE, &keyType, sizeof(keyType) }, + { CKA_VALUE, keyData, sizeof(keyData) }, + { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, + }; + CK_ULONG createTmplCnt = sizeof(createTmpl) / sizeof(*createTmpl); + + CK_ATTRIBUTE getTmpl[] = { + { CKA_CHECK_VALUE, checkValue, checkLen }, + }; + + XMEMSET(keyData, 0xBB, sizeof(keyData)); + XMEMSET(checkValue, 0, sizeof(checkValue)); + + ret = funcList->C_CreateObject(session, createTmpl, createTmplCnt, &key); + CHECK_CKR(ret, "Test2: C_CreateObject (5-byte generic secret)", CKR_OK); + + ret = funcList->C_GetAttributeValue(session, key, getTmpl, 1); + + /* + * BUG #496: GetEcbCheckValue returns CKR_OK even though + * WP11_AesEcb_Encrypt failed (5 bytes is not a valid AES key length). + * The correct behavior is to return CKR_FUNCTION_FAILED. + */ + CHECK_CKR(ret, "Test2: C_GetAttributeValue CKA_CHECK_VALUE (5-byte key)", + CKR_FUNCTION_FAILED); + +cleanup: + return result; +} + +static int ecb_check_value_error_test(void) +{ + CK_RV ret; + CK_SESSION_HANDLE session = 0; + int result = 0; + + printf("\n=== Testing GetEcbCheckValue error propagation ===\n"); + + cleanup_test_files(ECB_CV_TEST_DIR); + + ret = pkcs11_init(); + if (ret != CKR_OK) { + fprintf(stderr, "FAIL: pkcs11_init: 0x%lx\n", (unsigned long)ret); + test_failed++; + return -1; + } + + ret = pkcs11_init_token(); + if (ret != CKR_OK) { + fprintf(stderr, "FAIL: C_InitToken: 0x%lx\n", (unsigned long)ret); + test_failed++; + pkcs11_final(); + return -1; + } + + /* Set user PIN via SO session */ + { + CK_SESSION_HANDLE soSession; + int sessFlags = CKF_SERIAL_SESSION | CKF_RW_SESSION; + + ret = funcList->C_OpenSession(slot, sessFlags, NULL, NULL, &soSession); + if (ret != CKR_OK) { + fprintf(stderr, "FAIL: C_OpenSession (SO): 0x%lx\n", + (unsigned long)ret); + test_failed++; + pkcs11_final(); + return -1; + } + + ret = funcList->C_Login(soSession, CKU_SO, soPin, soPinLen); + if (ret != CKR_OK) { + fprintf(stderr, "FAIL: C_Login (SO): 0x%lx\n", + (unsigned long)ret); + test_failed++; + funcList->C_CloseSession(soSession); + pkcs11_final(); + return -1; + } + + ret = funcList->C_InitPIN(soSession, userPin, userPinLen); + if (ret != CKR_OK) { + fprintf(stderr, "FAIL: C_InitPIN: 0x%lx\n", (unsigned long)ret); + test_failed++; + funcList->C_Logout(soSession); + funcList->C_CloseSession(soSession); + pkcs11_final(); + return -1; + } + + funcList->C_Logout(soSession); + funcList->C_CloseSession(soSession); + } + + ret = pkcs11_open_session(&session); + if (ret != CKR_OK) { + fprintf(stderr, "FAIL: pkcs11_open_session: 0x%lx\n", + (unsigned long)ret); + test_failed++; + pkcs11_final(); + return -1; + } + + /* Test 1: Valid 16-byte key — positive control */ + if (test_valid_key_check_value(session) != 0) + result = -1; + + /* Test 2: Invalid 5-byte key — demonstrates bug #496 */ + if (test_invalid_key_check_value(session) != 0) + result = -1; + + pkcs11_close_session(session); + pkcs11_final(); + return result; +} + +static void print_results(void) +{ + printf("\n=== Test Results ===\n"); + printf("Tests passed: %d\n", test_passed); + printf("Tests failed: %d\n", test_failed); + + if (test_failed == 0) { + printf("ALL TESTS PASSED!\n"); + } else { + printf("SOME TESTS FAILED!\n"); + } +} + +int main(int argc, char* argv[]) +{ +#ifndef WOLFPKCS11_NO_ENV + XSETENV("WOLFPKCS11_TOKEN_PATH", ECB_CV_TEST_DIR, 1); +#endif + + (void)argc; + (void)argv; + + printf("=== wolfPKCS11 GetEcbCheckValue Error Propagation Test ===\n"); + + (void)ecb_check_value_error_test(); + + print_results(); + + return (test_failed == 0) ? 0 : 1; +} + +#else /* !HAVE_AESECB */ + +int main(int argc, char* argv[]) +{ + (void)argc; + (void)argv; + + printf("AES-ECB not available, skipping GetEcbCheckValue test\n"); + return 0; +} + +#endif /* HAVE_AESECB */ diff --git a/tests/include.am b/tests/include.am index 60a86454..725c474c 100644 --- a/tests/include.am +++ b/tests/include.am @@ -51,6 +51,11 @@ noinst_PROGRAMS += tests/aes_cbc_pad_padding_test tests_aes_cbc_pad_padding_test_SOURCES = tests/aes_cbc_pad_padding_test.c tests_aes_cbc_pad_padding_test_LDADD = +check_PROGRAMS += tests/ecb_check_value_error_test +noinst_PROGRAMS += tests/ecb_check_value_error_test +tests_ecb_check_value_error_test_SOURCES = tests/ecb_check_value_error_test.c +tests_ecb_check_value_error_test_LDADD = + check_PROGRAMS += tests/pkcs11v3test noinst_PROGRAMS += tests/pkcs11v3test tests_pkcs11v3test_SOURCES = tests/pkcs11v3test.c @@ -67,12 +72,14 @@ tests_object_id_uniqueness_test_LDADD += src/libwolfpkcs11.la tests_empty_pin_store_test_LDADD += src/libwolfpkcs11.la tests_find_objects_null_template_test_LDADD += src/libwolfpkcs11.la tests_aes_cbc_pad_padding_test_LDADD += src/libwolfpkcs11.la +tests_ecb_check_value_error_test_LDADD += src/libwolfpkcs11.la tests_pkcs11v3test_LDADD += src/libwolfpkcs11.la else tests_object_id_uniqueness_test_LDADD += src/libwolfpkcs11.la tests_empty_pin_store_test_LDADD += src/libwolfpkcs11.la tests_find_objects_null_template_test_LDADD += src/libwolfpkcs11.la tests_aes_cbc_pad_padding_test_LDADD += src/libwolfpkcs11.la +tests_ecb_check_value_error_test_LDADD += src/libwolfpkcs11.la endif EXTRA_DIST += tests/unit.h \ From c6706d22a1bc91aaaedc8aeb12150cf7712bab54 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Fri, 20 Mar 2026 09:40:13 +0000 Subject: [PATCH 11/13] Add ForceZero for wolfSSL < 5.8.4 --- src/internal.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/internal.c b/src/internal.c index 7e00c90b..42e8c4df 100644 --- a/src/internal.c +++ b/src/internal.c @@ -102,6 +102,17 @@ #error "wolfTPM and MAXQ10XX are incompatible with each other." #endif +/* wc_ForceZero was added in wolfSSL 5.8.4. Provide a fallback for older + * versions to securely zero sensitive memory. */ +#if defined(LIBWOLFSSL_VERSION_HEX) && LIBWOLFSSL_VERSION_HEX >= 0x05008004 + #include +#else + static void wc_ForceZero(void* mem, size_t len) { + volatile byte* p = (volatile byte*)mem; + while (len--) *p++ = 0; + } +#endif + /* Helper to get size of struct field */ #define FIELD_SIZE(type, field) (sizeof(((type *)0)->field)) From 17615c97f9232f04bf2238ee9e913bacde456fd3 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Fri, 20 Mar 2026 09:49:22 +0000 Subject: [PATCH 12/13] Fix stale comment --- tests/ecb_check_value_error_test.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/tests/ecb_check_value_error_test.c b/tests/ecb_check_value_error_test.c index fc564472..e04a9b73 100644 --- a/tests/ecb_check_value_error_test.c +++ b/tests/ecb_check_value_error_test.c @@ -20,10 +20,10 @@ * * Test for GetEcbCheckValue error propagation (bug #496). * - * GetEcbCheckValue always returns CKR_OK even when WP11_AesEcb_Encrypt fails. - * A generic secret key with an invalid AES key length (e.g. 5 bytes) causes - * the encrypt to fail, but C_GetAttributeValue still returns CKR_OK with - * uninitialized output. + * Verifies that GetEcbCheckValue correctly propagates failures from + * WP11_AesEcb_Encrypt. A generic secret key with an invalid AES key length + * (e.g. 5 bytes) causes the encrypt to fail, and C_GetAttributeValue must + * return CKR_FUNCTION_FAILED. */ #ifdef HAVE_CONFIG_H @@ -268,14 +268,12 @@ static int test_valid_key_check_value(CK_SESSION_HANDLE session) } /* - * Test 2 (bug demonstration): Create a 5-byte CKK_GENERIC_SECRET key and - * query CKA_CHECK_VALUE. The 5-byte key is not a valid AES key length, so - * WP11_AesEcb_Encrypt (called by GetEcbCheckValue) fails internally. + * Test 2: Create a 5-byte CKK_GENERIC_SECRET key and query CKA_CHECK_VALUE. + * The 5-byte key is not a valid AES key length, so WP11_AesEcb_Encrypt + * (called by GetEcbCheckValue) fails internally. * - * BUG #496: GetEcbCheckValue always returns CKR_OK regardless of whether the - * encrypt succeeded. This test expects CKR_OK (proving the bug exists). - * Once the bug is fixed, this test should be updated to expect - * CKR_FUNCTION_FAILED. + * Verifies the fix for BUG #496: GetEcbCheckValue now correctly propagates + * the encrypt failure, returning CKR_FUNCTION_FAILED. */ static int test_invalid_key_check_value(CK_SESSION_HANDLE session) { From 9bb9c33847e74288286b5bdecc73c493dc772d97 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Fri, 20 Mar 2026 10:06:02 +0000 Subject: [PATCH 13/13] Use XMEMSET / XMEMCPY in tests --- src/crypto.c | 7 +++---- tests/aes_cbc_pad_padding_test.c | 18 +++++++++--------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/crypto.c b/src/crypto.c index f83e92ce..58bacfea 100644 --- a/src/crypto.c +++ b/src/crypto.c @@ -3298,10 +3298,9 @@ CK_RV C_Decrypt(CK_SESSION_HANDLE hSession, CK_BYTE_PTR pEncryptedData, if (!WP11_Session_IsOpInitialized(session, WP11_INIT_AES_KEYWRAP_DEC)) return CKR_OPERATION_NOT_INITIALIZED; - /* AES Key Wrap unwrapping reduces the size by 8 bytes (the - * integrity check value). If using padding then its even smaller - * but we can't know the final size without decrypting first. */ - if (ulEncryptedDataLen < KEYWRAP_BLOCK_SIZE) + /* AES Key Wrap ciphertext is at least two semiblocks: one data + * semiblock plus the 8-byte integrity check value. */ + if (ulEncryptedDataLen < 2 * KEYWRAP_BLOCK_SIZE) return CKR_ENCRYPTED_DATA_LEN_RANGE; decDataLen = (word32)(ulEncryptedDataLen - KEYWRAP_BLOCK_SIZE); if (pData == NULL) { diff --git a/tests/aes_cbc_pad_padding_test.c b/tests/aes_cbc_pad_padding_test.c index ccd3d3c4..dbdfe0be 100644 --- a/tests/aes_cbc_pad_padding_test.c +++ b/tests/aes_cbc_pad_padding_test.c @@ -236,8 +236,8 @@ static int test_valid_roundtrip(CK_SESSION_HANDLE session, CK_ULONG encSz, decSz; int result = 0; - memset(plain, 9, sizeof(plain)); - memset(iv, 9, sizeof(iv)); + XMEMSET(plain, 9, sizeof(plain)); + XMEMSET(iv, 9, sizeof(iv)); mech.mechanism = CKM_AES_CBC_PAD; mech.ulParameterLen = sizeof(iv); @@ -270,7 +270,7 @@ static int test_valid_roundtrip(CK_SESSION_HANDLE session, ret = funcList->C_Decrypt(session, cipherOut, encSz, dec, &decSz); CHECK_CKR(ret, "Test1: C_Decrypt", CKR_OK); - if (decSz != sizeof(plain) || memcmp(dec, plain, sizeof(plain)) != 0) { + if (decSz != sizeof(plain) || XMEMCMP(dec, plain, sizeof(plain)) != 0) { fprintf(stderr, "FAIL: Test1: decrypted plaintext mismatch\n"); test_failed++; result = -1; @@ -299,8 +299,8 @@ static int test_tampered_last_byte_oneshot(CK_SESSION_HANDLE session, CK_ULONG decSz; int result = 0; - memset(iv, 9, sizeof(iv)); - memcpy(tampered, cipher, cipherLen); + XMEMSET(iv, 9, sizeof(iv)); + XMEMCPY(tampered, cipher, cipherLen); tampered[cipherLen - 1] ^= 0x01; /* flip one bit in last byte */ mech.mechanism = CKM_AES_CBC_PAD; @@ -334,8 +334,8 @@ static int test_tampered_last_byte_multipart(CK_SESSION_HANDLE session, CK_ULONG decSz, lastPartLen; int result = 0; - memset(iv, 9, sizeof(iv)); - memcpy(tampered, cipher, cipherLen); + XMEMSET(iv, 9, sizeof(iv)); + XMEMCPY(tampered, cipher, cipherLen); tampered[cipherLen - 1] ^= 0x01; mech.mechanism = CKM_AES_CBC_PAD; @@ -375,8 +375,8 @@ static int test_tampered_first_block_oneshot(CK_SESSION_HANDLE session, CK_ULONG decSz; int result = 0; - memset(iv, 9, sizeof(iv)); - memcpy(tampered, cipher, cipherLen); + XMEMSET(iv, 9, sizeof(iv)); + XMEMCPY(tampered, cipher, cipherLen); tampered[15] ^= 0x01; /* flip one bit in last byte of first block */ mech.mechanism = CKM_AES_CBC_PAD;