Add Negative testing and Validation for wolfPKCS11

[aidangarske]

F-2379, F-2380, F-2386, F-2375, F-2389, F-2368, F-2369, F-2381, F-2382, F-2376, F-2390

[Copilot]

Pull request overview

This PR strengthens wolfPKCS11’s negative testing coverage and tightens runtime behavior around key attribute defaults, padding length calculations, and session/token cleanup, improving validation of error paths and security defaults.

Changes:

• Add multiple new negative/validation tests in pkcs11test.c (operation-not-supported checks, digest size-query behavior, truncated HMAC signature verification, private handle access).
• Fix/extend internal cleanup and access control logic (session finalization frees HMAC/CMAC/digest state; enforce private-object access in WP11_Object_Find; zero token key on logout).
• Adjust cryptographic defaults/behavior (default CKA_SENSITIVE for secret/private keys, default private-key CKA_EXTRACTABLE to false, and correct AES-CBC-PAD output sizing to always add a padding block).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
tests/pkcs11test.c	Adds negative tests and validation cases for PKCS#11 behaviors (op permission checks, digest size queries, padding sizing, private object access).
src/internal.c	Improves session/token security and correctness via additional cleanup, private-object enforcement during handle lookup, and key zeroization on logout.
src/crypto.c	Updates key attribute defaults and fixes AES-CBC-PAD ciphertext size calculation to match PKCS#7 padding behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

test_aes_cbc_pad_block_aligned_size() creates an AES key via get_aes_128_key(), which sets CKA_TOKEN=CK_TRUE, but the key is never destroyed. This leaves persistent token objects behind and can affect later tests (object count/lookup collisions, state leakage). Ensure the created key is destroyed on all exit paths (e.g., destroy after the loop or via a cleanup block).

[Copilot]

In WP11_Object_Find(), the new private-object access check reads session->slot->token.loginState without consistently holding the lock that protects loginState. In the onToken case no lock is held at all (the earlier token lock was released), while other code paths read token.loginState under slot->lock (e.g., WP11_Slot_IsLoggedIn() / WP11_Session_GetLoginState()). Use the same lock/utility for both onToken and non-token cases so login state is checked atomically and thread-safely.

Suggested change

	if (!onToken)
	WP11_Lock_LockRO(&session->slot->token.lock);
	loginState = session->slot->token.loginState;
	if (!WP11_Slot_Has_Empty_Pin(session->slot) &&
	(loginState == WP11_APP_STATE_RW_PUBLIC ||
	loginState == WP11_APP_STATE_RO_PUBLIC)) {
	ret = BAD_FUNC_ARG;
	}
	if (!onToken)
	WP11_Lock_UnlockRO(&session->slot->token.lock);
	WP11_Lock_LockRO(&session->slot->lock);
	loginState = session->slot->token.loginState;
	if (!WP11_Slot_Has_Empty_Pin(session->slot) &&
	(loginState == WP11_APP_STATE_RW_PUBLIC ||
	loginState == WP11_APP_STATE_RO_PUBLIC)) {
	ret = BAD_FUNC_ARG;
	}
	WP11_Lock_UnlockRO(&session->slot->lock);

[LinuxJedi]

@wolfSSL-Fenrir-bot review

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #179

Scan targets checked: wolfpkcs11-bugs, wolfpkcs11-compliance, wolfpkcs11-consttime, wolfpkcs11-defaults, wolfpkcs11-mutation, wolfpkcs11-proptest, wolfpkcs11-src, wolfpkcs11-zeroize

Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[wolfSSL-Fenrir-bot]

🔶 [Medium] Secret keys default to CKA_EXTRACTABLE=TRUE while private keys are hardened to FALSE
Category: Weak cryptographic defaults

