Add support for CKR_OPERATION_ACTIVE

[LinuxJedi]

Stops overwriting previous state.

F-1616

[Copilot]

Pull request overview

This PR aims to implement PKCS#11-compliant CKR_OPERATION_ACTIVE behavior for operation init calls (preventing an active operation’s session state from being overwritten), and adds regression coverage for bug #1616 (F-1616).

Changes:

• Add WP11_Session_IsOpInitializedExact() and integrate CKR_OPERATION_ACTIVE checks into Encrypt/Decrypt/Sign/Verify init paths.
• Clear session operation-init state after completing single-part operations (e.g., Encrypt/Decrypt/Digest/Sign/Verify).
• Add a dedicated operation_active_test and update existing tests to re-init/cleanup between verify/sign scenarios; update test build rules and ignore patterns.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
wolfpkcs11/internal.h	Declares new session helper for exact init-value comparisons.
src/internal.c	Implements new helper; adjusts session finalization and digest state restore behavior.
src/crypto.c	Adds CKR_OPERATION_ACTIVE checks on *_Init functions; clears init state after single-part operations.
tests/pkcs11test.c	Adds additional VerifyInit / Sign / SignFinal “cleanup” calls in signature/HMAC tests.
tests/pkcs11mtt.c	Mirrors pkcs11test updates for multi-thread test suite.
tests/operation_active_test.c	New regression test asserting double *_Init returns CKR_OPERATION_ACTIVE.
tests/include.am	Builds/runs the new operation_active_test binary.
.gitignore	Broadens ignoring of test/store build artifacts and adds some dev-tool files.

---

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

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #176

Scan targets checked: wolfpkcs11-bugs, wolfpkcs11-compliance, wolfpkcs11-src
Findings: 1

Medium (1)

CKR_OPERATION_ACTIVE check bypassed when different mechanism is used for same operation type

File: src/crypto.c:2022-2027
Function: EncryptInit, DecryptInit, C_SignInit, C_VerifyInit, C_VerifyRecoverInit
Category: PKCS#11 API contract violations

The new WP11_Session_IsOpInitializedExact(session, init) check compares session->init == init, where init is the value computed for the mechanism being requested. If a different mechanism of the same operation type is already active (e.g., AES-CBC encrypt is active and the caller requests AES-GCM encrypt), the exact comparison will not match because the two mechanisms produce different init values (e.g., WP11_INIT_AES_CBC_ENC vs WP11_INIT_AES_GCM_ENC). The check returns false, and the code proceeds to overwrite the existing operation state via WP11_Session_SetMechanism/WP11_Session_SetObject/WP11_Session_SetOpInitialized — the exact state-clobbering bug this PR aims to prevent. This applies to EncryptInit (line ~2025), DecryptInit (line ~2997), C_SignInit (line ~4421), C_VerifyInit (line ~5490), and C_VerifyRecoverInit (line ~6127). Per PKCS#11 spec section 5.8 (and analogous sections for sign/verify), C_EncryptInit must return CKR_OPERATION_ACTIVE when any encryption operation is already active, regardless of which mechanism was used. In contrast, C_DigestInit correctly uses WP11_Session_IsOpInitialized(session, WP11_INIT_DIGEST) which masks out mechanism-specific bits and catches any active digest operation. The same broader check pattern should be used for encrypt, decrypt, sign, and verify operations.

if (WP11_Session_IsOpInitializedExact(session, init)) {
        rv = CKR_OPERATION_ACTIVE;
        WOLFPKCS11_LEAVE("C_EncryptInit", rv);
        return rv;
    }
    WP11_Session_SetMechanism(session, pMechanism->mechanism);
    WP11_Session_SetObject(session, obj);
    WP11_Session_SetOpInitialized(session, init);

Recommendation: Replace WP11_Session_IsOpInitializedExact with a broader check that detects any active operation of the same type, not just the exact same mechanism. For example, add a function like WP11_Session_IsAnyOpActive(session, opType) that checks whether any encrypt/decrypt/sign/verify init value is currently set, regardless of the specific mechanism. Alternatively, check session->init != 0 if only one operation can be active at a time, or use a masked comparison similar to what WP11_Session_IsOpInitialized does for digest operations. The DigestInit pattern using WP11_Session_IsOpInitialized(session, WP11_INIT_DIGEST) is the correct model to follow.

---

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

[Copilot]

Pull request overview

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

---

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

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #176

Scan targets checked: wolfpkcs11-bugs, wolfpkcs11-compliance, wolfpkcs11-src
Findings: 5

High (1)

CKR_OPERATION_ACTIVE check uses exact init match, failing to detect cross-mechanism active operations

File: src/crypto.c:2025-2029,2997-3001,4422-4426,5491-5495,6128-6132
Function: EncryptInit, DecryptInit, C_SignInit, C_VerifyInit, C_VerifyRecoverInit
Category: Cryptographic operation sequencing violations

The new WP11_Session_IsOpInitializedExact() function performs session->init == init, where init is a mechanism-specific value set in the per-mechanism switch cases (e.g., WP11_INIT_ECDSA_SIGN for ECDSA signing). This means the CKR_OPERATION_ACTIVE check only detects re-initialization with the exact same mechanism. If a different mechanism of the same operation type is already active (e.g., an HMAC sign operation is active and C_SignInit is called with ECDSA, or AES-CBC encrypt is active and C_EncryptInit is called with AES-GCM), the exact comparison will not match and the second C_*Init call will silently overwrite the active operation instead of returning CKR_OPERATION_ACTIVE.

Per PKCS#11 v2.40/v3.0 §5.9 (C_EncryptInit): "If there is an active encryption operation, the function returns CKR_OPERATION_ACTIVE." The same requirement applies to C_DecryptInit (§5.10), C_SignInit (§5.12), C_VerifyInit (§5.13), and C_DigestInit (§5.11). The spec does not qualify this by mechanism — ANY active operation of the same type must be detected.

Additionally, the check is placed AFTER the mechanism-specific switch statement. If the switch cases perform any session-state-modifying operations (e.g., initializing crypto contexts in session->params), the active operation's state may already be corrupted before the active-operation check is reached.

For C_DigestInit, init = WP11_INIT_DIGEST appears to be a fixed value (not mechanism-specific), so the exact check may work for digest. But the existing WP11_Session_IsOpInitialized masks out digest bits via WP11_INIT_DIGEST_MASK, suggesting session->init can have hash-algorithm bits OR'd in during digest processing, which would also cause the exact check to fail.

int WP11_Session_IsOpInitializedExact(WP11_Session* session, int init)
{
    return session->init == init;
}

/* In EncryptInit, after mechanism switch: */
    if (WP11_Session_IsOpInitializedExact(session, init)) {
        rv = CKR_OPERATION_ACTIVE;
        WOLFPKCS11_LEAVE("C_EncryptInit", rv);
        return rv;
    }

Recommendation: The CKR_OPERATION_ACTIVE check should detect ANY active operation of the same type, not just the exact same mechanism. Consider: (1) moving the check to the top of each Init function, before any mechanism-specific processing; (2) using a check that tests whether any operation of the relevant category is active, such as checking session->init != 0 (if only one operation can be active at a time) or using a category-based mask/range check rather than exact equality. For example, check session->init != 0 early in each Init function, since per the PKCS#11 spec a session can have at most one active operation of each type.

---

Medium (4)

C_Digest does not clear operation state on error, unlike C_DigestFinal

File: src/crypto.c:3822
Function: C_Digest
Category: Incorrect error handling

In C_Digest, the new code guards the operation-state clear with ret == CKR_OK:

if (pDigest != NULL && ret == CKR_OK)
    WP11_Session_SetOpInitialized(session, 0);

However, in C_DigestFinal, the analogous new code correctly clears without checking ret:

if (pDigest != NULL)
    WP11_Session_SetOpInitialized(session, 0);

Per PKCS#11 (section 5.11.2), C_Digest must always terminate the active digest operation on any return value other than CKR_BUFFER_TOO_SMALL (which corresponds to the pDigest == NULL query path). If C_Digest fails with pDigest != NULL, the ret == CKR_OK guard prevents the operation from being cleared. Combined with the new CKR_OPERATION_ACTIVE check in C_DigestInit, this leaves the session permanently stuck — the caller cannot retry the failed digest and cannot start a new one without closing the session.

if (pDigest != NULL && ret == CKR_OK)
        WP11_Session_SetOpInitialized(session, 0);
    return ret;

Recommendation: Remove the && ret == CKR_OK guard to match the C_DigestFinal pattern:

if (pDigest != NULL)
    WP11_Session_SetOpInitialized(session, 0);

---

IsOpInitializedExact may miss active operations when hash algorithm differs

File: src/crypto.c:4419
Function: C_SignInit
Category: Logic errors

