Skip to content

Commit 4bac443

Browse files
dgarskedgarske
authored
Merge pull request #186 from LinuxJedi/f-fixes9
Fix Fenrir PKCS#11 compliance findings
2 parents 9620c60 + 1e233c5 commit 4bac443

10 files changed

Lines changed: 919 additions & 32 deletions

File tree

‎CMakeLists.txt‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -721,7 +721,8 @@ if(WOLFPKCS11_TESTS)
721721
token_path_test
722722
rsa_session_persistence_test
723723
debug_test
724-
object_id_uniqueness_test)
724+
object_id_uniqueness_test
725+
pkcs11_compliance_test)
725726

726727
foreach(test_target IN LISTS WPKCS11_TEST_TARGETS)
727728
add_wpkcs11_test(${test_target}

‎src/crypto.c‎

Lines changed: 66 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6644,7 +6644,7 @@ CK_RV C_DigestEncryptUpdate(CK_SESSION_HANDLE hSession,
66446644

66456645
(void)pEncryptedPart;
66466646

6647-
rv = CKR_OPERATION_NOT_INITIALIZED;
6647+
rv = CKR_FUNCTION_NOT_SUPPORTED;
66486648
WOLFPKCS11_LEAVE("C_DigestEncryptUpdate", rv);
66496649
return rv;
66506650
}
@@ -6700,7 +6700,7 @@ CK_RV C_DecryptDigestUpdate(CK_SESSION_HANDLE hSession,
67006700

67016701
(void)pPart;
67026702

6703-
rv = CKR_OPERATION_NOT_INITIALIZED;
6703+
rv = CKR_FUNCTION_NOT_SUPPORTED;
67046704
WOLFPKCS11_LEAVE("C_DecryptDigestUpdate", rv);
67056705
return rv;
67066706
}
@@ -6756,7 +6756,7 @@ CK_RV C_SignEncryptUpdate(CK_SESSION_HANDLE hSession,
67566756

67576757
(void)pEncryptedPart;
67586758

6759-
rv = CKR_OPERATION_NOT_INITIALIZED;
6759+
rv = CKR_FUNCTION_NOT_SUPPORTED;
67606760
WOLFPKCS11_LEAVE("C_SignEncryptUpdate", rv);
67616761
return rv;
67626762
}
@@ -6812,7 +6812,7 @@ CK_RV C_DecryptVerifyUpdate(CK_SESSION_HANDLE hSession,
68126812

68136813
(void)pPart;
68146814

6815-
rv = CKR_OPERATION_NOT_INITIALIZED;
6815+
rv = CKR_FUNCTION_NOT_SUPPORTED;
68166816
WOLFPKCS11_LEAVE("C_DecryptVerifyUpdate", rv);
68176817
return rv;
68186818
}
@@ -6870,6 +6870,24 @@ CK_RV C_GenerateKey(CK_SESSION_HANDLE hSession,
68706870
WOLFPKCS11_LEAVE("C_GenerateKey", rv);
68716871
return rv;
68726872
}
6873+
/* Only require R/W session for token objects */
6874+
if (!WP11_Session_IsRW(session)) {
6875+
CK_ATTRIBUTE* tokenAttr = NULL;
6876+
FindAttributeType(pTemplate, ulCount, CKA_TOKEN, &tokenAttr);
6877+
if (tokenAttr != NULL) {
6878+
if (tokenAttr->pValue == NULL ||
6879+
tokenAttr->ulValueLen != sizeof(CK_BBOOL)) {
6880+
rv = CKR_ATTRIBUTE_VALUE_INVALID;
6881+
WOLFPKCS11_LEAVE("C_GenerateKey", rv);
6882+
return rv;
6883+
}
6884+
if (*(CK_BBOOL*)tokenAttr->pValue == CK_TRUE) {
6885+
rv = CKR_SESSION_READ_ONLY;
6886+
WOLFPKCS11_LEAVE("C_GenerateKey", rv);
6887+
return rv;
6888+
}
6889+
}
6890+
}
68736891

68746892
switch (pMechanism->mechanism) {
68756893
#ifndef NO_AES
@@ -7287,6 +7305,32 @@ CK_RV C_GenerateKeyPair(CK_SESSION_HANDLE hSession,
72877305
WOLFPKCS11_LEAVE("C_GenerateKeyPair", rv);
72887306
return rv;
72897307
}
7308+
/* Only require R/W session for token objects. Each template must be
7309+
* inspected independently — a public template with CKA_TOKEN=FALSE must
7310+
* not mask a private template requesting CKA_TOKEN=TRUE. */
7311+
if (!WP11_Session_IsRW(session)) {
7312+
CK_ATTRIBUTE_PTR tpls[2] = { pPublicKeyTemplate, pPrivateKeyTemplate };
7313+
CK_ULONG counts[2] = { ulPublicKeyAttributeCount,
7314+
ulPrivateKeyAttributeCount };
7315+
int i;
7316+
for (i = 0; i < 2; i++) {
7317+
CK_ATTRIBUTE* tokenAttr = NULL;
7318+
FindAttributeType(tpls[i], counts[i], CKA_TOKEN, &tokenAttr);
7319+
if (tokenAttr == NULL)
7320+
continue;
7321+
if (tokenAttr->pValue == NULL ||
7322+
tokenAttr->ulValueLen != sizeof(CK_BBOOL)) {
7323+
rv = CKR_ATTRIBUTE_VALUE_INVALID;
7324+
WOLFPKCS11_LEAVE("C_GenerateKeyPair", rv);
7325+
return rv;
7326+
}
7327+
if (*(CK_BBOOL*)tokenAttr->pValue == CK_TRUE) {
7328+
rv = CKR_SESSION_READ_ONLY;
7329+
WOLFPKCS11_LEAVE("C_GenerateKeyPair", rv);
7330+
return rv;
7331+
}
7332+
}
7333+
}
72907334

72917335
switch (pMechanism->mechanism) {
72927336
#if !defined(NO_RSA) && defined(WOLFSSL_KEY_GEN)
@@ -8308,6 +8352,24 @@ CK_RV C_DeriveKey(CK_SESSION_HANDLE hSession,
83088352
WOLFPKCS11_LEAVE("C_DeriveKey", rv);
83098353
return rv;
83108354
}
8355+
/* Only require R/W session for token objects */
8356+
if (!WP11_Session_IsRW(session)) {
8357+
CK_ATTRIBUTE* tokenAttr = NULL;
8358+
FindAttributeType(pTemplate, ulAttributeCount, CKA_TOKEN, &tokenAttr);
8359+
if (tokenAttr != NULL) {
8360+
if (tokenAttr->pValue == NULL ||
8361+
tokenAttr->ulValueLen != sizeof(CK_BBOOL)) {
8362+
rv = CKR_ATTRIBUTE_VALUE_INVALID;
8363+
WOLFPKCS11_LEAVE("C_DeriveKey", rv);
8364+
return rv;
8365+
}
8366+
if (*(CK_BBOOL*)tokenAttr->pValue == CK_TRUE) {
8367+
rv = CKR_SESSION_READ_ONLY;
8368+
WOLFPKCS11_LEAVE("C_DeriveKey", rv);
8369+
return rv;
8370+
}
8371+
}
8372+
}
83118373

83128374
ret = WP11_Object_Find(session, hBaseKey, &obj);
83138375
if (ret != 0)

‎src/internal.c‎

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6728,8 +6728,13 @@ int WP11_GetSlotList(int tokenIn, CK_SLOT_ID* slotIdList, CK_ULONG* count)
67286728

67296729
if (slotIdList == NULL)
67306730
*count = slotCnt;
6731-
else if ((int)*count < slotCnt)
6731+
else if ((int)*count < slotCnt) {
6732+
/* PKCS#11 spec: report the required size so the caller can resize
6733+
* and retry. Without this, callers using the standard two-call size
6734+
* query pattern stall at BUFFER_TOO_SMALL. */
6735+
*count = slotCnt;
67326736
ret = BUFFER_E;
6737+
}
67336738
else {
67346739
for (i = 0; i < slotCnt && i < (int)*count; i++)
67356740
slotIdList[i] = i + 1;
@@ -8330,15 +8335,21 @@ int WP11_Session_SetGcmParams(WP11_Session* session, unsigned char* iv,
83308335
int ret = 0;
83318336
WP11_GcmParams* gcm = &session->params.gcm;
83328337

8333-
if (tagBits > 128 || ivSz > WP11_MAX_GCM_NONCE_SZ)
8338+
if (tagBits > 128 || ivSz < 0 || ivSz > WP11_MAX_GCM_NONCE_SZ)
8339+
ret = BAD_FUNC_ARG;
8340+
/* Caller-supplied IV pointer and length must agree: NULL pairs with 0
8341+
* and only with 0, and a non-NULL buffer must come with a positive
8342+
* length. Either mismatch is a contract bug. */
8343+
if (ret == 0 && (iv == NULL) != (ivSz == 0))
83348344
ret = BAD_FUNC_ARG;
83358345

83368346
if (ret == 0) {
83378347
if (session->mechanism == CKM_AES_GCM && gcm->aad != NULL) {
83388348
XFREE(gcm->aad, NULL, DYNAMIC_TYPE_TMP_BUFFER);
83398349
}
83408350
XMEMSET(gcm, 0, sizeof(*gcm));
8341-
XMEMCPY(gcm->iv, iv, ivSz);
8351+
if (ivSz > 0)
8352+
XMEMCPY(gcm->iv, iv, ivSz);
83428353
gcm->ivSz = ivSz;
83438354
gcm->tagBits = tagBits;
83448355
if (aad != NULL) {
@@ -8380,7 +8391,12 @@ int WP11_Session_SetCcmParams(WP11_Session* session, int dataSz,
83808391
int ret = 0;
83818392
WP11_CcmParams* ccm = &session->params.ccm;
83828393

8383-
if (ivSz > WP11_MAX_GCM_NONCE_SZ)
8394+
if (ivSz < 0 || ivSz > WP11_MAX_GCM_NONCE_SZ)
8395+
ret = BAD_FUNC_ARG;
8396+
/* Caller-supplied IV pointer and length must agree: NULL pairs with 0
8397+
* and only with 0, and a non-NULL buffer must come with a positive
8398+
* length. Either mismatch is a contract bug. */
8399+
if (ret == 0 && (iv == NULL) != (ivSz == 0))
83848400
ret = BAD_FUNC_ARG;
83858401

83868402
if (ret == 0) {
@@ -8389,7 +8405,8 @@ int WP11_Session_SetCcmParams(WP11_Session* session, int dataSz,
83898405
}
83908406
XMEMSET(ccm, 0, sizeof(*ccm));
83918407
ccm->dataSz = dataSz;
8392-
XMEMCPY(ccm->iv, iv, ivSz);
8408+
if (ivSz > 0)
8409+
XMEMCPY(ccm->iv, iv, ivSz);
83938410
ccm->ivSz = ivSz;
83948411
if (aad != NULL) {
83958412
ccm->aad = (unsigned char*)XMALLOC(aadSz, NULL,

‎src/slot.c‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2062,12 +2062,23 @@ CK_RV C_WaitForSlotEvent(CK_FLAGS flags, CK_SLOT_ID_PTR pSlot,
20622062
WOLFPKCS11_LEAVE("C_WaitForSlotEvent", rv);
20632063
return rv;
20642064
}
2065+
/* PKCS#11: pSlot is an output parameter, pReserved must be NULL, and
2066+
* only the CKF_DONT_BLOCK bit is defined for flags. Reject malformed
2067+
* callers before returning the supported non-blocking result. */
2068+
if (pSlot == NULL || pReserved != NULL ||
2069+
(flags & ~CKF_DONT_BLOCK) != 0) {
2070+
rv = CKR_ARGUMENTS_BAD;
2071+
WOLFPKCS11_LEAVE("C_WaitForSlotEvent", rv);
2072+
return rv;
2073+
}
20652074

2066-
(void)pSlot;
2067-
(void)flags;
2068-
(void)pReserved;
2069-
2070-
rv = CKR_FUNCTION_NOT_SUPPORTED;
2075+
/* wolfPKCS11 has no removable slots, so no slot event ever occurs. For a
2076+
* non-blocking query the spec answer is CKR_NO_EVENT; blocking calls
2077+
* would deadlock forever, so report unsupported. */
2078+
if ((flags & CKF_DONT_BLOCK) != 0)
2079+
rv = CKR_NO_EVENT;
2080+
else
2081+
rv = CKR_FUNCTION_NOT_SUPPORTED;
20712082
WOLFPKCS11_LEAVE("C_WaitForSlotEvent", rv);
20722083
return rv;
20732084
}

‎src/wolfpkcs11.c‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,17 @@
2626
#include <wolfpkcs11/pkcs11.h>
2727
#include <wolfpkcs11/internal.h>
2828

29+
/* Windows headers macro-rewrite CreateMutex (and friends) to CreateMutexA/W
30+
* when UNICODE is defined, which collides with the CK_C_INITIALIZE_ARGS
31+
* struct member names. Undef the four Win32 macros so direct field access
32+
* compiles. */
33+
#ifdef _WIN32
34+
#undef CreateMutex
35+
#undef DestroyMutex
36+
#undef LockMutex
37+
#undef UnlockMutex
38+
#endif
39+
2940
/* Function list table for PKCS#11 v2.40 */
3041
static CK_FUNCTION_LIST wolfpkcs11FunctionList = {
3142
/* Version: Major, Minor */
@@ -509,6 +520,19 @@ CK_RV C_Initialize(CK_VOID_PTR pInitArgs)
509520
WOLFPKCS11_ENTER("C_Initialize");
510521

511522
if (args != NULL) {
523+
/* PKCS#11 spec: the four mutex callbacks must be all-set or all-NULL;
524+
* any partial combination is CKR_ARGUMENTS_BAD. pReserved must also
525+
* be NULL. */
526+
int callbacks_set =
527+
(args->CreateMutex != NULL) + (args->DestroyMutex != NULL) +
528+
(args->LockMutex != NULL) + (args->UnlockMutex != NULL);
529+
if ((callbacks_set != 0 && callbacks_set != 4) ||
530+
args->pReserved != NULL) {
531+
ret = CKR_ARGUMENTS_BAD;
532+
WOLFPKCS11_LEAVE("C_Initialize", ret);
533+
return ret;
534+
}
535+
512536
WOLFPKCS11_MSG("Warning: C_Initialize called with arguments, but most "
513537
"are ignored.");
514538
#if (defined(WOLFPKCS11_NSS) && !defined(WOLFPKCS11_NO_STORE))
@@ -556,9 +580,19 @@ CK_RV C_Finalize(CK_VOID_PTR pReserved)
556580
CK_RV ret;
557581
WOLFPKCS11_ENTER("C_Finalize");
558582

583+
if (!WP11_Library_IsInitialized()) {
584+
ret = CKR_CRYPTOKI_NOT_INITIALIZED;
585+
WOLFPKCS11_LEAVE("C_Finalize", ret);
586+
return ret;
587+
}
588+
if (pReserved != NULL) {
589+
ret = CKR_ARGUMENTS_BAD;
590+
WOLFPKCS11_LEAVE("C_Finalize", ret);
591+
return ret;
592+
}
593+
559594
WP11_Library_Final();
560595

561-
(void)pReserved;
562596
ret = CKR_OK;
563597
WOLFPKCS11_LEAVE("C_Finalize", ret);
564598
return ret;

‎tests/include.am‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,11 @@ noinst_PROGRAMS += tests/rsa_exponent_test
8686
tests_rsa_exponent_test_SOURCES = tests/rsa_exponent_test.c
8787
tests_rsa_exponent_test_LDADD =
8888

89+
check_PROGRAMS += tests/pkcs11_compliance_test
90+
noinst_PROGRAMS += tests/pkcs11_compliance_test
91+
tests_pkcs11_compliance_test_SOURCES = tests/pkcs11_compliance_test.c
92+
tests_pkcs11_compliance_test_LDADD =
93+
8994
if BUILD_STATIC
9095
tests_pkcs11test_LDADD += src/libwolfpkcs11.la
9196
tests_pkcs11mtt_LDADD += src/libwolfpkcs11.la
@@ -104,6 +109,7 @@ tests_aes_keygen_attrs_test_LDADD += src/libwolfpkcs11.la
104109
tests_pbkdf2_keygen_attrs_test_LDADD += src/libwolfpkcs11.la
105110
tests_pkcs11v3test_LDADD += src/libwolfpkcs11.la
106111
tests_rsa_exponent_test_LDADD += src/libwolfpkcs11.la
112+
tests_pkcs11_compliance_test_LDADD += src/libwolfpkcs11.la
107113
else
108114
tests_object_id_uniqueness_test_LDADD += src/libwolfpkcs11.la
109115
tests_empty_pin_store_test_LDADD += src/libwolfpkcs11.la
@@ -114,6 +120,7 @@ tests_ecb_check_value_error_test_LDADD += src/libwolfpkcs11.la
114120
tests_operation_active_test_LDADD += src/libwolfpkcs11.la
115121
tests_aes_keygen_attrs_test_LDADD += src/libwolfpkcs11.la
116122
tests_pbkdf2_keygen_attrs_test_LDADD += src/libwolfpkcs11.la
123+
tests_pkcs11_compliance_test_LDADD += src/libwolfpkcs11.la
117124
endif
118125

119126
EXTRA_DIST += tests/unit.h \

0 commit comments

Comments
 (0)