Reject SCP receive through pre-existing symlinks

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)

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");

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;

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

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__)