Skip to content

Reject UINT32_MAX before size + 1 allocations - #1364

Open
ejohnstown wants to merge 1 commit into
wolfSSL:masterfrom
ejohnstown:size-plus-one-allocs
Open

ejohnstown wants to merge 1 commit into
wolfSSL:masterfrom
ejohnstown:size-plus-one-allocs

Conversation

@ejohnstown

Copy link
Copy Markdown
Contributor

DoSshPubKey() and wolfSSH_SetUsernameRaw() return WS_BAD_ARGUMENT for
a size of UINT32_MAX, where size + 1 wraps to a zero-byte allocation.

  • api tests for both

Issue: F-15662, F-15663

Copilot AI balanced review requested due to automatic review settings November 2, 2026 18:56

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.

🟢 Approval recommended

No active review issues are recorded for the username and SSH public-key size-guard changes.

No open issues 🎉

What changed in this PR

This PR hardens username handling and SSH public-key parsing by rejecting UINT32_MAX before adding space for a null terminator, preventing allocation-size wraparound.

Changes:

  • Return WS_BAD_ARGUMENT for UINT32_MAX sizes in wolfSSH_SetUsernameRaw() and DoSshPubKey().
  • Add API regression tests for both rejection paths.
File Description
tests/​api.c Adds maximum-size rejection tests for usernames and SSH public keys.
src/​ssh.c Guards both size + 1 allocations against overflow.

Review effort: Balanced

DoSshPubKey() and wolfSSH_SetUsernameRaw() allocate a word32 size plus
one for a null terminator. Both now return WS_BAD_ARGUMENT for a size
of UINT32_MAX, where that sum wraps to a zero-byte allocation.

- api.c: SetUsernameRaw and ReadKey_buffer reject UINT32_MAX

Issue: F-15662, F-15663

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

Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 1 of 2 in-scope changed file(s) opened by the reviewer; not opened: tests/api.c

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

Review tier: Lite

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