Merge pull request #185 from LinuxJedi/f-fixes8

Fix Fenrir static analysis findings

Author: dgarske

README.md

@@ -97,6 +97,52 @@ See wolfpkcs11/store.h for prototypes of functions to implement.
 
 Sets the private key's label against the public key when generating key pairs.
 
+#### Define WOLFPKCS11_LEGACY_COPYABLE_FALSE_DEFAULT
+
+Restores the pre-fix behavior where `C_GetAttributeValue(CKA_COPYABLE)` always
+reports `CK_FALSE`, regardless of whether the attribute was explicitly set.
+The default (without this flag) reports `CK_TRUE` for new objects and reflects
+the stored value for objects whose `CKA_COPYABLE` was explicitly set, matching
+the PKCS#11 specification. Define this only if your application or stored
+tokens depend on the old read-back behavior. The `C_CopyObject` enforcement
+consults the stored copyable flag directly via `WP11_Object_IsCopyable`, so
+this macro does not change which objects can be copied; objects created with
+`CKA_COPYABLE=CK_TRUE` (the default) remain copyable, and objects created
+with `CKA_COPYABLE=CK_FALSE` are still rejected by `C_CopyObject` with
+`CKR_ACTION_PROHIBITED`.
+
+### Behavior changes for PKCS#11 spec compliance
+
+Several pre-existing gaps were closed; applications upgrading from earlier
+versions may need to update templates or error-handling:
+
+- `C_DeriveKey` now enforces `CKA_DERIVE` on the base key. Keys that do not
+  set `CKA_DERIVE=CK_TRUE` at creation are rejected with
+  `CKR_KEY_TYPE_INCONSISTENT`. RSA, EC, and ML-DSA private keys default
+  `CKA_DERIVE` to `CK_FALSE`, so applications doing ECDH or other key
+  derivation must now set `CKA_DERIVE=CK_TRUE` explicitly in the key
+  template. The check is skipped on `--enable-nss` builds because NSS
+  generates ephemeral ECDHE keys without setting `CKA_DERIVE` and relies on
+  the historic permissive behavior; non-NSS builds get the spec-compliant
+  enforcement.
+- `C_CopyObject` enforces `CKA_COPYABLE` (returns `CKR_ACTION_PROHIBITED`
+  on `CK_FALSE`).
+- `C_DestroyObject` enforces `CKA_DESTROYABLE` (returns
+  `CKR_ACTION_PROHIBITED` on `CK_FALSE`).
+- `C_EncapsulateKey`/`C_DecapsulateKey` enforce `CKA_ENCAPSULATE` /
+  `CKA_DECAPSULATE` and return `CKR_KEY_FUNCTION_NOT_PERMITTED` when the
+  flag is `CK_FALSE`.
+- `C_SetAttributeValue` rejects flipping `CKA_COPYABLE` or
+  `CKA_DESTROYABLE` from `CK_FALSE` back to `CK_TRUE`, returning
+  `CKR_ATTRIBUTE_READ_ONLY` per spec section 4.4.1.
+- `C_Login(CKU_SO, ...)` against a token whose SO PIN has not been set
+  now returns `CKR_USER_PIN_NOT_INITIALIZED` regardless of the PIN
+  length, closing an empty-PIN bypass where the zero-length constant
+  compare against the unset stored PIN returned equal. NSS builds
+  (`--enable-nss`) keep the empty-PIN probe accepted on uninitialized
+  tokens because `PK11_InitPin` bootstraps an empty-password NSS
+  database that way; non-empty PINs are still rejected.
+
 #### Analog Devices, Inc. MAXQ10xx Secure Elements ([MAXQ1065](https://www.analog.com/en/products/maxq1065.html)/MAXQ1080)
 
 Support has been added to use the MAXQ10xx hardware for cryptographic operations

src/crypto.c

@@ -7012,11 +7012,20 @@ int WP11_Slot_CheckSOPin(WP11_Slot* slot, char* pin, int pinLen)
 
     if (token->state != WP11_TOKEN_STATE_INITIALIZED)
         ret = PIN_NOT_SET_E;