All *Init functions use the new WP11_Session_IsOpInitializedExact to check for an already-active operation. This function compares session->init == init without masking. The existing WP11_Session_IsOpInitialized intentionally masks out WP11_INIT_DIGEST_MASK bits before comparing, because for hash-based sign/verify mechanisms (e.g., CKM_SHA256_RSA_PKCS vs CKM_SHA384_RSA_PKCS), the init value encodes both the operation type and the hash algorithm. If a sign operation is active with one hash algorithm and C_SignInit is called again with a different hash algorithm, IsOpInitializedExact returns false (the full init values differ), bypassing the CKR_OPERATION_ACTIVE check and overwriting the active operation state. The same issue applies to C_VerifyInit. Per PKCS#11, any active sign operation should block a new C_SignInit regardless of mechanism parameters.

if (WP11_Session_IsOpInitializedExact(session, init)) {
        rv = CKR_OPERATION_ACTIVE;
        WOLFPKCS11_LEAVE("C_SignInit", rv);
        return rv;
    }

Recommendation: Use WP11_Session_IsOpInitialized (which masks out WP11_INIT_DIGEST_MASK) instead of WP11_Session_IsOpInitializedExact for C_SignInit and C_VerifyInit, or check whether the operation-type bits (without hash bits) already indicate an active operation of the same class.

---

C_Verify clears operation state before returning CKR_MECHANISM_INVALID, leaving operation active on invalid mechanism path

File: src/crypto.c:5795,6034
Function: C_Verify, C_VerifyFinal
Category: Cryptographic operation sequencing violations

In C_Verify and C_VerifyFinal, the WP11_Session_SetOpInitialized(session, 0) call is placed AFTER the mechanism switch's default case which returns CKR_MECHANISM_INVALID. This means if the mechanism switch hits the default case and returns early, the active verify operation is NOT cleared. Per PKCS#11 v2.40 §5.13.3 (C_Verify): a call to C_Verify always terminates the active verify operation. If the mechanism is invalid at this point (which should not happen since C_VerifyInit validated it, but is possible if session state is corrupted), the operation remains active and subsequent calls to C_VerifyInit would correctly return CKR_OPERATION_ACTIVE, potentially leaving the session stuck.

The same pattern exists in C_Verify (line 5795) and C_VerifyFinal (line 6034), where the SetOpInitialized(session, 0) is placed after the switch but the default case returns before reaching it.

/* In C_Verify: */
            (void)ulSignatureLen;
            return CKR_MECHANISM_INVALID;
    }
    WP11_Session_SetOpInitialized(session, 0);
    if (ret < 0)
        return CKR_FUNCTION_FAILED;

Recommendation: Move the WP11_Session_SetOpInitialized(session, 0) call to before the mechanism switch, or ensure it is reached on all exit paths including the CKR_MECHANISM_INVALID default case. For example:

    default:
        WP11_Session_SetOpInitialized(session, 0);
        return CKR_MECHANISM_INVALID;

The same fix should be applied to C_VerifyFinal. Note that the same pattern may exist in C_Sign, C_Encrypt, and C_Decrypt — all early-return paths from the mechanism switch should clear the active operation.

---

CKR_OPERATION_ACTIVE check bypassed by using a different mechanism

File: src/crypto.c:2025-2030
Function: EncryptInit
Category: PKCS#11 API contract violations

WP11_Session_IsOpInitializedExact compares session->init == init using exact equality. Since each mechanism sets a distinct init value (e.g., WP11_INIT_AES_CBC_ENC for AES-CBC vs WP11_INIT_AES_GCM_ENC for AES-GCM), calling an Init function with a different mechanism than the currently active one will pass the check and silently overwrite the active operation state. Per PKCS#11 spec (Section 5.8-5.12), CKR_OPERATION_ACTIVE must be returned if any operation of that type is already active, regardless of mechanism.

This affects all six Init functions modified in this PR: EncryptInit, DecryptInit, C_DigestInit, C_SignInit, C_VerifyInit, and C_VerifyRecoverInit. For example, if AES-CBC encryption is active (session->init == WP11_INIT_AES_CBC_ENC) and C_EncryptInit is called with AES-GCM (init = WP11_INIT_AES_GCM_ENC), the exact comparison fails, the check is bypassed, and WP11_Session_SetOpInitialized(session, WP11_INIT_AES_GCM_ENC) overwrites the AES-CBC state. This can corrupt in-progress cryptographic operations.

