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/wolfscp.c

@@ -2820,6 +2820,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 && 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);
             }
@@ -2862,7 +2874,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;
         }
@@ -2981,8 +2996,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 (wIsSymlink(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");
@@ -3038,7 +3064,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
+                 * wIsSymlink 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) {

wolfssh/port.h

@@ -1534,7 +1534,8 @@ 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);