From 48b4602c34641a616ac11888f6e6d9c06bb05e6d Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Thu, 15 Oct 2026 16:24:06 +0100 Subject: [PATCH 1/6] Stop releasing WP11_Lock from a thread that did not take it WP11_Lock_LockRO locked the write mutex for the first reader and WP11_Lock_UnlockRO unlocked it for the last reader, which is often a different thread. Unlocking a pthread mutex, or leaving a critical section, from a thread that does not own it is undefined behaviour. Under Wine pkcs11mtt hangs because of it, and ThreadSanitizer cannot track the lock at all. Rebuild the lock from one mutex and a condition variable, with a reader count and a writer flag. The mutex only guards the counters and is not held while the lock is held. The semantics are unchanged: a waiting writer does not block new readers, so a thread can take the read lock again while holding it. pthreads use pthread_mutex_t and pthread_cond_t; Windows (Vista and later) uses CRITICAL_SECTION and CONDITION_VARIABLE. The two-mutex lock remains as a fallback for other threading APIs. --- src/internal.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 194 insertions(+), 1 deletion(-) diff --git a/src/internal.c b/src/internal.c index 6fa7d2a4..83097b8c 100644 --- a/src/internal.c +++ b/src/internal.c @@ -188,7 +188,36 @@ typedef int WP11_Lock; #define WP11_Lock_LockRO(l) ({ 0; }) #define WP11_Lock_UnlockRO(l) ({ 0; }) +#elif defined(WOLFSSL_PTHREADS) || (defined(USE_WINDOWS_API) && \ + !defined(_WIN32_WCE) && defined(_WIN32_WINNT) && _WIN32_WINNT >= 0x0600) +/* Reader/writer lock built from a mutex and a condition variable. The mutex + * only guards the counters and is never held while the lock itself is held, + * so the last reader out can release the lock without unlocking a mutex it + * does not own. As with the two-mutex lock below, a waiting writer does not + * stop new readers, so a thread may take the read lock again while holding + * it. Uses the same threading API as wolfSSL_Mutex: pthreads, or a critical + * section and condition variable on Windows (Vista and later). */ +#define WP11_LOCK_COND + +#ifdef WOLFSSL_PTHREADS +typedef pthread_mutex_t WP11_LockMutex; +typedef pthread_cond_t WP11_LockCond; #else +typedef CRITICAL_SECTION WP11_LockMutex; +typedef CONDITION_VARIABLE WP11_LockCond; +#endif + +typedef struct WP11_Lock { + WP11_LockMutex mutex; /* Mutex for accessing counters */ + WP11_LockCond cond; /* Signalled when lock may be free */ + int cnt; /* Count of readers */ + int writer; /* Whether a writer holds the lock */ +} WP11_Lock; +#else +/* Fallback for threading APIs without a condition variable. The first reader + * locks the write mutex and the last reader unlocks it, which may be a + * different thread: only correct where wolfSSL_Mutex may be released by a + * thread that did not lock it (e.g. a binary semaphore). */ typedef struct WP11_Lock { wolfSSL_Mutex read; /* Mutex for accessing count */ wolfSSL_Mutex write; /* Mutex for writing */ @@ -676,7 +705,171 @@ static wolfSSL_Mutex storeDirLock #endif -#ifndef SINGLE_THREADED +#ifdef WP11_LOCK_COND +/* Mutex and condition variable operations. The INIT, LOCK and WAIT operations + * evaluate to non-zero on success. */ +#ifdef WOLFSSL_PTHREADS +#define WP11_LOCK_MUTEX_INIT(m) (pthread_mutex_init((m), NULL) == 0) +#define WP11_LOCK_MUTEX_FREE(m) pthread_mutex_destroy(m) +#define WP11_LOCK_MUTEX_LOCK(m) (pthread_mutex_lock(m) == 0) +#define WP11_LOCK_MUTEX_UNLOCK(m) pthread_mutex_unlock(m) +#define WP11_LOCK_COND_INIT(c) (pthread_cond_init((c), NULL) == 0) +#define WP11_LOCK_COND_FREE(c) pthread_cond_destroy(c) +#define WP11_LOCK_COND_WAIT(c, m) (pthread_cond_wait((c), (m)) == 0) +#define WP11_LOCK_COND_WAKE_ALL(c) pthread_cond_broadcast(c) +#else +#define WP11_LOCK_MUTEX_INIT(m) (InitializeCriticalSection(m), 1) +#define WP11_LOCK_MUTEX_FREE(m) DeleteCriticalSection(m) +#define WP11_LOCK_MUTEX_LOCK(m) (EnterCriticalSection(m), 1) +#define WP11_LOCK_MUTEX_UNLOCK(m) LeaveCriticalSection(m) +#define WP11_LOCK_COND_INIT(c) (InitializeConditionVariable(c), 1) +/* A CONDITION_VARIABLE has nothing to free. */ +#define WP11_LOCK_COND_FREE(c) (void)(c) +#define WP11_LOCK_COND_WAIT(c, m) \ + (SleepConditionVariableCS((c), (m), INFINITE) != 0) +#define WP11_LOCK_COND_WAKE_ALL(c) WakeAllConditionVariable(c) +#endif + +/** + * Initialize a lock. + * + * @param lock [in] Lock object. + * @return BAD_MUTEX_E on failure. + * 0 on success. + */ +static int WP11_Lock_Init(WP11_Lock* lock) +{ + int ret = 0; + + if (!WP11_LOCK_MUTEX_INIT(&lock->mutex)) + ret = BAD_MUTEX_E; + if (ret == 0 && !WP11_LOCK_COND_INIT(&lock->cond)) { + WP11_LOCK_MUTEX_FREE(&lock->mutex); + ret = BAD_MUTEX_E; + } + if (ret == 0) { + lock->cnt = 0; + lock->writer = 0; + } + + return ret; +} + +/** + * Free a lock. + * + * @param lock [in] Lock object. + */ +static void WP11_Lock_Free(WP11_Lock* lock) +{ + WP11_LOCK_COND_FREE(&lock->cond); + WP11_LOCK_MUTEX_FREE(&lock->mutex); +} + +/** + * Lock for read/write. + * + * Waits until there is no writer and no reader. + * + * @param lock [in] Lock object. + * @return BAD_MUTEX_E on failure. + * 0 on success. + */ +static int WP11_Lock_LockRW(WP11_Lock* lock) +{ + int ret = 0; + + if (!WP11_LOCK_MUTEX_LOCK(&lock->mutex)) + return BAD_MUTEX_E; + while (ret == 0 && (lock->writer || lock->cnt > 0)) { + if (!WP11_LOCK_COND_WAIT(&lock->cond, &lock->mutex)) + ret = BAD_MUTEX_E; + } + if (ret == 0) + lock->writer = 1; +#ifdef DEBUG_LOCK + fprintf(stderr, "LRW: %p - %d\n", (void*)lock, lock->cnt); +#endif + WP11_LOCK_MUTEX_UNLOCK(&lock->mutex); + + return ret; +} + +/** + * Unlock after read/write. + * + * @param lock [in] Lock object. + * @return BAD_MUTEX_E on failure. + * 0 on success. + */ +static int WP11_Lock_UnlockRW(WP11_Lock* lock) +{ + if (!WP11_LOCK_MUTEX_LOCK(&lock->mutex)) + return BAD_MUTEX_E; +#ifdef DEBUG_LOCK + fprintf(stderr, "URW: %p - %d\n", (void*)lock, lock->cnt); +#endif + lock->writer = 0; + WP11_LOCK_COND_WAKE_ALL(&lock->cond); + WP11_LOCK_MUTEX_UNLOCK(&lock->mutex); + + return 0; +} + +/** + * Lock for read-only. + * + * Waits only while a writer holds the lock: a waiting writer does not stop + * new readers, so a thread already holding the read lock can take it again. + * + * @param lock [in] Lock object. + * @return BAD_MUTEX_E on failure. + * 0 on success. + */ +static int WP11_Lock_LockRO(WP11_Lock* lock) +{ + int ret = 0; + + if (!WP11_LOCK_MUTEX_LOCK(&lock->mutex)) + return BAD_MUTEX_E; + while (ret == 0 && lock->writer) { + if (!WP11_LOCK_COND_WAIT(&lock->cond, &lock->mutex)) + ret = BAD_MUTEX_E; + } + if (ret == 0) + lock->cnt++; +#ifdef DEBUG_LOCK + fprintf(stderr, "LRO: %p - %d\n", (void*)lock, lock->cnt); +#endif + WP11_LOCK_MUTEX_UNLOCK(&lock->mutex); + + return ret; +} + +/** + * Unlock after reading. + * + * @param lock [in] Lock object. + * @return BAD_MUTEX_E on failure. + * 0 on success. + */ +static int WP11_Lock_UnlockRO(WP11_Lock* lock) +{ + if (!WP11_LOCK_MUTEX_LOCK(&lock->mutex)) + return BAD_MUTEX_E; + /* Wake waiting writers once the last reader has left. */ + if (--lock->cnt == 0) + WP11_LOCK_COND_WAKE_ALL(&lock->cond); +#ifdef DEBUG_LOCK + fprintf(stderr, "URO: %p - %d\n", (void*)lock, lock->cnt); +#endif + WP11_LOCK_MUTEX_UNLOCK(&lock->mutex); + + return 0; +} +#elif !defined(SINGLE_THREADED) +/* Fallback two-mutex lock: see the WP11_Lock type for when it is used. */ + /** * Initialize a lock. * From b6cf59c18091140b5ed9ed1954dd933d5bc56148 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Thu, 15 Oct 2026 16:22:20 +0100 Subject: [PATCH 2/6] Fix use-after-free in WP11_Object_Find WP11_Object_Find walked the object list under the token lock, released the lock, and then read obj->handle and obj->opFlag. A concurrent C_DestroyObject of the same token object from another session could unlink and free the object in that window, which ASAN caught in concurrent_destroy_object_test. Read everything needed from the object, including the CKA_PRIVATE flag, while the token lock is held, and do not dereference it after the unlock. The slot lock used for the login state check is still only taken after the token lock is released, so the slot-then-token lock order is unchanged. --- src/internal.c | 49 ++++++++++++++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/src/internal.c b/src/internal.c index 83097b8c..70f4aaa7 100644 --- a/src/internal.c +++ b/src/internal.c @@ -12258,22 +12258,27 @@ int WP11_Object_Find(WP11_Session* session, CK_OBJECT_HANDLE objHandle, WP11_Object* obj; #ifdef WOLFPKCS11_NSS WP11_Session* scan; +#else + int isPrivate = 0; #endif int onToken = OBJ_HANDLE_ON_TOKEN(objHandle); - if (!onToken) { #ifdef WOLFPKCS11_NSS - /* The NSS cross-session walk below needs the slot lock so a concurrent - * remove-session can't reclaim a session node. Acquire it before the - * token lock to match the global lock order (slot then token) used by - * WP11_Session_RemoveObjectByHandle / WP11_Slot_CloseSessions; acquiring - * them in the opposite order would risk an AB-BA deadlock. */ + /* The NSS cross-session walk below needs the slot lock so a concurrent + * remove-session can't reclaim a session node. Acquire it before the + * token lock to match the global lock order (slot then token) used by + * WP11_Session_RemoveObjectByHandle / WP11_Slot_CloseSessions; acquiring + * them in the opposite order would risk an AB-BA deadlock. */ + if (!onToken) WP11_Lock_LockRO(&session->slot->lock); #endif - /* Hold the token lock across the session->object walk so a concurrent - * WP11_Session_RemoveObjectByHandle (which holds the same lock around - * the unlink) can't free a node we're stepping through. */ - WP11_Lock_LockRO(&session->slot->token.lock); + /* The token lock guards every object list on the slot, and + * WP11_Session_RemoveObjectByHandle holds it around the unlink. Hold it + * across the walk and every read of the matched object: once it is + * released a concurrent C_DestroyObject on the same handle (token objects + * are shared by all sessions) can unlink and free the object. */ + WP11_Lock_LockRO(&session->slot->token.lock); + if (!onToken) { obj = session->object; while (obj != NULL) { if (obj->handle == objHandle) { @@ -12296,14 +12301,9 @@ int WP11_Object_Find(WP11_Session* session, CK_OBJECT_HANDLE objHandle, } } } -#endif - WP11_Lock_UnlockRO(&session->slot->token.lock); -#ifdef WOLFPKCS11_NSS - WP11_Lock_UnlockRO(&session->slot->lock); #endif } else { - WP11_Lock_LockRO(&session->slot->token.lock); obj = session->slot->token.object; while (obj != NULL) { if (obj->handle == objHandle) { @@ -12312,15 +12312,26 @@ int WP11_Object_Find(WP11_Session* session, CK_OBJECT_HANDLE objHandle, } obj = obj->next; } - WP11_Lock_UnlockRO(&session->slot->token.lock); } +#ifndef WOLFPKCS11_NSS + if (ret == 0) + isPrivate = ((obj->opFlag & WP11_FLAG_PRIVATE) == WP11_FLAG_PRIVATE); +#endif + WP11_Lock_UnlockRO(&session->slot->token.lock); +#ifdef WOLFPKCS11_NSS + if (!onToken) + WP11_Lock_UnlockRO(&session->slot->lock); +#endif + /* obj may already be freed by a concurrent destroy: do not dereference it + * from here on. */ - if (ret == 0 && obj != NULL && (obj->handle == objHandle)) { + if (ret == 0) { #ifndef WOLFPKCS11_NSS /* Enforce CKA_PRIVATE: reject private objects from public sessions. * Skipped in NSS mode because NSS operates as the internal crypto - * module without calling C_Login. */ - if ((obj->opFlag & WP11_FLAG_PRIVATE) == WP11_FLAG_PRIVATE) { + * module without calling C_Login. The slot lock is only taken after + * the token lock is released, keeping the slot-then-token order. */ + if (isPrivate) { int loginState; WP11_Lock_LockRO(&session->slot->lock); loginState = session->slot->token.loginState; From e4861b5e1f0ad7ce2e44c35097d2890ad1138118 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Thu, 15 Oct 2026 16:24:59 +0100 Subject: [PATCH 3/6] Fix remaining TSAN races and enable TSAN in sanitizer CI WP11_Slot_ClearActiveObject reads and clears every session's active object under the slot lock, but each session wrote its own with no lock. Access session->curr under the slot lock in WP11_Session_GetObject and WP11_Session_SetObject. WP11_Slot_Has_Empty_Pin cached userPinEmpty without a lock. Read the PIN state under the slot lock and store the cached result under it, and only if the user PIN did not change while the expensive check ran with the lock released. tests/unit.h had its own copy of the cross-thread-unlock read/write lock; replace it with a start gate and a mutex-guarded stop flag. concurrent_destroy_object_test now uses atomic loads and stores for its spin barrier. Add tsan back to the sanitizer matrix. Its make check step pointed TSAN_OPTIONS at a suppressions file that does not exist, which makes every test abort at startup, so drop it. Also lower vm.mmap_rnd_bits for TSAN, whose fixed shadow mappings can collide with the higher ASLR entropy of current runner kernels. --- .github/workflows/sanitizer-tests.yml | 12 +++-- src/internal.c | 67 +++++++++++++++++--------- tests/concurrent_destroy_object_test.c | 33 +++++++------ tests/unit.h | 52 ++++++++------------ 4 files changed, 88 insertions(+), 76 deletions(-) diff --git a/.github/workflows/sanitizer-tests.yml b/.github/workflows/sanitizer-tests.yml index 3eb3d00f..e85db885 100644 --- a/.github/workflows/sanitizer-tests.yml +++ b/.github/workflows/sanitizer-tests.yml @@ -15,8 +15,7 @@ jobs: fail-fast: false matrix: # msan disabled due to too many failures - # tsan disabled due to weird failures - sanitizer: [asan, ubsan] + sanitizer: [asan, ubsan, tsan] config: - name: "Standard Build" configure_flags: "--enable-aeskeywrap" @@ -44,6 +43,12 @@ jobs: libnss3-dev \ libnspr4-dev +#TSAN maps its shadow memory at fixed addresses, which can collide with the +#higher ASLR entropy used by current runner kernels + - name: Reduce ASLR entropy for TSAN + if: matrix.sanitizer == 'tsan' + run: sudo sysctl vm.mmap_rnd_bits=28 + #setup wolfssl - uses: actions/checkout@v4 with: @@ -182,9 +187,6 @@ jobs: make - name: wolfpkcs11 make check run: | - if [ "${{ matrix.sanitizer }}" = "tsan" ]; then - export TSAN_OPTIONS="suppressions=$PWD/tsan_suppressions.txt" - fi if [[ "${{ matrix.config.configure_flags }}" == *"--enable-tpm"* ]]; then ./tests/pkcs11str && ./tests/pkcs11test else diff --git a/src/internal.c b/src/internal.c index 70f4aaa7..92d53a22 100644 --- a/src/internal.c +++ b/src/internal.c @@ -1063,33 +1063,44 @@ static int wp11_Session_New(WP11_Slot* slot, CK_OBJECT_HANDLE handle, */ int WP11_Slot_Has_Empty_Pin(WP11_Slot* slot) { + int pinSet; + int cached; + int empty; + byte userPin[PIN_HASH_SZ]; + if (slot == NULL) return 0; - if (slot->token.tokenFlags & WP11_TOKEN_FLAG_USER_PIN_SET) { - switch (slot->token.userPinEmpty) { - case 1: - /* Empty user PIN */ - return 1; - case 2: - /* Non-empty user PIN */ - return 0; - default: - /* Cache result as WP11_Slot_CheckUserPin is very expensive */ - if (WP11_Slot_CheckUserPin(slot, (char*)"", 0) == 0) { - /* Empty user PIN */ - slot->token.userPinEmpty = 1; - return 1; - } - else { - /* Non-empty user PIN */ - slot->token.userPinEmpty = 2; - return 0; - } - } + /* The user PIN and its cached emptiness are changed under the slot lock + * (C_InitPIN / C_SetPIN), so read them under it too. */ + WP11_Lock_LockRO(&slot->lock); + pinSet = (slot->token.tokenFlags & WP11_TOKEN_FLAG_USER_PIN_SET) != 0; + cached = slot->token.userPinEmpty; + if (pinSet && cached == 0) + XMEMCPY(userPin, slot->token.userPin, sizeof(userPin)); + WP11_Lock_UnlockRO(&slot->lock); + + if (!pinSet) + return 0; + if (cached != 0) { + /* 1: empty user PIN, 2: non-empty user PIN */ + return cached == 1; } - return 0; + /* Cache result as WP11_Slot_CheckUserPin is very expensive. It hashes + * with the slot lock released, so only cache the result if the user PIN + * was not changed in the meantime. */ + empty = (WP11_Slot_CheckUserPin(slot, (char*)"", 0) == 0); + WP11_Lock_LockRW(&slot->lock); + if (slot->token.userPinEmpty == 0 && + WP11_ConstantCompare(userPin, slot->token.userPin, + sizeof(userPin))) { + slot->token.userPinEmpty = empty ? 1 : 2; + } + WP11_Lock_UnlockRW(&slot->lock); + wc_ForceZero(userPin, sizeof(userPin)); + + return empty; } /** @@ -10871,7 +10882,8 @@ void WP11_Session_RemoveObject(WP11_Session* session, WP11_Object* object) * * The active object pointer (session->curr) is a raw pointer cached when an * operation is initialized. A token object can be the active object of more - * than one session. + * than one session. Since this reaches into other sessions, session->curr is + * only accessed under the slot lock (see WP11_Session_GetObject()). * * Symmetric operations (AES, HMAC/generic-secret, HKDF) copy the key into the * operation context during their *Init and run to completion without referring @@ -10924,23 +10936,32 @@ void WP11_Slot_ClearActiveObject(WP11_Slot* slot, WP11_Object* object) /** * Get the current object of the session - key for operation. * + * WP11_Slot_ClearActiveObject() may clear session->curr from another session's + * thread, holding the slot lock for write, so access it under the slot lock. + * * @param session [in] Session object. * @param object [out] Key Object object. */ void WP11_Session_GetObject(WP11_Session* session, WP11_Object** object) { + WP11_Lock_LockRO(&session->slot->lock); *object = session->curr; + WP11_Lock_UnlockRO(&session->slot->lock); } /** * Set the current object on the session - key for operation. * + * Writes session->curr under the slot lock; see WP11_Session_GetObject(). + * * @param session [in] Session object. * @param object [in] Key Object object. */ void WP11_Session_SetObject(WP11_Session* session, WP11_Object* object) { + WP11_Lock_LockRO(&session->slot->lock); session->curr = object; + WP11_Lock_UnlockRO(&session->slot->lock); } /** diff --git a/tests/concurrent_destroy_object_test.c b/tests/concurrent_destroy_object_test.c index 7edc50c1..255f7220 100644 --- a/tests/concurrent_destroy_object_test.c +++ b/tests/concurrent_destroy_object_test.c @@ -91,10 +91,13 @@ static CK_KEY_TYPE aesKeyType = CKK_AES; * A busy-wait ("spin") barrier is used deliberately: cond-var wakeups have * enough latency that one destroyer routinely finishes before the other * starts, which hides the race. Spinning releases both threads within a few - * cycles of each other so their C_DestroyObject calls truly overlap. */ -static volatile int g_go = 0; /* bumped by main to release a round */ -static volatile int g_done = 0; /* threads that finished current round */ -static volatile int g_stop = 0; /* set to make the threads exit */ + * cycles of each other so their C_DestroyObject calls truly overlap. + * g_go, g_done and g_stop are only accessed atomically: the release store of + * g_go publishes g_target, and each release increment of g_done publishes that + * thread's g_rv entry. */ +static int g_go = 0; /* bumped by main to release a round */ +static int g_done = 0; /* threads that finished current round */ +static int g_stop = 0; /* set to make the threads exit */ static CK_OBJECT_HANDLE g_target = CK_INVALID_HANDLE; static CK_RV g_rv[2]; /* per-thread result for the round */ @@ -262,16 +265,17 @@ static void* destroyer(void* arg) CK_OBJECT_HANDLE h; /* Tight spin until main releases the next round (or asks us to stop). */ - while (g_go == last && !g_stop) { } - if (g_stop) + while (__atomic_load_n(&g_go, __ATOMIC_ACQUIRE) == last && + !__atomic_load_n(&g_stop, __ATOMIC_ACQUIRE)) { } + if (__atomic_load_n(&g_stop, __ATOMIC_ACQUIRE)) break; - last = g_go; - __sync_synchronize(); /* observe g_target published before g_go */ + /* Acquiring g_go makes the g_target published before it visible. */ + last = __atomic_load_n(&g_go, __ATOMIC_ACQUIRE); h = g_target; g_rv[ctx->id] = funcList->C_DestroyObject(ctx->session, h); - __sync_fetch_and_add(&g_done, 1); + __atomic_fetch_add(&g_done, 1, __ATOMIC_RELEASE); } return NULL; @@ -355,11 +359,9 @@ static int concurrent_destroy_test(int rounds) * then spin until both have returned from C_DestroyObject. */ g_target = h; g_rv[0] = g_rv[1] = CKR_GENERAL_ERROR; - g_done = 0; - __sync_synchronize(); - g_go = r + 1; - while (g_done < 2) { } - __sync_synchronize(); + __atomic_store_n(&g_done, 0, __ATOMIC_RELAXED); + __atomic_store_n(&g_go, r + 1, __ATOMIC_RELEASE); + while (__atomic_load_n(&g_done, __ATOMIC_ACQUIRE) < 2) { } /* Exactly one destroy must win; the other must report an invalid * handle. Any other combination (two successes, or a status other @@ -409,8 +411,7 @@ static int concurrent_destroy_test(int rounds) cleanup: /* Tell the destroyer threads to exit and join them. */ - g_stop = 1; - __sync_synchronize(); + __atomic_store_n(&g_stop, 1, __ATOMIC_RELEASE); for (r = 0; r < started; r++) pthread_join(threads[r], NULL); for (r = 0; r < started; r++) { diff --git a/tests/unit.h b/tests/unit.h index 3a2ec870..4903f3d2 100644 --- a/tests/unit.h +++ b/tests/unit.h @@ -159,9 +159,8 @@ typedef struct TEST_FUNC static int verbose = 0; #ifdef TEST_MULTITHREADED -static wolfSSL_Mutex readMutex; +/* Held by run_tests while it starts the test threads, and guards stop. */ static wolfSSL_Mutex writeMutex; -static int lockCnt; static int stop = 0; static int secs = 10; @@ -169,14 +168,7 @@ static int LockInit(void) { int ret; - ret = wc_InitMutex(&readMutex); - if (ret == 0) { - ret = wc_InitMutex(&writeMutex); - if (ret != 0) - wc_FreeMutex(&readMutex); - } - if (ret == 0) - lockCnt = 0; + ret = wc_InitMutex(&writeMutex); if (ret != 0) ret = -1; @@ -186,7 +178,6 @@ static int LockInit(void) static void LockFree(void) { wc_FreeMutex(&writeMutex); - wc_FreeMutex(&readMutex); } static int LockRW(void) @@ -199,40 +190,38 @@ static int UnlockRW(void) return wc_UnLockMutex(&writeMutex); } -static int LockRO(void) +/* Wait until run_tests has started every test thread. */ +static int WaitForStart(void) { int ret; - ret = wc_LockMutex(&readMutex); - if (ret == 0) { - if (++lockCnt == 1) - ret = wc_LockMutex(&writeMutex); - } + ret = LockRW(); if (ret == 0) - ret = wc_UnLockMutex(&readMutex); + ret = UnlockRW(); if (ret != 0) ret = -1; return ret; } -static int UnlockRO(void) +static int IsStopped(void) { int ret; - ret = wc_LockMutex(&readMutex); - if (ret == 0) { - if (--lockCnt == 0) - ret = wc_UnLockMutex(&writeMutex); - } - if (ret == 0) - ret = wc_UnLockMutex(&readMutex); - if (ret != 0) - ret = -1; + LockRW(); + ret = stop; + UnlockRW(); return ret; } +static void SetStop(int value) +{ + LockRW(); + stop = value; + UnlockRW(); +} + static void* run_test(void* args) { int ret; @@ -242,7 +231,7 @@ static void* run_test(void* args) testArgs = XMALLOC(tf->argsSz, NULL, DYNAMIC_TYPE_TMP_BUFFER); if (testArgs != NULL) { - ret = LockRO(); + ret = WaitForStart(); if (ret != 0) fprintf(stderr, "Locking failed\n"); else { @@ -251,13 +240,12 @@ static void* run_test(void* args) if (rv != CKR_OK) fprintf(stderr, "Setup failed\n"); if (rv == CKR_OK) { - while (rv == CKR_OK && !stop) { + while (rv == CKR_OK && !IsStopped()) { rv = tf->ret = tf->func(testArgs); tf->cnt++; } tf->teardown(tf->flags, testArgs); } - UnlockRO(); } XFREE(testArgs, NULL, DYNAMIC_TYPE_TMP_BUFFER); @@ -310,7 +298,7 @@ static CK_RV run_tests(TEST_FUNC* testFunc, int testFuncCnt, int onlySet, fprintf(stderr, "."); } fprintf(stderr, "\n"); - stop = 1; + SetStop(1); for (i = 0; i < testFuncCnt; i++) { if (!testFunc[i].attempted) continue; From 04d63589c1263914830be008ac6f7bf87068043c Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Thu, 15 Oct 2026 16:51:12 +0100 Subject: [PATCH 4/6] Keep WP11_Lock acquisition free of cancellation points pthread_cond_wait() is a cancellation point, but acquiring a WP11_Lock was not one when it was built from mutexes alone. A thread cancelled while waiting for the lock would exit holding the lock's mutex and hang every later user of the lock, including the current holder's unlock. Disable cancellation around the wait, restoring the previous behaviour. The guard on PTHREAD_CANCEL_DISABLE keeps this building on pthreads implementations without cancellation support. --- src/internal.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/src/internal.c b/src/internal.c index 92d53a22..038e3eee 100644 --- a/src/internal.c +++ b/src/internal.c @@ -709,13 +709,44 @@ static wolfSSL_Mutex storeDirLock /* Mutex and condition variable operations. The INIT, LOCK and WAIT operations * evaluate to non-zero on success. */ #ifdef WOLFSSL_PTHREADS +/** + * Wait on the lock's condition variable with thread cancellation disabled. + * + * pthread_cond_wait() is a cancellation point, but acquiring the lock was not + * one when it was built from mutexes alone. A thread cancelled while waiting + * would exit holding the mutex, hanging every later user of the lock + * (including the current holder's unlock), so keep acquisition + * non-cancellable. + * + * @param cond [in] Condition variable to wait on. + * @param mutex [in] Mutex held by the caller. + * @return 1 on success. + * 0 on failure. + */ +static int wp11_Lock_CondWait(pthread_cond_t* cond, pthread_mutex_t* mutex) +{ + int ret; +#ifdef PTHREAD_CANCEL_DISABLE + int state; + int prevState; + + pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &prevState); +#endif + ret = pthread_cond_wait(cond, mutex); +#ifdef PTHREAD_CANCEL_DISABLE + pthread_setcancelstate(prevState, &state); +#endif + + return ret == 0; +} + #define WP11_LOCK_MUTEX_INIT(m) (pthread_mutex_init((m), NULL) == 0) #define WP11_LOCK_MUTEX_FREE(m) pthread_mutex_destroy(m) #define WP11_LOCK_MUTEX_LOCK(m) (pthread_mutex_lock(m) == 0) #define WP11_LOCK_MUTEX_UNLOCK(m) pthread_mutex_unlock(m) #define WP11_LOCK_COND_INIT(c) (pthread_cond_init((c), NULL) == 0) #define WP11_LOCK_COND_FREE(c) pthread_cond_destroy(c) -#define WP11_LOCK_COND_WAIT(c, m) (pthread_cond_wait((c), (m)) == 0) +#define WP11_LOCK_COND_WAIT(c, m) wp11_Lock_CondWait((c), (m)) #define WP11_LOCK_COND_WAKE_ALL(c) pthread_cond_broadcast(c) #else #define WP11_LOCK_MUTEX_INIT(m) (InitializeCriticalSection(m), 1) From b5552c0eba4b0c0ed01b54111c50279feb2fd921 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Thu, 15 Oct 2026 16:54:26 +0100 Subject: [PATCH 5/6] Test that a PIN change discards the in-flight empty-PIN check WP11_Slot_Has_Empty_Pin only caches its result if the user PIN did not change while the expensive check ran with the slot lock released. Add a test for that branch: a DEBUG_WOLFPKCS11-only hook, WP11_Test_SetEmptyPinCheckHook, runs between the check and the caching, and empty_pin_cache_test uses it to change the PIN from empty to non-empty at exactly that point. A later CKA_PRIVATE object creation in the public session must then be refused, and allowed after logging in with the new PIN. Without the PIN comparison the stale "empty" result is cached and the creation succeeds. The test needs an empty user PIN (WP11_MIN_PIN_LEN=0, the default for NSS builds) and reports itself skipped otherwise, so run it in the empty PIN store workflow, which builds that way. --- .github/workflows/empty-pin-store-test.yml | 12 +- src/internal.c | 22 ++ tests/empty_pin_cache_test.c | 387 +++++++++++++++++++++ tests/include.am | 7 + wolfpkcs11/internal.h | 1 + 5 files changed, 428 insertions(+), 1 deletion(-) create mode 100644 tests/empty_pin_cache_test.c diff --git a/.github/workflows/empty-pin-store-test.yml b/.github/workflows/empty-pin-store-test.yml index 867c5016..4b27e096 100644 --- a/.github/workflows/empty-pin-store-test.yml +++ b/.github/workflows/empty-pin-store-test.yml @@ -63,7 +63,7 @@ jobs: make - name: Create test store directory - run: mkdir -p store/empty_pin_test store/private_object_empty_pin_test + run: mkdir -p store/empty_pin_test store/private_object_empty_pin_test store/empty_pin_cache_test - name: Run empty PIN store test run: | @@ -85,6 +85,16 @@ jobs: echo "" echo "=== Test completed ===" + - name: Run empty PIN cache test + run: | + echo "=== Running Empty PIN Cache Test ===" + echo "This test verifies that changing an empty user PIN while the" + echo "empty-PIN check is in progress does not leave it cached as empty." + echo "" + ./tests/empty_pin_cache_test + echo "" + echo "=== Test completed ===" + - name: Show store directory contents on failure if: failure() run: | diff --git a/src/internal.c b/src/internal.c index 038e3eee..f703adf8 100644 --- a/src/internal.c +++ b/src/internal.c @@ -1085,6 +1085,24 @@ static int wp11_Session_New(WP11_Slot* slot, CK_OBJECT_HANDLE handle, return ret; } +#ifdef DEBUG_WOLFPKCS11 +/* Set by WP11_Test_SetEmptyPinCheckHook(). */ +static void (*wp11_EmptyPinCheckHook)(void) = NULL; + +/** + * Test hook: set a function for WP11_Slot_Has_Empty_Pin() to call after its + * uncached empty-PIN check and before it caches the result, with no locks + * held. Exposed only in debug builds so a white-box test can change the PIN + * at exactly that point. + * + * @param hook [in] Function to call, or NULL to remove the hook. + */ +WP11_API void WP11_Test_SetEmptyPinCheckHook(void (*hook)(void)) +{ + wp11_EmptyPinCheckHook = hook; +} +#endif + /** * Check if the slot has an empty user PIN. * @@ -1122,6 +1140,10 @@ int WP11_Slot_Has_Empty_Pin(WP11_Slot* slot) * with the slot lock released, so only cache the result if the user PIN * was not changed in the meantime. */ empty = (WP11_Slot_CheckUserPin(slot, (char*)"", 0) == 0); +#ifdef DEBUG_WOLFPKCS11 + if (wp11_EmptyPinCheckHook != NULL) + wp11_EmptyPinCheckHook(); +#endif WP11_Lock_LockRW(&slot->lock); if (slot->token.userPinEmpty == 0 && WP11_ConstantCompare(userPin, slot->token.userPin, diff --git a/tests/empty_pin_cache_test.c b/tests/empty_pin_cache_test.c new file mode 100644 index 00000000..51db8f55 --- /dev/null +++ b/tests/empty_pin_cache_test.c @@ -0,0 +1,387 @@ +/* empty_pin_cache_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 + * + * White-box test for the empty-PIN cache in WP11_Slot_Has_Empty_Pin(). + * + * Whether the user PIN is empty is expensive to work out, so the result is + * cached. The check hashes with the slot lock released, so the PIN can change + * between the check and caching its result. If the stale "empty" result were + * cached after the PIN was changed to a non-empty one, a public session could + * keep creating CKA_PRIVATE objects without logging in. + * + * This test uses the DEBUG_WOLFPKCS11-only hook + * WP11_Test_SetEmptyPinCheckHook() to change the PIN from empty to non-empty + * at exactly that point, then checks that a later private-object creation in + * the public session is refused, and is allowed again after logging in. + * + * Needs a build that allows an empty user PIN (WP11_MIN_PIN_LEN=0, the + * default for WOLFPKCS11_NSS builds); otherwise the test reports itself + * skipped. + */ + +#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(DEBUG_WOLFPKCS11) && !defined(NO_AES) && \ + !defined(WOLFPKCS11_LEGACY_PRIVATE_FALSE_DEFAULT) + +/* DEBUG_WOLFPKCS11-only test hook exported by libwolfpkcs11. */ +extern void WP11_Test_SetEmptyPinCheckHook(void (*hook)(void)); + +#define EMPTY_PIN_CACHE_TEST_DIR "./store/empty_pin_cache_test" +#define WOLFPKCS11_TOKEN_FILENAME "wp11_token_0000000000000001" + +static int test_passed = 0; +static int test_failed = 0; +static int test_skipped = 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) + +#define CHECK_COND(cond, op) do { \ + if (!(cond)) { \ + fprintf(stderr, "FAIL: %s\n", op); \ + 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* newUserPin = (byte*)"wolfpkcs11-test"; +static int newUserPinLen = 15; + +static CK_OBJECT_CLASS secretKeyClass = CKO_SECRET_KEY; +static CK_KEY_TYPE aesKeyType = CKK_AES; +static CK_BBOOL ckTrue = CK_TRUE; +static CK_BBOOL ckFalse = CK_FALSE; + +/* State for the hook, which runs inside C_CreateObject. */ +static CK_SESSION_HANDLE hookSession = 0; +static int hookCalls = 0; +static CK_RV hookRv = CKR_GENERAL_ERROR; + +static CK_RV pkcs11_init(void) +{ + CK_RV ret; + CK_C_INITIALIZE_ARGS args; + 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_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); +} + +/* Set an empty user PIN as the SO. */ +static CK_RV pkcs11_set_empty_user_pin(void) +{ + CK_SESSION_HANDLE soSession; + int sessFlags = CKF_SERIAL_SESSION | CKF_RW_SESSION; + CK_RV ret; + + ret = funcList->C_OpenSession(slot, sessFlags, NULL, NULL, &soSession); + if (ret != CKR_OK) + return ret; + ret = funcList->C_Login(soSession, CKU_SO, soPin, soPinLen); + if (ret != CKR_OK) { + funcList->C_CloseSession(soSession); + return ret; + } + ret = funcList->C_InitPIN(soSession, (CK_UTF8CHAR_PTR)"", 0); + funcList->C_Logout(soSession); + funcList->C_CloseSession(soSession); + return ret; +} + +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); +} + +/* Create a CKA_PRIVATE AES session key. */ +static CK_RV create_private_key(CK_SESSION_HANDLE session, + CK_OBJECT_HANDLE* key) +{ + CK_ATTRIBUTE tmpl[] = { + { CKA_CLASS, &secretKeyClass, sizeof(secretKeyClass) }, + { CKA_KEY_TYPE, &aesKeyType, sizeof(aesKeyType) }, + { CKA_VALUE, aes_128_key, sizeof(aes_128_key) }, + { CKA_TOKEN, &ckFalse, sizeof(ckFalse) }, + { CKA_PRIVATE, &ckTrue, sizeof(ckTrue) }, + }; + CK_ULONG tmplCnt = sizeof(tmpl) / sizeof(*tmpl); + + return funcList->C_CreateObject(session, tmpl, tmplCnt, key); +} + +/* Called by WP11_Slot_Has_Empty_Pin() after its uncached check and before it + * caches the result: change the user PIN from empty to non-empty. */ +static void change_pin_hook(void) +{ + hookCalls++; + if (hookCalls == 1) { + hookRv = funcList->C_SetPIN(hookSession, (CK_UTF8CHAR_PTR)"", 0, + newUserPin, newUserPinLen); + } +} + +static int empty_pin_cache_test(void) +{ + CK_RV ret; + CK_SESSION_HANDLE session = 0; + CK_OBJECT_HANDLE key = CK_INVALID_HANDLE; + int sessFlags = CKF_SERIAL_SESSION | CKF_RW_SESSION; + int result = 0; + + printf("\n=== Testing empty-PIN cache across a PIN change ===\n"); + + cleanup_test_files(EMPTY_PIN_CACHE_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; + } + ret = pkcs11_set_empty_user_pin(); + if (ret == CKR_PIN_LEN_RANGE) { + printf("Empty user PIN not allowed (WP11_MIN_PIN_LEN > 0), " + "skipping\n"); + test_skipped = 1; + pkcs11_final(); + return 0; + } + if (ret != CKR_OK) { + fprintf(stderr, "FAIL: set empty user PIN: 0x%lx\n", + (unsigned long)ret); + test_failed++; + pkcs11_final(); + return -1; + } + + /* Public (not logged in) R/W session. Nothing has cached the empty-PIN + * result yet: setting the PIN resets the cache. */ + ret = funcList->C_OpenSession(slot, sessFlags, NULL, NULL, &session); + CHECK_CKR(ret, "C_OpenSession", CKR_OK); + + /* The first private-object creation runs the uncached empty-PIN check; + * the hook changes the PIN before the result can be cached. Whether this + * in-flight call is allowed is not what is being tested. */ + hookSession = session; + WP11_Test_SetEmptyPinCheckHook(change_pin_hook); + ret = create_private_key(session, &key); + WP11_Test_SetEmptyPinCheckHook(NULL); + printf("INFO: in-flight C_CreateObject returned 0x%lx\n", + (unsigned long)ret); + if (ret == CKR_OK && key != CK_INVALID_HANDLE) + funcList->C_DestroyObject(session, key); + + CHECK_COND(hookCalls >= 1, "empty-PIN check ran uncached (hook called)"); + CHECK_CKR(hookRv, "C_SetPIN from empty to non-empty in the hook", CKR_OK); + + /* The stale "empty" result must not have been cached. */ + key = CK_INVALID_HANDLE; + ret = create_private_key(session, &key); + CHECK_CKR(ret, "private object refused in public session after PIN change", + CKR_USER_NOT_LOGGED_IN); + + ret = funcList->C_Login(session, CKU_USER, newUserPin, newUserPinLen); + CHECK_CKR(ret, "C_Login with the new user PIN", CKR_OK); + + ret = create_private_key(session, &key); + CHECK_CKR(ret, "private object allowed after login", CKR_OK); + +cleanup: + WP11_Test_SetEmptyPinCheckHook(NULL); + if (session != 0) { + funcList->C_Logout(session); + funcList->C_CloseSession(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", EMPTY_PIN_CACHE_TEST_DIR, 1); +#endif + + (void)argc; + (void)argv; + + printf("=== wolfPKCS11 Empty-PIN Cache Test ===\n"); + + (void)empty_pin_cache_test(); + + print_results(); + + if (test_failed != 0) + return 1; + if (test_skipped != 0) + return 77; /* automake: test skipped */ + return 0; +} + +#else /* !DEBUG_WOLFPKCS11 || NO_AES || WOLFPKCS11_LEGACY_PRIVATE_FALSE_DEFAULT */ + +int main(int argc, char* argv[]) +{ + (void)argc; + (void)argv; + + printf("Debug hook or AES not available, skipping empty-PIN cache " + "test\n"); + return 77; /* automake: test skipped */ +} + +#endif diff --git a/tests/include.am b/tests/include.am index 89040ccb..3e4503c5 100644 --- a/tests/include.am +++ b/tests/include.am @@ -191,6 +191,11 @@ noinst_PROGRAMS += tests/private_object_empty_pin_test tests_private_object_empty_pin_test_SOURCES = tests/private_object_empty_pin_test.c tests_private_object_empty_pin_test_LDADD = +check_PROGRAMS += tests/empty_pin_cache_test +noinst_PROGRAMS += tests/empty_pin_cache_test +tests_empty_pin_cache_test_SOURCES = tests/empty_pin_cache_test.c +tests_empty_pin_cache_test_LDADD = + check_PROGRAMS += tests/key_sensitive_default_test noinst_PROGRAMS += tests/key_sensitive_default_test tests_key_sensitive_default_test_SOURCES = tests/key_sensitive_default_test.c @@ -343,6 +348,7 @@ tests_set_attr_readonly_test_LDADD += src/libwolfpkcs11.la tests_derive_key_type_test_LDADD += src/libwolfpkcs11.la tests_trust_attr_bufsize_test_LDADD += src/libwolfpkcs11.la tests_private_object_empty_pin_test_LDADD += src/libwolfpkcs11.la +tests_empty_pin_cache_test_LDADD += src/libwolfpkcs11.la tests_key_sensitive_default_test_LDADD += src/libwolfpkcs11.la tests_derive_disabled_key_test_LDADD += src/libwolfpkcs11.la tests_derive_inherit_protection_test_LDADD += src/libwolfpkcs11.la @@ -397,6 +403,7 @@ tests_set_attr_readonly_test_LDADD += src/libwolfpkcs11.la tests_derive_key_type_test_LDADD += src/libwolfpkcs11.la tests_trust_attr_bufsize_test_LDADD += src/libwolfpkcs11.la tests_private_object_empty_pin_test_LDADD += src/libwolfpkcs11.la +tests_empty_pin_cache_test_LDADD += src/libwolfpkcs11.la tests_key_sensitive_default_test_LDADD += src/libwolfpkcs11.la tests_derive_disabled_key_test_LDADD += src/libwolfpkcs11.la tests_derive_inherit_protection_test_LDADD += src/libwolfpkcs11.la diff --git a/wolfpkcs11/internal.h b/wolfpkcs11/internal.h index a77ad662..91ace3a1 100644 --- a/wolfpkcs11/internal.h +++ b/wolfpkcs11/internal.h @@ -413,6 +413,7 @@ WP11_LOCAL int WP11_Slot_IsUserLoggedIn(WP11_Slot* slot); WP11_LOCAL void WP11_Slot_Logout(WP11_Slot* slot); #ifdef DEBUG_WOLFPKCS11 WP11_API int WP11_Slot_TokenKeyIsZero(CK_SLOT_ID slotId); +WP11_API void WP11_Test_SetEmptyPinCheckHook(void (*hook)(void)); #if defined(WOLFPKCS11_TPM) && (!defined(NO_RSA) || defined(HAVE_ECC)) WP11_API int WP11_Test_DecodeTpmKey(CK_SLOT_ID slotId, unsigned char* keyData, int keyDataLen); From b10143dfb808190c04337adbaa500a4cf361e0c5 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Fri, 16 Oct 2026 11:49:10 +0100 Subject: [PATCH 6/6] Refuse to build Windows locking without condition variables A multi-threaded Windows build that targets Windows CE or sets _WIN32_WINNT below 0x0600 silently fell back to the two-mutex lock, which leaves a critical section from a thread that did not enter it. Fail the build with an explanation instead. Single-threaded builds are unaffected. --- src/internal.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/internal.c b/src/internal.c index 86efd224..d42448f3 100644 --- a/src/internal.c +++ b/src/internal.c @@ -205,6 +205,10 @@ typedef struct WP11_Lock { int cnt; /* Count of readers */ int writer; /* Whether a writer holds the lock */ } WP11_Lock; +#elif defined(USE_WINDOWS_API) +/* The fallback below leaves a critical section from a thread that did not + * enter it, which is undefined behaviour on Windows. */ +#error "wolfPKCS11 locking needs Windows Vista or later (_WIN32_WINNT >= 0x0600, not Windows CE); set _WIN32_WINNT or build single threaded" #else /* Fallback for threading APIs without a condition variable. The first reader * locks the write mutex and the last reader unlocks it, which may be a