From 65b1a521950d3a6065b34aa164ac176150d0e2e3 Mon Sep 17 00:00:00 2001 From: Yosuke Shimizu Date: Fri, 26 Jun 2026 15:01:13 +0900 Subject: [PATCH] Bind userauth username to the first request --- src/internal.c | 25 +++-- tests/regress.c | 226 +++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 1 + 3 files changed, 246 insertions(+), 6 deletions(-) diff --git a/src/internal.c b/src/internal.c index 96650a4f4..c06a5ded4 100644 --- a/src/internal.c +++ b/src/internal.c @@ -8668,6 +8668,25 @@ static int DoUserAuthRequest(WOLFSSH* ssh, } } + if (ret == WS_SUCCESS && serviceValid) { + /* Bind the username to the first userauth request. A later request + * that changes it ends the session, so a pipelined request cannot + * rebind a pending keyboard-interactive exchange to another user. */ + if (!ssh->userAuthSeen) { + ret = wolfSSH_SetUsernameRaw(ssh, + authData.username, authData.usernameSz); + if (ret == WS_SUCCESS) + ssh->userAuthSeen = 1; + } + else if (authData.usernameSz != ssh->userNameSz + || WMEMCMP(authData.username, ssh->userName, + authData.usernameSz) != 0) { + WLOG(WS_LOG_DEBUG, "DUAR: username change not allowed"); + (void)SendDisconnect(ssh, WOLFSSH_DISCONNECT_PROTOCOL_ERROR); + ret = WS_INVALID_STATE_E; + } + } + if (ret == WS_SUCCESS && serviceValid) { authNameId = NameToId((const char*)authData.authName, authData.authNameSz); ssh->authId = authNameId; @@ -8698,12 +8717,6 @@ static int DoUserAuthRequest(WOLFSSH* ssh, begin = len; } - if (ret == WS_SUCCESS) { - /* Set the username for valid service only */ - ret = wolfSSH_SetUsernameRaw(ssh, - authData.username, authData.usernameSz); - } - *idx = begin; } diff --git a/tests/regress.c b/tests/regress.c index 5d9e46cdb..d7310bd33 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -69,6 +69,10 @@ #define AssertNotNull(x) Assert((x), ("%s is not null", #x), (#x " => NULL")) #define AssertIntEQ(x, y) do { int _x = (int)(x); int _y = (int)(y); \ Assert(_x == _y, ("%s == %s", #x, #y), ("%d != %d", _x, _y)); } while (0) +#define AssertStrEQ(x, y) do { const char* _x = (const char*)(x); \ + const char* _y = (const char*)(y); \ + Assert(WSTRCMP(_x, _y) == 0, ("%s == %s", #x, #y), \ + ("\"%s\" != \"%s\"", _x, _y)); } while (0) static void ResetSession(WOLFSSH* ssh) @@ -1477,6 +1481,225 @@ static void TestSecondSessionChannelRejected(void) FreeChannelOpenHarness(&harness); } +/* Records the username the auth callback was invoked with, so a test can prove + * which identity a userauth request bound to. */ +static char authCbUserName[64]; +static word32 authCbUserNameSz; +static int authCbInvoked; + +static void ResetAuthCbRecord(void) +{ + WMEMSET(authCbUserName, 0, sizeof(authCbUserName)); + authCbUserNameSz = 0; + authCbInvoked = 0; +} + +static int RecordUserAuthCb(byte authType, WS_UserAuthData* authData, void* ctx) +{ + (void)ctx; +#ifndef WOLFSSH_KEYBOARD_INTERACTIVE + (void)authType; +#endif + +#ifdef WOLFSSH_KEYBOARD_INTERACTIVE + if (authType == WOLFSSH_USERAUTH_KEYBOARD_SETUP) { + static byte kbPrompt[] = "Password: "; + static byte* kbPrompts[1]; + static word32 kbPromptLengths[1]; + static byte kbPromptEcho[1]; + + kbPrompts[0] = kbPrompt; + kbPromptLengths[0] = (word32)sizeof(kbPrompt) - 1; + kbPromptEcho[0] = 0; + authData->sf.keyboard.promptCount = 1; + authData->sf.keyboard.prompts = kbPrompts; + authData->sf.keyboard.promptLengths = kbPromptLengths; + authData->sf.keyboard.promptEcho = kbPromptEcho; + authData->sf.keyboard.promptName = NULL; + authData->sf.keyboard.promptInstruction = NULL; + authData->sf.keyboard.promptLanguage = NULL; + return WOLFSSH_USERAUTH_SUCCESS; + } +#endif + + authCbInvoked = 1; + authCbUserNameSz = authData->usernameSz; + if (authCbUserNameSz >= (word32)sizeof(authCbUserName)) + authCbUserNameSz = (word32)sizeof(authCbUserName) - 1; + if (authData->username != NULL && authCbUserNameSz > 0) + WMEMCPY(authCbUserName, authData->username, authCbUserNameSz); + authCbUserName[authCbUserNameSz] = '\0'; + +#ifdef WOLFSSH_KEYBOARD_INTERACTIVE + if (authType == WOLFSSH_USERAUTH_KEYBOARD) + return WOLFSSH_USERAUTH_SUCCESS; +#endif + /* Reject other methods so the connection stays up for the next request + * without completing authentication. */ + return WOLFSSH_USERAUTH_INVALID_PASSWORD; +} + +static word32 BuildUserAuthPasswordRequest(const char* user, const char* pw, + byte* out, word32 outSz) +{ + byte payload[256]; + word32 idx = 0; + + idx = AppendString(payload, sizeof(payload), idx, user); + idx = AppendString(payload, sizeof(payload), idx, "ssh-connection"); + idx = AppendString(payload, sizeof(payload), idx, "password"); + idx = AppendByte(payload, sizeof(payload), idx, 0); /* not changing pw */ + idx = AppendString(payload, sizeof(payload), idx, pw); + + return WrapPacket(MSGID_USERAUTH_REQUEST, payload, idx, out, outSz); +} + +/* Drive a userauth request against a fresh in-memory server harness. The accept + * state is rewound to before userauth completes so USERAUTH_REQUEST and + * USERAUTH_INFO_RESPONSE are accepted by the message filter. */ +static void InitUserAuthHarness(ChannelOpenHarness* harness, + byte* in, word32 inSz) +{ + InitChannelOpenHarness(harness, in, inSz); + wolfSSH_SetUserAuth(harness->ctx, RecordUserAuthCb); + harness->ssh->acceptState = ACCEPT_SERVER_USERAUTH_ACCEPT_SENT; +} + +static void RepointHarnessInput(ChannelOpenHarness* harness, + byte* in, word32 inSz) +{ + harness->io.in = in; + harness->io.inSz = inSz; + harness->io.inOff = 0; + harness->io.outSz = 0; +} + +/* A username change after the first userauth request must end the session. */ +static void TestUsernameChangeDisconnects(void) +{ + ChannelOpenHarness harness; + byte inAlice[128]; + byte inBob[128]; + word32 inAliceSz; + word32 inBobSz; + + inAliceSz = BuildUserAuthPasswordRequest("alice", "pw", + inAlice, sizeof(inAlice)); + inBobSz = BuildUserAuthPasswordRequest("bob", "pw", inBob, sizeof(inBob)); + + ResetAuthCbRecord(); + InitUserAuthHarness(&harness, inAlice, inAliceSz); + + /* First request binds the username; callback sees alice and rejects. */ + AssertIntEQ(DoReceive(harness.ssh), WS_SUCCESS); + AssertStrEQ(authCbUserName, "alice"); + + /* Second request changes the username: session must be torn down and the + * callback must not be invoked as bob. */ + ResetAuthCbRecord(); + RepointHarnessInput(&harness, inBob, inBobSz); + AssertIntEQ(DoReceive(harness.ssh), WS_FATAL_ERROR); + AssertIntEQ(harness.ssh->error, WS_INVALID_STATE_E); + AssertTrue(harness.io.outSz > 0); + AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz), MSGID_DISCONNECT); + AssertIntEQ(authCbInvoked, 0); + + FreeChannelOpenHarness(&harness); +} + +#ifdef WOLFSSH_KEYBOARD_INTERACTIVE +static word32 BuildUserAuthKeyboardRequest(const char* user, + byte* out, word32 outSz) +{ + byte payload[256]; + word32 idx = 0; + + idx = AppendString(payload, sizeof(payload), idx, user); + idx = AppendString(payload, sizeof(payload), idx, "ssh-connection"); + idx = AppendString(payload, sizeof(payload), idx, "keyboard-interactive"); + idx = AppendString(payload, sizeof(payload), idx, ""); /* language tag */ + idx = AppendString(payload, sizeof(payload), idx, ""); /* submethods */ + + return WrapPacket(MSGID_USERAUTH_REQUEST, payload, idx, out, outSz); +} + +static word32 BuildUserAuthInfoResponse(const char* response, + byte* out, word32 outSz) +{ + byte payload[128]; + word32 idx = 0; + + idx = AppendUint32(payload, sizeof(payload), idx, 1); /* responseCount */ + idx = AppendString(payload, sizeof(payload), idx, response); + + return WrapPacket(MSGID_USERAUTH_INFO_RESPONSE, payload, idx, out, outSz); +} + +/* The reported attack: pipeline a keyboard-interactive request for alice then + * bob before answering. The bob request must end the session, not rebind the + * pending challenge. */ +static void TestKbUsernameChangeDisconnects(void) +{ + ChannelOpenHarness harness; + byte inAlice[128]; + byte inBob[128]; + word32 inAliceSz; + word32 inBobSz; + + inAliceSz = BuildUserAuthKeyboardRequest("alice", inAlice, sizeof(inAlice)); + inBobSz = BuildUserAuthKeyboardRequest("bob", inBob, sizeof(inBob)); + + ResetAuthCbRecord(); + InitUserAuthHarness(&harness, inAlice, inAliceSz); + + /* alice's request opens the challenge: server emits an INFO_REQUEST. */ + AssertIntEQ(DoReceive(harness.ssh), WS_SUCCESS); + AssertTrue(harness.io.outSz > 0); + AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz), + MSGID_USERAUTH_INFO_REQUEST); + AssertIntEQ(authCbInvoked, 0); /* setup only, no response yet */ + + /* bob's pipelined request must disconnect, never reach the callback. */ + ResetAuthCbRecord(); + RepointHarnessInput(&harness, inBob, inBobSz); + AssertIntEQ(DoReceive(harness.ssh), WS_FATAL_ERROR); + AssertIntEQ(harness.ssh->error, WS_INVALID_STATE_E); + AssertTrue(harness.io.outSz > 0); + AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz), MSGID_DISCONNECT); + AssertIntEQ(authCbInvoked, 0); + + FreeChannelOpenHarness(&harness); +} + +/* A single-user keyboard-interactive exchange still reaches the callback bound + * to the originating user. */ +static void TestKbSameUserResponseSucceeds(void) +{ + ChannelOpenHarness harness; + byte inReq[128]; + byte inResp[128]; + word32 inReqSz; + word32 inRespSz; + + inReqSz = BuildUserAuthKeyboardRequest("alice", inReq, sizeof(inReq)); + inRespSz = BuildUserAuthInfoResponse("secret", inResp, sizeof(inResp)); + + ResetAuthCbRecord(); + InitUserAuthHarness(&harness, inReq, inReqSz); + + AssertIntEQ(DoReceive(harness.ssh), WS_SUCCESS); + AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz), + MSGID_USERAUTH_INFO_REQUEST); + + RepointHarnessInput(&harness, inResp, inRespSz); + AssertIntEQ(DoReceive(harness.ssh), WS_SUCCESS); + AssertIntEQ(authCbInvoked, 1); + AssertStrEQ(authCbUserName, "alice"); + + FreeChannelOpenHarness(&harness); +} +#endif /* WOLFSSH_KEYBOARD_INTERACTIVE */ + #ifdef WOLFSSH_FWD static void TestDirectTcpipRejectSendsOpenFail(void) { @@ -4157,6 +4380,7 @@ int main(int argc, char** argv) TestChannelAllowedAfterAuth(ssh); TestChannelOpenCallbackRejectSendsOpenFail(); TestSecondSessionChannelRejected(); + TestUsernameChangeDisconnects(); #ifdef WOLFSSH_FWD TestDirectTcpipRejectSendsOpenFail(); TestDirectTcpipNoFwdCbSendsOpenFail(); @@ -4240,6 +4464,8 @@ int main(int argc, char** argv) TestKeyboardResponseNoUserAuthCallback(ssh, ctx); TestKeyboardResponseNullSsh(); TestKeyboardResponseNullCtx(ssh); + TestKbUsernameChangeDisconnects(); + TestKbSameUserResponseSucceeds(); #endif /* TODO: add app-level regressions that simulate stdin EOF/password diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 19fa55cd7..5e505dd5e 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -991,6 +991,7 @@ struct WOLFSSH { word32 testSftpSendCap; /* test hook: cap per-call SFTP buffer send */ word32 testSftpStallPending; /* test hook: force N flush-only resumes */ #endif + byte userAuthSeen; /* a userauth request has bound the username */ };