Validate response payload sizes in wh_client.c handlers#473
Open
yosuke-wolfssl wants to merge 1 commit into
Open
Validate response payload sizes in wh_client.c handlers#473yosuke-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 wolfHSM client-side response handlers against malformed or truncated server replies by validating received payload sizes before parsing/copying, and by avoiding stack buffers in receive paths where the comm layer can memcpy unbounded payload sizes.
Changes:
- Update
src/wh_client.cresponse handlers to (1) validateresp_sizeagainst the overlaid response struct before reading fields likerc, and (2) ensure variable-length payload claims (e.g., key exportlen) fit within the received frame. - Move several response receives (CommInit/CommInfo/CustomCb/KeyEvict) to use the comm client’s internal data buffer (
wh_CommClient_GetDataPtr) to prevent stack overflow from oversized frames. - Add a new malformed-response regression test that drives the client against a raw comm server to inject short/oversized frames and verify handlers reject them safely.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/wh_client.c | Adds defensive response-size validation and switches vulnerable handlers to receive into the comm buffer to prevent stale reads and stack overflows. |
| test-refactor/misc/wh_test_client_malformed_resp.c | New test covering short/truncated fixed-size responses, key export length-vs-received checks, and oversized-frame handling for former stack receive sites. |
| test-refactor/wh_test_list.c | Registers the new malformed-response test in the misc test group. |
💡 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 #473
Scan targets checked: wolfhsm-core-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
wh_Client_RecvResponse()reports the received payload length but never validates it against the message the caller overlays on the comm buffer. 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: a truncated response reads back as a stalerc == 0with a garbageid/counter, and the key-export paths copy out key material the frame never carried.Separately,
wh_CommClient_RecvResponse()copies the whole received payload into the buffer it is handed, with no bound on that buffer. Four handlers passed a small stack struct, so a reply larger than the struct overflows the stack before any size check the caller could add —wh_Client_KeyEvictResponse()copied up toWOLFHSM_CFG_COMM_DATA_LENbytes over a 12-byte struct.Fix (
src/wh_client.c)KeyExportResponse,KeyExportPublicResponsesize < sizeof(*resp)→rc→resp->len > (size - sizeof(*resp))size < sizeof(*resp)before readingrcCommCloseResponseresp_size != 0CommInitResponse,CommInfoResponse,CustomCbResponse,KeyEvictResponsewh_CommClient_GetDataPtr()instead of a stack structWhere
rclives inside the struct being validated, the checks split across a branch chain so the subtraction is guarded by a preceding sibling branch:Follow-up to #457, which applied this check to
wh_client_she.c, and completes thewh_client.chandlers.KeyExportDmaResponseand the public-DMA path already validated an exact size and are unchanged.Tests
test-refactor/misc/wh_test_client_malformed_resp.cdrives the client against a rawwhCommServer, so a reply can carry the correct kind and sequence while declaring a malformed size. Every bound is pinned from both sides — reject at capacity + 1, accept at exactly capacity. Oversized frames cover the four former stack callers.Verification
test-refactorwithSHE=1: 44 passed, 0 failed. Legacytest/exits 0. Clean under-std=c90 -Werror -Wall -Wextra.>→>=,>→<, and the stack-buffer fix reverted); each mutation fails the suite.