Skip to content

Bound LMS, XMSS and ML-KEM DMA response frames in the crypto client - #476

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/lms_xmss
Open

Bound LMS, XMSS and ML-KEM DMA response frames in the crypto client#476
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/lms_xmss

Conversation

@yosuke-wolfssl

@yosuke-wolfssl yosuke-wolfssl commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Problem

wh_Client_RecvResponse reports the received payload length but nothing validates it against the message being parsed. The comm client reuses one buffer for request and response, so a reply shorter than the response struct leaves the tail holding the client's own outbound request bytes. An unchecked overlay read then returns stale data instead of failing.

Nine DMA response handlers in wh_client_crypto.c overlay their response struct with no frame bound:

Handler Unbounded read
wh_Client_{Lms,Xmss}MakeKeyDma res->keyId, stored into the caller's out-param and the key
wh_Client_{Lms,Xmss}SignDma res->sigLen, bounded against the caller's buffer but not the frame
wh_Client_{Lms,Xmss}VerifyDma resp->res, the verify verdict itself
wh_Client_{Lms,Xmss}SigsLeftDma res->sigsLeft, which gates whether a one-time key is reused
_MlKemMakeKeyDma res->keyId, and res->keySize bounded only by the staging buffer

Follow-up to #457, same family as #474 (CMAC) and #475 (AES DMA). _MlDsaMakeKeyDma already carries this bound on main; these nine are the remaining gap.

Fix (src/wh_client_crypto.c)

Every one of these responses is fixed size with no trailing payload, so a single bound covers each site:

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

These nine functions previously passed req_len as the length out-param of wh_Client_RecvResponse. Each now receives into its own res_len, matching every other handler in the file.

  • Inside the success branch. The DMA dispatcher sends only the generic header when a handler fails, so an unconditional bound would convert every legitimate error reply into WH_ERROR_ABORTED and hide the server's rc.
  • Assigns ret, never returns early, so the DMA unmaps and _MlKemMakeKeyDma's staging-buffer zeroize still run.
  • The pre-existing caller-capacity checks (res->sigLen > sigCap, res->keySize > buffer_len) bound a different thing and still return WH_ERROR_BADARGS.

The data_size parameter recently added to wh_Client_RecvResponse bounds the response from above (WH_ERROR_BUFFER_SIZE); this bounds it from below. This PR is now source-only.

Tests

The test added by the earlier revision has been removed. It was
test-refactor/misc/wh_test_client_malformed_resp.c (711 lines), built on a
scripted transport that echoed the request comm header, always wrote the response
body, and reported whatever frame length the sub-test asked for. Per review, a
test reachable only through a fake transport or a hand-built packet is testing the
harness, not the fix, so it and its wh_test_list.c entry are dropped. This also
removes the add/add conflict with #463/#473/#474/#475, which each created the same
file.

No replacement test is added: a conforming server always sends the full fixed-size
response, so the normal wh_Client_* API has no path to these guards.

Verification

Rebased onto 2d1b25a.

  • test-refactor: default 43 passed / 26 skipped / 0 failed of 69 · DMA=1 47/22/0 · SHE=1 49/20/0
  • Legacy test/ suite clean.
  • Clean under -std=c90 -Werror -Wall -Wextra.
  • All nine guards were previously mutation-checked (weakened to hdr_sz / 2),
    all caught; three hoist controls moving a bound out of the success branch also
    failed, while the test existed.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jul 22, 2026
Copilot AI review requested due to automatic review settings July 22, 2026 05:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the crypto client’s DMA response parsing for LMS, XMSS, and ML-KEM by validating that the received response frame is large enough to safely overlay and read fixed-size response structs, preventing stale request bytes (from the reused comm buffer) from being interpreted as response fields. It also adds a malformed-response test harness that simulates truncated frames and header-only error replies.

Changes:

  • Add per-handler res_len receive lengths and fixed-size frame bounds before reading response struct fields in src/wh_client_crypto.c.
  • Add a scripted-transport misc test (whTest_ClientMalformedResp) that exercises exact-fit, truncation, server-error header-only replies, and caller-capacity rejection cases.
  • Register the new misc test in test-refactor/wh_test_list.c.

Reviewed changes

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

File Description
src/wh_client_crypto.c Adds response-length bounds (via res_len) for LMS/XMSS/ML-KEM DMA response parsing to prevent stale-buffer overlay reads.
test-refactor/misc/wh_test_client_malformed_resp.c Adds a scripted transport + subtests to validate client behavior on truncated/invalid response frames.
test-refactor/wh_test_list.c Registers the new malformed-response misc test entry point.

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

Comment thread src/wh_client_crypto.c Outdated

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #476

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

No new issues found in the changed files. ✅

@Frauschi Frauschi self-assigned this Jul 23, 2026
Frauschi
Frauschi previously approved these changes Jul 23, 2026

@Frauschi Frauschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but has conflicts to main that needs fixing.

@yosuke-wolfssl

Copy link
Copy Markdown
Contributor Author

Hi @Frauschi ,
Thanks for reviewing, I solved the merge conflict

Frauschi
Frauschi previously approved these changes Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants