Skip to content

Commit ac418ab

Browse files
committed
slhdsa: round-3 review fixes
1. wc_SlhDsaKey_Free: make double-free / cryptocb-recursive-free safe. - NULL out key->params at the end so a second Free is a no-op. - Lift the cryptocb-free dispatch above the SW-cleanup gate and re-check params!=NULL before the SHAKE/SHA2 teardown. The test pattern in myCryptoDevCb (mirroring dilithium) recursively calls wc_SlhDsaKey_Free from the device callback; the inner call now does the SW cleanup once and zeroes params, the outer skips, and wc_Shake256_Free no longer relies on idempotence to survive a re-Free. 2. Drop the stale 'with-digest' doc-block above slhdsakey_signhash_external left over from the round-2 helper extraction (the function takes a raw msg + hashType, not a precomputed digest). 3. wc_SlhDsaKey_Init_id: document the BAD_FUNC_ARG return for (id == NULL, len > 0). 4. wc_SlhDsaKey_Init_label: document the XSTRLEN-based length contract and embedded-NUL truncation. 5. wc_SlhDsaKey_MakeKey cryptocb hook: comment the per-algorithm meaning of pqc_sig_kg.size. ML-DSA uses NIST level, SLH-DSA uses enum SlhDsaParam (a single security level maps to two parameter sets, S vs F, that callers must distinguish). Devices keying off WC_PQC_SIG_TYPE_SLHDSA must interpret size as enum SlhDsaParam. 6. slhdsa_id_label_test: zero the stack key up-front so rejection-path tests don't read uninitialized fields if a future Init refactor changes the moment of zeroization.
1 parent 167a8bd commit ac418ab

2 files changed

Lines changed: 34 additions & 15 deletions

File tree

wolfcrypt/src/wc_slhdsa.c