-    /* NSS PK11_InitPin tries to login with an empty pin before setting the pin.
-     * This is effectively a public access, so should be OK.
+    /* When the SO PIN has not been set, reject any PIN check; otherwise an
+     * empty PIN would constant-compare equal to the unset zero-length
+     * stored PIN and grant SO authentication. NSS's PK11_InitPin bootstraps
+     * a fresh database by calling C_Login(CKU_SO, "", 0) before any SO PIN
+     * exists and relies on that probe succeeding, so for NSS builds the
+     * empty-PIN path is left intact and only non-empty PINs are rejected.
      */
+#ifdef WOLFPKCS11_NSS
     if (!(token->tokenFlags & WP11_TOKEN_FLAG_SO_PIN_SET) && pinLen > 0)
         ret = PIN_NOT_SET_E;
+#else
+    if (!(token->tokenFlags & WP11_TOKEN_FLAG_SO_PIN_SET))
+        ret = PIN_NOT_SET_E;
+#endif
 
     if (ret == 0) {
         WP11_Lock_UnlockRO(&slot->lock);
@@ -8941,6 +8950,32 @@ CK_OBJECT_CLASS WP11_Object_GetClass(WP11_Object* object)
     return object->objClass;
 }
 
+/**
+ * Check whether the object is copyable.
+ *
+ * Reads the underlying WP11_FLAG_NOT_COPYABLE bit directly so the result is
+ * not affected by the WOLFPKCS11_LEGACY_COPYABLE_FALSE_DEFAULT macro that
+ * controls the C_GetAttributeValue view.
+ *
+ * @param  object  [in]  Object object.
+ * @return  1 when copyable, 0 when not.
+ */
+int WP11_Object_IsCopyable(WP11_Object* object)
+{
+    return (object->opFlag & WP11_FLAG_NOT_COPYABLE) == 0;
+}
+
+/**
+ * Check whether the object is destroyable.
+ *
+ * @param  object  [in]  Object object.
+ * @return  1 when destroyable, 0 when not.
+ */
+int WP11_Object_IsDestroyable(WP11_Object* object)
+{
+    return (object->opFlag & WP11_FLAG_NOT_DESTROYABLE) == 0;
+}
+
 #if !defined(NO_RSA) || defined(HAVE_ECC)
 /**
  * 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,
             ret = GetOpFlagBool(object->opFlag, WP11_FLAG_TRUSTED, data, len);
             break;
         case CKA_COPYABLE:
+#ifdef WOLFPKCS11_LEGACY_COPYABLE_FALSE_DEFAULT
             ret = GetBool(CK_FALSE, data, len);
+#else
+            ret = GetBool(
+                !(object->opFlag & WP11_FLAG_NOT_COPYABLE), data, len);
+#endif
             break;
         case CKA_DESTROYABLE:
-            ret = GetBool(CK_TRUE, data, len);
+            ret = GetBool(
+                !(object->opFlag & WP11_FLAG_NOT_DESTROYABLE), data, len);
             break;
         case CKA_APPLICATION:
             if (object->objClass == CKO_DATA) {
@@ -11220,6 +11261,15 @@ int WP11_Object_SetAttr(WP11_Object* object, CK_ATTRIBUTE_TYPE type, byte* data,
         case CKA_DERIVE:
             WP11_Object_SetOpFlag(object, WP11_FLAG_DERIVE, *(CK_BBOOL*)data);
             break;
+        case CKA_COPYABLE:
+            /* Stored as the inverse: flag set when value is CK_FALSE. */
+            WP11_Object_SetOpFlag(object, WP11_FLAG_NOT_COPYABLE,
+                                  !*(CK_BBOOL*)data);
+            break;
+        case CKA_DESTROYABLE:
+            WP11_Object_SetOpFlag(object, WP11_FLAG_NOT_DESTROYABLE,
+                                  !*(CK_BBOOL*)data);
+            break;
         case CKA_ID:
             ret = WP11_Object_SetKeyId(object, data, (int)len);
             break;

src/internal.c

@@ -41,6 +41,11 @@ noinst_PROGRAMS += tests/empty_pin_store_test
 tests_empty_pin_store_test_SOURCES = tests/empty_pin_store_test.c
 tests_empty_pin_store_test_LDADD =
 
+check_PROGRAMS += tests/so_login_uninit_test
+noinst_PROGRAMS += tests/so_login_uninit_test
+tests_so_login_uninit_test_SOURCES = tests/so_login_uninit_test.c
+tests_so_login_uninit_test_LDADD =
+
 check_PROGRAMS += tests/find_objects_null_template_test
 noinst_PROGRAMS += tests/find_objects_null_template_test
 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
 tests_debug_test_LDADD += src/libwolfpkcs11.la
 tests_object_id_uniqueness_test_LDADD += src/libwolfpkcs11.la
 tests_empty_pin_store_test_LDADD += src/libwolfpkcs11.la
+tests_so_login_uninit_test_LDADD += src/libwolfpkcs11.la
 tests_find_objects_null_template_test_LDADD += src/libwolfpkcs11.la
 tests_aes_cbc_pad_padding_test_LDADD += src/libwolfpkcs11.la
 tests_ecb_check_value_error_test_LDADD += src/libwolfpkcs11.la
@@ -101,6 +107,7 @@ tests_rsa_exponent_test_LDADD += src/libwolfpkcs11.la
 else
 tests_object_id_uniqueness_test_LDADD += src/libwolfpkcs11.la
 tests_empty_pin_store_test_LDADD += src/libwolfpkcs11.la
+tests_so_login_uninit_test_LDADD += src/libwolfpkcs11.la
 tests_find_objects_null_template_test_LDADD += src/libwolfpkcs11.la
 tests_aes_cbc_pad_padding_test_LDADD += src/libwolfpkcs11.la
 tests_ecb_check_value_error_test_LDADD += src/libwolfpkcs11.la

tests/include.am

@@ -805,6 +805,7 @@ static CK_RV get_generic_key(CK_SESSION_HANDLE session, unsigned char* data,
         { CKA_SENSITIVE,         &sensitive,         sizeof(CK_BBOOL)          },
         { CKA_SIGN,              &ckTrue,           sizeof(ckTrue)            },
         { CKA_VERIFY,            &ckTrue,           sizeof(ckTrue)            },
+        { CKA_DERIVE,            &ckTrue,           sizeof(ckTrue)            },
         { CKA_VALUE,             data,              len                       },
     };
     int cnt = sizeof(generic_key)/sizeof(*generic_key);
@@ -1258,6 +1259,18 @@ static CK_RV test_digest(void* args)
         CHECK_CKR_FAIL(ret, CKR_SESSION_HANDLE_INVALID,
                                            "Digest Key invalid session handle");
     }
+    if (ret == CKR_OK) {
+        /* C_DigestKey must return CKR_OPERATION_NOT_INITIALIZED before any
+         * other validation when C_DigestInit has not been called. */
+        ret = funcList->C_DigestKey(session, key);
+        CHECK_CKR_FAIL(ret, CKR_OPERATION_NOT_INITIALIZED,
+                                        "Digest Key without DigestInit");
+    }
+    if (ret == CKR_OK) {
+        /* Now initialize and exercise the invalid-object-handle path. */
+        ret = funcList->C_DigestInit(session, &mech);
+        CHECK_CKR(ret, "Digest Init for invalid-handle case");
+    }
     if (ret == CKR_OK) {
         ret = funcList->C_DigestKey(session, CK_INVALID_HANDLE);
         CHECK_CKR_FAIL(ret, CKR_OBJECT_HANDLE_INVALID,
@@ -3434,6 +3447,7 @@ static CK_OBJECT_HANDLE get_ecc_priv_key(CK_SESSION_HANDLE session,
         { CKA_EXTRACTABLE,       &extractable,      sizeof(CK_BBOOL)          },
         { CKA_SENSITIVE,         &sensitive,         sizeof(CK_BBOOL)          },
         { CKA_VERIFY,            &ckTrue,           sizeof(ckTrue)            },
+        { CKA_DERIVE,            &ckTrue,           sizeof(ckTrue)            },
         { CKA_EC_PARAMS,         ecc_p256_params,   sizeof(ecc_p256_params)   },
         { CKA_VALUE,             ecc_p256_priv,     sizeof(ecc_p256_priv)     },
     };