Add crypto callback mode for SLH-DSA - #512
padelsbach wants to merge 8 commits into
Conversation
aed3238 to
7fe4069
Compare
64514ae to
a9eb8a9
Compare
a9eb8a9 to
43e9f7f
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #512
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 11
11 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #512
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 7
7 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
Went through the SLH-DSA callback support. Big addition, and the structure mirrors the ML-DSA paths closely enough to follow easily.
Comments inline. The ones I would most like to see addressed are the DMA READ_POST after a denied READ_PRE in _HandleSlhDsaVerifyDma, the unvalidated seedSz / 3 in both keygen handlers, and the seed left resident in the comm packet. The rest are smaller consistency points against the ML-DSA and ML-KEM siblings, and the last few are nits you can take or leave.
Nothing here is a blocker.
| key_id = *inout_keyId; | ||
| } | ||
|
|
||
| ret = wh_Crypto_SlhDsaSerializeKeyDer(key, sizeof(buffer), buffer, |
There was a problem hiding this comment.
The wc_ForceZero(buffer, sizeof(buffer)) here only covers half the exposure. wh_Client_KeyCacheRequest_ex memcpys the same private-key DER into the shared transport buffer from wh_CommClient_GetDataPtr, and nothing clears that. _SlhDsaMakeKey in this PR does handle it, with wc_ForceZero(dataPtr, WOLFHSM_CFG_COMM_DATA_LEN), so the two new paths disagree.
It is not reliably overwritten by what follows either: the DER runs to about 320 bytes and a following CheckPrivKey writes roughly 88, so the tail survives in a buffer the server side can see under a shared-memory transport. wh_Client_SlhDsaImportKeyDma has the same gap through wh_Client_KeyCacheDma.
There was a problem hiding this comment.
In wh_Client_SlhDsaImportKey() and the DMA equivalent, the CommBuffer still has the private key DER without a ForceZero.
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
Two small follow-ups from the last round - neither is blocking, and the rest of what I raised looks good now.
|
|
||
| if (!_IsMlKemLevelSupported((int)req.level)) { | ||
| if (seedSz > (uint32_t)(inSize - | ||
| sizeof(whMessageCrypto_SlhDsaKeyGenRequest))) { | ||
| return WH_ERROR_BADARGS; |
There was a problem hiding this comment.
The new wc_ForceZero(seed, seedSz) covers the unsupported-param and wc_SlhDsaKey_Init failures as well as the post-keygen path, but this bounds rejection returns above all of it, so a rejected request still leaves the client's seed bytes in the comm buffer.
seedSz is the value being rejected here, so it cannot be the length to scrub - it would run past the request. inSize - sizeof(whMessageCrypto_SlhDsaKeyGenRequest) is what is actually there.
| pub->devId = WH_CLIENT_DEVID(ctx); | ||
| } | ||
| else if (committed && !WH_KEYID_ISERASED(*inout_key_id)) { | ||
| /* The server committed a key but the best-effort export returned no |
There was a problem hiding this comment.
This block and its twin in wh_Client_SlhDsaMakeCacheKeyDma (line 12337) both run to eight lines, and most of it is arguing the alternative that was not taken. That shape has been getting marked as slop in review lately, and I only asked for one line on the DMA side.
One sentence naming the constraint is enough, e.g. /* Gated on the commit latch: the response-frame check also reports ABORTED, before any key id has been read. */ - the reasoning behind it reads better in the commit message, where it will not go stale.
4 participants
No description provided.