Add SHE (Secure Hardware Extension) support to wolfCrypt#10009
Add SHE (Secure Hardware Extension) support to wolfCrypt#10009night1rider wants to merge 13 commits intowolfSSL:masterfrom
Conversation
39d4163 to
54b673a
Compare
bigbrett
left a comment
There was a problem hiding this comment.
Absolutely fantastic work @night1rider. A few issues to address but overall looks great.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10009
Scan targets checked: wolfcrypt-bugs, wolfcrypt-src
Findings: 1
Low (1)
Missing NULL check on id parameter in wc_SHE_Init_Id
File: wolfcrypt/src/she.c:155-157
Function: wc_SHE_Init_Id
Category: NULL pointer dereference
The function wc_SHE_Init_Id validates she == NULL and checks that len is within bounds (0 to WC_SHE_MAX_ID_LEN), but does not check whether id is NULL before calling XMEMCPY(she->id, id, (size_t)len). If a caller passes id = NULL with len > 0, this results in a NULL pointer dereference. By contrast, the sibling function wc_SHE_Init_Label correctly validates label == NULL before use. The impact is limited because this requires caller misuse and would crash immediately (no memory corruption beyond the dereference), but it is inconsistent with the defensive validation pattern used throughout the rest of the SHE API.
if (len < 0 || len > WC_SHE_MAX_ID_LEN) {
return BUFFER_E;
}
XMEMCPY(she->id, id, (size_t)len);Recommendation: Add a NULL check for id when len > 0, consistent with the pattern in wc_SHE_Init_Label: if (id == NULL && len > 0) { return BAD_FUNC_ARG; } or simply if (id == NULL) { return BAD_FUNC_ARG; } before the length check.
This review was generated automatically by Fenrir. Findings are non-blocking.
…_she.{c,h}, fix naming consistency, auto-enable CMAC/AES dependencies, add WC_SHE_SW_DEFAULT opt-inAddress PR wolfSSL#10009 review comments from bigbrett and Fenrir
…_she.{c,h}, fix naming consistency, auto-enable CMAC/AES dependencies, add WC_SHE_SW_DEFAULT opt-inAddress PR wolfSSL#10009 review comments from bigbrett and Fenrir
|
Rebased off current master |
|
Added wc_SHE_LoadKey, wc_SHE_LoadKey_Id, wc_SHE_LoadKey_Label and their verify counterparts. These wrap Init, ImportM1M2M3, GenerateM4M5, and Free into a single call for hardware crypto callback usage. Verify variants compare returned M4/M5 against expected values using ConstantCompare. All functions require a valid devId and can be compiled out with NO_WC_SHE_LOADKEY. |
|
Jenkins retest this please |
…_she.{c,h}, fix naming consistency, auto-enable CMAC/AES dependencies, add WC_SHE_SW_DEFAULT opt-inAddress PR wolfSSL#10009 review comments from bigbrett and Fenrir
|
Rebased to fix merge conflict |
…_she.{c,h}, fix naming consistency, auto-enable CMAC/AES dependencies, add WC_SHE_SW_DEFAULT opt-inAddress PR wolfSSL#10009 review comments from bigbrett and Fenrir
|
Had to rebase and force push due to merge conflict with |
|
Jenkins retest this please |
…_she.{c,h}, fix naming consistency, auto-enable CMAC/AES dependencies, add WC_SHE_SW_DEFAULT opt-inAddress PR wolfSSL#10009 review comments from bigbrett and Fenrir
…LoadKey, wc_SHE_LoadKey_Id, wc_SHE_LoadKey_Label and their verify counterparts
…_SHE_LOADKEY, fix sort order
… XSTRLEN double call, configure.ac AES-CBC guard, and add LoadKey/LoadKey_Verify test coverage
|
Rebased and fixed merge conflict of |
|
| #endif /* WOLFSSL_CMAC */ | ||
|
|
||
| #ifdef WOLFSSL_SHE | ||
| int wc_CryptoCb_SheGetUid(wc_SHE* she, byte* uid, word32 uidSz, |
There was a problem hiding this comment.
Could this be in wc_she.c?
There was a problem hiding this comment.
The existing convention in wolfSSL is that all wc_CryptoCb_* wrapper functions live in cryptocb.c. wc_CryptoCb_AesCbcEncrypt, wc_CryptoCb_Rsa, wc_CryptoCb_EccSign, wc_CryptoCb_Sha256Hash, wc_CryptoCb_Cmac, etc. live in cryptocb.c, not in aes.c / rsa.c / ecc.c / sha256.c / cmac.c.
I had SHE wrappers follow that same pattern so the callback dispatching logic stays in one file and wc_she.c does not have to pull in cryptocb.h / the dispatch plumbing.
I could look at moving them to wc_she.c if that is preferred, but it would be the first deviation from the current pattern.
| int type; | ||
| } cmac; | ||
| #endif | ||
| #ifdef WOLFSSL_SHE |
There was a problem hiding this comment.
Do we really have to customize cryptocb to add this SHE support? Are the existing ones not enough + the keyCtx that each has?
There was a problem hiding this comment.
The she union exists to carry the arguments of the public SHE API across the callback. Each SHE API function has a distinct argument list that needs to reach the callback unchanged.
wc_SHE_GenerateM1M2M3 takes {wc_SHE*, uid, uidSz, authKeyId, authKey, authKeySz, targetKeyId, newKey, newKeySz, counter, flags, m1, m1Sz, m2, m2Sz, m3, m3Sz}, and the other SHE calls each have their own shape.
The she sub structs were added to mirror each SHE API call's argument list 1:1 so the callback receives exactly what the caller passed in without any repacking.
I can restructure if you have a specific layout in mind, or suggestion for how to pass the API parameters.
| */ | ||
|
|
||
| /* | ||
| * SHE (Secure Hardware Extension) key update message generation. |
There was a problem hiding this comment.
Where is the README.md for SHE? What hardware platform does this run on? How does someone test it?
There was a problem hiding this comment.
I just put the PR for the manual chapter for SHE in wolfSSL/documentation#263, and a follow up commit that adds the doxygen for the public SHE API at 85423cd. I can add that doxygen commit into this PR if you would like it here instead of as a separate change.
This only implements the software M1 through M5 generation path (wc_SHE_Init, wc_SHE_GenerateM1M2M3, wc_SHE_GenerateM4M5, wc_SHE_VerifyM4M5, wc_SHE_Free).
To load a SHE key on a device or use ImportM1M2M3 / GetUID / GetCounter / LoadKey / LoadKey_Verify / ExportKey, pair this PR with a crypto callback (our NXP S32K3 HSE port is an example) or use wolfHSM instead. LoadKey returns BAD_FUNC_ARG on INVALID_DEVID. GetUID / GetCounter have a WC_SHE_SW_DEFAULT stub, off by default and labeled example only (dummy UID, incrementing static counter).
Nothing special is required to test. Any build with WOLFSSL_CMAC and WOLFSSL_SHE enabled runs the software path via she_test() in ./wolfcrypt/test/testwolfcrypt. Test vectors come from https://github.com/wolfSSL/wolfHSM/blob/main/test/wh_test_she.c so results line up with wolfHSM.
Two common uses: (1) provisioning / key manager server tooling, where a cloud backend generates M1-M5 to push to devices and a source side callback handles counter tracking or protocol header packing; (2) vendor extended SHE specs with custom KDF constants or header layouts.
Implements software-only SHE CMD_LOAD_KEY message generation (M1/M2/M3)
and verification message computation (M4/M5) with optional crypto
callback support for hardware offload.
New files:
verification computation, and crypto callback integration
API:
M1/M2/M3, callback optional)
Crypto callback integration:
WC_SHE_GENERATE_M1M2M3, WC_SHE_GENERATE_M4M5, WC_SHE_EXPORT_KEY
NO_WC_SHE_EXPORTKEY, NO_WC_SHE_IMPORT_M123
Build system:
and NO_* disable flag combinations
Ported from wolfHSM wh_she_crypto.c.