From c55e1ad7a5f077d9bda0e52db0f2132b00da06a2 Mon Sep 17 00:00:00 2001 From: Paul Adelsbach Date: Thu, 19 Mar 2026 12:29:41 -0700 Subject: [PATCH] Add ret_size checking in server keystore --- src/wh_server_keystore.c | 495 ++++++++++++++++++++------------ test/wh_test.c | 3 + test/wh_test_keystore_reqsize.c | 353 +++++++++++++++++++++++ test/wh_test_keystore_reqsize.h | 24 ++ 4 files changed, 684 insertions(+), 191 deletions(-) create mode 100644 test/wh_test_keystore_reqsize.c create mode 100644 test/wh_test_keystore_reqsize.h diff --git a/src/wh_server_keystore.c b/src/wh_server_keystore.c index 8b2a378a..b296fb18 100644 --- a/src/wh_server_keystore.c +++ b/src/wh_server_keystore.c @@ -1664,8 +1664,6 @@ int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, const void* req_packet, uint16_t* out_resp_size, void* resp_packet) { - (void)req_size; - int ret = WH_ERROR_OK; uint8_t* in; uint8_t* out; @@ -1679,29 +1677,46 @@ int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, switch (action) { case WH_KEY_CACHE: { - whMessageKeystore_CacheRequest req; - whMessageKeystore_CacheResponse resp; + whMessageKeystore_CacheRequest req = {0}; + whMessageKeystore_CacheResponse resp = {0}; - /* translate request */ - (void)wh_MessageKeystore_TranslateCacheRequest( - magic, (whMessageKeystore_CacheRequest*)req_packet, &req); + /* Validate req_size can hold the fixed request struct */ + if (req_size < sizeof(req)) { + ret = WH_ERROR_BADARGS; + } - /* in is after fixed size fields */ - in = (uint8_t*)req_packet + sizeof(req); + if (ret == WH_ERROR_OK) { + /* translate request */ + (void)wh_MessageKeystore_TranslateCacheRequest( + magic, (whMessageKeystore_CacheRequest*)req_packet, &req); + + /* Validate that the variable-length key data fits within the + * received packet */ + if (req.sz > req_size - sizeof(req)) { + ret = WH_ERROR_BADARGS; + } + } - /* set the metadata fields */ - meta->id = wh_KeyId_TranslateFromClient( - WH_KEYTYPE_CRYPTO, server->comm->client_id, req.id); - meta->access = WH_NVM_ACCESS_ANY; - meta->flags = req.flags; - meta->len = req.sz; - /* truncate label if it's too large */ - if (req.labelSz > WH_NVM_LABEL_LEN) { - req.labelSz = WH_NVM_LABEL_LEN; + if (ret == WH_ERROR_OK) { + /* in is after fixed size fields */ + in = (uint8_t*)req_packet + sizeof(req); + + /* set the metadata fields */ + meta->id = wh_KeyId_TranslateFromClient( + WH_KEYTYPE_CRYPTO, server->comm->client_id, req.id); + meta->access = WH_NVM_ACCESS_ANY; + meta->flags = req.flags; + meta->len = req.sz; + /* truncate label if it's too large */ + if (req.labelSz > WH_NVM_LABEL_LEN) { + req.labelSz = WH_NVM_LABEL_LEN; + } + memcpy(meta->label, req.label, req.labelSz); } - memcpy(meta->label, req.label, req.labelSz); - ret = WH_SERVER_NVM_LOCK(server); + if (ret == WH_ERROR_OK) { + ret = WH_SERVER_NVM_LOCK(server); + } if (ret == WH_ERROR_OK) { /* get a new id if one wasn't provided */ if (WH_KEYID_ISERASED(meta->id)) { @@ -1709,7 +1724,8 @@ int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, } /* write the key */ if (ret == WH_ERROR_OK) { - ret = wh_Server_KeystoreCacheKeyChecked(server, meta, in); + ret = wh_Server_KeystoreCacheKeyChecked(server, meta, + in); } (void)WH_SERVER_NVM_UNLOCK(server); @@ -1730,26 +1746,35 @@ int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, #ifdef WOLFHSM_CFG_DMA case WH_KEY_CACHE_DMA: { - whMessageKeystore_CacheDmaRequest req; - whMessageKeystore_CacheDmaResponse resp; - - /* translate request */ - (void)wh_MessageKeystore_TranslateCacheDmaRequest( - magic, (whMessageKeystore_CacheDmaRequest*)req_packet, &req); - - /* set the metadata fields */ - meta->id = wh_KeyId_TranslateFromClient( - WH_KEYTYPE_CRYPTO, server->comm->client_id, req.id); - meta->access = WH_NVM_ACCESS_ANY; - meta->flags = req.flags; - meta->len = req.key.sz; - /* truncate label if it's too large */ - if (req.labelSz > WH_NVM_LABEL_LEN) { - req.labelSz = WH_NVM_LABEL_LEN; - } - memcpy(meta->label, req.label, req.labelSz); - - ret = WH_SERVER_NVM_LOCK(server); + whMessageKeystore_CacheDmaRequest req = {0}; + whMessageKeystore_CacheDmaResponse resp = {0}; + + if (req_size < sizeof(req)) { + ret = WH_ERROR_BADARGS; + } + + if (ret == WH_ERROR_OK) { + /* translate request */ + (void)wh_MessageKeystore_TranslateCacheDmaRequest( + magic, (whMessageKeystore_CacheDmaRequest*)req_packet, + &req); + + /* set the metadata fields */ + meta->id = wh_KeyId_TranslateFromClient( + WH_KEYTYPE_CRYPTO, server->comm->client_id, req.id); + meta->access = WH_NVM_ACCESS_ANY; + meta->flags = req.flags; + meta->len = req.key.sz; + /* truncate label if it's too large */ + if (req.labelSz > WH_NVM_LABEL_LEN) { + req.labelSz = WH_NVM_LABEL_LEN; + } + memcpy(meta->label, req.label, req.labelSz); + } + + if (ret == WH_ERROR_OK) { + ret = WH_SERVER_NVM_LOCK(server); + } if (ret == WH_ERROR_OK) { /* get a new id if one wasn't provided */ if (WH_KEYID_ISERASED(meta->id)) { @@ -1758,10 +1783,10 @@ int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, /* write the key using DMA */ if (ret == WH_ERROR_OK) { - ret = wh_Server_KeystoreCacheKeyDmaChecked(server, meta, - req.key.addr); - /* propagate bad address to client if DMA operation failed - */ + ret = wh_Server_KeystoreCacheKeyDmaChecked( + server, meta, req.key.addr); + /* propagate bad address to client if DMA operation + * failed */ if (ret != WH_ERROR_OK) { resp.dmaAddrStatus.badAddr.addr = req.key.addr; resp.dmaAddrStatus.badAddr.sz = req.key.sz; @@ -1784,19 +1809,29 @@ int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, } break; case WH_KEY_EXPORT_DMA: { - whMessageKeystore_ExportDmaRequest req; - whMessageKeystore_ExportDmaResponse resp; + whMessageKeystore_ExportDmaRequest req = {0}; + whMessageKeystore_ExportDmaResponse resp = {0}; + + if (req_size < sizeof(req)) { + ret = WH_ERROR_BADARGS; + } - /* translate request */ - (void)wh_MessageKeystore_TranslateExportDmaRequest( - magic, (whMessageKeystore_ExportDmaRequest*)req_packet, &req); + if (ret == WH_ERROR_OK) { + /* translate request */ + (void)wh_MessageKeystore_TranslateExportDmaRequest( + magic, (whMessageKeystore_ExportDmaRequest*)req_packet, + &req); + } - ret = WH_SERVER_NVM_LOCK(server); + if (ret == WH_ERROR_OK) { + ret = WH_SERVER_NVM_LOCK(server); + } if (ret == WH_ERROR_OK) { ret = wh_Server_KeystoreExportKeyDmaChecked( server, - wh_KeyId_TranslateFromClient( - WH_KEYTYPE_CRYPTO, server->comm->client_id, req.id), + wh_KeyId_TranslateFromClient(WH_KEYTYPE_CRYPTO, + server->comm->client_id, + req.id), req.key.addr, req.key.sz, meta); /* propagate bad address to client if DMA operation failed */ @@ -1823,18 +1858,27 @@ int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, #endif /* WOLFHSM_CFG_DMA */ case WH_KEY_EVICT: { - whMessageKeystore_EvictRequest req; + whMessageKeystore_EvictRequest req = {0}; whMessageKeystore_EvictResponse resp = {0}; - (void)wh_MessageKeystore_TranslateEvictRequest( - magic, (whMessageKeystore_EvictRequest*)req_packet, &req); + if (req_size < sizeof(req)) { + ret = WH_ERROR_BADARGS; + } + + if (ret == WH_ERROR_OK) { + (void)wh_MessageKeystore_TranslateEvictRequest( + magic, (whMessageKeystore_EvictRequest*)req_packet, &req); + } - ret = WH_SERVER_NVM_LOCK(server); + if (ret == WH_ERROR_OK) { + ret = WH_SERVER_NVM_LOCK(server); + } if (ret == WH_ERROR_OK) { ret = wh_Server_KeystoreEvictKeyChecked( server, - wh_KeyId_TranslateFromClient( - WH_KEYTYPE_CRYPTO, server->comm->client_id, req.id)); + wh_KeyId_TranslateFromClient(WH_KEYTYPE_CRYPTO, + server->comm->client_id, + req.id)); resp.ok = 0; /* unused */ (void)WH_SERVER_NVM_UNLOCK(server); @@ -1847,26 +1891,36 @@ int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, } break; case WH_KEY_EXPORT: { - whMessageKeystore_ExportRequest req; - whMessageKeystore_ExportResponse resp; + whMessageKeystore_ExportRequest req = {0}; + whMessageKeystore_ExportResponse resp = {0}; uint32_t keySz; - /* translate request */ - (void)wh_MessageKeystore_TranslateExportRequest( - magic, (whMessageKeystore_ExportRequest*)req_packet, &req); + resp.len = 0; - /* out is after fixed size fields */ - out = (uint8_t*)resp_packet + sizeof(resp); - keySz = WOLFHSM_CFG_COMM_DATA_LEN - sizeof(resp); + if (req_size < sizeof(req)) { + ret = WH_ERROR_BADARGS; + } - resp.len = 0; - ret = WH_SERVER_NVM_LOCK(server); + if (ret == WH_ERROR_OK) { + /* translate request */ + (void)wh_MessageKeystore_TranslateExportRequest( + magic, (whMessageKeystore_ExportRequest*)req_packet, &req); + + /* out is after fixed size fields */ + out = (uint8_t*)resp_packet + sizeof(resp); + keySz = WOLFHSM_CFG_COMM_DATA_LEN - sizeof(resp); + } + + if (ret == WH_ERROR_OK) { + ret = WH_SERVER_NVM_LOCK(server); + } if (ret == WH_ERROR_OK) { /* read the key */ ret = wh_Server_KeystoreReadKeyChecked( server, - wh_KeyId_TranslateFromClient( - WH_KEYTYPE_CRYPTO, server->comm->client_id, req.id), + wh_KeyId_TranslateFromClient(WH_KEYTYPE_CRYPTO, + server->comm->client_id, + req.id), meta, out, &keySz); /* Only provide key output if no error */ @@ -1886,19 +1940,28 @@ int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, } break; case WH_KEY_COMMIT: { - whMessageKeystore_CommitRequest req; - whMessageKeystore_CommitResponse resp; + whMessageKeystore_CommitRequest req = {0}; + whMessageKeystore_CommitResponse resp = {0}; - /* translate request */ - (void)wh_MessageKeystore_TranslateCommitRequest( - magic, (whMessageKeystore_CommitRequest*)req_packet, &req); + if (req_size < sizeof(req)) { + ret = WH_ERROR_BADARGS; + } - ret = WH_SERVER_NVM_LOCK(server); + if (ret == WH_ERROR_OK) { + /* translate request */ + (void)wh_MessageKeystore_TranslateCommitRequest( + magic, (whMessageKeystore_CommitRequest*)req_packet, &req); + } + + if (ret == WH_ERROR_OK) { + ret = WH_SERVER_NVM_LOCK(server); + } if (ret == WH_ERROR_OK) { ret = wh_Server_KeystoreCommitKeyChecked( server, - wh_KeyId_TranslateFromClient( - WH_KEYTYPE_CRYPTO, server->comm->client_id, req.id)); + wh_KeyId_TranslateFromClient(WH_KEYTYPE_CRYPTO, + server->comm->client_id, + req.id)); resp.ok = 0; /* unused */ (void)WH_SERVER_NVM_UNLOCK(server); @@ -1912,19 +1975,28 @@ int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, } break; case WH_KEY_ERASE: { - whMessageKeystore_EraseRequest req; - whMessageKeystore_EraseResponse resp; + whMessageKeystore_EraseRequest req = {0}; + whMessageKeystore_EraseResponse resp = {0}; + + if (req_size < sizeof(req)) { + ret = WH_ERROR_BADARGS; + } - /* translate request */ - (void)wh_MessageKeystore_TranslateEraseRequest( - magic, (whMessageKeystore_EraseRequest*)req_packet, &req); + if (ret == WH_ERROR_OK) { + /* translate request */ + (void)wh_MessageKeystore_TranslateEraseRequest( + magic, (whMessageKeystore_EraseRequest*)req_packet, &req); + } - ret = WH_SERVER_NVM_LOCK(server); + if (ret == WH_ERROR_OK) { + ret = WH_SERVER_NVM_LOCK(server); + } if (ret == WH_ERROR_OK) { ret = wh_Server_KeystoreEraseKeyChecked( server, - wh_KeyId_TranslateFromClient( - WH_KEYTYPE_CRYPTO, server->comm->client_id, req.id)); + wh_KeyId_TranslateFromClient(WH_KEYTYPE_CRYPTO, + server->comm->client_id, + req.id)); resp.ok = 0; /* unused */ (void)WH_SERVER_NVM_UNLOCK(server); @@ -1938,19 +2010,28 @@ int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, } break; case WH_KEY_REVOKE: { - whMessageKeystore_RevokeRequest req; - whMessageKeystore_RevokeResponse resp; + whMessageKeystore_RevokeRequest req = {0}; + whMessageKeystore_RevokeResponse resp = {0}; + + if (req_size < sizeof(req)) { + ret = WH_ERROR_BADARGS; + } - (void)wh_MessageKeystore_TranslateRevokeRequest( - magic, (whMessageKeystore_RevokeRequest*)req_packet, &req); + if (ret == WH_ERROR_OK) { + (void)wh_MessageKeystore_TranslateRevokeRequest( + magic, (whMessageKeystore_RevokeRequest*)req_packet, &req); + } - ret = WH_SERVER_NVM_LOCK(server); + if (ret == WH_ERROR_OK) { + ret = WH_SERVER_NVM_LOCK(server); + } resp.rc = ret; if (ret == WH_ERROR_OK) { resp.rc = wh_Server_KeystoreRevokeKey( server, - wh_KeyId_TranslateFromClient( - WH_KEYTYPE_CRYPTO, server->comm->client_id, req.id)); + wh_KeyId_TranslateFromClient(WH_KEYTYPE_CRYPTO, + server->comm->client_id, + req.id)); (void)WH_SERVER_NVM_UNLOCK(server); } /* WH_SERVER_NVM_LOCK() */ @@ -1967,36 +2048,42 @@ int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, whMessageKeystore_KeyWrapResponse wrapResp = {0}; uint8_t* reqData; uint8_t* respData; - uint32_t reqDataSz = WOLFHSM_CFG_COMM_DATA_LEN - sizeof(wrapReq); uint32_t respDataSz = WOLFHSM_CFG_COMM_DATA_LEN - sizeof(wrapResp); + uint32_t reqDataSz; - /* Validate the bounds of the request data */ - if (reqDataSz < req_size) { - return WH_ERROR_BUFFER_SIZE; + /* Validate req_size can hold the fixed request struct */ + if (req_size < sizeof(wrapReq)) { + ret = WH_ERROR_BADARGS; } - /* Translate request */ - (void)wh_MessageKeystore_TranslateKeyWrapRequest(magic, req_packet, - &wrapReq); + if (ret == WH_ERROR_OK) { + /* Compute actual variable data size from the received packet */ + reqDataSz = req_size - sizeof(wrapReq); + /* Translate request */ + (void)wh_MessageKeystore_TranslateKeyWrapRequest( + magic, req_packet, &wrapReq); - /* Set the request data pointer directly after the request */ - reqData = - (uint8_t*)req_packet + sizeof(whMessageKeystore_KeyWrapRequest); + /* Set the request data pointer directly after the request */ + reqData = (uint8_t*)req_packet + + sizeof(whMessageKeystore_KeyWrapRequest); - /* Set the response data pointer directly after the response */ - respData = (uint8_t*)resp_packet + - sizeof(whMessageKeystore_KeyWrapResponse); + /* Set the response data pointer directly after the response */ + respData = (uint8_t*)resp_packet + + sizeof(whMessageKeystore_KeyWrapResponse); + } - /* Note: Locking here is mega-overkill, as there is only one small - * section inside this request pipeline that needs to be locked - - * freshening the server key and checking usage. Consider relocating - * locking to this section */ - ret = WH_SERVER_NVM_LOCK(server); + /* Note: Locking here is mega-overkill, as there is only one + * small section inside this request pipeline that needs to be + * locked - freshening the server key and checking usage. + * Consider relocating locking to this section */ if (ret == WH_ERROR_OK) { - ret = - _HandleKeyWrapRequest(server, &wrapReq, reqData, reqDataSz, - &wrapResp, respData, respDataSz); + ret = WH_SERVER_NVM_LOCK(server); + } + if (ret == WH_ERROR_OK) { + ret = _HandleKeyWrapRequest(server, &wrapReq, reqData, + reqDataSz, &wrapResp, respData, + respDataSz); (void)WH_SERVER_NVM_UNLOCK(server); } /* WH_SERVER_NVM_LOCK() */ @@ -2013,32 +2100,39 @@ int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, whMessageKeystore_KeyUnwrapAndExportResponse unwrapResp = {0}; uint8_t* reqData; uint8_t* respData; - uint32_t reqDataSz = WOLFHSM_CFG_COMM_DATA_LEN - sizeof(unwrapReq); uint32_t respDataSz = WOLFHSM_CFG_COMM_DATA_LEN - sizeof(unwrapResp); + uint32_t reqDataSz; - /* Validate the bounds of the request data */ - if (reqDataSz < req_size) { - return WH_ERROR_BUFFER_SIZE; + /* Validate req_size can hold the fixed request struct */ + if (req_size < sizeof(unwrapReq)) { + ret = WH_ERROR_BADARGS; } - /* Translate request */ - (void)wh_MessageKeystore_TranslateKeyUnwrapAndExportRequest( - magic, req_packet, &unwrapReq); + if (ret == WH_ERROR_OK) { + /* Compute actual variable data size from the received packet */ + reqDataSz = req_size - sizeof(unwrapReq); - /* Set the request data pointer directly after the request */ - reqData = (uint8_t*)req_packet + - sizeof(whMessageKeystore_KeyUnwrapAndExportRequest); + /* Translate request */ + (void)wh_MessageKeystore_TranslateKeyUnwrapAndExportRequest( + magic, req_packet, &unwrapReq); - /* Set the response data pointer directly after the response */ - respData = (uint8_t*)resp_packet + - sizeof(whMessageKeystore_KeyUnwrapAndExportResponse); + /* Set the request data pointer directly after the request */ + reqData = (uint8_t*)req_packet + + sizeof(whMessageKeystore_KeyUnwrapAndExportRequest); + + /* Set the response data pointer directly after the response */ + respData = (uint8_t*)resp_packet + + sizeof(whMessageKeystore_KeyUnwrapAndExportResponse); + } - /* Note: Locking here is mega-overkill, as there is only one small - * section inside this request pipeline that needs to be locked - - * freshening the server key and checking usage. Consider relocating - * locking to this section */ - ret = WH_SERVER_NVM_LOCK(server); + /* Note: Locking here is mega-overkill, as there is only one + * small section inside this request pipeline that needs to be + * locked - freshening the server key and checking usage. + * Consider relocating locking to this section */ + if (ret == WH_ERROR_OK) { + ret = WH_SERVER_NVM_LOCK(server); + } if (ret == WH_ERROR_OK) { ret = _HandleKeyUnwrapAndExportRequest( server, &unwrapReq, reqData, reqDataSz, &unwrapResp, @@ -2061,31 +2155,38 @@ int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, whMessageKeystore_KeyUnwrapAndCacheResponse cacheResp = {0}; uint8_t* reqData; uint8_t* respData; - uint32_t reqDataSz = WOLFHSM_CFG_COMM_DATA_LEN - sizeof(cacheReq); uint32_t respDataSz = WOLFHSM_CFG_COMM_DATA_LEN - sizeof(cacheResp); + uint32_t reqDataSz; - /* Validate the bounds of the request data */ - if (reqDataSz < req_size) { - return WH_ERROR_BUFFER_SIZE; + /* Validate req_size can hold the fixed request struct */ + if (req_size < sizeof(cacheReq)) { + ret = WH_ERROR_BADARGS; } - /* Translate request */ - (void)wh_MessageKeystore_TranslateKeyUnwrapAndCacheRequest( - magic, req_packet, &cacheReq); + if (ret == WH_ERROR_OK) { + /* Compute actual variable data size from the received packet */ + reqDataSz = req_size - sizeof(cacheReq); + + /* Translate request */ + (void)wh_MessageKeystore_TranslateKeyUnwrapAndCacheRequest( + magic, req_packet, &cacheReq); - /* Set the request data pointer directly after the request */ - reqData = (uint8_t*)req_packet + - sizeof(whMessageKeystore_KeyUnwrapAndCacheRequest); + /* Set the request data pointer directly after the request */ + reqData = (uint8_t*)req_packet + + sizeof(whMessageKeystore_KeyUnwrapAndCacheRequest); - /* Set the response data pointer directly after the response */ - respData = (uint8_t*)resp_packet + - sizeof(whMessageKeystore_KeyUnwrapAndCacheResponse); + /* Set the response data pointer directly after the response */ + respData = (uint8_t*)resp_packet + + sizeof(whMessageKeystore_KeyUnwrapAndCacheResponse); + } - ret = WH_SERVER_NVM_LOCK(server); + if (ret == WH_ERROR_OK) { + ret = WH_SERVER_NVM_LOCK(server); + } if (ret == WH_ERROR_OK) { ret = _HandleKeyUnwrapAndCacheRequest( - server, &cacheReq, reqData, reqDataSz, &cacheResp, respData, - respDataSz); + server, &cacheReq, reqData, reqDataSz, &cacheResp, + respData, respDataSz); (void)WH_SERVER_NVM_UNLOCK(server); } /* WH_SERVER_NVM_LOCK() */ @@ -2103,36 +2204,42 @@ int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, whMessageKeystore_DataWrapResponse wrapResp = {0}; uint8_t* reqData; uint8_t* respData; - uint32_t reqDataSz = WOLFHSM_CFG_COMM_DATA_LEN - sizeof(wrapReq); uint32_t respDataSz = WOLFHSM_CFG_COMM_DATA_LEN - sizeof(wrapResp); + uint32_t reqDataSz; - /* Validate the bounds of the request data */ - if (reqDataSz < req_size) { - return WH_ERROR_BUFFER_SIZE; + /* Validate req_size can hold the fixed request struct */ + if (req_size < sizeof(wrapReq)) { + ret = WH_ERROR_BADARGS; } - /* Translate request */ - (void)wh_MessageKeystore_TranslateDataWrapRequest(magic, req_packet, - &wrapReq); + if (ret == WH_ERROR_OK) { + /* Compute actual variable data size from the received packet */ + reqDataSz = req_size - sizeof(wrapReq); + /* Translate request */ + (void)wh_MessageKeystore_TranslateDataWrapRequest( + magic, req_packet, &wrapReq); - /* Set the request data pointer directly after the request */ - reqData = (uint8_t*)req_packet + - sizeof(whMessageKeystore_DataWrapRequest); + /* Set the request data pointer directly after the request */ + reqData = (uint8_t*)req_packet + + sizeof(whMessageKeystore_DataWrapRequest); - /* Set the response data pointer directly after the response */ - respData = (uint8_t*)resp_packet + - sizeof(whMessageKeystore_DataWrapResponse); + /* Set the response data pointer directly after the response */ + respData = (uint8_t*)resp_packet + + sizeof(whMessageKeystore_DataWrapResponse); + } - /* Note: Locking here is mega-overkill, as there is only one small - * section inside this request pipeline that needs to be locked - - * freshening the server key and checking usage. Consider relocating - * locking to this section */ - ret = WH_SERVER_NVM_LOCK(server); + /* Note: Locking here is mega-overkill, as there is only one + * small section inside this request pipeline that needs to be + * locked - freshening the server key and checking usage. + * Consider relocating locking to this section */ + if (ret == WH_ERROR_OK) { + ret = WH_SERVER_NVM_LOCK(server); + } if (ret == WH_ERROR_OK) { - ret = - _HandleDataWrapRequest(server, &wrapReq, reqData, reqDataSz, - &wrapResp, respData, respDataSz); + ret = _HandleDataWrapRequest(server, &wrapReq, reqData, + reqDataSz, &wrapResp, respData, + respDataSz); (void)WH_SERVER_NVM_UNLOCK(server); } /* WH_SERVER_NVM_LOCK() */ @@ -2149,37 +2256,43 @@ int wh_Server_HandleKeyRequest(whServerContext* server, uint16_t magic, whMessageKeystore_DataUnwrapResponse unwrapResp = {0}; uint8_t* reqData; uint8_t* respData; - uint32_t reqDataSz = WOLFHSM_CFG_COMM_DATA_LEN - sizeof(unwrapReq); uint32_t respDataSz = WOLFHSM_CFG_COMM_DATA_LEN - sizeof(unwrapResp); + uint32_t reqDataSz; - /* Validate the bounds of the request data */ - if (reqDataSz < req_size) { - return WH_ERROR_BUFFER_SIZE; + /* Validate req_size can hold the fixed request struct */ + if (req_size < sizeof(unwrapReq)) { + ret = WH_ERROR_BADARGS; } - /* Translate request */ - (void)wh_MessageKeystore_TranslateDataUnwrapRequest( - magic, req_packet, &unwrapReq); + if (ret == WH_ERROR_OK) { + /* Compute actual variable data size from the received packet */ + reqDataSz = req_size - sizeof(unwrapReq); + /* Translate request */ + (void)wh_MessageKeystore_TranslateDataUnwrapRequest( + magic, req_packet, &unwrapReq); - /* Set the request data pointer directly after the request */ - reqData = (uint8_t*)req_packet + - sizeof(whMessageKeystore_DataUnwrapRequest); + /* Set the request data pointer directly after the request */ + reqData = (uint8_t*)req_packet + + sizeof(whMessageKeystore_DataUnwrapRequest); - /* Set the response data pointer directly after the response */ - respData = (uint8_t*)resp_packet + - sizeof(whMessageKeystore_DataUnwrapResponse); + /* Set the response data pointer directly after the response */ + respData = (uint8_t*)resp_packet + + sizeof(whMessageKeystore_DataUnwrapResponse); + } - /* Note: Locking here is mega-overkill, as there is only one small - * section inside this request pipeline that needs to be locked - - * freshening the server key and checking usage. Consider relocating - * locking to this section */ - ret = WH_SERVER_NVM_LOCK(server); + /* Note: Locking here is mega-overkill, as there is only one + * small section inside this request pipeline that needs to be + * locked - freshening the server key and checking usage. + * Consider relocating locking to this section */ + if (ret == WH_ERROR_OK) { + ret = WH_SERVER_NVM_LOCK(server); + } if (ret == WH_ERROR_OK) { - ret = _HandleDataUnwrapRequest(server, &unwrapReq, reqData, - reqDataSz, &unwrapResp, respData, - respDataSz); + ret = _HandleDataUnwrapRequest( + server, &unwrapReq, reqData, reqDataSz, &unwrapResp, + respData, respDataSz); (void)WH_SERVER_NVM_UNLOCK(server); } /* WH_SERVER_NVM_LOCK() */ diff --git a/test/wh_test.c b/test/wh_test.c index 09f2f992..086e486d 100644 --- a/test/wh_test.c +++ b/test/wh_test.c @@ -45,6 +45,7 @@ #include "wh_test_crypto_affinity.h" #include "wh_test_timeout.h" #include "wh_test_dma.h" +#include "wh_test_keystore_reqsize.h" #if defined(WOLFHSM_CFG_CERTIFICATE_MANAGER) #include "wh_test_cert.h" @@ -91,6 +92,8 @@ int whTest_Unit(void) #ifdef WOLFHSM_CFG_DMA WH_TEST_ASSERT(0 == whTest_Dma()); #endif + /* Keystore req_size validation (bug #125) */ + WH_TEST_ASSERT(0 == whTest_KeystoreReqSize()); /* Comm tests */ WH_TEST_ASSERT(0 == whTest_Comm()); diff --git a/test/wh_test_keystore_reqsize.c b/test/wh_test_keystore_reqsize.c new file mode 100644 index 00000000..4c66a04b --- /dev/null +++ b/test/wh_test_keystore_reqsize.c @@ -0,0 +1,353 @@ +/* + * Copyright (C) 2025 wolfSSL Inc. + * + * This file is part of wolfHSM. + * + * wolfHSM is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfHSM is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with wolfHSM. If not, see . + */ +/* + * test/wh_test_keystore_reqsize.c + * + * Unit tests for bug #125: wh_Server_HandleKeyRequest ignores req_size, + * allowing unbounded reads from the request packet. + * + * Each test crafts a request packet with a valid fixed-size header but passes + * a req_size that is too small to contain the variable-length data claimed by + * the header. A correct implementation must reject these with an error (e.g. + * WH_ERROR_BUFFER_SIZE) rather than blindly reading past the end of the + * packet. + */ + +#include "wolfhsm/wh_settings.h" + +#include +#include + +#include "wolfhsm/wh_error.h" +#include "wolfhsm/wh_comm.h" +#include "wolfhsm/wh_message.h" +#include "wolfhsm/wh_message_keystore.h" +#include "wolfhsm/wh_server.h" +#include "wolfhsm/wh_server_keystore.h" +#include "wolfhsm/wh_transport_mem.h" +#include "wolfhsm/wh_nvm.h" +#include "wolfhsm/wh_nvm_flash.h" +#include "wolfhsm/wh_flash_ramsim.h" + +#if !defined(WOLFHSM_CFG_NO_CRYPTO) +#include "wolfssl/wolfcrypt/settings.h" +#include "wolfssl/wolfcrypt/types.h" +#endif + +#include "wh_test_common.h" + +#if defined(WOLFHSM_CFG_ENABLE_SERVER) && !defined(WOLFHSM_CFG_NO_CRYPTO) + +#define BUFFER_SIZE 4096 +#define FLASH_RAM_SIZE (1024 * 1024) +#define FLASH_SECTOR_SIZE (128 * 1024) +#define FLASH_PAGE_SIZE 8 + +/* + * Helper: set up a minimal server context suitable for calling + * wh_Server_HandleKeyRequest directly. Mirrors the pattern used in + * wh_test_she.c:wh_She_TestReqSizeChecking(). + */ +typedef struct { + whServerContext server[1]; + whNvmContext nvm[1]; +#ifndef WOLFHSM_CFG_NO_CRYPTO + whServerCryptoContext crypto[1]; +#endif + /* Transport (not exercised, but required for server init) */ + uint8_t reqBuf[BUFFER_SIZE]; + uint8_t respBuf[BUFFER_SIZE]; + whTransportMemConfig tmcf[1]; + whTransportServerCb tscb[1]; + whTransportMemServerContext tmsc[1]; + whCommServerConfig cs_conf[1]; + /* Flash / NVM */ + whFlashRamsimCtx fc[1]; + whFlashRamsimCfg fc_conf[1]; + whFlashCb fcb[1]; + whNvmFlashConfig nf_conf[1]; + whNvmFlashContext nfc[1]; + whNvmCb nfcb[1]; + whNvmConfig n_conf[1]; + whServerConfig s_conf[1]; +} TestCtx; + +/* Flash memory is static to avoid 1MB on the stack */ +static uint8_t _flashMemory[FLASH_RAM_SIZE]; + +static int _SetupServer(TestCtx* ctx) +{ + memset(ctx, 0, sizeof(*ctx)); + memset(_flashMemory, 0, sizeof(_flashMemory)); + + /* Transport */ + ctx->tmcf[0] = (whTransportMemConfig){ + .req = (whTransportMemCsr*)ctx->reqBuf, + .req_size = sizeof(ctx->reqBuf), + .resp = (whTransportMemCsr*)ctx->respBuf, + .resp_size = sizeof(ctx->respBuf), + }; + ctx->tscb[0] = (whTransportServerCb)WH_TRANSPORT_MEM_SERVER_CB; + ctx->cs_conf[0] = (whCommServerConfig){ + .transport_cb = ctx->tscb, + .transport_context = (void*)ctx->tmsc, + .transport_config = (void*)ctx->tmcf, + .server_id = 124, + }; + + /* Flash */ + ctx->fc_conf[0] = (whFlashRamsimCfg){ + .size = FLASH_RAM_SIZE, + .sectorSize = FLASH_SECTOR_SIZE, + .pageSize = FLASH_PAGE_SIZE, + .erasedByte = ~(uint8_t)0, + .memory = _flashMemory, + }; + ctx->fcb[0] = (whFlashCb)WH_FLASH_RAMSIM_CB; + ctx->nf_conf[0] = (whNvmFlashConfig){ + .cb = ctx->fcb, + .context = ctx->fc, + .config = ctx->fc_conf, + }; + ctx->nfcb[0] = (whNvmCb)WH_NVM_FLASH_CB; + ctx->n_conf[0] = (whNvmConfig){ + .cb = ctx->nfcb, + .context = ctx->nfc, + .config = ctx->nf_conf, + }; + + /* Server config */ + ctx->s_conf[0] = (whServerConfig){ + .comm_config = ctx->cs_conf, + .nvm = ctx->nvm, +#ifndef WOLFHSM_CFG_NO_CRYPTO + .crypto = ctx->crypto, +#endif + }; + + WH_TEST_RETURN_ON_FAIL(wh_Nvm_Init(ctx->nvm, ctx->n_conf)); +#ifndef WOLFHSM_CFG_NO_CRYPTO + WH_TEST_RETURN_ON_FAIL(wolfCrypt_Init()); + WH_TEST_RETURN_ON_FAIL( + wc_InitRng_ex(ctx->crypto->rng, NULL, INVALID_DEVID)); +#endif + WH_TEST_RETURN_ON_FAIL(wh_Server_Init(ctx->server, ctx->s_conf)); + WH_TEST_RETURN_ON_FAIL( + wh_Server_SetConnected(ctx->server, WH_COMM_CONNECTED)); + return 0; +} + +static void _CleanupServer(TestCtx* ctx) +{ + (void)wh_Server_Cleanup(ctx->server); + (void)wh_Nvm_Cleanup(ctx->nvm); +#ifndef WOLFHSM_CFG_NO_CRYPTO + (void)wc_FreeRng(ctx->crypto->rng); + (void)wolfCrypt_Cleanup(); +#endif +} + +static int wh_Keystore_TestReqSizeChecking(void) +{ + TestCtx ctx[1]; + uint8_t req_packet[WOLFHSM_CFG_COMM_DATA_LEN] = {0}; + uint8_t resp_packet[WOLFHSM_CFG_COMM_DATA_LEN] = {0}; + uint16_t req_size; + uint16_t resp_size; + int ret; + + WH_TEST_RETURN_ON_FAIL(_SetupServer(ctx)); + + /* + * Test 1: WH_KEY_CACHE with req_size too small for the variable-length + * key data. + * + * The request declares 32 bytes of key data following the fixed header, + * but req_size only covers the header. + */ + { + whMessageKeystore_CacheRequest* req = + (whMessageKeystore_CacheRequest*)req_packet; + whMessageKeystore_CacheResponse* cacheResp = + (whMessageKeystore_CacheResponse*)resp_packet; + memset(req_packet, 0, sizeof(req_packet)); + memset(cacheResp, 0, sizeof(*cacheResp)); + resp_size = 0; + req->sz = 32; + req->labelSz = 0; + req->id = 0; + req->flags = 0; + req_size = sizeof(*req); + ret = wh_Server_HandleKeyRequest(ctx->server, WH_COMM_MAGIC_NATIVE, + WH_KEY_CACHE, req_size, req_packet, + &resp_size, resp_packet); + if (ret == WH_ERROR_OK) { + WH_TEST_ASSERT_RETURN(resp_size == sizeof(*cacheResp)); + WH_TEST_ASSERT_RETURN(cacheResp->rc != WH_ERROR_OK); + } + } + + /* + * Test 2: WH_KEY_CACHE with req_size smaller than the fixed header. + */ + { + whMessageKeystore_CacheResponse* cacheResp = + (whMessageKeystore_CacheResponse*)resp_packet; + memset(req_packet, 0, sizeof(req_packet)); + memset(cacheResp, 0, sizeof(*cacheResp)); + resp_size = 0; + req_size = sizeof(whMessageKeystore_CacheRequest) - 1; + ret = wh_Server_HandleKeyRequest(ctx->server, WH_COMM_MAGIC_NATIVE, + WH_KEY_CACHE, req_size, req_packet, + &resp_size, resp_packet); + if (ret == WH_ERROR_OK) { + WH_TEST_ASSERT_RETURN(resp_size == sizeof(*cacheResp)); + WH_TEST_ASSERT_RETURN(cacheResp->rc != WH_ERROR_OK); + } + } + + /* + * Test 3: WH_KEY_CACHE with req.sz claiming more data than the entire + * comm buffer. + */ + { + whMessageKeystore_CacheRequest* req = + (whMessageKeystore_CacheRequest*)req_packet; + whMessageKeystore_CacheResponse* cacheResp = + (whMessageKeystore_CacheResponse*)resp_packet; + memset(req_packet, 0, sizeof(req_packet)); + memset(cacheResp, 0, sizeof(*cacheResp)); + resp_size = 0; + req->sz = WOLFHSM_CFG_COMM_DATA_LEN; + req->labelSz = 0; + req->id = 0; + req->flags = 0; + req_size = sizeof(*req); + ret = wh_Server_HandleKeyRequest(ctx->server, WH_COMM_MAGIC_NATIVE, + WH_KEY_CACHE, req_size, req_packet, + &resp_size, resp_packet); + if (ret == WH_ERROR_OK) { + WH_TEST_ASSERT_RETURN(resp_size == sizeof(*cacheResp)); + WH_TEST_ASSERT_RETURN(cacheResp->rc != WH_ERROR_OK); + } + } + + /* + * Test 4: WH_KEY_EVICT with req_size smaller than the fixed request + * header. + */ + { + whMessageKeystore_EvictResponse* evictResp = + (whMessageKeystore_EvictResponse*)resp_packet; + memset(req_packet, 0, sizeof(req_packet)); + memset(evictResp, 0, sizeof(*evictResp)); + resp_size = 0; + req_size = sizeof(whMessageKeystore_EvictRequest) - 1; + ret = wh_Server_HandleKeyRequest(ctx->server, WH_COMM_MAGIC_NATIVE, + WH_KEY_EVICT, req_size, req_packet, + &resp_size, resp_packet); + if (ret == WH_ERROR_OK) { + WH_TEST_ASSERT_RETURN(resp_size == sizeof(*evictResp)); + WH_TEST_ASSERT_RETURN(evictResp->rc != WH_ERROR_OK); + } + } + + /* + * Test 5: WH_KEY_EXPORT with req_size smaller than the fixed request + * header. + */ + { + whMessageKeystore_ExportResponse* exportResp = + (whMessageKeystore_ExportResponse*)resp_packet; + memset(req_packet, 0, sizeof(req_packet)); + memset(exportResp, 0, sizeof(*exportResp)); + resp_size = 0; + req_size = sizeof(whMessageKeystore_ExportRequest) - 1; + ret = wh_Server_HandleKeyRequest(ctx->server, WH_COMM_MAGIC_NATIVE, + WH_KEY_EXPORT, req_size, req_packet, + &resp_size, resp_packet); + if (ret == WH_ERROR_OK) { + WH_TEST_ASSERT_RETURN(resp_size == sizeof(*exportResp)); + WH_TEST_ASSERT_RETURN(exportResp->rc != WH_ERROR_OK); + } + } + + /* + * Test 6: WH_KEY_COMMIT with req_size smaller than the fixed request + * header. + */ + { + whMessageKeystore_CommitResponse* commitResp = + (whMessageKeystore_CommitResponse*)resp_packet; + memset(req_packet, 0, sizeof(req_packet)); + memset(commitResp, 0, sizeof(*commitResp)); + resp_size = 0; + req_size = sizeof(whMessageKeystore_CommitRequest) - 1; + ret = wh_Server_HandleKeyRequest(ctx->server, WH_COMM_MAGIC_NATIVE, + WH_KEY_COMMIT, req_size, req_packet, + &resp_size, resp_packet); + if (ret == WH_ERROR_OK) { + WH_TEST_ASSERT_RETURN(resp_size == sizeof(*commitResp)); + WH_TEST_ASSERT_RETURN(commitResp->rc != WH_ERROR_OK); + } + } + + /* + * Test 7: WH_KEY_ERASE with req_size smaller than the fixed request + * header. + */ + { + whMessageKeystore_EraseResponse* eraseResp = + (whMessageKeystore_EraseResponse*)resp_packet; + memset(req_packet, 0, sizeof(req_packet)); + memset(eraseResp, 0, sizeof(*eraseResp)); + resp_size = 0; + req_size = sizeof(whMessageKeystore_EraseRequest) - 1; + ret = wh_Server_HandleKeyRequest(ctx->server, WH_COMM_MAGIC_NATIVE, + WH_KEY_ERASE, req_size, req_packet, + &resp_size, resp_packet); + if (ret == WH_ERROR_OK) { + WH_TEST_ASSERT_RETURN(resp_size == sizeof(*eraseResp)); + WH_TEST_ASSERT_RETURN(eraseResp->rc != WH_ERROR_OK); + } + } + + WH_TEST_PRINT("Keystore req_size validation tests: ALL PASSED\n"); + + _CleanupServer(ctx); + return 0; +} + +/* + * Main entry point: run all keystore req_size validation tests. + */ +int whTest_KeystoreReqSize(void) +{ + WH_TEST_PRINT("Testing keystore HandleKeyRequest req_size validation...\n"); + return wh_Keystore_TestReqSizeChecking(); +} + +#else /* !WOLFHSM_CFG_ENABLE_SERVER || WOLFHSM_CFG_NO_CRYPTO */ + +int whTest_KeystoreReqSize(void) +{ + return 0; +} + +#endif /* WOLFHSM_CFG_ENABLE_SERVER && !WOLFHSM_CFG_NO_CRYPTO */ diff --git a/test/wh_test_keystore_reqsize.h b/test/wh_test_keystore_reqsize.h new file mode 100644 index 00000000..43bdb486 --- /dev/null +++ b/test/wh_test_keystore_reqsize.h @@ -0,0 +1,24 @@ +/* + * Copyright (C) 2025 wolfSSL Inc. + * + * This file is part of wolfHSM. + * + * wolfHSM is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfHSM is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with wolfHSM. If not, see . + */ +#ifndef WH_TEST_KEYSTORE_REQSIZE_H_ +#define WH_TEST_KEYSTORE_REQSIZE_H_ + +int whTest_KeystoreReqSize(void); + +#endif /* WH_TEST_KEYSTORE_REQSIZE_H_ */