From 52e4e3252903cb54ac1ddf39b5924b1b6294477e Mon Sep 17 00:00:00 2001 From: JacobBarthelmeh Date: Mon, 30 Sep 2024 15:49:33 -0600 Subject: [PATCH 1/3] keep trailing delimiter with SFTP open --- apps/wolfsshd/test/run_all_sshd_tests.sh | 1 + apps/wolfsshd/test/sshd_bad_sftp_test.sh | 34 ++++++++++++++++ src/internal.c | 51 ++++++++++++++++++++++++ src/ssh.c | 13 +++--- src/wolfsftp.c | 21 +++++----- wolfssh/internal.h | 6 +++ wolfssh/port.h | 8 ++++ 7 files changed, 118 insertions(+), 16 deletions(-) create mode 100755 apps/wolfsshd/test/sshd_bad_sftp_test.sh diff --git a/apps/wolfsshd/test/run_all_sshd_tests.sh b/apps/wolfsshd/test/run_all_sshd_tests.sh index b851963ef..e72421b85 100755 --- a/apps/wolfsshd/test/run_all_sshd_tests.sh +++ b/apps/wolfsshd/test/run_all_sshd_tests.sh @@ -60,6 +60,7 @@ run_test() { run_test "sshd_exec_test.sh" run_test "sshd_term_size_test.sh" run_test "sshd_large_sftp_test.sh" +run_test "sshd_bad_sftp_test.sh" #Github actions needs resolved for these test cases #run_test "error_return.sh" diff --git a/apps/wolfsshd/test/sshd_bad_sftp_test.sh b/apps/wolfsshd/test/sshd_bad_sftp_test.sh new file mode 100755 index 000000000..c5a74a392 --- /dev/null +++ b/apps/wolfsshd/test/sshd_bad_sftp_test.sh @@ -0,0 +1,34 @@ +#!/bin/sh + +# sshd local test + +PWD=`pwd` +cd ../../.. + +TEST_SFTP_CLIENT="./examples/sftpclient/wolfsftp" +USER=`whoami` +PRIVATE_KEY="./keys/hansel-key-ecc.der" +PUBLIC_KEY="./keys/hansel-key-ecc.pub" + +if [ -z "$1" ] || [ -z "$2" ]; then + echo "expecting host and port as arguments" + echo "./sshd_exec_test.sh 127.0.0.1 22222" + exit 1 +fi + +mkdir test-$$ +mkdir test-$$/subfolder + +echo "$TEST_SFTP_CLIENT -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY -g -l configure -r `pwd`/test-$$/subfolder/ -h \"$1\" -p \"$2\"" +"$TEST_SFTP_CLIENT -u $USER -i $PRIVATE_KEY -j $PUBLIC_KEY -g -l configure -r `pwd`/test-$$/subfolder/ -h $1 -p $2" + +RESULT=$? +if [ "$RESULT" = "0" ]; then + echo "Expecting to fail transfer to folder" + exit 1 +fi +rm -rf test-$$ + +cd $PWD +exit 0 + diff --git a/src/internal.c b/src/internal.c index b2be0cd94..b2d6ef69a 100644 --- a/src/internal.c +++ b/src/internal.c @@ -15686,6 +15686,57 @@ int SendChannelSuccess(WOLFSSH* ssh, word32 channelId, int success) #if (defined(WOLFSSH_SFTP) || defined(WOLFSSH_SCP)) && \ !defined(NO_WOLFSSH_SERVER) +/* Checks if 'in' is absolute path, if not the returns the concat. of + * 'defaultPath' | 'in'. This leaves 'in' as-is and does not handle + * simplification of the path, such as removing ../ + * + * Sanity checks outSz and then adjusts it for the size used + * returns WS_SUCCESS on success + */ +int wolfSSH_GetPath(const char* defaultPath, byte* in, word32 inSz, + char* out, word32* outSz) +{ + word32 curSz = 0; + + if (out != NULL) { + WMEMSET(out, 0, *outSz); + } + + if (inSz == 0 || (!WOLFSSH_SFTP_IS_DELIM(in[0]) && + !WOLFSSH_SFTP_IS_WINPATH(inSz, in))) { + if (defaultPath != NULL) { + curSz = (word32)WSTRLEN(defaultPath); + if (out != NULL && curSz >= *outSz) { + return WS_INVALID_PATH_E; + } + if (out != NULL) { + WSTRNCPY(out, defaultPath, *outSz); + if (out[curSz] != '/') { + out[curSz] = '/'; + curSz++; + } + } + } + } + + if (out != NULL) { + WMEMCPY(out + curSz, in, inSz); + } + curSz += inSz; + if (out != NULL) { + if (!WOLFSSH_SFTP_IS_DELIM(out[0])) { + WMEMMOVE(out+1, out, curSz); + out[0] = '/'; + curSz++; + } + out[curSz] = 0; + } + *outSz = curSz; + + return WS_SUCCESS; +} + + /* cleans up absolute path * returns size of new path on success (strlen sz) and negative values on fail*/ int wolfSSH_CleanPath(WOLFSSH* ssh, char* in) diff --git a/src/ssh.c b/src/ssh.c index e25037925..4fdff0c77 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -3276,10 +3276,6 @@ void* wolfSSH_GetChannelCloseCtx(WOLFSSH* ssh) #if (defined(WOLFSSH_SFTP) || defined(WOLFSSH_SCP)) && \ !defined(NO_WOLFSSH_SERVER) -#define DELIM "/\\" -#define IS_DELIM(x) ((x) == '/' || (x) == '\\') -#define IS_WINPATH(x,y) ((x) > 1 && (y)[1] == ':') - /* * Paths starting with a slash are absolute, rooted at "/". Any path that * doesn't have a starting slash is assumed to be relative to the default @@ -3288,6 +3284,8 @@ void* wolfSSH_GetChannelCloseCtx(WOLFSSH* ssh) * The path "/." is stripped out. The path "/.." strips out the previous * path value. The root path, "/", is always present. * + * Trailing delimiters are stripped, i.e /tmp/path/ becomes /tmp/path + * * Example: "/home/fred/frob/frizz/../../../barney/bar/baz/./././../.." * will return "/home/barney". "/../.." will return "/". "." will return * the default path. @@ -3319,7 +3317,8 @@ int wolfSSH_RealPath(const char* defaultPath, char* in, inSz = (word32)WSTRLEN(in); out[0] = '/'; curSz = 1; - if (inSz == 0 || (!IS_DELIM(in[0]) && !IS_WINPATH(inSz, in))) { + if (inSz == 0 || (!WOLFSSH_SFTP_IS_DELIM(in[0]) && + !WOLFSSH_SFTP_IS_WINPATH(inSz, in))) { if (defaultPath != NULL) { curSz = (word32)WSTRLEN(defaultPath); if (curSz >= outSz) { @@ -3330,9 +3329,9 @@ int wolfSSH_RealPath(const char* defaultPath, char* in, } out[curSz] = 0; - for (seg = WSTRTOK(in, DELIM, &tail); + for (seg = WSTRTOK(in, WOLFSSH_SFTP_DELIM, &tail); seg; - seg = WSTRTOK(NULL, DELIM, &tail)) { + seg = WSTRTOK(NULL, WOLFSSH_SFTP_DELIM, &tail)) { segSz = (word32)WSTRLEN(seg); /* Try to match "." */ diff --git a/src/wolfsftp.c b/src/wolfsftp.c index 2638e15ff..1b1793725 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -1974,7 +1974,7 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) { WS_SFTP_FILEATRB atr; WFD fd; - word32 sz; + word32 sz, dirSz; char dir[WOLFSSH_MAX_FILENAME]; word32 reason; word32 idx = 0; @@ -2010,10 +2010,11 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) return WS_BUFFER_E; } - ret = GetAndCleanPath(ssh->sftpDefaultPath, data + idx, sz, - dir, sizeof(dir)); - if (ret < 0) { - return ret; + dirSz = sizeof(dir); + if (wolfSSH_GetPath(ssh->sftpDefaultPath, data + idx, sz, dir, &dirSz) + != WS_SUCCESS) { + WLOG(WS_LOG_SFTP, "Creating path for file to open failed"); + return WS_FATAL_ERROR; } idx += sz; @@ -2128,7 +2129,7 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) { /* WS_SFTP_FILEATRB atr;*/ HANDLE fileHandle; - word32 sz; + word32 sz, dirSz; char dir[WOLFSSH_MAX_FILENAME]; word32 reason; word32 idx = 0; @@ -2165,9 +2166,11 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) return WS_BUFFER_E; } - if (GetAndCleanPath(ssh->sftpDefaultPath, - data + idx, sz, dir, sizeof(dir)) < 0) { - return WS_BUFFER_E; + dirSz = sizeof(dir); + if (wolfSSH_GetPath(ssh->sftpDefaultPath, data + idx, sz, dir, &dirSz) + != WS_SUCCESS) { + WLOG(WS_LOG_SFTP, "Creating path for file to open failed"); + return WS_FATAL_ERROR; } idx += sz; diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 7aa36d87b..731a9688d 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -620,6 +620,12 @@ typedef struct HandshakeInfo { } privKey; } HandshakeInfo; +#if (defined(WOLFSSH_SFTP) || defined(WOLFSSH_SCP)) && \ + !defined(NO_WOLFSSH_SERVER) +WOLFSSH_LOCAL int wolfSSH_GetPath(const char* defaultPath, byte* in, + word32 inSz, char* out, word32* outSz); +#endif + #ifdef WOLFSSH_SFTP #define WOLFSSH_MAX_SFTPOFST 3 diff --git a/wolfssh/port.h b/wolfssh/port.h index b65d32fd6..dbd0de9b9 100644 --- a/wolfssh/port.h +++ b/wolfssh/port.h @@ -575,6 +575,14 @@ extern "C" { #define WLOCALTIME(c,r) (localtime_r((c),(r))!=NULL) #endif +#ifndef WOLFSSH_SFTP_DELIM + /* Delimiter's used between two SFTP peers should be the same regardless of + * operating system. WS_DELIM defined elsewhere is OS specific delimiter. */ + #define WOLFSSH_SFTP_DELIM "/\\" + #define WOLFSSH_SFTP_IS_DELIM(x) ((x) == '/' || (x) == '\\') + #define WOLFSSH_SFTP_IS_WINPATH(x,y) ((x) > 1 && (y)[1] == ':') +#endif + #if (defined(WOLFSSH_SFTP) || \ defined(WOLFSSH_SCP) || defined(WOLFSSH_SSHD)) && \ !defined(NO_WOLFSSH_SERVER) && !defined(NO_FILESYSTEM) From 8a1b62ffeaf18d745107ae1f03eb9d4fd24f9caa Mon Sep 17 00:00:00 2001 From: JacobBarthelmeh Date: Thu, 10 Oct 2024 14:28:19 -0600 Subject: [PATCH 2/3] add sanity checks --- src/internal.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/internal.c b/src/internal.c index b2d6ef69a..2cff6ddfb 100644 --- a/src/internal.c +++ b/src/internal.c @@ -15720,16 +15720,22 @@ int wolfSSH_GetPath(const char* defaultPath, byte* in, word32 inSz, } if (out != NULL) { - WMEMCPY(out + curSz, in, inSz); + if (curSz + inSz < *outSz) { + WMEMCPY(out + curSz, in, inSz); + } } curSz += inSz; if (out != NULL) { if (!WOLFSSH_SFTP_IS_DELIM(out[0])) { - WMEMMOVE(out+1, out, curSz); - out[0] = '/'; + if (curSz + 1 < *outSz) { + WMEMMOVE(out+1, out, curSz); + out[0] = '/'; + } curSz++; } - out[curSz] = 0; + if (curSz < *outSz) { + out[curSz] = 0; + } } *outSz = curSz; From 6037d0bba8530b18abe14abe800f1bf84db08915 Mon Sep 17 00:00:00 2001 From: JacobBarthelmeh Date: Wed, 30 Oct 2024 09:43:13 -0600 Subject: [PATCH 3/3] error out when buffer is too small for null terminator --- src/internal.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/internal.c b/src/internal.c index 2cff6ddfb..d041a692f 100644 --- a/src/internal.c +++ b/src/internal.c @@ -15736,6 +15736,9 @@ int wolfSSH_GetPath(const char* defaultPath, byte* in, word32 inSz, if (curSz < *outSz) { out[curSz] = 0; } + else { + return WS_BUFFER_E; + } } *outSz = curSz;