wolfsshd: fail closed when a per-connection privilege drop fails

.gitignore

@@ -68,6 +68,7 @@ examples/scpclient/wolfscp
 apps/wolfssh/wolfssh
 apps/wolfsshd/wolfsshd
 apps/wolfsshd/test/test_configuration
+apps/wolfsshd/test/wolfsshd_faultinj
 apps/wolfsshd/test/log.txt
 apps/wolfsshd/test/sshd_config_*
 apps/wolfsshd/test/authorized_keys_test

apps/wolfsshd/auth.c

@@ -93,8 +93,34 @@
 #endif
 
 #if defined(WOLFSSHD_UNIT_TEST) && !defined(_WIN32)
+#ifdef WOLFSSHD_FAULT_INJECT
+/* Simulate a kernel privilege-drop failure when WOLFSSHD_FAULT_PRIVDROP is set
+ * in the environment, so the test suite can reach the fail-closed paths taken
+ * by the subsystem handlers when the drop to the authenticated user fails. */
+static int wsshd_setregid_fault(WGID_T rgid, WGID_T egid)
+{
+    if (getenv("WOLFSSHD_FAULT_PRIVDROP") != NULL) {
+        errno = EPERM;
+        return -1;
+    }
+    return setregid(rgid, egid);
+}
+
+static int wsshd_setreuid_fault(WUID_T ruid, WUID_T euid)
+{
+    if (getenv("WOLFSSHD_FAULT_PRIVDROP") != NULL) {
+        errno = EPERM;
+        return -1;
+    }
+    return setreuid(ruid, euid);
+}
+
+int (*wsshd_setregid_cb)(WGID_T, WGID_T) = wsshd_setregid_fault;
+int (*wsshd_setreuid_cb)(WUID_T, WUID_T) = wsshd_setreuid_fault;
+#else
 int (*wsshd_setregid_cb)(WGID_T, WGID_T) = setregid;
 int (*wsshd_setreuid_cb)(WUID_T, WUID_T) = setreuid;
