Skip to content

Validate ML-DSA keygen response against received length#464

Closed
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/f_5462
Closed

Validate ML-DSA keygen response against received length#464
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/f_5462

Conversation

@yosuke-wolfssl

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

Copy link
Copy Markdown

Summary

Two ML-DSA client response handlers consumed server-reported lengths without
checking them against the frame that actually arrived.

_MlDsaMakeKey recorded the response length from wh_Client_RecvResponse but
never used it. It read res->keyId and res->len and, on the ephemeral export
path, deserialized res->len bytes from (res + 1) without confirming the
frame carried that payload. A short but otherwise well-formed OK response
therefore made the client consume whatever trailed the response body in its own
packet buffer.

wh_Client_MlDsaSignDma had the same gap: it stored res->sigLen into the
caller's *out_len without checking that the frame was long enough to contain
the field, or that the reported length fit the caller's buffer.

ECC, RSA and ML-KEM keygen already validate this; ML-DSA was the last keygen
path without it. _MlDsaMakeKey backs both wh_Client_MlDsaMakeCacheKey and
wh_Client_MlDsaMakeExportKey, and the export wrapper is reached from the PQC
signature crypto callback.

Fixes F_5462.

Changes

src/wh_client_crypto.c

Keygen — bounds check matching the existing ECC/RSA/ML-KEM condition verbatim:

if (res_len < hdr_sz || res->len > (res_len - hdr_sz)) {
    return WH_ERROR_ABORTED;
}

Placed up front, before any response field is consumed, so the res->keyId read
on the cache path is covered too. Cache responses carry no payload (len == 0,
frame == hdr_sz) and still pass.

DMA sign — the response carries no inline payload, so the guard checks the frame
length and the reported signature length against the caller's capacity. That
capacity is stashed in sig_cap before the request is sent, because the
response overwrites the request struct it was read from.

test-refactor/misc/wh_test_client_malformed_resp.c (new) — negative tests.
These APIs send and receive in one blocking call, so they cannot be driven by a
raw comm server on a single thread. The test installs a scripted
whTransportClientCb that echoes the request comm header and returns a response
whose reported length is set independently of the frame length it delivers.

  • Keygen: a well-formed cache keygen (positive control, also poisons the packet
    buffer), a headers-only frame claiming 256 DER bytes, and a frame shorter than
    the crypto headers.
  • DMA sign (WOLFHSM_CFG_DMA only): a complete reply that fits, a frame one byte
    short of the response, and a complete frame reporting a signature larger than
    the caller's buffer.

All malformed cases must return WH_ERROR_ABORTED.

test-refactor/wh_test_list.c — registered in the Misc group.

Verification

Both guards were negative-controlled: with the check neutered the corresponding
subtest fails, and passes once restored.

build result
make check 38 passed, 25 skipped, 0 failed of 63
make check DMA=1 42 passed, 21 skipped, 0 failed of 63

The DMA sign subtest is compiled out without DMA=1, so the DMA=1 run is the
one that exercises it. With the ML-DSA guard forced off, the entry point falls
through to the weak WH_TEST_DECL stub and reports SKIPPED rather than an empty
pass.

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

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 ML-DSA client key-generation path by validating that the received response frame is long enough to contain the claimed exported key payload, preventing the client from deserializing trailing/stale bytes from its packet buffer. It brings ML-DSA in line with the existing ECC/RSA/ML-KEM keygen response-length validation pattern and adds a regression test to cover malformed server responses.

Changes:

  • Added a defensive bounds check in _MlDsaMakeKey() to ensure res->len fits within the received frame length before consuming response fields/payload.
  • Added a new scripted-transport test that simulates truncated-but-“OK” keygen responses and asserts the client returns WH_ERROR_ABORTED.
  • Registered the new test in the Misc test group.

Reviewed changes

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

File Description
src/wh_client_crypto.c Adds response-length validation for ML-DSA keygen before using res->keyId/res->len and before DER deserialization.
test-refactor/misc/wh_test_client_malformed_resp.c New negative test using a scripted transport to simulate malformed/truncated ML-DSA keygen responses.
test-refactor/wh_test_list.c Registers the new malformed-response test in the Misc test list.

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

@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 #464

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

No new issues found in the changed files. ✅

@yosuke-wolfssl

Copy link
Copy Markdown
Author

Issue 5462 was fixed by Commit 35bfcf1 in parallel.
Close this.

@yosuke-wolfssl
yosuke-wolfssl deleted the fix/f_5462 branch July 23, 2026 06:52
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.

4 participants