Bind userauth username to the first request#1063
Open
yosuke-wolfssl wants to merge 1 commit into
Open
Conversation
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1063
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
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.
Bind userauth username to the first request
Summary
Fixes finding f_5829. In keyboard-interactive (KBI) authentication, the
server bound the auth callback's username to whatever
ssh->userNamecurrentlyheld, not to the username that opened the exchange.
DoUserAuthRequestoverwrote
ssh->userNameon everyUSERAUTH_REQUEST, so a client couldpipeline a request for a second user while the first user's
INFO_REQUESTwasstill outstanding and have the callback run against the second username with the
pending challenge.
Root cause
DoUserAuthInfoResponsesources the username from the live session value:while
DoUserAuthRequestcalledwolfSSH_SetUsernameRaw(...)on every request,with nothing preventing a second
USERAUTH_REQUESTfrom being processed while aKBI
INFO_REQUESTwas pending. A pipelinedUSERAUTH_REQUEST(alice, kbi)thenUSERAUTH_REQUEST(bob, kbi)followed by anINFO_RESPONSEinvoked the callback asbobfor alice's exchange.This path is compiled only with
--enable-keyboard-interactive(
WOLFSSH_KEYBOARD_INTERACTIVE), which is off by default. Forpasswordandpublickeythe username and credential travel in the same packet, so only thesplit KBI flow was exploitable.
Fix
Match OpenSSH's model (
auth2.c,input_userauth_request): the username isbound by the first userauth request and stays fixed for the rest of the
authentication. A later request that changes it ends the session. With
ssh->userNameimmutable after the first request, the existingDoUserAuthInfoResponseassignment is always the originating user, so the swapis closed without KBI-specific state.
The per-request overwrite that previously ran after method dispatch is removed.
The new
byte userAuthSeenis appended to the end ofstruct WOLFSSHand iszero-initialized by the existing full-struct
WMEMSETinSshInit.Behavior
unaffected: a fresh
WOLFSSHstarts withuserAuthSeen = 0and bindsnormally.
unaffected, since the username does not change.
a disconnect, including the pipelined KBI swap.
Tests
Added regression coverage in
tests/regress.cdriving the server with theexisting in-memory transport harness (no sockets, threads, or real KEX):
TestUsernameChangeDisconnects(default build, password method): a secondrequest with a different username returns
WS_FATAL_ERROR, emitsMSGID_DISCONNECT, and never reaches the callback. Covers the guard in themost-shipped configuration.
TestKbUsernameChangeDisconnects(WOLFSSH_KEYBOARD_INTERACTIVE): thereported attack. alice's request emits an
INFO_REQUEST; bob's pipelinedrequest disconnects and never reaches the callback.
TestKbSameUserResponseSucceeds(WOLFSSH_KEYBOARD_INTERACTIVE): a normalsingle-user exchange still reaches the callback bound to the originating user.
Verification
-Werrorin both the default build and--enable-keyboard-interactive --enable-sftp.tests/regress.testandtests/api.testpass in both configurations;the existing
test_wolfSSH_KeyboardInteractivestill passes.fail (the second request is accepted), confirming they catch the bug.