internal: commit a session only once accepted

A shell, exec or subsystem request changes the channel only once the
callback accepts it. The session type and command are set for the
callback to read and put back if it refuses, and CLIENT_DONE follows
acceptance alone, so wolfSSH_accept() no longer reports an established
session, or starts SFTP, on a request it answered CHANNEL_FAILURE.

- DoChannelRequestSession() carries the three arms, which differed only
  in the type and the callback consulted
- unit.c drives a refused shell, exec and subsystem request through
  DoChannelRequest() and checks nothing was committed
- regress.c runs a server whose shell callback refuses and checks
  accept() stays at ACCEPT_SERVER_CHANNEL_ACCEPT_SENT

Issue: F-8852

Author: ejohnstown

src/internal.c

@@ -12617,6 +12617,61 @@ static void SetTerminalSize(WOLFSSH* ssh, word32 widthChar, word32 heightRows,
 #endif /* WOLFSSH_TERM */
 
 
+/* Answers a shell, exec, or subsystem request. The session type, and the
+ * command for the two that carry one, are set for the callback to read and
+ * kept only if it accepts; a refused request leaves the channel as it was
+ * and the accept loop still waiting, so nothing serves a session the
+ * application turned down. Without a callback the request is accepted,
+ * unless the application drives its own channels. */
+static int DoChannelRequestSession(WOLFSSH* ssh, WOLFSSH_CHANNEL* channel,
+        byte sessionType, WS_CallbackChannelReq cb,
+        byte* buf, word32 len, word32* idx, int* rej)
+{
+    char* prevCommand = NULL;
+    byte prevType = channel->sessionType;
+    byte hasCommand = (sessionType != WOLFSSH_SESSION_SHELL);
+    int ret = WS_SUCCESS;
+
+    if (hasCommand) {
+        prevCommand = channel->command;
+        channel->command = NULL;
+        ret = GetStringAlloc(ssh->ctx->heap, &channel->command, NULL,
+                buf, len, idx);
+        if (ret == WS_SUCCESS) {
+            WLOG(WS_LOG_DEBUG, "  command = %s", channel->command);
+        }
+    }
+
+    if (ret == WS_SUCCESS) {
+        channel->sessionType = sessionType;
+        if (cb != NULL) {
+            *rej = cb(channel, ssh->channelReqCtx);
+        }
+        else {
+            *rej = ssh->appChannels;
+        }
+    }
+
+    if (ret == WS_SUCCESS && !*rej) {
+        if (prevCommand != NULL) {
+            WFREE(prevCommand, ssh->ctx->heap, DYNTYPE_STRING);
+        }
+        ssh->clientState = CLIENT_DONE;
+    }
+    else {
+        if (hasCommand) {
+            if (channel->command != NULL) {
+                WFREE(channel->command, ssh->ctx->heap, DYNTYPE_STRING);
+            }
+            channel->command = prevCommand;
+        }
+        channel->sessionType = prevType;
+    }
+
+    return ret;
+}
+
+
 static int DoChannelRequest(WOLFSSH* ssh,
                             byte* buf, word32 len, word32* idx)
 {
@@ -12672,42 +12727,17 @@ static int DoChannelRequest(WOLFSSH* ssh,
             WLOG(WS_LOG_DEBUG, "  %s = %s", name, value);
         }
         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;
-            }
-            ssh->clientState = CLIENT_DONE;
+            ret = DoChannelRequestSession(ssh, channel, WOLFSSH_SESSION_SHELL,
+                    ssh->ctx->channelReqShellCb, buf, len, &begin, &rej);
         }
         else if (ChannelRequestIs(type, typeSz, "exec")) {
-            ret = GetStringAlloc(ssh->ctx->heap, &channel->command, NULL,
-                    buf, len, &begin);
-            channel->sessionType = WOLFSSH_SESSION_EXEC;
-            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);
+            ret = DoChannelRequestSession(ssh, channel, WOLFSSH_SESSION_EXEC,
+                    ssh->ctx->channelReqExecCb, buf, len, &begin, &rej);
         }
         else if (ChannelRequestIs(type, typeSz, "subsystem")) {
-            ret = GetStringAlloc(ssh->ctx->heap, &channel->command, NULL,
-                    buf, len, &begin);
-            channel->sessionType = WOLFSSH_SESSION_SUBSYSTEM;
-            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);
+            ret = DoChannelRequestSession(ssh, channel,
+                    WOLFSSH_SESSION_SUBSYSTEM, ssh->ctx->channelReqSubsysCb,
+                    buf, len, &begin, &rej);
         }
         #ifdef WOLFSSH_TERM
         else if (ChannelRequestIs(type, typeSz, "pty-req")) {

tests/regress.c

@@ -1646,6 +1646,57 @@ 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;
+
+    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);
+    AssertIntEQ(wolfSSH_GetSessionType(harness.server),
+            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);
@@ -12480,6 +12531,7 @@ int main(int argc, char** argv)
     TestAppChannelsAcceptStopsAtUserAuth();
     TestAppChannelsNoShellCbRejects();
     TestAppChannelsLateEnableReturns();
+    TestSessionReqRejectedKeepsAcceptWaiting();
     TestKexDhReplyRejectsRsaSha2_256SigNameDowngrade();
     #endif
     #ifndef WOLFSSH_NO_RSA_SHA2_512

tests/unit.c

@@ -8584,6 +8584,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;
 
@@ -8836,6 +8853,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     */
@@ -8962,6 +8988,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;
@@ -9249,15 +9347,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;