Repository navigation
Harden password and keyboard-interactive auth - #1366
Open
ejohnstown wants to merge 3 commits into
Open
ejohnstown wants to merge 3 commits into
ejohnstown wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Keyboard-interactive authentication responses can still reach the authentication callback in cleartext after a rekey to none, requiring transport checks in both directions and a regression test for this sequence.
1 open issue
What changed in this PR
This PR hardens wolfSSH authentication against cleartext secret handling, packet-length disclosure, and timing differences in wolfsshd failures.
Changes:
- Rejects password and keyboard-interactive requests when either transport direction negotiates
none. - Pads client password and keyboard-interactive response packets to 64-byte boundaries.
- Adds per-user minimum failure delays in wolfsshd, with regression and unit coverage.
| File | Description |
|---|---|
| wolfssh/internal.h | Tracks negotiated none ciphers in both directions. |
| tests/unit.c | Tests secret-auth refusal over cleartext transports. |
| tests/regress.c | Checks secret-packet padding across transport modes. |
| src/internal.c | Adds cleartext-auth restrictions and secret-packet padding. |
| apps/wolfsshd/test/test_configuration.c | Tests failure-delay calculation and enforcement. |
| apps/wolfsshd/auth.h | Declares delay helpers and the test sleep hook. |
| apps/wolfsshd/auth.c | Implements keyed per-user delays for failed authentication. |
Review effort: Balanced
ejohnstown
force-pushed
the
userauth-hardening
branch
from
November 2, 2026 20:09
53a5b1a to
15b1669
Compare
With WOLFSSH_ALLOW_NONE_CIPHER, a server whose NEWKEYS installed the none cipher in either direction answers password and keyboard-interactive requests, and an INFO_RESPONSE after a rekey to none, with USERAUTH_FAILURE before the userauth callback sees a secret, and leaves both methods out of the method list. - DoNewKeys() and SendNewKeys() record a none cipher per direction - a refused request or response clears authId, so a later INFO_RESPONSE is rejected - unit tests drive both methods with each direction cleartext, and a rekey to none with a prompt outstanding Issue: F-10578
The client pads a password USERAUTH_REQUEST and a keyboard-interactive INFO_RESPONSE up to a multiple of 64 bytes, as OpenSSH does, so the packet length no longer tracks the secret's length to the cipher block. - BundlePacketPad() adds whole blocks of padding up to a bucket - regress test checks 1 to 64 byte secrets over the none cipher, AES-CTR with HMAC, and AES-GCM Issue: F-13390
DefaultUserAuth() holds a failed attempt to a per-user minimum, modeled on OpenSSH sshd: 5 ms plus up to 4.29 ms from a keyed hash of the username, doubled while the time already spent exceeds it, and skipped past 5 s. It sleeps and rechecks until the minimum has passed. The library itself stays non-blocking. - wolfSSHD_AuthInit() draws the per-daemon key before any fork - the hash is SHA-256, else SHA-512 or SHA-384; with none, the floor - Windows times the wait with QueryPerformanceCounter - tests for the delay range, the doubling, an early-waking sleep, and a failing request Issue: F-11938
ejohnstown
force-pushed
the
userauth-hardening
branch
from
November 2, 2026 20:51
15b1669 to
599807d
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1366
Scan targets checked: wolfssh-src, wolfssh-bugs
Not reviewed (no scan target covers them): apps/wolfsshd/auth.h
Coverage: 2 of 6 in-scope changed file(s) opened by the reviewer; not opened: apps/wolfsshd/test/test_configuration.c, tests/regress.c, tests/unit.c, wolfssh/internal.h
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
ejohnstown
requested review from
wolfSSL-Bot
and removed request for
wolfSSL-Bot
November 2, 2026 21:14
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.
Three changes keep userauth secrets from leaking through the transport
or through timing.
none cipher
on OpenSSH sshd
Issue: F-10578, F-13390, F-11938