Add support for CKR_OPERATION_ACTIVE#176
Conversation
There was a problem hiding this comment.
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 integrateCKR_OPERATION_ACTIVEchecks 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_testand 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
left a comment
There was a problem hiding this comment.
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.
02deee8 to
038c8e6
Compare
038c8e6 to
48f677a
Compare
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
Stops overwriting previous state. F-1616
48f677a to
94ceadf
Compare
Stops overwriting previous state.
F-1616