Stop READTRUSTED transmitting unwritten response bytes#459
Stop READTRUSTED transmitting unwritten response bytes#459yosuke-wolfssl wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an information-leak/response-corruption issue in the non-DMA READTRUSTED certificate handler where the server could transmit bytes from the shared comm buffer that were never written (e.g., after failures), by tightening length reporting and response sizing. It also updates wh_Server_CertReadTrusted() to honor its documented contract by reporting the stored certificate length on both success and WH_ERROR_BUFFER_SIZE.
Changes:
- Update
wh_Server_CertReadTrusted()to always reportmeta.lenvia*inout_cert_len, including onWH_ERROR_BUFFER_SIZE. - Fix non-DMA
READTRUSTEDresponse construction to only include payload bytes in*out_resp_sizeon success, and only setresp.cert_lenwhen a length is actually resolved. - Add targeted regression tests (server handler-level poisoning test + client API end-to-end coverage under DMA).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/wh_server_cert.c |
Fixes the wh_Server_CertReadTrusted() length contract and prevents unwritten comm-buffer bytes from being included in READTRUSTED responses. |
test-refactor/server/wh_test_cert_readtrusted.c |
Adds direct handler tests that poison the response packet and assert header-only sizing/no writes past header on non-success paths. |
test-refactor/client-server/wh_test_client_certs.c |
Adds DMA-gated end-to-end client test asserting the reported stored length is preserved on WH_ERROR_BUFFER_SIZE. |
test-refactor/wh_test_list.c |
Registers the new whTest_CertReadTrusted test in the server 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 #459
Scan targets checked: wolfhsm-core-bugs, wolfhsm-src
No new issues found in the changed files. ✅
dce04bf to
7036dda
Compare
Summary
The non-DMA
READTRUSTEDhandler sized its response from a length the callee neverresolved. When
wh_Server_CertReadTrusted()failed, no certificate data was staged, butthe handler still ran
resp.cert_len = cert_lenand*out_resp_size = sizeof(resp) + resp.cert_len, so the server transmitted response-packetbytes it had never written.
The handler's return code does not prevent this:
wh_Server_HandleRequestMessage()alwayssends the response regardless (
src/wh_server.c:720-726), and the response packet is thesame buffer that carried the request, so the untouched region holds prior comm-buffer
contents.
Measured before the fix: a stored 8185-byte certificate produced
resp_size = 8192(thewhole comm buffer), of which 8184 bytes were never written. The correct response is the
8-byte header alone.
Addressed by f_6020.
Root cause
wh_Server_CertReadTrusted()returnedWH_ERROR_BUFFER_SIZEbefore assigning*inout_cert_len, contradicting its contract inwolfhsm/wh_server_cert.h:70-72, so thehandler had only the staging clamp to report. The handler then trusted that value
unconditionally, which also leaked on internal
wh_Nvm_GetMetadata()andwh_Nvm_Read()failures.
Fix
wh_Server_CertReadTrusted()reportsmeta.lenon both the success andBUFFER_SIZEpaths, honouring its documented contract. The caller's buffer size is captured first so
the size check is unaffected.
resp.cert_lenonly for outcomes that resolve a length (OKorBUFFER_SIZE), and adds the payload to*out_resp_sizeonly on success. Every otheroutcome yields a header-only response.
Other callers (
CertVerifyMultiRoot,CertVerifyAcert, the DMA handler) never read thelength on a failure return, so the contract change is safe for them.
Reachability
Requires a stored certificate larger than the staging clamp
min(WOLFHSM_CFG_MAX_CERT_SIZE, WOLFHSM_CFG_COMM_DATA_LEN - sizeof(ReadTrustedResponse)).Whether a client can create that depends on config, since the add paths and the read clamp
are bounded differently: the DMA add reaches it in the shipping default
(
COMM_DATA_LEN1280, clamp 1272, DMA add up to 4096), and the inline add reaches it in thetest config (clamp 4096, inline add up to 8156). In a pure non-DMA build the inline add is
bounded below the clamp, so it is only reachable via out-of-band provisioning.
Tests
New
test-refactor/server/wh_test_cert_readtrusted.cdrives the handler directly against apoisoned response packet, pinning the clamp from both sides:
STAGED_LENmust read backwhole,
STAGED_LEN + 1must report the stored size with a header-only response and no bytepast the header touched. A second sub-test locks the
WH_ERROR_ACCESSpath to no length andno payload.
_whTest_CertReadTrustedOverClampinwh_test_client_certs.ccovers the same contractend-to-end at the client API, where the reported size is actually consumed. It picks
whichever add path can exceed the clamp for the active config and asserts such a window
exists rather than silently probing nothing. Gated on
WOLFHSM_CFG_DMA.Both the oversized and client tests were confirmed to fail against the unfixed server, so
they are negative controls. The
ACCESStest passes either way and is kept as aforward-looking guard.
Verification
test-refactordefault (34 passed, 0 failed) andDMA=1 ASAN=1(38 passed, 0 failed). Legacytest/suite unchanged and passing.Scope
READTRUSTED_DMAis out of scope: it answers withwhMessageCert_SimpleResponse, which hasno
cert_lenfield, so the stored length cannot be returned there. Its stale comment iscorrected to record that; extending the DMA response is separate work.