Lines changed: 29 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6613,7 +6613,7 @@ int wc_SlhDsaKey_Init(SlhDsaKey* key, enum SlhDsaParam param, void* heap,
66136613
* @param [in] heap Dynamic memory allocation hint.
66146614
* @param [in] devId Device Id.
66156615
* @return 0 on success.
6616-
* @return BAD_FUNC_ARG when key is NULL.
6616+
* @return BAD_FUNC_ARG when key is NULL or when id is NULL with len > 0.
66176617
* @return BUFFER_E when len is negative or larger than SLHDSA_MAX_ID_LEN.
66186618
*/
66196619
int wc_SlhDsaKey_Init_id(SlhDsaKey* key, enum SlhDsaParam param,
@@ -6647,7 +6647,9 @@ int wc_SlhDsaKey_Init_id(SlhDsaKey* key, enum SlhDsaParam param,
66476647

66486648
/* Initialize an SLH-DSA key with a device key label.
66496649
*
6650-
* Mirrors wc_dilithium_init_label.
6650+
* Mirrors wc_dilithium_init_label. Label length is taken via XSTRLEN, so
6651+
* the label must be NUL-terminated and an embedded NUL terminates the
6652+
* effective label; bytes past the first NUL are ignored.
66516653
*
66526654
* @param [in] key SLH-DSA key.
66536655
* @param [in] param SLH-DSA parameter set to use.
@@ -6700,16 +6702,20 @@ void wc_SlhDsaKey_Free(SlhDsaKey* key)
67006702
* is a reliable signal that devId is meaningful. Releasing the
67016703
* device-side handle on a never-initialized key (devId == garbage from
67026704
* stack) would dispatch to whatever device happens to match. */
6703-
if (key->params != NULL) {
67046705
#if defined(WOLF_CRYPTO_CB) && defined(WOLF_CRYPTO_CB_FREE)
6705-
if (key->devId != INVALID_DEVID) {
6706-
(void)wc_CryptoCb_Free(key->devId, WC_ALGO_TYPE_PK,
6707-
WC_PK_TYPE_PQC_SIG_KEYGEN,
6708-
WC_PQC_SIG_TYPE_SLHDSA,
6709-
(void*)key);
6710-
/* always continue to software cleanup */
6711-
}
6706+
if ((key->params != NULL) && (key->devId != INVALID_DEVID)) {
6707+
(void)wc_CryptoCb_Free(key->devId, WC_ALGO_TYPE_PK,
6708+
WC_PK_TYPE_PQC_SIG_KEYGEN,
6709+
WC_PQC_SIG_TYPE_SLHDSA,
6710+
(void*)key);
6711+
/* The device callback may itself recursively call wc_SlhDsaKey_Free
6712+
* (this is the test pattern in myCryptoDevCb, mirroring dilithium).
6713+
* The recursive Free runs the SW cleanup once and zeroes params;
6714+
* checking params!=NULL again below skips a double teardown. */
6715+
}
67126716
#endif
6717+
6718+
if (key->params != NULL) {
67136719
/* Ensure the private key data is zeroized. */
67146720
ForceZero(key->sk, (size_t)key->params->n * 2);
67156721
#ifdef WOLFSSL_SLHDSA_SHA2
@@ -6757,6 +6763,12 @@ void wc_SlhDsaKey_Free(SlhDsaKey* key)
67576763
key->devCtx = NULL;
67586764
key->devId = INVALID_DEVID;
67596765
#endif
6766+
/* Clearing params last makes a second wc_SlhDsaKey_Free a no-op (the
6767+
* params!=NULL gate above will skip the SHAKE/SHA2 teardown that has
6768+
* already run). It also means the cryptocb-driven recursive Free path
6769+
* does the SW cleanup exactly once: the inner call zeroes params, the
6770+
* outer call sees params==NULL and skips. */
6771+
key->params = NULL;
67606772
}
67616773

67626774
/* Set the HashAddress based on message digest data.
@@ -6875,6 +6887,13 @@ int wc_SlhDsaKey_MakeKey(SlhDsaKey* key, WC_RNG* rng)
68756887
if (key->devId != INVALID_DEVID)
68766888
#endif
68776889
{
6890+
/* The cryptocb pqc_sig_kg.size field is overloaded per
6891+
* algorithm: ML-DSA passes the NIST level (2/3/5), SLH-DSA
6892+
* passes the parameter-set enum (enum SlhDsaParam) since a
6893+
* single security level (n=16/24/32) corresponds to two
6894+
* parameter sets ("S" / "F") that callers must distinguish.
6895+
* Devices keying off WC_PQC_SIG_TYPE_SLHDSA must interpret
6896+
* size as enum SlhDsaParam, not as an ML-DSA-style level. */
68786897
ret = wc_CryptoCb_MakePqcSignatureKey(rng,
68796898
WC_PQC_SIG_TYPE_SLHDSA, (int)key->params->param, key);
68806899
if (ret != WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE))
@@ -7979,11 +7998,6 @@ static int slhdsakey_prehash_msg(const byte* msg, word32 msgSz,
79797998
* @return MEMORY_E on dynamic memory allocation failure.
79807999
* @return SHAKE-256 error return code on digest failure.
79818000
*/
7982-
/* Pre-hash SLH-DSA sign given an already-computed digest+OID.
7983-
*
7984-
* Caller must have validated key/ctx/sig and pre-hashed msg via
7985-
* slhdsakey_prehash_msg.
7986-
*/
79878001
static int slhdsakey_signhash_external(SlhDsaKey* key, const byte* ctx,
79888002
byte ctxSz, const byte* msg, word32 msgSz, enum wc_HashType hashType,
79898003
byte* sig, word32* sigSz, byte* addRnd)

wolfcrypt/test/test.c

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54527,6 +54527,11 @@ static wc_test_ret_t slhdsa_id_label_test(void)
5452754527
SLHDSA_SHAKE128F;
5452854528
#endif
5452954529

54530+
/* Zero the stack key so rejection-path tests below don't read
54531+
* uninitialized fields if a future Init refactor inspects key state
54532+
* before zeroizing it. */
54533+
XMEMSET(&key, 0, sizeof(key));
54534+
5453054535
/* NULL key rejected. */
5453154536
ret = wc_SlhDsaKey_Init_id(NULL, param, id, (int)sizeof(id), HEAP_HINT,
5453254537
INVALID_DEVID);

0 commit comments

Comments
 (0)