Bound AES DMA response frames in the crypto client#475
Open
yosuke-wolfssl wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the wolfHSM crypto client’s AES DMA response parsing to prevent truncated/malformed server replies from causing the client to read stale bytes from the reused comm buffer (which can silently corrupt AES state for CTR/CBC or copy unintended bytes into a GCM tag buffer).
Changes:
- Add response-length bounds checks in the AES DMA response handlers (CTR/ECB/CBC/GCM) in
src/wh_client_crypto.c, keeping the checks inside therc == WH_ERROR_OKpath and preserving DMA POST cleanup behavior. - Add a new scripted-transport test (
whTest_ClientMalformedResp) that exercises accept-at-bound, reject-at-bound−1, and header-only server-error replies for the AES DMA family. - Register the new test in the refactor test list.
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 per-handler res_len bounds checks before parsing/copying trailing AES DMA response payload. |
test-refactor/misc/wh_test_client_malformed_resp.c |
New misc test using a scripted transport to validate the client rejects truncated AES DMA responses without corrupting caller state. |
test-refactor/wh_test_list.c |
Registers whTest_ClientMalformedResp in the misc test suite. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #475
Scan targets checked: wolfhsm-crypto-bugs, wolfhsm-src
No new issues found in the changed files. ✅
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The four AES DMA response handlers in
wh_client_crypto.coverlaid theirresponse struct on the comm buffer without validating the length
wh_Client_RecvResponsereported. The comm client reuses one buffer forrequest and response, so a short reply leaves the tail holding the client's
own outbound request bytes:
AesCtrDmaResponserestoresaes->regandaes->tmpfromAES_IV_SIZE + AES_BLOCK_SIZEbytes read past the struct.AesCbcDmaResponserestores the chaining IV the same way.AesGcmDmaResponsecopiesres->authTagSztag bytes from past thestruct into the caller's buffer, bounded only by the caller's
tag_len.AesEcbDmaResponseparses the header only, but overlays unchecked.A malicious or malfunctioning server returns stale data instead of an error;
for CTR/CBC that silently corrupts cipher state, for GCM it hands the caller
a tag built from its own request bytes.
Follow-up to #457, continuing the
wh_Client_RecvResponseaudit. This is theAES DMA family; CMAC, LMS/XMSS DMA, ML-KEM DMA and
EccCheckPubKeyfollow inseparate PRs.
Fix (
src/wh_client_crypto.c)Each handler now bounds
res_lenbefore parsing:AesCtrDmaResponsesizeof(*res) + AES_IV_SIZE + AES_BLOCK_SIZEAesEcbDmaResponsesizeof(*res)AesCbcDmaResponsesizeof(*res) + AES_IV_SIZEAesGcmDmaResponsesizeof(*res), plusres->authTagSzagainst the remainderEach size matches exactly what the server emits (
*outSizein_HandleAes*Dma). Two placement rules the DMA paths impose:rc == WH_ERROR_OKbranch. The DMAdispatcher sends a header-only frame when a handler fails, so an
unconditional bound would convert every real server error into
WH_ERROR_ABORTED.retrather than returning early, so the DMA*_POSTunmapping at the bottom of each function still runs.
GCM keeps its pre-existing
authTagSz > tag_lenbranch — that bounds thecaller's buffer, which is a different limit from the received frame.
Test harness (
test-refactor/misc/wh_test_client_malformed_resp.c)New
whTest_ClientMalformedResp(misc group). The AES DMA APIs send andreceive in one blocking call, so a scripted transport client callback echoes
the request's comm header and replies with a crafted frame. The body is
always written and only the reported length varies — otherwise the client
reads stale mapped DMA addresses and the wrong check fires.
Per family: accept at exactly the required size, reject at size − 1 with the
caller's state still holding the fill from the accepted exchange, and a
header-only reply carrying
WH_ERROR_BADARGSthat must returnBADARGS.GCM adds a case where the claimed tag exceeds the frame but still fits
tag_len, so only the new bound can reject it, and one where a fully carriedtag overruns
tag_len, leaving the buffer tail untouched.Verification
make check DMA=1— 42 passed, 0 failed.make check SHE=1— 43 passed;the new test reports skipped via the weak stub in non-DMA builds.
test/suite builds and exits 0.<→>, and ±1 on each ofthe four guards, plus deleting and
>→>=on GCM's tag clause. An extracontrol hoisting the CTR bound above the
rccheck also fails the test,proving the error-reply case is enforced and not merely asserted.