Implement SHE GET_ID command with client side request, server side handler, and tests by AlexLanzano · Pull Request #450 · wolfSSL/wolfHSM

AlexLanzano · 2026-07-13T17:01:27Z

No description provided.

Copilot

Pull request overview

This PR adds support for the AUTOSAR SHE CMD_GET_ID command across the wolfHSM client/server messaging layer, exposing a client API to request module identity and implementing the server-side handler to return UID, status (SREG), and an identity CMAC.

Changes:

Added WH_SHE_GET_ID to the SHE action enum and introduced GET_ID request/response message types plus translation helpers.
Implemented client APIs (wh_Client_SheGetId*) and server handler (_GetId) including SREG composition refactoring via _BuildSreg.
Added end-to-end and server-direct tests covering MAC verification, truncated request handling, empty MASTER_ECU_KEY fallback-to-zero-key behavior, and pre-secure-boot allowance; updated docs accordingly.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
wolfhsm/wh_message.h	Adds `WH_SHE_GET_ID` action identifier.
wolfhsm/wh_message_she.h	Defines GET_ID request/response structs and translation function prototypes.
wolfhsm/wh_client_she.h	Adds public client API declarations and documentation for `CMD_GET_ID`.
src/wh_server_she.c	Implements server-side GET_ID handler, refactors SREG building into `_BuildSreg`, and updates state-gating/dispatch.
src/wh_message_she.c	Implements GET_ID request/response translation functions.
src/wh_client_she.c	Implements client request/response helpers and blocking `wh_Client_SheGetId`.
test/wh_test_she.c	Adds end-to-end GET_ID validation and server-direct GET_ID cases (empty key + pre-boot) plus req-size truncation test.
test/wh_test_check_struct_padding.c	Includes new GET_ID message structs in padding/struct compilation coverage.
test-refactor/server/wh_test_she_server.c	Adds GET_ID truncated request test in refactor suite.
test-refactor/misc/wh_test_check_struct_padding.c	Includes new GET_ID message structs in refactor padding coverage.
test-refactor/client-server/wh_test_she.c	Adds end-to-end GET_ID identity+MAC verification in refactor client-server test.
docs/src/5-Features.md	Documents `wh_Client_SheGetId` in the SHE client API feature list.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (3)

test-refactor/misc/wh_test_she_uid_cb.c:148

Casting the byte buffer resp_packet to int32_t* for writes/reads can cause unaligned accesses (UB) on architectures that require 32-bit alignment. Use memcpy to/from a local int32_t instead of pointer casts when seeding/reading the response rc.

    *((int32_t*)resp_packet) = WH_SHE_ERC_NO_ERROR;
    ret = wh_Server_HandleSheRequest(server, WH_COMM_MAGIC_NATIVE, action,
                                     req_size, req_packet, &resp_size,
                                     resp_packet);
    if (ret != 0 || resp_size < sizeof(int32_t)) {
        return WH_SHE_ERC_GENERAL_ERROR;
    }
    return *((const int32_t*)resp_packet);

wolfhsm/wh_server_she.h:47

The getUidCb comment notes it is called on every gated SHE request, but it doesn’t mention that this happens inside the state gate (before any NVM lock is taken). Callers implementing a backend in NVM/flash may need this detail to avoid unsafe assumptions about locking/thread-safety.

/* Reads WH_SHE_UID_SZ bytes into outUid. Returns 0, WH_ERROR_NOTFOUND if no UID
 * is provisioned, or another wolfHSM error. Called on every gated SHE request,
 * so it must be cheap and idempotent. */

docs/src/5-Features.md:766

The secure-boot gate description just above still says only CMD_GET_STATUS and CMD_SET_UID are permitted before secure boot succeeds, but the server gate now also whitelists CMD_GET_ID (see wh_server_she.c). The docs should reflect that GET_ID is allowed in all states.

