Skip to content

Fix Fenrir PKCS#11 compliance findings#186

Merged
dgarske merged 1 commit into
wolfSSL:masterfrom
LinuxJedi:f-fixes9
May 12, 2026
Merged

Fix Fenrir PKCS#11 compliance findings#186
dgarske merged 1 commit into
wolfSSL:masterfrom
LinuxJedi:f-fixes9

Conversation

@LinuxJedi
Copy link
Copy Markdown
Member

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.

Copilot AI review requested due to automatic review settings May 12, 2026 15:12
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.c to 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 when ivSz == 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.

Comment thread src/internal.c Outdated
Comment thread src/wolfpkcs11.c
Comment thread src/slot.c Outdated
Comment thread tests/include.am
Comment thread tests/pkcs11_compliance_test.c Outdated
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 dgarske merged commit 4bac443 into wolfSSL:master May 12, 2026
86 of 87 checks passed
@LinuxJedi LinuxJedi deleted the f-fixes9 branch May 12, 2026 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants