From 02014ea201c534bb03497a355c01e1a3498e1f67 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 15 Oct 2026 14:05:27 -0700 Subject: [PATCH 1/2] wolfsshd: apply ForceCommand to SFTP and SCP A ForceCommand other than a bare "internal-sftp" now denies the SFTP subsystem, and any ForceCommand denies SCP, as sshd_config(5) describes it: a forced command applies to shell, command, and subsystem execution. The session dispatch checks the configured command alongside a certificate's, so either one on its own confines the session. - the subsystem arm and both SCP arms consult the configured ForceCommand, not only a certificate's force-command - "internal-sftp" followed by options denies SFTP, since no option is applied, while the shell still refuses it as "internal-sftp" - every site testing for "internal-sftp" now shares one predicate - root's transfer refusal no longer points at ForceCommand internal-sftp, which does not permit SCP - tests require a ForceCommand to deny SFTP and SCP, read the daemon's decisions from its log, and pair a configured internal-sftp with a cert Issue: ZD-22623 Issue: F-10597 --- apps/wolfsshd/test/sshd_forcedcmd_test.sh | 134 +++++++++++++++++++++- apps/wolfsshd/test/sshd_ossh_cert_test.sh | 37 +++++- apps/wolfsshd/wolfsshd.c | 94 +++++++++++---- 3 files changed, 229 insertions(+), 36 deletions(-) diff --git a/apps/wolfsshd/test/sshd_forcedcmd_test.sh b/apps/wolfsshd/test/sshd_forcedcmd_test.sh index 024a54c40..05f035130 100755 --- a/apps/wolfsshd/test/sshd_forcedcmd_test.sh +++ b/apps/wolfsshd/test/sshd_forcedcmd_test.sh @@ -21,7 +21,45 @@ source ./start_sshd.sh # root daemon holding the shared test port, so every later test in the suite # would talk to this config. stop_wolfsshd clears PID, so this is a no-op after # each explicit stop below. -trap stop_wolfsshd EXIT +cleanup() { + stop_wolfsshd + rm -f "$TESTDIR/scp_src.dat" "$TESTDIR/scp_dst.dat" + rm -f "$TESTDIR/sshd_config_test_forcedcmd_args" + rm -f "$TESTDIR/sshd_config_test_forcedcmd_prefix" +} +trap cleanup EXIT + +# The daemon's decisions are read from log.txt, which the whole run shares: +# log_mark notes its length and log_since prints what came after. +log_mark() { + sudo cat "$TESTDIR/log.txt" | wc -l +} + +log_since() { + sudo cat "$TESTDIR/log.txt" | tail -n +`expr $1 + 1` +} + +# Native SCP is an exec, a path of its own. wolfscp masks its exit code, so +# the decision is read from the log. Fails the test unless SCP is denied. +check_scp_denied() { + if [ ! -x "$TEST_SCP" ]; then + return 0 + fi + MARK=`log_mark` + rm -f "$TESTDIR/scp_dst.dat" + echo "scp payload" > "$TESTDIR/scp_src.dat" + $TEST_SCP -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY \ + -S"$TESTDIR/scp_src.dat:$TESTDIR/scp_dst.dat" \ + -H $TEST_HOST -p $TEST_PORT > /dev/null 2>&1 || true + if ! log_since $MARK | grep -q "denying SCP"; then + echo "SCP should be denied with $1" + exit 1 + fi + if [ -f "$TESTDIR/scp_dst.dat" ]; then + echo "SCP wrote its destination despite $1" + exit 1 + fi +} cat < sshd_config_test_forcedcmd Port $TEST_PORT @@ -44,6 +82,7 @@ cd ../../.. TEST_CLIENT="./examples/client/client" TEST_SFTP="./examples/sftpclient/wolfsftp" +TEST_SCP="./examples/scpclient/wolfscp" PRIVATE_KEY="./keys/hansel-key-ecc.der" PUBLIC_KEY="./keys/hansel-key-ecc.pub" @@ -59,12 +98,91 @@ fi set -e echo exit | $TEST_SFTP -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY -h $TEST_HOST -p $TEST_PORT +check_scp_denied "ForceCommand internal-sftp" + +cd "$TESTDIR" +stop_wolfsshd + +# sshd_config(5) permits options after "internal-sftp", but none is +# applied: shell is still refused and SFTP is refused too. +cat < sshd_config_test_forcedcmd_args +Port $TEST_PORT +Protocol 2 +LoginGraceTime 600 +PermitRootLogin yes +PasswordAuthentication yes +PermitEmptyPasswords no +UsePrivilegeSeparation no +UseDNS no +HostKey $TESTDIR/../../../keys/server-key.pem +AuthorizedKeysFile $TESTDIR/authorized_keys_test + +Match User $USER + ForceCommand internal-sftp -u 0002 +EOF + +start_wolfsshd "sshd_config_test_forcedcmd_args" +cd ../../.. + +# The refused login exits non-zero and this runs under the "set -e" above. +# Running "internal-sftp -u 0002" as a command would print no "bob" either, +# so the refusal is read from the log. +MARK=`log_mark` +RESULT=$( $TEST_CLIENT -c 'echo bob' -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY -h $TEST_HOST -p $TEST_PORT ) || true +if echo $RESULT | grep bob || + ! log_since $MARK | grep -q "Only SFTP connections allowed"; then + echo "Shell login should fail with internal-sftp and options" + exit 1 +fi + +MARK=`log_mark` +if echo exit | $TEST_SFTP -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY \ + -h $TEST_HOST -p $TEST_PORT; then + echo "SFTP should fail with internal-sftp and options" + exit 1 +fi +if ! log_since $MARK | grep -q "internal-sftp options are not supported"; then + echo "SFTP should be refused for its internal-sftp options" + exit 1 +fi + cd "$TESTDIR" stop_wolfsshd -# A configured ForceCommand that is not "internal-sftp" must still permit the -# SFTP subsystem. Only a certificate force-command denies file transfer, so a -# long-standing ForceCommand config keeps working. +# A name that only starts with "internal-sftp" is some other command. +sed 's/ForceCommand internal-sftp -u 0002/ForceCommand internal-sftpd/' \ + sshd_config_test_forcedcmd_args > sshd_config_test_forcedcmd_prefix + +start_wolfsshd "sshd_config_test_forcedcmd_prefix" +cd ../../.. + +# Refused as a forced command, not as internal-sftp with an option "d". +MARK=`log_mark` +if echo exit | $TEST_SFTP -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY \ + -h $TEST_HOST -p $TEST_PORT; then + echo "SFTP should fail with a command named like internal-sftp" + exit 1 +fi +if ! log_since $MARK | grep -q "denying subsystem request" || + log_since $MARK | grep -q "options are not supported"; then + echo "internal-sftpd was taken for internal-sftp" + exit 1 +fi + +# The shell runs it as the forced command, without the internal-sftp refusal. +MARK=`log_mark` +$TEST_CLIENT -c 'echo bob' -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY \ + -h $TEST_HOST -p $TEST_PORT > /dev/null 2>&1 || true +if log_since $MARK | grep -q "Only SFTP connections allowed"; then + echo "Shell refused internal-sftpd as internal-sftp" + exit 1 +fi + +cd "$TESTDIR" +stop_wolfsshd + +# A ForceCommand other than "internal-sftp" denies the SFTP subsystem and +# native SCP: the client may only run the forced command. cat < sshd_config_test_forcedcmd_sftp Port $TEST_PORT Protocol 2 @@ -84,7 +202,13 @@ EOF start_wolfsshd "sshd_config_test_forcedcmd_sftp" cd ../../.. -echo exit | $TEST_SFTP -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY -h $TEST_HOST -p $TEST_PORT +if echo exit | $TEST_SFTP -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY \ + -h $TEST_HOST -p $TEST_PORT; then + echo "SFTP should fail with a forced command other than internal-sftp" + exit 1 +fi + +check_scp_denied "a forced command" cd "$TESTDIR" stop_wolfsshd diff --git a/apps/wolfsshd/test/sshd_ossh_cert_test.sh b/apps/wolfsshd/test/sshd_ossh_cert_test.sh index 6d496a32f..43fc588de 100755 --- a/apps/wolfsshd/test/sshd_ossh_cert_test.sh +++ b/apps/wolfsshd/test/sshd_ossh_cert_test.sh @@ -228,8 +228,8 @@ HostKey $HOSTKEY TrustedUserCAKeys $WORK/trusted-cas.pub EOF -# Second config, adding a ForceCommand that is not "internal-sftp". Used to -# separate the config-sourced command from the certificate's. +# Second config, adding a ForceCommand that is not "internal-sftp", which +# denies SFTP on its own. cat > "$WORK/sshd_config_ossh_fc" < "$WORK/sshd_config_ossh_isftp" < Date: Thu, 15 Oct 2026 14:46:06 -0700 Subject: [PATCH 2/2] wolfsshd: refuse a config line that does not fit ConfigLoad() reports a line too long for its buffer and fails the load. A cut line used to take effect with whatever fitted, since what is left of it is read as a line of its own and a leftover of two bytes or less is skipped as blank. The buffer is 1024 bytes now, so a long path or forced command still loads. - one byte is read past a full buffer, so a line that ends exactly there, at a newline or the end of the file, is not taken for a cut one - the size is WOLFSSHD_MAX_LINE_SIZE in configuration.h, shared with the test - test_configuration covers a long line, lines that exactly fill the buffer, and a line cut to leave a blank-looking remainder Issue: ZD-22623 --- apps/wolfsshd/configuration.c | 23 ++++- apps/wolfsshd/configuration.h | 3 + apps/wolfsshd/test/test_configuration.c | 125 ++++++++++++++++++++++++ 3 files changed, 147 insertions(+), 4 deletions(-) diff --git a/apps/wolfsshd/configuration.c b/apps/wolfsshd/configuration.c index 074c5fda0..1106161f9 100644 --- a/apps/wolfsshd/configuration.c +++ b/apps/wolfsshd/configuration.c @@ -462,8 +462,6 @@ void wolfSSHD_ConfigFree(WOLFSSHD_CONFIG* conf) } } -#define MAX_LINE_SIZE 160 - typedef struct { int tag; const char* name; @@ -1739,7 +1737,7 @@ static int ConfigLoad(WOLFSSHD_CONFIG* conf, const char* filename, int depth) WFILE *f; WOLFSSHD_CONFIG* currentConfig; int ret = WS_SUCCESS; - char buf[MAX_LINE_SIZE]; + char buf[WOLFSSHD_MAX_LINE_SIZE]; const char* current; if (conf == NULL || filename == NULL) @@ -1761,9 +1759,26 @@ static int ConfigLoad(WOLFSSHD_CONFIG* conf, const char* filename, int depth) depth++; currentConfig = conf; - while ((current = XFGETS(buf, MAX_LINE_SIZE, f)) != NULL) { + while ((current = XFGETS(buf, WOLFSSHD_MAX_LINE_SIZE, f)) != NULL) { int currentSz = (int)XSTRLEN(current); + /* A full buffer with no newline is a cut line, and its leftover is + * read as a line of its own, so going on applies a truncated + * directive. One more byte tells a cut line from one that ends at + * the buffer: a newline, or the end of the file. */ + if (currentSz == WOLFSSHD_MAX_LINE_SIZE - 1 && + buf[currentSz - 1] != '\n') { + char next; + + if (WFREAD(NULL, &next, 1, 1, f) == 1 && next != '\n') { + wolfSSH_Log(WS_LOG_ERROR, + "[SSHD] Config line longer than %d in %s", + WOLFSSHD_MAX_LINE_SIZE - 1, filename); + ret = WS_FATAL_ERROR; + break; + } + } + /* remove leading spaces and tabs */ while (currentSz > 0 && (current[0] == ' ' || current[0] == '\t')) { diff --git a/apps/wolfsshd/configuration.h b/apps/wolfsshd/configuration.h index 4ac1b5a59..514f00223 100644 --- a/apps/wolfsshd/configuration.h +++ b/apps/wolfsshd/configuration.h @@ -40,6 +40,9 @@ typedef struct WOLFSSHD_CONFIG WOLFSSHD_CONFIG; #define WOLFSSHD_WIN_STORE_CONFIG #endif +/* Longest config line. ConfigLoad() refuses a longer one. */ +#define WOLFSSHD_MAX_LINE_SIZE 1024 + #include "auth.h" /* 0 so that privilege separation is default on after struct memset'd on init */ diff --git a/apps/wolfsshd/test/test_configuration.c b/apps/wolfsshd/test/test_configuration.c index d8dc67f89..604b41e86 100644 --- a/apps/wolfsshd/test/test_configuration.c +++ b/apps/wolfsshd/test/test_configuration.c @@ -1354,6 +1354,130 @@ static int test_IncludeRecursionBound(void) return ret; } +/* Writes 'path' holding one ForceCommand line 'lineSz' bytes long before any + * newline, padded with leading blanks, which the loader strips. */ +static int WriteForceCommandLine(const char* path, const char* cmd, + int lineSz, int newline) +{ + static const char key[] = "ForceCommand "; + char contents[WOLFSSHD_MAX_LINE_SIZE + 8]; + int keySz = (int)WSTRLEN(key); + int cmdSz = (int)WSTRLEN(cmd); + int padSz = lineSz - keySz - cmdSz; + + if (padSz < 0 || lineSz + 2 > (int)sizeof(contents)) { + return WS_BAD_ARGUMENT; + } + + WMEMSET(contents, ' ', padSz); + WMEMCPY(contents + padSz, key, keySz); + WMEMCPY(contents + padSz + keySz, cmd, cmdSz); + if (newline) { + contents[lineSz++] = '\n'; + } + contents[lineSz] = '\0'; + + return WriteConfigFile(path, contents); +} + +/* Loads 'path' into a new config. Returns WS_SUCCESS when the load succeeds + * and the forced command is 'expect', or when 'expect' is NULL and the load + * fails. */ +static int LoadForcedCmd(const char* path, const char* expect) +{ + WOLFSSHD_CONFIG* conf; + char* cmd; + int ret; + + conf = wolfSSHD_ConfigNew(NULL); + if (conf == NULL) { + return WS_MEMORY_E; + } + + ret = wolfSSHD_ConfigLoad(conf, path); + if (expect == NULL) { + ret = (ret == WS_SUCCESS) ? WS_FATAL_ERROR : WS_SUCCESS; + } + else if (ret == WS_SUCCESS) { + cmd = wolfSSHD_ConfigGetForcedCmd(conf); + if (cmd == NULL || WSTRCMP(cmd, expect) != 0) { + ret = WS_FATAL_ERROR; + } + } + + wolfSSHD_ConfigFree(conf); + return ret; +} + +/* A config line longer than the loader's buffer must fail the load, not take + * effect truncated. A line that fits loads whole, up to the last byte. */ +static int test_ConfigLineTooLong(void) +{ + const char* path = "./line_size.conf"; + const int lastSz = WOLFSSHD_MAX_LINE_SIZE - 1; + char cmd[256]; + int ret = WS_SUCCESS; + + /* longer than the old 160 byte buffer */ + WMEMCPY(cmd, "/bin/echo ", 10); + WMEMSET(cmd + 10, 'X', 200); + cmd[210] = '\0'; + + Log(" Testing scenario: long config line kept whole."); + if (WriteForceCommandLine(path, cmd, 230, 1) == WS_SUCCESS && + LoadForcedCmd(path, cmd) == WS_SUCCESS) { + Log(" PASSED.\n"); + } + else { + Log(" FAILED.\n"); + ret = WS_FATAL_ERROR; + } + + if (ret == WS_SUCCESS) { + Log(" Testing scenario: line filling the buffer loads."); + if (WriteForceCommandLine(path, "/bin/echo edge", lastSz, 1) + == WS_SUCCESS && + LoadForcedCmd(path, "/bin/echo edge") == WS_SUCCESS) { + Log(" PASSED.\n"); + } + else { + Log(" FAILED.\n"); + ret = WS_FATAL_ERROR; + } + } + + if (ret == WS_SUCCESS) { + Log(" Testing scenario: last line filling the buffer loads."); + if (WriteForceCommandLine(path, "/bin/echo eof", lastSz, 0) + == WS_SUCCESS && + LoadForcedCmd(path, "/bin/echo eof") == WS_SUCCESS) { + Log(" PASSED.\n"); + } + else { + Log(" FAILED.\n"); + ret = WS_FATAL_ERROR; + } + } + + /* The cut leaves "X\n", which reads as a blank line, so only the length + * check stops "/bin/echo cut" from loading. */ + if (ret == WS_SUCCESS) { + Log(" Testing scenario: over-long config line refused."); + if (WriteForceCommandLine(path, "/bin/echo cutX", lastSz + 1, 1) + == WS_SUCCESS && + LoadForcedCmd(path, NULL) == WS_SUCCESS) { + Log(" PASSED.\n"); + } + else { + Log(" FAILED.\n"); + ret = WS_FATAL_ERROR; + } + } + + (void)WREMOVE(NULL, path); + return ret; +} + /* Each Match block is built from the global config, not from the Match block * before it. A user selected by a later block must not pick up settings that * an earlier, non-matching block changed. */ @@ -7311,6 +7435,7 @@ const TEST_CASE testCases[] = { TEST_DECL(test_ConfigParseAuthorizedUPNDomains), TEST_DECL(test_MatchUPNToUser), TEST_DECL(test_IncludeRecursionBound), + TEST_DECL(test_ConfigLineTooLong), TEST_DECL(test_GetUserConfMatchNoInherit), TEST_DECL(test_GetUserConfMatchOverlapCompose), TEST_DECL(test_ConfigIncludeMatchChain),