Skip to content

Commit bbc5a63

Browse files
padelsbachpadelsbach
committed
PR feedback: add checks for EPHEMERAL, missing zeroizing, committed flag for cleanup, zero check on cleanup
1 parent 84ed610 commit bbc5a63

4 files changed

Lines changed: 220 additions & 37 deletions

File tree

‎src/wh_client_crypto.c‎

Lines changed: 41 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -11375,6 +11375,7 @@ int wh_Client_SlhDsaImportKey(whClientContext* ctx, SlhDsaKey* key,
1137511375
whKeyId key_id = WH_KEYID_ERASED;
1137611376
byte buffer[WH_CRYPTO_SLHDSA_MAX_KEY_DER_SIZE];
1137711377
uint16_t buffer_len = 0;
11378+
uint8_t* dataPtr = NULL;
1137811379

1137911380
if ((ctx == NULL) || (key == NULL) ||
1138011381
((label_len != 0) && (label == NULL))) {
@@ -11394,6 +11395,11 @@ int wh_Client_SlhDsaImportKey(whClientContext* ctx, SlhDsaKey* key,
1139411395
if ((ret == WH_ERROR_OK) && (inout_keyId != NULL)) {
1139511396
*inout_keyId = key_id;
1139611397
}
11398+
/* The request copied the DER into the shared packet buffer */
11399+
dataPtr = (uint8_t*)wh_CommClient_GetDataPtr(ctx->comm);
11400+
if (dataPtr != NULL) {
11401+
wc_ForceZero(dataPtr, WOLFHSM_CFG_COMM_DATA_LEN);
11402+
}
1139711403
}
1139811404

1139911405
WH_DEBUG_CLIENT_VERBOSE("label:%.*s ret:%d keyid:%u\n", label_len, label,
@@ -11608,14 +11614,30 @@ int wh_Client_SlhDsaMakeCacheKey(whClientContext* ctx, int param,
1160811614
whKeyId* inout_key_id, whNvmFlags flags,
1160911615
uint16_t label_len, uint8_t* label)
1161011616
{
11611-
if (inout_key_id == NULL) {
11617+
if ((inout_key_id == NULL) || (flags & WH_NVM_FLAGS_EPHEMERAL)) {
1161211618
return WH_ERROR_BADARGS;
1161311619
}
1161411620

1161511621
return _SlhDsaMakeKey(ctx, param, NULL, 0, inout_key_id, flags, label_len,
1161611622
label, NULL, NULL);
1161711623
}
1161811624

11625+
/* Make pub a handle to the cached key, or evict a key the server cached */
11626+
static int _SlhDsaCacheKeyFinish(whClientContext* ctx, int ret, int committed,
11627+
whKeyId* inout_key_id, SlhDsaKey* pub)
11628+
{
11629+
if (ret >= 0) {
11630+
/* Set again: the public-key deserialize re-inits pub and clears it */
11631+
wh_Client_SlhDsaSetKeyId(pub, *inout_key_id);
11632+
pub->devId = WH_CLIENT_DEVID(ctx);
11633+
}
11634+
else if (committed && !WH_KEYID_ISERASED(*inout_key_id)) {
11635+
(void)wh_Client_KeyEvict(ctx, *inout_key_id);
11636+
*inout_key_id = WH_KEYID_ERASED;
11637+
}
11638+
return ret;
11639+
}
11640+
1161911641
int wh_Client_SlhDsaMakeCacheKeyAndExportPublic(
1162011642
whClientContext* ctx, int param, whKeyId* inout_key_id, whNvmFlags flags,
1162111643
uint16_t label_len, const uint8_t* label, SlhDsaKey* pub)
@@ -11634,21 +11656,7 @@ int wh_Client_SlhDsaMakeCacheKeyAndExportPublic(
1163411656

1163511657
ret = _SlhDsaMakeKey(ctx, param, NULL, 0, inout_key_id, flags, label_len,
1163611658
label, pub, &committed);
11637-
if (ret >= 0) {
11638-
/* Stamp the cached keyId and the client's HSM devId so pub is
11639-
* immediately usable as a handle to the cached private key. The keyId
11640-
* is set here as well because the public-key deserialize re-inits
11641-
* pub and clears it. */
11642-
wh_Client_SlhDsaSetKeyId(pub, *inout_key_id);
11643-
pub->devId = WH_CLIENT_DEVID(ctx);
11644-
}
11645-
else if (committed && !WH_KEYID_ISERASED(*inout_key_id)) {
11646-
/* Gated on the commit latch: the response-frame check also reports
11647-
* ABORTED, before any key id has been read. */
11648-
(void)wh_Client_KeyEvict(ctx, *inout_key_id);
11649-
*inout_key_id = WH_KEYID_ERASED;
11650-
}
11651-
return ret;
11659+
return _SlhDsaCacheKeyFinish(ctx, ret, committed, inout_key_id, pub);
1165211660
}
1165311661

1165411662
int wh_Client_SlhDsaMakeExportKey(whClientContext* ctx, int param,
@@ -11680,7 +11688,8 @@ int wh_Client_SlhDsaMakeCacheKeyFromSeed(whClientContext* ctx, int param,
1168011688
whNvmFlags flags, uint16_t label_len,
1168111689
uint8_t* label)
1168211690
{
11683-
if ((inout_key_id == NULL) || (seed == NULL) || (seedSz == 0)) {
11691+
if ((inout_key_id == NULL) || (seed == NULL) || (seedSz == 0) ||
11692+
(flags & WH_NVM_FLAGS_EPHEMERAL)) {
1168411693
return WH_ERROR_BADARGS;
1168511694
}
1168611695

@@ -12195,7 +12204,8 @@ int wh_Client_SlhDsaExportPublicKeyDma(whClientContext* ctx, whKeyId keyId,
1219512204
static int _SlhDsaMakeKeyDma(whClientContext* ctx, int param, const byte* seed,
1219612205
word32 seedSz, whKeyId* inout_key_id,
1219712206
whNvmFlags flags, uint16_t label_len,
12198-
const uint8_t* label, SlhDsaKey* key)
12207+
const uint8_t* label, SlhDsaKey* key,
12208+
int* out_committed)
1219912209
{
1220012210
int ret = WH_ERROR_OK;
1220112211
whKeyId key_id = WH_KEYID_ERASED;
@@ -12215,6 +12225,10 @@ static int _SlhDsaMakeKeyDma(whClientContext* ctx, int param, const byte* seed,
1221512225
uint16_t group;
1221612226
uint16_t action;
1221712227

12228+
if (out_committed != NULL) {
12229+
*out_committed = 0;
12230+
}
12231+
1221812232
if ((ctx == NULL) || ((seed == NULL) && (seedSz > 0))) {
1221912233
return WH_ERROR_BADARGS;
1222012234
}
@@ -12331,6 +12345,9 @@ static int _SlhDsaMakeKeyDma(whClientContext* ctx, int param, const byte* seed,
1233112345
if (ret >= 0) {
1233212346
/* Key is cached on server or is ephemeral */
1233312347
key_id = (whKeyId)(res->keyId);
12348+
if (out_committed != NULL) {
12349+
*out_committed = 1;
12350+
}
1233412351

1233512352
/* Update output variable if requested */
1233612353
if (inout_key_id != NULL) {
@@ -12380,7 +12397,7 @@ int wh_Client_SlhDsaMakeExportKeyDma(whClientContext* ctx, int param,
1238012397
}
1238112398

1238212399
return _SlhDsaMakeKeyDma(ctx, param, NULL, 0, NULL, WH_NVM_FLAGS_EPHEMERAL,
12383-
0, NULL, key);
12400+
0, NULL, key, NULL);
1238412401
}
1238512402

1238612403
int wh_Client_SlhDsaMakeExportKeyFromSeedDma(whClientContext* ctx, int param,
@@ -12392,16 +12409,16 @@ int wh_Client_SlhDsaMakeExportKeyFromSeedDma(whClientContext* ctx, int param,
1239212409
}
1239312410

1239412411
return _SlhDsaMakeKeyDma(ctx, param, seed, seedSz, NULL,
12395-
WH_NVM_FLAGS_EPHEMERAL, 0, NULL, key);
12412+
WH_NVM_FLAGS_EPHEMERAL, 0, NULL, key, NULL);
1239612413
}
1239712414

1239812415
int wh_Client_SlhDsaMakeCacheKeyDma(whClientContext* ctx, int param,
1239912416
whKeyId* inout_key_id, whNvmFlags flags,
1240012417
uint16_t label_len, const uint8_t* label,
1240112418
SlhDsaKey* pub)
1240212419
{
12403-
int ret;
12404-
whKeyId in_keyId;
12420+
int ret;
12421+
int committed = 0;
1240512422

1240612423
if ((ctx == NULL) || (inout_key_id == NULL) || (pub == NULL)) {
1240712424
return WH_ERROR_BADARGS;
@@ -12412,20 +12429,9 @@ int wh_Client_SlhDsaMakeCacheKeyDma(whClientContext* ctx, int param,
1241212429
return WH_ERROR_BADARGS;
1241312430
}
1241412431

12415-
in_keyId = *inout_key_id;
1241612432
ret = _SlhDsaMakeKeyDma(ctx, param, NULL, 0, inout_key_id, flags, label_len,
12417-
label, pub);
12418-
if (ret >= 0) {
12419-
wh_Client_SlhDsaSetKeyId(pub, *inout_key_id);
12420-
pub->devId = WH_CLIENT_DEVID(ctx);
12421-
}
12422-
else if (WH_KEYID_ISERASED(in_keyId) && !WH_KEYID_ISERASED(*inout_key_id)) {
12423-
/* A changed key id is itself proof of commit: it moves off ERASED
12424-
* only once the response has been read. */
12425-
(void)wh_Client_KeyEvict(ctx, *inout_key_id);
12426-
*inout_key_id = WH_KEYID_ERASED;
12427-
}
12428-
return ret;
12433+
label, pub, &committed);
12434+
return _SlhDsaCacheKeyFinish(ctx, ret, committed, inout_key_id, pub);
1242912435
}
1243012436

1243112437
int wh_Client_SlhDsaSignDma(whClientContext* ctx, const byte* in,

‎src/wh_crypto.c‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -424,6 +424,9 @@ int wh_Crypto_SlhDsaSerializeKeyDer(SlhDsaKey* key, uint16_t max_size,
424424
/* Clear buffer to avoid leaking partial key material on error */
425425
wc_ForceZero(buffer, max_size);
426426
*out_size = 0;
427+
if (ret == 0) {
428+
ret = WH_ERROR_ABORTED;
429+
}
427430
}
428431
return ret;
429432
}

‎test-refactor/client-server/wh_test_crypto_slhdsa.c‎

Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1588,6 +1588,100 @@ static int _whTest_CryptoSlhDsaSeededKeyGen(whClientContext* ctx)
15881588
wc_SlhDsaKey_Free(key);
15891589
return ret;
15901590
}
1591+
1592+
/* EPHEMERAL returns the private key, so cache-key wrappers must refuse it */
1593+
static int _whTest_CryptoSlhDsaCacheKeyEphemeral(whClientContext* ctx)
1594+
{
1595+
int devId = WH_CLIENT_DEVID(ctx);
1596+
int ret;
1597+
whKeyId keyId = WH_KEYID_ERASED;
1598+
SlhDsaKey pub[1];
1599+
const int param = WH_TEST_SLHDSA_KAT_PARAM;
1600+
const whNvmFlags flags = WH_NVM_FLAGS_EPHEMERAL;
1601+
1602+
ret = wc_SlhDsaKey_Init(pub, param, NULL, devId);
1603+
if (ret != 0) {
1604+
return ret;
1605+
}
1606+
1607+
if (wh_Client_SlhDsaMakeCacheKey(ctx, param, &keyId, flags, 0, NULL) !=
1608+
WH_ERROR_BADARGS) {
1609+
WH_ERROR_PRINT("SLH-DSA cache keygen accepted EPHEMERAL\n");
1610+
ret = WH_TEST_FAIL;
1611+
}
1612+
/* A valid seed, so the flag is the only reason to refuse */
1613+
if (wh_Client_SlhDsaMakeCacheKeyFromSeed(
1614+
ctx, param, whTestSlhDsaKeyGenSeed, sizeof(whTestSlhDsaKeyGenSeed),
1615+
&keyId, flags, 0, NULL) != WH_ERROR_BADARGS) {
1616+
WH_ERROR_PRINT("SLH-DSA seeded cache keygen accepted EPHEMERAL\n");
1617+
ret = WH_TEST_FAIL;
1618+
}
1619+
if (wh_Client_SlhDsaMakeCacheKeyAndExportPublic(
1620+
ctx, param, &keyId, flags, 0, NULL, pub) != WH_ERROR_BADARGS) {
1621+
WH_ERROR_PRINT("SLH-DSA cache and export accepted EPHEMERAL\n");
1622+
ret = WH_TEST_FAIL;
1623+
}
1624+
#ifdef WOLFHSM_CFG_DMA
1625+
if (wh_Client_SlhDsaMakeCacheKeyDma(ctx, param, &keyId, flags, 0, NULL,
1626+
pub) != WH_ERROR_BADARGS) {
1627+
WH_ERROR_PRINT("SLH-DSA DMA cache keygen accepted EPHEMERAL\n");
1628+
ret = WH_TEST_FAIL;
1629+
}
1630+
#endif /* WOLFHSM_CFG_DMA */
1631+
1632+
if (ret == 0) {
1633+
WH_TEST_PRINT("SLH-DSA CACHE EPHEMERAL DEVID=0x%X SUCCESS\n", devId);
1634+
}
1635+
1636+
wc_SlhDsaKey_Free(pub);
1637+
return ret;
1638+
}
1639+
1640+
/* Importing a private key must not leave its DER in the comm buffer */
1641+
static int _whTest_CryptoSlhDsaImportClearsComm(whClientContext* ctx)
1642+
{
1643+
int devId = WH_CLIENT_DEVID(ctx);
1644+
int ret;
1645+
whKeyId keyId = WH_KEYID_ERASED;
1646+
SlhDsaKey key[1];
1647+
const uint8_t* comm = (const uint8_t*)wh_CommClient_GetDataPtr(ctx->comm);
1648+
/* The seed starts with SK.seed, which the private key DER contains */
1649+
const size_t skSz = sizeof(whTestSlhDsaKeyGenSeed) / 3;
1650+
size_t i;
1651+
1652+
ret = wc_SlhDsaKey_Init(key, WH_TEST_SLHDSA_KAT_PARAM, NULL, devId);
1653+
if (ret != 0) {
1654+
return ret;
1655+
}
1656+
1657+
ret = wh_Client_SlhDsaMakeExportKeyFromSeed(
1658+
ctx, WH_TEST_SLHDSA_KAT_PARAM, whTestSlhDsaKeyGenSeed,
1659+
sizeof(whTestSlhDsaKeyGenSeed), key);
1660+
if (ret == 0) {
1661+
ret = wh_Client_SlhDsaImportKey(ctx, key, &keyId,
1662+
WH_NVM_FLAGS_USAGE_SIGN, 0, NULL);
1663+
}
1664+
if (ret != 0) {
1665+
WH_ERROR_PRINT("Failed to import an SLH-DSA private key: %d\n", ret);
1666+
}
1667+
1668+
for (i = 0; (ret == 0) && (i + skSz <= WOLFHSM_CFG_COMM_DATA_LEN); i++) {
1669+
if (memcmp(comm + i, whTestSlhDsaKeyGenSeed, skSz) == 0) {
1670+
WH_ERROR_PRINT("SLH-DSA private key left in the comm buffer\n");
1671+
ret = WH_TEST_FAIL;
1672+
}
1673+
}
1674+
1675+
if (ret == 0) {
1676+
WH_TEST_PRINT("SLH-DSA IMPORT CLEARS COMM DEVID=0x%X SUCCESS\n", devId);
1677+
}
1678+
1679+
if (!WH_KEYID_ISERASED(keyId)) {
1680+
(void)wh_Client_KeyEvict(ctx, keyId);
1681+
}
1682+
wc_SlhDsaKey_Free(key);
1683+
return ret;
1684+
}
15911685
#endif /* !WOLFSSL_SLHDSA_VERIFY_ONLY */
15921686

15931687
/* Drives the vector through the plain wolfCrypt API, which is how an
@@ -2029,6 +2123,83 @@ static int _whTest_CryptoSlhDsaDmaCachedKey(whClientContext* ctx)
20292123
}
20302124
return ret;
20312125
}
2126+
2127+
/* Fails the client copy-back, which runs after the server has done its work */
2128+
static int _whTest_SlhDsaFailWritePostCb(whClientContext* client,
2129+
uintptr_t clientAddr, void** ptr,
2130+
size_t len, whDmaOper oper,
2131+
whDmaFlags flags)
2132+
{
2133+
(void)client;
2134+
(void)len;
2135+
(void)flags;
2136+
*ptr = (void*)clientAddr;
2137+
return (oper == WH_DMA_OPER_CLIENT_WRITE_POST) ? WH_ERROR_ABORTED
2138+
: WH_ERROR_OK;
2139+
}
2140+
2141+
/* Evict the cached key on a client failure, even for a caller-chosen id */
2142+
static int _whTest_CryptoSlhDsaDmaCacheKeyRollback(whClientContext* ctx)
2143+
{
2144+
int devId = WH_CLIENT_DEVID(ctx);
2145+
int ret;
2146+
whKeyId keyId = WH_KEYID_ERASED;
2147+
whKeyId explicitId = WH_KEYID_ERASED;
2148+
SlhDsaKey pub[1];
2149+
whClientDmaClientMemCb savedCb = ctx->dma.cb;
2150+
2151+
ret = wc_SlhDsaKey_Init(pub, WH_TEST_SLHDSA_DMA_PARAM, NULL, devId);
2152+
if (ret != 0) {
2153+
return ret;
2154+
}
2155+
2156+
/* Borrow an id the server knows is free */
2157+
ret = wh_Client_SlhDsaMakeCacheKeyDma(ctx, WH_TEST_SLHDSA_DMA_PARAM, &keyId,
2158+
WH_NVM_FLAGS_NONE, 0, NULL, pub);
2159+
if (ret == 0) {
2160+
explicitId = keyId;
2161+
ret = wh_Client_KeyEvict(ctx, explicitId);
2162+
}
2163+
if (ret != 0) {
2164+
WH_ERROR_PRINT("Failed to reserve an SLH-DSA key id: %d\n", ret);
2165+
}
2166+
2167+
if (ret == 0) {
2168+
(void)wh_Client_DmaRegisterCb(ctx, _whTest_SlhDsaFailWritePostCb);
2169+
ret = wh_Client_SlhDsaMakeCacheKeyDma(ctx, WH_TEST_SLHDSA_DMA_PARAM,
2170+
&keyId, WH_NVM_FLAGS_NONE, 0,
2171+
NULL, pub);
2172+
(void)wh_Client_DmaRegisterCb(ctx, savedCb);
2173+
if ((ret != WH_ERROR_ABORTED) || !WH_KEYID_ISERASED(keyId)) {
2174+
WH_ERROR_PRINT("SLH-DSA DMA keygen copy-back failure returned "
2175+
"%d keyId 0x%X\n",
2176+
ret, keyId);
2177+
ret = WH_TEST_FAIL;
2178+
}
2179+
else {
2180+
ret = 0;
2181+
}
2182+
}
2183+
2184+
/* Nothing may stay cached under the caller's id */
2185+
if (ret == 0) {
2186+
ret = wh_Client_KeyEvict(ctx, explicitId);
2187+
if (ret != WH_ERROR_NOTFOUND) {
2188+
WH_ERROR_PRINT("SLH-DSA key left cached after failure: %d\n", ret);
2189+
ret = WH_TEST_FAIL;
2190+
}
2191+
else {
2192+
ret = 0;
2193+
}
2194+
}
2195+
2196+
if (ret == 0) {
2197+
WH_TEST_PRINT("SLH-DSA DMA CACHE ROLLBACK DEVID=0x%X SUCCESS\n", devId);
2198+
}
2199+
2200+
wc_SlhDsaKey_Free(pub);
2201+
return ret;
2202+
}
20322203
#endif /* WH_TEST_SLHDSA_DMA_PARAM && WOLFHSM_CFG_DMA */
20332204

20342205
#ifdef WH_TEST_SLHDSA_COMM_PARAM
@@ -2098,6 +2269,8 @@ int whTest_Crypto_SlhDsa(whClientContext* ctx)
20982269
WH_TEST_RETURN_ON_FAIL(_whTest_CryptoSlhDsaNoSign(ctx));
20992270
#else
21002271
WH_TEST_RETURN_ON_FAIL(_whTest_CryptoSlhDsaSeededKeyGen(ctx));
2272+
WH_TEST_RETURN_ON_FAIL(_whTest_CryptoSlhDsaCacheKeyEphemeral(ctx));
2273+
WH_TEST_RETURN_ON_FAIL(_whTest_CryptoSlhDsaImportClearsComm(ctx));
21012274
#endif
21022275
#endif /* WH_TEST_SLHDSA_KAT_PARAM */
21032276

@@ -2131,6 +2304,7 @@ int whTest_Crypto_SlhDsa(whClientContext* ctx)
21312304
#if defined(WH_TEST_SLHDSA_DMA_PARAM) && defined(WOLFHSM_CFG_DMA)
21322305
WH_TEST_RETURN_ON_FAIL(_whTest_CryptoSlhDsaDmaClient(ctx));
21332306
WH_TEST_RETURN_ON_FAIL(_whTest_CryptoSlhDsaDmaCachedKey(ctx));
2307+
WH_TEST_RETURN_ON_FAIL(_whTest_CryptoSlhDsaDmaCacheKeyRollback(ctx));
21342308
#endif
21352309

21362310
(void)ctx;

‎wolfhsm/wh_client_crypto.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3463,7 +3463,7 @@ int wh_Client_SlhDsaExportPublicKey(whClientContext* ctx, whKeyId keyId,
34633463
* @param[in] ctx Pointer to the client context
34643464
* @param[in] param Parameter set to generate (enum SlhDsaParam)
34653465
* @param[in,out] inout_key_id Pointer to key ID to use/receive
3466-
* @param[in] flags Flags to control key persistence
3466+
* @param[in] flags Flags to control key persistence, must not be EPHEMERAL
34673467
* @param[in] label_len Length of optional label
34683468
* @param[in] label Optional label to associate with key
34693469
* @return int Returns 0 on success or a negative error code on failure.
@@ -3528,7 +3528,7 @@ int wh_Client_SlhDsaMakeExportKeyFromSeed(whClientContext* ctx, int param,
35283528
* @param[in] seed Pointer to the 3n seed bytes
35293529
* @param[in] seedSz Length of seed in bytes
35303530
* @param[in,out] inout_key_id Pointer to key ID to use/receive
3531-
* @param[in] flags Flags to control key persistence
3531+
* @param[in] flags Flags to control key persistence, must not be EPHEMERAL
35323532
* @param[in] label_len Length of optional label
35333533
* @param[in] label Optional label to associate with key
35343534
* @return int Returns 0 on success or a negative error code on failure.

0 commit comments

Comments
 (0)