From 85dd923355525864837644df365c146dc42adbda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Thu, 26 Mar 2026 15:08:33 +0100 Subject: [PATCH] cryptocb: always run software cleanup in key Free functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. 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. --- wolfcrypt/src/dilithium.c | 11 ++--------- wolfcrypt/src/ecc.c | 19 +++++++++---------- wolfcrypt/src/wc_mlkem.c | 16 +++++----------- wolfcrypt/src/wc_pkcs11.c | 19 ------------------- 4 files changed, 16 insertions(+), 49 deletions(-) diff --git a/wolfcrypt/src/dilithium.c b/wolfcrypt/src/dilithium.c index 4b556bd5bb..8c0fbc80d0 100644 --- a/wolfcrypt/src/dilithium.c +++ b/wolfcrypt/src/dilithium.c @@ -10963,22 +10963,15 @@ int wc_dilithium_get_level(dilithium_key* key, byte* level) */ void wc_dilithium_free(dilithium_key* key) { -#if defined(WOLF_CRYPTO_CB) && defined(WOLF_CRYPTO_CB_FREE) - int ret = 0; -#endif - if (key != NULL) { #if defined(WOLF_CRYPTO_CB) && defined(WOLF_CRYPTO_CB_FREE) if (key->devId != INVALID_DEVID) { - ret = wc_CryptoCb_Free(key->devId, WC_ALGO_TYPE_PK, + (void)wc_CryptoCb_Free(key->devId, WC_ALGO_TYPE_PK, WC_PK_TYPE_PQC_SIG_KEYGEN, WC_PQC_SIG_TYPE_DILITHIUM, (void*)key); - if (ret != WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE)) - return; - /* fall-through to software cleanup */ + /* always continue to software cleanup */ } - (void)ret; #endif #ifdef WOLFSSL_WC_DILITHIUM #ifndef WC_DILITHIUM_FIXED_ARRAY diff --git a/wolfcrypt/src/ecc.c b/wolfcrypt/src/ecc.c index 4752fa2630..2da1a22e6b 100644 --- a/wolfcrypt/src/ecc.c +++ b/wolfcrypt/src/ecc.c @@ -7930,23 +7930,19 @@ void wc_ecc_free_curve(ecc_set_type* curve, void* heap) WOLFSSL_ABI int wc_ecc_free(ecc_key* key) { -#if defined(WOLF_CRYPTO_CB) && defined(WOLF_CRYPTO_CB_FREE) - int ret = 0; -#endif - if (key == NULL) { return 0; } #if defined(WOLF_CRYPTO_CB) && defined(WOLF_CRYPTO_CB_FREE) if (key->devId != INVALID_DEVID) { - ret = wc_CryptoCb_Free(key->devId, WC_ALGO_TYPE_PK, + /* Best-effort HSM resource release; errors are intentionally discarded + * so that software cleanup always runs and wc_ecc_free() retains its + * ABI guarantee of returning 0 on success. */ + (void)wc_CryptoCb_Free(key->devId, WC_ALGO_TYPE_PK, WC_PK_TYPE_EC_KEYGEN, 0, key); - if (ret != WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE)) - return ret; - /* fall-through to software cleanup */ + /* always continue to software cleanup */ } - (void)ret; #endif #if defined(WOLFSSL_ECDSA_SET_K) || defined(WOLFSSL_ECDSA_SET_K_ONE_LOOP) || \ @@ -7960,6 +7956,7 @@ int wc_ecc_free(ecc_key* key) mp_free(key->sign_k); #ifndef WOLFSSL_NO_MALLOC XFREE(key->sign_k, key->heap, DYNAMIC_TYPE_ECC); + key->sign_k = NULL; #endif } #endif @@ -8025,8 +8022,10 @@ int wc_ecc_free(ecc_key* key) #endif #ifdef WOLFSSL_CUSTOM_CURVES - if (key->deallocSet && key->dp != NULL) + if (key->deallocSet && key->dp != NULL) { wc_ecc_free_curve((ecc_set_type *)(wc_ptr_t)key->dp, key->heap); + key->dp = NULL; + } #endif #ifdef WOLFSSL_CHECK_MEM_ZERO diff --git a/wolfcrypt/src/wc_mlkem.c b/wolfcrypt/src/wc_mlkem.c index 17e3af7247..a3c6b1fa83 100644 --- a/wolfcrypt/src/wc_mlkem.c +++ b/wolfcrypt/src/wc_mlkem.c @@ -391,21 +391,15 @@ int wc_MlKemKey_Init_Label(MlKemKey* key, int type, const char* label, */ int wc_MlKemKey_Free(MlKemKey* key) { -#if defined(WOLF_CRYPTO_CB) && defined(WOLF_CRYPTO_CB_FREE) - int ret = 0; -#endif - if (key != NULL) { #if defined(WOLF_CRYPTO_CB) && defined(WOLF_CRYPTO_CB_FREE) if (key->devId != INVALID_DEVID) { - ret = wc_CryptoCb_Free(key->devId, WC_ALGO_TYPE_PK, - WC_PK_TYPE_PQC_KEM_KEYGEN, WC_PQC_KEM_TYPE_KYBER, (void*)key); - if (ret != WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE)) { - return ret; - } - /* fall-through to software cleanup */ + (void)wc_CryptoCb_Free(key->devId, WC_ALGO_TYPE_PK, + WC_PK_TYPE_PQC_KEM_KEYGEN, + WC_PQC_KEM_TYPE_KYBER, + (void*)key); + /* always continue to software cleanup */ } - (void)ret; #endif /* Dispose of PRF object. */ mlkem_prf_free(&key->prf); diff --git a/wolfcrypt/src/wc_pkcs11.c b/wolfcrypt/src/wc_pkcs11.c index 99a0e5217e..36a80d2524 100644 --- a/wolfcrypt/src/wc_pkcs11.c +++ b/wolfcrypt/src/wc_pkcs11.c @@ -6582,12 +6582,6 @@ int wc_Pkcs11_CryptoDevCb(int devId, wc_CryptoInfo* info, void* ctx) (ecc_key*)info->free.obj); Pkcs11CloseSession(token, &session); } - /* Return CRYPTOCB_UNAVAILABLE so wc_ecc_free() still - * performs software cleanup. This callback only releases - * the HSM object. Conditional because wc_ecc_free returns - * int and can propagate an HSM error to the caller. */ - if (ret == 0) - ret = CRYPTOCB_UNAVAILABLE; } else #endif @@ -6601,11 +6595,6 @@ int wc_Pkcs11_CryptoDevCb(int devId, wc_CryptoInfo* info, void* ctx) (MlDsaKey*)info->free.obj); Pkcs11CloseSession(token, &session); } - /* Always return CRYPTOCB_UNAVAILABLE so wc_dilithium_free() - * performs software cleanup. This callback only releases - * the HSM object. Unconditional because wc_dilithium_free - * returns void and cannot propagate an error. */ - ret = CRYPTOCB_UNAVAILABLE; } else #endif @@ -6619,14 +6608,6 @@ int wc_Pkcs11_CryptoDevCb(int devId, wc_CryptoInfo* info, void* ctx) (MlKemKey*)info->free.obj); Pkcs11CloseSession(token, &session); } - /* Return CRYPTOCB_UNAVAILABLE so wc_MlKemKey_Free() still - * performs software cleanup. This callback only releases - * the HSM object. Conditional because wc_MlKemKey_Free - * returns int and can propagate an HSM error to the caller. - */ - if (ret == 0) { - ret = CRYPTOCB_UNAVAILABLE; - } } else #endif