Skip to content

Commit 703dd6e

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 703dd6e

9 files changed

Lines changed: 1087 additions & 40 deletions

File tree

README.md

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,49 @@ 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 `C_GetAttributeValue(CKA_COPYABLE)` always
103+
reports `CK_FALSE`, regardless of whether the attribute was explicitly set.
104+
The default (without this flag) reports `CK_TRUE` for new objects and reflects
105+
the stored value for objects whose `CKA_COPYABLE` was explicitly set, matching
106+
the PKCS#11 specification. Define this only if your application or stored
107+
tokens depend on the old read-back behavior. The `C_CopyObject` enforcement
108+
consults the stored copyable flag directly via `WP11_Object_IsCopyable`, so
109+
this macro does not change which objects can be copied; objects created with
110+
`CKA_COPYABLE=CK_TRUE` (the default) remain copyable, and objects created
111+
with `CKA_COPYABLE=CK_FALSE` are still rejected by `C_CopyObject` with
112+
`CKR_ACTION_PROHIBITED`.
113+
114+
### Behavior changes for PKCS#11 spec compliance
115+
116+
Several pre-existing gaps were closed; applications upgrading from earlier
117+
versions may need to update templates or error-handling:
118+
119+
- `C_DeriveKey` now enforces `CKA_DERIVE` on the base key. Keys that do not
120+
set `CKA_DERIVE=CK_TRUE` at creation are rejected with
121+
`CKR_KEY_TYPE_INCONSISTENT`. RSA, EC, and ML-DSA private keys default
122+
`CKA_DERIVE` to `CK_FALSE`, so applications doing ECDH or other key
123+
derivation must now set `CKA_DERIVE=CK_TRUE` explicitly in the key
124+
template.
125+
- `C_CopyObject` enforces `CKA_COPYABLE` (returns `CKR_ACTION_PROHIBITED`
126+
on `CK_FALSE`).
127+
- `C_DestroyObject` enforces `CKA_DESTROYABLE` (returns
128+
`CKR_ACTION_PROHIBITED` on `CK_FALSE`).
129+
- `C_EncapsulateKey`/`C_DecapsulateKey` enforce `CKA_ENCAPSULATE` /
130+
`CKA_DECAPSULATE` and return `CKR_KEY_FUNCTION_NOT_PERMITTED` when the
131+
flag is `CK_FALSE`.
132+
- `C_SetAttributeValue` rejects flipping `CKA_COPYABLE` or
133+
`CKA_DESTROYABLE` from `CK_FALSE` back to `CK_TRUE`, returning
134+
`CKR_ATTRIBUTE_READ_ONLY` per spec section 4.4.1.
135+
- `C_Login(CKU_SO, ...)` against a token whose SO PIN has not been set
136+
now returns `CKR_USER_PIN_NOT_INITIALIZED` regardless of the PIN
137+
length, closing an empty-PIN bypass where the zero-length constant
138+
compare against the unset stored PIN returned equal. NSS builds
139+
(`--enable-nss`) keep the empty-PIN probe accepted on uninitialized
140+
tokens because `PK11_InitPin` bootstraps an empty-password NSS
141+
database that way; non-empty PINs are still rejected.
142+
100143
#### Analog Devices, Inc. MAXQ10xx Secure Elements ([MAXQ1065](https://www.analog.com/en/products/maxq1065.html)/MAXQ1080)
101144

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

src/crypto.c

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

src/internal.c

Lines changed: 53 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7012,11 +7012,20 @@ 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+
/* When the SO PIN has not been set, reject any PIN check; otherwise an
7016+
* empty PIN would constant-compare equal to the unset zero-length
7017+
* stored PIN and grant SO authentication. NSS's PK11_InitPin bootstraps
7018+
* a fresh database by calling C_Login(CKU_SO, "", 0) before any SO PIN
7019+
* exists and relies on that probe succeeding, so for NSS builds the
7020+
* empty-PIN path is left intact and only non-empty PINs are rejected.
70177021
*/
7022+
#ifdef WOLFPKCS11_NSS
70187023
if (!(token->tokenFlags & WP11_TOKEN_FLAG_SO_PIN_SET) && pinLen > 0)
70197024
ret = PIN_NOT_SET_E;
7025+
#else
7026+
if (!(token->tokenFlags & WP11_TOKEN_FLAG_SO_PIN_SET))
7027+
ret = PIN_NOT_SET_E;
7028+
#endif
70207029

70217030
if (ret == 0) {
70227031
WP11_Lock_UnlockRO(&slot->lock);
@@ -8941,6 +8950,32 @@ CK_OBJECT_CLASS WP11_Object_GetClass(WP11_Object* object)
89418950
return object->objClass;
89428951
}
89438952