+#endif /* WOLFSSHD_FAULT_INJECT */
 #endif
 
 struct WOLFSSHD_AUTH {

apps/wolfsshd/include.am

@@ -21,4 +21,17 @@ apps_wolfsshd_test_test_configuration_DEPENDENCIES = src/libwolfssh.la
 # the getgrouplist buffer-growth/realloc loop under the sanitizers.
 apps_wolfsshd_test_test_configuration_CPPFLAGS     = $(AM_CPPFLAGS) -DWOLFSSH_SSHD -DWOLFSSHD_UNIT_TEST -DWOLFSSHD_GROUP_LIST_INIT=1 -I$(srcdir)/apps/wolfsshd/
 
+# Same daemon as wolfsshd, but able to simulate a failing privilege drop when
+# WOLFSSHD_FAULT_PRIVDROP is set. Never installed; used by
+# sshd_privdrop_fail_test.sh to exercise the fail-closed subsystem paths.
+noinst_PROGRAMS += apps/wolfsshd/test/wolfsshd_faultinj
+apps_wolfsshd_test_wolfsshd_faultinj_SOURCES = apps/wolfsshd/wolfsshd.c \
+                                               apps/wolfsshd/configuration.c \
+                                               apps/wolfsshd/configuration.h \
+                                               apps/wolfsshd/auth.c \
+                                               apps/wolfsshd/auth.h
+apps_wolfsshd_test_wolfsshd_faultinj_LDADD        = src/libwolfssh.la
+apps_wolfsshd_test_wolfsshd_faultinj_DEPENDENCIES = src/libwolfssh.la
+apps_wolfsshd_test_wolfsshd_faultinj_CPPFLAGS     = $(AM_CPPFLAGS) -DWOLFSSH_SSHD -DWOLFSSHD_UNIT_TEST -DWOLFSSHD_FAULT_INJECT -I$(srcdir)/apps/wolfsshd/
+
 endif BUILD_SSHD

apps/wolfsshd/test/run_all_sshd_tests.sh

@@ -397,9 +397,10 @@ else
         run_test "sshd_empty_password_test.sh"
         run_strictmodes_negative_test
         run_test "sshd_login_grace_test.sh"
+        run_test "sshd_privdrop_fail_test.sh"
     else
         printf "Skipping tests that need to setup local SSHD\n"
-        SKIPPED=$((SKIPPED+5))
+        SKIPPED=$((SKIPPED+6))
     fi
 
     # these tests run with X509 sshd-config loaded

apps/wolfsshd/test/sshd_privdrop_fail_test.sh

@@ -0,0 +1,161 @@
+#!/bin/bash
+
+# Regression test for the fail-closed privilege-drop paths in wolfsshd.
+#
+# When the per-connection drop to the authenticated user's uid/gid fails, the
+# subsystem handlers must terminate the connection process. Previously they
+# attempted a fallback that is a no-op under "UsePrivilegeSeparation no" and
+# then returned, leaving HandleConnection to run wolfSSH_shutdown and up to ten
+# wolfSSH_worker iterations while still at the daemon's elevated privilege
+# level.
+#
+# The drop is forced to fail with the wolfsshd_faultinj build, which honors
+# WOLFSSHD_FAULT_PRIVDROP. Every subsystem that drops privileges is driven:
+# exec/shell (SHELL_Subsystem), sftp (SFTP_Subsystem) and scp (SCP_Subsystem).
+
+if [ -z "$1" ] || [ -z "$2" ]; then
+    echo "expecting host and port as arguments"
+    echo "./sshd_privdrop_fail_test.sh 127.0.0.1 22222"
+    exit 1
+fi
+
+PWD=`pwd`
+USER=`whoami`
+TEST_HOST="$1"
+TEST_PORT="$2"
+
+FAULT_SSHD="./wolfsshd_faultinj"
+if [ ! -x "$FAULT_SSHD" ]; then
+    echo "SKIP: $FAULT_SSHD not built"
+    exit 77
+fi
+
+if [ -f ./log.txt ]; then
+    sudo rm -rf log.txt
+fi
+touch log.txt
+
+TEST_CLIENT="../../../examples/client/client"
+SFTP_CLIENT="../../../examples/sftpclient/wolfsftp"
+SCP_CLIENT="../../../examples/scpclient/wolfscp"
+PRIVATE_KEY="../../../keys/hansel-key-ecc.der"
+PUBLIC_KEY="../../../keys/hansel-key-ecc.pub"
+
+# Small payload for the sftp/scp transfers. The connection dies at the failed
+# drop long before any data moves, so the contents do not matter.
+PAYLOAD="privdrop_payload.txt"
+echo "privdrop" > "$PAYLOAD"
+
+source ./start_sshd.sh
+
+cat <<EOF > sshd_config_test_privdrop
+Port $TEST_PORT
+Protocol 2
+LoginGraceTime 600
+PermitRootLogin yes
+PasswordAuthentication yes
+PermitEmptyPasswords no
+UsePrivilegeSeparation no
+UseDNS no
+HostKey $PWD/../../../keys/server-key.pem
+AuthorizedKeysFile $PWD/authorized_keys_test
+EOF
+
+# Run the fault-injecting daemon so the drop to the authenticated user fails.
+# "UsePrivilegeSeparation no" is the worst case: the old fallback did nothing
+# there, so the handler would have carried on as root.
+SSHD_BIN="$FAULT_SSHD"
+SSHD_ENV="WOLFSSHD_FAULT_PRIVDROP=1"
+export SSHD_BIN SSHD_ENV
+
+start_wolfsshd "sshd_config_test_privdrop"
+
+# Stop the daemon and drop the generated files on every exit path, including
+# the failure exits below. log.txt is left in place for debugging, matching the
+# other tests in this directory.
+cleanup() {
+    stop_wolfsshd
+    # stop_wolfsshd kills the pid it captured at startup, which can be a
+    # wrapper rather than the daemon that outlived it. The fault binary is
+    # unique to this test, so clearing it by name cannot hit another daemon.
+    sudo pkill -f wolfsshd_faultinj > /dev/null 2>&1
+    rm -f sshd_config_test_privdrop "$PAYLOAD"
+    return 0
+}
+trap cleanup EXIT
+
+DEADLINE=30
+
+# Drives one client and checks the daemon's behavior after the drop fails. The
+# client is expected to lose its connection, so its exit status is not the
+# assertion and it is killed if it outlives the deadline.
+check_subsystem() {
+    LABEL="$1"
+    shift
+
+    BEFORE=`grep -c "Error setting user ID" log.txt`
+
+    "$@" > /dev/null 2>&1 &
+    CLIENT_PID=$!
+
+    # Wait, bounded, for the drop error to be logged and the connection process
+    # to exit, leaving only the listening daemon. A process that has exited
+    # cannot log anything more, so the absence check below is not a race.
+    WAITED=0
+    while [ "$WAITED" -lt "$DEADLINE" ]; do
+        AFTER=`grep -c "Error setting user ID" log.txt`
+        LIVE=`pgrep -f wolfsshd_faultinj | wc -l | tr -d ' '`
+        if [ "$AFTER" -gt "$BEFORE" ] && [ "$LIVE" -le 1 ]; then
+            break
+        fi
+        sleep 1
+        WAITED=`expr $WAITED + 1`
+    done
+
+    kill $CLIENT_PID > /dev/null 2>&1
+    wait $CLIENT_PID > /dev/null 2>&1
+
+    # The drop must actually have failed, otherwise the rest proves nothing.
+    AFTER=`grep -c "Error setting user ID" log.txt`
+    if [ "$AFTER" -le "$BEFORE" ]; then
+        echo "FAIL: $LABEL never reached the privilege drop"
+        exit 1
+    fi
+
+    # A handler still alive here never terminated, which is the failure this
+    # test exists to catch.
+    if [ "$WAITED" -ge "$DEADLINE" ]; then
+        echo "FAIL: $LABEL connection process still running after ${DEADLINE}s"
+        exit 1
+    fi
+
+    # The fail-closed assertion. exit(1) means the connection process is gone,
+    # so HandleConnection can never log this. If it appears, the handler kept
+    # running after the failed drop at an un-dropped privilege level.
+    if grep -q "Attempting to close down connection" log.txt; then
+        echo "FAIL: $LABEL handler continued after a failed privilege drop"
+        exit 1
+    fi
+
+    printf "  %s: connection process terminated on privilege-drop failure\n" \
+        "$LABEL"
+}
+
+# SHELL_Subsystem, via an exec session.
+check_subsystem "exec" \
+    "$TEST_CLIENT" -c 'echo privdrop' -u "$USER" -i "$PRIVATE_KEY" \
+    -j "$PUBLIC_KEY" -h "$TEST_HOST" -p "$TEST_PORT"
+
+# SFTP_Subsystem. -g is a one-shot put, so the client cannot sit at a prompt.
+check_subsystem "sftp" \
+    "$SFTP_CLIENT" -u "$USER" -i "$PRIVATE_KEY" -j "$PUBLIC_KEY" \
+    -g -l "$PAYLOAD" -r "/tmp/privdrop_remote_$$.txt" \
+    -h "$TEST_HOST" -p "$TEST_PORT"
+
+# SCP_Subsystem.
+check_subsystem "scp" \
+    "$SCP_CLIENT" -u "$USER" -i "$PRIVATE_KEY" -j "$PUBLIC_KEY" \
+    -S"$PWD/$PAYLOAD:." -H "$TEST_HOST" -p "$TEST_PORT"
+
+echo "PASS: all subsystems terminate on privilege-drop failure"
+exit 0

apps/wolfsshd/test/start_sshd.sh

@@ -81,8 +81,11 @@ EOF
         fi
     fi
 
-    # find a port
-    sudo ../wolfsshd -d -E ./log.txt -f "$CONFIG"
+    # Default to the normal daemon. A test can set SSHD_BIN to run a different
+    # build (e.g. ./wolfsshd_faultinj) and SSHD_ENV to pass it environment,
+    # which plain sudo would otherwise strip.
+    SSHD_BIN="${SSHD_BIN:-../wolfsshd}"
+    sudo env $SSHD_ENV "$SSHD_BIN" -d -E ./log.txt -f "$CONFIG"
 
     # set the PID of started sshd
     NEW_PID=`ps -e | grep wolfsshd | grep -oE "[0-9]+"`