From 5bdfaf98e7ecc06a71a4add51b676b1959dcf60f Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 2 Sep 2026 11:50:13 -0700 Subject: [PATCH] internal: commit a session only once accepted A shell, exec or subsystem request changes the channel only once the callback accepts it. The session type and command are set for the callback to read and put back if it refuses, and CLIENT_DONE follows acceptance alone, so wolfSSH_accept() stays where it is rather than reporting a session it answered CHANNEL_FAILURE as established. - DoChannelRequestSession() carries the three arms, which differed only in the type and the callback consulted - a refusal puts the type and command back, so a grant an earlier request won still stands - FreeChannelCommand() wipes and releases a command line for both ChannelDelete() and the refusal path - unit.c drives a refused shell, exec and subsystem request through DoChannelRequest() and checks nothing was committed - regress.c checks accept() stays at ACCEPT_SERVER_CHANNEL_ACCEPT_SENT on a refused shell, and that neither divert runs Issue: F-8852 --- src/internal.c | 161 ++++++++++++++++++++++++--------------------- src/ssh.c | 5 +- tests/regress.c | 63 ++++++++++++++++-- tests/unit.c | 133 ++++++++++++++++++++++++++++++++++--- wolfssh/internal.h | 5 +- 5 files changed, 272 insertions(+), 95 deletions(-) diff --git a/src/internal.c b/src/internal.c index 644b080d2..229148b91 100644 --- a/src/internal.c +++ b/src/internal.c @@ -4173,6 +4173,18 @@ static void NotifyFwdLocalCleanup(WOLFSSH_CHANNEL* channel) #endif /* WOLFSSH_FWD */ +/* Wipe a command line, which can carry credentials, and free it. */ +static void FreeChannelCommand(void* heap, char* command, word32 commandSz) +{ + WOLFSSH_UNUSED(heap); + + if (command != NULL) { + WS_FORCEZERO(command, commandSz); + WFREE(command, heap, DYNTYPE_STRING); + } +} + + void ChannelDelete(WOLFSSH_CHANNEL* channel, void* heap) { WOLFSSH_UNUSED(heap); @@ -4200,11 +4212,7 @@ void ChannelDelete(WOLFSSH_CHANNEL* channel, void* heap) channel->channel); } ShrinkBuffer(&channel->extDataBuffer, 1); - /* Scrub the peer's command line, which can carry credentials. */ - if (channel->command != NULL) { - WS_FORCEZERO(channel->command, channel->commandSz); - WFREE(channel->command, heap, DYNTYPE_STRING); - } + FreeChannelCommand(heap, channel->command, channel->commandSz); WFREE(channel, heap, DYNTYPE_CHANNEL); } } @@ -13090,14 +13098,71 @@ static void SetTerminalSize(WOLFSSH* ssh, word32 widthChar, word32 heightRows, #endif /* WOLFSSH_TERM */ -/* Wipe the old command ahead of the GetStringAlloc() that frees it, so a - * repeat request leaves no credentials behind in the freed block. */ -static void ScrubChannelCommand(WOLFSSH_CHANNEL* channel) +/* Answers a shell, exec, or subsystem request. Sets the session type and + * command for the callback to read, and keeps them only if it accepts. */ +static int DoChannelRequestSession(WOLFSSH* ssh, word32 channelId, + WOLFSSH_CHANNEL* channel, byte sessionType, WS_CallbackChannelReq cb, + byte* buf, word32 len, word32* idx, int* rej) { - if (channel->command != NULL) { - WS_FORCEZERO(channel->command, channel->commandSz); - channel->commandSz = 0; + void* heap = ssh->ctx->heap; + byte prevType = channel->sessionType; + byte hasCommand = (sessionType != WOLFSSH_SESSION_SHELL); + char* prevCommand = NULL; + word32 prevCommandSz = 0; + char* command = NULL; + word32 commandSz = 0; + int ret = WS_SUCCESS; + + /* A shell request carries no command, so it leaves the old one alone. + * The others read into a local, so the old survives a refusal. */ + if (hasCommand) { + prevCommand = channel->command; + prevCommandSz = channel->commandSz; + + ret = GetStringAlloc(heap, &command, &commandSz, buf, len, idx); + if (ret == WS_SUCCESS) + WLOG(WS_LOG_DEBUG, " command = %s", command); + else + WLOG(WS_LOG_DEBUG, " command = %s", ""); } + + if (ret == WS_SUCCESS) { + if (hasCommand) { + channel->command = command; + channel->commandSz = commandSz; + } + channel->sessionType = sessionType; + + if (cb != NULL) + *rej = cb(channel, ssh->channelReqCtx); + else + *rej = ssh->appChannels; + + /* A callback may free its own channel, so look it up again. */ + channel = ChannelFind(ssh, channelId, WS_CHANNEL_ID_SELF); + if (channel == NULL) { + /* The new command went with it. */ + FreeChannelCommand(heap, prevCommand, prevCommandSz); + return ret; + } + } + + if (ret == WS_SUCCESS && !*rej) { + FreeChannelCommand(heap, prevCommand, prevCommandSz); + channel->sessionGranted = 1; + ssh->clientState = CLIENT_DONE; + } + else { + /* A refusal changes nothing, so an earlier grant still stands. */ + if (hasCommand) { + FreeChannelCommand(heap, command, commandSz); + channel->command = prevCommand; + channel->commandSz = prevCommandSz; + } + channel->sessionType = prevType; + } + + return ret; } @@ -13110,7 +13175,7 @@ static int DoChannelRequest(WOLFSSH* ssh, word32 typeSz; char type[32]; byte wantReply; - int ret, rej = 0, sessionReq = 0; + int ret, rej = 0; WLOG(WS_LOG_DEBUG, "Entering DoChannelRequest()"); @@ -13160,59 +13225,19 @@ static int DoChannelRequest(WOLFSSH* ssh, } } else if (ChannelRequestIs(type, typeSz, "shell")) { - channel->sessionType = WOLFSSH_SESSION_SHELL; - if (ssh->ctx->channelReqShellCb) { - rej = ssh->ctx->channelReqShellCb(channel, ssh->channelReqCtx); - } - else { - rej = ssh->appChannels; - } - sessionReq = 1; - ssh->clientState = CLIENT_DONE; + ret = DoChannelRequestSession(ssh, channelId, channel, + WOLFSSH_SESSION_SHELL, ssh->ctx->channelReqShellCb, + buf, len, &begin, &rej); } else if (ChannelRequestIs(type, typeSz, "exec")) { - ScrubChannelCommand(channel); - ret = GetStringAlloc(ssh->ctx->heap, - &channel->command, &channel->commandSz, - buf, len, &begin); - if (ret == WS_SUCCESS) - WLOG(WS_LOG_DEBUG, " command = %s", channel->command); - else - WLOG(WS_LOG_DEBUG, " command = %s", ""); - if (ret == WS_SUCCESS) { - channel->sessionType = WOLFSSH_SESSION_EXEC; - if (ssh->ctx->channelReqExecCb) { - rej = ssh->ctx->channelReqExecCb(channel, - ssh->channelReqCtx); - } - else { - rej = ssh->appChannels; - } - } - sessionReq = 1; - ssh->clientState = CLIENT_DONE; + ret = DoChannelRequestSession(ssh, channelId, channel, + WOLFSSH_SESSION_EXEC, ssh->ctx->channelReqExecCb, + buf, len, &begin, &rej); } else if (ChannelRequestIs(type, typeSz, "subsystem")) { - ScrubChannelCommand(channel); - ret = GetStringAlloc(ssh->ctx->heap, - &channel->command, &channel->commandSz, - buf, len, &begin); - if (ret == WS_SUCCESS) - WLOG(WS_LOG_DEBUG, " subsystem = %s", channel->command); - else - WLOG(WS_LOG_DEBUG, " subsystem = %s", ""); - if (ret == WS_SUCCESS) { - channel->sessionType = WOLFSSH_SESSION_SUBSYSTEM; - if (ssh->ctx->channelReqSubsysCb) { - rej = ssh->ctx->channelReqSubsysCb(channel, - ssh->channelReqCtx); - } - else { - rej = ssh->appChannels; - } - } - sessionReq = 1; - ssh->clientState = CLIENT_DONE; + ret = DoChannelRequestSession(ssh, channelId, channel, + WOLFSSH_SESSION_SUBSYSTEM, ssh->ctx->channelReqSubsysCb, + buf, len, &begin, &rej); } #ifdef WOLFSSH_TERM else if (ChannelRequestIs(type, typeSz, "pty-req")) { @@ -13347,20 +13372,6 @@ static int DoChannelRequest(WOLFSSH* ssh, *idx = len; } - /* Record the answer, not the ask: sessionType and command are set before - * the reject decision and stay set on a refusal, so they cannot say - * whether the session was granted. Set even without a wantReply, which - * changes only whether the peer is told. - * - * Look the channel up again rather than reusing the pointer from - * before the callback. A callback may close its own channel, and - * wolfSSH_ChannelFree() frees it, so the old pointer can be dead. */ - if (sessionReq) { - channel = ChannelFind(ssh, channelId, WS_CHANNEL_ID_SELF); - if (channel != NULL) - channel->sessionGranted = (ret == WS_SUCCESS && !rej); - } - if (wantReply) { int replyRet; diff --git a/src/ssh.c b/src/ssh.c index 4432e6c40..f6a5f20d7 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -810,9 +810,8 @@ int wolfSSH_accept(WOLFSSH* ssh) } } - /* Divert only into a granted session. The type and - * command stay set on a refusal, so they do not say - * what was granted. */ + /* Divert only into a granted session; a refusal puts + * the type and command back. */ #ifdef WOLFSSH_SCP if (ssh->channelList != NULL && ssh->channelList->sessionGranted diff --git a/tests/regress.c b/tests/regress.c index 49f4e1c55..c45c2899a 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -1692,6 +1692,58 @@ static void TestAppChannelsLateEnableReturns(void) FreeKexReplyHarness(&harness); } +/* Refuses the session request, and records what the channel showed. */ +static int rejectShellReqCalls; +static WS_SessionType rejectShellReqType; + +static int RejectShellReqCb(WOLFSSH_CHANNEL* channel, void* ctx) +{ + (void)ctx; + rejectShellReqCalls++; + rejectShellReqType = wolfSSH_ChannelGetSessionType(channel); + return 1; +} + +/* A shell request the callback refuses gets CHANNEL_FAILURE and nothing + * more: the channel keeps no session type, and accept() stays where it was, + * waiting on a request it can grant, rather than reporting an established + * session it just refused. */ +static void TestSessionReqRejectedKeepsAcceptWaiting(void) +{ + KexReplyHarness harness; + KexReplyRunResult result; + WOLFSSH_CHANNEL* channel; + WS_SessionType sessionType; + + rejectShellReqCalls = 0; + rejectShellReqType = WOLFSSH_SESSION_UNKNOWN; + + InitKexReplyHarness(&harness, "rsa-sha2-256", REGRESS_SERVER_KEY_PATH, + 0, NULL); + AssertIntEQ(wolfSSH_CTX_SetChannelReqShellCb(harness.serverCtx, + RejectShellReqCb), WS_SUCCESS); + + RunKexReplyHandshake(&harness, &result); + + AssertIntEQ(rejectShellReqCalls, 1); + AssertIntEQ(rejectShellReqType, WOLFSSH_SESSION_SHELL); + AssertFalse(result.clientSuccess); + AssertIntEQ(result.clientErr, WS_CHANOPEN_FAILED); + AssertFalse(result.serverSuccess); + AssertIntEQ(harness.server->acceptState, + ACCEPT_SERVER_CHANNEL_ACCEPT_SENT); + AssertTrue(harness.server->clientState < CLIENT_DONE); + sessionType = wolfSSH_GetSessionType(harness.server); + AssertIntEQ(sessionType, WOLFSSH_SESSION_UNKNOWN); + channel = wolfSSH_ChannelNext(harness.server, NULL); + AssertNotNull(channel); + AssertIntEQ(channel->sessionType, WOLFSSH_SESSION_UNKNOWN); + AssertFalse(harness.clientIo.sawDisconnect); + AssertFalse(harness.serverIo.sawDisconnect); + + FreeKexReplyHarness(&harness); +} + static void TestKexDhReplyRejectsRsaSha2_256SigNameDowngrade(void) { AssertHandshakeSucceeds("rsa-sha2-256", REGRESS_SERVER_KEY_PATH); @@ -4591,9 +4643,10 @@ static void CheckAcceptDivertNeedsSftpGrant(int reject) harness.ssh->acceptState = ACCEPT_SERVER_CHANNEL_ACCEPT_SENT; if (reject) { - AssertIntEQ(wolfSSH_accept(harness.ssh), WS_SUCCESS); + /* Short of CLIENT_DONE, so accept() diverts nowhere. */ + AssertIntEQ(wolfSSH_accept(harness.ssh), WS_FATAL_ERROR); AssertIntEQ(harness.ssh->acceptState, - ACCEPT_CLIENT_SESSION_ESTABLISHED); + ACCEPT_SERVER_CHANNEL_ACCEPT_SENT); } else { /* The control: the same name, granted, does reach the built-in @@ -4647,9 +4700,10 @@ static void CheckAcceptDivertNeedsScpGrant(int reject) harness.ssh->acceptState = ACCEPT_SERVER_CHANNEL_ACCEPT_SENT; if (reject) { - AssertIntEQ(wolfSSH_accept(harness.ssh), WS_SUCCESS); + /* Short of CLIENT_DONE, so accept() diverts nowhere. */ + AssertIntEQ(wolfSSH_accept(harness.ssh), WS_FATAL_ERROR); AssertIntEQ(harness.ssh->acceptState, - ACCEPT_CLIENT_SESSION_ESTABLISHED); + ACCEPT_SERVER_CHANNEL_ACCEPT_SENT); } else { AssertIntEQ(wolfSSH_accept(harness.ssh), WS_SCP_INIT); @@ -14961,6 +15015,7 @@ int main(int argc, char** argv) TestAppChannelsAcceptStopsAtUserAuth(); TestAppChannelsNoShellCbRejects(); TestAppChannelsLateEnableReturns(); + TestSessionReqRejectedKeepsAcceptWaiting(); TestKexDhReplyRejectsRsaSha2_256SigNameDowngrade(); #endif #ifndef WOLFSSH_NO_RSA_SHA2_512 diff --git a/tests/unit.c b/tests/unit.c index 918592553..0d6ca5edb 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -8760,6 +8760,23 @@ static int CaptureMsgId(const byte* buf, word32 len) * A custom IoSend callback captures the outgoing packet in plaintext * (no cipher negotiated on a fresh session). Message ID is read via * CaptureMsgId() using LENGTH_SZ + PAD_LENGTH_SZ. */ +/* A session request callback that refuses everything, and counts. The + * callback sees the session type and command of the request it is vetting; + * what it does not see is a session already committed to the channel. */ +static int s_rejectChanReqCalls; + +static int RejectChanReqCb(WOLFSSH_CHANNEL* channel, void* ctx) +{ + (void)ctx; + s_rejectChanReqCalls++; + if (channel == NULL + || wolfSSH_ChannelGetSessionType(channel) + == WOLFSSH_SESSION_UNKNOWN) { + return 0; + } + return 1; +} + static byte s_chanReqCapture[256]; static word32 s_chanReqCaptureSz = 0; @@ -9012,6 +9029,15 @@ static int test_DoChannelRequest(void) 0x00,0x00,0x00,0x02, /* cmdSz = 2 */ 0x6C,0x73 /* "ls" */ }; + static const byte paySubsys[] = { + 0x00,0x00,0x00,0x00, /* channelId = 0 */ + 0x00,0x00,0x00,0x09, /* typeSz = 9 */ + 0x73,0x75,0x62,0x73,0x79,0x73, + 0x74,0x65,0x6D, /* "subsystem" */ + 0x01, /* wantReply = 1 */ + 0x00,0x00,0x00,0x04, /* nameSz = 4 */ + 0x73,0x66,0x74,0x70 /* "sftp" */ + }; static const byte payUnknown[] = { 0x00,0x00,0x00,0x00, /* channelId = 0 */ 0x00,0x00,0x00,0x0C, /* typeSz = 12 */ @@ -9138,6 +9164,78 @@ static int test_DoChannelRequest(void) } } + /* A callback that refuses a shell, exec or subsystem request must leave + * nothing behind: no session type or command on the channel, and the + * client state short of CLIENT_DONE, or wolfSSH_accept() would go on to + * serve the session it just refused. */ + { + struct { + const char* label; + const byte* payload; + word32 payloadSz; + int errBase; + } rejCases[] = { + { "shell", payShell, (word32)sizeof(payShell), -520 }, + { "exec", payExec, (word32)sizeof(payExec), -525 }, + { "subsystem", paySubsys, (word32)sizeof(paySubsys), -530 } + }; + int r; + + wolfSSH_CTX_SetChannelReqShellCb(ctx, RejectChanReqCb); + wolfSSH_CTX_SetChannelReqExecCb(ctx, RejectChanReqCb); + wolfSSH_CTX_SetChannelReqSubsysCb(ctx, RejectChanReqCb); + + for (r = 0; r < (int)(sizeof(rejCases) / sizeof(rejCases[0])); r++) { + word32 idxRej = 0; + int retRej, capMsgId; + + s_chanReqCaptureSz = 0; + WMEMSET(s_chanReqCapture, 0, sizeof(s_chanReqCapture)); + s_rejectChanReqCalls = 0; + + retRej = wolfSSH_TestDoChannelRequest(ssh, + (byte*)rejCases[r].payload, rejCases[r].payloadSz, + &idxRej); + if (retRej != WS_SUCCESS) { + printf("DoChannelRequest[rej-%s]: ret=%d, expected=%d\n", + rejCases[r].label, retRej, WS_SUCCESS); + result = rejCases[r].errBase; + goto done; + } + if (s_rejectChanReqCalls != 1) { + printf("DoChannelRequest[rej-%s]: callback ran %d times\n", + rejCases[r].label, s_rejectChanReqCalls); + result = rejCases[r].errBase - 1; + goto done; + } + capMsgId = CaptureMsgId(s_chanReqCapture, s_chanReqCaptureSz); + if (capMsgId != (int)MSGID_CHANNEL_FAILURE) { + printf("DoChannelRequest[rej-%s]: msg_id=0x%02x, " + "expected=0x%02x\n", rejCases[r].label, capMsgId, + MSGID_CHANNEL_FAILURE); + result = rejCases[r].errBase - 2; + goto done; + } + if (ch->sessionType != WOLFSSH_SESSION_UNKNOWN + || ch->command != NULL) { + printf("DoChannelRequest[rej-%s]: session committed\n", + rejCases[r].label); + result = rejCases[r].errBase - 3; + goto done; + } + if (ssh->clientState == CLIENT_DONE) { + printf("DoChannelRequest[rej-%s]: client state changed\n", + rejCases[r].label); + result = rejCases[r].errBase - 4; + goto done; + } + } + + wolfSSH_CTX_SetChannelReqShellCb(ctx, NULL); + wolfSSH_CTX_SetChannelReqExecCb(ctx, NULL); + wolfSSH_CTX_SetChannelReqSubsysCb(ctx, NULL); + } + for (i = 0; i < (int)(sizeof(cases) / sizeof(cases[0])); i++) { word32 idx = 0; int ret; @@ -9175,6 +9273,32 @@ static int test_DoChannelRequest(void) } } + /* A shell request carries no command, so it must leave the one the + * exec above set alone rather than release it. */ + { + word32 idxShell = 0; + const char* cmd; + + if (wolfSSH_TestDoChannelRequest(ssh, (byte*)payShell, + (word32)sizeof(payShell), &idxShell) != WS_SUCCESS) { + printf("DoChannelRequest[shell-after-exec]: failed\n"); + result = -500; + goto done; + } + cmd = wolfSSH_ChannelGetSessionCommand(ch); + if (cmd == NULL || WSTRCMP(cmd, "ls") != 0) { + printf("DoChannelRequest[shell-after-exec]: command = %s\n", + cmd == NULL ? "(null)" : cmd); + result = -501; + goto done; + } + if (wolfSSH_ChannelGetSessionType(ch) != WOLFSSH_SESSION_SHELL) { + printf("DoChannelRequest[shell-after-exec]: type not shell\n"); + result = -502; + goto done; + } + } + /* RFC 4254 sec 6.10: exit-status and exit-signal must not send a reply * even if the wire wantReply byte is 1. DoChannelRequest overrides * wantReply=0 for these types, so no CHANNEL_SUCCESS/FAILURE packet @@ -9425,15 +9549,6 @@ static int test_DoChannelRequest(void) * accept() already returned there is nothing left to start a shell, * exec or subsystem, so all three are refused rather than accepted. */ { - static const byte paySubsys[] = { - 0x00,0x00,0x00,0x00, /* channelId = 0 */ - 0x00,0x00,0x00,0x09, /* typeSz = 9 */ - 0x73,0x75,0x62,0x73,0x79,0x73, - 0x74,0x65,0x6D, /* "subsystem" */ - 0x01, /* wantReply = 1 */ - 0x00,0x00,0x00,0x04, /* nameSz = 4 */ - 0x73,0x66,0x74,0x70 /* "sftp" */ - }; struct { const char* label; const byte* payload; diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 16c653251..d3b6a07aa 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1419,10 +1419,7 @@ struct WOLFSSH_CHANNEL { byte ptyReq : 1; /* flag for if interactive pty request was received */ byte fwdSetupTxd : 1; /* a LOCAL_SETUP succeeded, a cleanup is owed */ byte sessionGranted : 1; /* a shell, exec or subsystem request was - * answered CHANNEL_SUCCESS. sessionType and - * command are recorded before that answer is - * decided and stay set on a refusal, so they - * do not say whether anything was granted. */ + * answered CHANNEL_SUCCESS */ word32 channel; word32 windowSz; word32 maxPacketSz;