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_ */