Validate SHE client response payload size before use

[yosuke-wolfssl]

Problem

The SHE client response helpers read resp->rc, resp->sz, and payload bytes
without first checking that the server actually returned enough data. Fourteen
handlers across src/wh_client_she.c follow this pattern (f-5466, CWE-1284,
Medium).

SHE reads its response in place from the shared comm buffer — every handler
passes wh_CommClient_GetDataPtr(c->comm), which returns context->data. That
aliasing short-circuits the payload-size check added in wh_CommClient_RecvResponse:

if ((data != NULL) && (payload_size != 0) && (data != context->data)) {
    if (payload_size > data_size) { rc = WH_ERROR_BUFFER_SIZE; }

So for SHE the length is never validated in any layer. On a short response the
fields past payload_size are leftover bytes from the previous message: a
stale rc reading as WH_SHE_ERC_NO_ERROR turns a failed operation into an
apparent success, and the ECB/CBC helpers would trust a stale resp->sz when
copying out.

Fix (src/wh_client_she.c)

Every wh_Client_RecvResponse() call site (17 across the 14 handlers) now
validates the payload before any field is read:

• Minimum size — dataSz < sizeof(*resp) fails with WH_ERROR_ABORTED.
• Variable-length payloads — the four ECB/CBC enc/dec helpers additionally
  require resp->sz > (dataSz - sizeof(*resp)) to fail, ordered after the
  minimum-size guard so the subtraction cannot wrap.
• dataSz is initialized to 0 so an untouched value can't read as valid.

Not included, by design: per-handler group/action checks. The finding also
recommends validating WH_MESSAGE_GROUP_SHE and the expected action, but
wh_Client_RecvResponse() already rejects any response whose resp_kind
differs from c->last_req_kind, and kind = WH_MESSAGE_KIND(group, action) —
both are covered for every caller. Duplicating that per handler would be
unreachable code.

Closes f-5466.

Verification

• Clean build with make SHE=1 under -std=c90 -Werror -Wall -Wextra -Wpointer-arith.
• Full suite rc=0; SHE coverage all SUCCESS — secure boot (incl. boundary),
  LoadKey + UID checks, RND, ECB/CBC/CMAC, keywrap interop, write protect,
  wrapped-key reboot interop, NVM-less flows.
• No sanitizer run: the diff adds only size comparisons and early error returns,
  which is not the memory/UB class.

No negative test injecting a truncated SHE response is included — the in-tree
harness has no fault-injecting transport for this path.

[Copilot]

Pull request overview

This PR hardens the SHE client response parsing by validating that the received payload is large enough before overlaying/reading response structs, preventing stale-buffer data from being interpreted as valid output on truncated replies.

Changes:

• Added a shared _CheckRespSz() guard and applied it at all SHE client receive sites to reject undersized responses before reading fixed fields.
• For variable-length ECB/CBC crypt responses, additionally verify the declared resp->sz fits within the bytes actually received before copying.
• Added a new resp_size regression test that drives the client against a raw comm server and injects malformed/truncated responses.

Reviewed changes

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

File	Description
src/wh_client_she.c	Adds response-size gates before accessing fixed/variable-length response fields and bounds copies to received data.
test/wh_test_she.c	Adds a new test that validates client-side rejection of truncated/mis-sized SHE responses.

---

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

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #457

Scan targets checked: wolfhsm-core-bugs, wolfhsm-src

No new issues found in the changed files. ✅

[padelsbach]

@yosuke-wolfssl, please resolve conflicts

[padelsbach]

wh_client_crypto and wh_server_crypto have similar checking, but they are in this form:

const uint32_t hdr_sz = sizeof(whMessageCrypto_GenericResponseHeader) + sizeof(*res);
if (res_len < hdr_sz || res->sz > (res_len - hdr_sz)) {
    ret = WH_ERROR_ABORTED;
}

Can we preserve that style? Or is there a reason to change?

[yosuke-wolfssl]

Hi @padelsbach ,
Thank you for reviewing. I fixed it as you mentioned:

else if (dataSz < sizeof(*resp) || resp->sz > (dataSz - sizeof(*resp))) {
    ret = WH_ERROR_ABORTED;
}

One SHE-specific note: I kept the _CheckRespSz call before the resp->rc read. Unlike the crypto path, these handlers must read rc first — error replies carry only the fixed struct, so folding everything into a single pre-rc check would let an error response's indeterminate sz mask the real return code. That leaves the dataSz < sizeof(*resp) disjunct as belt-and-suspenders here, which is consistent with how the crypto helpers carry it too.

[padelsbach]

@yosuke-wolfssl, couple requests:

First, can we remove _CheckRespSz and instead do a simple inline check? This is more consistent with the rest of wolfHSM.

if (ret == WH_ERROR_OK && dataSz < sizeof(*resp)) 
    ret = WH_ERROR_ABORTED;

Second, for the SheEncEcb/EncCbc/DecEcb/DecCbc checks, let's do something like this:

    ret = wh_Client_RecvResponse(c, &group, &action, &dataSz, (uint8_t*)resp);
    if (ret == 0) {
        if (dataSz < sizeof(*resp)) {
            ret = WH_ERROR_ABORTED;
        }
        else if (resp->rc != WH_SHE_ERC_NO_ERROR) {
            ret = resp->rc;
        }
        else if (resp->sz > (dataSz - sizeof(*resp))) {
            ret = WH_ERROR_ABORTED;
        }
       else { memcpy(...) }

Also, can you do a follow-up PR to apply this change to other callers of wh_Client_RecvResponse that need this check?

[yosuke-wolfssl]

Hi @padelsbach ,
Both changes applied.

1. _CheckRespSz is gone; the 13 fixed-size sites now inline the check as you wrote it.
2. The four Enc/Dec handlers use your chain. Better than what I had — the fixed-field check as the first branch guards the subtraction via a sibling branch, so no helper and no redundant disjunct.
   One addition to your sketch: I kept the sz < resp->sz → WH_ERROR_BADARGS branch before the memcpy. It's pre-existing on main and bounds the response against the caller's buffer, a separate limit from the received frame. Order is fixed-field → rc → payload bound → caller buffer → copy. Happy to drop it if you'd rather.

Regarding to other callers, The real gap is src/wh_client.c (~14 sites, no open PR touches it); KeyExport / KeyExportPublic / KeyExportDma are the same variable-length shape as this bug. wh_client_crypto.c is partly covered by #463/#464/#465/#467, with same convention you suggested.
They are split by family, based on Fenrir report.
I'll open the PR for src/wh_client.c later.

[padelsbach]

LGTM, let's give @bigbrett a chance to look it over.

[bigbrett]

In general, I think we need to have a generic way of handling these "fuzz"-style argument validation tests that doesn't require a server in the loop at all. Since we trust the server once connected, we haven't been adding these since it is really verbose and not entirely necessary (more of a defense in depth thing) based on the threat model of wolfHSM on real systems. @padelsbach I know we have talked about this too. Whether its in the form of a test-specific transport backend, or an additional callback injection point, I'd like a way to aggregate all the "fuzz-style" argument validation and input sanitization tests in a readable clean way that can be separate from the logical/functional/unit tests and not require a server at all. No action needed for now.

[bigbrett]

@yosuke-wolfssl needs a rebase then I can merge.