Skip to content

Commit 982f118

Browse files
committed
Fix Fenrir static analysis findings
Address Fenrir findings covering missing object attribute enforcement, missing operation termination on error paths, and an authentication fail-open on the SO PIN check. Attribute enforcement: - C_CopyObject rejects non-copyable objects (CKR_ACTION_PROHIBITED). - C_DestroyObject rejects non-destroyable objects (CKR_ACTION_PROHIBITED). - C_DeriveKey enforces CKA_DERIVE on the base key. - C_EncapsulateKey/C_DecapsulateKey enforce CKA_ENCAPSULATE/DECAPSULATE and return CKR_KEY_FUNCTION_NOT_PERMITTED. - C_SetAttributeValue rejects flipping CKA_COPYABLE/CKA_DESTROYABLE from CK_FALSE back to CK_TRUE per PKCS#11 v2.40 sec. 4.4.1. - CKA_COPYABLE/CKA_DESTROYABLE are now stored via inverted opFlag bits (WP11_FLAG_NOT_COPYABLE/NOT_DESTROYABLE) so the values survive C_GetAttributeValue/C_SetAttributeValue round trips. CKA_COPYABLE default flips from CK_FALSE to CK_TRUE to match the PKCS#11 spec. The legacy read-back behavior is preserved behind WOLFPKCS11_LEGACY_COPYABLE_FALSE_DEFAULT. The C_CopyObject gate reads the stored flag bit directly via WP11_Object_IsCopyable so the legacy macro never disables copy for objects that explicitly opted in. Operation termination: C_Encrypt, C_Decrypt, C_EncryptUpdate, C_DecryptUpdate, C_DigestUpdate, C_SignUpdate, C_VerifyUpdate, and C_DigestKey now call WP11_Session_SetOpInitialized(session, 0) on their early-return error paths so a follow-up *Init succeeds. C_DigestKey preserves positive CK_RV returns (e.g. CKR_FUNCTION_NOT_SUPPORTED on WOLFPKCS11_NO_STORE builds) instead of collapsing them to CKR_FUNCTION_FAILED. SO PIN check: WP11_Slot_CheckSOPin no longer accepts an empty PIN when the SO PIN has not been set. Previously the empty-PIN constant-compare against the zero-length unset PIN returned equal, granting WP11_APP_STATE_RW_SO without authentication. C_CopyObject and C_DestroyObject now run the COPYABLE/DESTROYABLE gates after the R/W session check so the previously-returned CKR_SESSION_READ_ONLY is preserved for read-only sessions. Tests: new test_copy_object_not_copyable, test_destroy_object_not_- destroyable, test_derive_key_not_allowed, test_mlkem_encap_decap_- not_permitted, test_op_active_after_data_len_range, test_op_active_- after_update_data_len_range, test_op_active_after_sign_verify_- update_failure, test_op_active_after_digest_key_failure, and WOLFPKCS11_NO_STORE-gated test_op_active_after_digest_key_no_store in pkcs11test/pkcs11v3test, plus a new tests/so_login_uninit_test binary that walks C_Login(CKU_SO, ...) on a fresh uninitialized token. test_copy_object_not_copyable and test_destroy_object_not_- destroyable also assert that the FALSE->TRUE flip is rejected. Several existing helpers (get_generic_key, get_ecc_priv_key, get_aes_128_key, gen_aes_key, the HKDF and ML-KEM templates) now set CKA_DERIVE=CK_TRUE explicitly so they survive the new gate. README.md documents the new WOLFPKCS11_LEGACY_COPYABLE_FALSE_DEFAULT build define and the spec-compliance behavior changes that callers upgrading from earlier versions need to be aware of.
1 parent 47e1453 commit 982f118

9 files changed

Lines changed: 1024 additions & 41 deletions

File tree

README.md

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,38 @@ See wolfpkcs11/store.h for prototypes of functions to implement.
9797

9898
Sets the private key's label against the public key when generating key pairs.
9999

100+
#### Define WOLFPKCS11_LEGACY_COPYABLE_FALSE_DEFAULT
101+
102+
Restores the pre-fix behavior where an unset `CKA_COPYABLE` attribute reads as
103+
`CK_FALSE`. The default (without this flag) is `CK_TRUE`, matching the PKCS#11
104+
specification. Define this only if your application or stored tokens depend on
105+
the old read-back behavior. The `C_CopyObject` enforcement consults the stored
106+
copyable flag directly, so this macro does not change which objects can be
107+
copied; it only affects what `C_GetAttributeValue` reports for objects whose
108+
`CKA_COPYABLE` was never explicitly set.
109+
110+
### Behavior changes for PKCS#11 spec compliance
111+
112+
Several pre-existing gaps were closed; applications upgrading from earlier
113+
versions may need to update templates or error-handling:
114+
115+
- `C_DeriveKey` now enforces `CKA_DERIVE` on the base key. Keys that do not
116+
set `CKA_DERIVE=CK_TRUE` at creation are rejected with
117+
`CKR_KEY_TYPE_INCONSISTENT`. RSA, EC, and ML-DSA private keys default
118+
`CKA_DERIVE` to `CK_FALSE`, so applications doing ECDH or other key
119+
derivation must now set `CKA_DERIVE=CK_TRUE` explicitly in the key
120+
template.
121+
- `C_CopyObject` enforces `CKA_COPYABLE` (returns `CKR_ACTION_PROHIBITED`
122+
on `CK_FALSE`).
123+
- `C_DestroyObject` enforces `CKA_DESTROYABLE` (returns
124+
`CKR_ACTION_PROHIBITED` on `CK_FALSE`).
125+
- `C_EncapsulateKey`/`C_DecapsulateKey` enforce `CKA_ENCAPSULATE` /
126+
`CKA_DECAPSULATE` and return `CKR_KEY_FUNCTION_NOT_PERMITTED` when the
127+
flag is `CK_FALSE`.
128+
- `C_SetAttributeValue` rejects flipping `CKA_COPYABLE` or
129+
`CKA_DESTROYABLE` from `CK_FALSE` back to `CK_TRUE`, returning
130+
`CKR_ATTRIBUTE_READ_ONLY` per spec section 4.4.1.
131+
100132
#### Analog Devices, Inc. MAXQ10xx Secure Elements ([MAXQ1065](https://www.analog.com/en/products/maxq1065.html)/MAXQ1080)
101133

102134
Support has been added to use the MAXQ10xx hardware for cryptographic operations

src/crypto.c

Lines changed: 166 additions & 34 deletions
Large diffs are not rendered by default.

src/internal.c

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7012,10 +7012,12 @@ int WP11_Slot_CheckSOPin(WP11_Slot* slot, char* pin, int pinLen)
70127012

70137013
if (token->state != WP11_TOKEN_STATE_INITIALIZED)
70147014
ret = PIN_NOT_SET_E;
7015-
/* NSS PK11_InitPin tries to login with an empty pin before setting the pin.
7016-
* This is effectively a public access, so should be OK.
7015+
/* Reject any PIN check (including the NSS empty-PIN public probe) when
7016+
* the SO PIN has not been set; otherwise an empty PIN would
7017+
* constant-compare equal to the unset zero-length stored PIN and grant
7018+
* SO authentication.
70177019
*/
7018-
if (!(token->tokenFlags & WP11_TOKEN_FLAG_SO_PIN_SET) && pinLen > 0)
7020+
if (!(token->tokenFlags & WP11_TOKEN_FLAG_SO_PIN_SET))
70197021
ret = PIN_NOT_SET_E;
70207022

70217023
if (ret == 0) {
@@ -8941,6 +8943,32 @@ CK_OBJECT_CLASS WP11_Object_GetClass(WP11_Object* object)
89418943
return object->objClass;
89428944
}
89438945

8946+
/**
8947+
* Check whether the object is copyable.
8948+
*
8949+
* Reads the underlying WP11_FLAG_NOT_COPYABLE bit directly so the result is
8950+
* not affected by the WOLFPKCS11_LEGACY_COPYABLE_FALSE_DEFAULT macro that
8951+
* controls the C_GetAttributeValue view.
8952+
*
8953+
* @param object [in] Object object.
8954+
* @return 1 when copyable, 0 when not.
8955+
*/
8956+
int WP11_Object_IsCopyable(WP11_Object* object)
8957+
{
8958+
return (object->opFlag & WP11_FLAG_NOT_COPYABLE) == 0;
8959+
}
8960+
8961+
/**
8962+
* Check whether the object is destroyable.
8963+
*
8964+
* @param object [in] Object object.
8965+
* @return 1 when destroyable, 0 when not.
8966+
*/
8967+
int WP11_Object_IsDestroyable(WP11_Object* object)
8968+
{
8969+
return (object->opFlag & WP11_FLAG_NOT_DESTROYABLE) == 0;
8970+
}
8971+
89448972
#if !defined(NO_RSA) || defined(HAVE_ECC)
89458973
/**
89468974
* Set the multi-precision integer from the data.
@@ -10866,10 +10894,16 @@ int WP11_Object_GetAttr(WP11_Object* object, CK_ATTRIBUTE_TYPE type, byte* data,
1086610894
ret = GetOpFlagBool(object->opFlag, WP11_FLAG_TRUSTED, data, len);
1086710895
break;
1086810896
case CKA_COPYABLE:
10897+
#ifdef WOLFPKCS11_LEGACY_COPYABLE_FALSE_DEFAULT
1086910898
ret = GetBool(CK_FALSE, data, len);
10899+
#else
10900+
ret = GetBool(
10901+
!(object->opFlag & WP11_FLAG_NOT_COPYABLE), data, len);
10902+
#endif
1087010903
break;
1087110904
case CKA_DESTROYABLE:
10872-
ret = GetBool(CK_TRUE, data, len);
10905+
ret = GetBool(
10906+
!(object->opFlag & WP11_FLAG_NOT_DESTROYABLE), data, len);
1087310907
break;
1087410908
case CKA_APPLICATION:
1087510909
if (object->objClass == CKO_DATA) {
@@ -11220,6 +11254,15 @@ int WP11_Object_SetAttr(WP11_Object* object, CK_ATTRIBUTE_TYPE type, byte* data,
1122011254
case CKA_DERIVE:
1122111255
WP11_Object_SetOpFlag(object, WP11_FLAG_DERIVE, *(CK_BBOOL*)data);
1122211256
break;
11257+
case CKA_COPYABLE:
11258+
/* Stored as the inverse: flag set when value is CK_FALSE. */
11259+
WP11_Object_SetOpFlag(object, WP11_FLAG_NOT_COPYABLE,
11260+
!*(CK_BBOOL*)data);
11261+
break;
11262+
case CKA_DESTROYABLE:
11263+
WP11_Object_SetOpFlag(object, WP11_FLAG_NOT_DESTROYABLE,
11264+
!*(CK_BBOOL*)data);
11265+
break;
1122311266
case CKA_ID:
1122411267
ret = WP11_Object_SetKeyId(object, data, (int)len);
1122511268
break;

tests/include.am

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,11 @@ noinst_PROGRAMS += tests/empty_pin_store_test
4141
tests_empty_pin_store_test_SOURCES = tests/empty_pin_store_test.c
4242
tests_empty_pin_store_test_LDADD =
4343

44+
check_PROGRAMS += tests/so_login_uninit_test
45+
noinst_PROGRAMS += tests/so_login_uninit_test
46+
tests_so_login_uninit_test_SOURCES = tests/so_login_uninit_test.c
47+
tests_so_login_uninit_test_LDADD =
48+
4449
check_PROGRAMS += tests/find_objects_null_template_test
4550
noinst_PROGRAMS += tests/find_objects_null_template_test
4651
tests_find_objects_null_template_test_SOURCES = tests/find_objects_null_template_test.c
@@ -90,6 +95,7 @@ tests_rsa_session_persistence_test_LDADD += src/libwolfpkcs11.la
9095
tests_debug_test_LDADD += src/libwolfpkcs11.la
9196
tests_object_id_uniqueness_test_LDADD += src/libwolfpkcs11.la
9297
tests_empty_pin_store_test_LDADD += src/libwolfpkcs11.la
98+
tests_so_login_uninit_test_LDADD += src/libwolfpkcs11.la
9399
tests_find_objects_null_template_test_LDADD += src/libwolfpkcs11.la
94100
tests_aes_cbc_pad_padding_test_LDADD += src/libwolfpkcs11.la
95101
tests_ecb_check_value_error_test_LDADD += src/libwolfpkcs11.la
@@ -101,6 +107,7 @@ tests_rsa_exponent_test_LDADD += src/libwolfpkcs11.la
101107
else
102108
tests_object_id_uniqueness_test_LDADD += src/libwolfpkcs11.la
103109
tests_empty_pin_store_test_LDADD += src/libwolfpkcs11.la
110+
tests_so_login_uninit_test_LDADD += src/libwolfpkcs11.la
104111
tests_find_objects_null_template_test_LDADD += src/libwolfpkcs11.la
105112
tests_aes_cbc_pad_padding_test_LDADD += src/libwolfpkcs11.la
106113
tests_ecb_check_value_error_test_LDADD += src/libwolfpkcs11.la

tests/pkcs11mtt.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -805,6 +805,7 @@ static CK_RV get_generic_key(CK_SESSION_HANDLE session, unsigned char* data,
805805
{ CKA_SENSITIVE, &sensitive, sizeof(CK_BBOOL) },
806806
{ CKA_SIGN, &ckTrue, sizeof(ckTrue) },
807807
{ CKA_VERIFY, &ckTrue, sizeof(ckTrue) },
808+
{ CKA_DERIVE, &ckTrue, sizeof(ckTrue) },
808809
{ CKA_VALUE, data, len },
809810
};
810811
int cnt = sizeof(generic_key)/sizeof(*generic_key);
@@ -3434,6 +3435,7 @@ static CK_OBJECT_HANDLE get_ecc_priv_key(CK_SESSION_HANDLE session,
34343435
{ CKA_EXTRACTABLE, &extractable, sizeof(CK_BBOOL) },
34353436
{ CKA_SENSITIVE, &sensitive, sizeof(CK_BBOOL) },
34363437
{ CKA_VERIFY, &ckTrue, sizeof(ckTrue) },
3438+
{ CKA_DERIVE, &ckTrue, sizeof(ckTrue) },
34373439
{ CKA_EC_PARAMS, ecc_p256_params, sizeof(ecc_p256_params) },
34383440
{ CKA_VALUE, ecc_p256_priv, sizeof(ecc_p256_priv) },
34393441
};

0 commit comments

Comments
 (0)