Add CB_FREE mode for Falcon - #11204
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11204
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 7
7 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
fef0eb4 to
07edb52
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11204
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 5
5 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
| #ifdef WOLF_CRYPTO_CB | ||
| /* Zeroing leaves devId at 0, which is a usable device id. Mark the | ||
| * key as having no device so a second free does not call out again. */ | ||
| key->devId = INVALID_DEVID; |
There was a problem hiding this comment.
🔵 [Low] wc_MlDsaKey_Free leaves key-shake.devId at 0, so a repeat free still dispatches a crypto callback · Incorrect error handling
The new reset covers only key->devId; the ForceZero at line 12187 also leaves the embedded key->shake.devId at 0, a valid device id. The unconditional wc_Shake256_Free(&key->shake) at line 12175 then calls wc_CryptoCb_Free(0, WC_ALGO_TYPE_HASH, ...) on every subsequent free, delivering a bogus hash-free for a zeroed SHAKE object to any device registered as id 0. The sibling wc_MlKemKey_Free hunk in this PR resets its embedded hash/prf devIds.
Fix: Also set key->shake.devId = INVALID_DEVID under the same WOLF_CRYPTO_CB guard, matching the ML-KEM hunk.
| #ifdef WOLF_CRYPTO_CB | ||
| /* Zeroing leaves devId at 0, which is a usable device id. Mark the | ||
| * key as having no device so a second free does not call out again. */ | ||
| key->devId = INVALID_DEVID; |
There was a problem hiding this comment.
🔵 [Low] ML-DSA free invalidates key-devId but leaves the embedded SHAKE object at devId 0 · Logic errors
ForceZero(key, sizeof(*key)) also sets key->shake.devId to 0, a usable device id, and the new key->devId = INVALID_DEVID does not cover it. A repeat free therefore still reaches wc_Shake256_Free -> wc_Sha3Free, whose devId != INVALID_DEVID test passes and dispatches a hash free-callback to device 0 with a zeroed SHAKE object. The sibling wc_MlKemKey_Free in this same PR invalidates hash.devId/prf.devId.
Fix: Also set key->shake.devId = INVALID_DEVID after the ForceZero, guarded like the existing wc_Shake256_Free call.
| falcon_cb_free_cb, &seen), 0); | ||
|
|
||
| ExpectIntEQ(wc_falcon_init_ex(&key, NULL, TEST_FALCON_CB_FREE_DEVID), 0); | ||
| wc_falcon_free(&key); |
There was a problem hiding this comment.
⚪ [Info] New Falcon free-callback test never exercises the NULL-key guard · Missing edge-case coverage on a function the PR also changed
wc_falcon_free gained a key->devId read behind its if (key != NULL) guard, but wc_falcon_free(NULL) is not called by the new test nor anywhere else in the repository, leaving that guard uncovered.
Fix: Add a wc_falcon_free(NULL); call at the start of test_falcon_cb_free.
| #ifdef WOLF_CRYPTO_CB | ||
| /* Zeroing leaves devId at 0, which is a usable device id. Mark the | ||
| * key as having no device so a second free does not call out again. */ | ||
| key->devId = INVALID_DEVID; |
There was a problem hiding this comment.
🔵 [Low] ML-DSA free-callback invalidation misses the embedded SHAKE context · Resource leaks
The new invalidation covers only key->devId; ForceZero(key, sizeof(*key)) also resets key->shake.devId to 0, so a repeat wc_MlDsaKey_Free() reaches wc_Shake256_Free(&key->shake) (line 12175) and dispatches wc_CryptoCb_Free(0, WC_ALGO_TYPE_HASH, WC_HASH_TYPE_NONE, ...) to a device registered at id 0. The sibling fix in wc_MlKemKey_Free invalidates hash.devId/prf.devId; ML-DSA does not.
Fix: Also set key->shake.devId = INVALID_DEVID after the ForceZero, matching the hash/prf handling in wc_MlKemKey_Free.
| * release what it holds. A key with no device must not, and neither must a | ||
| * second free of a key already freed: a freed key names no device. A device | ||
| * that reports an error does not stop the software cleanup. */ | ||
| int test_falcon_cb_free(void) |
There was a problem hiding this comment.
🔵 [Low] New cb-free tests skip the NULL-key branch of the free functions the PR changed · Missing edge-case coverage on a function the PR also changed
wc_falcon_free(NULL) and wc_MlKemKey_Free(NULL) appear nowhere in the tree, so the NULL guard of the two free functions this PR modifies stays unexercised, while the sibling tests already cover it for ML-DSA (test_mldsa.c:575) and SLH-DSA (test_slhdsa.c:149).
Fix: Add wc_falcon_free(NULL); to this test and ExpectIntEQ(wc_MlKemKey_Free(NULL), 0); to test_wc_mlkem_cb_free.
2 participants
Description
Add missing CryptoCb_Free for Falcon/FN-DSA. Also, add test cases for CryptoCb_Free test cases and CI for other algs.
Testing
New test cases
Checklist