From 13f11a3005e3a4836a09edd31ea6bffc53e093c4 Mon Sep 17 00:00:00 2001 From: Yosuke Shimizu Date: Wed, 10 Jun 2026 10:14:36 +0900 Subject: [PATCH] Reject symlinks in default SCP send callback --- examples/client/client.c | 5 ++ scripts/scp.test | 56 +++++++++++++++ src/internal.c | 4 ++ src/port.c | 76 ++++++++++++++++++++ src/wolfscp.c | 129 ++++++++++++++++++++++++++++++++-- tests/api.c | 145 ++++++++++++++++++++++++++++++++++++++- wolfssh/internal.h | 3 + wolfssh/port.h | 39 ++++++++++- 8 files changed, 446 insertions(+), 11 deletions(-) diff --git a/examples/client/client.c b/examples/client/client.c index 6429d28b9..5ab01b19a 100644 --- a/examples/client/client.c +++ b/examples/client/client.c @@ -129,6 +129,11 @@ static void ShowUsage(void) printf(" -k set the list of key algos\n"); printf(" -C set the list of encrypt algos\n"); printf(" -q turn off debugging output\n"); +#ifndef WOLFSSH_HAVE_SYMLINK + /* report disabled symlink checking so test scripts can skip the + * symlink-rejection cases the server would otherwise follow */ + printf(" symlink checking off (e.g. WOLFSSH_NO_SYMLINK_CHECK)\n"); +#endif } diff --git a/scripts/scp.test b/scripts/scp.test index 5af3c173c..eaf34b2c0 100755 --- a/scripts/scp.test +++ b/scripts/scp.test @@ -54,6 +54,9 @@ do_cleanup() { kill -9 $server_pid fi remove_ready_file + # remove symlink-test artifacts so an early exit (e.g. create_port failure) + # does not leave a planted secret/symlink behind in the source tree + [ -n "$scp_secret" ] && rm -f "$scp_secret" "$scp_symlink" "$scp_symlink_out" } do_trap() { @@ -150,6 +153,59 @@ if test $RESULT -eq 0; then exit 1 fi +# The symlink-rejection guard is compiled out by WOLFSSH_NO_SYMLINK_CHECK, which +# the example client reports in its usage output (-?). Skip this test when it is +# set: the server then follows symlinks by design and the check below would +# false-fail. +./examples/client/client '-?' | grep WOLFSSH_NO_SYMLINK_CHECK +if [ $? -eq 0 ]; then + echo "symlink checking disabled, skipping symlink test" +else + echo "Test that the server refuses to follow a symlink (server to local)" + # This exercises the single-file send path (WOLFSSH_SCP_SINGLE_FILE_REQUEST), + # whose file open now goes through wFopenNoFollow (atomic O_NOFOLLOW on + # POSIX), so this case drives the no-follow open end to end. + # The recursive sinks (the recursive-root leaf check and the per-entry check + # in ScpProcessEntry) use the same shared wIsSymlink helper but cannot be + # driven from here: the wolfSSH example client (wolfSSH_SCP_from) only ever + # issues "scp -f ", never "scp -f -r", so the server's recursive + # request state is never reached by this harness. wIsSymlink itself is + # additionally exercised through the SFTP confinement unit test + # (test_wolfSSH_SFTP_Confinement), so the detection logic shared by all sinks + # is covered even though the SCP recursive wiring is not driven e2e. + scp_secret=$PWD/scripts/scp_secret_$$ + scp_symlink=$PWD/scp_symlink_$$ + scp_symlink_out=$PWD/scp_symlink_out_$$ + echo "TOP SECRET SYMLINK TARGET" > $scp_secret + # A symlink whose target exists would, without the fix, be opened and its + # contents streamed to the client. The fix must reject it instead. + ln -s $scp_secret $scp_symlink + if test -L $scp_symlink; then + ./examples/echoserver/echoserver -1 -R $ready_file & + server_pid=$! + create_port + ./examples/scpclient/wolfscp -u jill -P upthehill -p $port -S $scp_symlink:$scp_symlink_out + remove_ready_file + + # The server must not deliver the symlink target: no local output file + # may be produced from the refused request. The client exit code is not + # asserted (as with the other -S cases it does not propagate a + # server-side SCP abort); connectivity in this environment is already + # proven by the "basic copy from server to local" case above, which uses + # the same mechanism and aborts the whole script on failure, so this + # cannot pass vacuously. + if test -e $scp_symlink_out; then + rm -f $scp_symlink_out $scp_secret $scp_symlink + echo -e "\n\nserver followed a symlink, confinement bypass" + do_cleanup + exit 1 + fi + else + echo "symlink not supported on this filesystem, skipping symlink test" + fi + rm -f $scp_secret $scp_symlink $scp_symlink_out +fi + echo -e "\nALL Tests Passed" exit 0 diff --git a/src/internal.c b/src/internal.c index deb11d14a..dfb92d728 100644 --- a/src/internal.c +++ b/src/internal.c @@ -1415,6 +1415,10 @@ void SshResourceFree(WOLFSSH* ssh, void* heap) ssh->scpBasePathDynamic = NULL; ssh->scpBasePathSz = 0; } + #if !defined(WOLFSSH_SCP_USER_CALLBACKS) && !defined(NO_FILESYSTEM) + /* free any send-side directory stack left by an aborted recursive transfer */ + ScpSendCtxFreeDirs(ssh->fs, &ssh->scpSendCbCtx, heap); + #endif #endif #ifdef WOLFSSH_SFTP if (ssh->sftpDefaultPath) { diff --git a/src/port.c b/src/port.c index 036974e10..78814fd56 100644 --- a/src/port.c +++ b/src/port.c @@ -719,6 +719,82 @@ int wIsSymlink(const char* path) #endif return isLink; } + +/* Open path for reading without following a final-component symbolic link. + * On POSIX this is atomic through O_NOFOLLOW: the open itself fails if the leaf + * is a link, closing the check-then-open race. Where no such primitive exists + * (Windows, or O_NOFOLLOW undefined) it falls back to a best-effort wIsSymlink + * test before the follow-prone open. Returns 0 on success and non-zero on + * failure, matching the wfopen convention. */ +int wFopenNoFollow(void* fs, WFILE** f, const char* path) +{ + int ret = 0; +#if !defined(USE_WINDOWS_API) && defined(O_NOFOLLOW) + WFD fd = -1; +#endif + + if (f != NULL) + *f = NULL; + if (f == NULL || path == NULL) + ret = 1; + +#if !defined(USE_WINDOWS_API) && defined(O_NOFOLLOW) + if (ret == 0) { + fd = WOPEN(fs, path, WOLFSSH_O_RDONLY | WOLFSSH_O_NOFOLLOW, 0); + if (fd < 0) + ret = 1; + } + if (ret == 0 && WFDOPEN(fs, f, fd, "rb") != 0) { + WCLOSE(fs, fd); + ret = 1; + } +#else + if (ret == 0 && wIsSymlink(path)) + ret = 1; + if (ret == 0 && WFOPEN(fs, f, path, "rb") != 0) + ret = 1; +#endif + WOLFSSH_UNUSED(fs); + return ret; +} + +#if !defined(USE_WINDOWS_API) && !defined(NO_WOLFSSH_DIR) +/* Open a directory without following a final-component symlink: POSIX uses + * O_DIRECTORY|O_NOFOLLOW + fdopendir (atomic), else falls back to wIsSymlink + + * opendir. Returns 0 on success, non-zero on failure (matches WOPENDIR). */ +int wOpendirNoFollow(void* fs, WDIR* dir, const char* path) +{ + int ret = 0; +#if defined(O_NOFOLLOW) && defined(O_DIRECTORY) + WFD fd = -1; +#endif + + if (dir != NULL) + *dir = NULL; + if (dir == NULL || path == NULL) + ret = 1; + +#if defined(O_NOFOLLOW) && defined(O_DIRECTORY) + if (ret == 0) { + fd = WOPEN(fs, path, WOLFSSH_O_RDONLY | WOLFSSH_O_DIRECTORY | + WOLFSSH_O_NOFOLLOW, 0); + if (fd < 0) + ret = 1; + } + if (ret == 0 && WFDOPENDIR(fs, dir, fd) != 0) { + WCLOSE(fs, fd); + ret = 1; + } +#else + if (ret == 0 && wIsSymlink(path)) + ret = 1; + if (ret == 0 && WOPENDIR(fs, NULL, dir, path) != 0) + ret = 1; +#endif + WOLFSSH_UNUSED(fs); + return ret; +} +#endif /* !USE_WINDOWS_API && !NO_WOLFSSH_DIR */ #endif /* WOLFSSH_HAVE_SYMLINK && (WOLFSSH_SFTP || WOLFSSH_SCP) */ #ifndef WSTRING_USER diff --git a/src/wolfscp.c b/src/wolfscp.c index 98b023963..12ecf8ae7 100644 --- a/src/wolfscp.c +++ b/src/wolfscp.c @@ -646,6 +646,16 @@ int DoScpSource(WOLFSSH* ssh) continue; } else if (ssh->scpConfirm == WS_SCP_ABORT) { + #if !defined(NO_FILESYSTEM) && \ + !defined(WOLFSSH_SCP_USER_CALLBACKS) + /* drain any partial recursive dir stack so a later exec on + * this connection starts from a fresh root, not a stale + * handle left by the aborted walk */ + ScpSendCtx* sendCtx = + (ScpSendCtx*)wolfSSH_GetScpSendCtx(ssh); + if (sendCtx != NULL) + ScpSendCtxFreeDirs(ssh->fs, sendCtx, ssh->ctx->heap); + #endif ssh->scpState = SCP_SEND_CONFIRMATION; ssh->scpNextState = SCP_DONE; continue; @@ -2454,8 +2464,15 @@ static int GetFileStats(void *fs, ScpSendCtx* ctx, const char* fileName, *fileMode = 0555 | (ctx->s.dwFileAttributes & FILE_ATTRIBUTE_READONLY ? 0 : 0200); *fileMode |= (ctx->s.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) ? 040000 : 0; +#else + /* WLSTAT (lstat on POSIX) leaves a symlink unfollowed, so it classifies as + * neither dir nor file and is skipped; WOLFSSH_NO_SYMLINK_CHECK falls back + * to WSTAT so links are followed by design. */ +#ifdef WOLFSSH_HAVE_SYMLINK + if (WLSTAT(fs, fileName, &ctx->s) < 0) { #else if (WSTAT(fs, fileName, &ctx->s) < 0) { +#endif ret = WS_BAD_FILE_E; #ifdef WOLFSSL_NUCLEUS if (WSTRLEN(fileName) < 4 && WSTRLEN(fileName) > 2 && @@ -2543,11 +2560,16 @@ static ScpDir* ScpNewDir(void *fs, const char* path, void* heap) } } #else + #ifdef WOLFSSH_HAVE_SYMLINK + /* refuse a symlinked directory leaf atomically (closes the descend race) */ + if (wOpendirNoFollow(fs, &entry->dir, path) != 0) { + #else if (WOPENDIR(fs, heap, &entry->dir, path) != 0 #if !defined(WOLFSSL_NUCLEUS) && !defined(WOLFSSH_ZEPHYR) || entry->dir == NULL #endif ) { + #endif WFREE(entry, heap, DYNTYPE_SCPDIR); WLOG(WS_LOG_ERROR, scpError, "opendir failed on directory", WS_INVALID_PATH_E); @@ -2626,6 +2648,17 @@ int ScpPopDir(void *fs, ScpSendCtx* ctx, void* heap) return WS_SUCCESS; } +/* Drain dir-stack entries (and open dir handles) left on a send context after + * a recursive transfer aborts mid-tree before popping. Safe on an empty + * stack. */ +void ScpSendCtxFreeDirs(void* fs, ScpSendCtx* ctx, void* heap) +{ + if (ctx != NULL) { + while (ctx->currentDir != NULL) + (void)ScpPopDir(fs, ctx, heap); + } +} + /* Get next entry in directory, either file or directory, skips self (.) * and parent (..) directories, stores in ctx->entry. * Return WS_SUCCESS on success or negative upon error */ @@ -2820,6 +2853,16 @@ static int ScpProcessEntry(WOLFSSH* ssh, char* fileName, word64* mTime, WSTRNCPY(fileName, sendCtx->entry->d_name, DEFAULT_SCP_FILE_NAME_SZ); #endif + #ifdef WOLFSSH_HAVE_SYMLINK + /* filePath is fully built; reject a planted symlink before + * GetFileStats or any descend/open follows it. */ + if (ret == WS_SUCCESS && wIsSymlink(filePath)) { + WLOG(WS_LOG_ERROR, + "scp: symlink entry rejected, aborting transfer"); + ret = WS_SCP_ABORT; + } + #endif /* WOLFSSH_HAVE_SYMLINK */ + if (ret == WS_SUCCESS) { ret = GetFileStats(ssh->fs, sendCtx, filePath, mTime, aTime, fileMode); } @@ -2839,7 +2882,11 @@ static int ScpProcessEntry(WOLFSSH* ssh, char* fileName, word64* mTime, } } else if (ScpFileIsFile(sendCtx)) { + #ifdef WOLFSSH_HAVE_SYMLINK + if (wFopenNoFollow(ssh->fs, &(sendCtx->fp), filePath) != 0) { + #else if (WFOPEN(ssh->fs, &(sendCtx->fp), filePath, "rb") != 0) { + #endif WLOG(WS_LOG_ERROR, "scp: Error with opening file, abort"); wolfSSH_SetScpErrorMsg(ssh, "unable to open file " "for reading"); @@ -2862,7 +2909,10 @@ static int ScpProcessEntry(WOLFSSH* ssh, char* fileName, word64* mTime, } } else { - if (ret != WS_NEXT_ERROR) { + /* WS_SCP_ABORT entries (e.g. a rejected symlink) were already logged at + * their source, so only the generic, unexpected-error case is noted + * here to avoid a misleading second log line. */ + if (ret != WS_NEXT_ERROR && ret != WS_SCP_ABORT) { WLOG(WS_LOG_ERROR, "scp: ret does not equal WS_NEXT_ERROR, abort"); ret = WS_SCP_ABORT; } @@ -2948,6 +2998,22 @@ static int ScpProcessEntry(WOLFSSH* ssh, char* fileName, word64* mTime, * is complete. * WS_SCP_ABORT - abort file transfer request * WS_BAD_FILE_E - local file open error hit + * + * Symlink handling: file-content opens go through wFopenNoFollow and directory + * opens (both the recursive root and every descend) go through wOpendirNoFollow. + * Both are atomic against a swapped link on POSIX (O_NOFOLLOW, plus O_DIRECTORY + * for the directory open) and fall back to a wIsSymlink check-then-open on + * Windows or where O_NOFOLLOW is absent. The root also gets an explicit + * wIsSymlink pre-check because a trailing separator (open("link/", O_NOFOLLOW)) + * can still follow the link; symlinks below the root are rejected as + * ScpProcessEntry traverses them. No "stays under a trusted base" containment + * is attempted: SCP has no library-level base path (wolfsshd relies on OS + * chroot) and wolfSSH_RealPath does not resolve links. GetFileStats uses WLSTAT + * so it does not follow a link for metadata or classification. On the + * Windows/fallback path the open is check-then-open, so a concurrent in-jail + * writer racing it remains a best-effort gap. For hostile multi-tenant use, + * confine the session with an OS mechanism (chroot, dropped privileges) and + * treat these checks as defense in depth. */ int wsScpSendCallback(WOLFSSH* ssh, int state, const char* peerRequest, char* fileName, word32 fileNameSz, word64* mTime, word64* aTime, @@ -2981,9 +3047,14 @@ int wsScpSendCallback(WOLFSSH* ssh, int state, const char* peerRequest, break; case WOLFSSH_SCP_SINGLE_FILE_REQUEST: - if ((sendCtx == NULL) || WFOPEN(ssh->fs, &(sendCtx->fp), peerRequest, - "rb") != 0) { - + /* open without following a symlink so its target is not streamed + * to the peer; see this function's symlink-handling note. */ + if ((sendCtx == NULL) || + #ifdef WOLFSSH_HAVE_SYMLINK + wFopenNoFollow(ssh->fs, &(sendCtx->fp), peerRequest) != 0) { + #else + WFOPEN(ssh->fs, &(sendCtx->fp), peerRequest, "rb") != 0) { + #endif WLOG(WS_LOG_ERROR, "scp: unable to open file, abort"); wolfSSH_SetScpErrorMsg(ssh, "unable to open file for reading"); ret = WS_BAD_FILE_E; @@ -3037,9 +3108,51 @@ int wsScpSendCallback(WOLFSSH* ssh, int state, const char* peerRequest, case WOLFSSH_SCP_RECURSIVE_REQUEST: if (ScpDirStackIsEmpty(sendCtx)) { + #ifdef WOLFSSH_HAVE_SYMLINK + word32 rootLen; + #endif + + /* first request, keep track of request directory. Reject a + * symlink root here (a trailing separator can still follow); + * see the symlink-handling note in this function's header. */ + ret = WS_SUCCESS; + if (peerRequest == NULL) { + WLOG(WS_LOG_ERROR, + "scp: missing recursive root path, abort"); + ret = WS_SCP_ABORT; + } + #ifdef WOLFSSH_HAVE_SYMLINK + /* lstat() follows the link when the path ends in a separator, + * so check the root with any trailing separators removed */ + else { + rootLen = (word32)WSTRLEN(peerRequest); + while (rootLen > 1 && (peerRequest[rootLen - 1] == '/' || + peerRequest[rootLen - 1] == '\\')) + rootLen--; + if (rootLen >= DEFAULT_SCP_FILE_NAME_SZ) { + WLOG(WS_LOG_ERROR, + "scp: recursive root path too long, abort"); + wolfSSH_SetScpErrorMsg(ssh, + "unable to open file for reading"); + ret = WS_SCP_ABORT; + } + else { + WMEMCPY(filePath, peerRequest, rootLen); + filePath[rootLen] = '\0'; + if (wIsSymlink(filePath)) { + WLOG(WS_LOG_ERROR, + "scp: refusing recursive root symlink, abort"); + wolfSSH_SetScpErrorMsg(ssh, + "unable to open file for reading"); + ret = WS_SCP_ABORT; + } + } + } + #endif /* WOLFSSH_HAVE_SYMLINK */ - /* first request, keep track of request directory */ - ret = ScpPushDir(ssh->fs, sendCtx, peerRequest, ssh->ctx->heap); + if (ret == WS_SUCCESS) + ret = ScpPushDir(ssh->fs, sendCtx, peerRequest, + ssh->ctx->heap); if (ret == WS_SUCCESS) { /* get file name from request */ @@ -3053,7 +3166,9 @@ int wsScpSendCallback(WOLFSSH* ssh, int state, const char* peerRequest, if (ret == WS_SUCCESS) { ret = WS_SCP_ENTER_DIR; - } else { + } else if (ret != WS_SCP_ABORT) { + /* a rejected symlink root already logged its own reason; + * only note the generic stat failure here */ WLOG(WS_LOG_ERROR, "scp: error getting file stats, abort"); ret = WS_SCP_ABORT; } diff --git a/tests/api.c b/tests/api.c index afa16f757..d511419ca 100644 --- a/tests/api.c +++ b/tests/api.c @@ -32,11 +32,13 @@ #include #include -#if defined(WOLFSSH_SFTP) && !defined(NO_WOLFSSH_CLIENT) && \ +#if ((defined(WOLFSSH_SFTP) && !defined(NO_WOLFSSH_CLIENT)) || \ + (defined(WOLFSSH_SCP) && defined(WOLFSSH_HAVE_SYMLINK) && \ + !defined(WOLFSSH_SCP_USER_CALLBACKS))) && \ !defined(SINGLE_THREADED) && !defined(WOLFSSH_ZEPHYR) && \ !defined(USE_WINDOWS_API) - /* mkdtemp() for staging unique, per-test out-of-jail SFTP fixtures and - * symlink() for the symlink-escape confinement case */ + /* mkdtemp() and symlink() for the SFTP confinement and SCP symlink-reject + * tests (staging out-of-jail fixtures and planted links) */ #include #include #endif @@ -1045,6 +1047,142 @@ static void test_wolfSSH_SCP_CB(void) static void test_wolfSSH_SCP_CB(void) { ; } #endif /* WOLFSSH_SCP */ +#if defined(WOLFSSH_SCP) && defined(WOLFSSH_HAVE_SYMLINK) && \ + !defined(WOLFSSH_SCP_USER_CALLBACKS) && !defined(SINGLE_THREADED) && \ + !defined(WOLFSSH_ZEPHYR) && !defined(USE_WINDOWS_API) && \ + !defined(NO_WOLFSSH_DIR) +/* The default SCP send callback must refuse a symlink so its target is not + * streamed to the peer. The example client cannot issue "scp -f -r", so the + * recursive guards are unreachable through scripts/scp.test; drive both the + * recursive-root and per-entry guards here at the callback level, and exercise + * wFopenNoFollow (shared by the file opens) directly. */ +static void test_wolfSSH_SCP_SendSymlinkReject(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + ScpSendCtx sendCtx; + WFILE* fp = NULL; + WDIR wdir; + char scpRoot[] = "/tmp/wolfssh_scp_sym_XXXXXX"; + char realFile[WOLFSSH_MAX_FILENAME]; + char symToFile[WOLFSSH_MAX_FILENAME]; + char symToDir[WOLFSSH_MAX_FILENAME]; + char symToDirSlash[WOLFSSH_MAX_FILENAME]; + char subDir[WOLFSSH_MAX_FILENAME]; + char subLink[WOLFSSH_MAX_FILENAME]; + char fileName[DEFAULT_SCP_FILE_NAME_SZ]; + byte buf[256]; + word64 mTime = 0; + word64 aTime = 0; + int fileMode = 0; + word32 totalSz = 0; + + AssertNotNull(mkdtemp(scpRoot)); + + WSNPRINTF(realFile, sizeof(realFile), "%s/secret", scpRoot); + WSNPRINTF(symToFile, sizeof(symToFile), "%s/link_file", scpRoot); + WSNPRINTF(symToDir, sizeof(symToDir), "%s/link_dir", scpRoot); + + /* stage an empty real file plus a symlink to it and to the temp dir */ + AssertIntEQ(WFOPEN(NULL, &fp, realFile, "wb"), 0); + WFCLOSE(NULL, fp); + fp = NULL; + AssertIntEQ(symlink(realFile, symToFile), 0); + AssertIntEQ(symlink(scpRoot, symToDir), 0); + + /* the guard's discriminator: a real path is not a link, the planted ones + * are - so a genuine directory would not trip the recursive-root check */ + AssertIntEQ(wIsSymlink(scpRoot), 0); + AssertIntEQ(wIsSymlink(symToDir), 1); + + /* wFopenNoFollow opens a real file but refuses a symlink */ + AssertIntEQ(wFopenNoFollow(NULL, &fp, realFile), 0); + AssertNotNull(fp); + WFCLOSE(NULL, fp); + fp = NULL; + AssertIntNE(wFopenNoFollow(NULL, &fp, symToFile), 0); + AssertNull(fp); + + /* wOpendirNoFollow opens a real directory but refuses a symlinked one */ + AssertIntEQ(wOpendirNoFollow(NULL, &wdir, scpRoot), 0); + WCLOSEDIR(NULL, &wdir); + AssertIntNE(wOpendirNoFollow(NULL, &wdir, symToDir), 0); + + AssertNotNull(ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL)); + AssertNotNull(ssh = wolfSSH_new(ctx)); + WMEMSET(&sendCtx, 0, sizeof(sendCtx)); + WMEMSET(fileName, 0, sizeof(fileName)); + + /* a benign state returns success (the callback does not blanket-abort) */ + AssertIntEQ(wsScpSendCallback(ssh, WOLFSSH_SCP_NEW_REQUEST, symToDir, + fileName, (word32)sizeof(fileName), &mTime, &aTime, &fileMode, 0, + &totalSz, buf, (word32)sizeof(buf), &sendCtx), WS_SUCCESS); + + /* a single-file request for a symlink is refused (WS_BAD_FILE_E) */ + AssertIntEQ(wsScpSendCallback(ssh, WOLFSSH_SCP_SINGLE_FILE_REQUEST, + symToFile, fileName, (word32)sizeof(fileName), &mTime, &aTime, + &fileMode, 0, &totalSz, buf, (word32)sizeof(buf), &sendCtx), + WS_BAD_FILE_E); + AssertNull(sendCtx.fp); + + /* a recursive root that is a symlink aborts before any directory is + * opened, so nothing is pushed onto the stack and nothing leaks */ + AssertIntEQ(wsScpSendCallback(ssh, WOLFSSH_SCP_RECURSIVE_REQUEST, symToDir, + fileName, (word32)sizeof(fileName), &mTime, &aTime, &fileMode, 0, + &totalSz, buf, (word32)sizeof(buf), &sendCtx), WS_SCP_ABORT); + AssertNull(sendCtx.currentDir); + + /* a trailing separator must not bypass the root guard: lstat on "link/" + * would follow the link, so the guard strips it before checking */ + WSNPRINTF(symToDirSlash, sizeof(symToDirSlash), "%s/link_dir/", scpRoot); + AssertIntEQ(wsScpSendCallback(ssh, WOLFSSH_SCP_RECURSIVE_REQUEST, + symToDirSlash, fileName, (word32)sizeof(fileName), &mTime, &aTime, + &fileMode, 0, &totalSz, buf, (word32)sizeof(buf), &sendCtx), + WS_SCP_ABORT); + AssertNull(sendCtx.currentDir); + + /* a recursive root with a missing path must abort, not dereference NULL */ + AssertIntEQ(wsScpSendCallback(ssh, WOLFSSH_SCP_RECURSIVE_REQUEST, NULL, + fileName, (word32)sizeof(fileName), &mTime, &aTime, &fileMode, 0, + &totalSz, buf, (word32)sizeof(buf), &sendCtx), WS_SCP_ABORT); + AssertNull(sendCtx.currentDir); + + /* per-entry guard: drive two iterations so ScpProcessEntry processes a + * planted entry. The link points at a directory on purpose - a symlinked + * dir entry is caught only by the per-entry wIsSymlink check (not the + * file-open no-follow guard), so this isolates that branch. */ + WSNPRINTF(subDir, sizeof(subDir), "%s/sub", scpRoot); + WSNPRINTF(subLink, sizeof(subLink), "%s/sub/evil", scpRoot); + AssertIntEQ(WMKDIR(NULL, subDir, 0700), 0); + AssertIntEQ(symlink(scpRoot, subLink), 0); + + WMEMSET(&sendCtx, 0, sizeof(sendCtx)); + AssertIntEQ(wsScpSendCallback(ssh, WOLFSSH_SCP_RECURSIVE_REQUEST, subDir, + fileName, (word32)sizeof(fileName), &mTime, &aTime, &fileMode, 0, + &totalSz, buf, (word32)sizeof(buf), &sendCtx), WS_SCP_ENTER_DIR); + AssertIntEQ(wsScpSendCallback(ssh, WOLFSSH_SCP_RECURSIVE_REQUEST, subDir, + fileName, (word32)sizeof(fileName), &mTime, &aTime, &fileMode, 0, + &totalSz, buf, (word32)sizeof(buf), &sendCtx), WS_SCP_ABORT); + /* the per-entry abort left the pushed dir on the stack (this is what would + * leak); the production teardown drain must release it */ + AssertNotNull(sendCtx.currentDir); + ScpSendCtxFreeDirs(ssh->fs, &sendCtx, ssh->ctx->heap); + AssertNull(sendCtx.currentDir); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + + WREMOVE(NULL, subLink); + WRMDIR(NULL, subDir); + WREMOVE(NULL, symToFile); + WREMOVE(NULL, symToDir); + WREMOVE(NULL, realFile); + WRMDIR(NULL, scpRoot); +} +#else +static void test_wolfSSH_SCP_SendSymlinkReject(void) { ; } +#endif + #ifdef WOLFSSH_AGENT typedef struct AgentTestCtx { int partialWrite; @@ -3610,6 +3748,7 @@ int wolfSSH_ApiTest(int argc, char** argv) /* SCP tests */ test_wolfSSH_SCP_CB(); + test_wolfSSH_SCP_SendSymlinkReject(); test_wolfSSH_SCP_ReKey(); test_wolfSSH_SCP_ReKey_NonBlock(); test_wolfSSH_SCP_ReKey_ToServer(); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index f9cb3daea..add174120 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1578,6 +1578,9 @@ WOLFSSH_LOCAL int wsScpSendCallback(WOLFSSH* ssh, int state, word64* aTime, int* fileMode, word32 fileOffset, word32* totalFileSz, byte* buf, word32 bufSz, void* ctx); +#if !defined(WOLFSSH_SCP_USER_CALLBACKS) && !defined(NO_FILESYSTEM) +WOLFSSH_LOCAL void ScpSendCtxFreeDirs(void* fs, ScpSendCtx* ctx, void* heap); +#endif #endif diff --git a/wolfssh/port.h b/wolfssh/port.h index e357a256e..9661bf3e9 100644 --- a/wolfssh/port.h +++ b/wolfssh/port.h @@ -1325,6 +1325,7 @@ extern "C" { #define WGETCWD(fs,r,rSz) _getcwd((r),(rSz)) #define WOPEN(fs,f,m,p) _open((f),(m),(p)) #define WCLOSE(fs,fd) _close((fd)) + #define WFDOPEN(fs,f,fd,m) ((*(f) = _fdopen((fd),(m))) == NULL) #define WFD int int wPwrite(WFD fd, unsigned char* buf, unsigned int sz, @@ -1363,6 +1364,7 @@ extern "C" { #define WMKDIR(fs,p,m) fs_mkdir((p)) #define WRMDIR(fs,d) fs_unlink((d)) #define WSTAT(fs,p,b) wssh_z_fstat((p),(b)) + #define WLSTAT(fs,p,b) wssh_z_fstat((p),(b)) #define WREMOVE(fs,d) fs_unlink((d)) #define WRENAME(fs,o,n) fs_rename((o),(n)) #define WS_DELIM '/' @@ -1403,6 +1405,7 @@ extern "C" { #define WMKDIR(fs,p,m) SYS_FS_DirectoryMake((p)) #define WRMDIR(fs,d) SYS_FS_FileDirectoryRemove((d)) #define WSTAT(fs,p,b) wStat((p), (b)) + #define WLSTAT(fs,p,b) wStat((p), (b)) #define WREMOVE(fs,d) SYS_FS_FileDirectoryRemove((d)) #define WRENAME(fs,o,n) SYS_FS_FileDirectoryRenameMove((o),(n)) #define WS_DELIM '/' @@ -1495,6 +1498,7 @@ extern "C" { #define WOPEN(fs,f,m,p) open((f),(m),(p)) #define WCLOSE(fs,fd) close((fd)) + #define WFDOPEN(fs,f,fd,m) ((*(f) = fdopen((fd),(m))) == NULL) int wPwrite(WFD fd, unsigned char* buf, unsigned int sz, const unsigned int* shortOffset); int wPread(WFD fd, unsigned char* buf, unsigned int sz, @@ -1508,6 +1512,7 @@ extern "C" { /* returns 0 on success */ #define WOPENDIR(fs,h,c,d) ((*(c) = opendir((d))) == NULL) + #define WFDOPENDIR(fs,d,fd) ((*(d) = fdopendir((fd))) == NULL) #define WCLOSEDIR(fs,d) closedir(*(d)) #define WREADDIR(fs,d) readdir(*(d)) #define WREWINDDIR(fs,d) rewinddir(*(d)) @@ -1534,10 +1539,42 @@ extern "C" { /* wIsSymlink lives in the always-compiled port.c, but its filesystem * dependencies (WSTAT_T/WLSTAT/S_ISLNK on POSIX, WS_GetFileAttributesExA on * Windows) and its only callers exist solely in the SFTP and SCP code, so - * gate it on those features in addition to the platform capability. */ + * gate it on those features in addition to the platform capability. Shared by + * the SFTP path-confinement guard and the SCP send guards. */ #if defined(WOLFSSH_HAVE_SYMLINK) && \ (defined(WOLFSSH_SFTP) || defined(WOLFSSH_SCP)) WOLFSSH_LOCAL int wIsSymlink(const char* path); + + /* Open flag that refuses to follow a final-component symbolic link, so the + * open is atomic against a swapped link. Maps to the platform primitive + * where available (POSIX O_NOFOLLOW) and to 0 elsewhere (Windows and any + * filesystem lacking it), where wFopenNoFollow falls back to a wIsSymlink + * check before the open. */ + #ifdef O_NOFOLLOW + #define WOLFSSH_O_NOFOLLOW O_NOFOLLOW + #else + #define WOLFSSH_O_NOFOLLOW 0 + #endif + /* Require the opened path to be a directory; pairs with O_NOFOLLOW so a + * symlinked directory leaf is refused atomically. 0 where unavailable. */ + #ifdef O_DIRECTORY + #define WOLFSSH_O_DIRECTORY O_DIRECTORY + #else + #define WOLFSSH_O_DIRECTORY 0 + #endif + + /* Open a file for reading without following a final-component symlink. + * Returns 0 on success, non-zero on failure, matching the wfopen + * convention. Shared, un-followed read-open used by the SCP send guards. */ + WOLFSSH_LOCAL int wFopenNoFollow(void* fs, WFILE** f, const char* path); + /* Open a directory without following a final-component symlink; returns 0 + * on success, non-zero on failure, matching WOPENDIR. Not provided for + * Windows, which opens directories via WS_FindFirstFileA, nor when the + * directory macros are compiled out (NO_WOLFSSH_DIR). */ + #if !defined(USE_WINDOWS_API) && !defined(NO_WOLFSSH_DIR) + WOLFSSH_LOCAL int wOpendirNoFollow(void* fs, WDIR* dir, + const char* path); + #endif #endif #ifndef WS_MAYBE_UNUSED