From f563fc84b46da062359cb0e066d95a7ba3f0a89c Mon Sep 17 00:00:00 2001 From: Brett Nicholas <7547222+bigbrett@users.noreply.github.com> Date: Mon, 11 May 2026 10:00:19 -0600 Subject: [PATCH] Guard against client ID aliasing, fix examples --- docs/src-ja/chapter03.md | 2 +- docs/src-ja/chapter05.md | 4 +- docs/src/chapter03.md | 2 +- docs/src/chapter05.md | 4 +- src/wh_server.c | 4 ++ test/wh_test_clientserver.c | 61 ++++++++++++++++++++++++++ test/wh_test_posix_threadsafe_stress.c | 3 +- wolfhsm/wh_keyid.h | 13 +++++- 8 files changed, 85 insertions(+), 8 deletions(-) diff --git a/docs/src-ja/chapter03.md b/docs/src-ja/chapter03.md index a92b0f4b2..f7bc04fea 100644 --- a/docs/src-ja/chapter03.md +++ b/docs/src-ja/chapter03.md @@ -46,7 +46,7 @@ whCommClientConfig commClientCfg = { .transport_cb = transportMemClientCb, .transport_context = (void*)transportMemClientCtx, .transport_config = (void*)transportMemCfg, - .client_id = 123, /* 一意のクライアント識別子 */ + .client_id = 1, /* 一意のクライアント識別子。1-15（WOLFHSM_CFG_GLOBAL_KEYSなしでは0-15） */ }; /* 手順3: クライアント設定の割り当てと初期化 */ diff --git a/docs/src-ja/chapter05.md b/docs/src-ja/chapter05.md index e0c8db9c7..ced7a8608 100644 --- a/docs/src-ja/chapter05.md +++ b/docs/src-ja/chapter05.md @@ -88,7 +88,7 @@ whCommClientConfig commClientCfg[1] = {{ .transport_cb = transportMemClientCb, .transport_context = (void*)transportMemClientCtx, .transport_config = (void*)transportMemCfg, - .client_id = 123, /* 一意のクライアント識別子 */ + .client_id = 1, /* 一意のクライアント識別子。1-15（WOLFHSM_CFG_GLOBAL_KEYSなしでは0-15） */ }}; /* ステップ3: クライアント設定の割り当てと初期化 */ @@ -150,7 +150,7 @@ whCommClientConfig commClientCfg = {{ .transport_cb = posixTransportTcpCb, .transport_context = (void*)posixTransportTcpCtx, .transport_config = (void*)posixTransportTcpCfg, - .client_id = 123, /* 一意のクライアント識別子 */ + .client_id = 1, /* 一意のクライアント識別子。1-15（WOLFHSM_CFG_GLOBAL_KEYSなしでは0-15） */ }}; /* 以降のステップは同じ... */ diff --git a/docs/src/chapter03.md b/docs/src/chapter03.md index c00dad25c..4f1699505 100644 --- a/docs/src/chapter03.md +++ b/docs/src/chapter03.md @@ -45,7 +45,7 @@ whCommClientConfig commClientCfg = { .transport_cb = transportMemClientCb, .transport_context = (void*)transportMemClientCtx, .transport_config = (void*)transportMemCfg, - .client_id = 123, /* unique client identifier */ + .client_id = 1, /* unique client identifier, 1-15 (or 0-15 without WOLFHSM_CFG_GLOBAL_KEYS) */ }; /* Step 3: Allocate and initialize the client configuration */ diff --git a/docs/src/chapter05.md b/docs/src/chapter05.md index 53d917f15..a4d3372c9 100644 --- a/docs/src/chapter05.md +++ b/docs/src/chapter05.md @@ -90,7 +90,7 @@ whCommClientConfig commClientCfg[1] = {{ .transport_cb = transportMemClientCb, .transport_context = (void*)transportMemClientCtx, .transport_config = (void*)transportMemCfg, - .client_id = 123, /* unique client identifier */ + .client_id = 1, /* unique client identifier, 1-15 (or 0-15 without WOLFHSM_CFG_GLOBAL_KEYS) */ }}; /* Step 3: Allocate and initialize the client configuration */ @@ -149,7 +149,7 @@ whCommClientConfig commClientCfg = {{ .transport_cb = posixTransportTcpCb, .transport_context = (void*)posixTransportTcpCtx, .transport_config = (void*)posixTransportTcpCfg, - .client_id = 123, /* unique client identifier */ + .client_id = 1, /* unique client identifier, 1-15 (or 0-15 without WOLFHSM_CFG_GLOBAL_KEYS) */ }}; /* Subsequent steps remain the same... */ diff --git a/src/wh_server.c b/src/wh_server.c index 943e583c1..dd9cb7229 100644 --- a/src/wh_server.c +++ b/src/wh_server.c @@ -204,6 +204,10 @@ static int _wh_Server_HandleCommRequest(whServerContext* server, wh_MessageComm_TranslateInitRequest(magic, (whMessageCommInitRequest*)req_packet, &req); + if (req.client_id > WH_CLIENT_ID_MAX) { + *out_resp_size = 0; + return WH_ERROR_BADARGS; + } #ifdef WOLFHSM_CFG_GLOBAL_KEYS /* USER=0 is reserved for global keys, client_id must be non-zero */ if (req.client_id == WH_KEYUSER_GLOBAL) { diff --git a/test/wh_test_clientserver.c b/test/wh_test_clientserver.c index 947e527d0..de8e66561 100644 --- a/test/wh_test_clientserver.c +++ b/test/wh_test_clientserver.c @@ -516,6 +516,49 @@ static int _clientServerSequentialTestConnectCb(void* context, connected); } +/* Drive one INIT cycle on an already-initialized client/server pair to verify + * the server enforces the [0, WH_CLIENT_ID_MAX] bound on client_id (so values + * that would be silently truncated by the 4-bit USER field of whKeyId — e.g. + * 16 aliasing the USER=0 global namespace, 17 aliasing client 1 — are + * rejected at INIT instead of breaking per-client key isolation). Mutates + * client->comm->client_id; caller is responsible for restoring it. + * + * Note: boundary coverage is limited to [0, 255] because + * client->comm->client_id is uint8_t; values above 255 are truncated by the + * host-side comm field before reaching the wire. */ +static int _testInitClientIdBoundary(whClientContext* client, + whServerContext* server, + uint32_t client_id, int expect_ok) +{ + uint32_t resp_client_id = 0; + uint32_t resp_server_id = 0; + int rc; + uint8_t prior_server_client_id = server->comm->client_id; + + client->comm->client_id = (uint8_t)client_id; + + WH_TEST_RETURN_ON_FAIL(wh_Client_CommInitRequest(client)); + WH_TEST_RETURN_ON_FAIL(wh_Server_HandleRequestMessage(server)); + rc = wh_Client_CommInitResponse(client, &resp_client_id, &resp_server_id); + + if (expect_ok) { + WH_TEST_ASSERT_RETURN(rc == WH_ERROR_OK); + WH_TEST_ASSERT_RETURN(resp_client_id == client_id); + } + else { + /* Server returns BADARGS with size=0; client decodes that as ABORTED. + */ + WH_TEST_ASSERT_RETURN(rc == WH_ERROR_ABORTED); + /* Rejection must not corrupt server-side identity: a regression that + * moved the assignment in _wh_Server_HandleCommRequest above the + * bound check would still produce ABORTED on the wire but would + * leave server->comm->client_id holding the rejected value. */ + WH_TEST_ASSERT_RETURN(server->comm->client_id == + prior_server_client_id); + } + return WH_ERROR_OK; +} + static int _testOutOfBoundsNvmReads(whClientContext* client, whServerContext* server, whNvmId id) { @@ -739,6 +782,24 @@ int whTest_ClientServerSequential(whTestNvmBackendType nvmType) WH_TEST_RETURN_ON_FAIL(wh_Client_CommInitResponse(client, &client_id, &server_id)); WH_TEST_ASSERT_RETURN(client_id == client->comm->client_id); + /* Verify INIT rejects out-of-range client_id values that would otherwise + * be silently truncated by the 4-bit USER field of whKeyId. */ + WH_TEST_RETURN_ON_FAIL(_testInitClientIdBoundary(client, server, 16, 0)); + WH_TEST_RETURN_ON_FAIL(_testInitClientIdBoundary(client, server, 17, 0)); + WH_TEST_RETURN_ON_FAIL(_testInitClientIdBoundary(client, server, 32, 0)); + WH_TEST_RETURN_ON_FAIL(_testInitClientIdBoundary(client, server, 255, 0)); +#ifdef WOLFHSM_CFG_GLOBAL_KEYS + /* USER=0 is reserved for global keys */ + WH_TEST_RETURN_ON_FAIL(_testInitClientIdBoundary(client, server, 0, 0)); +#else + WH_TEST_RETURN_ON_FAIL(_testInitClientIdBoundary(client, server, 0, 1)); +#endif + WH_TEST_RETURN_ON_FAIL( + _testInitClientIdBoundary(client, server, WH_CLIENT_ID_MAX, 1)); + /* Restore default client_id so the rest of the sequential test runs with + * the expected per-client identity on both ends. */ + WH_TEST_RETURN_ON_FAIL(_testInitClientIdBoundary( + client, server, WH_TEST_DEFAULT_CLIENT_ID, 1)); /* Send the comm info message */ WH_TEST_RETURN_ON_FAIL(wh_Client_CommInfoRequest(client)); diff --git a/test/wh_test_posix_threadsafe_stress.c b/test/wh_test_posix_threadsafe_stress.c index 66f1c422d..c26c4714a 100644 --- a/test/wh_test_posix_threadsafe_stress.c +++ b/test/wh_test_posix_threadsafe_stress.c @@ -629,7 +629,8 @@ static int initClientServerPair(StressTestContext* ctx, int pairIndex) pair->clientCommConfig.transport_cb = &clientTransportCb; pair->clientCommConfig.transport_context = &pair->clientTransportCtx; pair->clientCommConfig.transport_config = &pair->tmConfig; - pair->clientCommConfig.client_id = (uint16_t)(100 + pairIndex); + /* client_id must fit in WH_CLIENT_ID_MAX (4-bit USER field) */ + pair->clientCommConfig.client_id = (uint8_t)(1 + pairIndex); /* Configure client */ pair->clientConfig.comm = &pair->clientCommConfig; diff --git a/wolfhsm/wh_keyid.h b/wolfhsm/wh_keyid.h index a60056bbd..c206f82f3 100644 --- a/wolfhsm/wh_keyid.h +++ b/wolfhsm/wh_keyid.h @@ -46,6 +46,15 @@ typedef uint16_t whKeyId; #define WH_KEYTYPE_MASK 0xF000 #define WH_KEYTYPE_SHIFT 12 +/* Maximum valid client_id. The USER field of whKeyId is 4 bits, so client_id + * must fit in [0, WH_CLIENT_ID_MAX]. Values outside this range would be + * silently truncated by WH_MAKE_KEYID, breaking per-client key isolation, so + * the server rejects them at WH_MESSAGE_COMM_ACTION_INIT. With + * WOLFHSM_CFG_GLOBAL_KEYS enabled, value 0 is reserved for the global-keys + * namespace and is also rejected. Derived from WH_KEYUSER_MASK so the bound + * stays in sync if the USER field is ever widened. */ +#define WH_CLIENT_ID_MAX (WH_KEYUSER_MASK >> WH_KEYUSER_SHIFT) + /* * Client-facing key flags (temporary, stripped by server during translation) * @@ -110,7 +119,9 @@ typedef uint16_t whKeyId; * @param type Key type to use as the TYPE field. Input value is ignored and * WH_KEYTYPE_WRAPPED is used if the input clientId has the * WH_CLIENT_KEYID_WRAPPED flag set. - * @param clientId Client identifier to use as USER field + * @param clientId Client identifier to use as USER field. Must be in + * [0, WH_CLIENT_ID_MAX]; the server enforces this at INIT so callers may + * assume the value fits in the 4-bit USER field. * @param reqId Requested keyId from client (may include flags) * @return Server-internal keyId with TYPE, USER, and ID fields properly set. */