Harden the PID file, login shell and port parsing

apps/wolfssh/common.c

@@ -912,6 +912,29 @@ void ClientFreeBuffers(void)
 }
 
 
+/* Parse a port number. Text that is not wholly a number in 1..65535 is
+ * rejected with WS_BAD_ARGUMENT rather than truncated, and *port is left
+ * untouched. */
+int ClientParsePort(const char* in, word16* port)
+{
+    char* endptr = NULL;
+    long portVal;
+
+    if (in == NULL || port == NULL) {
+        return WS_BAD_ARGUMENT;
+    }
+
+    portVal = strtol(in, &endptr, 10);
+    if (endptr == in || *endptr != '\0' || portVal < 1 || portVal > 65535) {
+        fprintf(stderr, "invalid port \"%s\"\n", in);
+        return WS_BAD_ARGUMENT;
+    }
+    *port = (word16)portVal;
+
+    return WS_SUCCESS;
+}
+
+
 /* Parse an SSH destination into its parts. Two forms are accepted:
  *   - [user@]hostname
  *   - ssh://[user@]hostname[:port]
@@ -923,9 +946,9 @@ void ClientFreeBuffers(void)
  * *hostname. *hostname is set only when host text is present, so a malformed
  * "ssh://" or "ssh://user@" leaves it untouched and the caller's NULL check can
  * reject it cleanly. *port is overwritten only when an explicit port is given
- * (URI form); a port that is non-numeric or outside 1..65535 is rejected with
- * WS_BAD_ARGUMENT rather than silently truncated. Returns WS_SUCCESS on
- * success, or a negative WS error code. */
+ * (URI form); a port that is empty, non-numeric or outside 1..65535 is
+ * rejected with WS_BAD_ARGUMENT rather than defaulted or silently truncated.
+ * Returns WS_SUCCESS on success, or a negative WS error code. */
 int ClientParseDestination(const char* in, char** user, char** hostname,
         word16* port)
 {
@@ -936,8 +959,6 @@ int ClientParseDestination(const char* in, char** user, char** hostname,
     char* found = NULL;
     char* newUser = NULL;
     char* newHostname = NULL;
-    char* endptr = NULL;
-    long portVal = 0;
     word16 newPort = 0;
     size_t sz = 0;
     int checkPort = 0;
@@ -1006,18 +1027,7 @@ int ClientParseDestination(const char* in, char** user, char** hostname,
     }
 
     if (ret == WS_SUCCESS && found != NULL) {
-        cursor = found + 1;
-        if (*cursor != 0) {
-            portVal = strtol(cursor, &endptr, 10);
-            if (endptr == cursor || *endptr != '\0'
-                    || portVal < 1 || portVal > 65535) {
-                fprintf(stderr, "invalid port \"%s\"\n", cursor);
-                ret = WS_BAD_ARGUMENT;
-            }
-            else {
-                newPort = (word16)portVal;
-            }
-        }
+        ret = ClientParsePort(found + 1, &newPort);
     }
 
     if (ret == WS_SUCCESS) {

apps/wolfssh/common.h

@@ -32,6 +32,7 @@ WOLFSSH_LOCAL int ClientPublicKeyCheck(const byte* pubKey, word32 pubKeySz,
         void* ctx);
 WOLFSSH_LOCAL void ClientIPOverride(int flag);
 WOLFSSH_LOCAL void ClientFreeBuffers(void);
+WOLFSSH_LOCAL int ClientParsePort(const char* in, word16* port);
 WOLFSSH_LOCAL int ClientParseDestination(const char* in, char** user,
         char** hostname, word16* port);
 #ifdef WOLFSSH_TEST_INTERNAL

apps/wolfssh/wolfssh.c

@@ -1132,7 +1132,9 @@ static int config_parse_command_line(struct config* config,
                 break;
 
             case 'p':
-                config->port = (word16)atoi(myoptarg);
+                if (ClientParsePort(myoptarg, &config->port) != WS_SUCCESS) {
+                    exit(EXIT_FAILURE);
+                }
                 break;
 
             case 'V':

apps/wolfsshd/configuration.c

@@ -2505,12 +2505,11 @@ void wolfSSHD_ConfigSavePID(const WOLFSSHD_CONFIG* conf)
 #ifndef _WIN32
         /* raw open, not WFOPEN: the O_NOFOLLOW hardening needs a real fd */
 #if defined(WOLFSSH_HAVE_SYMLINK) && WOLFSSH_O_NOFOLLOW == 0
-        /* no O_NOFOLLOW here, so pre-check with a racy best-effort lstat */
-        if (lstat(conf->pidFile, &st) == 0 && S_ISLNK(st.st_mode)) {
-            wolfSSH_Log(WS_LOG_ERROR,
-                "[SSHD] Refusing symlinked PID file %s", conf->pidFile);
-            ok = 0;
-        }
+        /* without O_NOFOLLOW the open below follows a link swapped in after
+         * any check, and the fstat gate accepts a root-owned target */
+        wolfSSH_Log(WS_LOG_ERROR,
+            "[SSHD] No O_NOFOLLOW, not writing PID file %s", conf->pidFile);
+        ok = 0;
 #endif
         if (ok) {
             /* O_EXCL first so 'created' distinguishes creation from reuse; on

apps/wolfsshd/test/run_all_sshd_tests.sh

@@ -41,6 +41,7 @@ extra_test_cases=(
  "sshd_permitroot_test.sh"
  "sshd_permitroot_prohibit_password.sh"
  "sshd_permitroot_forced_cmd.sh"
+ "sshd_long_shell_test.sh"
  "strictmodes_hostkey_negative"
  "upn_unenforceable_negative"
  "sshd_login_grace_test.sh"
@@ -759,6 +760,7 @@ local_sshd_tests=(
  "sshd_permitroot_test.sh"
  "sshd_permitroot_prohibit_password.sh"
  "sshd_permitroot_forced_cmd.sh"
+ "sshd_long_shell_test.sh"
  "strictmodes_hostkey_negative"
  "upn_unenforceable_negative"
  "sshd_login_grace_test.sh"

apps/wolfsshd/test/sshd_long_shell_test.sh

@@ -0,0 +1,112 @@
+#!/bin/bash
+
+# A login shell path longer than wolfsshd's old 80-byte command buffer must be
+# executed whole. The first 79 bytes of the path name a different executable,
+# so a truncated copy would run that one instead. The shell also has to see
+# "-" plus its own basename as argv[0], not "-sh". Creates a throwaway account,
+# so it only runs on Linux with useradd/userdel and passwordless sudo.
+
+if [ -z "$1" ] || [ -z "$2" ]; then
+    echo "expecting host and port as arguments"
+    echo "$0 127.0.0.1 22222"
+    exit 1
+fi
+
+TEST_HOST="$1"
+TEST_PORT="$2"
+PWD=`pwd`
+
+if [ "$(uname -s)" != "Linux" ] || ! command -v useradd >/dev/null 2>&1 || \
+   ! command -v userdel >/dev/null 2>&1 || ! sudo -n true 2>/dev/null; then
+    echo "SKIP: needs root on Linux with useradd/userdel"
+    exit 77
+fi
+
+source ./start_sshd.sh
+
+TEST_CLIENT="../../../examples/client/client"
+PRIVATE_KEY="$PWD/../../../keys/hansel-key-ecc.der"
+PUBLIC_KEY="$PWD/../../../keys/hansel-key-ecc.pub"
+LONG_USER="wolfsshdlsh$$"
+USER_CREATED=""
+SHELL_DIR=""
+
+cleanup() {
+    stop_wolfsshd
+    if [ -n "$USER_CREATED" ]; then
+        sudo userdel "$LONG_USER" 2>/dev/null
+    fi
+    if [ -n "$SHELL_DIR" ]; then
+        sudo rm -rf "$SHELL_DIR"
+    fi
+    rm -f sshd_config_test_long_shell authorized_keys_test_long_shell
+}
+trap cleanup EXIT
+trap 'exit 1' INT TERM HUP
+
+# Fixed short prefix under the world-traversable /tmp, whatever $TMPDIR is, so
+# the 79-byte decoy lands inside SHELL_DIR and the new account can reach it.
+SHELL_DIR=$(mktemp -d /tmp/wolfsshd_lsh.XXXXXX)
+chmod 755 "$SHELL_DIR"
+LONG_DIR="$SHELL_DIR/$(printf 'd%.0s' $(seq 1 60))"
+mkdir "$LONG_DIR"
+chmod 755 "$LONG_DIR"
+LONG_SHELL="$LONG_DIR/long_shell_real"
+SHORT_SHELL="${LONG_SHELL:0:79}"
+if [ ${#LONG_SHELL} -le 80 ] || [ "$SHORT_SHELL" = "$LONG_SHELL" ]; then
+    echo "FAIL: shell path is only ${#LONG_SHELL} bytes"
+    exit 1
+fi
+# A link, not a wrapper script, so sh's $0 is the argv[0] wolfsshd passed.
+ln -s /bin/sh "$LONG_SHELL"
+printf '#!/bin/sh\necho TRUNCATED SHELL RAN\n' > "$SHORT_SHELL"
+chmod 755 "$SHORT_SHELL"
+
+if ! sudo useradd -M -s "$LONG_SHELL" "$LONG_USER"; then
+    echo "SKIP: could not create $LONG_USER"
+    exit 77
+fi
+USER_CREATED=1
+
+sed "s/hansel/$LONG_USER/" $PUBLIC_KEY > authorized_keys_test_long_shell
+
+cat <<EOF > sshd_config_test_long_shell
+Port $TEST_PORT
+Protocol 2
+LoginGraceTime 600
+PermitRootLogin no
+PasswordAuthentication no
+StrictModes no
+UsePrivilegeSeparation no
+UseDNS no
+HostKey $PWD/../../../keys/server-key.pem
+AuthorizedKeysFile $PWD/authorized_keys_test_long_shell
+EOF
+
+start_wolfsshd "sshd_config_test_long_shell"
+if [ -z "$PID" ]; then
+    echo "FAIL: could not start wolfsshd"
+    exit 1
+fi
+
+# -q keeps a debug build's log, which echoes the command text, out of RESULT.
+# The expected line only exists once the shell has expanded $0.
+RESULT=$(timeout 10 $TEST_CLIENT -q -c 'echo LONG SHELL RAN as $0' \
+    -u "$LONG_USER" -i $PRIVATE_KEY -j $PUBLIC_KEY \
+    -h "$TEST_HOST" -p "$TEST_PORT" 2>&1)
+CLIENT_RESULT=$?
+echo "$RESULT"
+if echo "$RESULT" | grep -q "TRUNCATED SHELL RAN"; then
+    echo "FAIL: wolfsshd ran the truncated shell path"
+    exit 1
+fi
+if [ "$CLIENT_RESULT" != 0 ]; then
+    echo "FAIL: client exited $CLIENT_RESULT"
+    exit 1
+fi
+if ! echo "$RESULT" | grep -qx "LONG SHELL RAN as -long_shell_real"; then
+    echo "FAIL: the long shell did not run as -long_shell_real"
+    exit 1
+fi
+
+exit 0

apps/wolfsshd/test/test_configuration.c

@@ -6013,6 +6013,38 @@ static int pidSave(const char* confPath, const char* pidTarget)
     return ret;
 }
 
+#if defined(WOLFSSH_HAVE_SYMLINK) && WOLFSSH_O_NOFOLLOW == 0
+/* Without O_NOFOLLOW a symlink check cannot be made atomic, so
+ * wolfSSHD_ConfigSavePID writes no PID file at all. */
+static int test_ConfigSavePID(void)
+{
+    int ret = WS_SUCCESS;
+    char base[] = "/tmp/wolfsshd_pidXXXXXX";
+    char conf[80] = "";
+    char pidPath[96] = "";
+    struct stat st;
+
+    if (mkdtemp(base) == NULL) {
+        Log("    mkdtemp failed.\n");
+        ret = WS_FATAL_ERROR;
+    }
+    if (ret == WS_SUCCESS) {
+        snprintf(conf, sizeof(conf), "%s/sshd_config", base);
+        snprintf(pidPath, sizeof(pidPath), "%s/wolfsshd.pid", base);
+        ret = pidSave(conf, pidPath);
+    }
+    if (ret == WS_SUCCESS) {
+        ret = smExpect("PID file refused without O_NOFOLLOW",
+            (stat(pidPath, &st) != 0) ? WS_SUCCESS : WS_FATAL_ERROR, 1);
+    }
+
+    unlink(pidPath);
+    unlink(conf);
+    rmdir(base);
+
+    return ret;
+}
+#else
 /* wolfSSHD_ConfigSavePID must refuse a symlink at the PID path so a planted
  * link cannot truncate another file, and must leave the PID file without group
  * or world write even under a permissive umask, whether it creates the file or
@@ -6096,9 +6128,8 @@ static int test_ConfigSavePID(void)
         }
     }
 
-    /* Scenario 2: a symlink at the PID path is refused, link target untouched.
-     * The common build exercises the atomic O_NOFOLLOW path; the lstat fallback
-     * needs a symlink-capable platform without O_NOFOLLOW. */
+    /* Scenario 2: a symlink at the PID path is refused, link target untouched
+     * (O_NOFOLLOW makes the open fail atomically). */
     if (ret == WS_SUCCESS) {
         f = fopen(victim, "w");
         if (f == NULL) {
@@ -6305,6 +6336,7 @@ static int test_ConfigSavePID(void)
 
     return ret;
 }
+#endif /* WOLFSSH_HAVE_SYMLINK && WOLFSSH_O_NOFOLLOW == 0 */
 #endif /* !_WIN32 */
 #if defined(WOLFSSH_OSSH_CERTS) && !defined(_WIN32)
 /* Direct coverage for the bit-level prefix matcher used by the certificate

apps/wolfsshd/wolfsshd.c

@@ -2902,7 +2902,7 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
         /* Child process. Any setup failure below exit(1)s rather than returning
          * into the connection handler while still at a raised privilege level. */
         const char *args[] = {"-sh", NULL, NULL, NULL};
-        char cmd[MAX_COMMAND_SZ];
+        const char* cmd;
         char shell[MAX_COMMAND_SZ];
         int ret;
 
@@ -2965,19 +2965,21 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
         setenv("LOGNAME", pPasswd->pw_name, 1);
         setenv("SHELL", pPasswd->pw_shell, 1);
 
-        if (WSTRLEN(pPasswd->pw_shell) < sizeof(shell)) {
-            char* cursor;
-            char* start;
+        /* login shell argv[0] is "-" plus the basename, which fits even when
+         * the whole path would not */
+        {
+            const char* base = pPasswd->pw_shell;
+            const char* slash;
 
-            WSTRNCPY(shell, pPasswd->pw_shell, sizeof(shell)-1);
-            shell[sizeof(shell)-1] = 0;
-            cursor = shell;
-            do {
-                start = cursor;
-                *cursor = '-';
-                cursor = WSTRCHR(start, '/');
-            } while (cursor && *cursor != '\0');
-            args[0] = start;
+            while ((slash = WSTRCHR(base, '/')) != NULL) {
+                base = slash + 1;
+            }
+            if (WSTRLEN(base) + 1 < sizeof(shell)) {
+                shell[0] = '-';
+                WSTRNCPY(shell + 1, base, sizeof(shell) - 2);
+                shell[sizeof(shell) - 1] = 0;
+                args[0] = shell;
+            }
         }
 
         rc = chdir(pPasswd->pw_dir);
@@ -2987,12 +2989,13 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh,
             pPasswd->pw_dir);
         }
 
-        /* default to /bin/sh if user shell is not set */
+        /* default to /bin/sh if user shell is not set; exec pw_shell itself,
+         * a copy into a fixed buffer would truncate a long path */
         if (pPasswd->pw_shell && WSTRLEN(pPasswd->pw_shell)) {
-            WSNPRINTF(cmd, sizeof(cmd), "%s", pPasswd->pw_shell);
+            cmd = pPasswd->pw_shell;
         }
         else {
-            WSNPRINTF(cmd, sizeof(cmd), "%s", "/bin/sh");
+            cmd = "/bin/sh";
         }
 
         errno = 0;

examples/client/client.c

@@ -913,11 +913,9 @@ THREAD_RETURN WOLFSSH_THREAD client_test(void* args)
                 if (myoptarg == NULL) {
                     err_sys("port number cannot be NULL");
                 }
-                port = (word16)atoi(myoptarg);
-                #if !defined(NO_MAIN_DRIVER) || defined(USE_WINDOWS_API)
-                    if (port == 0)
-                        err_sys("port number cannot be 0");
-                #endif
+                if (ClientParsePort(myoptarg, &port) != WS_SUCCESS) {
+                    err_sys("port number must be 1 to 65535");
+                }
                 break;
 
             case 'u':