Validate inSize before translating fixed crypto requests#466
Open
yosuke-wolfssl wants to merge 1 commit into
Open
Validate inSize before translating fixed crypto requests#466yosuke-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 wh_Server_HandleCryptoRequest’s fixed-struct crypto request handlers by ensuring inSize is validated before translating/reading request structs, preventing stale-parameter consumption from previously-used comm buffers. It also adds a focused regression test that reproduces the truncated-request scenario by reusing an unmodified request buffer.
Changes:
- Add early
inSize < sizeof(fixed_request_struct)guards for ECC, Curve25519, Ed25519, and ML-DSA keygen handlers, and move existing ML-DSA sign/verify size checks ahead of translation. - Add new server-side test
whTest_CryptoReqSizeto confirm truncated requests are rejected withWH_ERROR_BADARGSwhile a full-length control succeeds. - Register the new test in the server test list.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/wh_server_crypto.c |
Adds/moves fixed-struct inSize validation ahead of translate calls in several crypto handlers. |
test-refactor/server/wh_test_crypto_reqsize.c |
New regression test that exercises truncated fixed-struct crypto requests against wh_Server_HandleCryptoRequest. |
test-refactor/wh_test_list.c |
Registers whTest_CryptoReqSize in the server test group. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yosuke-wolfssl
force-pushed
the
fix/f_5463
branch
from
July 21, 2026 06:46
7525e49 to
a74641d
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #466
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.
Summary
Four server crypto handlers translated their fixed request struct out of
cryptoDataInwithout first proving the request was long enough to hold it.Two more validated the length only after translating.
wh_Server_HandleCryptoRequestvalidates the generic header, then passes theremaining length straight through:
A request carrying only the generic header yields
cryptoInSize == 0, but thehandler still reads a full struct. Since crypto requests are processed in place
in a buffer that retains the previous message, it consumes bytes left over from
an earlier request as key generation parameters.
Addressed by f_5463.
Changes
_HandleEccKeyGen,_HandleCurve25519KeyGen,_HandleEd25519KeyGenand_HandleMlDsaKeyGengain the guard_HandleRsaKeyGenalready used:_HandleMlDsaSignand_HandleMlDsaVerifykeep their existing check, movedahead of the translate call.
Every other fixed-struct handler already validated
inSizefirst, includingECDH and Curve25519 shared secret. Impact is stale-parameter use, not a read
past the comm buffer. No API or wire format changes.
Test
New
test-refactor/server/wh_test_crypto_reqsize.c(whTest_CryptoReqSize,Server group) drives
wh_Server_HandleCryptoRequestdirectly. Per algorithm itsends a valid request as a positive control, then reissues the unmodified
buffer truncated to the header and to one byte short of the struct — leaving
the control request in place is what reproduces the stale read. Truncated calls
must return
WH_ERROR_BADARGS.Keygen cases use ephemeral keys so nothing touches the keystore. ML-DSA sign
and verify need real state, so that subtest caches a key, signs, and verifies
before its truncated cases, then evicts on every path and confirms the eviction.
Against
mainthe four keygen cases fail:Sign and verify are regression guards, not reproducers — the old ordering also
rejected, just after reading past the payload. Deleting their guard outright
does break them, which is the point.
Verification
cd test-refactor && make check— 38 passed, 25 skipped, 0 failed of 63cd test && make run— clean-std=c90 -Werror -Wall -Wextra, including reduced algorithmconfigs; with no covered algorithm the test is skipped rather than built