wolfsshd: refuse sessions it cannot serve

apps/wolfsshd/test/run_all_sshd_tests.sh

@@ -9,6 +9,7 @@ test_cases=(
  "sshd_large_sftp_test.sh"
  "sshd_bad_sftp_test.sh"
  "sshd_sftp_idle_cpu_test.sh"
+ "sshd_bad_subsystem_test.sh"
  "sshd_scp_fail.sh"
  "sshd_term_close_test.sh"
  "sshd_stdin_eof_test.sh"

apps/wolfsshd/test/sshd_bad_subsystem_test.sh

@@ -0,0 +1,90 @@
+#!/bin/sh
+
+# sshd local test: a subsystem the daemon does not serve is refused at the
+# request, so the client sees CHANNEL_FAILURE rather than a session that
+# is accepted and then dropped. Uses the system OpenSSH client, since the
+# in-tree clients only ask for sftp.
+
+# Not named PWD: the shell rewrites that variable on every cd, so a saved
+# copy would not survive the cd to the repository root below.
+TESTDIR=`pwd`
+cd ../../..
+
+USER="$3"
+if [ -z "$USER" ]; then
+    USER=`whoami`
+fi
+PRIVATE_KEY="./keys/hansel-key-ecc.pem"
+
+if [ -z "$1" ] || [ -z "$2" ]; then
+    echo "expecting host and port as arguments"
+    echo "./sshd_bad_subsystem_test.sh 127.0.0.1 22222"
+    exit 1
+fi
+
+if ! command -v ssh >/dev/null 2>&1; then
+    echo "OpenSSH client not found, skipping"
+    exit 77
+fi
+
+# The regression this test looks for is a request the daemon never answers,
+# which leaves the client waiting. Bound every call so that hangs the test
+# rather than the suite.
+if ! command -v timeout >/dev/null 2>&1; then
+    echo "timeout not found, skipping"
+    exit 77
+fi
+TIMEOUT="timeout 20"
+
+# OpenSSH refuses a key file other users can read.
+KEY=`mktemp 2>/dev/null` || KEY=`mktemp -t sshdbadsubsys`
+OUT=`mktemp 2>/dev/null` || OUT=`mktemp -t sshdbadsubsysout`
+if [ -z "$KEY" ] || [ ! -f "$KEY" ] || [ -z "$OUT" ] || [ ! -f "$OUT" ]; then
+    echo "could not create temp files"
+    rm -f "$KEY" "$OUT"
+    exit 1
+fi
+trap 'rm -f "$KEY" "$OUT"' EXIT
+
+cat "$PRIVATE_KEY" > "$KEY" || exit 1
+chmod 600 "$KEY"
+
+ssh_to_sshd() {
+    $TIMEOUT ssh -p "$2" -i "$KEY" -o IdentitiesOnly=yes \
+        -o StrictHostKeyChecking=no \
+        -o UserKnownHostsFile=/dev/null -o PreferredAuthentications=publickey \
+        -o BatchMode=yes -o ConnectTimeout=5 "$USER@$1" "$3" "$4"
+}
+
+# Control: the same client and key can run a command.
+ssh_to_sshd "$1" "$2" "echo ok" > "$OUT" 2>&1
+RESULT=$?
+if [ "$RESULT" != "0" ] || ! grep -q "^ok" "$OUT"; then
+    echo "Control exec through OpenSSH failed ($RESULT):"
+    cat "$OUT"
+    exit 1
+fi
+
+# A subsystem nothing serves: the client reports the refusal and exits
+# non-zero. Check the timeout first, its 124 is non-zero too but means the
+# request went unanswered, the opposite of what this test wants.
+ssh_to_sshd "$1" "$2" -s no-such-subsystem > "$OUT" 2>&1
+RESULT=$?
+if [ "$RESULT" = "124" ]; then
+    echo "The unknown subsystem request went unanswered:"
+    cat "$OUT"
+    exit 1
+fi
+if [ "$RESULT" = "0" ]; then
+    echo "Expecting the unknown subsystem request to fail"
+    cat "$OUT"
+    exit 1
+fi
+if ! grep -q "subsystem request failed" "$OUT"; then
+    echo "Expecting the client to report the refused subsystem request:"
+    cat "$OUT"
+    exit 1
+fi
+
+cd "$TESTDIR"
+exit 0

apps/wolfsshd/wolfsshd.c

@@ -471,6 +471,94 @@ static void CleanupCTX(WOLFSSHD_CONFIG* conf, WOLFSSH_CTX** ctx,
     (void)conf;
 }
 
+/* Answers a shell, exec or subsystem request as it arrives: a session this
+ * build cannot serve is refused with CHANNEL_FAILURE, rather than accepted
+ * and then dropped once the session is up. Returns 0 to accept and 1 to
+ * refuse. The command is NULL when the request carried none that fit. */
+static int SessionRequestCb(WOLFSSH_CHANNEL* channel, void* vCtx)
+{
+    WOLFSSHD_CONNECTION* conn = (WOLFSSHD_CONNECTION*)vCtx;
+    const char* cmd;
+    const char* reason = NULL;
+    int rej = 1;
+
+    if (conn == NULL || channel == NULL) {
+        return 1;
+    }
+
+    cmd = wolfSSH_ChannelGetSessionCommand(channel);
+    switch (wolfSSH_ChannelGetSessionType(channel)) {
+        case WOLFSSH_SESSION_SHELL:
+        #ifdef WOLFSSH_SHELL
+            rej = 0;
+        #else
+            reason = "shell support is disabled";
+        #endif
+            break;
+
+        case WOLFSSH_SESSION_EXEC:
+            if (cmd == NULL) {
+                reason = "exec request carried no command";
+                break;
+            }
+        #ifdef WOLFSSH_SCP
+            /* Shared with the SCP divert in wolfSSH_accept(), so the two
+             * cannot disagree about what starts a transfer. */
+            if (wolfSSH_ChannelCommandIsScp(channel) == 1) {
+                rej = 0;
+                break;
+            }
+        #endif
+        #ifdef WOLFSSH_SHELL
+            rej = 0;
+        #else
+            reason = "exec support is disabled";
+        #endif
+            break;
+
+        case WOLFSSH_SESSION_SUBSYSTEM:
+            if (cmd == NULL) {
+                reason = "subsystem request carried no name";
+            }
+        #ifdef WOLFSSH_SFTP
+            /* Matched whole, length and bytes, as the sftp divert asks:
+             * sftp with an embedded NUL is another subsystem. */
+            else if (wolfSSH_ChannelGetSessionCommandSz(channel)
+                        == (word32)WSTRLEN("sftp")
+                    && WSTRCMP(cmd, "sftp") == 0) {
+                rej = 0;
+            }
+        #endif
+            else {
+                reason = "unknown or unsupported subsystem";
+            }
+            break;
+
+        case WOLFSSH_SESSION_UNKNOWN:
+        case WOLFSSH_SESSION_TERMINAL:
+        default:
+            reason = "unsupported session type";
+            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. */
+    if (!rej && wolfSSH_ChannelGetSessionGranted(channel) == 1) {
+        rej = 1;
+        reason = "a session is already running on the channel";
+    }
+
+    if (rej) {
+        wolfSSH_Log(WS_LOG_ERROR,
+            "[SSHD] Refusing session request from %s: %s [%s]",
+            conn->ip, reason, cmd != NULL ? cmd : "");
+    }
+
+    return rej;
+}
+
+
 #if defined(WOLFSSH_CERTS) && defined(WOLFSSH_WINDOWS_CERT_STORE)
 /* Returns 1 only for the store hives that need elevation to write: the three
  * LOCAL_MACHINE locations. Every other hive (per-user, per-service,
@@ -893,6 +981,9 @@ static int SetupCTX(WOLFSSHD_CONFIG* conf, WOLFSSH_CTX** ctx,
     if (ret == WS_SUCCESS) {
         wolfSSH_SetUserAuth(*ctx, DefaultUserAuth);
         wolfSSH_SetUserAuthResult(*ctx, UserAuthResult);
+        wolfSSH_CTX_SetChannelReqShellCb(*ctx, SessionRequestCb);
+        wolfSSH_CTX_SetChannelReqExecCb(*ctx, SessionRequestCb);
+        wolfSSH_CTX_SetChannelReqSubsysCb(*ctx, SessionRequestCb);
     }
 
     /* set banner to display on connection */
@@ -3511,6 +3602,7 @@ static void* HandleConnection(void* arg)
         /* let UserAuthResult reach this connection to cancel the grace timer
          * and to reach conn->auth for the cert force-command */
         wolfSSH_SetUserAuthResultCtx(ssh, conn);
+        wolfSSH_SetChannelReqCtx(ssh, conn);
     #if defined(WOLFSSH_OSSH_CERTS) && !defined(_WIN32)
         /* Unix-only: each connection is a forked child with its own copy of the
          * auth struct. Windows does not enforce OpenSSH certs. */

src/internal.c

@@ -13123,9 +13123,13 @@ static int DoChannelRequestSession(WOLFSSH* ssh, word32 channelId,
 
         ret = GetStringAlloc(heap, &command, &commandSz, buf, len, idx);
         if (ret == WS_SUCCESS)
-            WLOG(WS_LOG_DEBUG, "  command = %s", command);
+            WLOG(WS_LOG_DEBUG, "  %s = %s",
+                    sessionType == WOLFSSH_SESSION_SUBSYSTEM
+                        ? "subsystem" : "command", command);
         else
-            WLOG(WS_LOG_DEBUG, "  command = %s", "<bad value>");
+            WLOG(WS_LOG_DEBUG, "  %s = %s",
+                    sessionType == WOLFSSH_SESSION_SUBSYSTEM
+                        ? "subsystem" : "command", "<bad value>");
     }
 
     if (ret == WS_SUCCESS) {

src/ssh.c

@@ -818,7 +818,8 @@ int wolfSSH_accept(WOLFSSH* ssh)
 #ifdef WOLFSSH_SCP
                 if (ssh->channelList != NULL
                         && ssh->channelList->sessionGranted
-                        && ChannelCommandIsScp(ssh)) {
+                        && wolfSSH_ChannelCommandIsScp(ssh->channelList)
+                                == 1) {
                     ssh->acceptState = ACCEPT_INIT_SCP_TRANSFER;
                     WLOG(WS_LOG_DEBUG, acceptState, "ACCEPT_INIT_SCP_TRANSFER");
                     return WS_SCP_INIT;
@@ -5760,6 +5761,17 @@ word32 wolfSSH_ChannelGetSessionCommandSz(const WOLFSSH_CHANNEL* channel)
 }
 
 
+/* returns 1 if a session was granted on the channel, 0 if not, and
+ * negative on failure */
+int wolfSSH_ChannelGetSessionGranted(const WOLFSSH_CHANNEL* channel)
+{
+    if (channel == NULL) {
+        return WS_BAD_ARGUMENT;
+    }
+    return channel->sessionGranted;
+}
+
+
 int wolfSSH_CTX_SetChannelOpenCb(WOLFSSH_CTX* ctx, WS_CallbackChannelOpen cb)
 {
     int ret = WS_SSH_CTX_NULL_E;

src/wolfscp.c

@@ -1040,20 +1040,34 @@ WOLFSSH_API int wolfSSH_SetScpErrorMsg(WOLFSSH* ssh, const char* message)
     return ret;
 }
 
-/* Determine if channel command sent in initial negotiation is scp.
- * Return 1 if yes, 0 if no */
-int ChannelCommandIsScp(WOLFSSH* ssh)
+/* Determine if the channel's session command is scp. See wolfscp.h for
+ * the contract; "scp" must stand as its own token. */
+int wolfSSH_ChannelCommandIsScp(const WOLFSSH_CHANNEL* channel)
 {
     const char* cmd;
+    word32 cmdSz;
+    word32 scpSz = (word32)WSTRLEN("scp");
+    word32 i;
     int ret = 0;
 
-    if (ssh == NULL)
+    if (channel == NULL)
         return WS_BAD_ARGUMENT;
 
-    cmd = wolfSSH_GetSessionCommand(ssh);
-    if (cmd != NULL && WSTRLEN(cmd) >= 3) {
-        if (WSTRNCMP(cmd, "scp", 3) == 0)
-            ret = 1;
+    cmd = wolfSSH_ChannelGetSessionCommand(channel);
+    cmdSz = wolfSSH_ChannelGetSessionCommandSz(channel);
+
+    if (cmd != NULL && cmdSz >= scpSz
+            && WSTRNCMP(cmd, "scp", scpSz) == 0
+            && (cmdSz == scpSz || cmd[scpSz] == ' ')) {
+        ret = 1;
+    }
+
+    /* The parse that follows is a C string walk, so a NUL inside the
+     * command would drop the rest of it. Refuse rather than transfer
+     * something other than what was asked for. */
+    for (i = 0; ret == 1 && i < cmdSz; i++) {
+        if (cmd[i] == '\0')
+            ret = 0;
     }
 
     return ret;