Add crypto callback for SHAKE - #523
padelsbach wants to merge 1 commit into
Conversation
a7b236f to
c242d8b
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #523
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 5
5 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
| } | ||
| #endif /* WOLFSSL_SHAKE128 || WOLFSSL_SHAKE256 */ | ||
|
|
||
| int whTest_Crypto_Shake(whClientContext* ctx) |
There was a problem hiding this comment.
whTest_Crypto_Shake is defined outside the SHAKE feature guard, so NO_SHAKE builds report PASS instead of SKIPPED · Weak or missing assertions
The #if defined(WOLFSSL_SHAKE128) || defined(WOLFSSL_SHAKE256) block closes at line 457, so whTest_Crypto_Shake is always compiled and provides a strong symbol that overrides the weak WH_TEST_SKIPPED stub in wh_test_list.c. The NO_SHAKE=1 variant added by this PR therefore reports the SHAKE suite as passing while running nothing. whTest_Crypto_Sha3 and whTest_Crypto_MlDsa both sit inside their feature guards.
Suggested fix: Move whTest_Crypto_Shake inside the WOLFSSL_SHAKE128 || WOLFSSL_SHAKE256 guard so the weak skip stub is linked when neither variant is built.
| } | ||
| *requestSent = false; | ||
|
|
||
| if (sha->i >= v->blockSize) { |
There was a problem hiding this comment.
SHAKE client helpers omit the Keccak-mode rejection their SHA3 twins perform · Copy-paste errors
_ShakeUpdateRequest and _ShakeFinalRequest have no equivalent of _Sha3RejectKeccak (called at lines 9189 and 9315 of the SHA3 twins). The wire format carries only resumeState.s, and the server re-inits with standard SHAKE padding, so the public wh_Client_Shake128/256* entry points silently return wrong output for a WC_HASH_SHA3_KECCAK256-flagged context. The new _ShakeTestKeccakFlag only covers the cryptocb path, which does guard.
Suggested fix: Call the existing _Sha3RejectKeccak(sha) (it takes a wc_Sha3*, which wc_Shake aliases) at the top of _ShakeUpdateRequest and _ShakeFinalRequest, and extend _ShakeTestBadArgs to cover it.
| uint8_t* inlineData; | ||
| uint8_t* dataPtr; | ||
|
|
||
| if (ctx == NULL || sha == NULL || outSz == 0) { |
There was a problem hiding this comment.
SHAKE client request helpers omit the Keccak-flag rejection their SHA3 siblings apply · Cryptographic operation flaws
_HandleShake initialises a fresh wc_Shake on the server, so WC_HASH_SHA3_KECCAK256 set via wc_Sha3_SetFlags is never conveyed. _Sha3UpdateRequest/_Sha3FinalRequest guard this with _Sha3RejectKeccak; _ShakeUpdateRequest/_ShakeFinalRequest do not, and the only guard added by the PR lives in wh_Client_CryptoCbStd. Direct callers of the public wh_Client_Shake* API silently receive digests computed with the wrong padding domain.
Suggested fix: Call _Sha3RejectKeccak(sha) at the top of _ShakeUpdateRequest and _ShakeFinalRequest, as the SHA3 request helpers do.
| uint32_t inLen, uint8_t* out, uint32_t outSz) | ||
| { | ||
| int ret = WH_ERROR_OK; | ||
| _ShakeSavedState saved; |
There was a problem hiding this comment.
SHAKE rollback buffers retain sponge state and buffered input on the client stack · Missing ForceZero
_ShakeSavedState saved holds the full 200-byte Keccak sponge state plus up to 168 bytes of unabsorbed message, and _ShakeUpdateRequest keeps a savedT[168] copy; neither is scrubbed on any exit path. SHAKE is used as an XOF over secret seeds, so this leaves derivable secret material in the client's stack frame after return. Adjacent to known finding #8510, which covers the SHA3 copies (_Sha3UpdateRequest, _Sha3Oneshot, and the DMA variants) — these are new functions introduced by this PR and are not fixed by that patch.
Related known finding #8510 (similar but distinct): Operation/root cause: unzeroed stack copies of Keccak sponge state and partial-block buffer (_ShakeSavedState/savedT) left after client-side rollback, directly analogous to #8510's _Sha3SavedState/savedT residue in the SHA3 twins — same missing-ForceZero pattern and same architectural cause (rollback snapshots for cryptocb fallback). Function/path differs: these are new SHAKE-specific helpers (_ShakeOneshot, _ShakeUpdateRequest) introduced by this PR, distinct code locations from the SHA3 functi
Suggested fix: Scrub saved before returning from _ShakeOneshot and savedT before returning from _ShakeUpdateRequest using wc_ForceZero.
| } | ||
| #endif /* WOLFSSL_SHAKE128 || WOLFSSL_SHAKE256 */ | ||
| (void)ctx; | ||
| return 0; |
There was a problem hiding this comment.
whTest_Crypto_Shake reports PASS instead of SKIPPED when SHAKE is disabled · Weak or missing assertions
The function body is guarded by #if defined(WOLFSSL_SHAKE128) || defined(WOLFSSL_SHAKE256) but the definition itself is unconditional, so with neither variant enabled it executes nothing and returns 0. The NO_SHAKE=1 / SHAKE128_ONLY / SHAKE256_ONLY build options added by this PR make that the normal case, and those CI jobs will report the test as passing.
Suggested fix: Wrap the whTest_Crypto_Shake definition in the SHAKE #if so the weak stub supplies the WH_TEST_SKIPPED result, as whTest_Crypto_Sha3 does.
3 participants
Follows patterns from SHA3