PKCS11 access-control, session-state, and concurrency hardening#207
Draft
julek-wolfssl wants to merge 14 commits into
Draft
PKCS11 access-control, session-state, and concurrency hardening#207julek-wolfssl wants to merge 14 commits into
julek-wolfssl wants to merge 14 commits into
Conversation
WP11_Slot_CloseSessions freed/finalized every session but never reset the token login state, unlike the single-session WP11_Slot_CloseSession path which calls WP11_Slot_Logout once no in-use sessions remain. Because loginState is token-level, a session opened after C_CloseAllSessions inherited the stale RW_USER/RW_SO state and was treated as logged in without any C_Login, exposing CKA_PRIVATE objects and SO/User functions. Call WP11_Slot_Logout after closing all sessions, mirroring the single-session close path.
session->params is a union shared by all mechanisms and is only zeroed at session allocation. Every other Set*Params routine re-initializes its parameters, but WP11_Session_SetCbcParams left cbc->partial/partialSz untouched. A CBC operation following a different mechanism could therefore read a stale partialSz: values 1..15 silently corrupt the stream, and a value >16 makes sz = AES_BLOCK_SIZE - partialSz negative, which widens to a huge size_t in the XMEMCPY and overflows the 16-byte partial buffer. Zero partial/partialSz at (re)initialization, matching SetGcmParams.
C_CopyObject created the new object's CKA_PRIVATE flag from the caller
template without checking the session login state. A public (unauthenticated)
session holding any copyable non-private object handle could call
C_CopyObject with {CKA_PRIVATE=TRUE, CKA_TOKEN=TRUE} to create a persistent
private token object without ever calling C_Login. The R/W gate only covered
CKA_TOKEN placement and WP11_Object_Find only enforced login on the source
object.
Gate the copy with CheckPrivateLogin (the same helper used by
C_CreateObject/C_GenerateKey for Fenrir 3144). WP11_NO_IMPLICIT_CLASS is
used so only an explicit template CKA_PRIVATE=TRUE triggers the check - the
inherited default already had its login enforced on the source object.
SetAttributeValue guarded CKA_SENSITIVE/EXTRACTABLE/COPYABLE/DESTROYABLE but not CKA_TRUSTED. Per PKCS#11 v2.40 sec 4.5 only the SO may set CKA_TRUSTED to CK_TRUE. Without the guard a regular-user session could forge CKA_TRUSTED on a wrapping key it owns (via C_SetAttributeValue or directly in C_CreateObject) and then use C_WrapKey to export keys protected by CKA_WRAP_WITH_TRUSTED, defeating that export control. Reject setting CKA_TRUSTED=CK_TRUE from any non-SO session. The check is not qualified with newObject so it also blocks C_CreateObject/C_GenerateKey from minting a trusted key. De-trusting (CK_TRUE->CK_FALSE) is left unrestricted: PKCS#11 does not make CKA_TRUSTED sticky. test_wrap_key_wrap_with_trusted created the trusted wrapping key under the harness user session, which the guard now correctly rejects. Rework it to provision the trusted key as a public token object under an SO session (logout user / login SO / create / logout SO / login user) and add an assertion that a user session forging CKA_TRUSTED is rejected.
WP11_SetStoreDir held no lock while doing XFREE(storeDir)/XMALLOC/XMEMCPY. A concurrent C_Finalize -> WP11_Library_Final frees the same global storeDir (after releasing globalLock), so the window between XMALLOC and XMEMCPY could free the fresh block and turn the XMEMCPY of attacker-controlled configdir bytes into a heap use-after-free or NULL-pointer write. Add a permanently-live leaf mutex storeDirLock (statically initialized, gated like storeDir on NSS && !TPM_STORE) and hold it across the whole free/malloc/copy in WP11_SetStoreDir and across the free in WP11_Library_Final. It is always taken as a leaf so it cannot invert any lock ordering; in Library_Final it nests inside libraryInitLock in a fixed order.
wolfPKCS11_Store_Name read the module-global storeDir with no lock and then dereferenced it (XSTRLEN/XSNPRINTF) to build the token path. A concurrent C_Finalize -> WP11_Library_Final frees storeDir after releasing globalLock, so the read path could use freed memory. Under the same storeDirLock introduced for F-5868, copy storeDir into a local buffer and release the lock before formatting the path, so the free cannot race the use. The lock is a leaf (nothing else is acquired while held), so the token->lock -> storeDirLock order on this path introduces no deadlock.
WP11_AesCbc_EncryptUpdate/DecryptUpdate (and WP11_AesCbcPad_DecryptUpdate) performed a non-atomic read-modify-write on cbc->partialSz with no lock held: WP11_Session_Get releases slot->lock before returning the session pointer and WP11_Session has no lock of its own. Two threads sharing one session handle could both read partialSz=N, both XMEMCPY into cbc->partial+N and both add, leaving partialSz = N + 2*(AES_BLOCK_SIZE-N) > 16. The next update then makes sz = AES_BLOCK_SIZE - partialSz negative, which widens to a huge size_t in XMEMCPY and overflows the 16-byte partial buffer into adjacent session memory. Hold session->slot->lock (RW) across the whole body of each *Update, matching the existing lock idiom. WP11_AesCbcPad_EncryptUpdate is left untouched: it forwards to WP11_AesCbc_EncryptUpdate, which now locks, so locking it too would self-deadlock (the RW lock is non-recursive). No caller holds slot->lock at dispatch, so no ordering or deadlock is introduced; in SINGLE_THREADED the lock macros compile to no-ops.
The default (non-FIPS, non-PBKDF2, non-scrypt) build derives the SO/user PIN hashes and the AES-256 token storage key with a single unsalted SHA-256 pass that discards the persisted per-token seed. This offers no salt and no key stretching, letting an attacker who obtains the token-store file brute-force a weak PIN offline and recover the storage key. The KDF is selected at compile time and was otherwise silent, so integrators were unaware the strong KDF was not compiled in. Emit a portable build-time warning (#pragma message on MSVC, #warning on GCC/Clang) on this path, silenceable with WOLFPKCS11_ALLOW_WEAK_PIN_KDF, and document the weakness in-code. Actually strengthening the derivation in place (salting/stretching, or enabling PBKDF2 by default) is deliberately NOT done here: it changes the persisted PIN-hash and storage-key bytes and would render every existing default-build token store unrecoverable with no migration path. That compatibility break is a maintainer/product decision.
When WOLFSSL_MAXQ10XX_CRYPTO was set without MAXQ10XX_PRODUCTION_KEY, a 96-byte hardcoded P-256 key pair was compiled in and used by ECDSA_sign to authenticate every root certificate provisioned into the MAXQ10xx secure element. The private scalar is public in the source, so anyone could forge a valid provisioning signature for an arbitrary malicious root certificate, reachable via C_CreateObject(CKO_CERTIFICATE, CKA_TOKEN=TRUE). Only a source comment guarded this; a production build missing MAXQ10XX_PRODUCTION_KEY silently shipped the test key. Make the insecure test key opt-in: production builds must define MAXQ10XX_PRODUCTION_KEY, and using the built-in test key now requires explicitly defining WOLFPKCS11_MAXQ10XX_TEST_KEY. With neither, the build fails with an #error instead of silently embedding a public private key. Only WOLFSSL_MAXQ10XX_CRYPTO builds are affected; such dev builds must now pass -DWOLFPKCS11_MAXQ10XX_TEST_KEY.
When pMechanismList was too small, C_GetMechanismList returned CKR_BUFFER_TOO_SMALL without writing the required count to *pulCount. PKCS#11 mandates that *pulCount be set on this error path so the standard two-call (size-query then fetch) idiom works; callers could otherwise loop or fail. Set *pulCount = mechanismCnt before returning, mirroring the success path.
…141) The CKR_SESSION_EXISTS check was gated behind WP11_Slot_IsTokenInitialized, so a never-initialized token with open sessions skipped it and proceeded to WP11_Slot_TokenReset. PKCS#11 requires an open session on the token to fail C_InitToken with CKR_SESSION_EXISTS regardless of initialization state. Hoist the WP11_Slot_HasSession check out of the IsTokenInitialized branch so it runs unconditionally before any token-reset logic.
slot.c gated CKM_AES_KEY_WRAP/CKM_AES_KEY_WRAP_PAD advertisement (mechanism list and mechanismInfo) on HAVE_AES_KEY_WRAP, while every implementation path in crypto.c/internal.c gates on the wolfSSL-standard HAVE_AES_KEYWRAP. The two macros are independent, so a normal build (HAVE_AES_KEYWRAP from wolfSSL) implemented the mechanisms but never advertised them, while --enable-aeskeywrap (HAVE_AES_KEY_WRAP only) advertised them but EncryptInit/DecryptInit rejected them with CKR_MECHANISM_INVALID. Gate slot.c on HAVE_AES_KEYWRAP so advertisement matches the implementation in every build. slot.c receives HAVE_AES_KEYWRAP transitively from wolfSSL, the same source the implementation uses. The build-system macro is intentionally not renamed: --enable-aeskeywrap's -DHAVE_AES_KEY_WRAP path also feeds DISABLE_DEFS, and renaming it would regress the default/CI config.
On --enable-nss builds WP11_MIN_PIN_LEN defaults to 0, permitting a zero-length user PIN which intentionally disables PIN-based authentication (C_GetTokenInfo clears CKF_LOGIN_REQUIRED and token objects are decoded at load without C_Login). This is required for NSS tools (certutil/PK11_InitPin) that bootstrap empty-password databases. The default is deliberately left unchanged: flipping it to non-zero would break that NSS bootstrap and the empty-PIN CI/tests. Document the behavior and the -DWP11_MIN_PIN_LEN=N opt-in in the header and README so integrators are aware the auth model is disabled by default on NSS.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues wolfPKCS11 hardening work by tightening PKCS#11 access-control rules, correcting session/token login-state behavior, and adding serialization around a few concurrency-sensitive code paths (especially for NSS builds and AES-CBC streaming).
Changes:
- Enforces stricter access control: SO-only
CKA_TRUSTED=TRUE, and requires authenticated user login when copying objects asCKA_PRIVATE=TRUE. - Fixes conformance/session-state issues: logout on
C_CloseAllSessions, unconditionalCKR_SESSION_EXISTSforC_InitTokenwhen any session exists, and correct*pulCountbehavior forCKR_BUFFER_TOO_SMALLinC_GetMechanismList. - Adds concurrency hardening: serializes
storeDirlifetime/reads vsC_Finalize, resets CBC streaming state, and serializes AES-CBC multipart partial-block updates.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
wolfpkcs11/internal.h |
Documents NSS empty-PIN default behavior and how to override minimum PIN length. |
tests/pkcs11test.c |
Updates AES key-wrap trusted-wrapping-key test to provision CKA_TRUSTED under an SO session and adds a user-forge rejection check. |
src/slot.c |
Conformance fixes: correct C_GetMechanismList count semantics and session-exists rejection ordering in C_InitToken; adjusts AES key-wrap advertisement gating macro. |
src/internal.c |
Adds storeDir locking for NSS finalize/set/read races; logs out token on bulk session close; resets CBC params state; serializes AES-CBC multipart partial-block updates. |
src/crypto.c |
Enforces SO-only CKA_TRUSTED=TRUE setting; requires login when copying objects as private via template. |
README.md |
Documents NSS empty-PIN / WP11_MIN_PIN_LEN default behavior and override mechanism. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
384
to
390
| #endif | ||
| #ifndef NO_AES | ||
| CKM_AES_KEY_GEN, | ||
| #ifdef HAVE_AES_KEY_WRAP | ||
| #ifdef HAVE_AES_KEYWRAP | ||
| CKM_AES_KEY_WRAP, | ||
| CKM_AES_KEY_WRAP_PAD, | ||
| #endif |
Comment on lines
+6845
to
6853
| #ifdef WP11_HAVE_STORE_DIR_LOCK | ||
| wc_LockMutex(&storeDirLock); | ||
| #endif | ||
| XFREE(storeDir, NULL, DYNAMIC_TYPE_TMP_BUFFER); | ||
| storeDir = NULL; | ||
| #ifdef WP11_HAVE_STORE_DIR_LOCK | ||
| wc_UnLockMutex(&storeDirLock); | ||
| #endif | ||
| #endif |
…868) The free path acquired storeDirLock without checking wc_LockMutex's return and always called wc_UnLockMutex, which is an invalid unlock (and may fail/crash on some mutex implementations) if the lock was not actually acquired. Track the lock result and only unlock when it succeeded. (WP11_SetStoreDir already returns BAD_MUTEX_E on failure; WP11_Library_Final is void so it proceeds with the free either way, but no longer unlocks a lock it does not hold.)
julek-wolfssl
force-pushed
the
wolfpkcs11-fenrir-security-fixes
branch
from
July 16, 2026 14:22
4433f1a to
fab1cb1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This branch continues the PKCS#11 hardening work with a set of access-control, session-state, and concurrency fixes (tracked as F-* items).
Access control
CKA_TRUSTEDCKA_PRIVATE=TRUEWP11_Slot_CloseSessionsnever reset the token login state, so a session opened afterC_CloseAllSessionsinherited a staleRW_USER/RW_SOstate and was treated as logged in without anyC_LoginConcurrency
partialSzupdates on a sessionstoreDirwrites against theC_Finalizefree in NSS buildsstoreDirreads against theC_Finalizefree in NSS buildsSession / crypto state
WP11_Session_SetCbcParamsConformance
C_InitTokenwith an open session regardless of init state*pulCountonCKR_BUFFER_TOO_SMALLinC_GetMechanismListHardening / documentation
Overlap with #204: the F-6492 commit here is the same fix as #204 (adding
WP11_Slot_LogouttoWP11_Slot_CloseSessions). Whichever lands first, the other should drop it.