Fix Fenrir PKCS#11 compliance findings#186
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a focused compliance regression test suite for Fenrir Tier-1 PKCS#11 findings and updates wolfPKCS11’s PKCS#11 entrypoints/internal helpers to match expected spec behavior (return codes, argument validation, and session gating).
Changes:
- Added
tests/pkcs11_compliance_test.cto cover ten spec-compliance findings and wired it into the Automake test build. - Updated PKCS#11 API behavior for slot listing, init/finalize validation, non-blocking slot events, unimplemented “dual function” stubs, and R/W session enforcement when creating token objects.
- Hardened GCM/CCM parameter handling to avoid NULL-deref on
(pIv == NULL, ulIvLen > 0)and skip IV copies whenivSz == 0.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
wolfpkcs11/pkcs11.h |
Adds CKF_DONT_BLOCK for C_WaitForSlotEvent flags. |
tests/pkcs11test.c |
Updates expected error codes for dual-function stubs to CKR_FUNCTION_NOT_SUPPORTED. |
tests/pkcs11mtt.c |
Same dual-function stub expectation updates as pkcs11test.c. |
tests/pkcs11_compliance_test.c |
New regression tests for the ten Fenrir compliance findings. |
tests/include.am |
Adds pkcs11_compliance_test to Automake’s test programs and links it. |
src/wolfpkcs11.c |
Adds C_Initialize mutex-callback “all-or-nothing” validation and tightens C_Finalize return codes/args validation. |
src/slot.c |
Implements non-blocking C_WaitForSlotEvent behavior (CKR_NO_EVENT) plus flag/pReserved validation. |
src/internal.c |
Fixes slot-list *count reporting on buffer-too-small and hardens GCM/CCM param copying against NULL IVs. |
src/crypto.c |
Updates dual-function stubs return codes and enforces R/W session for token-object key generation/derivation. |
Comments suppressed due to low confidence (2)
src/internal.c:8362
- WP11_Session_SetGcmParams now guards ivSz < 0, but aadLen and tagBits are still unchecked signed ints even though callers pass them via casts from CK_ULONG (e.g., (int)params->ulAADLen / ulTagBits). Large CK_ULONG values can overflow to negative and then be converted to huge size_t in XMALLOC/XMEMCPY, risking excessive allocation attempts or undefined behavior. Add explicit bounds checks for aadLen < 0, tagBits < 0 (and ideally enforce tagBits <= 128) and validate (aad == NULL) implies aadLen == 0 before allocating/copying.
if (tagBits > 128 || ivSz < 0 || ivSz > WP11_MAX_GCM_NONCE_SZ)
ret = BAD_FUNC_ARG;
/* Caller-supplied params must agree: a NULL IV pointer cannot have a
* positive length, and a positive length must come with a buffer. */
if (ret == 0 && (iv == NULL) != (ivSz == 0))
ret = BAD_FUNC_ARG;
if (ret == 0) {
if (session->mechanism == CKM_AES_GCM && gcm->aad != NULL) {
XFREE(gcm->aad, NULL, DYNAMIC_TYPE_TMP_BUFFER);
}
XMEMSET(gcm, 0, sizeof(*gcm));
if (ivSz > 0)
XMEMCPY(gcm->iv, iv, ivSz);
gcm->ivSz = ivSz;
gcm->tagBits = tagBits;
if (aad != NULL) {
gcm->aad = (unsigned char*)XMALLOC(aadLen, NULL,
DYNAMIC_TYPE_TMP_BUFFER);
if (gcm->aad == NULL)
ret = MEMORY_E;
if (ret == 0) {
XMEMCPY(gcm->aad, aad, aadLen);
gcm->aadSz = aadLen;
}
src/internal.c:8418
- WP11_Session_SetCcmParams has the same signed-length issue as the GCM variant: dataSz/aadSz/macSz are ints derived from CK_ULONG casts at the PKCS#11 boundary, but there are no checks for negative values before using aadSz in XMALLOC/XMEMCPY. Add bounds checks for dataSz < 0, aadSz < 0, macSz < 0 and validate (aad == NULL) implies aadSz == 0 to avoid overflow-to-huge allocations.
if (ivSz < 0 || ivSz > WP11_MAX_GCM_NONCE_SZ)
ret = BAD_FUNC_ARG;
/* Caller-supplied params must agree: a NULL IV pointer cannot have a
* positive length, and a positive length must come with a buffer. */
if (ret == 0 && (iv == NULL) != (ivSz == 0))
ret = BAD_FUNC_ARG;
if (ret == 0) {
if (session->mechanism == CKM_AES_CCM && ccm->aad != NULL) {
XFREE(ccm->aad, NULL, DYNAMIC_TYPE_TMP_BUFFER);
}
XMEMSET(ccm, 0, sizeof(*ccm));
ccm->dataSz = dataSz;
if (ivSz > 0)
XMEMCPY(ccm->iv, iv, ivSz);
ccm->ivSz = ivSz;
if (aad != NULL) {
ccm->aad = (unsigned char*)XMALLOC(aadSz, NULL,
DYNAMIC_TYPE_TMP_BUFFER);
if (ccm->aad == NULL) {
ret = MEMORY_E;
}
if (ret == 0) {
XMEMCPY(ccm->aad, aad, aadSz);
ccm->aadSz = aadSz;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add tests/pkcs11_compliance_test.c covering ten spec-compliance findings, then fix each. - F-3399: C_GetSlotList sets *count to slotCnt on CKR_BUFFER_TOO_SMALL so callers can resize and retry. - F-1337: C_Finalize returns CKR_CRYPTOKI_NOT_INITIALIZED when the library has not been initialized. - F-1336: C_Finalize rejects non-NULL pReserved with CKR_ARGUMENTS_BAD. - F-3140: C_Initialize rejects CK_C_INITIALIZE_ARGS with a partial (not all-or-nothing) set of mutex callbacks. - F-1338: C_GenerateKey and C_GenerateKeyPair enforce R/W session when CKA_TOKEN=TRUE. Test gated #ifndef WOLFPKCS11_NSS because NSS builds treat every session as R/W. - F-1342: C_DeriveKey enforces R/W session when CKA_TOKEN=TRUE. Same NSS gating. - F-1339: C_DigestEncryptUpdate, C_DecryptDigestUpdate, C_SignEncryptUpdate, C_DecryptVerifyUpdate now return CKR_FUNCTION_NOT_SUPPORTED instead of CKR_OPERATION_NOT_INITIALIZED. Existing test_encdec_digest/signverify in pkcs11test.c and pkcs11mtt.c updated to match. - F-1340: C_WaitForSlotEvent returns CKR_NO_EVENT when CKF_DONT_BLOCK is set; blocking calls still return CKR_FUNCTION_NOT_SUPPORTED. CKF_DONT_BLOCK macro added to wolfpkcs11/pkcs11.h. - F-3634: WP11_Session_SetGcmParams rejects mismatched (iv==NULL)/(ivSz>0) with BAD_FUNC_ARG and skips XMEMCPY when ivSz==0, fixing a NULL-deref crash from C_EncryptInit/C_DecryptInit. - F-3635: WP11_Session_SetCcmParams gets the same guard.
dgarske
approved these changes
May 12, 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.
Add tests/pkcs11_compliance_test.c covering ten spec-compliance findings, then fix each.