internal: refuse a second session request

RFC 4254 sec 6.5 lets only one shell, exec or subsystem request succeed
per channel. DoChannelRequestSession() now refuses any request after
the first grant without asking a callback, so the granted session and
its command stand.

- a refusal before any grant leaves the channel with no command
- ssh.h documents the refusal; wolfsshd's own check is defence in depth
- regress.c sends a second exec, shell and subsystem after a granted
  exec, and a second exec over a generic-callback grant
- unit.c expects a shell after an exec to be refused and checks that a
  refused exec's command is scrubbed

Author: ejohnstown

apps/wolfsshd/wolfsshd.c

@@ -546,9 +546,9 @@ static int SessionRequestCb(WOLFSSH_CHANNEL* channel, void* vCtx)
             break;
     }
 
-    /* One program start per channel, as RFC 4254 section 6.5 allows. This
-     * request's grant is recorded once the callback returns, so a flag
-     * already set is an earlier request's. */
+    /* One program start per channel, as RFC 4254 section 6.5 allows. The
+     * library already refuses a second request before calling here, so
+     * this is defence in depth. */
     if (!rej && wolfSSH_ChannelGetSessionGranted(channel) == 1) {
         rej = 1;
         reason = "a session is already running on the channel";

src/internal.c

@@ -13550,26 +13550,23 @@ static void SetTerminalSize(WOLFSSH* ssh, word32 widthChar, word32 heightRows,
 
 /* 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.
- * A request the generic callback already granted asks no callback. */
+ * A request the generic callback already granted asks no callback, and
+ * one on a channel that already has a session is refused without asking
+ * any. Only the first grant sets a command, so a request that gets past
+ * that check finds the channel with none. */
 static int DoChannelRequestSession(WOLFSSH* ssh, word32 channelId,
         WOLFSSH_CHANNEL* channel, byte sessionType, WS_CallbackChannelReq cb,
         int granted, byte* buf, word32 len, word32* idx, int* rej)
 {
     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. */
+    /* A shell request carries no command. */
     if (hasCommand) {
-        prevCommand = channel->command;
-        prevCommandSz = channel->commandSz;
-
         ret = GetStringAlloc(heap, &command, &commandSz, buf, len, idx);
         if (ret == WS_SUCCESS)
             WLOG(WS_LOG_DEBUG, "  %s = %s",
@@ -13581,6 +13578,15 @@ static int DoChannelRequestSession(WOLFSSH* ssh, word32 channelId,
                         ? "subsystem" : "command", "<bad value>");
     }
 
+    /* RFC 4254 sec 6.5: one session request succeeds per channel. Refuse
+     * the rest without asking a callback; the grant stands. */
+    if (ret == WS_SUCCESS && channel->sessionGranted) {
+        WLOG(WS_LOG_DEBUG, "  session already granted, rejecting.");
+        FreeChannelCommand(heap, command, commandSz);
+        *rej = 1;
+        return ret;
+    }
+
     if (ret == WS_SUCCESS) {
         if (hasCommand) {
             channel->command = command;
@@ -13595,29 +13601,31 @@ static int DoChannelRequestSession(WOLFSSH* ssh, word32 channelId,
         else
             *rej = ssh->appChannels;
 
-        /* A callback may free its own channel, so look it up again. */
+        /* A callback may free its own channel, and the command with it,
+         * 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);
+        if (channel == NULL)
             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. */
+    else if (ret == WS_SUCCESS) {
+        /* A refusal leaves the channel as it found it, with no command. */
         if (hasCommand) {
             FreeChannelCommand(heap, command, commandSz);
-            channel->command = prevCommand;
-            channel->commandSz = prevCommandSz;
+            channel->command = NULL;
+            channel->commandSz = 0;
         }
         channel->sessionType = prevType;
     }
+    else {
+        /* A bad request never reached the channel, which may hold an
+         * earlier grant. */
+        FreeChannelCommand(heap, command, commandSz);
+    }
 
     return ret;
 }

tests/regress.c

@@ -2005,6 +2005,11 @@ static void TestAppChannelsLateEnableReturns(void)
     AssertIntEQ(harness.server->acceptState,
             ACCEPT_CLIENT_SESSION_ESTABLISHED);
 
+    /* White box: the handshake's shell holds the session. Clear the grant
+     * before each request so only the mode decides. */
+    AssertNotNull(harness.server->channelList);
+    harness.server->channelList->sessionGranted = 0;
+
     /* Default mode, no callback registered: the request is granted. */
     idx = 0;
     AssertIntEQ(wolfSSH_TestDoChannelRequest(harness.server, payShell,
@@ -2017,6 +2022,7 @@ static void TestAppChannelsLateEnableReturns(void)
             ACCEPT_CLIENT_SESSION_ESTABLISHED);
 
     /* Same request, same session, mode now on: refused instead. */
+    harness.server->channelList->sessionGranted = 0;
     idx = 0;
     AssertIntEQ(wolfSSH_TestDoChannelRequest(harness.server, payShell,
             (word32)sizeof(payShell), &idx), WS_SUCCESS);
@@ -7550,6 +7556,84 @@ static void TestSessionReqCallbackMayFreeChannel(void)
     FreeChannelOpenHarness(&harness);
 }
 
+/* RFC 4254 sec 6.5: after a granted exec, a second session request is
+ * refused without asking its callback, and the exec stands. */
+static void CheckSecondSessionRequestRefused(const char* type,
+        const char* arg)
+{
+    ChannelOpenHarness harness;
+    WOLFSSH_CHANNEL* channel;
+    WS_SessionType sessionType;
+    byte in[128];
+    word32 inSz;
+
+    sessionReqCbCalls = 0;
+    sessionReqCbReturn = 0;
+
+    InitChannelOpenHarness(&harness, NULL, 0);
+    AssertIntEQ(wolfSSH_CTX_SetChannelReqShellCb(harness.ctx,
+                RecordingSessionReqCb), WS_SUCCESS);
+    AssertIntEQ(wolfSSH_CTX_SetChannelReqExecCb(harness.ctx,
+                RecordingSessionReqCb), WS_SUCCESS);
+    AssertIntEQ(wolfSSH_CTX_SetChannelReqSubsysCb(harness.ctx,
+                RecordingSessionReqCb), WS_SUCCESS);
+    channel = SeedConfirmedSessionChannel(&harness);
+
+    inSz = BuildChannelStringRequestPacket(channel->channel, "exec", 1,
+            "ls", in, sizeof(in));
+    AssertIntEQ(ReplyToPacket(&harness, in, inSz), MSGID_CHANNEL_SUCCESS);
+    AssertIntEQ(sessionReqCbCalls, 1);
+    AssertIntEQ(channel->sessionGranted, 1);
+
+    inSz = BuildChannelStringRequestPacket(channel->channel, type, 1, arg,
+            in, sizeof(in));
+    AssertIntEQ(ReplyToPacket(&harness, in, inSz), MSGID_CHANNEL_FAILURE);
+    AssertIntEQ(sessionReqCbCalls, 1);
+    AssertIntEQ(channel->sessionGranted, 1);
+    sessionType = wolfSSH_ChannelGetSessionType(channel);
+    AssertIntEQ(sessionType, WOLFSSH_SESSION_EXEC);
+    AssertIntEQ(wolfSSH_ChannelGetSessionCommandSz(channel), 2);
+    AssertIntEQ(WSTRCMP(wolfSSH_ChannelGetSessionCommand(channel), "ls"), 0);
+
+    FreeChannelOpenHarness(&harness);
+}
+
+static void TestSecondSessionRequestRefused(void)
+{
+    CheckSecondSessionRequestRefused("exec", "whoami");
+    CheckSecondSessionRequestRefused("shell", NULL);
+    CheckSecondSessionRequestRefused("subsystem", "sftp");
+}
+
+/* A grant from the generic callback does not override the rule. */
+static void TestSecondSessionRequestRefusedOverGenericGrant(void)
+{
+    ChannelOpenHarness harness;
+    WOLFSSH_CHANNEL* channel;
+    byte in[128];
+    word32 inSz;
+
+    ResetAnyReqCb(WOLFSSH_REQ_ACCEPT);
+    InitChannelOpenHarness(&harness, NULL, 0);
+    AssertIntEQ(wolfSSH_CTX_SetChannelReqAnyCb(harness.ctx,
+                RecordingChannelReqAnyCb), WS_SUCCESS);
+    channel = SeedConfirmedSessionChannel(&harness);
+
+    inSz = BuildChannelStringRequestPacket(channel->channel, "exec", 1,
+            "ls", in, sizeof(in));
+    AssertIntEQ(ReplyToPacket(&harness, in, inSz), MSGID_CHANNEL_SUCCESS);
+    AssertIntEQ(anyReqCbCalls, 1);
+    AssertIntEQ(channel->sessionGranted, 1);
+
+    inSz = BuildChannelStringRequestPacket(channel->channel, "exec", 1,
+            "whoami", in, sizeof(in));
+    AssertIntEQ(ReplyToPacket(&harness, in, inSz), MSGID_CHANNEL_FAILURE);
+    AssertIntEQ(anyReqCbCalls, 2);
+    AssertIntEQ(WSTRCMP(wolfSSH_ChannelGetSessionCommand(channel), "ls"), 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
@@ -20380,6 +20464,8 @@ int main(int argc, char** argv)
     TestChannelGetSessionGrantedAccessor();
     TestMalformedSessionRequestSkipsCallback();
     TestSessionReqCallbackMayFreeChannel();
+    TestSecondSessionRequestRefused();
+    TestSecondSessionRequestRefusedOverGenericGrant();
     TestAppChannelsAcceptKeepsStopWithPendingOutput();
 #ifdef WOLFSSH_SFTP
     TestSftpAcceptAppChannelsNeedsSession();