PKCS11 conformance and robustness fixes#194
Merged
Merged
Conversation
WP11_Object_Find returns BAD_FUNC_ARG when the key handle does not match any object. C_VerifyRecoverInit forwarded that raw negative wolfCrypt code directly, casting it to CK_RV and yielding an undefined return value. Return CKR_OBJECT_HANDLE_INVALID instead, matching the other C_*Init functions and the PKCS#11 specification. Add tests/verify_recover_badkey_test.c regression test.
C_Login discarded the checkPinLen result and substituted CKR_PIN_INCORRECT for an out-of-range PIN length. Propagate the checkPinLen return value so CKR_PIN_LEN_RANGE reaches the caller, matching C_InitToken, C_InitPIN and C_SetPIN. Add tests/login_pin_len_range_test.c regression test.
…erInit (F-4067) When C_VerifyRecoverInit resolved a handle to a valid object whose CKA_CLASS was not CKO_PUBLIC_KEY it returned CKR_KEY_HANDLE_INVALID, which the spec reserves for handles that do not refer to keys. Return CKR_KEY_TYPE_INCONSISTENT for a key of the wrong object class. Add tests/verify_recover_class_test.c regression test.
WP11_Slot_SOLogin and WP11_Slot_UserLogin returned LOGGED_IN_E for any already-logged-in state, so C_Login always reported CKR_USER_ALREADY_LOGGED_IN. Add LOGGED_IN_ANOTHER_E for the case where a different user type is logged in and map it to CKR_USER_ANOTHER_ALREADY_LOGGED_IN, keeping CKR_USER_ALREADY_LOGGED_IN for the same user type. Add tests/login_another_user_test.c regression test.
CheckOpSupported returned CKR_KEY_TYPE_INCONSISTENT when a key's CKA_<op> usage attribute was CK_FALSE. PKCS#11 specifies CKR_KEY_FUNCTION_NOT_PERMITTED for that case and reserves CKR_KEY_TYPE_INCONSISTENT for a key type that does not match the mechanism (handled by the per-mechanism checks). This affects C_EncryptInit, C_DecryptInit, C_SignInit, C_VerifyInit, C_VerifyRecoverInit, C_WrapKey, C_UnwrapKey and C_DeriveKey. Update the existing pkcs11test.c usage-denied assertions, which encoded the old code, and add tests/key_function_not_permitted_test.c.
The CKM_AES_CBC_PAD branch of C_DecryptFinal passed WP11_AesCbcPad_DecryptFinal the buffered block length (always 16) as the output capacity instead of the caller's *pulLastPartLen, so the helper's BUFFER_E guard could never fire and an undersized caller buffer was overflowed by up to 15 bytes. Pass the caller's actual capacity, like the single-shot C_Decrypt path, so a too-small buffer yields CKR_BUFFER_TOO_SMALL. The size query now reports AES_BLOCK_SIZE - 1. Add tests/decrypt_final_bufsize_test.c regression test.
CreateObject initialised objectClass to the (CK_OBJECT_CLASS)-1 sentinel and only updated it when CKA_CLASS was present. A key template that omitted CKA_CLASS fell into the key-object path and created an object whose CKA_CLASS was the invalid sentinel value. Reject a C_CreateObject template without CKA_CLASS with CKR_TEMPLATE_INCOMPLETE. Existing negative tests that omitted CKA_CLASS relied on falling through to the key/token attribute checks; give them a valid CKA_CLASS so they still exercise those checks. Add tests/create_object_class_test.c regression test.
WP11_Session_SetGcmParams and WP11_Session_SetCcmParams validated the IV pointer/length agreement but not the AAD. With aad == NULL and aadLen > 0 they returned success and stored a zero-length AAD, so C_EncryptInit / C_DecryptInit accepted malformed parameters. Apply the same pointer/length agreement check to the AAD, which the callers map to CKR_MECHANISM_PARAM_INVALID. Add tests/aead_null_aad_test.c regression test.
C_GenerateKeyPair built the key objects and invoked the mechanism generator without first checking the mechanism's required public-key template attributes. A missing CKA_MODULUS_BITS surfaced as CKR_FUNCTION_FAILED, and a missing CKA_EC_PARAMS dereferenced a NULL domain-parameter pointer. Require CKA_MODULUS_BITS for RSA, CKA_EC_PARAMS for EC and CKA_PRIME/CKA_BASE for DH, returning CKR_TEMPLATE_INCOMPLETE. Add tests/gen_keypair_incomplete_test.c regression test.
SetAttributeValue forwarded every template attribute to WP11_Object_SetAttr, which accepted updates to attributes PKCS#11 marks read-only once the object exists: CKA_CLASS, CKA_KEY_TYPE, CKA_LOCAL, CKA_KEY_GEN_MECHANISM, CKA_ALWAYS_SENSITIVE and CKA_NEVER_EXTRACTABLE. Reject an attempt to change any of them with CKR_ATTRIBUTE_READ_ONLY. A set to the current value remains a no-op so C_CopyObject, which funnels through the same helper, is unaffected. Add tests/set_attr_readonly_test.c regression test.
…-4065) C_DeriveKey passed the base key straight to the mechanism-specific derive routine without checking that its key type matched the mechanism, so a wrong base key type failed deep in the crypto (CKR_FUNCTION_FAILED, or a NULL dereference) instead of the spec-mandated CKR_KEY_TYPE_INCONSISTENT. Require CKK_EC for CKM_ECDH1_DERIVE, CKK_DH for CKM_DH_PKCS_DERIVE, CKK_AES for CKM_AES_CBC_ENCRYPT_DATA, CKK_HKDF/CKK_GENERIC_SECRET for the HKDF mechanisms and CKK_GENERIC_SECRET for the TLS 1.2 mechanisms. Add tests/derive_key_type_test.c regression test.
GetTrustAttr set *len to sizeof(CK_ULONG)/sizeof(CK_BBOOL) before calling GetULong/GetBool for the CKA_TRUST_* attributes, defeating those helpers' *len < dataLen buffer-too-small check and overflowing an undersized caller buffer. Let GetULong/GetBool handle the size query and the buffer-too-small case themselves, matching the canonical pattern. Add tests/trust_attr_bufsize_test.c regression test (NSS-only; verified against an --enable-nss build, skipped otherwise).
The F-* regression tests each duplicated the module load/unload, the funcList/dlib globals, the CHECK_RV/CHECK_TRUE macros and the pass/fail summary. Move them to tests/pkcs11_test_util.h along with a pkcs11_open_session() helper for the common C_Initialize + slot + R/W session setup, and use them from the tests.
Drop comments that narrated the previous (buggy) behaviour or merely restated the code, keeping only the high-level/spec rationale. Hoist the single-use block-scoped declarations added by the fixes (classAttr in C_CreateObject, reqAttr in C_GenerateKeyPair, the read-only-attribute scratch buffer in SetAttributeValue) to the start of their functions.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR delivers a set of PKCS#11 conformance/robustness fixes across login handling, key usage gating, object template validation, AEAD parameter validation, derive-key key-type checks, and output buffer enforcement, plus adds targeted regression tests for each tracked F-* item.
Changes:
- Tighten PKCS#11 return codes and validation logic for login/user-state, object templates, key usage restrictions, derive-key base key types, AEAD AAD validation, and buffer sizing.
- Add standalone regression tests (with shared helper header) covering the newly enforced behaviors and corrected return codes.
- Update existing test expectations to match the corrected error codes (notably
CKR_KEY_FUNCTION_NOT_PERMITTEDvsCKR_KEY_TYPE_INCONSISTENT).
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfpkcs11/internal.h | Adds an internal error code to distinguish “another user already logged in”. |
| src/slot.c | Propagates PIN length range errors from checkPinLen(); maps new “another user” login error to PKCS#11 RV. |
| src/internal.c | Implements “same user type” vs “different user type” login distinction; validates AEAD AAD pointer/length agreement; fixes NSS trust attribute sizing path. |
| src/crypto.c | Adjusts op-support gating to return CKR_KEY_FUNCTION_NOT_PERMITTED; enforces read-only attributes in C_SetAttributeValue; requires CKA_CLASS in C_CreateObject; enforces CBC-PAD C_DecryptFinal buffer sizing; corrects C_VerifyRecoverInit handle/class RVs; validates required keygen attrs; adds derive-key base key type checks. |
| tests/include.am | Adds new standalone regression tests to the automake test programs list and dist. |
| tests/pkcs11_test_util.h | New shared helper header for standalone regression tests (loading module, opening session, summary, etc.). |
| tests/pkcs11test.c | Updates object-creation negative tests to include CKA_CLASS; updates expected RVs for usage-denied keys. |
| tests/pkcs11mtt.c | Mirrors pkcs11test.c test updates for multi-thread test harness. |
| tests/verify_recover_badkey_test.c | New test for C_VerifyRecoverInit invalid-handle RV. |
| tests/verify_recover_class_test.c | New test for C_VerifyRecoverInit wrong-class RV (CKR_KEY_TYPE_INCONSISTENT). |
| tests/login_pin_len_range_test.c | New test ensuring C_Login returns CKR_PIN_LEN_RANGE for over-length PIN. |
| tests/login_another_user_test.c | New test ensuring C_Login distinguishes same-user vs other-user already logged in. |
| tests/key_function_not_permitted_test.c | New test asserting usage-denied operations return CKR_KEY_FUNCTION_NOT_PERMITTED. |
| tests/decrypt_final_bufsize_test.c | New test verifying CBC-PAD C_DecryptFinal enforces output buffer size. |
| tests/create_object_class_test.c | New test enforcing CKA_CLASS required in C_CreateObject templates. |
| tests/aead_null_aad_test.c | New test rejecting NULL AAD pointer with non-zero length for GCM/CCM. |
| tests/gen_keypair_incomplete_test.c | New test ensuring C_GenerateKeyPair returns CKR_TEMPLATE_INCOMPLETE when required public attrs are missing. |
| tests/set_attr_readonly_test.c | New test ensuring read-only attributes cannot be changed via C_SetAttributeValue (except no-op same-value). |
| tests/derive_key_type_test.c | New test ensuring derive mechanisms reject wrong-typed base keys with CKR_KEY_TYPE_INCONSISTENT. |
| tests/trust_attr_bufsize_test.c | New NSS-only test ensuring undersized buffers yield CKR_BUFFER_TOO_SMALL without overwrite for trust attrs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
C_DeriveKey applied the HKDF base key-type check to CKM_HKDF_DATA, which derives from a CKO_DATA object rather than a key, so NSS's TLS 1.3 key schedule failed with CKR_KEY_TYPE_INCONSISTENT (test-nss-curl: 'the key does not support the requested operation'). Restrict the key-type check to CKM_HKDF_DERIVE. Add an NSS-only regression test deriving via CKM_HKDF_DATA from a CKO_DATA object. aead_null_aad_test asserted CKR_MECHANISM_PARAM_INVALID for AES-GCM, but the --disable-aesgcm build (no_aesgcm CI job) returns CKR_MECHANISM_INVALID; skip the GCM checks when the mechanism is unavailable, as already done for CCM.
- keyTypeBadValue used sizeof(&badKeyType) (pointer size) for the CKA_KEY_TYPE attribute length, wrong on LLP64; use sizeof(badKeyType). - trust_attr_bufsize_test: size the canary buffer larger than a CK_ULONG and claim sizeof(CK_ULONG)-1 so the undersized-buffer check is genuinely undersized and the canary region exists on 32- and 64-bit builds.
julek-wolfssl
changed the title
NSS drives the TLS 1.3 key schedule with a data-based HKDF derive on a CKO_DATA base object. NSS sends the PKCS#11 standard mechanism value 0x402A, which wolfPKCS11's headers name CKM_HKDF_DERIVE, so that call reaches the key-based derive case. The previous guard keyed off the mechanism value and wrongly rejected the CKO_DATA base object with CKR_KEY_TYPE_INCONSISTENT, breaking the NSS handshake (test-nss-curl, test-firefox). Gate the base key-type check on the object class instead: a CKO_DATA object has no key type and is always a valid HKDF data input; only key base objects are checked for an HKDF-compatible type. The regression test now uses the mechanism value NSS actually sends so it exercises the failing path.
… docs - AES-GCM/CCM SetParams: only reject a NULL AAD pointer paired with a non-zero length. A non-NULL pointer with zero length is a valid no-AAD encoding; rejecting it broke callers that pass an unused buffer with a zero length. Guard the AAD copy on a positive length so the no-AAD case never allocates. Regression test now asserts non-NULL + zero length is accepted. - C_Login doc comment: reflect CKR_PIN_LEN_RANGE for out-of-range PIN length (now propagated from checkPinLen) and document the CKR_USER_ANOTHER_ALREADY_LOGGED_IN path.
… header - GetTrustAttr: correct the comment over the CKA_TRUST_* cases. GetULong/ GetBool set *len on a size query or success and return BUFFER_E without touching *len when the buffer is too small; the old wording implied they also reported the size on the too-small path. - pkcs11_test_util.h: include <stdio.h> so the helper header is self-contained and not dependent on include order.
julek-wolfssl
changed the title
LinuxJedi
approved these changes
Jun 19, 2026
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 contains a series of PKCS#11 conformance and robustness fixes (tracked as F-* items):
C_VerifyRecoverInitinvalid-handle return codeCKR_PIN_LEN_RANGEfromC_LoginCKR_KEY_TYPE_INCONSISTENTfor wrong-class key inC_VerifyRecoverInitCKR_USER_ANOTHER_ALREADY_LOGGED_INinC_LoginCKR_KEY_FUNCTION_NOT_PERMITTEDfor usage-denied keysC_DecryptFinalCBC-PAD branchCKA_CLASSinC_CreateObjecttemplatesC_GenerateKeyPairC_SetAttributeValueC_DeriveKeyAlso factors shared test boilerplate into
pkcs11_test_util.hand tidies the F-* fix comments.