From a47d723feba86e1cc195636c7f3ce6361c86dcac Mon Sep 17 00:00:00 2001 From: Yosuke Shimizu Date: Tue, 16 Jun 2026 09:54:21 +0900 Subject: [PATCH] Reject SCP receive through pre-existing symlinks --- src/port.c | 32 +++++++++ src/wolfscp.c | 22 +++++++ src/wolfsftp.c | 35 +--------- tests/unit.c | 173 +++++++++++++++++++++++++++++++++++++++++++++++++ wolfssh/port.h | 9 +++ 5 files changed, 237 insertions(+), 34 deletions(-) diff --git a/src/port.c b/src/port.c index ac53a3b07..036974e10 100644 --- a/src/port.c +++ b/src/port.c @@ -689,6 +689,38 @@ int wChmod(const char *path, int mode) } #endif #endif /* NO_FILESYSTEM */ + +#if defined(WOLFSSH_HAVE_SYMLINK) && \ + (defined(WOLFSSH_SFTP) || defined(WOLFSSH_SCP)) +/* Returns 1 if path is a symbolic link (POSIX) or a reparse point such as a + * symlink or junction (Windows), otherwise 0. A non-existent path (stat + * fails) is reported as not-a-link so that create requests for a new leaf are + * still permitted by the caller. Shared by the SFTP and SCP path-confinement + * checks. */ +int wIsSymlink(const char* path) +{ + int isLink = 0; +#ifdef USE_WINDOWS_API + WIN32_FILE_ATTRIBUTE_DATA attrs; + + /* Route through WS_GetFileAttributesExA so the wide-char API and any path + * trimming match the other Windows file ops. GetFileAttributesEx reports + * the link's own attributes; it does not follow the reparse point. */ + if (path != NULL && WS_GetFileAttributesExA(path, &attrs, NULL) != 0 && + (attrs.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0) { + isLink = 1; + } +#else + WSTAT_T lst; + + if (path != NULL && WLSTAT(NULL, path, &lst) == 0 && S_ISLNK(lst.st_mode)) { + isLink = 1; + } +#endif + return isLink; +} +#endif /* WOLFSSH_HAVE_SYMLINK && (WOLFSSH_SFTP || WOLFSSH_SCP) */ + #ifndef WSTRING_USER char* wstrdup(const char* s1, void* heap, int type) diff --git a/src/wolfscp.c b/src/wolfscp.c index 62591596e..8e7511da6 100644 --- a/src/wolfscp.c +++ b/src/wolfscp.c @@ -2039,6 +2039,17 @@ int wsScpRecvCallback(WOLFSSH* ssh, int state, const char* basePath, wolfSSH_CleanPath(ssh, abslut, WOLFSSH_MAX_FILENAME); if (WFOPEN(ssh->fs, &fp, abslut, "wb") != 0) { #else + #ifdef WOLFSSH_HAVE_SYMLINK + /* refuse to write through a pre-existing symlink, which would + * escape the destination directory */ + if (wIsSymlink(fileName)) { + WLOG(WS_LOG_ERROR, + "scp: refusing to write through symlink, abort"); + wolfSSH_SetScpErrorMsg(ssh, "symlink target rejected"); + ret = WS_SCP_ABORT; + break; + } + #endif if (WFOPEN(ssh->fs, &fp, fileName, "wb") != 0) { #endif WLOG(WS_LOG_ERROR, @@ -2153,6 +2164,17 @@ int wsScpRecvCallback(WOLFSSH* ssh, int state, const char* basePath, wolfSSH_CleanPath(ssh, (char*)basePath, WOLFSSH_MAX_FILENAME); ssh->scpDirDepth++; #else + #ifdef WOLFSSH_HAVE_SYMLINK + /* WMKDIR returning EEXIST above may have matched a pre-existing + * symlink; refuse to follow it out of the destination dir */ + if (wIsSymlink(fileName)) { + WLOG(WS_LOG_ERROR, + "scp: refusing to enter symlinked directory, abort"); + wolfSSH_SetScpErrorMsg(ssh, "symlink in destination path"); + ret = WS_SCP_ABORT; + break; + } + #endif if (WCHDIR(ssh->fs, fileName) != 0) { WLOG(WS_LOG_ERROR, "scp: unable to cd into directory, abort"); diff --git a/src/wolfsftp.c b/src/wolfsftp.c index 921341772..4fabea0e0 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -1783,39 +1783,6 @@ int wolfSSH_SFTP_CreateStatus(WOLFSSH* ssh, word32 status, word32 reqId, } -#ifdef WOLFSSH_HAVE_SYMLINK -/* Returns 1 if path is a symbolic link (POSIX) or a reparse point such as a - * symlink or junction (Windows), otherwise 0. A non-existent path (stat - * fails) is reported as not-a-link so that create requests for a new leaf are - * still permitted by the caller. */ -static int SFTP_IsSymlink(const char* path) -{ - int isLink = 0; -#ifdef USE_WINDOWS_API - WIN32_FILE_ATTRIBUTE_DATA attrs; - - /* GetAndCleanPath produces an SFTP-canonical "/C:/..." path. Route it - * through WS_GetFileAttributesExA, which trims the leading slash and uses - * the wide-char API like every other Windows file op here; calling - * GetFileAttributesA on the raw "/C:/..." form would always fail and - * silently disable link detection. GetFileAttributesEx reports the link's - * own attributes (it does not follow the reparse point). */ - if (WS_GetFileAttributesExA(path, &attrs, NULL) != 0 && - (attrs.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0) { - isLink = 1; - } -#else - WSTAT_T lst; - - if (WLSTAT(NULL, path, &lst) == 0 && S_ISLNK(lst.st_mode)) { - isLink = 1; - } -#endif - return isLink; -} -#endif /* WOLFSSH_HAVE_SYMLINK */ - - /* * This is a wrapper around the function wolfSSH_RealPath. Since it modifies * the source path value, copy the path from the data stream into a local @@ -1899,7 +1866,7 @@ static int GetAndCleanPath(const char* defaultPath, } saved = s[i]; s[i] = '\0'; - if (SFTP_IsSymlink(s)) { + if (wIsSymlink(s)) { ret = WS_PERMISSIONS; } s[i] = saved; diff --git a/tests/unit.c b/tests/unit.c index 85092fd6c..306500945 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -3861,6 +3861,174 @@ static int test_ScpRecvCallback_NewDirChdirFail(void) return result; } +/* A pre-existing symlink in the destination directory must not be followed + * out of that directory, neither when entering it as a directory nor when + * opening it as a file. */ +static int test_ScpRecvCallback_SymlinkGuard(void) +{ +#ifndef WOLFSSH_HAVE_SYMLINK + /* symlink rejection is compiled out on this configuration */ + return 0; +#else + char tmpDir[] = "/tmp/wolfssh_scpXXXXXX"; + char basePathRaw[PATH_MAX]; + char outsidePath[PATH_MAX]; + char linkDirPath[PATH_MAX]; + char linkFilePath[PATH_MAX]; + char leakedPath[PATH_MAX]; + char origCwd[PATH_MAX]; + char* basePath = NULL; + struct stat st; + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + int baseMkdirDone = 0; + int outsideMkdirDone = 0; + int linkDirDone = 0; + int linkFileDone = 0; + int origCwdSaved = 0; + int ret; + int result = 0; + + basePathRaw[0] = '\0'; + outsidePath[0] = '\0'; + linkDirPath[0] = '\0'; + linkFilePath[0] = '\0'; + leakedPath[0] = '\0'; + + if (getcwd(origCwd, sizeof(origCwd)) == NULL) + return -840; + origCwdSaved = 1; + + if (mkdtemp(tmpDir) == NULL) + return -841; + + ret = snprintf(basePathRaw, sizeof(basePathRaw), "%s/scp_target", tmpDir); + if (!scpTestSnprintfOk(ret, sizeof(basePathRaw))) { + result = -842; + goto cleanup; + } + if (mkdir(basePathRaw, 0755) != 0) { + result = -843; + goto cleanup; + } + baseMkdirDone = 1; + + ret = snprintf(outsidePath, sizeof(outsidePath), "%s/outside", tmpDir); + if (!scpTestSnprintfOk(ret, sizeof(outsidePath))) { + result = -844; + goto cleanup; + } + if (mkdir(outsidePath, 0755) != 0) { + result = -845; + goto cleanup; + } + outsideMkdirDone = 1; + + basePath = realpath(basePathRaw, NULL); + if (basePath == NULL) { + result = -846; + goto cleanup; + } + + ret = snprintf(linkDirPath, sizeof(linkDirPath), "%s/linkdir", basePath); + if (!scpTestSnprintfOk(ret, sizeof(linkDirPath))) { + result = -847; + goto cleanup; + } + ret = snprintf(linkFilePath, sizeof(linkFilePath), "%s/linkfile", basePath); + if (!scpTestSnprintfOk(ret, sizeof(linkFilePath))) { + result = -858; + goto cleanup; + } + ret = snprintf(leakedPath, sizeof(leakedPath), "%s/leaked.txt", outsidePath); + if (!scpTestSnprintfOk(ret, sizeof(leakedPath))) { + result = -859; + goto cleanup; + } + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) { + result = -848; + goto cleanup; + } + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { + result = -849; + goto cleanup; + } + + /* NEW_REQUEST changes the working directory into basePath */ + ret = wsScpRecvCallback(ssh, WOLFSSH_SCP_NEW_REQUEST, basePath, + NULL, 0, 0, 0, 0, NULL, 0, 0, NULL); + if (ret != WS_SCP_CONTINUE) { + result = -850; + goto cleanup; + } + + /* plant a directory symlink pointing outside basePath */ + if (symlink(outsidePath, linkDirPath) != 0) { + result = -851; + goto cleanup; + } + linkDirDone = 1; + + /* WMKDIR returns EEXIST for the existing symlink; the callback must + * refuse to chdir through it rather than escape basePath */ + ret = wsScpRecvCallback(ssh, WOLFSSH_SCP_NEW_DIR, basePath, + "linkdir", 0755, 0, 0, 0, NULL, 0, 0, NULL); + if (ret != WS_SCP_ABORT) { + result = -852; + goto cleanup; + } + if (ssh->scpDirDepth != 0) { + result = -853; + goto cleanup; + } + + /* plant a (dangling) file symlink pointing outside basePath */ + if (symlink(leakedPath, linkFilePath) != 0) { + result = -854; + goto cleanup; + } + linkFileDone = 1; + + /* the callback must refuse to open the symlink rather than write through + * it to the outside target */ + ret = wsScpRecvCallback(ssh, WOLFSSH_SCP_NEW_FILE, basePath, + "linkfile", 0644, 0, 0, 0, NULL, 0, 0, NULL); + if (ret != WS_SCP_ABORT) { + result = -855; + goto cleanup; + } + if (stat(leakedPath, &st) == 0) { + (void)remove(leakedPath); + result = -856; + goto cleanup; + } + +cleanup: + if (ssh != NULL) + wolfSSH_free(ssh); + if (ctx != NULL) + wolfSSH_CTX_free(ctx); + free(basePath); + /* NEW_REQUEST changed the process CWD into basePath, so leave it before + * removing the created directories or the rmdir calls would fail */ + if (origCwdSaved && chdir(origCwd) != 0 && result == 0) + result = -857; + if (linkDirDone) + (void)remove(linkDirPath); + if (linkFileDone) + (void)remove(linkFilePath); + if (outsideMkdirDone) + (void)rmdir(outsidePath); + if (baseMkdirDone) + (void)rmdir(basePathRaw); + (void)rmdir(tmpDir); + return result; +#endif /* WOLFSSH_HAVE_SYMLINK */ +} + #endif /* WOLFSSH_SCP recv callback depth guard test */ #endif /* WOLFSSH_TEST_INTERNAL */ @@ -4031,6 +4199,11 @@ int wolfSSH_UnitTest(int argc, char** argv) printf("ScpRecvCallback_NewDirChdirFail: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; + + unitResult = test_ScpRecvCallback_SymlinkGuard(); + printf("ScpRecvCallback_SymlinkGuard: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; #endif #ifdef WOLFSSH_TEST_CAPTURING_ALLOCATOR diff --git a/wolfssh/port.h b/wolfssh/port.h index e327c12e7..e357a256e 100644 --- a/wolfssh/port.h +++ b/wolfssh/port.h @@ -1531,6 +1531,15 @@ extern "C" { #define WOLFSSH_HAVE_SYMLINK #endif +/* 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. */ +#if defined(WOLFSSH_HAVE_SYMLINK) && \ + (defined(WOLFSSH_SFTP) || defined(WOLFSSH_SCP)) + WOLFSSH_LOCAL int wIsSymlink(const char* path); +#endif + #ifndef WS_MAYBE_UNUSED #if (defined(__GNUC__) && (__GNUC__ >= 4)) || defined(__clang__) || \ defined(__IAR_SYSTEMS_ICC__)