8953+
/**
8954+
* Check whether the object is copyable.
8955+
*
8956+
* Reads the underlying WP11_FLAG_NOT_COPYABLE bit directly so the result is
8957+
* not affected by the WOLFPKCS11_LEGACY_COPYABLE_FALSE_DEFAULT macro that
8958+
* controls the C_GetAttributeValue view.
8959+
*
8960+
* @param object [in] Object object.
8961+
* @return 1 when copyable, 0 when not.
8962+
*/
8963+
int WP11_Object_IsCopyable(WP11_Object* object)
8964+
{
8965+
return (object->opFlag & WP11_FLAG_NOT_COPYABLE) == 0;
8966+
}
8967+
8968+
/**
8969+
* Check whether the object is destroyable.
8970+
*
8971+
* @param object [in] Object object.
8972+
* @return 1 when destroyable, 0 when not.
8973+
*/
8974+
int WP11_Object_IsDestroyable(WP11_Object* object)
8975+
{
8976+
return (object->opFlag & WP11_FLAG_NOT_DESTROYABLE) == 0;
8977+
}
8978+
89448979
#if !defined(NO_RSA) || defined(HAVE_ECC)
89458980
/**
89468981
* Set the multi-precision integer from the data.
@@ -10866,10 +10901,16 @@ int WP11_Object_GetAttr(WP11_Object* object, CK_ATTRIBUTE_TYPE type, byte* data,
1086610901
ret = GetOpFlagBool(object->opFlag, WP11_FLAG_TRUSTED, data, len);
1086710902
break;
1086810903
case CKA_COPYABLE:
10904+
#ifdef WOLFPKCS11_LEGACY_COPYABLE_FALSE_DEFAULT
1086910905
ret = GetBool(CK_FALSE, data, len);
10906+
#else
10907+
ret = GetBool(
10908+
!(object->opFlag & WP11_FLAG_NOT_COPYABLE), data, len);
10909+
#endif
1087010910
break;
1087110911
case CKA_DESTROYABLE:
10872-
ret = GetBool(CK_TRUE, data, len);
10912+
ret = GetBool(
10913+
!(object->opFlag & WP11_FLAG_NOT_DESTROYABLE), data, len);
1087310914
break;
1087410915
case CKA_APPLICATION:
1087510916
if (object->objClass == CKO_DATA) {
@@ -11220,6 +11261,15 @@ int WP11_Object_SetAttr(WP11_Object* object, CK_ATTRIBUTE_TYPE type, byte* data,
1122011261
case CKA_DERIVE:
1122111262
WP11_Object_SetOpFlag(object, WP11_FLAG_DERIVE, *(CK_BBOOL*)data);
1122211263
break;
11264+
case CKA_COPYABLE:
11265+
/* Stored as the inverse: flag set when value is CK_FALSE. */
11266+
WP11_Object_SetOpFlag(object, WP11_FLAG_NOT_COPYABLE,
11267+
!*(CK_BBOOL*)data);
11268+
break;
11269+
case CKA_DESTROYABLE:
11270+
WP11_Object_SetOpFlag(object, WP11_FLAG_NOT_DESTROYABLE,
11271+
!*(CK_BBOOL*)data);
11272+
break;
1122311273
case CKA_ID:
1122411274
ret = WP11_Object_SetKeyId(object, data, (int)len);
1122511275
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: 14 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);
@@ -1258,6 +1259,18 @@ static CK_RV test_digest(void* args)
12581259
CHECK_CKR_FAIL(ret, CKR_SESSION_HANDLE_INVALID,
12591260
"Digest Key invalid session handle");
12601261
}
1262+
if (ret == CKR_OK) {
1263+
/* C_DigestKey must return CKR_OPERATION_NOT_INITIALIZED before any
1264+
* other validation when C_DigestInit has not been called. */
1265+
ret = funcList->C_DigestKey(session, key);
1266+
CHECK_CKR_FAIL(ret, CKR_OPERATION_NOT_INITIALIZED,
1267+
"Digest Key without DigestInit");
1268+
}
1269+
if (ret == CKR_OK) {
1270+
/* Now initialize and exercise the invalid-object-handle path. */
1271+
ret = funcList->C_DigestInit(session, &mech);
1272+
CHECK_CKR(ret, "Digest Init for invalid-handle case");
1273+
}
12611274
if (ret == CKR_OK) {
12621275
ret = funcList->C_DigestKey(session, CK_INVALID_HANDLE);
12631276
CHECK_CKR_FAIL(ret, CKR_OBJECT_HANDLE_INVALID,
@@ -3434,6 +3447,7 @@ static CK_OBJECT_HANDLE get_ecc_priv_key(CK_SESSION_HANDLE session,
34343447
{ CKA_EXTRACTABLE, &extractable, sizeof(CK_BBOOL) },
34353448
{ CKA_SENSITIVE, &sensitive, sizeof(CK_BBOOL) },
34363449
{ CKA_VERIFY, &ckTrue, sizeof(ckTrue) },
3450+
{ CKA_DERIVE, &ckTrue, sizeof(ckTrue) },
34373451
{ CKA_EC_PARAMS, ecc_p256_params, sizeof(ecc_p256_params) },
34383452
{ CKA_VALUE, ecc_p256_priv, sizeof(ecc_p256_priv) },
34393453
};

0 commit comments

Comments
 (0)