SSH hardening: buffers, algo lists, auth attempts, parsing#1117
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens wolfSSH’s SSH transport and authentication handling by (1) zeroizing decrypted buffers before release, (2) validating algorithm name-lists before storing/advertising them, and (3) enforcing a server-side cap on failed userauth attempts with disconnect-on-limit behavior.
Changes:
- Add algo-list validation via
CheckAlgoList()and updatewolfSSH_{CTX_,}SetAlgoList*()setters to reject invalid lists. - Add
maxAuthAttempts(default 6) with new public CTX/session APIs and enforce counting/disconnect logic in server-side userauth paths. - Zeroize channel/buffer plaintext before free/realloc and add
--enable-none-cipherbuild option for explicitly allowing"none"cipher/MAC.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfssh/ssh.h | Documents new algo-list validation behavior and adds MaxAuthAttempts public API declarations. |
| wolfssh/internal.h | Adds defaults/fields for auth attempt limiting and declares CheckAlgoList(). |
| src/ssh.c | Implements algo-list validation in setters and adds CTX/session MaxAuthAttempts getters/setters. |
| src/internal.c | Implements CheckAlgoList(), adds buffer zeroization on free/grow/shrink, and enforces auth attempt counting/limit logic. |
| tests/unit.c | Adds a unit test to verify the auth-attempt cap behavior, including WANT_WRITE send behavior. |
| tests/api.c | Adds API tests for MaxAuthAttempts and expands algo-list tests to cover new validation behavior. |
| configure.ac | Adds --enable-none-cipher to gate negotiating insecure "none" cipher/MAC. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1117
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
ejohnstown
changed the title
- accept() userauth loop had no cap on failed attempts - Add maxAuthAttempts (CTX, default 6) and per-session authFailures - CountUserAuthFailure() counts only genuine pw/pubkey/kbd-int failures - On limit: send SSH_MSG_DISCONNECT, fail accept; SetMaxAuthAttempts(<=0)=default Issue: 2473
- Setters stored caller string verbatim; "none" cipher/MAC risked cleartext - Add CheckAlgoList()/NameToIdType() to validate each token by category - Set* now returns WS_INVALID_ALGO_ID on bad/NULL input, list unchanged - "none" is rejected in the key list, and allowed for cipher/MAC only under WOLFSSH_ALLOW_NONE_CIPHER - Names unknown to the build are skipped rather than rejected, so a portable superset list still works; a known name in the wrong category fails - Reject empty list elements apart from one trailing comma, which would otherwise reach KEXINIT as a zero-length name - Give ID_NONE a block size so the none cipher no longer divides by zero in BundlePacket(), and skip the zero-length key derivations it implies - Treat an empty peer server-sig-algs as no extension rather than a fatal parse error - The "at least one X" guards checked the implementation macros but not the soft-disable macros that gate joining the canned default list, so a build whose only survivors are soft-disabled got an empty default list: KEXINIT advertised nothing and CheckAlgoList() rejected it. Reachable as -DWOLFSSH_NO_HMAC_SHA2_256 -DWOLFSSH_NO_HMAC_SHA2_512 and as -DWOLFSSH_NO_AES_GCM -DWOLFSSH_NO_AES_CTR; both now fail at compile time naming the soft-disable macro to define - Zero MAC algorithms is already a hard #error, so assert the queried MAC list in test_wolfSSH_SetAlgoList() rather than returning early, which was hiding the KEX/KEY/CIPHER coverage behind an unreachable guard Issue: 2474
- Input buffer holds decrypted traffic incl. plaintext passwords - WS_FORCEZERO() dynamic contents before WFREE() - Applies to GrowBuffer (realloc) and ShrinkBuffer (release) Issue: 2495
- Widen SSH_PROTO_SZ to 8 so the compare covers the '-' that ends "SSH-2.0". A peer sending "SSH-2.01-x" matched the seven byte prefix and was accepted as an SSH 2.0 speaker. - The existing length guard now covers that byte as well. - A line ending right after "SSH-2.0", with no softwareversion, is rejected too. - Compare the peer's identification against a fixed "SSH-2.0-" instead of ctx->sshProtoIdStr, which the embedder can replace with wolfSSH_CTX_SetSshProtoIdStr(). The peer's protoversion is fixed by RFC 4253 4.2 and is not the local one. - Add unit tests. Issue: F-6975
- A caller size of zero wrapped to 0xFFFFFFFF in the subtraction that reserves the null char, driving a copy of that length and an out of bounds write past the buffer. - Reject it before the length is parsed, so a rejected call advances neither the parse index nor the caller's size. - Every in-tree caller passes a non-zero sizeof, so no released path reached the wrap. - Add a unit test. Issue: F-6819
- certmanLoadFile: use checked fseek(), not rewind(), whose only failure signal is errno and gets clobbered by the next malloc() - drop retainInstalled and dhInited; both were set and cleared with no intervening goto, so their cleanup branches were unreachable
aidangarske
reviewed
Jul 22, 2026
aidangarske
left a comment
aidangarske
left a comment
Member
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-security + bugsOverall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] [review-security+bugs] Opening none auth probe is counted when userauth-none support is compiled in —
src/internal.c:8000-8043 - [Medium] [bugs] Keyboard-interactive restarts only count when the next request is also keyboard —
src/internal.c:9790-9802 - [Medium] [review] Add positive coverage for --enable-none-cipher —
tests/api.c:4007-4010 - [Medium] [review] Auth-attempt cap tests only cover password failures —
tests/unit.c:3823-3864
Review generated by Skoll
JacobBarthelmeh
approved these changes
Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Zeroize input/output buffer contents before freeing -- decrypted traffic (incl. passwords) no longer left in freed heap.
Algo-list setters now validate each name and return WS_INVALID_ALGO_ID instead of storing it blindly; "none" cipher/MAC only under WOLFSSH_ALLOW_NONE_CIPHER. Builds whose default list would come out empty now fail at compile time naming the macro to define.
Server caps failed userauth attempts (CTX maxAuthAttempts, default 6), then disconnects.
DoProtoId compares the peer's identification against a fixed "SSH-2.0-" including the trailing '-', so "SSH-2.01-x" is no longer accepted as an SSH 2.0 speaker.
GetString rejects a zero-size destination, which wrapped the null-char subtraction to 0xFFFFFFFF and wrote past the buffer. No in-tree caller reached it.
Issues: F-2473, F-2474, F-2495, F-6819, F-6975