Skip to content

Unify client response size checks into _getCryptoResponse - #505

Open
padelsbach wants to merge 2 commits into
wolfSSL:mainfrom
padelsbach:client-min-resp-size
Open

Unify client response size checks into _getCryptoResponse#505
padelsbach wants to merge 2 commits into
wolfSSL:mainfrom
padelsbach:client-min-resp-size

Conversation

@padelsbach

Copy link
Copy Markdown
Contributor

Moves a handful of repeated hand-rolled size checks into a common function for DRYness.

@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 #505

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 src/wh_client_crypto.c Outdated
(uint8_t**)&res, NULL);
if (ret >= 0) {
/* No trailing payload on update, but the state must be in the frame */
if (res_len < sizeof(whMessageCrypto_GenericResponseHeader) +

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

@padelsbach
padelsbach force-pushed the client-min-resp-size branch from bace2a9 to a74cde3 Compare August 6, 2026 21:58
Comment thread src/wh_client_crypto.c Outdated
(uint8_t**)&res, NULL);
if (ret >= 0) {
/* No trailing payload on update, but the state must be in the frame */
if (res_len < sizeof(whMessageCrypto_GenericResponseHeader) +

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

@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 #505

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

No new issues found in the changed files. ✅

@yosuke-wolfssl
yosuke-wolfssl self-requested a review August 7, 2026 04:11
@padelsbach
padelsbach force-pushed the client-min-resp-size branch from a74cde3 to 1b194d6 Compare August 7, 2026 04:27

@yosuke-wolfssl yosuke-wolfssl 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.

Good cleanup — Comment is about shape, not correctness.

The helper's purpose changes here

_getCryptoResponse() was a message-translation helper: check algoType, return a pointer past the generic header, propagate rc. It had no length parameter. This PR makes it the frame-bounds authority for the whole client crypto layer.

My concern is that it centralizes both the generic check (header present, right algorithm, bytes received) and the per-message check. Since the helper can't know the latter, the caller passes it in as minRespLen — so sizeof(*res) ends up two arguments away from the (uint8_t**)&res it describes, with a cast in between that erases the type. Nothing catches a mismatch, and at 44 of the 74 sites no length check is visible locally at all.

Suggested signature

static int _getCryptoResponse(uint8_t* respBuf, uint16_t type,
                              uint16_t respLen, uint8_t** outResponse,
                              uint16_t* outBodyLen);

Helper does the generic half and reports bodyLen = respLen - sizeof(*header); each caller keeps a local if (bodyLen < sizeof(*res)) return WH_ERROR_ABORTED;. Same DRY win, but the sizeof stays next to the pointer it bounds. The AES sites read better too: bodyLen < sizeof(*res) + AES_IV_SIZE locally, instead of a mandatory trailer hidden in minRespLen.

It also removes the const on the by-value params and the size_tuint16_t narrowing at every call site. Either way, the helper now needs a real doc comment — the one-liner plus TODO no longer covers it.

@padelsbach padelsbach assigned bigbrett and unassigned bigbrett and padelsbach Aug 7, 2026
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.

4 participants