The same gate also enforces that a UID has been provisioned. When [UID storage callbacks](#she-uid-storage) are installed, that check queries the integrator's store, and a store that reports a failure causes every command except `CMD_GET_STATUS` to return `WH_SHE_ERC_MEMORY_FAILURE`. Status stays readable in every case, per the spec.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #450

Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 1

Medium (1)

SHE UID callback invoked without NVM lock for SET_UID and the pre-dispatch state gate

File: src/wh_server_she.c:2057
Function: wh_Server_HandleSheRequest
Category: NV storage vulnerabilities

_ReportInvalidSheState calls _UidIsProvisioned/getUidCb for nearly every action before any lock is taken, and the WH_SHE_SET_UID case runs _SetUid (which can call setUidCb) with no WH_SERVER_NVM_LOCK, unlike every other handler that touches the keystore/NVM. Since docs explicitly endorse NVM-backed UID callbacks, this permits concurrent unsynchronized NVM access in WOLFHSM_CFG_THREADSAFE builds.

Recommendation: Wrap the _ReportInvalidSheState call and the WH_SHE_SET_UID case with WH_SERVER_NVM_LOCK/WH_SERVER_NVM_UNLOCK like the other handlers.

Referenced code: src/wh_server_she.c:2057-2063 (7 lines)

This review was generated automatically by Fenrir. Findings are non-blocking.

bigbrett

Run git-clang-format main to reformat the diff plz

bigbrett · 2026-08-05T18:50:26Z

+whServerConfig serverConfig = { /* ... */ .she = she, .sheConfig = &sheConfig };
+```
+
+When a getter is installed the server reads the UID on demand at each point that needs it and keeps no copy in `whServerSheContext`, so the UID never outlives a single request in server RAM. The trade-off is that the getter is called on every gated SHE request, and more than once for commands that use the UID, so it must be cheap, idempotent, and free of side effects. Leaving `sheConfig` unset preserves the original in-context behavior exactly.


remove this line

bigbrett · 2026-08-05T18:51:30Z


+### SHE UID Storage
+
+By default the 15-byte ECU UID lives in the caller-owned `whServerSheContext`, which means it is RAM-resident and must be re-provisioned with `CMD_SET_UID` after every reset. Integrators whose UID is fused, held in OTP, or kept in NVM can instead point the server at that store with a pair of callbacks:


This reads like the default behavior is that the UID is RAM only, but really there isn't a default it just depends on if you register callbacks or now. I'd rephrase to make it clear that the callback field would be what dictates the behavior.

bigbrett · 2026-08-05T19:14:52Z

+        memcpy(dest->uid, src->uid, WH_SHE_UID_SZ);
+        memcpy(dest->mac, src->mac, WH_SHE_KEY_SZ);


would these not need translation?

These are accessed/used as byte arrays on both ends so there wouldn't be any need to translate

bigbrett · 2026-08-05T19:15:32Z

+    /* The SHE context is caller-owned and not covered by the memset above, so
+     * the callbacks are assigned on both paths. */


Suggested change

/* The SHE context is caller-owned and not covered by the memset above, so

* the callbacks are assigned on both paths. */

bigbrett · 2026-08-05T19:20:36Z

+struct whServerContext_t;
+
+/* Reads WH_SHE_UID_SZ bytes into outUid. Returns 0, WH_ERROR_NOTFOUND if no UID
+ * is provisioned, or another wolfHSM error. Called on every gated SHE request,
+ * so it must be cheap and idempotent. */
+typedef int (*whServerSheGetUidCb)(struct whServerContext_t* server, void* ctx,
+                                   uint8_t* outUid);
+
+/* Persists the WH_SHE_UID_SZ byte UID provisioned by WH_SHE_SET_UID. */
+typedef int (*whServerSheSetUidCb)(struct whServerContext_t* server, void* ctx,
+                                   const uint8_t* uid);
+
+typedef struct {
+    whServerSheGetUidCb getUidCb; /* NULL = use in-context uid[]/uidSet */
+    whServerSheSetUidCb setUidCb; /* NULL = UID is read-only */
+    void*               uidCtx;   /* opaque, passed back to both callbacks */
+} whServerSheConfig;


could we not have the callbacks take a pointer to a whServerSheContext instead of whServerContext to avoid the impicit wh_server.h dependency? In my mind the only server-specific thing that it would need would be a pointer to the NVM instance, which could be passed through the callback-specific context pointer anyway?

I feel like at that point we should just have the signature be:
int (*whServerSheGetUidCb)(void* ctx, uint8_t* outUid)

Thoughts?

bigbrett · 2026-08-05T19:33:02Z

+        ret = _GetUid(server, uid);
+        if (ret != 0) {
+            ret = WH_SHE_ERC_MEMORY_FAILURE;
+        }


this pattern is repeated a lot, would it make more sense for _GetUid() to just return the right error codes so they don't need to be remuxed everywhere?

I think it's best that the underling caller of the _GetUid determines the SHE error code else we will need to pass in some sort of state to the _GetUid function in order to determine the right SHE error code. I.E. take a look at _UidIsProvisioned which calls _GetUid and how it's caller determines the right SHE error code

bigbrett · 2026-08-05T19:34:09Z

+         * wh_Server_KeystoreReadKey already applies this substitution for the
+         * MASTER_ECU_KEY slot (returns a zero-filled key and rc 0), so the
+         * WH_ERROR_NOTFOUND branch below is only a defensive backstop for
+         * builds/paths where that substitution does not occur. */


Suggested change

* wh_Server_KeystoreReadKey already applies this substitution for the

* MASTER_ECU_KEY slot (returns a zero-filled key and rc 0), so the

* WH_ERROR_NOTFOUND branch below is only a defensive backstop for

* builds/paths where that substitution does not occur. */

bigbrett · 2026-08-05T19:38:57Z

            } /* WH_SERVER_NVM_LOCK() */
            break;
+        case WH_SHE_GET_ID:
+            ret = WH_SERVER_NVM_LOCK(server);


why are we taking the lock here?

To sync the KeyStoreRead of the MASTER_ECU_KEY.

bigbrett · 2026-08-05T19:41:01Z

 - **Bulk crypto**: `wh_Client_SheEncEcb` / `wh_Client_SheEncCbc` / `wh_Client_SheDecEcb` / `wh_Client_SheDecCbc` (`CMD_ENC_*` / `CMD_DEC_*`) — AES-ECB and AES-CBC encrypt and decrypt against a selected key slot
 - **MAC**: `wh_Client_SheGenerateMac` / `wh_Client_SheVerifyMac` (`CMD_GENERATE_MAC` / `CMD_VERIFY_MAC`) — CMAC generation and verification against a selected key slot
 - **Status**: `wh_Client_SheGetStatus` (`CMD_GET_STATUS`) — reads the SHE status register (SREG)
+- **Module identity**: `wh_Client_SheGetId` (`CMD_GET_ID`) — returns the ECU UID, the status register, and a CMAC over the caller's challenge, UID, and status register computed under the `MASTER_ECU_KEY`, letting a party that holds that key verify the module's identity. If the `MASTER_ECU_KEY` slot is empty the MAC is computed with an all-zero key, per the spec.


Suggested change

- **Module identity**: `wh_Client_SheGetId` (`CMD_GET_ID`) — returns the ECU UID, the status register, and a CMAC over the caller's challenge, UID, and status register computed under the `MASTER_ECU_KEY`, letting a party that holds that key verify the module's identity. If the `MASTER_ECU_KEY` slot is empty the MAC is computed with an all-zero key, per the spec.

- **Module identity**: `wh_Client_SheGetId` (`CMD_GET_ID`) — returns the ECU UID, the status register, and a CMAC over the caller's challenge, UID, and status register computed under the `MASTER_ECU_KEY`, letting a party that holds that key verify the module's identity. If the `MASTER_ECU_KEY` slot is empty the MAC is computed with an all-zero key.

bigbrett · 2026-08-05T19:41:57Z


 While the state machine is in any state other than `SUCCESS`, the SHE handler refuses every non-boot command except `CMD_GET_STATUS` and `CMD_SET_UID`, returning `WH_SHE_ERC_SEQUENCE_ERROR`. This is what allows the SHE module to gate cryptographic services on a successful boot measurement: once boot has succeeded, the rest of the SHE command set unlocks; on a boot failure the keys remain inaccessible and only status queries are honored.

+The same gate also enforces that a UID has been provisioned. When [UID storage callbacks](#she-uid-storage) are installed, that check queries the integrator's store, and a store that reports a failure causes every command except `CMD_GET_STATUS` to return `WH_SHE_ERC_MEMORY_FAILURE`. Status stays readable in every case, per the spec.


Suggested change

The same gate also enforces that a UID has been provisioned. When [UID storage callbacks](#she-uid-storage) are installed, that check queries the integrator's store, and a store that reports a failure causes every command except `CMD_GET_STATUS` to return `WH_SHE_ERC_MEMORY_FAILURE`. Status stays readable in every case, per the spec.

The same stateful gate also enforces that a UID has been provisioned. When [UID storage callbacks](#she-uid-storage) are installed, that check queries the integrator's store, and a store that reports a failure causes every command except `CMD_GET_STATUS` to return `WH_SHE_ERC_MEMORY_FAILURE`.

Copilot AI review requested due to automatic review settings July 13, 2026 17:01

AlexLanzano self-assigned this Jul 13, 2026

Copilot started reviewing on behalf of AlexLanzano July 13, 2026 17:02 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

AlexLanzano requested a review from Copilot July 27, 2026 13:01

Copilot started reviewing on behalf of AlexLanzano July 27, 2026 13:01 View session

Copilot AI reviewed Jul 27, 2026

View reviewed changes

Comment thread src/wh_message_she.c

AlexLanzano force-pushed the she-get-id branch from 7695357 to afdb4fa Compare August 5, 2026 14:27

Implement SHE GET_UID API. Implement optional SHE ID callbacks

ae513ee

AlexLanzano force-pushed the she-get-id branch from afdb4fa to ae513ee Compare August 5, 2026 14:42

AlexLanzano requested review from wolfSSL-Fenrir-bot and a lite review from Copilot August 5, 2026 14:43

Copilot started reviewing on behalf of AlexLanzano August 5, 2026 14:43 View session

AlexLanzano marked this pull request as ready for review August 5, 2026 14:49

Copilot AI reviewed Aug 5, 2026

View reviewed changes

wolfSSL-Fenrir-bot reviewed Aug 5, 2026

View reviewed changes

AlexLanzano assigned bigbrett and unassigned AlexLanzano Aug 5, 2026

AlexLanzano requested a review from bigbrett August 5, 2026 14:58

bigbrett requested changes Aug 5, 2026

View reviewed changes

bigbrett assigned AlexLanzano and unassigned bigbrett Aug 5, 2026

Address review feedback

499fb77


		### SHE UID Storage

		By default the 15-byte ECU UID lives in the caller-owned `whServerSheContext`, which means it is RAM-resident and must be re-provisioned with `CMD_SET_UID` after every reset. Integrators whose UID is fused, held in OTP, or kept in NVM can instead point the server at that store with a pair of callbacks:

		memcpy(dest->uid, src->uid, WH_SHE_UID_SZ);
		memcpy(dest->mac, src->mac, WH_SHE_KEY_SZ);

		/* The SHE context is caller-owned and not covered by the memset above, so
		* the callbacks are assigned on both paths. */

	- Module identity: `wh_Client_SheGetId` (`CMD_GET_ID`) — returns the ECU UID, the status register, and a CMAC over the caller's challenge, UID, and status register computed under the `MASTER_ECU_KEY`, letting a party that holds that key verify the module's identity. If the `MASTER_ECU_KEY` slot is empty the MAC is computed with an all-zero key, per the spec.
	- Module identity: `wh_Client_SheGetId` (`CMD_GET_ID`) — returns the ECU UID, the status register, and a CMAC over the caller's challenge, UID, and status register computed under the `MASTER_ECU_KEY`, letting a party that holds that key verify the module's identity. If the `MASTER_ECU_KEY` slot is empty the MAC is computed with an all-zero key.


		While the state machine is in any state other than `SUCCESS`, the SHE handler refuses every non-boot command except `CMD_GET_STATUS` and `CMD_SET_UID`, returning `WH_SHE_ERC_SEQUENCE_ERROR`. This is what allows the SHE module to gate cryptographic services on a successful boot measurement: once boot has succeeded, the rest of the SHE command set unlocks; on a boot failure the keys remain inaccessible and only status queries are honored.

		The same gate also enforces that a UID has been provisioned. When [UID storage callbacks](#she-uid-storage) are installed, that check queries the integrator's store, and a store that reports a failure causes every command except `CMD_GET_STATUS` to return `WH_SHE_ERC_MEMORY_FAILURE`. Status stays readable in every case, per the spec.

	The same gate also enforces that a UID has been provisioned. When [UID storage callbacks](#she-uid-storage) are installed, that check queries the integrator's store, and a store that reports a failure causes every command except `CMD_GET_STATUS` to return `WH_SHE_ERC_MEMORY_FAILURE`. Status stays readable in every case, per the spec.
	The same stateful gate also enforces that a UID has been provisioned. When [UID storage callbacks](#she-uid-storage) are installed, that check queries the integrator's store, and a store that reports a failure causes every command except `CMD_GET_STATUS` to return `WH_SHE_ERC_MEMORY_FAILURE`.

Uh oh!

Conversation

AlexLanzano commented Jul 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #450

Medium (1)

SHE UID callback invoked without NVM lock for SET_UID and the pre-dispatch state gate

Uh oh!

bigbrett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants