Unify client response size checks into _getCryptoResponse - #505
Unify client response size checks into _getCryptoResponse#505padelsbach wants to merge 1 commit into
Conversation
acaa0c0 to
bace2a9
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
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.
| (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) + |
bace2a9 to
a74cde3
Compare
| (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) + |
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #505
Scan targets checked: wolfhsm-crypto-bugs, wolfhsm-src
No new issues found in the changed files. ✅
a74cde3 to
1b194d6
Compare
yosuke-wolfssl
left a comment
There was a problem hiding this comment.
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_t→uint16_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.
Moves a handful of repeated hand-rolled size checks into a common function for DRYness.