Bound CMAC response payloads against the received frame#474
Open
yosuke-wolfssl wants to merge 1 commit into
Open
Bound CMAC response payloads against the received frame#474yosuke-wolfssl wants to merge 1 commit into
yosuke-wolfssl wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the client-side CMAC response handlers to ensure that any server-declared CMAC output/state being consumed is actually present in the received frame, preventing reuse-buffer tail data from being treated as valid MAC/state.
Changes:
- Add response-length bounds checks to all six CMAC response handlers before copying MAC bytes or restoring CMAC state.
- Preserve server error codes by keeping the new bounds checks inside the success path (so header-only error replies still return
rc). - Add a new refactor misc test that scripts malformed/truncated responses and asserts
WH_ERROR_ABORTEDplus “caller output untouched” behavior.
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 frame-length validation for CMAC response payloads/state prior to restore/copy. |
| test-refactor/misc/wh_test_client_malformed_resp.c | New negative test covering truncated/short CMAC replies (DMA and non-DMA). |
| test-refactor/wh_test_list.c | Registers the new malformed-response misc test. |
💡 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 #474
Scan targets checked: wolfhsm-crypto-bugs, wolfhsm-src
No new issues found in the changed files. ✅
Frauschi
approved these changes
Jul 23, 2026
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
wh_Client_RecvResponsereports the received payload length but never validatesit against the message being parsed. The comm client reuses one buffer for
request and response, so a short reply leaves the tail holding the client's own
outbound request bytes.
None of the six CMAC response handlers bounded the frame.
CmacGenerateResponse,CmacFinalResponse,CmacGenerateDmaResponseandCmacDmaFinalResponsedomemcpy(outMac, res + 1, *outMacLen)over trailing bytes the frame may notcarry, landing stale data in the caller's MAC buffer; the two Update handlers
restore CMAC state from an unvalidated struct. A malicious or malfunctioning
server returns a MAC the client silently accepts.
This is one of the follow-up requested on #457.
Fix (
src/wh_client_crypto.c)Every CMAC response goes through
_getCryptoResponse(), which has alreadyconsumed
rc, so the bound is the combined form guarded by the left operand:CmacGenerateResponsehdr_sz+res->outSzCmacFinalResponsehdr_sz+res->outSzCmacGenerateDmaResponsehdr_sz+res->outSzCmacDmaFinalResponsehdr_sz+res->outSzCmacUpdateResponseCmacDmaUpdateResponseheader alone when a handler fails, so an unconditional bound would convert
every legitimate error reply into
WH_ERROR_ABORTEDand hide the server'src.wh_Crypto_CmacAesRestoreStateFromMsg()readres->resumeStateahead of any bound; it now sits under the new guard.res->outSz < *outMacLen) is untouched — it boundsthe caller's buffer, a different bound from the frame.
_CmacKdfMakeKeyis KDF-via-CMAC and is covered by #467, not here.Test harness (
test-refactor/misc/wh_test_client_malformed_resp.c)New misc test
whTest_ClientMalformedResp, using a scripted transport clientcallback that echoes the request's comm header and reports a frame length
independent of the body it wrote. Per handler: a well-formed exchange that
poisons the buffer, a claim one byte past the frame (rejected, caller buffer
verified untouched), a frame shorter than the response struct on a success
reply, an exact-fit acceptance, and a header-only error reply that must still
return the server's
rc.Verification
make check SHE=1— 44 passed, 0 failed of 63.make check SHE=1 DMA=1— 48 passed, 0 failed of 63.make check SHE=1 DMA=1 ASAN=1— 48 passed, 0 failed, no sanitizer reports.test/suite withSHE=1— clean, exit 0.>to<, to>=, dropping the header clause, and neutering the guard body each make thenew test fail. 24/24 mutants caught.
Note: this adds
test-refactor/misc/wh_test_client_malformed_resp.c, which#463/#464 also add. Expect an add/add conflict; the registry entry and transport
scaffolding are byte-identical to #464's so the resolution is a concatenation of
sub-tests.