Add support for LMS and XMSS#352
Conversation
padelsbach
force-pushed
the
lms-xmss
branch
2 times, most recently
from
e01c4e8 to
322c2ba
Compare
padelsbach
force-pushed
the
lms-xmss
branch
2 times, most recently
from
d524cee to
c9dad02
Compare
|
wolfSSL/wolfssl#10488 is required for CI to pass |
padelsbach
force-pushed
the
lms-xmss
branch
2 times, most recently
from
d104594 to
2c5db59
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #352
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #352
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #352
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #352
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
No new issues found in the changed files. ✅
|
@Frauschi FYI merge conflicts are irrelevant to the review content so you can ignore for now |
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
Some more general (but critical) issues:
- Key export is allowed. This enables exporting the whole key state including the private key state (which is implementation-dependent, so a user can only use it with wolfCrypt btw). This should imho entirely prohibited for a stateful scheme. Hence, I'd force set
WH_NVM_FLAGS_NONEXPORTABLEfor LMS and XMSS keys during key generation. - Public key export is not routed. Although key generation exports the public key with the response, there is no way currently to export the public key of an already present key in the keystore. That should be wired up properly (and that would also not conflict with the non-exportable flag of finding 1 above, as this is safely ignored for public keys already).
- Import of a private key is allowed. Hence, a user can either craft a state buffer himself or re-import a previously exported private key state (see finding 1), enabling a “roll-back” of the private key state. That completely breaks the security of the stateful scheme.
- A public-key only import function is missing. A big use case for LMS and XMSS is firmware signature verification in a wolfBoot scenario. For that, a pure public key lives in the keystore to be used to verify the signatures. However, there is no way to import such a public key at the moment. I think the current implementation doesn't even support public-only keys properly in the blob format. So this probably needs some rework for that. But I think this is an essential functionality block for LMS and XMSS.
- The whole "key state in NVM security" aspects build upon the assumption that the underlying flash backend (mainly the two functions
cb->Programandcb->Verify) iswrite-throughto the actual storage and does not incorporate any caching logic. There is currently no way we can enforce or check this programmatically, but this should be worth a quick note in the documentation (mainly relevant for port maintainers). - In some comments, a documentation is mentioned, which is currently absent from the PR. That should be added for both LMS and XMSS.
| const byte* msg, word32 msgSz, int* res, | ||
| LmsKey* key); | ||
|
|
||
| /* Query remaining signatures on an HSM-resident LMS key. */ |
There was a problem hiding this comment.
Our internal wolfCrypt API does not support querying the actual number of remaining signatures, only whether there are any remaining signatures left. Hence, the number obtained here reflects this booleaness. This should be reflected in the comment.
| } while (ret == WH_ERROR_NOTREADY); | ||
| } | ||
|
|
||
| (void)wh_Client_DmaProcessClientAddress( |
There was a problem hiding this comment.
I think discarding the return value of a WRITE_POST post-processing step is not correct and should actually be passed to the user (also see the recent #403).
Within a WRITE_POST post-process, there could be a MEMCPY from the DMA buffer to the actual user buffer (when the passed buffer from the user is not in the correct DMA memory region already). Hence, in case the post-processing fails, we cannot be 100% sure that the user buffer actually contains the right data (in contrast to READ post-processing, where we are only talking about cleaning-up work).
This goes for all four of the WRITE_POST calls in this file.
| { | ||
| uint16_t group = WH_MESSAGE_GROUP_CRYPTO_DMA; | ||
| uint16_t action = WC_ALGO_TYPE_PK; | ||
| uint16_t req_len = |
There was a problem hiding this comment.
Nit: all other functions do a bounds-check req_len > WOLFHSM_CFG_COMM_DATA_LEN except for the LMS and XMSS SigsLeft() methods. Will probably never be a problem as the request is so small, but maybe adding the check for consistency might be beneficial (AI flaged this actually).
| uint32_t lmsHeight; | ||
| uint32_t lmsWinternitz; | ||
| uint8_t label[WH_NVM_LABEL_LEN]; | ||
| char xmssParamStr[32]; |
There was a problem hiding this comment.
Nit: Maybe not worth the effort, but can we place the xmssParamStr[] array and the three LMS parameters (lmsLevels, lmsHeight, lmsWinternitz) into a union? As mentioned in the introductory comment above, the discrimination is well defined.
I mean we are currently talking about 12 bytes of saving, but maybe when additional algorithms are added in the future, this can grow further?
There was a problem hiding this comment.
TBH I highly doubt presently that there will be any new stateful signature algorithm any time in the nearer future, so this is purely hypothetical for now.
| * call (e.g. via wc_LmsKey_SetParameters). On success the key's devCtx | ||
| * carries the server-side keyId. | ||
| * | ||
| * If flags include WH_NVM_FLAGS_EPHEMERAL, the server returns the public key |
There was a problem hiding this comment.
I really love that the public key is now directly exported back to the client in the keyGen round-trip (that lack has been the main purpose of the ExportPublicKey API), but why is this only the case for EPHEMERAL keys?
So tbh, I don't see any real-life use case where a user wants an ephemeral stateful key pair in the first place. But independent of that, I see the role of the ephemeral flag solely in the distinction between saving the key in the non-volatile keystore or having it only in RAM (is that assumption correct for overall wolfHSM?). In both cases, one probably needs the public key on the client side to do something useful. So in the non-ephemeral case, the user has to use the ExportPublicKey() API to get the public key now.
TL;DR: I would export the public key in both situations, as the logic is already there anyway.
| p = buffer + WH_CRYPTO_STATEFUL_SIG_HEADER_SZ + paramLen; | ||
| memcpy(key->pub, p, pubLen); | ||
| p += pubLen; | ||
| memcpy(key->priv_raw, p, privLen); |
There was a problem hiding this comment.
As noted in wh_server_crypto.c already, this copy is actually unnecessary and could be removed (XMSS does not have it at all, too).
| if (WH_KEYID_ISERASED(keyId)) { | ||
| return WH_ERROR_BADARGS; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think we also have to lock the NVM here as well (like in the Sign function), as wh_Server_KeystoreFreshenKey() may conflict with a concurrent NVM operation (not sure about this tbh). Same goes for XMSS as well.
| if (WH_KEYID_ISERASED(keyId)) { | ||
| return WH_ERROR_BADARGS; | ||
| } | ||
|
|
| if (WH_KEYID_ISERASED(keyId)) { | ||
| return WH_ERROR_BADARGS; | ||
| } | ||
|
|
| if (WH_KEYID_ISERASED(keyId)) { | ||
| return WH_ERROR_BADARGS; | ||
| } | ||
|
|
5 participants
Requires wolfSSL/wolfssl#10380 to be merged first (done).
Adds support for "stateful" PQC using crypto callbacks added to wolfssl.