Reject symlinks in default SCP send callback

Author: yosuke-wolfssl

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,46 @@ if test $RESULT -eq 0; then
     exit 1
 fi
 
+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).
+# The recursive directory-traversal sink in ScpProcessEntry uses the same shared
+# WS_IsSymlink() helper but cannot be driven from here: the wolfSSH example
+# client (wolfSSH_SCP_from) only ever issues "scp -f <path>", never "scp -f -r",
+# so the server's recursive request state is never reached by this harness.
+# WS_IsSymlink itself is additionally exercised through the SFTP confinement
+# unit test (test_wolfSSH_SFTP_Confinement), so the detection logic shared by
+# both 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
+
 echo -e "\nALL Tests Passed"
 
 exit 0

src/port.c

@@ -527,6 +527,44 @@ int WS_DeleteFileA(const char* fileName, void* heap)
 
 #endif /* USE_WINDOWS_API WOLFSSH_SFTP WOLFSSH_SCP */
 
+#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 NULL or non-existent path
+ * (stat fails) is reported as not-a-link.  Shared by the SFTP path-confinement
+ * check and the SCP send guards: both must reject a link before a file
+ * operation follows it, and one implementation keeps that security-relevant
+ * test from drifting between the two. */
+int WS_IsSymlink(const char* path)
+{
+    int isLink = 0;
+#ifdef USE_WINDOWS_API
+    WIN32_FILE_ATTRIBUTE_DATA attrs;
+#else
+    WSTAT_T lst;
+#endif
+
+    if (path == NULL)
+        return 0;
+
+#ifdef USE_WINDOWS_API
+    /* GetFileAttributesEx reports the link's own attributes; it does not follow
+     * the reparse point.  WS_GetFileAttributesExA trims any leading slash from
+     * the SFTP-canonical "/C:/..." form and uses the wide-char API like every
+     * other Windows file op here. */
+    if (WS_GetFileAttributesExA(path, &attrs, NULL) != 0 &&
+            (attrs.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0) {
+        isLink = 1;
+    }
+#else
+    if (WLSTAT(NULL, path, &lst) == 0 && S_ISLNK(lst.st_mode)) {
+        isLink = 1;
+    }
+#endif
+    return isLink;
+}
+#endif /* WOLFSSH_HAVE_SYMLINK && (WOLFSSH_SFTP || WOLFSSH_SCP) */
+
 #if !defined(NO_FILESYSTEM) && \
     defined(WOLFSSH_ZEPHYR) && (defined(WOLFSSH_SFTP) || defined(WOLFSSH_SCP))
 

src/wolfscp.c

@@ -2610,6 +2610,18 @@ 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 now fully built.  Reject a planted symlink here,
+             * before GetFileStats (which classifies via WSTAT and follows
+             * links) or any later descend/open follows it - the whole point is
+             * to refuse the link before any file operation traverses it. */
+            if (ret == WS_SUCCESS && WS_IsSymlink(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);
             }
@@ -2652,7 +2664,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;
         }
@@ -2771,8 +2786,19 @@ 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) {
+        #ifdef WOLFSSH_HAVE_SYMLINK
+            /* WFOPEN follows symlinks; refuse to open one so its target is not
+             * streamed to the peer. */
+            if (WS_IsSymlink(peerRequest)) {
+                WLOG(WS_LOG_ERROR, "scp: refusing to open symlink, abort");
+                wolfSSH_SetScpErrorMsg(ssh, "unable to open file for reading");
+                ret = WS_BAD_FILE_E;
+            }
+        #endif /* WOLFSSH_HAVE_SYMLINK */
+
+            if (ret == WS_SUCCESS &&
+                ((sendCtx == NULL) || WFOPEN(ssh->fs, &(sendCtx->fp),
+                                             peerRequest, "rb") != 0)) {
 
                 WLOG(WS_LOG_ERROR, "scp: unable to open file, abort");
                 wolfSSH_SetScpErrorMsg(ssh, "unable to open file for reading");
@@ -2828,7 +2854,17 @@ int wsScpSendCallback(WOLFSSH* ssh, int state, const char* peerRequest,
 
             if (ScpDirStackIsEmpty(sendCtx)) {
 
-                /* first request, keep track of request directory */
+                /* first request, keep track of request directory.  The
+                 * peer-named recursive root is intentionally not run through
+                 * WS_IsSymlink here: unlike SFTP there is no library-level
+                 * trusted base path to contain a resolved root against (SCP
+                 * confinement in wolfsshd is enforced by OS chroot, inside
+                 * which the kernel already prevents a symlink from escaping),
+                 * and wolfSSH_RealPath does not resolve links so a "stays under
+                 * the root" check is not possible.  Symlinks discovered while
+                 * traversing below the root are still rejected by
+                 * ScpProcessEntry, which is the planted-link threat this guards
+                 * against. */
                 ret = ScpPushDir(ssh->fs, sendCtx, peerRequest, ssh->ctx->heap);
 
                 if (ret == WS_SUCCESS) {

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 (WS_IsSymlink(s)) {
                 ret = WS_PERMISSIONS;
             }
             s[i] = saved;

wolfssh/port.h

@@ -1521,6 +1521,13 @@ extern "C" {
     #define WOLFSSH_HAVE_SYMLINK
 #endif
 
+#if defined(WOLFSSH_HAVE_SYMLINK) && \
+    (defined(WOLFSSH_SFTP) || defined(WOLFSSH_SCP))
+    /* Shared, un-followed symlink/reparse-point check used by both the SFTP
+     * path-confinement guard and the SCP send guards. */
+    WOLFSSH_LOCAL int WS_IsSymlink(const char* path);
+#endif
+
 #ifndef WS_MAYBE_UNUSED
     #if (defined(__GNUC__) && (__GNUC__ >= 4)) || defined(__clang__) || \
             defined(__IAR_SYSTEMS_ICC__)