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/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" < (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), diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 61977d4df..80c055e7c 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -1734,6 +1734,47 @@ static const char* GetCertForcedCmd(WOLFSSHD_CONNECTION* conn) #endif } + +static const char internalSftp[] = "internal-sftp"; +#define INTERNAL_SFTP_SZ ((word32)(sizeof(internalSftp) - 1)) + +/* 1 when 'forcedCmd' is "internal-sftp". Only the first token is matched; + * sshd_config(5) permits options after it. */ +static int ForcedCmdIsInternalSftp(const char* forcedCmd) +{ + return forcedCmd != NULL && + WSTRNCMP(forcedCmd, internalSftp, INTERNAL_SFTP_SZ) == 0 && + (forcedCmd[INTERNAL_SFTP_SZ] == '\0' || + WISSPACE((unsigned char)forcedCmd[INTERNAL_SFTP_SZ])); +} + + +/* 1 when 'forcedCmd' is set to anything but a bare "internal-sftp". No + * internal-sftp option is applied, so one given denies SFTP rather than + * being ignored. NULL forbids nothing. */ +static int ForcedCmdDeniesSftp(const char* forcedCmd) +{ + const char* opt; + + if (forcedCmd == NULL) { + return 0; + } + if (!ForcedCmdIsInternalSftp(forcedCmd)) { + return 1; + } + + for (opt = forcedCmd + INTERNAL_SFTP_SZ; *opt != '\0'; opt++) { + if (!WISSPACE((unsigned char)*opt)) { + wolfSSH_Log(WS_LOG_ERROR, + "[SSHD] internal-sftp options are not supported: %s", + forcedCmd); + return 1; + } + } + + return 0; +} + #ifdef WOLFSSH_SCP static int SCP_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, WPASSWD* pPasswd, WOLFSSHD_CONFIG* usrConf) @@ -2147,7 +2188,7 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, /* @TODO check for conpty support LoadLibrary()and GetProcAddress(). */ - if (forcedCmd != NULL && WSTRCMP(forcedCmd, "internal-sftp") == 0) { + if (ForcedCmdIsInternalSftp(forcedCmd)) { wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Only SFTP connections allowed for user " "%s", wolfSSH_GetUsername(ssh)); @@ -2818,7 +2859,7 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, forcedCmd = subCmd; } - if (forcedCmd != NULL && WSTRCMP(forcedCmd, "internal-sftp") == 0) { + if (ForcedCmdIsInternalSftp(forcedCmd)) { wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Only SFTP connections allowed for user " "%s", wolfSSH_GetUsername(ssh)); @@ -3752,10 +3793,10 @@ static int UserAuthResult(byte result, } #if defined(WOLFSSH_SFTP) || defined(WOLFSSH_SCP) -/* Returns 1 if root's forced-commands-only ForceCommand isn't - * "internal-sftp": SFTP/SCP never honor ForceCommand (only SHELL_Subsystem - * does), so this blocks the confinement bypass. No-op on Windows - * (pPasswd is NULL there). */ +/* 1 when root is under forced-commands-only and its ForceCommand isn't a + * bare "internal-sftp". The dispatch denies SCP under any ForceCommand + * before this, so root never gets SCP there. No-op on Windows (pPasswd is + * NULL there). */ static int IsRootTransferBlocked(WPASSWD* pPasswd, WOLFSSHD_CONFIG* usrConf) { #ifdef _WIN32 @@ -3774,7 +3815,7 @@ static int IsRootTransferBlocked(WPASSWD* pPasswd, WOLFSSHD_CONFIG* usrConf) } forcedCmd = wolfSSHD_ConfigGetForcedCmd(usrConf); - return (forcedCmd == NULL || WSTRCMP(forcedCmd, "internal-sftp") != 0); + return forcedCmd == NULL || ForcedCmdDeniesSftp(forcedCmd); #endif } @@ -3784,8 +3825,8 @@ static int RejectRootTransfer(WPASSWD* pPasswd, WOLFSSHD_CONFIG* usrConf, const char* what) { if (IsRootTransferBlocked(pPasswd, usrConf)) { - wolfSSH_Log(WS_LOG_ERROR, "[SSHD] %s for root requires ForceCommand " - "internal-sftp under forced-commands-only", what); + wolfSSH_Log(WS_LOG_ERROR, "[SSHD] %s for root is not allowed " + "under forced-commands-only", what); return 1; } return 0; @@ -3926,7 +3967,8 @@ static void* HandleConnection(void* arg) WPASSWD* pPasswd = NULL; WOLFSSHD_CONFIG* usrConf; char* usr; - const char* forcedCmd = NULL; + const char* certCmd = NULL; + const char* confCmd = NULL; /* get configuration for user */ usr = wolfSSH_GetUsername(ssh); @@ -3968,14 +4010,15 @@ static void* HandleConnection(void* arg) break; case WOLFSSH_SESSION_SUBSYSTEM: - /* A certificate force-command overrides the requested - * subsystem. "internal-sftp" still permits SFTP; any other - * command denies file transfer (fail closed). */ - forcedCmd = GetCertForcedCmd(conn); - if (forcedCmd != NULL && - WSTRCMP(forcedCmd, "internal-sftp") != 0) { + /* A force-command, configured or from a certificate, + * overrides the requested subsystem. Only + * "internal-sftp" still permits SFTP. */ + certCmd = GetCertForcedCmd(conn); + confCmd = wolfSSHD_ConfigGetForcedCmd(usrConf); + if (ForcedCmdDeniesSftp(certCmd) || + ForcedCmdDeniesSftp(confCmd)) { wolfSSH_Log(WS_LOG_ERROR, - "[SSHD] Cert force-command set for user %s; " + "[SSHD] Force-command set for user %s; " "denying subsystem request", wolfSSH_GetUsername(ssh)); ret = WS_FATAL_ERROR; @@ -4002,11 +4045,11 @@ static void* HandleConnection(void* arg) case WS_SCP_INIT: #ifdef WOLFSSH_SCP - /* any certificate force-command, including - * internal-sftp, denies SCP */ - if (forcedCmd != NULL) { + /* any force-command, including internal-sftp, + * denies SCP */ + if (certCmd != NULL || confCmd != NULL) { wolfSSH_Log(WS_LOG_ERROR, - "[SSHD] Cert force-command set for user " + "[SSHD] Force-command set for user " "%s; denying SCP", wolfSSH_GetUsername(ssh)); ret = WS_FATAL_ERROR; @@ -4051,11 +4094,12 @@ static void* HandleConnection(void* arg) /* SCP can be an exec type */ if (ret == WS_SCP_INIT) { #ifdef WOLFSSH_SCP - /* any certificate force-command denies SCP */ - forcedCmd = GetCertForcedCmd(conn); - if (forcedCmd != NULL) { + /* any force-command denies SCP */ + certCmd = GetCertForcedCmd(conn); + confCmd = wolfSSHD_ConfigGetForcedCmd(usrConf); + if (certCmd != NULL || confCmd != NULL) { wolfSSH_Log(WS_LOG_ERROR, - "[SSHD] Cert force-command set for user %s; " + "[SSHD] Force-command set for user %s; " "denying SCP", wolfSSH_GetUsername(ssh)); ret = WS_FATAL_ERROR; }