The existing WP11_Session_IsOpInitialized uses masking (session->init & ~WP11_INIT_DIGEST_MASK) which could be adapted with a broader operation-type mask, or a separate check could verify session->init != 0 before allowing a new Init.

int WP11_Session_IsOpInitializedExact(WP11_Session* session, int init)
{
    return session->init == init;
}

/* In EncryptInit - init is mechanism-specific: */
    if (WP11_Session_IsOpInitializedExact(session, init)) {
        rv = CKR_OPERATION_ACTIVE;
        WOLFPKCS11_LEAVE("C_EncryptInit", rv);
        return rv;
    }

Recommendation: Replace the exact-match check with a broader check that detects any active operation. For example, check session->init != 0 as a first guard (if the session only supports one concurrent operation), or introduce operation-type masks (e.g., WP11_INIT_ENCRYPT_MASK, WP11_INIT_SIGN_MASK) and check (session->init & WP11_INIT_ENCRYPT_MASK) != 0 to catch any encrypt operation regardless of specific mechanism.

---

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

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #176

Scan targets checked: wolfpkcs11-bugs, wolfpkcs11-compliance, wolfpkcs11-src

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

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

[Copilot]

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.

---

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

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #176

Scan targets checked: wolfpkcs11-bugs, wolfpkcs11-compliance, wolfpkcs11-src

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

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

[Copilot]

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

---

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

[Copilot]

Pull request overview

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

---

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

[Copilot]

This uses ret == 0 while the nearby C_Digest path uses ret == CKR_OK. Since this function returns a CK_RV, comparing against CKR_OK is clearer and consistent (even if CKR_OK is currently 0). Aligning the comparison also reduces the chance of subtle bugs if return conventions change or if ret is refactored.

Suggested change

	if (pDigest != NULL && ret == 0)
	if (pDigest != NULL && ret == CKR_OK)

[Copilot]

The cleanup call uses the same buffer for input and output (pData == pEncryptedData). PKCS#11 does not generally guarantee in-place operation safety, and some backends may not support overlapping buffers. To make the test portable/robust, use separate input and output buffers for cleanup (same applies to the analogous C_Decrypt cleanup).

[dgarske]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 5 total — 5 posted, 0 skipped

Posted findings

• [High] CKR_OPERATION_ACTIVE check placed after session state mutation corrupts existing operation — src/crypto.c:2085-2089
• [Medium] IsOpInitializedExact does not catch cross-mechanism re-initialization — src/internal.c:7160-7163
• [Medium] C_Digest and C_DigestFinal only clear operation state on success — src/crypto.c:3920-3921
• [Medium] Tests do not verify first operation remains usable after CKR_OPERATION_ACTIVE — tests/operation_active_test.c:230-256
• [Low] Cosmetic blank line removals reduce readability — src/crypto.c:1857

Review generated by Skoll via openclaw

[dgarske]

🟠 [High] CKR_OPERATION_ACTIVE check placed after session state mutation corrupts existing operation
🚫 BLOCK bug

In EncryptInit, DecryptInit, SignInit, and VerifyInit, the WP11_Session_IsOpInitializedExact() check is placed AFTER the mechanism switch block. The switch block calls functions that mutate session state — WP11_Session_SetCbcParams, WP11_Session_SetGcmParams, WP11_Session_SetCcmParams, WP11_Session_SetCtrParams, WP11_Session_SetCtsParams, WP11_Session_SetOaepParams, WP11_Session_SetAesWrapParams, WP11_Session_SetPssParams, WP11_Session_SetMldsaParams, WP11_Hmac_Init, WP11_Aes_Cmac_Init, etc. When the check triggers and returns CKR_OPERATION_ACTIVE, the first operation's parameters have already been overwritten. Per PKCS#11 spec §5.2, returning CKR_OPERATION_ACTIVE must leave the existing operation undisturbed.

For example, calling C_EncryptInit(AES-CBC, iv1) then C_EncryptInit(AES-CBC, iv2) would: (1) call SetCbcParams(session, iv2, ...) overwriting iv1, (2) detect the operation is active, (3) return CKR_OPERATION_ACTIVE — but the first operation's IV is now iv2 instead of iv1. Similarly, C_SignInit(SHA256_HMAC) called twice would re-initialize the HMAC context via WP11_Hmac_Init before the check, corrupting the first sign operation.

