Skip to content

Add data len check in wh_CommClient_RecvResponse#388

Merged
bigbrett merged 2 commits into
wolfSSL:mainfrom
padelsbach:comm-client-recv-len-check
Jul 22, 2026
Merged

Add data len check in wh_CommClient_RecvResponse#388
bigbrett merged 2 commits into
wolfSSL:mainfrom
padelsbach:comm-client-recv-len-check

Conversation

@padelsbach

Copy link
Copy Markdown
Contributor

wh_CommClient_RecvResponse does a memcpy into a caller provided buf without checking the length -- the function does not contain a length param, however the calling functions do.

This change adds the data_size param, so this change does break the API. Alternative was to use existing uint16_t* out_size as an inout param, but that felt too sneaky and easy to overlook.

All 130ish callers updated. src-jp references the params (and is updated), but the English src does not.

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

Scan targets checked: wolfhsm-core-bugs, 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_comm.c Outdated
Comment thread src/wh_comm.c Outdated
@AlexLanzano

Copy link
Copy Markdown
Member

@padelsbach Please fix the merge conflicts when you can. Thanks

@padelsbach
padelsbach force-pushed the comm-client-recv-len-check branch 2 times, most recently from f88da69 to 45d4c4c Compare July 15, 2026 15:17

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

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

No new issues found in the changed files. ✅

@padelsbach
padelsbach requested a review from AlexLanzano July 15, 2026 16:09
AlexLanzano
AlexLanzano previously approved these changes Jul 22, 2026
@AlexLanzano

Copy link
Copy Markdown
Member

@bigbrett Passing this off to you since this is going to technically break the public API but I don't believe that many (if any) customer's directly call the function in question

@bigbrett bigbrett 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.

@padelsbach looks great, but this will need to be rebased and then you will see CI fail to show you where new callsites have to be modified (all in the SHE layer)

@bigbrett bigbrett removed their assignment Jul 22, 2026
@padelsbach
padelsbach force-pushed the comm-client-recv-len-check branch from 45d4c4c to a36d55e Compare July 22, 2026 20:39
@bigbrett
bigbrett merged commit e94899f into wolfSSL:main Jul 22, 2026
108 checks passed
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