Validate SHE client response payload size before use

[yosuke-wolfssl]

Summary

The SHE client response helpers overlaid a response struct on the comm buffer and
read its fields without checking the received payload was large enough to contain
them. The comm layer validates magic, kind and sequence and bounds the payload to
the MTU, but nothing checked a payload against the size of the message being
parsed.

The comm client reuses one buffer for the outgoing request and the incoming
response, so a short reply leaves the tail holding the client's own request
bytes. A truncated Generate MAC response therefore returned success with a
MAC assembled partly from those stale bytes. Confirmed against the unfixed code:
the call returned 0 with a final MAC byte of 0xA5, the filler from the
outbound buffer.

The ECB and CBC helpers also validated the declared sz against the caller's
output buffer rather than the data actually received, so a reply could claim more
payload than arrived. The wrappers cap this, but the response helpers are public
API, so calling one directly with a large buffer reads past the comm buffer.

Addressed by f_5466.

Changes

src/wh_client_she.c

• Reject a response too short for its fixed fields, at all 17 receive sites.
• For the four ECB/CBC helpers, also require the declared payload to fit the data
  received before copying.
• Initialize every dataSz; wh_Client_RecvResponse only writes it on success
  and the new checks read it by value on failure paths.

test-refactor/misc/wh_test_she_client.c, test-refactor/wh_test_list.c

• New whTest_SheClient entry point covering client-side response size
  validation: a fixed-size reply one byte short of its struct, each of the four
  variable-length helpers at the exact bound, and a reply too short for its fixed
  fields.
• It lives in the misc group because it drives the client against a raw comm
  server, so a reply can carry the expected kind and sequence but a malformed
  size. The client-server group supplies a live server, which gives no way to
  inject a malformed reply.

Notes

The bound sums in a wider type instead of subtracting the fixed size from the
received size. The subtracting form is only safe given a precondition established
lines earlier and repeated four times; an edit that dropped it would wrap the
subtraction and silently make the bound a no-op. The minimum size check is kept
alongside it: it guards the rc read, the only check that runs when the server
reports an error, while the bound guards the copy. The payload check stays after
the rc branch, since error replies carry only the fixed struct and checking
first would mask real SHE error codes.

Testing

Runs on stock POSIX hosts with the in-memory transport; no SHE hardware needed.
Both CI workflows already build SHE=1 ASAN=1, so the new test runs there.

cd test-refactor && make check SHE=1
cd test && make SHE=1 && ./Build/wh_test.elf

• Clean under -Werror -Wall -Wextra -std=c90; both suites pass.
• Existing SHE flows (secure boot, ECB, CBC, CMAC, load key, RND) still pass, so
  the checks do not reject valid traffic.
• The new test fails against the unfixed code and passes with the fix.
• Each bound check was mutation tested with permissive and off-by-one forms; all
  caught, as is removing a minimum size check.

[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.