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 5ba3a942d..9c4e119cf 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -813,9 +813,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..08e0adfb0 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); @@ -4433,10 +4485,42 @@ 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. */ +/* The grant clause of the gate on its own. Everything else it asks for + * still matches -- a subsystem channel carrying "sftp" -- so the cleared + * grant is the only thing left that can refuse. The refusals above all + * stop on the type or the command, and would pass with the clause gone. */ +static void TestSftpAcceptAppChannelsNeedsGrantAlone(void) +{ + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + byte in[64]; + word32 inSz; + + channel = SeedAppChannelsSession(&harness, "subsystem", "sftp"); + AssertIntEQ(channel->sessionType, WOLFSSH_SESSION_SUBSYSTEM); + AssertIntEQ(channel->commandSz, 4); + AssertIntEQ(channel->sessionGranted, 1); + + /* White box: the callback's answer taken back, the request that won + * it left as it was. */ + channel->sessionGranted = 0; + + 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 refused "subsystem sftp" leaves the channel as it was: the refusal + * puts the session type and command back and records no grant, so + * wolfSSH_SFTP_accept() has nothing to serve. rejectVia 0 registers no + * callback at all (app channels alone refuse); 1 registers one that + * rejects. */ static void CheckSftpAcceptRefusesUngranted(int rejectVia) { ChannelOpenHarness harness; @@ -4556,10 +4640,10 @@ static void TestAcceptDivertMatchesSftpNameWhole(void) -/* The divert asks for the grant, not just the name. A subsystem callback - * that refuses sftp answers CHANNEL_FAILURE, yet sessionType and command - * are recorded ahead of that answer and stay set, so the name alone would - * hand the refused session to the built-in server. */ +/* A subsystem callback that refuses sftp answers CHANNEL_FAILURE and + * leaves the client state short of CLIENT_DONE, so accept() stops there + * rather than handing the refused session to the built-in server. The + * same name, granted, does divert. */ static void CheckAcceptDivertNeedsSftpGrant(int reject) { ChannelOpenHarness harness; @@ -4591,9 +4675,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 @@ -4611,13 +4696,55 @@ static void TestAcceptDivertNeedsSftpGrant(void) CheckAcceptDivertNeedsSftpGrant(1); CheckAcceptDivertNeedsSftpGrant(0); } + + +/* The divert's grant clause on its own. The request was granted, so the + * type, the command and the client state are all what the built-in server + * wants; only the grant is gone. The refusal above stops short of the + * divert on the client state, so it would pass with the clause gone. */ +static void TestAcceptDivertNeedsSftpGrantAlone(void) +{ + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + byte in[128]; + word32 inSz; + + sessionReqCbCalls = 0; + sessionReqCbReturn = 0; + + InitChannelOpenHarness(&harness, NULL, 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); + AssertIntEQ(sessionReqCbCalls, 1); + AssertIntEQ(channel->sessionGranted, 1); + AssertIntEQ(harness.ssh->clientState, CLIENT_DONE); + RepointHarnessInput(&harness, NULL, 0); + + /* White box: as above, the grant alone taken back. */ + channel->sessionGranted = 0; + + harness.ssh->acceptState = ACCEPT_SERVER_CHANNEL_ACCEPT_SENT; + AssertIntEQ(wolfSSH_accept(harness.ssh), WS_SUCCESS); + AssertIntEQ(harness.ssh->acceptState, ACCEPT_CLIENT_SESSION_ESTABLISHED); + + FreeChannelOpenHarness(&harness); +} #endif /* WOLFSSH_SFTP */ #ifdef WOLFSSH_SCP -/* Same for the SCP divert, which reads the command with no grant test of - * its own. An exec callback that refuses "scp ..." must not leave the - * built-in SCP server holding the session it just refused. */ +/* Same for the SCP divert, which otherwise reads only the command. An + * exec callback that refuses "scp ..." must not leave the built-in SCP + * server holding the session it just refused. */ static void CheckAcceptDivertNeedsScpGrant(int reject) { ChannelOpenHarness harness; @@ -4647,9 +4774,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); @@ -4666,6 +4794,45 @@ static void TestAcceptDivertNeedsScpGrant(void) CheckAcceptDivertNeedsScpGrant(0); } + +/* Same for the SCP divert, which reads the command the exec callback + * granted. */ +static void TestAcceptDivertNeedsScpGrantAlone(void) +{ + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + byte in[128]; + word32 inSz; + + sessionReqCbCalls = 0; + sessionReqCbReturn = 0; + + InitChannelOpenHarness(&harness, NULL, 0); + AssertIntEQ(wolfSSH_CTX_SetChannelReqExecCb(harness.ctx, + RecordingSessionReqCb), WS_SUCCESS); + + channel = SeedUnconfirmedChannel(&harness); + AssertIntEQ(ChannelUpdatePeer(channel, 5, 1024, 1024), WS_SUCCESS); + channel->openConfirmed = 1; + + inSz = BuildChannelStringRequestPacket(channel->channel, "exec", 1, + "scp -t /tmp/f", in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + AssertIntEQ(DoReceive(harness.ssh), WS_SUCCESS); + AssertIntEQ(sessionReqCbCalls, 1); + AssertIntEQ(channel->sessionGranted, 1); + AssertIntEQ(harness.ssh->clientState, CLIENT_DONE); + RepointHarnessInput(&harness, NULL, 0); + + channel->sessionGranted = 0; + + harness.ssh->acceptState = ACCEPT_SERVER_CHANNEL_ACCEPT_SENT; + AssertIntEQ(wolfSSH_accept(harness.ssh), WS_SUCCESS); + AssertIntEQ(harness.ssh->acceptState, ACCEPT_CLIENT_SESSION_ESTABLISHED); + + FreeChannelOpenHarness(&harness); +} + #endif /* WOLFSSH_SCP */ /* A username change after the first userauth request must end the session. */ @@ -14745,13 +14912,16 @@ int main(int argc, char** argv) TestSftpAcceptAppChannelsServesGrantedSftp(); TestSftpAcceptAppChannelsRefusesEstablishedShell(); TestSftpAcceptAppChannelsServesEstablishedSftp(); + TestSftpAcceptAppChannelsNeedsGrantAlone(); TestSftpAcceptAppChannelsRefusesNoCb(); TestSftpAcceptAppChannelsRefusesRejectedCb(); TestAcceptDivertMatchesSftpNameWhole(); TestAcceptDivertNeedsSftpGrant(); + TestAcceptDivertNeedsSftpGrantAlone(); #endif #ifdef WOLFSSH_SCP TestAcceptDivertNeedsScpGrant(); + TestAcceptDivertNeedsScpGrantAlone(); #endif TestSecondSessionChannelRejected(); TestUsernameChangeDisconnects(); @@ -14961,6 +15131,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..d04bfab4e 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,108 @@ static int test_DoChannelRequest(void) } } + /* A refused exec has to put back the command the granted one left: + * an earlier grant still stands, and a server reads the command off + * the channel. The reject cases above ran on a bare channel, where + * the restore had nothing to put back. */ + { + static const byte payExecOther[] = { + 0x00,0x00,0x00,0x00, /* channelId = 0 */ + 0x00,0x00,0x00,0x04, /* typeSz = 4 */ + 0x65,0x78,0x65,0x63, /* "exec" */ + 0x01, /* wantReply = 1 */ + 0x00,0x00,0x00,0x06, /* cmdSz = 6 */ + 0x77,0x68,0x6F,0x61,0x6D,0x69 /* "whoami" */ + }; + word32 idxRej = 0; + const char* cmd; + int retRej, capMsgId; + + if (wolfSSH_CTX_SetChannelReqExecCb(ctx, RejectChanReqCb) + != WS_SUCCESS) { + printf("DoChannelRequest[rej-exec-granted]: no callback set\n"); + result = -505; + goto done; + } + + s_chanReqCaptureSz = 0; + WMEMSET(s_chanReqCapture, 0, sizeof(s_chanReqCapture)); + s_rejectChanReqCalls = 0; + + retRej = wolfSSH_TestDoChannelRequest(ssh, (byte*)payExecOther, + (word32)sizeof(payExecOther), &idxRej); + if (retRej != WS_SUCCESS || s_rejectChanReqCalls != 1) { + printf("DoChannelRequest[rej-exec-granted]: ret=%d, callback ran " + "%d times\n", retRej, s_rejectChanReqCalls); + result = -506; + goto done; + } + + capMsgId = CaptureMsgId(s_chanReqCapture, s_chanReqCaptureSz); + if (capMsgId != (int)MSGID_CHANNEL_FAILURE) { + printf("DoChannelRequest[rej-exec-granted]: msg_id=0x%02x, " + "expected=0x%02x\n", capMsgId, MSGID_CHANNEL_FAILURE); + result = -507; + goto done; + } + + /* The command the refused request carried is gone, and "ls" is + * back whole: the pointer and the length together. */ + cmd = wolfSSH_ChannelGetSessionCommand(ch); + if (cmd == NULL || WSTRCMP(cmd, "ls") != 0 + || wolfSSH_ChannelGetSessionCommandSz(ch) != 2) { + printf("DoChannelRequest[rej-exec-granted]: command = %s, sz = %u\n", + cmd == NULL ? "(null)" : cmd, + wolfSSH_ChannelGetSessionCommandSz(ch)); + result = -508; + goto done; + } + if (wolfSSH_ChannelGetSessionType(ch) != WOLFSSH_SESSION_EXEC) { + printf("DoChannelRequest[rej-exec-granted]: type not exec\n"); + result = -509; + goto done; + } + /* The refusal answered this request, it did not take back the + * session the granted exec won. */ + if (!ch->sessionGranted) { + printf("DoChannelRequest[rej-exec-granted]: grant lost\n"); + result = -510; + goto done; + } + + if (wolfSSH_CTX_SetChannelReqExecCb(ctx, NULL) != WS_SUCCESS) { + printf("DoChannelRequest[rej-exec-granted]: callback not cleared\n"); + result = -511; + goto done; + } + } + + /* 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 +9625,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;