Skip to content

Check RNG response length before copying inline payload#463

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/f_5460
Open

Check RNG response length before copying inline payload#463
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/f_5460

Conversation

@yosuke-wolfssl

@yosuke-wolfssl yosuke-wolfssl commented Jul 21, 2026

Copy link
Copy Markdown

Problem

wh_Client_RngGenerateResponse validated the server-reported size against the inline cap (WH_MESSAGE_CRYPTO_RNG_MAX_INLINE_SZ) and the caller's buffer, but never against res_len — the length of the frame actually received. A truncated or malformed OK response could set sz past what the frame delivered, making the client memcpy stale comm-buffer bytes and hand them back as random data. Every other inline-copy handler in wh_client_crypto.c (AES-CTR/ECB/CBC/GCM, ECC, Curve25519, Ed25519, RSA, CMAC) already performed this check; RNG was the last one missing it.

The DMA variant copies nothing inline, so it had no read exposure, but it equally never confirmed that a success reply was backed by a full response frame.

Fix (src/wh_client_crypto.c)

  • wh_Client_RngGenerateResponse derives hdr_sz from the generic response header plus whMessageCrypto_RngResponse and rejects when res_len < hdr_sz || res->sz > (res_len - hdr_sz). The first term also prevents the unsigned underflow in the second.
  • wh_Client_RngGenerateDmaResponse rejects a success reply whose frame is shorter than that same header pair. This is defense in depth, not an out-of-bounds fix: no field of the DMA response is read inline today, so the check pins the frame to the protocol contract for future readers. Error replies legitimately carry only the generic header and are unaffected, since the check sits behind the rc gate.

Addressed by f_5460.

Tests (test-refactor/misc/wh_test_client_malformed_resp.c, new)

Misc-group test pairing a real client with a raw whCommServer over the mem transport, so it can emit frames a real server never would.

Case Frame sent Expected
inline, well-formed headers + 64B payload OK; primes the client buffer with a known pattern
inline, payload omitted header pair only WH_ERROR_ABORTED, output untouched
inline, short frame below the header pair WH_ERROR_ABORTED, output untouched
DMA, well-formed full response OK
DMA, one byte short header pair − 1 WH_ERROR_ABORTED
DMA, error reply generic header only, rc WH_ERROR_ACCESS WH_ERROR_ACCESS reaches the caller

DMA cases are gated on WOLFHSM_CFG_DMA. Registered in test-refactor/wh_test_list.c.

Verification

  • make check: 38 passed, 25 skipped, 0 failed of 63. make check DMA=1: 42 passed, 21 skipped, 0 failed.
  • Legacy suite (cd test && make && make run) clean.
  • Negative control: removing only res_len < hdr_sz || makes the short-frame case — and only that case — fail, since the subtraction underflows and the copy proceeds.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jul 21, 2026
Copilot AI review requested due to automatic review settings July 21, 2026 04:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens the client-side RNG response handler against truncated/malformed server frames by validating that the received response length actually contains the claimed inline payload, preventing out-of-bounds reads of stale comm-buffer data.

Changes:

  • Add a response-frame-length bound check in wh_Client_RngGenerateResponse before copying inline RNG payload.
  • Add a new misc test that crafts malformed RNG responses over the mem transport to verify the abort behavior and that caller output remains untouched.
  • Register the new test in the refactored 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 res_len vs header/payload size validation before inline RNG payload memcpy.
test-refactor/misc/wh_test_client_malformed_resp.c New regression test that sends truncated RNG responses and asserts WH_ERROR_ABORTED + no buffer clobber.
test-refactor/wh_test_list.c Registers whTest_ClientMalformedResp in misc tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #463

Scan targets checked: wolfhsm-crypto-bugs, wolfhsm-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

Comment thread test-refactor/misc/wh_test_client_malformed_resp.c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants