Bound keywrap client requests against the comm data buffer - #494
Bound keywrap client requests against the comm data buffer#494yosuke-wolfssl wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a client-side buffer overflow in the keywrap/data-wrap request builders by ensuring the full on-wire request length is validated against the comm data buffer size before any payload is copied into whCommClient.packet[]. It also makes configuration safer by deriving keywrap max payload sizes from WOLFHSM_CFG_COMM_DATA_LEN when needed and adding compile-time assertions to prevent unsafe explicit configurations.
Changes:
- Add pre-copy request size bounds checks in all five keywrap/data-wrap client request builders.
- Add compile-time static asserts to guarantee “max payload + header” fits within
WOLFHSM_CFG_COMM_DATA_LEN, and derive default keywrap max sizes from the comm buffer size when 2000 won’t fit. - Add a regression test ensuring oversize payloads are rejected before any copy (ASan-negative-control friendly), and update configuration documentation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
wolfhsm/wh_settings.h |
Derives keywrap max sizes from comm buffer length and defines request-overhead constant for safe defaults. |
wolfhsm/wh_message_keystore.h |
Adds compile-time static asserts ensuring keywrap requests fit in the comm data buffer. |
src/wh_client_keywrap.c |
Rejects oversize keywrap/data-wrap requests before copying into the comm data buffer. |
test-refactor/client-server/wh_test_keywrap.c |
Adds an oversize-request regression test across all five public wrappers. |
docs/src/9-Configuration.md |
Documents derived defaults and build-time failure behavior for invalid max-size configurations. |
💡 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 #494
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
No new issues found in the changed files. ✅
Problem
The five keywrap/data-wrap client request builders validated the caller-supplied
length only against
WOLFHSM_CFG_KEYWRAP_MAX_KEY_SIZE/_MAX_DATA_SIZE(default
2000), then assembled the request directly in the comm data buffer,which holds
WOLFHSM_CFG_COMM_DATA_LENbytes (default1280). A maximum-sizedkey wrap copied
6 + 32 + 2000 = 2038bytes into that 1280-byte region, runningoff the end of
whCommClient.packet[]and over the adjacent transport contextpointers.
wh_CommClient_SendRequestdoes reject an over-longdata_size, butonly after the copy has already happened. Reachable from every public blocking
wrapper; no existing test used a key large enough to cross the boundary.
Fix (
src/wh_client_keywrap.c)sizeof(*req) [+ sizeof(*metadata)] + payloadSz > WOLFHSM_CFG_COMM_DATA_LENreturns
WH_ERROR_BADARGS, matching the existingwh_Client_KeyCacheRequest_expattern.
wh_message_keystore.hstatic-asserts, per request type, that theconfigured maximum plus its header fits the comm data buffer, so an explicit
over-large setting fails the build instead of overflowing at runtime.
wh_settings.hderives the two maximums fromWOLFHSM_CFG_COMM_DATA_LENwhen 2000 will not fit, so keywrap builds out of the box. The defaults also
moved next to
WOLFHSM_CFG_COMM_DATA_LEN; they previously sat in a blockskipped under
WH_PADDING_CHECK, where the wire-format audit could not seethem.
The server side needed no change — its handlers already bound every copy with
reqDataSz/respDataSz.Closes f-7150.
Behavior change
WOLFHSM_CFG_COMM_DATA_LEN1280(default)200012408192(all in-tree configs)20002000Requests of 1240–2000 bytes on a default-sized comm buffer now return
WH_ERROR_BADARGS. They were never serviceable — they overran the buffer.Integrators needing the full 2000 bytes should raise
WOLFHSM_CFG_COMM_DATA_LENto at least 2040.
Tests
_whTest_KeywrapOversizeRequestintest-refactor/client-server/wh_test_keywrap.cdrives all five public wrappers one byte past the configured maximum and requires
WH_ERROR_BADARGS. The input buffer is deliberately small, so a builder thatcopied before checking trips ASan.
Verification
test/and bothPOSIX examples build clean under
-Werror.AddressSanitizer: stack-buffer-overflow ... in wh_Client_KeyWrapRequest; anexplicit
MAX_KEY_SIZE 2000atCOMM_DATA_LEN 1280fails the build on thestatic asserts; a default config with only
WOLFHSM_CFG_KEYWRAPset compilesand derives 1240.