The PR correctly hardens CKO_PRIVATE_KEY defaults by setting CKA_SENSITIVE=TRUE and CKA_EXTRACTABLE=FALSE. However, for CKO_SECRET_KEY, only CKA_SENSITIVE=TRUE is added while CKA_EXTRACTABLE remains defaulting to trueVal. This asymmetry means that AES keys, HMAC keys, and other secret keys created without explicitly setting CKA_EXTRACTABLE=FALSE can be extracted from the token by default. Since this PR is explicitly focused on hardening insecure defaults (commit F-2368), the omission of CKA_EXTRACTABLE=FALSE for secret keys is inconsistent with the stated security goals. A caller who creates a secret key without specifying CKA_EXTRACTABLE will get an extractable key, potentially allowing key material to be exported via C_WrapKey or C_GetAttributeValue.

case CKO_SECRET_KEY:
    if (ret == CKR_OK)
        ret = SetIfNotFound(obj, CKA_SENSITIVE, trueVal, pTemplate,
                            ulCount);
    if (ret == CKR_OK)
        ret = SetIfNotFound(obj, CKA_EXTRACTABLE, trueVal, pTemplate,
                            ulCount);

Recommendation: Change the CKO_SECRET_KEY default for CKA_EXTRACTABLE from trueVal to falseVal, consistent with the hardening applied to CKO_PRIVATE_KEY. If there is a deliberate reason to keep secret keys extractable by default (e.g., key backup workflows), document this decision in a code comment and consider gating the permissive default behind a compile-time flag.

[wolfSSL-Fenrir-bot]

🔶 [Medium] CKA_PRIVATE enforcement in WP11_Object_Find bypassed when token PIN is empty
Category: Fail-open behavior

The new CKA_PRIVATE access control check in WP11_Object_Find contains a fail-open condition: if (!WP11_Slot_Has_Empty_Pin(session->slot) && (loginState == WP11_APP_STATE_RW_PUBLIC || loginState == WP11_APP_STATE_RO_PUBLIC)). When WP11_Slot_Has_Empty_Pin returns true, the entire private-object gate is skipped, allowing unauthenticated sessions to access objects marked CKA_PRIVATE=TRUE. While this may be intentional for uninitialized tokens (where C_Login would fail since no PIN exists), it creates a structural risk: any code path that causes the PIN to appear empty — including potential interactions with the wc_ForceZero(slot->token.key, sizeof(slot->token.key)) added in WP11_Slot_Logout at line 6983 — could open all private objects to unauthenticated access. The PKCS#11 specification gates CKA_PRIVATE access on login state (C_Login having been called), not on PIN existence. Coupling the access control decision to token PIN state conflates authentication enforcement with token initialization state.

if ((obj->opFlag & WP11_FLAG_PRIVATE) == WP11_FLAG_PRIVATE) {
    int loginState;
    if (!onToken)
        WP11_Lock_LockRO(&session->slot->token.lock);
    loginState = session->slot->token.loginState;
    if (!WP11_Slot_Has_Empty_Pin(session->slot) &&
        (loginState == WP11_APP_STATE_RW_PUBLIC ||
         loginState == WP11_APP_STATE_RO_PUBLIC)) {
        ret = BAD_FUNC_ARG;
    }
    if (!onToken)
        WP11_Lock_UnlockRO(&session->slot->token.lock);
}

Recommendation: Gate private-object access solely on session login state rather than on PIN existence. The check should be: if the session is in a public state, deny access to private objects unconditionally. If uninitialized tokens need a special bootstrapping mode, enforce that via an explicit build-time flag (e.g., WOLFPKCS11_ALLOW_UNINITIALIZED_ACCESS) rather than a runtime PIN-state check. Additionally, verify that wc_ForceZero(slot->token.key, ...) on logout does not affect the data that WP11_Slot_Has_Empty_Pin inspects — if the PIN state and the encryption key share storage, logout could inadvertently disable private-object protection for subsequent sessions on that slot.

[wolfSSL-Fenrir-bot]

🔶 [Medium] wp11_Session_Final: session->init = 0 in HMAC/CMAC blocks may prevent concurrent digest resource cleanup
Category: Surviving deletion mutations

The new cleanup blocks in wp11_Session_Final each set session->init = 0 after freeing their respective resource. The existence of WP11_INIT_DIGEST_MASK (used to mask out digest bits when checking the operation type) implies that a digest operation can run concurrently with another operation (e.g., encrypt-and-digest or sign-and-digest as per PKCS#11 dual-function operations). If session->init holds both HMAC state (in the non-digest-mask bits) and a concurrent digest state (in the WP11_INIT_DIGEST_MASK bits), the HMAC cleanup block will match, free the HMAC context, and zero session->init entirely. The subsequent digest cleanup block if ((session->init & ~WP11_INIT_DIGEST_MASK) == WP11_INIT_DIGEST) then sees session->init == 0, does not match, and wc_HashFree is never called — leaking the digest hash context. The same applies to the CMAC block. If a mutation removed the wc_HashFree call in the digest block, no test involving concurrent digest+sign sessions during session teardown would detect it, because the HMAC block already prevents the digest block from executing.

if ((session->init & ~WP11_INIT_DIGEST_MASK) == WP11_INIT_HMAC_SIGN ||
    (session->init & ~WP11_INIT_DIGEST_MASK) == WP11_INIT_HMAC_VERIFY) {
    wc_HmacFree(&session->params.hmac.hmac);
    session->init = 0;  /* clears ALL bits including digest state */
}
...
if ((session->init & ~WP11_INIT_DIGEST_MASK) == WP11_INIT_DIGEST) {
    wc_HashFree(&session->params.digest.hash,
                 session->params.digest.hashType);
    session->init = 0;
}

Recommendation: In the HMAC and CMAC cleanup blocks, preserve the digest-mask bits instead of zeroing all state. Replace session->init = 0 with session->init &= WP11_INIT_DIGEST_MASK so that any concurrent digest state survives for the subsequent digest cleanup block to handle. Similarly, the digest block should only clear the digest-related bits: session->init &= ~WP11_INIT_DIGEST_MASK. Alternatively, if operations are truly mutually exclusive, use else if chains to make the exclusivity explicit and self-documenting.

[wolfSSL-Fenrir-bot]

🔹 [Low] Integer overflow in AES-CBC-PAD encrypted length calculation for near-maximum word32 inputs
Category: Integer overflows

The new PKCS#7 padding size formula ((ulDataLen / AES_BLOCK_SIZE) + 1) * AES_BLOCK_SIZE can overflow word32 when ulDataLen is near the maximum 32-bit value. Specifically, for ulDataLen >= 0xFFFFFFF0 (4294967280), the intermediate value (ulDataLen / 16) + 1 equals 0x10000000, and multiplying by 16 produces 0x100000000 which wraps to 0 in word32 arithmetic. This would set encDataLen = 0, causing the subsequent size check (*pulEncryptedDataLen < encDataLen) to pass trivially, and the encryption would then write up to a full AES block set past the start of a potentially zero-sized or undersized caller buffer.

The CK_ULONG_FITS_WORD32(ulDataLen) check at line 2279 validates that ulDataLen fits in word32, but does not account for the additional block of padding making the result overflow word32. The old formula ((ulDataLen + AES_BLOCK_SIZE - 1) / AES_BLOCK_SIZE) * AES_BLOCK_SIZE did not overflow at this boundary for block-aligned inputs (though it had a different correctness bug that this PR fixes).

The practical impact is very low because encrypting ~4GB through a single C_Encrypt call is extremely unlikely in real PKCS#11 usage.

/* PKCS#7 padding always adds at least 1 byte */
            encDataLen = (word32)((ulDataLen / AES_BLOCK_SIZE) + 1) *
                        AES_BLOCK_SIZE;

Recommendation: Add an explicit upper-bound check on ulDataLen before the padding calculation to ensure the result fits in word32: if (ulDataLen > (CK_ULONG)(UINT32_MAX - AES_BLOCK_SIZE)) return CKR_DATA_LEN_RANGE; This mirrors the intent of the existing CK_ULONG_FITS_WORD32 check but accounts for the added padding block.