Fix static code compliance findings#187
Open
LinuxJedi wants to merge 1 commit into
Open
Conversation
Member
LinuxJedi
commented
May 13, 2026
- F-3637: pin SecretObject_GetAttr CKA_SENSITIVE guard with a test exercising CKA_SENSITIVE=TRUE + CKA_EXTRACTABLE=TRUE.
- F-2776: RsaObject_GetAttr / EcObject_GetAttr / DhObject_GetAttr / MldsaObject_GetAttr / MlKemObject_GetAttr now return CKR_ATTRIBUTE_SENSITIVE alongside the existing CK_UNAVAILABLE_INFORMATION sentinel. Updated test_attributes_rsa / ecc / dh in pkcs11test.c and pkcs11mtt.c to expect the new return.
- F-1343: introduced WP11_FLAG_NOT_MODIFIABLE (inverted, mirroring NOT_COPYABLE / NOT_DESTROYABLE) and WP11_Object_IsModifiable; C_SetAttributeValue returns CKR_ACTION_PROHIBITED for CKA_MODIFIABLE=FALSE objects. Gate lives in the public entry point only so C_CopyObject (governed by CKA_COPYABLE) is unaffected.
- F-3144: new CheckPrivateLogin helper in crypto.c wired into C_CreateObject, C_GenerateKey, C_GenerateKeyPair (both templates), and C_UnwrapKey. Returns CKR_USER_NOT_LOGGED_IN for CKA_PRIVATE=TRUE on a public session; empty-PIN tokens bypassed to match the find-time filter in WP11_Session_Find.
- F-2370: SetAttributeDefaults seeds CKA_PRIVATE=CK_TRUE for CKO_PRIVATE_KEY / CKO_SECRET_KEY and CK_FALSE for CKO_PUBLIC_KEY, gated by WOLFPKCS11_LEGACY_PRIVATE_FALSE_DEFAULT.
- F-2774: CKA_WRAP / CKA_UNWRAP default flipped to CK_FALSE in SetAttributeDefaults, gated by WOLFPKCS11_LEGACY_WRAP_TRUE_DEFAULT. pkcs11test.c / pkcs11mtt.c get_aes_128_key, get_generic_key, get_rsa_pub_key, get_rsa_priv_key set CKA_WRAP/CKA_UNWRAP=TRUE explicitly where wrap/unwrap is exercised.
- F-3407: wp11_Token_Init no longer eager-marks state INITIALIZED; WP11_Slot_TokenReset sets it on C_InitToken. Redundant state-vs-INITIALIZED checks dropped from WP11_Slot_CheckSOPin / CheckUserPin so the NSS empty-PIN bootstrap probe still passes on a fresh slot.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates wolfPKCS11’s PKCS#11 behavior to satisfy multiple static/compliance findings (Fenrir), primarily around attribute sensitivity, default attribute values, login gating for private objects, and token initialization state, with corresponding test updates and new compliance regression coverage.
Changes:
- Tighten PKCS#11 compliance for sensitive attributes and modifiability enforcement (e.g., return
CKR_ATTRIBUTE_SENSITIVE, enforceCKA_MODIFIABLEinC_SetAttributeValue). - Adjust default attribute seeding (
CKA_PRIVATE,CKA_WRAP/CKA_UNWRAP) behind legacy compatibility macros. - Fix token initialization flagging (
CKF_TOKEN_INITIALIZED) and expand/adjust test coverage accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
wolfpkcs11/internal.h |
Renames/modifies the persisted modifiable flag bit semantics and adds WP11_Object_IsModifiable prototype. |
src/internal.c |
Implements token state initialization changes, adds WP11_Object_IsModifiable, and returns CKR_ATTRIBUTE_SENSITIVE for sensitive asymmetric attrs. |
src/crypto.c |
Adds CheckPrivateLogin, enforces CKA_MODIFIABLE in C_SetAttributeValue, and flips defaults for CKA_PRIVATE and CKA_WRAP/UNWRAP (macro-gated). |
tests/pkcs11test.c |
Updates expected return codes/attrs and explicitly sets wrap/unwrap where needed after default flips. |
tests/pkcs11mtt.c |
Mirrors pkcs11test updates for MT tests (wrap/unwrap defaults and sensitive attr expectations). |
tests/pkcs11_compliance_test.c |
Adds targeted regression tests for findings 3637/2776/3407/1343/3144/2370/2774 and adjusts helper key templates for the new defaults. |
Comments suppressed due to low confidence (2)
src/crypto.c:363
- CheckPrivateLogin treats a malformed CKA_PRIVATE attribute (present but pValue==NULL or ulValueLen != sizeof(CK_BBOOL)) as “absent” and may fall back to class-based defaults, potentially returning CKR_USER_NOT_LOGGED_IN before the normal template validation later returns CKR_ATTRIBUTE_VALUE_INVALID/CKR_BUFFER_TOO_SMALL. To preserve correct error precedence, either validate CKA_PRIVATE here (and return the appropriate template error) or ignore the attribute entirely when it is present-but-invalid (so later validation can report the right failure).
FindAttributeType(pTemplate, ulCount, CKA_PRIVATE, &attr);
if (attr != NULL && attr->pValue != NULL &&
attr->ulValueLen == sizeof(CK_BBOOL)) {
isPrivate = *(CK_BBOOL*)attr->pValue;
}
else {
#ifndef WOLFPKCS11_LEGACY_PRIVATE_FALSE_DEFAULT
CK_ATTRIBUTE* classAttr = NULL;
FindAttributeType(pTemplate, ulCount, CKA_CLASS, &classAttr);
if (classAttr != NULL && classAttr->pValue != NULL &&
classAttr->ulValueLen == sizeof(CK_OBJECT_CLASS)) {
CK_OBJECT_CLASS objClass =
*(CK_OBJECT_CLASS*)classAttr->pValue;
if (objClass == CKO_PRIVATE_KEY || objClass == CKO_SECRET_KEY)
isPrivate = CK_TRUE;
}
#endif
}
src/crypto.c:594
- The intent comment says this sets an explicit default CKA_PRIVATE=FALSE for public keys, but SetIfNotFound() is a no-op when the default state is CK_FALSE (it returns early). As written, this block doesn’t actually set CKA_PRIVATE on the object; it only relies on the implicit “flag bit absent => FALSE” behavior. Consider either removing/rewording the comment, or using a helper that can explicitly persist a CK_FALSE default when that’s the goal.
#ifndef WOLFPKCS11_LEGACY_PRIVATE_FALSE_DEFAULT
/* Spec default: public keys are CKA_PRIVATE=FALSE (this is also
* what the historical implementation produced, but make it
* explicit so the default is stable when the macro toggles
* private/secret keys below). */
if (ret == CKR_OK)
ret = SetIfNotFound(obj, CKA_PRIVATE, falseVal, pTemplate,
ulCount);
#endif
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- F-3637: pin SecretObject_GetAttr CKA_SENSITIVE guard with a test exercising CKA_SENSITIVE=TRUE + CKA_EXTRACTABLE=TRUE. - F-2776: RsaObject_GetAttr / EcObject_GetAttr / DhObject_GetAttr / MldsaObject_GetAttr / MlKemObject_GetAttr now return CKR_ATTRIBUTE_SENSITIVE alongside the existing CK_UNAVAILABLE_INFORMATION sentinel. Updated test_attributes_rsa / ecc / dh in pkcs11test.c and pkcs11mtt.c to expect the new return. - F-1343: introduced WP11_FLAG_NOT_MODIFIABLE (inverted, mirroring NOT_COPYABLE / NOT_DESTROYABLE) and WP11_Object_IsModifiable; C_SetAttributeValue returns CKR_ACTION_PROHIBITED for CKA_MODIFIABLE=FALSE objects. Gate lives in the public entry point only so C_CopyObject (governed by CKA_COPYABLE) is unaffected. - F-3144: new CheckPrivateLogin helper in crypto.c wired into C_CreateObject, C_GenerateKey, C_GenerateKeyPair (both templates), and C_UnwrapKey. Returns CKR_USER_NOT_LOGGED_IN for CKA_PRIVATE=TRUE on a public session; empty-PIN tokens bypassed to match the find-time filter in WP11_Session_Find. - F-2370: SetAttributeDefaults seeds CKA_PRIVATE=CK_TRUE for CKO_PRIVATE_KEY / CKO_SECRET_KEY and CK_FALSE for CKO_PUBLIC_KEY, gated by WOLFPKCS11_LEGACY_PRIVATE_FALSE_DEFAULT. - F-2774: CKA_WRAP / CKA_UNWRAP default flipped to CK_FALSE in SetAttributeDefaults, gated by WOLFPKCS11_LEGACY_WRAP_TRUE_DEFAULT. pkcs11test.c / pkcs11mtt.c get_aes_128_key, get_generic_key, get_rsa_pub_key, get_rsa_priv_key set CKA_WRAP/CKA_UNWRAP=TRUE explicitly where wrap/unwrap is exercised. - F-3407: wp11_Token_Init no longer eager-marks state INITIALIZED; WP11_Slot_TokenReset sets it on C_InitToken. Redundant state-vs-INITIALIZED checks dropped from WP11_Slot_CheckSOPin / CheckUserPin so the NSS empty-PIN bootstrap probe still passes on a fresh slot.
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.