From aa95786858f7dcb086122451ba07c07ca23edc60 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 2 Sep 2026 09:47:15 -0700 Subject: [PATCH 01/11] ssh: add opt-in application-driven channels A server that wants to own its channels had no way to get them: accept() ran the session state machine to the end, and a shell, exec or subsystem request with no callback registered was granted regardless. - add wolfSSH_CTX_SetAppChannels() and wolfSSH_SetAppChannels(), off by default, a byte on the context copied into the session - on, accept() returns once the user is authenticated, and a session request with no callback behind it is refused: nothing is left to serve - keep the stop state out of the pending-send advance, so a re-entry with queued output cannot step over where this call is meant to stop - stop early only while the session is short of that state, so turning the mode on afterward cannot leave the loop hunting a state it went past - teach wolfSSH_SFTP_accept() that the mode parks accept() short of an established session, so it stops redoing the handshake on every poll --- src/internal.c | 12 ++++++++++- src/ssh.c | 54 +++++++++++++++++++++++++++++++++++++++++++--- src/wolfsftp.c | 8 +++++-- wolfssh/internal.h | 2 ++ wolfssh/ssh.h | 23 ++++++++++++++++++++ 5 files changed, 93 insertions(+), 6 deletions(-) diff --git a/src/internal.c b/src/internal.c index b2153e323..48d38a821 100644 --- a/src/internal.c +++ b/src/internal.c @@ -1781,6 +1781,7 @@ WOLFSSH* SshInit(WOLFSSH* ssh, WOLFSSH_CTX* ctx) ssh->highwaterMark = ctx->highwaterMark; ssh->msgHighwaterMark = ctx->msgHighwaterMark; ssh->maxAuthAttempts = ctx->maxAuthAttempts; + ssh->appChannels = ctx->appChannels; ssh->highwaterCtx = (void*)ssh; ssh->reqSuccessCtx = (void*)ssh; ssh->fs = NULL; @@ -13130,6 +13131,9 @@ static int DoChannelRequest(WOLFSSH* ssh, if (ssh->ctx->channelReqShellCb) { rej = ssh->ctx->channelReqShellCb(channel, ssh->channelReqCtx); } + else { + rej = ssh->appChannels; + } ssh->clientState = CLIENT_DONE; } else if (ChannelRequestIs(type, typeSz, "exec")) { @@ -13139,6 +13143,9 @@ static int DoChannelRequest(WOLFSSH* ssh, if (ssh->ctx->channelReqExecCb) { rej = ssh->ctx->channelReqExecCb(channel, ssh->channelReqCtx); } + else { + rej = ssh->appChannels; + } ssh->clientState = CLIENT_DONE; WLOG(WS_LOG_DEBUG, " command = %s", channel->command); @@ -13150,6 +13157,9 @@ static int DoChannelRequest(WOLFSSH* ssh, if (ssh->ctx->channelReqSubsysCb) { rej = ssh->ctx->channelReqSubsysCb(channel, ssh->channelReqCtx); } + else { + rej = ssh->appChannels; + } ssh->clientState = CLIENT_DONE; WLOG(WS_LOG_DEBUG, " subsystem = %s", channel->command); @@ -13291,7 +13301,7 @@ static int DoChannelRequest(WOLFSSH* ssh, int replyRet; if (rej) { - WLOG(WS_LOG_DEBUG, "Callback rejecting channel request."); + WLOG(WS_LOG_DEBUG, "Rejecting channel request."); } replyRet = SendChannelSuccess(ssh, channelId, (ret == WS_SUCCESS && !rej)); diff --git a/src/ssh.c b/src/ssh.c index 83f41d762..f27d1c1b0 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -624,6 +624,8 @@ const char acceptState[] = "accept state: %s"; int wolfSSH_accept(WOLFSSH* ssh) { + byte stopState; + WLOG(WS_LOG_DEBUG, "Entering wolfSSH_accept()"); if (ssh == NULL) @@ -643,6 +645,15 @@ int wolfSSH_accept(WOLFSSH* ssh) return WS_INVALID_STATE_E; } + /* In application-driven mode the state machine stops as soon as the + * user is authenticated; everything past that is the application's. + * Only stop there if the session has not already gone by: the loop + * below tests the stop state exactly, so a state it has stepped over + * would never terminate it. */ + stopState = (ssh->appChannels + && ssh->acceptState <= ACCEPT_SERVER_USERAUTH_SENT) ? + ACCEPT_SERVER_USERAUTH_SENT : ACCEPT_CLIENT_SESSION_ESTABLISHED; + /* check if data pending to be sent */ if (ssh->outputBuffer.length > 0 && ssh->acceptState < ACCEPT_CLIENT_SESSION_ESTABLISHED) { @@ -654,7 +665,11 @@ int wolfSSH_accept(WOLFSSH* ssh) ssh->acceptState != ACCEPT_SERVER_USERAUTH_ACCEPT_SENT && ssh->acceptState != ACCEPT_SERVER_KEXINIT_SENT && ssh->acceptState != ACCEPT_KEYED && - ssh->acceptState != ACCEPT_SERVER_CHANNEL_ACCEPT_SENT) { + ssh->acceptState != ACCEPT_SERVER_CHANNEL_ACCEPT_SENT && + /* Never step over where this call is meant to stop. The + * loop below tests for that state exactly, and the SCP and + * SFTP re-entry states sort after it. */ + ssh->acceptState != stopState) { WLOG(WS_LOG_DEBUG, "Advancing accept state"); ssh->acceptState++; } @@ -676,7 +691,7 @@ int wolfSSH_accept(WOLFSSH* ssh) } } - while (ssh->acceptState != ACCEPT_CLIENT_SESSION_ESTABLISHED) { + while (ssh->acceptState != stopState) { switch (ssh->acceptState) { case ACCEPT_BEGIN: @@ -766,6 +781,12 @@ int wolfSSH_accept(WOLFSSH* ssh) } ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; WLOG(WS_LOG_DEBUG, acceptState, "SERVER_USERAUTH_SENT"); + if (stopState == ACCEPT_SERVER_USERAUTH_SENT) { + /* The application takes it from here. Tested through + * stopState so a callback that changed the flag during + * this call cannot half-apply it. */ + break; + } FALL_THROUGH; case ACCEPT_SERVER_USERAUTH_SENT: @@ -4772,7 +4793,8 @@ WOLFSSH_CHANNEL* wolfSSH_ChannelFwdNewRemote(WOLFSSH* ssh, if (newChannel != NULL) ChannelAppend(ssh, newChannel); - WLOG(WS_LOG_DEBUG, "Leaving wolfSSH_ChannelFwdNewRemote(), newChannel = %p, ret = %d", + WLOG(WS_LOG_DEBUG, + "Leaving wolfSSH_ChannelFwdNewRemote(), newChannel = %p, ret = %d", newChannel, ret); return newChannel; } @@ -5766,6 +5788,32 @@ int wolfSSH_CTX_SetChannelReqSubsysCb(WOLFSSH_CTX* ctx, } +int wolfSSH_CTX_SetAppChannels(WOLFSSH_CTX* ctx, byte enable) +{ + int ret = WS_SSH_CTX_NULL_E; + + if (ctx != NULL) { + ctx->appChannels = (enable != 0); + ret = WS_SUCCESS; + } + + return ret; +} + + +int wolfSSH_SetAppChannels(WOLFSSH* ssh, byte enable) +{ + int ret = WS_SSH_NULL_E; + + if (ssh != NULL) { + ssh->appChannels = (enable != 0); + ret = WS_SUCCESS; + } + + return ret; +} + + int wolfSSH_SetChannelOpenCtx(WOLFSSH* ssh, void* ctx) { int ret = WS_SSH_NULL_E; diff --git a/src/wolfsftp.c b/src/wolfsftp.c index 88cca98f8..1b7d93cf1 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -1383,8 +1383,12 @@ int wolfSSH_SFTP_accept(WOLFSSH* ssh) if (ssh->error == WS_WANT_READ || ssh->error == WS_WANT_WRITE) ssh->error = WS_SUCCESS; - /* check accept is done, if not call wolfSSH accept */ - if (ssh->acceptState < ACCEPT_CLIENT_SESSION_ESTABLISHED) { + /* check accept is done, if not call wolfSSH accept. In + * application-driven mode accept() parks at ACCEPT_SERVER_USERAUTH_SENT + * and never advances, so that state counts as done here. */ + if (ssh->acceptState < ACCEPT_CLIENT_SESSION_ESTABLISHED + && !(ssh->appChannels + && ssh->acceptState >= ACCEPT_SERVER_USERAUTH_SENT)) { byte name[] = "sftp"; WLOG(WS_LOG_SFTP, "Trying to do SSH accept first"); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index be5905afd..6d8598fd1 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -898,6 +898,7 @@ struct WOLFSSH_CTX { word32 maxAuthAttempts; /* server cap on failed userauth */ byte side; /* client or server */ byte showBanner; + byte appChannels; /* app drives channels, see ssh.h */ #ifdef WOLFSSH_AGENT byte agentEnabled; #endif /* WOLFSSH_AGENT */ @@ -1167,6 +1168,7 @@ struct WOLFSSH { byte serverState; byte processReplyState; byte isKeying; + byte appChannels; /* app drives channels, see ssh.h */ byte authId; /* if using public key or password */ byte supportedAuth[4]; /* supported auth IDs public key , password */ diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index f768fe9e7..7a1548c60 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -461,6 +461,29 @@ WOLFSSH_API int wolfSSH_CTX_SetChannelReqSubsysCb(WOLFSSH_CTX* ctx, WOLFSSH_API int wolfSSH_SetChannelReqCtx(WOLFSSH* ssh, void* ctx); WOLFSSH_API void* wolfSSH_GetChannelReqCtx(WOLFSSH* ssh); +/* Application-driven channel handling, server side, off by default. + * + * Off, wolfSSH_accept() runs the session state machine through to an + * established session with the first channel open, as it always has, and a + * shell, exec, or subsystem request with no callback registered for it is + * accepted. + * + * On, wolfSSH_accept() returns WS_SUCCESS as soon as the user has + * authenticated, and the application owns every channel from there, driving + * the session with wolfSSH_worker() and the callbacks above. A shell, exec, + * or subsystem request with no callback registered is then rejected: with + * accept() already returned, nothing is left to service it. + * + * Set it on the context before wolfSSH_new(), or on a session before the + * first wolfSSH_accept() call. Turning it on once accept() has established + * the session has no effect on that session. + * + * The mode drives the session channels itself, so it does not combine with + * the built-in wolfSSH_SFTP_accept() and WS_SCP_INIT entry points; an + * application using those leaves this off. */ +WOLFSSH_API int wolfSSH_CTX_SetAppChannels(WOLFSSH_CTX* ctx, byte enable); +WOLFSSH_API int wolfSSH_SetAppChannels(WOLFSSH* ssh, byte enable); + typedef int (*WS_CallbackChannelEof)(WOLFSSH_CHANNEL* channel, void* ctx); WOLFSSH_API int wolfSSH_CTX_SetChannelEofCb(WOLFSSH_CTX* ctx, WS_CallbackChannelEof cb); From f0a5b7054bcf542678a292deb7b6b228fd4285fe Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 2 Sep 2026 09:49:31 -0700 Subject: [PATCH 02/11] tests: cover application-driven channels wolfSSH_SetAppChannels() changes where wolfSSH_accept() stops and what becomes of a session request with no callback behind it, so both modes are exercised. - regress.c drives a server with the pivot on, one with a shell callback and one without, and checks accept() stops at ACCEPT_SERVER_USERAUTH_SENT - regress.c pins the context setter, the session's inheritance of it, and that turning it on after accept() established the session still returns - regress.c re-enters a parked accept() with output still queued, which is the one path that flushes before reading the state, and pins that it leaves the state on the stop - unit.c checks DoChannelRequest() refuses a shell, exec and subsystem request with no callback once the pivot is on --- tests/regress.c | 217 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/unit.c | 62 ++++++++++++++ 2 files changed, 279 insertions(+) diff --git a/tests/regress.c b/tests/regress.c index c8c3016de..7d9ec3459 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -1486,6 +1486,185 @@ static void AssertHandshakeRejectsMutatedReply(const char* keyAlgo, } #ifndef WOLFSSH_NO_RSA_SHA2_256 +/* Counts the shell requests the application-driven server answered. */ +static int appChannelsShellReqCount; + +static int AppChannelsShellCb(WOLFSSH_CHANNEL* channel, void* ctx) +{ + (void)channel; + (void)ctx; + appChannelsShellReqCount++; + return 0; +} + +/* Drive an application-driven server: wolfSSH_accept() is expected to return + * at userauth, so the channel open and the shell request are answered by + * wolfSSH_worker() calls the application makes itself. */ +static void RunAppChannelsHandshake(KexReplyHarness* harness, + KexReplyRunResult* result) +{ + word32 step; + + WMEMSET(result, 0, sizeof(*result)); + result->clientRet = WS_FATAL_ERROR; + result->serverRet = WS_FATAL_ERROR; + + for (step = 0; step < REGRESS_MAX_HANDSHAKE_STEPS; step++) { + if (!result->clientSuccess) { + result->clientRet = wolfSSH_connect(harness->client); + result->clientErr = wolfSSH_get_error(harness->client); + if (result->clientRet == WS_SUCCESS) { + result->clientSuccess = 1; + } + else if (!IsHandshakeRetryable(result->clientErr)) { + result->steps = step + 1; + return; + } + } + + if (!result->serverSuccess) { + result->serverRet = wolfSSH_accept(harness->server); + result->serverErr = wolfSSH_get_error(harness->server); + if (result->serverRet == WS_SUCCESS) { + result->serverSuccess = 1; + } + else if (!IsHandshakeRetryable(result->serverErr)) { + result->steps = step + 1; + return; + } + } + else if (harness->server->clientState < CLIENT_DONE) { + result->serverRet = wolfSSH_worker(harness->server, NULL); + result->serverErr = wolfSSH_get_error(harness->server); + if (result->serverRet < WS_SUCCESS + && result->serverErr != WS_CHAN_RXD + && !IsHandshakeRetryable(result->serverErr)) { + result->steps = step + 1; + return; + } + } + + if (result->clientSuccess && result->serverSuccess + && harness->server->clientState >= CLIENT_DONE) { + result->steps = step + 1; + return; + } + } + + result->steps = REGRESS_MAX_HANDSHAKE_STEPS; +} + +/* With wolfSSH_SetAppChannels() on, accept() stops once the user is + * authenticated and the shell request lands on the callback instead. */ +static void TestAppChannelsAcceptStopsAtUserAuth(void) +{ + KexReplyHarness harness; + KexReplyRunResult result; + + appChannelsShellReqCount = 0; + + InitKexReplyHarness(&harness, "rsa-sha2-256", REGRESS_SERVER_KEY_PATH, + 0, NULL); + AssertIntEQ(wolfSSH_CTX_SetChannelReqShellCb(harness.serverCtx, + AppChannelsShellCb), WS_SUCCESS); + AssertIntEQ(wolfSSH_SetAppChannels(harness.server, 1), WS_SUCCESS); + + RunAppChannelsHandshake(&harness, &result); + + AssertTrue(result.clientSuccess); + AssertTrue(result.serverSuccess); + AssertIntEQ(harness.server->acceptState, ACCEPT_SERVER_USERAUTH_SENT); + AssertIntEQ(harness.server->clientState, CLIENT_DONE); + AssertIntEQ(appChannelsShellReqCount, 1); + AssertIntEQ(harness.client->connectState, + CONNECT_SERVER_CHANNEL_REQUEST_DONE); + AssertFalse(harness.clientIo.sawDisconnect); + AssertFalse(harness.serverIo.sawDisconnect); + + FreeKexReplyHarness(&harness); +} + +/* Same mode, no callback registered: nothing can start the shell once + * accept() has returned, so the request is refused. The default mode + * accepts it, which AssertHandshakeSucceeds() covers. */ +static void TestAppChannelsNoShellCbRejects(void) +{ + KexReplyHarness harness; + KexReplyRunResult result; + + InitKexReplyHarness(&harness, "rsa-sha2-256", REGRESS_SERVER_KEY_PATH, + 0, NULL); + AssertIntEQ(wolfSSH_SetAppChannels(harness.server, 1), WS_SUCCESS); + + RunAppChannelsHandshake(&harness, &result); + + /* RunAppChannelsHandshake() also leaves clientSuccess clear when it + * runs out of steps with neither side erroring, so pin the refusal + * itself: the client stopped early, and for the right reason. */ + AssertFalse(result.clientSuccess); + AssertTrue(result.steps < REGRESS_MAX_HANDSHAKE_STEPS); + AssertIntEQ(result.clientErr, WS_CHANOPEN_FAILED); + AssertTrue(harness.client->connectState < + CONNECT_SERVER_CHANNEL_REQUEST_DONE); + AssertIntEQ(harness.server->acceptState, ACCEPT_SERVER_USERAUTH_SENT); + + FreeKexReplyHarness(&harness); +} + +/* The flag is documented as a context setting first, so pin the setter + * returns and the inheritance wolfSSH_new() does. */ +static void TestAppChannelsCtxInherits(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + + AssertIntEQ(wolfSSH_CTX_SetAppChannels(NULL, 1), WS_SSH_CTX_NULL_E); + AssertIntEQ(wolfSSH_SetAppChannels(NULL, 1), WS_SSH_NULL_E); + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + AssertNotNull(ctx); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AssertIntEQ(ssh->appChannels, 0); + wolfSSH_free(ssh); + + AssertIntEQ(wolfSSH_CTX_SetAppChannels(ctx, 1), WS_SUCCESS); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AssertIntEQ(ssh->appChannels, 1); + AssertIntEQ(wolfSSH_SetAppChannels(ssh, 0), WS_SUCCESS); + AssertIntEQ(ssh->appChannels, 0); + wolfSSH_free(ssh); + + wolfSSH_CTX_free(ctx); +} + +/* Turning the mode on after accept() established the session must not leave + * the accept loop hunting for a state it has already stepped past. */ +static void TestAppChannelsLateEnableReturns(void) +{ + KexReplyHarness harness; + KexReplyRunResult result; + + InitKexReplyHarness(&harness, "rsa-sha2-256", REGRESS_SERVER_KEY_PATH, + 0, NULL); + + RunKexReplyHandshake(&harness, &result); + + AssertTrue(result.serverSuccess); + AssertIntEQ(harness.server->acceptState, + ACCEPT_CLIENT_SESSION_ESTABLISHED); + + AssertIntEQ(wolfSSH_SetAppChannels(harness.server, 1), WS_SUCCESS); + AssertIntEQ(wolfSSH_accept(harness.server), WS_SUCCESS); + AssertIntEQ(harness.server->acceptState, + ACCEPT_CLIENT_SESSION_ESTABLISHED); + + FreeKexReplyHarness(&harness); +} + static void TestKexDhReplyRejectsRsaSha2_256SigNameDowngrade(void) { AssertHandshakeSucceeds("rsa-sha2-256", REGRESS_SERVER_KEY_PATH); @@ -3530,6 +3709,39 @@ static void TestChannelReqSubsysCallbackRuns(void) WOLFSSH_SESSION_SUBSYSTEM), MSGID_CHANNEL_FAILURE); } +/* accept() re-entered while it is already parked, with a reply still + * queued, has to flush and stay put. Stepping the state on from here + * would put the stop behind it, and the loop tests for that state + * exactly, so the session would run on to established instead. */ +static void TestAppChannelsAcceptKeepsStopWithPendingOutput(void) +{ + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + + InitChannelOpenHarness(&harness, NULL, 0); + AssertIntEQ(wolfSSH_SetAppChannels(harness.ssh, 1), WS_SUCCESS); + + channel = SeedUnconfirmedChannel(&harness); + AssertIntEQ(ChannelUpdatePeer(channel, 5, 1024, 1024), WS_SUCCESS); + channel->openConfirmed = 1; + + /* A blocked send leaves the channel data queued. Nothing here drives + * a channel request, so clientState stays short of CLIENT_DONE and a + * state stepped past the stop fails the accept below rather than + * spinning in it. */ + harness.io.blockNext = 1; + AssertIntEQ(wolfSSH_stream_send(harness.ssh, (byte*)"x", 1), 1); + AssertTrue(harness.ssh->outputBuffer.length > 0); + AssertTrue(harness.ssh->clientState < CLIENT_DONE); + + AssertIntEQ(harness.ssh->acceptState, ACCEPT_SERVER_USERAUTH_SENT); + AssertIntEQ(wolfSSH_accept(harness.ssh), WS_SUCCESS); + AssertIntEQ(harness.ssh->acceptState, ACCEPT_SERVER_USERAUTH_SENT); + AssertIntEQ(harness.ssh->outputBuffer.length, 0); + + FreeChannelOpenHarness(&harness); +} + /* A username change after the first userauth request must end the session. */ static void TestUsernameChangeDisconnects(void) { @@ -13575,6 +13787,7 @@ int main(int argc, char** argv) TestChannelCloseCallbackReturnIgnored(); TestChannelReqExecCallbackRuns(); TestChannelReqSubsysCallbackRuns(); + TestAppChannelsAcceptKeepsStopWithPendingOutput(); TestSecondSessionChannelRejected(); TestUsernameChangeDisconnects(); TestSameUserRetryAllowed(); @@ -13780,6 +13993,10 @@ int main(int argc, char** argv) #ifdef KEXDH_REPLY_REGRESS_KEX_ALGO #ifndef WOLFSSH_NO_RSA_SHA2_256 + TestAppChannelsCtxInherits(); + TestAppChannelsAcceptStopsAtUserAuth(); + TestAppChannelsNoShellCbRejects(); + TestAppChannelsLateEnableReturns(); TestKexDhReplyRejectsRsaSha2_256SigNameDowngrade(); #endif #ifndef WOLFSSH_NO_RSA_SHA2_512 diff --git a/tests/unit.c b/tests/unit.c index f67c84084..3e67cbe3b 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -9295,6 +9295,68 @@ static int test_DoChannelRequest(void) } #endif /* WOLFSSH_SHELL && WOLFSSH_TERM */ + /* Application-driven channels flip the no-callback default: with + * 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; + word32 payloadSz; + int errBase; + } appCases[] = { + { "shell", payShell, (word32)sizeof(payShell), -495 }, + { "exec", payExec, (word32)sizeof(payExec), -497 }, + { "subsystem", paySubsys, (word32)sizeof(paySubsys), -499 } + }; + int a; + + for (a = 0; a < (int)(sizeof(appCases) / sizeof(appCases[0])); a++) { + word32 idxApp = 0; + int retApp, capMsgId; + + if (wolfSSH_SetAppChannels(ssh, 1) != WS_SUCCESS) { + printf("DoChannelRequest[app-%s]: set failed\n", + appCases[a].label); + result = appCases[a].errBase; + goto done; + } + + s_chanReqCaptureSz = 0; + WMEMSET(s_chanReqCapture, 0, sizeof(s_chanReqCapture)); + + retApp = wolfSSH_TestDoChannelRequest(ssh, + (byte*)appCases[a].payload, appCases[a].payloadSz, + &idxApp); + wolfSSH_SetAppChannels(ssh, 0); + + if (retApp != WS_SUCCESS) { + printf("DoChannelRequest[app-%s]: ret=%d, expected=%d\n", + appCases[a].label, retApp, WS_SUCCESS); + result = appCases[a].errBase; + goto done; + } + + capMsgId = CaptureMsgId(s_chanReqCapture, s_chanReqCaptureSz); + if (capMsgId != (int)MSGID_CHANNEL_FAILURE) { + printf("DoChannelRequest[app-%s]: msg_id=0x%02x, " + "expected=0x%02x\n", appCases[a].label, capMsgId, + MSGID_CHANNEL_FAILURE); + result = appCases[a].errBase - 1; + goto done; + } + } + } + done: wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); From b9c18534ad82bb62ee782cb8b7bb6a64db435b8f Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 3 Sep 2026 22:20:29 -0700 Subject: [PATCH 03/11] ssh: correct what a late app-channels enable does DoChannelRequest() reads ssh->appChannels when the request arrives, so turning the mode on after accept() established the session still refuses an uncallbacked shell, exec or subsystem request from then on. Only accept()'s stopping point is pinned, by the guard around stopState. - say the flag reaches the requests that follow, and that what it cannot do is move where accept() returns - drive a shell request over the wire in both modes from the late-enable test, pinning the behaviour the header now describes --- tests/regress.c | 25 ++++++++++++++++++++++++- wolfssh/ssh.h | 5 +++-- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/tests/regress.c b/tests/regress.c index 7d9ec3459..736bf4777 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -1642,11 +1642,21 @@ static void TestAppChannelsCtxInherits(void) } /* Turning the mode on after accept() established the session must not leave - * the accept loop hunting for a state it has already stepped past. */ + * the accept loop hunting for a state it has already stepped past. The flag + * still reaches DoChannelRequest() from there, which is what ssh.h promises, + * so pin both halves: accept() stays put, the requests that follow flip. */ static void TestAppChannelsLateEnableReturns(void) { KexReplyHarness harness; KexReplyRunResult result; + /* SSH_MSG_CHANNEL_REQUEST body: channel 0, "shell", wantReply. */ + static byte payShell[] = { + 0x00,0x00,0x00,0x00, /* channelId = 0 */ + 0x00,0x00,0x00,0x05, /* typeSz = 5 */ + 0x73,0x68,0x65,0x6C,0x6C, /* "shell" */ + 0x01 /* wantReply = 1 */ + }; + word32 idx; InitKexReplyHarness(&harness, "rsa-sha2-256", REGRESS_SERVER_KEY_PATH, 0, NULL); @@ -1657,11 +1667,24 @@ static void TestAppChannelsLateEnableReturns(void) AssertIntEQ(harness.server->acceptState, ACCEPT_CLIENT_SESSION_ESTABLISHED); + /* Default mode, no callback registered: the request is granted. */ + idx = 0; + AssertIntEQ(wolfSSH_TestDoChannelRequest(harness.server, payShell, + (word32)sizeof(payShell), &idx), WS_SUCCESS); + AssertIntEQ(wolfSSH_worker(harness.client, NULL), WS_SUCCESS); + AssertIntEQ(wolfSSH_SetAppChannels(harness.server, 1), WS_SUCCESS); AssertIntEQ(wolfSSH_accept(harness.server), WS_SUCCESS); AssertIntEQ(harness.server->acceptState, ACCEPT_CLIENT_SESSION_ESTABLISHED); + /* Same request, same session, mode now on: refused instead. */ + idx = 0; + AssertIntEQ(wolfSSH_TestDoChannelRequest(harness.server, payShell, + (word32)sizeof(payShell), &idx), WS_SUCCESS); + AssertTrue(wolfSSH_worker(harness.client, NULL) < WS_SUCCESS); + AssertIntEQ(wolfSSH_get_error(harness.client), WS_CHANOPEN_FAILED); + FreeKexReplyHarness(&harness); } diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 7a1548c60..6f3f348cc 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -475,8 +475,9 @@ WOLFSSH_API void* wolfSSH_GetChannelReqCtx(WOLFSSH* ssh); * accept() already returned, nothing is left to service it. * * Set it on the context before wolfSSH_new(), or on a session before the - * first wolfSSH_accept() call. Turning it on once accept() has established - * the session has no effect on that session. + * first wolfSSH_accept() call. Turning it on later still applies to the + * channel requests that follow, but it cannot move where accept() returns + * on a session that has already gone past the user-auth stop. * * The mode drives the session channels itself, so it does not combine with * the built-in wolfSSH_SFTP_accept() and WS_SCP_INIT entry points; an From e35200d2752730e1dd1ef2bfe835ab649630fcb8 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Sat, 5 Sep 2026 13:01:58 -0700 Subject: [PATCH 04/11] sftp: serve app-channels only on a granted sftp In application-driven mode wolfSSH_accept() parks at userauth, so the sftp test its divert applies never runs. wolfSSH_SFTP_accept() applies it itself: the session channel must be a subsystem the application's callback granted sftp on, or the call returns WS_INVALID_STATE_E and leaves the wire alone without recording an error. - gate the app-channels branch on wolfSSH_GetSessionType() and wolfSSH_GetSessionCommand(), the same test accept() makes - ask for that grant in every accept state: below the user-auth stop accept() returns with no channel open, and past the stop there is no accept() left that could have checked anything - say in ssh.h that the mode serves SFTP through that grant and never reaches the SCP entry point - regress.c refuses the call with no channel, ahead of accept(), on a granted shell and on an established one, and serves an INIT on a granted sftp subsystem --- src/wolfsftp.c | 26 ++++-- tests/regress.c | 211 +++++++++++++++++++++++++++++++++++++++++++++++- wolfssh/ssh.h | 8 +- 3 files changed, 234 insertions(+), 11 deletions(-) diff --git a/src/wolfsftp.c b/src/wolfsftp.c index 1b7d93cf1..8a2705193 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -1383,12 +1383,26 @@ int wolfSSH_SFTP_accept(WOLFSSH* ssh) if (ssh->error == WS_WANT_READ || ssh->error == WS_WANT_WRITE) ssh->error = WS_SUCCESS; - /* check accept is done, if not call wolfSSH accept. In - * application-driven mode accept() parks at ACCEPT_SERVER_USERAUTH_SENT - * and never advances, so that state counts as done here. */ - if (ssh->acceptState < ACCEPT_CLIENT_SESSION_ESTABLISHED - && !(ssh->appChannels - && ssh->acceptState >= ACCEPT_SERVER_USERAUTH_SENT)) { + /* The grant is what says this session may be served, so it is asked + * for in every accept state. Below the user-auth stop the legacy + * branch would run the handshake itself, which in this mode returns + * with no channel open at all; at the stop or past it there is no + * accept() left that could have checked anything. */ + if (ssh->appChannels) { + /* Application-driven mode parks accept() here for good, so the + * sftp grant it would have checked is the application's subsystem + * callback: serve only a session channel it granted sftp on. Same + * test as wolfSSH_accept()'s divert. */ + const char* cmd = wolfSSH_GetSessionCommand(ssh); + + if (wolfSSH_GetSessionType(ssh) != WOLFSSH_SESSION_SUBSYSTEM + || cmd == NULL || WSTRNCMP(cmd, "sftp", 4) != 0) { + WLOG(WS_LOG_SFTP, "No sftp subsystem granted on the session"); + return WS_INVALID_STATE_E; + } + } + /* check accept is done, if not call wolfSSH accept */ + else if (ssh->acceptState < ACCEPT_CLIENT_SESSION_ESTABLISHED) { byte name[] = "sftp"; WLOG(WS_LOG_SFTP, "Trying to do SSH accept first"); diff --git a/tests/regress.c b/tests/regress.c index 736bf4777..da56174ba 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -3612,7 +3612,8 @@ static void TestChannelCloseCallbackReturnIgnored(void) } /* Builds a plaintext SSH_MSG_CHANNEL_REQUEST whose type-specific tail is a - * single string, which is the shape of both "exec" and "subsystem". */ + * single string, which is the shape of both "exec" and "subsystem". A NULL + * "arg" leaves the tail off, which is the shape of "shell". */ static word32 BuildChannelStringRequestPacket(word32 recipientChannelId, const char* type, byte wantReply, const char* arg, byte* out, word32 outSz) @@ -3623,7 +3624,9 @@ static word32 BuildChannelStringRequestPacket(word32 recipientChannelId, idx = AppendUint32(payload, sizeof(payload), idx, recipientChannelId); idx = AppendString(payload, sizeof(payload), idx, type); idx = AppendByte(payload, sizeof(payload), idx, wantReply); - idx = AppendString(payload, sizeof(payload), idx, arg); + if (arg != NULL) { + idx = AppendString(payload, sizeof(payload), idx, arg); + } return WrapPacket(MSGID_CHANNEL_REQUEST, payload, idx, out, outSz); } @@ -3765,6 +3768,202 @@ static void TestAppChannelsAcceptKeepsStopWithPendingOutput(void) FreeChannelOpenHarness(&harness); } +#ifdef WOLFSSH_SFTP +/* SSH_MSG_CHANNEL_DATA carrying an SFTP INIT, version 3. */ +static word32 BuildSftpInitDataPacket(word32 recipientChannelId, byte* out, + word32 outSz) +{ + static const byte init[] = { + 0x00,0x00,0x00,0x05, /* length */ + WOLFSSH_FTP_INIT, + 0x00,0x00,0x00,0x03 /* version = 3 */ + }; + byte payload[32]; + word32 idx = 0; + + idx = AppendUint32(payload, sizeof(payload), idx, recipientChannelId); + idx = AppendUint32(payload, sizeof(payload), idx, (word32)sizeof(init)); + idx = AppendData(payload, sizeof(payload), idx, init, sizeof(init)); + + return WrapPacket(MSGID_CHANNEL_DATA, payload, idx, out, outSz); +} + +/* An application-driven server with a confirmed session channel, its request + * callback for type registered to grant, and one request of that type driven + * through it. Returns the channel; the harness input is left empty. */ +static WOLFSSH_CHANNEL* SeedAppChannelsSession(ChannelOpenHarness* harness, + const char* type, const char* arg) +{ + WOLFSSH_CHANNEL* channel; + byte in[128]; + word32 inSz; + + sessionReqCbCalls = 0; + sessionReqCbReturn = 0; + + InitChannelOpenHarness(harness, NULL, 0); + AssertIntEQ(wolfSSH_SetAppChannels(harness->ssh, 1), WS_SUCCESS); + if (WSTRCMP(type, "shell") == 0) { + AssertIntEQ(wolfSSH_CTX_SetChannelReqShellCb(harness->ctx, + RecordingSessionReqCb), WS_SUCCESS); + } + else { + AssertIntEQ(wolfSSH_CTX_SetChannelReqSubsysCb(harness->ctx, + RecordingSessionReqCb), WS_SUCCESS); + } + + channel = SeedUnconfirmedChannel(harness); + AssertIntEQ(ChannelUpdatePeer(channel, 5, 1024, 1024), WS_SUCCESS); + channel->openConfirmed = 1; + + inSz = BuildChannelStringRequestPacket(channel->channel, type, 1, arg, + in, sizeof(in)); + RepointHarnessInput(harness, in, inSz); + AssertIntEQ(DoReceive(harness->ssh), WS_SUCCESS); + AssertIntEQ(sessionReqCbCalls, 1); + AssertIntEQ(ParseMsgId(harness->io.out, harness->io.outSz), + MSGID_CHANNEL_SUCCESS); + RepointHarnessInput(harness, NULL, 0); + + return channel; +} + +/* wolfSSH_SFTP_accept() in application-driven mode. accept() parks short of + * the session, so the sftp grant it would have checked is the application's + * subsystem callback: with no session channel there is nothing to serve. */ +static void TestSftpAcceptAppChannelsNeedsSession(void) +{ + ChannelOpenHarness harness; + + InitChannelOpenHarness(&harness, NULL, 0); + AssertIntEQ(wolfSSH_SetAppChannels(harness.ssh, 1), WS_SUCCESS); + + AssertIntEQ(wolfSSH_SFTP_accept(harness.ssh), WS_INVALID_STATE_E); + AssertIntEQ(harness.io.outSz, 0); + AssertIntEQ(harness.ssh->error, WS_SUCCESS); + AssertIntEQ(harness.ssh->acceptState, ACCEPT_SERVER_USERAUTH_SENT); + + FreeChannelOpenHarness(&harness); +} + +/* Called ahead of accept(), which is how a server that set the flag on the + * context reaches this entry point. The refusal has to come from the gate: + * running the handshake instead returns with no channel open in this mode, + * and the SFTP exchange then fails on the missing channel. */ +static void TestSftpAcceptAppChannelsRefusesPreAccept(void) +{ + ChannelOpenHarness harness; + + InitChannelOpenHarness(&harness, NULL, 0); + AssertIntEQ(wolfSSH_SetAppChannels(harness.ssh, 1), WS_SUCCESS); + harness.ssh->acceptState = ACCEPT_BEGIN; + + AssertIntEQ(wolfSSH_SFTP_accept(harness.ssh), WS_INVALID_STATE_E); + /* Nothing sent, so no handshake was started ... */ + AssertIntEQ(harness.io.outSz, 0); + AssertIntEQ(harness.ssh->acceptState, ACCEPT_BEGIN); + /* ... and the subsystem name the legacy branch sets was not set. */ + AssertNull(harness.ssh->channelName); + AssertIntEQ(harness.ssh->error, WS_SUCCESS); + + FreeChannelOpenHarness(&harness); +} + +/* A granted shell is not an sftp grant: the INIT the peer pushes on that + * channel stays unread. */ +static void TestSftpAcceptAppChannelsRefusesShell(void) +{ + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + byte in[64]; + word32 inSz; + + channel = SeedAppChannelsSession(&harness, "shell", NULL); + inSz = BuildSftpInitDataPacket(channel->channel, in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + + AssertIntEQ(wolfSSH_SFTP_accept(harness.ssh), WS_INVALID_STATE_E); + AssertIntEQ(harness.io.outSz, 0); + AssertIntEQ(harness.io.inOff, 0); + AssertIntEQ(harness.ssh->error, WS_SUCCESS); + + FreeChannelOpenHarness(&harness); +} + +/* The grant the mode relies on: the subsystem callback took sftp, so the + * INIT is answered with a VERSION and accept() stays parked. */ +static void TestSftpAcceptAppChannelsServesGrantedSftp(void) +{ + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + byte in[64]; + word32 inSz; + /* Offset of the SFTP type byte in the packet the server sends: the + * SSH packet header, then the CHANNEL_DATA payload of recipient + * channel and data-string length, then the SFTP length field. */ + const word32 sftpIdx = LENGTH_SZ + PAD_LENGTH_SZ + MSG_ID_SZ + + UINT32_SZ + UINT32_SZ + UINT32_SZ; + + channel = SeedAppChannelsSession(&harness, "subsystem", "sftp"); + inSz = BuildSftpInitDataPacket(channel->channel, in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + + AssertIntEQ(wolfSSH_SFTP_accept(harness.ssh), WS_SFTP_COMPLETE); + AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz), + MSGID_CHANNEL_DATA); + AssertTrue(harness.io.outSz > sftpIdx); + AssertIntEQ(harness.io.out[sftpIdx], WOLFSSH_FTP_VERSION); + AssertIntEQ(harness.ssh->acceptState, ACCEPT_SERVER_USERAUTH_SENT); + + FreeChannelOpenHarness(&harness); +} + +/* An established session is gated too. A server that turns the mode on + * late is past everything accept() would have checked, so the grant is + * the only thing left saying what the channel is: a granted shell is not + * an sftp grant, whatever state accept() finished in. */ +static void TestSftpAcceptAppChannelsRefusesEstablishedShell(void) +{ + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + byte in[64]; + word32 inSz; + + channel = SeedAppChannelsSession(&harness, "shell", NULL); + harness.ssh->acceptState = ACCEPT_CLIENT_SESSION_ESTABLISHED; + inSz = BuildSftpInitDataPacket(channel->channel, in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + + AssertIntEQ(wolfSSH_SFTP_accept(harness.ssh), WS_INVALID_STATE_E); + AssertIntEQ(harness.io.outSz, 0); + AssertIntEQ(harness.io.inOff, 0); + AssertIntEQ(harness.ssh->error, WS_SUCCESS); + + FreeChannelOpenHarness(&harness); +} + +/* The other half of that: asking in every state must not refuse a session + * the callback did grant sftp on, wherever accept() left off. */ +static void TestSftpAcceptAppChannelsServesEstablishedSftp(void) +{ + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + byte in[64]; + word32 inSz; + + channel = SeedAppChannelsSession(&harness, "subsystem", "sftp"); + harness.ssh->acceptState = ACCEPT_CLIENT_SESSION_ESTABLISHED; + inSz = BuildSftpInitDataPacket(channel->channel, in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + + AssertIntEQ(wolfSSH_SFTP_accept(harness.ssh), WS_SFTP_COMPLETE); + AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz), + MSGID_CHANNEL_DATA); + + FreeChannelOpenHarness(&harness); +} +#endif /* WOLFSSH_SFTP */ + /* A username change after the first userauth request must end the session. */ static void TestUsernameChangeDisconnects(void) { @@ -13811,6 +14010,14 @@ int main(int argc, char** argv) TestChannelReqExecCallbackRuns(); TestChannelReqSubsysCallbackRuns(); TestAppChannelsAcceptKeepsStopWithPendingOutput(); +#ifdef WOLFSSH_SFTP + TestSftpAcceptAppChannelsNeedsSession(); + TestSftpAcceptAppChannelsRefusesPreAccept(); + TestSftpAcceptAppChannelsRefusesShell(); + TestSftpAcceptAppChannelsServesGrantedSftp(); + TestSftpAcceptAppChannelsRefusesEstablishedShell(); + TestSftpAcceptAppChannelsServesEstablishedSftp(); +#endif TestSecondSessionChannelRejected(); TestUsernameChangeDisconnects(); TestSameUserRetryAllowed(); diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 6f3f348cc..815a1a486 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -479,9 +479,11 @@ WOLFSSH_API void* wolfSSH_GetChannelReqCtx(WOLFSSH* ssh); * channel requests that follow, but it cannot move where accept() returns * on a session that has already gone past the user-auth stop. * - * The mode drives the session channels itself, so it does not combine with - * the built-in wolfSSH_SFTP_accept() and WS_SCP_INIT entry points; an - * application using those leaves this off. */ + * accept() never reaches the built-in SCP entry point in this mode, so + * WS_SCP_INIT is off the table. wolfSSH_SFTP_accept() still serves, but only + * a session channel the subsystem callback granted sftp on; called ahead of + * that it returns WS_INVALID_STATE_E without recording an error. A pending + * want-read or want-write is still cleared, as on any other call. */ WOLFSSH_API int wolfSSH_CTX_SetAppChannels(WOLFSSH_CTX* ctx, byte enable); WOLFSSH_API int wolfSSH_SetAppChannels(WOLFSSH* ssh, byte enable); From e2ea55240d7eb281984db98fdb526e7a8a5ec22d Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 8 Sep 2026 11:22:43 -0700 Subject: [PATCH 05/11] sftp: require a granted subsystem to serve wolfSSH_SFTP_accept() serves an application-driven session only on a channel whose subsystem request was answered CHANNEL_SUCCESS. DoChannelRequest() records the session type and command before it decides, and leaves both set on a refusal, so they cannot say by themselves whether anything was granted. - add channel->sessionGranted, set from the answer a shell, exec or subsystem request gets rather than from the request arriving - look the channel up again before recording it: a callback may close its own channel, and wolfSSH_ChannelFree() frees it - log a request's strings where they are known good: once the parse has succeeded, and ahead of a callback that may free the channel - gate the app-channels path on that flag alongside the session type and the command - cover a refusal from both sides, no callback registered and a callback that rejects, and a callback that frees its channel --- src/internal.c | 42 ++++++++++++++---- src/wolfsftp.c | 16 ++++--- tests/regress.c | 107 +++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 5 +++ 4 files changed, 156 insertions(+), 14 deletions(-) diff --git a/src/internal.c b/src/internal.c index 48d38a821..9c1f69174 100644 --- a/src/internal.c +++ b/src/internal.c @@ -13081,7 +13081,7 @@ static int DoChannelRequest(WOLFSSH* ssh, word32 typeSz; char type[32]; byte wantReply; - int ret, rej = 0; + int ret, rej = 0, sessionReq = 0; WLOG(WS_LOG_DEBUG, "Entering DoChannelRequest()"); @@ -13121,10 +13121,15 @@ static int DoChannelRequest(WOLFSSH* ssh, nameSz = (word32)sizeof(name); valueSz = (word32)sizeof(value); ret = GetString(name, &nameSz, buf, len, &begin); - if (ret == WS_SUCCESS) + if (ret != WS_SUCCESS) + WLOG(WS_LOG_DEBUG, " name = %s", ""); + else { ret = GetString(value, &valueSz, buf, len, &begin); - - WLOG(WS_LOG_DEBUG, " %s = %s", name, value); + if (ret != WS_SUCCESS) + WLOG(WS_LOG_DEBUG, " %s = %s", name, ""); + else + WLOG(WS_LOG_DEBUG, " %s = %s", name, value); + } } else if (ChannelRequestIs(type, typeSz, "shell")) { channel->sessionType = WOLFSSH_SESSION_SHELL; @@ -13134,11 +13139,16 @@ static int DoChannelRequest(WOLFSSH* ssh, else { rej = ssh->appChannels; } + sessionReq = 1; ssh->clientState = CLIENT_DONE; } else if (ChannelRequestIs(type, typeSz, "exec")) { ret = GetStringAlloc(ssh->ctx->heap, &channel->command, NULL, buf, len, &begin); + if (ret == WS_SUCCESS) + WLOG(WS_LOG_DEBUG, " command = %s", channel->command); + else + WLOG(WS_LOG_DEBUG, " command = %s", ""); channel->sessionType = WOLFSSH_SESSION_EXEC; if (ssh->ctx->channelReqExecCb) { rej = ssh->ctx->channelReqExecCb(channel, ssh->channelReqCtx); @@ -13146,13 +13156,16 @@ static int DoChannelRequest(WOLFSSH* ssh, else { rej = ssh->appChannels; } + sessionReq = 1; ssh->clientState = CLIENT_DONE; - - WLOG(WS_LOG_DEBUG, " command = %s", channel->command); } else if (ChannelRequestIs(type, typeSz, "subsystem")) { ret = GetStringAlloc(ssh->ctx->heap, &channel->command, NULL, buf, len, &begin); + if (ret == WS_SUCCESS) + WLOG(WS_LOG_DEBUG, " subsystem = %s", channel->command); + else + WLOG(WS_LOG_DEBUG, " subsystem = %s", ""); channel->sessionType = WOLFSSH_SESSION_SUBSYSTEM; if (ssh->ctx->channelReqSubsysCb) { rej = ssh->ctx->channelReqSubsysCb(channel, ssh->channelReqCtx); @@ -13160,9 +13173,8 @@ static int DoChannelRequest(WOLFSSH* ssh, else { rej = ssh->appChannels; } + sessionReq = 1; ssh->clientState = CLIENT_DONE; - - WLOG(WS_LOG_DEBUG, " subsystem = %s", channel->command); } #ifdef WOLFSSH_TERM else if (ChannelRequestIs(type, typeSz, "pty-req")) { @@ -13297,6 +13309,20 @@ 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/wolfsftp.c b/src/wolfsftp.c index 8a2705193..fc0cc784f 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -1391,12 +1391,16 @@ int wolfSSH_SFTP_accept(WOLFSSH* ssh) if (ssh->appChannels) { /* Application-driven mode parks accept() here for good, so the * sftp grant it would have checked is the application's subsystem - * callback: serve only a session channel it granted sftp on. Same - * test as wolfSSH_accept()'s divert. */ - const char* cmd = wolfSSH_GetSessionCommand(ssh); - - if (wolfSSH_GetSessionType(ssh) != WOLFSSH_SESSION_SUBSYSTEM - || cmd == NULL || WSTRNCMP(cmd, "sftp", 4) != 0) { + * callback: serve only a session channel it granted sftp on. The + * request having named sftp is not enough, so this asks for the + * grant as well -- unlike wolfSSH_accept()'s divert, which reads + * only the type and command. */ + const WOLFSSH_CHANNEL* channel = ssh->channelList; + + if (channel == NULL || !channel->sessionGranted + || channel->sessionType != WOLFSSH_SESSION_SUBSYSTEM + || channel->command == NULL + || WSTRNCMP(channel->command, "sftp", 4) != 0) { WLOG(WS_LOG_SFTP, "No sftp subsystem granted on the session"); return WS_INVALID_STATE_E; } diff --git a/tests/regress.c b/tests/regress.c index da56174ba..bd7fa0ef9 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -3735,6 +3735,53 @@ static void TestChannelReqSubsysCallbackRuns(void) WOLFSSH_SESSION_SUBSYSTEM), MSGID_CHANNEL_FAILURE); } +/* A request callback owns its channel and may close it. The grant is + * recorded after the callback returns, so it has to find the channel + * again: wolfSSH_ChannelFree() frees it, and writing through the old + * pointer would touch freed memory. */ +static int freeChannelCbCalls; + +static int FreeingSessionReqCb(WOLFSSH_CHANNEL* channel, void* ctx) +{ + (void)ctx; + freeChannelCbCalls++; + AssertIntEQ(wolfSSH_ChannelFree(channel), WS_SUCCESS); + return 0; +} + +static void TestSessionReqCallbackMayFreeChannel(void) +{ + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + byte in[128]; + word32 inSz; + + freeChannelCbCalls = 0; + + InitChannelOpenHarness(&harness, NULL, 0); + AssertIntEQ(wolfSSH_CTX_SetChannelReqShellCb(harness.ctx, + FreeingSessionReqCb), WS_SUCCESS); + + channel = SeedUnconfirmedChannel(&harness); + AssertIntEQ(ChannelUpdatePeer(channel, 5, 1024, 1024), WS_SUCCESS); + channel->openConfirmed = 1; + + inSz = BuildChannelStringRequestPacket(channel->channel, "shell", 1, + NULL, in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + AssertIntEQ(DoReceive(harness.ssh), WS_FATAL_ERROR); + + AssertIntEQ(freeChannelCbCalls, 1); + /* The channel the grant would have been recorded on is gone, so the + * reply cannot be sent either and the session says why. */ + AssertIntEQ(harness.ssh->channelListSz, 0); + AssertNull(harness.ssh->channelList); + AssertIntEQ(wolfSSH_get_error(harness.ssh), WS_INVALID_CHANID); + AssertIntEQ(harness.io.outSz, 0); + + FreeChannelOpenHarness(&harness); +} + /* accept() re-entered while it is already parked, with a reply still * queued, has to flush and stay put. Stepping the state on from here * would put the stop behind it, and the loop tests for that state @@ -3918,6 +3965,7 @@ static void TestSftpAcceptAppChannelsServesGrantedSftp(void) FreeChannelOpenHarness(&harness); } + /* An established session is gated too. A server that turns the mode on * late is past everything accept() would have checked, so the grant is * the only thing left saying what the channel is: a granted shell is not @@ -3962,6 +4010,62 @@ static void TestSftpAcceptAppChannelsServesEstablishedSftp(void) FreeChannelOpenHarness(&harness); } + +/* A refused "subsystem sftp" still leaves sessionType/command set on the + * channel, so check wolfSSH_SFTP_accept() looks at the grant, not the + * leftovers. rejectVia 0 registers no callback at all (app channels alone + * refuse); 1 registers one that rejects. */ +static void CheckSftpAcceptRefusesUngranted(int rejectVia) +{ + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + byte in[128]; + word32 inSz; + + sessionReqCbCalls = 0; + sessionReqCbReturn = (rejectVia == 0) ? 0 : 1; + + InitChannelOpenHarness(&harness, NULL, 0); + AssertIntEQ(wolfSSH_SetAppChannels(harness.ssh, 1), WS_SUCCESS); + if (rejectVia != 0) { + AssertIntEQ(wolfSSH_CTX_SetChannelReqSubsysCb(harness.ctx, + RecordingSessionReqCb), WS_SUCCESS); + } + + channel = SeedUnconfirmedChannel(&harness); + AssertIntEQ(ChannelUpdatePeer(channel, 5, 1024, 1024), WS_SUCCESS); + channel->openConfirmed = 1; + + inSz = BuildChannelStringRequestPacket(channel->channel, "subsystem", 1, + "sftp", in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + AssertIntEQ(DoReceive(harness.ssh), WS_SUCCESS); + /* Either way the peer is told the subsystem was refused. */ + AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz), + MSGID_CHANNEL_FAILURE); + + inSz = BuildSftpInitDataPacket(channel->channel, in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + + AssertIntEQ(wolfSSH_SFTP_accept(harness.ssh), WS_INVALID_STATE_E); + AssertIntEQ(harness.io.outSz, 0); + + FreeChannelOpenHarness(&harness); +} + + +static void TestSftpAcceptAppChannelsRefusesNoCb(void) +{ + CheckSftpAcceptRefusesUngranted(0); +} + + +static void TestSftpAcceptAppChannelsRefusesRejectedCb(void) +{ + CheckSftpAcceptRefusesUngranted(1); +} + + #endif /* WOLFSSH_SFTP */ /* A username change after the first userauth request must end the session. */ @@ -14009,6 +14113,7 @@ int main(int argc, char** argv) TestChannelCloseCallbackReturnIgnored(); TestChannelReqExecCallbackRuns(); TestChannelReqSubsysCallbackRuns(); + TestSessionReqCallbackMayFreeChannel(); TestAppChannelsAcceptKeepsStopWithPendingOutput(); #ifdef WOLFSSH_SFTP TestSftpAcceptAppChannelsNeedsSession(); @@ -14017,6 +14122,8 @@ int main(int argc, char** argv) TestSftpAcceptAppChannelsServesGrantedSftp(); TestSftpAcceptAppChannelsRefusesEstablishedShell(); TestSftpAcceptAppChannelsServesEstablishedSftp(); + TestSftpAcceptAppChannelsRefusesNoCb(); + TestSftpAcceptAppChannelsRefusesRejectedCb(); #endif TestSecondSessionChannelRejected(); TestUsernameChangeDisconnects(); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 6d8598fd1..20d63e5a5 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1413,6 +1413,11 @@ struct WOLFSSH_CHANNEL { byte openConfirmed : 1; 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. */ word32 channel; word32 windowSz; word32 maxPacketSz; From 851aad29543e2dfc39b053ea4a8ee2ef77160827 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 9 Sep 2026 11:50:29 -0700 Subject: [PATCH 06/11] ssh, sftp: match the sftp subsystem name exactly The built-in SFTP server takes a session only when the subsystem name is sftp, matched whole. DoChannelRequest() keeps the parsed length in channel->commandSz, so neither wolfSSH_SFTP_accept()'s grant gate nor wolfSSH_accept()'s divert serves "sftpx" or "sftp\0evil". - cover a granted name longer than sftp, one of its length, and one running past an embedded NUL - cover the divert with those three names and a control that diverts - exec keeps its command length too Issue: F-11665 --- src/internal.c | 6 +- src/ssh.c | 4 +- src/wolfsftp.c | 7 +- tests/regress.c | 161 +++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 1 + 5 files changed, 174 insertions(+), 5 deletions(-) diff --git a/src/internal.c b/src/internal.c index 9c1f69174..fb9b90ac3 100644 --- a/src/internal.c +++ b/src/internal.c @@ -13143,7 +13143,8 @@ static int DoChannelRequest(WOLFSSH* ssh, ssh->clientState = CLIENT_DONE; } else if (ChannelRequestIs(type, typeSz, "exec")) { - ret = GetStringAlloc(ssh->ctx->heap, &channel->command, NULL, + ret = GetStringAlloc(ssh->ctx->heap, + &channel->command, &channel->commandSz, buf, len, &begin); if (ret == WS_SUCCESS) WLOG(WS_LOG_DEBUG, " command = %s", channel->command); @@ -13160,7 +13161,8 @@ static int DoChannelRequest(WOLFSSH* ssh, ssh->clientState = CLIENT_DONE; } else if (ChannelRequestIs(type, typeSz, "subsystem")) { - ret = GetStringAlloc(ssh->ctx->heap, &channel->command, NULL, + ret = GetStringAlloc(ssh->ctx->heap, + &channel->command, &channel->commandSz, buf, len, &begin); if (ret == WS_SUCCESS) WLOG(WS_LOG_DEBUG, " subsystem = %s", channel->command); diff --git a/src/ssh.c b/src/ssh.c index f27d1c1b0..7592792b8 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -822,7 +822,9 @@ int wolfSSH_accept(WOLFSSH* ssh) const char* cmd = wolfSSH_GetSessionCommand(ssh); if (cmd != NULL && WOLFSSH_SESSION_SUBSYSTEM == wolfSSH_GetSessionType(ssh) - && (WSTRNCMP(cmd, "sftp", 4) == 0)) { + && ssh->channelList->commandSz == + (word32)WSTRLEN("sftp") + && (WSTRCMP(cmd, "sftp") == 0)) { ssh->acceptState = ACCEPT_INIT_SFTP; return wolfSSH_SFTP_accept(ssh); } diff --git a/src/wolfsftp.c b/src/wolfsftp.c index fc0cc784f..70a27ed95 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -1394,13 +1394,16 @@ int wolfSSH_SFTP_accept(WOLFSSH* ssh) * callback: serve only a session channel it granted sftp on. The * request having named sftp is not enough, so this asks for the * grant as well -- unlike wolfSSH_accept()'s divert, which reads - * only the type and command. */ + * only the type and command. The name matches whole, length + * and bytes: sftpx, or sftp with an embedded NUL, is some + * other subsystem. */ const WOLFSSH_CHANNEL* channel = ssh->channelList; if (channel == NULL || !channel->sessionGranted || channel->sessionType != WOLFSSH_SESSION_SUBSYSTEM || channel->command == NULL - || WSTRNCMP(channel->command, "sftp", 4) != 0) { + || channel->commandSz != (word32)WSTRLEN("sftp") + || WSTRCMP(channel->command, "sftp") != 0) { WLOG(WS_LOG_SFTP, "No sftp subsystem granted on the session"); return WS_INVALID_STATE_E; } diff --git a/tests/regress.c b/tests/regress.c index bd7fa0ef9..4fcc8e71f 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -3937,6 +3937,100 @@ static void TestSftpAcceptAppChannelsRefusesShell(void) FreeChannelOpenHarness(&harness); } +/* A name that only starts with sftp is not an sftp grant. */ +static void TestSftpAcceptAppChannelsRefusesPrefixName(void) +{ + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + byte in[64]; + word32 inSz; + + channel = SeedAppChannelsSession(&harness, "subsystem", "sftpx"); + inSz = BuildSftpInitDataPacket(channel->channel, in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + + AssertIntEQ(wolfSSH_SFTP_accept(harness.ssh), WS_INVALID_STATE_E); + AssertIntEQ(harness.io.outSz, 0); + AssertIntEQ(harness.io.inOff, 0); + AssertIntEQ(harness.ssh->error, WS_SUCCESS); + + FreeChannelOpenHarness(&harness); +} + +/* A granted name whose wire length runs past an embedded NUL is not an + * sftp grant: the four bytes ahead of the NUL match, the name does not. */ +static void TestSftpAcceptAppChannelsRefusesNulName(void) +{ + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + static const byte nulName[] = { + 's', 'f', 't', 'p', 0, 'e', 'v', 'i', 'l' + }; + byte payload[128]; + byte in[128]; + word32 idx = 0; + word32 inSz; + + sessionReqCbCalls = 0; + sessionReqCbReturn = 0; + + InitChannelOpenHarness(&harness, NULL, 0); + AssertIntEQ(wolfSSH_SetAppChannels(harness.ssh, 1), WS_SUCCESS); + AssertIntEQ(wolfSSH_CTX_SetChannelReqSubsysCb(harness.ctx, + RecordingSessionReqCb), WS_SUCCESS); + + channel = SeedUnconfirmedChannel(&harness); + AssertIntEQ(ChannelUpdatePeer(channel, 5, 1024, 1024), WS_SUCCESS); + channel->openConfirmed = 1; + + /* Built here rather than with BuildChannelStringRequestPacket(): that + * takes the name as a C string, which cannot carry the NUL. */ + idx = AppendUint32(payload, sizeof(payload), idx, channel->channel); + idx = AppendString(payload, sizeof(payload), idx, "subsystem"); + idx = AppendByte(payload, sizeof(payload), idx, 1); + idx = AppendUint32(payload, sizeof(payload), idx, (word32)sizeof(nulName)); + idx = AppendData(payload, sizeof(payload), idx, nulName, sizeof(nulName)); + inSz = WrapPacket(MSGID_CHANNEL_REQUEST, payload, idx, in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + + /* The callback reads a C string, so it sees sftp and grants it. */ + AssertIntEQ(DoReceive(harness.ssh), WS_SUCCESS); + AssertIntEQ(sessionReqCbCalls, 1); + AssertIntEQ(WSTRCMP(sessionReqCbCommand, "sftp"), 0); + AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz), + MSGID_CHANNEL_SUCCESS); + + inSz = BuildSftpInitDataPacket(channel->channel, in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + + AssertIntEQ(wolfSSH_SFTP_accept(harness.ssh), WS_INVALID_STATE_E); + AssertIntEQ(harness.io.outSz, 0); + AssertIntEQ(harness.io.inOff, 0); + AssertIntEQ(harness.ssh->error, WS_SUCCESS); + + FreeChannelOpenHarness(&harness); +} + +/* Four bytes that are not sftp are not an sftp grant. */ +static void TestSftpAcceptAppChannelsRefusesSameLengthName(void) +{ + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + byte in[64]; + word32 inSz; + + channel = SeedAppChannelsSession(&harness, "subsystem", "sfxp"); + inSz = BuildSftpInitDataPacket(channel->channel, in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + + AssertIntEQ(wolfSSH_SFTP_accept(harness.ssh), WS_INVALID_STATE_E); + AssertIntEQ(harness.io.outSz, 0); + AssertIntEQ(harness.io.inOff, 0); + AssertIntEQ(harness.ssh->error, WS_SUCCESS); + + FreeChannelOpenHarness(&harness); +} + /* The grant the mode relies on: the subsystem callback took sftp, so the * INIT is answered with a VERSION and accept() stays parked. */ static void TestSftpAcceptAppChannelsServesGrantedSftp(void) @@ -4066,6 +4160,69 @@ static void TestSftpAcceptAppChannelsRefusesRejectedCb(void) } +/* wolfSSH_accept()'s divert to the built-in server matches the subsystem + * name whole, by length as well as bytes. The last case is the control: + * with no name that does divert, a harness that never reached the check + * would pass every refusal above it. */ +static void TestAcceptDivertMatchesSftpNameWhole(void) +{ + static const struct { + const char* name; + word32 nameSz; + byte divert; + } cases[] = { + { "sftpx", 5, 0 }, /* longer than sftp */ + { "sfxp", 4, 0 }, /* the length of sftp, other bytes */ + { "sftp\0evil", 9, 0 }, /* sftp up to an embedded NUL */ + { "sftp", 4, 1 }, + }; + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + byte payload[128]; + byte in[128]; + word32 idx; + word32 inSz; + word32 i; + + for (i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) { + InitChannelOpenHarness(&harness, NULL, 0); + channel = SeedUnconfirmedChannel(&harness); + AssertIntEQ(ChannelUpdatePeer(channel, 5, 1024, 1024), WS_SUCCESS); + channel->openConfirmed = 1; + + idx = 0; + idx = AppendUint32(payload, sizeof(payload), idx, channel->channel); + idx = AppendString(payload, sizeof(payload), idx, "subsystem"); + idx = AppendByte(payload, sizeof(payload), idx, 1); + idx = AppendUint32(payload, sizeof(payload), idx, cases[i].nameSz); + idx = AppendData(payload, sizeof(payload), idx, + (const byte*)cases[i].name, cases[i].nameSz); + inSz = WrapPacket(MSGID_CHANNEL_REQUEST, payload, idx, in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + + /* Neither app-channels nor a callback, so the request is granted + * and the session is the one wolfSSH_accept() goes on to serve. */ + AssertIntEQ(DoReceive(harness.ssh), WS_SUCCESS); + AssertIntEQ(channel->commandSz, cases[i].nameSz); + RepointHarnessInput(&harness, NULL, 0); + + harness.ssh->acceptState = ACCEPT_SERVER_CHANNEL_ACCEPT_SENT; + if (cases[i].divert) { + /* The built-in server has the session, and stops on the INIT + * the empty input cannot supply. */ + wolfSSH_accept(harness.ssh); + AssertIntEQ(harness.ssh->acceptState, ACCEPT_INIT_SFTP); + } + else { + AssertIntEQ(wolfSSH_accept(harness.ssh), WS_SUCCESS); + AssertIntEQ(harness.ssh->acceptState, + ACCEPT_CLIENT_SESSION_ESTABLISHED); + } + + FreeChannelOpenHarness(&harness); + } +} + #endif /* WOLFSSH_SFTP */ /* A username change after the first userauth request must end the session. */ @@ -14119,11 +14276,15 @@ int main(int argc, char** argv) TestSftpAcceptAppChannelsNeedsSession(); TestSftpAcceptAppChannelsRefusesPreAccept(); TestSftpAcceptAppChannelsRefusesShell(); + TestSftpAcceptAppChannelsRefusesPrefixName(); + TestSftpAcceptAppChannelsRefusesNulName(); + TestSftpAcceptAppChannelsRefusesSameLengthName(); TestSftpAcceptAppChannelsServesGrantedSftp(); TestSftpAcceptAppChannelsRefusesEstablishedShell(); TestSftpAcceptAppChannelsServesEstablishedSftp(); TestSftpAcceptAppChannelsRefusesNoCb(); TestSftpAcceptAppChannelsRefusesRejectedCb(); + TestAcceptDivertMatchesSftpNameWhole(); #endif TestSecondSessionChannelRejected(); TestUsernameChangeDisconnects(); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 20d63e5a5..d5b9efa53 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1447,6 +1447,7 @@ struct WOLFSSH_CHANNEL { * Accumulates unread data, does not overwrite * it. */ char* command; + word32 commandSz; struct WOLFSSH* ssh; struct WOLFSSH_CHANNEL* next; }; From 86c1d875c847fcfd832f43e97892ddae4041c2a6 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 8 Sep 2026 13:58:43 -0700 Subject: [PATCH 07/11] tests: check the rejecting callback actually ran CheckSftpAcceptRefusesUngranted() drives the same refusal two ways, a registered subsystem callback saying no and app channels standing in for a missing one, and asserted nothing that told them apart. Assert the call count each case expects. --- tests/regress.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/regress.c b/tests/regress.c index 4fcc8e71f..57d9a1923 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -4134,6 +4134,9 @@ static void CheckSftpAcceptRefusesUngranted(int rejectVia) "sftp", in, sizeof(in)); RepointHarnessInput(&harness, in, inSz); AssertIntEQ(DoReceive(harness.ssh), WS_SUCCESS); + /* With a callback registered, it did the refusing, not app channels + * standing in for a missing one. */ + AssertIntEQ(sessionReqCbCalls, (rejectVia == 0) ? 0 : 1); /* Either way the peer is told the subsystem was refused. */ AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz), MSGID_CHANNEL_FAILURE); From 94589b6835525288ba0c651b8ea61f7a5b3dda59 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 9 Sep 2026 14:13:17 -0700 Subject: [PATCH 08/11] ssh: expose the session command length An application vetting an exec or subsystem request in its channel request callback is handed the command as a C string, which stops at an embedded NUL. wolfSSH_ChannelGetSessionCommandSz() and wolfSSH_GetSessionCommandSz() report the parsed wire length, so a callback can match a name whole the way DoChannelRequest() does. - both accessors report 0 for a NULL channel or session - wolfSSH_GetSessionCommand() defers to the channel accessor - correct the trace name in wolfSSH_ChannelGetSessionCommand() - cover a callback seeing "sftp\0evil" through exec and subsystem --- src/ssh.c | 39 ++++++++++++++++++-- tests/regress.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++ wolfssh/ssh.h | 3 ++ 3 files changed, 135 insertions(+), 4 deletions(-) diff --git a/src/ssh.c b/src/ssh.c index 7592792b8..b3f10dd37 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -4551,12 +4551,29 @@ WS_SessionType wolfSSH_GetSessionType(const WOLFSSH* ssh) const char* wolfSSH_GetSessionCommand(const WOLFSSH* ssh) { + const char* cmd = NULL; + WLOG(WS_LOG_DEBUG, "Entering wolfSSH_GetSessionCommand()"); - if (ssh && ssh->channelList) - return ssh->channelList->command; + if (ssh) { + cmd = wolfSSH_ChannelGetSessionCommand(ssh->channelList); + } - return NULL; + return cmd; +} + + +word32 wolfSSH_GetSessionCommandSz(const WOLFSSH* ssh) +{ + word32 commandSz = 0; + + WLOG(WS_LOG_DEBUG, "Entering wolfSSH_GetSessionCommandSz()"); + + if (ssh) { + commandSz = wolfSSH_ChannelGetSessionCommandSz(ssh->channelList); + } + + return commandSz; } @@ -5710,7 +5727,7 @@ const char* wolfSSH_ChannelGetSessionCommand(const WOLFSSH_CHANNEL* channel) { const char* cmd = NULL; - WLOG(WS_LOG_DEBUG, "Entering wolfSSH_ChannelGetCommand()"); + WLOG(WS_LOG_DEBUG, "Entering wolfSSH_ChannelGetSessionCommand()"); if (channel) { cmd = channel->command; @@ -5720,6 +5737,20 @@ const char* wolfSSH_ChannelGetSessionCommand(const WOLFSSH_CHANNEL* channel) } +word32 wolfSSH_ChannelGetSessionCommandSz(const WOLFSSH_CHANNEL* channel) +{ + word32 commandSz = 0; + + WLOG(WS_LOG_DEBUG, "Entering wolfSSH_ChannelGetSessionCommandSz()"); + + if (channel) { + commandSz = channel->commandSz; + } + + return commandSz; +} + + int wolfSSH_CTX_SetChannelOpenCb(WOLFSSH_CTX* ctx, WS_CallbackChannelOpen cb) { int ret = WS_SSH_CTX_NULL_E; diff --git a/tests/regress.c b/tests/regress.c index 57d9a1923..c5456692d 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -3735,6 +3735,102 @@ static void TestChannelReqSubsysCallbackRuns(void) WOLFSSH_SESSION_SUBSYSTEM), MSGID_CHANNEL_FAILURE); } +/* What a length-aware session request callback saw. */ +static word32 sessionReqCbCommandSz; +static word32 sessionReqCbCommandStrLen; + +static int LengthRecordingSessionReqCb(WOLFSSH_CHANNEL* channel, void* ctx) +{ + const char* command; + + (void)ctx; + + sessionReqCbCalls++; + sessionReqCbCommandSz = wolfSSH_ChannelGetSessionCommandSz(channel); + command = wolfSSH_ChannelGetSessionCommand(channel); + sessionReqCbCommandStrLen = (command == NULL) ? + 0 : (word32)WSTRLEN(command); + + return 0; +} + +/* Drives one session request carrying a command that the C string alone + * cannot describe, and checks what the callback could see of it. */ +static void CheckSessionReqCbSeesCommandSz(const char* type, + const byte* command, word32 commandSz, word32 expectStrLen) +{ + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + byte payload[128]; + byte in[128]; + word32 idx = 0; + word32 inSz; + + sessionReqCbCalls = 0; + sessionReqCbCommandSz = 0; + sessionReqCbCommandStrLen = 0; + + InitChannelOpenHarness(&harness, NULL, 0); + if (WSTRCMP(type, "exec") == 0) { + AssertIntEQ(wolfSSH_CTX_SetChannelReqExecCb(harness.ctx, + LengthRecordingSessionReqCb), WS_SUCCESS); + } + else { + AssertIntEQ(wolfSSH_CTX_SetChannelReqSubsysCb(harness.ctx, + LengthRecordingSessionReqCb), WS_SUCCESS); + } + + channel = SeedUnconfirmedChannel(&harness); + AssertIntEQ(ChannelUpdatePeer(channel, 5, 1024, 1024), WS_SUCCESS); + channel->openConfirmed = 1; + + /* Built here rather than with BuildChannelStringRequestPacket(): that + * takes the command as a C string, which cannot carry the NUL. */ + idx = AppendUint32(payload, sizeof(payload), idx, channel->channel); + idx = AppendString(payload, sizeof(payload), idx, type); + idx = AppendByte(payload, sizeof(payload), idx, 1); + idx = AppendUint32(payload, sizeof(payload), idx, commandSz); + idx = AppendData(payload, sizeof(payload), idx, command, commandSz); + inSz = WrapPacket(MSGID_CHANNEL_REQUEST, payload, idx, in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + + AssertIntEQ(DoReceive(harness.ssh), WS_SUCCESS); + AssertIntEQ(sessionReqCbCalls, 1); + AssertIntEQ(sessionReqCbCommandSz, commandSz); + AssertIntEQ(sessionReqCbCommandStrLen, expectStrLen); + + /* The session-wide accessor reports the same channel's command. */ + AssertIntEQ(wolfSSH_GetSessionCommandSz(harness.ssh), commandSz); + + FreeChannelOpenHarness(&harness); +} + +/* An application vetting a command in its callback needs the wire length. + * The string it is handed stops at an embedded NUL, so "sftp\0evil" reads + * there as "sftp" and passes a name check the whole name has to fail; the + * length is what tells the two apart. */ +static void TestSessionReqCallbackSeesCommandSz(void) +{ + static const byte nulCommand[] = { + 's', 'f', 't', 'p', 0, 'e', 'v', 'i', 'l' + }; + static const byte plainCommand[] = { 'l', 's' }; + + /* The control: with no NUL in it, length and C string agree, so the + * cases below are the NUL and not the accessor reporting anything it + * likes. */ + CheckSessionReqCbSeesCommandSz("exec", plainCommand, + (word32)sizeof(plainCommand), (word32)sizeof(plainCommand)); + CheckSessionReqCbSeesCommandSz("exec", nulCommand, + (word32)sizeof(nulCommand), 4); + CheckSessionReqCbSeesCommandSz("subsystem", nulCommand, + (word32)sizeof(nulCommand), 4); + + /* Nothing to report is zero, not a read through a NULL. */ + AssertIntEQ(wolfSSH_ChannelGetSessionCommandSz(NULL), 0); + AssertIntEQ(wolfSSH_GetSessionCommandSz(NULL), 0); +} + /* A request callback owns its channel and may close it. The grant is * recorded after the callback returns, so it has to find the channel * again: wolfSSH_ChannelFree() frees it, and writing through the old @@ -14273,6 +14369,7 @@ int main(int argc, char** argv) TestChannelCloseCallbackReturnIgnored(); TestChannelReqExecCallbackRuns(); TestChannelReqSubsysCallbackRuns(); + TestSessionReqCallbackSeesCommandSz(); TestSessionReqCallbackMayFreeChannel(); TestAppChannelsAcceptKeepsStopWithPendingOutput(); #ifdef WOLFSSH_SFTP diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 815a1a486..bd461a77d 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -436,6 +436,8 @@ WOLFSSH_API WS_SessionType wolfSSH_ChannelGetSessionType( const WOLFSSH_CHANNEL* channel); WOLFSSH_API const char* wolfSSH_ChannelGetSessionCommand( const WOLFSSH_CHANNEL* channel); +WOLFSSH_API word32 wolfSSH_ChannelGetSessionCommandSz( + const WOLFSSH_CHANNEL* channel); WOLFSSH_API int wolfSSH_ChannelIsPty(const WOLFSSH_CHANNEL* channel); /* Channel callbacks */ @@ -889,6 +891,7 @@ WOLFSSH_API int wolfSSH_ConvertConsole(WOLFSSH* ssh, WOLFSSH_HANDLE handle, WOLFSSH_API int wolfSSH_DoModes(const byte* modes, word32 modesSz, int fd); WOLFSSH_API WS_SessionType wolfSSH_GetSessionType(const WOLFSSH* ssh); WOLFSSH_API const char* wolfSSH_GetSessionCommand(const WOLFSSH* ssh); +WOLFSSH_API word32 wolfSSH_GetSessionCommandSz(const WOLFSSH* ssh); WOLFSSH_API int wolfSSH_SetChannelType(WOLFSSH* ssh, byte type, byte* name, word32 nameSz); WOLFSSH_API int wolfSSH_ChangeTerminalSize(WOLFSSH* ssh, word32 columns, From 331ee91f55276bb134bdc50a1ce747e0415cc7b0 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 9 Sep 2026 14:37:54 -0700 Subject: [PATCH 09/11] internal: act on a session request only if parsed DoChannelRequest() records the session type and asks the exec and subsystem callbacks whether to grant a session only when the command string parsed. A failed parse is refused on ret alone, and channel->command still holds an earlier request's value rather than the one being answered. - cover a command length header running past the end of the packet, on exec and on subsystem Issue: F-11674 --- src/internal.c | 30 ++++++++++++++---------- tests/regress.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 12 deletions(-) diff --git a/src/internal.c b/src/internal.c index fb9b90ac3..01e736017 100644 --- a/src/internal.c +++ b/src/internal.c @@ -13150,12 +13150,15 @@ static int DoChannelRequest(WOLFSSH* ssh, WLOG(WS_LOG_DEBUG, " command = %s", channel->command); else WLOG(WS_LOG_DEBUG, " command = %s", ""); - channel->sessionType = WOLFSSH_SESSION_EXEC; - if (ssh->ctx->channelReqExecCb) { - rej = ssh->ctx->channelReqExecCb(channel, ssh->channelReqCtx); - } - else { - rej = ssh->appChannels; + 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; @@ -13168,12 +13171,15 @@ static int DoChannelRequest(WOLFSSH* ssh, WLOG(WS_LOG_DEBUG, " subsystem = %s", channel->command); else WLOG(WS_LOG_DEBUG, " subsystem = %s", ""); - channel->sessionType = WOLFSSH_SESSION_SUBSYSTEM; - if (ssh->ctx->channelReqSubsysCb) { - rej = ssh->ctx->channelReqSubsysCb(channel, ssh->channelReqCtx); - } - else { - rej = ssh->appChannels; + 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; diff --git a/tests/regress.c b/tests/regress.c index c5456692d..5e8cff4cf 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -3831,6 +3831,67 @@ static void TestSessionReqCallbackSeesCommandSz(void) AssertIntEQ(wolfSSH_GetSessionCommandSz(NULL), 0); } +/* Drives one session request whose command string runs past the end of the + * packet, and returns the message id the server answered with. */ +static byte RunMalformedSessionRequest(const char* type) +{ + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + byte payload[128]; + byte in[128]; + word32 idx = 0; + word32 inSz; + byte replyId; + + sessionReqCbCalls = 0; + sessionReqCbReturn = 0; + + InitChannelOpenHarness(&harness, NULL, 0); + if (WSTRCMP(type, "exec") == 0) { + AssertIntEQ(wolfSSH_CTX_SetChannelReqExecCb(harness.ctx, + RecordingSessionReqCb), WS_SUCCESS); + } + else { + AssertIntEQ(wolfSSH_CTX_SetChannelReqSubsysCb(harness.ctx, + RecordingSessionReqCb), WS_SUCCESS); + } + + channel = SeedUnconfirmedChannel(&harness); + AssertIntEQ(ChannelUpdatePeer(channel, 5, 1024, 1024), WS_SUCCESS); + channel->openConfirmed = 1; + + /* The command's length header claims more than the packet holds. */ + idx = AppendUint32(payload, sizeof(payload), idx, channel->channel); + idx = AppendString(payload, sizeof(payload), idx, type); + idx = AppendByte(payload, sizeof(payload), idx, 1); + idx = AppendUint32(payload, sizeof(payload), idx, 64); + idx = AppendData(payload, sizeof(payload), idx, (const byte*)"ls", 2); + inSz = WrapPacket(MSGID_CHANNEL_REQUEST, payload, idx, in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + + /* A malformed packet ends the connection, but the refusal goes out + * first. */ + AssertIntEQ(DoReceive(harness.ssh), WS_FATAL_ERROR); + AssertIntEQ(harness.ssh->error, WS_BUFFER_E); + AssertIntEQ(sessionReqCbCalls, 0); + AssertIntEQ(channel->sessionGranted, 0); + + replyId = ParseMsgId(harness.io.out, harness.io.outSz); + FreeChannelOpenHarness(&harness); + + return replyId; +} + +/* A command that failed to parse is refused without asking the callback: + * there is nothing to vet, and channel->command still holds whatever an + * earlier request on the channel left behind. */ +static void TestMalformedSessionRequestSkipsCallback(void) +{ + AssertIntEQ(RunMalformedSessionRequest("exec"), MSGID_CHANNEL_FAILURE); + AssertIntEQ(RunMalformedSessionRequest("subsystem"), + MSGID_CHANNEL_FAILURE); +} + /* A request callback owns its channel and may close it. The grant is * recorded after the callback returns, so it has to find the channel * again: wolfSSH_ChannelFree() frees it, and writing through the old @@ -14370,6 +14431,7 @@ int main(int argc, char** argv) TestChannelReqExecCallbackRuns(); TestChannelReqSubsysCallbackRuns(); TestSessionReqCallbackSeesCommandSz(); + TestMalformedSessionRequestSkipsCallback(); TestSessionReqCallbackMayFreeChannel(); TestAppChannelsAcceptKeepsStopWithPendingOutput(); #ifdef WOLFSSH_SFTP From 3aae762a250a2b0d0445b42c20315410f8303d8e Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 9 Sep 2026 14:43:24 -0700 Subject: [PATCH 10/11] internal: scrub the channel command on free ChannelDelete() wipes the peer's exec or subsystem command line before releasing it, the way it already wipes the decrypted inputBuffer just above. A command line can carry a password or a token among its arguments. - cover the wipe with the retain-on-free allocator - release the test's hand-built channel on a setup failure Issue: F-8850 --- src/internal.c | 5 ++- tests/unit.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) diff --git a/src/internal.c b/src/internal.c index 01e736017..f5ad61e85 100644 --- a/src/internal.c +++ b/src/internal.c @@ -4199,8 +4199,11 @@ void ChannelDelete(WOLFSSH_CHANNEL* channel, void* heap) channel->channel); } ShrinkBuffer(&channel->extDataBuffer, 1); - if (channel->command) + /* 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); + } WFREE(channel, heap, DYNTYPE_CHANNEL); } } diff --git a/tests/unit.c b/tests/unit.c index 3e67cbe3b..3d7a0eda0 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -16910,6 +16910,89 @@ static int test_SshResourceFree_zeroesSecrets(void) return result; } + +/* Verify ChannelDelete wipes the peer's exec/subsystem command line before + * releasing it. A command can carry a password or token in its arguments, + * and the buffer sits right below the inputBuffer this function already + * scrubs. The retain-on-free allocator is installed just around + * ChannelDelete so the freed bytes can be read back without touching + * freed memory. */ +static int test_ChannelDelete_zeroesCommand(void) +{ + static const char command[] = "sh -c 'login --password hunter2'"; + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* channel = NULL; + const byte* commandBytes; + word32 commandSz; + word32 i; + int result = 0; + wolfSSL_Malloc_cb prevMf = NULL; + wolfSSL_Free_cb prevFf = NULL; + wolfSSL_Realloc_cb prevRf = NULL; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -710; + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { + result = -711; + goto out; + } + + channel = ChannelNew(ssh, ID_CHANTYPE_SESSION, + DEFAULT_WINDOW_SZ, DEFAULT_MAX_PACKET_SZ); + if (channel == NULL) { + result = -712; + goto out; + } + + commandSz = (word32)WSTRLEN(command); + channel->command = (char*)WMALLOC(commandSz + 1, NULL, DYNTYPE_STRING); + if (channel->command == NULL) { + result = -713; + goto out; + } + WMEMCPY(channel->command, command, commandSz + 1); + channel->commandSz = commandSz; + commandBytes = (const byte*)channel->command; + + wolfSSL_GetAllocators(&prevMf, &prevFf, &prevRf); + /* Allocators unchanged on failure; nothing to restore. */ + if (wolfSSL_SetAllocators(RetainMalloc, RetainFree, + RetainRealloc) != 0) { + result = -714; + goto out; + } + ChannelDelete(channel, NULL); + wolfSSL_SetAllocators(prevMf, prevFf, prevRf); + channel = NULL; + + if (!IsRetained((void*)commandBytes)) { + result = -715; + goto out; + } + + for (i = 0; i < commandSz; i++) { + if (commandBytes[i] != 0) { + result = -716; + goto out; + } + } + +out: + DrainRetained(); + /* Only the setup-failure paths reach here with a channel; it is never + * on ssh->channelList, so wolfSSH_free() would not release it. */ + if (channel != NULL) + ChannelDelete(channel, ssh->ctx->heap); + if (ssh != NULL) + wolfSSH_free(ssh); + if (ctx != NULL) + wolfSSH_CTX_free(ctx); + return result; +} + #endif /* WOLFSSH_TEST_CAPTURING_ALLOCATOR */ #ifndef WOLFSSH_NO_DH @@ -21338,6 +21421,11 @@ int wolfSSH_UnitTest(int argc, char** argv) printf("SshResourceFree_zeroesSecrets: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; + + unitResult = test_ChannelDelete_zeroesCommand(); + printf("ChannelDelete_zeroesCommand: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; #endif #ifndef WOLFSSH_NO_DH From ca71d666d2211bf00b9e4deea5f4791f6e296eb1 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 9 Sep 2026 15:01:36 -0700 Subject: [PATCH 11/11] internal: drop an always-true guard DoChannelRequest() returns early when the header parse fails, so the ret == WS_SUCCESS test that followed it could never be false. The channel lookup moves into the else, which is the only way the function reaches it. Issue: F-11657 --- src/internal.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/internal.c b/src/internal.c index f5ad61e85..e50b0edda 100644 --- a/src/internal.c +++ b/src/internal.c @@ -13101,8 +13101,7 @@ static int DoChannelRequest(WOLFSSH* ssh, WLOG(WS_LOG_DEBUG, "Leaving DoChannelRequest(), ret = %d", ret); return ret; } - - if (ret == WS_SUCCESS) { + else { channel = ChannelFind(ssh, channelId, WS_CHANNEL_ID_SELF); if (channel == NULL) ret = WS_INVALID_CHANID;