From e4cd121ad1ec8b883951997e9ef751851573fea2 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 16 Oct 2026 14:29:40 -0700 Subject: [PATCH 1/4] wolfsshd: refuse the PID file without O_NOFOLLOW Where open() cannot refuse a final-component symlink, no pre-check can stop a link swapped in before the open, and the fstat gate accepts a root-owned target. wolfSSHD_ConfigSavePID now writes no PID file there instead of relying on a racy lstat. - test_ConfigSavePID gets a variant for that build that expects no file Issue: F-14764 --- apps/wolfsshd/configuration.c | 11 ++++--- apps/wolfsshd/test/test_configuration.c | 38 +++++++++++++++++++++++-- 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/apps/wolfsshd/configuration.c b/apps/wolfsshd/configuration.c index 074c5fda0..e2cc8285f 100644 --- a/apps/wolfsshd/configuration.c +++ b/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 diff --git a/apps/wolfsshd/test/test_configuration.c b/apps/wolfsshd/test/test_configuration.c index d8dc67f89..aa5a5d436 100644 --- a/apps/wolfsshd/test/test_configuration.c +++ b/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 From d8322c8a11a554dfb87c574e8ff113696e3a0baf Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 16 Oct 2026 14:33:21 -0700 Subject: [PATCH 2/4] wolfsshd: exec the login shell path whole SHELL_Subsystem copied pw_shell into an 80-byte buffer before execv(), so a longer path ran whatever its first 79 bytes named. The child now passes pw_shell to execv() directly, and builds the login argv[0] from its basename so a long path no longer falls back to "-sh". - add sshd_long_shell_test.sh, which logs in as a throwaway user whose shell path is longer than 80 bytes and checks the shell's $0 Issue: F-14757 --- apps/wolfsshd/test/run_all_sshd_tests.sh | 2 + apps/wolfsshd/test/sshd_long_shell_test.sh | 112 +++++++++++++++++++++ apps/wolfsshd/wolfsshd.c | 35 ++++--- 3 files changed, 133 insertions(+), 16 deletions(-) create mode 100755 apps/wolfsshd/test/sshd_long_shell_test.sh diff --git a/apps/wolfsshd/test/run_all_sshd_tests.sh b/apps/wolfsshd/test/run_all_sshd_tests.sh index 47ea02274..3fb16ce3e 100755 --- a/apps/wolfsshd/test/run_all_sshd_tests.sh +++ b/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" diff --git a/apps/wolfsshd/test/sshd_long_shell_test.sh b/apps/wolfsshd/test/sshd_long_shell_test.sh new file mode 100755 index 000000000..86b756442 --- /dev/null +++ b/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 < 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 diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 55ad3507b..fbf277861 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/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; From d3505e3ffecd741bafc5b3eb23529b1a590d9a15 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 16 Oct 2026 14:34:35 -0700 Subject: [PATCH 3/4] internal: check ssh in SendChannelTerminalResize It was the one channel sender that read ssh->defaultPeerChannelId without a NULL check. It now returns WS_BAD_ARGUMENT for a NULL ssh and logs entry and exit like its siblings. - add TestTerminalResizeNullSsh to regress.test Issue: F-14769 --- src/internal.c | 6 ++++++ tests/regress.c | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/src/internal.c b/src/internal.c index 5f0407c22..7e4e3f889 100644 --- a/src/internal.c +++ b/src/internal.c @@ -24224,6 +24224,11 @@ int SendChannelTerminalResize(WOLFSSH* ssh, word32 columns, word32 rows, const char* cType = "window-change"; word32 typeSz = 0; + WLOG(WS_LOG_DEBUG, "Entering SendChannelTerminalResize()"); + + if (ssh == NULL) + ret = WS_BAD_ARGUMENT; + if (ret == WS_SUCCESS) { channel = ChannelFind(ssh, ssh->defaultPeerChannelId, WS_CHANNEL_ID_PEER); @@ -24272,6 +24277,7 @@ int SendChannelTerminalResize(WOLFSSH* ssh, word32 columns, word32 rows, if (ret == WS_SUCCESS) ret = wolfSSH_SendPacket(ssh); + WLOG(WS_LOG_DEBUG, "Leaving SendChannelTerminalResize(), ret = %d", ret); return ret; } diff --git a/tests/regress.c b/tests/regress.c index 76bc7c4d9..47503e14b 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -15026,6 +15026,12 @@ static void TestTerminalResizeBlockedAfterDisconnect(void) wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); } + +static void TestTerminalResizeNullSsh(void) +{ + AssertIntEQ(SendChannelTerminalResize(NULL, 80, 24, 0, 0), + WS_BAD_ARGUMENT); +} #endif /* WOLFSSH_TERM && !NO_FILESYSTEM */ #endif /* !NO_WOLFSSH_CLIENT */ @@ -20651,6 +20657,7 @@ int main(int argc, char** argv) #if defined(WOLFSSH_TERM) && !defined(NO_FILESYSTEM) \ && !defined(NO_WOLFSSH_CLIENT) TestTerminalResizeBlockedAfterDisconnect(); + TestTerminalResizeNullSsh(); #endif #if !(defined(WOLFSSH_NO_RSA) && defined(WOLFSSH_NO_ECDSA_SHA2_NISTP256)) TestClientBuffersIdempotent(); From 71f305a05e8bafc3e137b7cc402fc9625f735343 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 16 Oct 2026 14:37:02 -0700 Subject: [PATCH 4/4] Reject an out-of-range -p port The -p option of wolfssh, client, wolfscp, wolfsftp and echoserver now goes through a strtol check, so a value that is not wholly a number in range exits with an error instead of wrapping into a different port. - apps/wolfssh: ClientParsePort(), shared with ClientParseDestination(), which also rejects an empty "ssh://host:" port instead of defaulting - examples: a ClientParsePort() in examples/client/common.c; port 0 is rejected in every build, not only with a main driver - echoserver: accepts 0 to 65535, keeping 0 for the test harness - add TestClientParsePort and an empty URI port case to regress.test Issue: F-14763, F-14767, F-14768, F-12552 --- apps/wolfssh/common.c | 44 ++++++++++++++++++++------------ apps/wolfssh/common.h | 1 + apps/wolfssh/wolfssh.c | 4 ++- examples/client/client.c | 8 +++--- examples/client/common.c | 22 ++++++++++++++++ examples/client/common.h | 1 + examples/echoserver/echoserver.c | 11 +++++++- examples/scpclient/scpclient.c | 7 ++--- examples/sftpclient/sftpclient.c | 7 ++--- tests/regress.c | 38 +++++++++++++++++++++++++++ 10 files changed, 109 insertions(+), 34 deletions(-) diff --git a/apps/wolfssh/common.c b/apps/wolfssh/common.c index 0092fd8c9..9858e39c5 100644 --- a/apps/wolfssh/common.c +++ b/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) { diff --git a/apps/wolfssh/common.h b/apps/wolfssh/common.h index 3868328d6..0b1a116cb 100644 --- a/apps/wolfssh/common.h +++ b/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 diff --git a/apps/wolfssh/wolfssh.c b/apps/wolfssh/wolfssh.c index b6b8c2495..b86406521 100644 --- a/apps/wolfssh/wolfssh.c +++ b/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': diff --git a/examples/client/client.c b/examples/client/client.c index 37463645f..9e218aadb 100644 --- a/examples/client/client.c +++ b/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': diff --git a/examples/client/common.c b/examples/client/common.c index 9e8e9d22e..d78f2fa56 100644 --- a/examples/client/common.c +++ b/examples/client/common.c @@ -365,6 +365,28 @@ void ClientIPOverride(int flag) #endif /* WOLFSSH_CERTS */ +/* 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) { + return WS_BAD_ARGUMENT; + } + *port = (word16)portVal; + + return WS_SUCCESS; +} + + int ClientPublicKeyCheck(const byte* pubKey, word32 pubKeySz, void* ctx) { int ret = 0; diff --git a/examples/client/common.h b/examples/client/common.h index 81ffd078c..100412d5f 100644 --- a/examples/client/common.h +++ b/examples/client/common.h @@ -36,6 +36,7 @@ int ClientUserAuth(byte authType, WS_UserAuthData* authData, void* ctx); int ClientPublicKeyCheck(const byte* pubKey, word32 pubKeySz, void* ctx); void ClientIPOverride(int flag); +int ClientParsePort(const char* in, word16* port); void ClientFreeBuffers(const char* pubKeyName, const char* privKeyName, void* heap); #ifdef WOLFSSH_TPM diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index ca45977b0..ec728e381 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -3915,7 +3915,16 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args) ES_ERROR("NULL port value\n"); } else { - port = (word16)atoi(myoptarg); + /* 0 is allowed here: the test harness asks for an + * ephemeral port */ + char* portEnd = NULL; + long portVal = strtol(myoptarg, &portEnd, 10); + + if (portEnd == myoptarg || *portEnd != '\0' + || portVal < 0 || portVal > 65535) { + ES_ERROR("port number must be 0 to 65535\n"); + } + port = (word16)portVal; #if !defined(NO_MAIN_DRIVER) if (port == 0) { ES_ERROR("port number cannot be 0\n"); diff --git a/examples/scpclient/scpclient.c b/examples/scpclient/scpclient.c index 33994a6e5..dae2afda8 100644 --- a/examples/scpclient/scpclient.c +++ b/examples/scpclient/scpclient.c @@ -156,11 +156,8 @@ THREAD_RETURN WOLFSSH_THREAD scp_client(void* args) if (myoptarg == NULL) err_sys("null argument found"); - 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 'i': diff --git a/examples/sftpclient/sftpclient.c b/examples/sftpclient/sftpclient.c index 097f70b27..bc4a41ae7 100644 --- a/examples/sftpclient/sftpclient.c +++ b/examples/sftpclient/sftpclient.c @@ -1642,11 +1642,8 @@ THREAD_RETURN WOLFSSH_THREAD sftpclient_test(void* args) case 'p': if (myoptarg == NULL) err_sys("null argument found"); - 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': diff --git a/tests/regress.c b/tests/regress.c index 47503e14b..82fa22ed9 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -19621,6 +19621,34 @@ static void TestStrictKexSurvivesRekeyKexInit(void) +/* The wolfssh -p option: a port that is not wholly a number in 1..65535 is + * rejected, not wrapped into a different port, and leaves *port alone. */ +static void TestClientParsePort(void) +{ + static const char* bad[] = { + "70000", "65536", "0", "-1", "abc", "", "22x", "99999999999999999999" + }; + word16 port; + word32 i; + + port = 22; + AssertIntEQ(ClientParsePort("2222", &port), WS_SUCCESS); + AssertIntEQ(port, 2222); + AssertIntEQ(ClientParsePort("65535", &port), WS_SUCCESS); + AssertIntEQ(port, 65535); + AssertIntEQ(ClientParsePort("1", &port), WS_SUCCESS); + AssertIntEQ(port, 1); + + for (i = 0; i < (word32)(sizeof(bad) / sizeof(bad[0])); i++) { + port = 22; + AssertIntEQ(ClientParsePort(bad[i], &port), WS_BAD_ARGUMENT); + AssertIntEQ(port, 22); + } + + AssertIntEQ(ClientParsePort(NULL, &port), WS_BAD_ARGUMENT); + AssertIntEQ(ClientParsePort("22", NULL), WS_BAD_ARGUMENT); +} + /* Regression coverage for issue 5575: the documented ssh://hostname form must * set the hostname even without an explicit port, and a malformed destination * with no host text must leave the hostname unset so the client can reject it. @@ -19768,6 +19796,15 @@ static void TestClientParseDestination(void) AssertTrue(user == NULL); AssertTrue(hostname == NULL); + /* An explicit but empty port is malformed, not a request for the + * default. */ + user = NULL; hostname = NULL; port = 22; + AssertIntEQ(ClientParseDestination("ssh://127.0.0.1:", + &user, &hostname, &port), WS_BAD_ARGUMENT); + AssertIntEQ(port, 22); + AssertTrue(user == NULL); + AssertTrue(hostname == NULL); + /* Non-numeric, trailing-garbage, and zero ports are rejected too. */ user = NULL; hostname = NULL; port = 22; AssertIntEQ(ClientParseDestination("ssh://127.0.0.1:abc", @@ -20261,6 +20298,7 @@ int main(int argc, char** argv) AssertNotNull(serverSsh); #endif + TestClientParsePort(); TestClientParseDestination(); TestRekeyPendingAccessor(); #ifdef WOLFSSH_TEST_INTERNAL