Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10077
Scan targets checked: src, src-bugs, src-compliance, wolfcrypt-bugs, wolfcrypt-src
Findings: 3
High (1)
Missing ret == 0 guard in wc_MlKemKey_MakeKey crypto CB block causes null pointer dereference
File: wolfcrypt/src/wc_mlkem.c:458-467
Function: wc_MlKemKey_MakeKey
Category: Logic errors
The new WOLF_CRYPTO_CB block in wc_MlKemKey_MakeKey does not guard against a prior validation failure before accessing key->devId and key->type. If key == NULL, the preceding validation sets ret = BAD_FUNC_ARG but does not return. Execution falls through to the crypto CB block which unconditionally dereferences key->devId (when WOLF_CRYPTO_CB_FIND is not defined) or key->type (when WOLF_CRYPTO_CB_FIND is defined and the if guard is removed). This is inconsistent with wc_MlKemKey_Encapsulate and wc_MlKemKey_Decapsulate in the same PR, which both correctly wrap their crypto CB blocks with if ((ret == 0) ... && (key->devId != INVALID_DEVID)).
#ifdef WOLF_CRYPTO_CB
#ifndef WOLF_CRYPTO_CB_FIND
if (key->devId != INVALID_DEVID)
#endif
{
ret = wc_CryptoCb_MakePqcKemKey(rng, WC_PQC_KEM_TYPE_KYBER,
key->type, key);
if (ret != WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE))
return ret;
/* fall-through when unavailable */
ret = 0;
}
#endifRecommendation: Add a ret == 0 guard to match the pattern used in wc_MlKemKey_Encapsulate and wc_MlKemKey_Decapsulate:
#ifdef WOLF_CRYPTO_CB
if ((ret == 0)
#ifndef WOLF_CRYPTO_CB_FIND
&& (key->devId != INVALID_DEVID)
#endif
) {
ret = wc_CryptoCb_MakePqcKemKey(rng, WC_PQC_KEM_TYPE_KYBER,
key->type, key);
if (ret != WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE))
return ret;
ret = 0;
}
#endifMedium (1)
NULL pointer dereference in wc_MlKemKey_MakeKey crypto callback path
File: wolfcrypt/src/wc_mlkem.c:458-468
Function: wc_MlKemKey_MakeKey
Category: NULL pointer dereference
The newly added crypto callback dispatch block in wc_MlKemKey_MakeKey does not check ret == 0 before accessing key->devId and key->type. If key is NULL, the preceding validation sets ret = BAD_FUNC_ARG but does not return. Execution falls through to the #ifdef WOLF_CRYPTO_CB block which dereferences key unconditionally. This contrasts with wc_MlKemKey_Encapsulate and wc_MlKemKey_Decapsulate added in the same PR, which both correctly guard the crypto callback block with if ((ret == 0) ...) before accessing key->devId.
#ifdef WOLF_CRYPTO_CB
#ifndef WOLF_CRYPTO_CB_FIND
if (key->devId != INVALID_DEVID)
#endif
{
ret = wc_CryptoCb_MakePqcKemKey(rng, WC_PQC_KEM_TYPE_KYBER,
key->type, key);
if (ret != WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE))
return ret;
/* fall-through when unavailable */
ret = 0;
}
#endifRecommendation: Add a ret == 0 guard consistent with the Encapsulate/Decapsulate functions:
#ifdef WOLF_CRYPTO_CB
if ((ret == 0)
#ifndef WOLF_CRYPTO_CB_FIND
&& (key->devId != INVALID_DEVID)
#endif
) {
ret = wc_CryptoCb_MakePqcKemKey(...);
...
}
#endifLow (1)
Incomplete ForceZero of ML-KEM private key material in wc_Pkcs11StoreKey
File: wolfcrypt/src/wc_pkcs11.c:2215-2218
Function: wc_Pkcs11StoreKey
Category: Missing ForceZero
When clear is set in the PKCS11_KEY_TYPE_MLKEM case of wc_Pkcs11StoreKey, only mlkemKey->priv is zeroed via ForceZero. The z field (32-byte implicit rejection seed) is also secret key material but is not cleared. By comparison, wc_MlKemKey_Free correctly zeros both priv and z fields. After storing the private key to the PKCS#11 token, the in-memory z value remains, leaving residual secret material in the software key object.
if (ret == 0 && clear &&
((mlkemKey->flags & MLKEM_FLAG_PRIV_SET) != 0)) {
ForceZero(mlkemKey->priv, sizeof(mlkemKey->priv));
}Recommendation: Also clear the z field when clear is set:
if (ret == 0 && clear &&
((mlkemKey->flags & MLKEM_FLAG_PRIV_SET) != 0)) {
ForceZero(mlkemKey->priv, sizeof(mlkemKey->priv));
ForceZero(mlkemKey->z, sizeof(mlkemKey->z));
}This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10077
Scan targets checked: src, src-bugs, src-compliance, wolfcrypt-bugs, wolfcrypt-src
No new issues found in the changed files. ✅
Add PKCS#11 integration for ML-KEM with key generation, encapsulation and decapsulation support through the crypto callback path. Includes ML-KEM PKCS#11 constants/types, key store handling, token object lifecycle management, and ML-KEM key init helpers for private-key ID/label workflows. Align implementation details with current upstream conventions and review feedback: - internal wolfCrypt ML-KEM path only for PKCS#11 - inline ML-KEM key-type/flag checks in PKCS#11 code - proper key template formatting and enum placement - ensure TLS ML-KEM object storage behavior is compatible with PKCS#11 ephemeral-key decapsulation flow
The WOLF_CRYPTO_CB_FREE path in wc_MlKemKey_Free, wc_dilithium_free, and wc_ecc_free returned early when the crypto callback succeeded, skipping local cleanup: ForceZero on private key material, PRF/hash object frees (ML-KEM), SHAKE free and cached vector frees (ML-DSA), and mp_forcezero on the private scalar and all hardware port frees (ECC). Any non-PKCS#11 callback returning 0 would silently leave key material in memory. The PKCS#11 backend worked around this by returning CRYPTOCB_UNAVAILABLE on success to force the fallthrough — a fragile contract that is not part of the documented callback interface. Fix by always continuing to software cleanup after invoking the callback: - ML-KEM / ECC (int-returning): translate CRYPTOCB_UNAVAILABLE to 0, surface real HSM errors to the caller via the return value. - ML-DSA (void): discard the callback return with (void) cast since there is no way to propagate it. Remove the CRYPTOCB_UNAVAILABLE workaround from the three PKCS#11 free dispatchers (ECC, ML-DSA, ML-KEM); they now return the real result of C_DestroyObject.
2 participants
PKCS#11: Add ML-KEM support
Adds PKCS#11 integration for ML-KEM (key generation, encapsulation, decapsulation) through the crypto callback path, using the PKCS#11 3.2
C_EncapsulateKey/C_DecapsulateKeyinterface.What's included:
CKK_ML_KEM,CKM_ML_KEM,CKM_ML_KEM_KEY_PAIR_GEN,CKA_ENCAPSULATE/CKA_DECAPSULATE,CKP_ML_KEM_{512,768,1024}C_GenerateKeyPair; public key bytes retrieved into the wolfSSL key object, private key handle kept indevCtxfor direct decapsulationwc_CryptoCb_PqcEncapsulate/Decapsulatein wc_mlkem.cwc_MlKemKey_Init_Id/wc_MlKemKey_Init_Labelfor private-key lookup by token ID or labelwc_Pkcs11StoreKeysupport forPKCS11_KEY_TYPE_MLKEMAlso fixes a pre-existing issue across ML-KEM, ML-DSA, and ECC:
The
WOLF_CRYPTO_CB_FREEpath in all three Key_Free functions returned early on callback success, silently skippingForceZeroon private key material and freeing of internal objects (PRF/hash for ML-KEM, SHAKE + cached vectors for ML-DSA, mp_forcezero + hardware port frees for ECC).The PKCS#11 backend worked around this by returning
CRYPTOCB_UNAVAILABLEon success, a fragile undocumented contract. All three functions are fixed to always run software cleanup regardless of the callback result; theCRYPTOCB_UNAVAILABLEworkaround is removed fromwc_Pkcs11_CryptoDevCb.