The same issue exists in DecryptInit (line ~3078), SignInit (lines 4247, 4518), and VerifyInit (line 5594).

Suggestion:

Suggested change

	if (WP11_Session_IsOpInitializedExact(session, init)) {
	Move the operation-active check BEFORE the mechanism switch block. Since `init` is not yet known, check `session->init != 0` instead (any active operation). This is appropriate given the single-field implementation:
	// After basic validation (session, mechanism, key):
	if (session->init != 0) {
	rv = CKR_OPERATION_ACTIVE;
	WOLFPKCS11_LEAVE("C_EncryptInit", rv);
	return rv;
	}
	// ... then mechanism switch
	Alternatively, add `WP11_Session_IsOpInitializedExact(session, init)` check at the top using a helper function, but this requires knowing `init` before the switch which isn't possible with the current structure.

[dgarske]

🟡 [Medium] IsOpInitializedExact does not catch cross-mechanism re-initialization
💡 SUGGEST bug

The new WP11_Session_IsOpInitializedExact performs session->init == init, which only catches re-init with the exact same mechanism. If C_EncryptInit(AES-CBC) is called (setting session->init = WP11_INIT_AES_CBC_ENC = 0x0001) and then C_EncryptInit(AES-GCM) is called, the check compares 0x0001 == WP11_INIT_AES_GCM_ENC (0x...different value) which is false. The second init silently overwrites the AES-CBC state.

Per PKCS#11: "If C_EncryptInit is called when the Encrypt operation is already active, CKR_OPERATION_ACTIVE is returned" — this applies regardless of the mechanism used for the first vs second init.

This is related to the BLOCK finding above. If the check is changed to session->init != 0, this issue is also resolved.

Suggestion:

Suggested change

	int WP11_Session_IsOpInitializedExact(WP11_Session* session, int init)
	int WP11_Session_IsOpInitializedExact(WP11_Session* session, int init)
	{
	(void)init;
	return session->init != 0;
	}

[dgarske]

🟡 [Medium] C_Digest and C_DigestFinal only clear operation state on success
💡 SUGGEST bug

The PR adds if (pDigest != NULL && ret == CKR_OK) WP11_Session_SetOpInitialized(session, 0); in C_Digest and a similar pattern in C_DigestFinal. Per PKCS#11 spec §5.10: "A call to C_Digest always terminates the active digest operation unless it returns CKR_BUFFER_TOO_SMALL or is a successful call (i.e., one which returns CKR_OK) to determine the length of the buffer needed to hold the message digest."

This means that on failure (other than BUFFER_TOO_SMALL or size query), the operation should also be terminated. Currently, a failed C_Digest with pDigest != NULL and ret != CKR_OK leaves the operation active, preventing re-initialization.

Suggestion:

Suggested change

	if (pDigest != NULL && ret == CKR_OK)
	if (pDigest != NULL)
	WP11_Session_SetOpInitialized(session, 0);
	return ret;

[dgarske]

🟡 [Medium] Tests do not verify first operation remains usable after CKR_OPERATION_ACTIVE
💡 SUGGEST test

The tests verify that the second C_*Init call returns CKR_OPERATION_ACTIVE, which is good. However, they don't verify that the FIRST operation remains functional after the rejected second init. Given the BLOCK finding above (params get overwritten before the check), a test like:

1. C_EncryptInit(AES-CBC, iv1) → CKR_OK
2. C_EncryptInit(AES-CBC, iv2) → CKR_OPERATION_ACTIVE
3. C_Encrypt(data) → should succeed and use iv1, not iv2

would catch the state corruption bug. Additionally, cross-mechanism double-init tests (e.g., C_EncryptInit(AES-CBC) then C_EncryptInit(AES-GCM)) are missing.

Recommendation: Add tests that: (1) verify the first operation can still complete successfully after a rejected second init, and (2) test cross-mechanism double init (e.g., AES-CBC then AES-ECB).

[dgarske]

🔵 [Low] Cosmetic blank line removals reduce readability
🔧 NIT style

The PR removes blank lines between parameter validation and WP11_Object_Find calls in EncryptInit (line 1857), DecryptInit (line 2860), C_SignInit (line 4238), and C_VerifyInit (line 5344). These blank lines provided visual separation between validation blocks and operational logic. The removals appear unintentional (not mentioned in the PR description) and slightly reduce readability.

Recommendation: Restore the blank lines for consistency with the rest of the codebase, or leave as-is if this is a deliberate style change.