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,49 @@ 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),
+# 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 <path>", 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
+
 echo -e "\nALL Tests Passed"
 
 exit 0

src/internal.c

@@ -1399,6 +1399,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) {

src/port.c

@@ -719,6 +719,44 @@ 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;
+}
 #endif /* WOLFSSH_HAVE_SYMLINK && (WOLFSSH_SFTP || WOLFSSH_SCP) */
 
 #ifndef WSTRING_USER

src/wolfscp.c

@@ -2455,7 +2455,10 @@ static int GetFileStats(void *fs, ScpSendCtx* ctx, const char* fileName,
         (ctx->s.dwFileAttributes & FILE_ATTRIBUTE_READONLY ? 0 : 0200);
     *fileMode |= (ctx->s.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) ? 040000 : 0;
 #else
-    if (WSTAT(fs, fileName, &ctx->s) < 0) {
+    /* WLSTAT (lstat on POSIX) does not follow a symlink, so the metadata and
+     * the dir/file classification below reflect the link itself, not its
+     * target; a symlink is then neither dir nor file and is not traversed. */
+    if (WLSTAT(fs, fileName, &ctx->s) < 0) {
         ret = WS_BAD_FILE_E;
         #ifdef WOLFSSL_NUCLEUS
         if (WSTRLEN(fileName) < 4 && WSTRLEN(fileName) > 2 &&
@@ -2626,6 +2629,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 +2834,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 +2863,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 +2890,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 +2979,21 @@ 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, atomic
+ * against a swapped link on POSIX (O_NOFOLLOW) and falling back to a wIsSymlink
+ * check-then-open on Windows or where O_NOFOLLOW is absent.  The recursive root
+ * is opened with opendir (which follows links), so it is leaf-checked with
+ * wIsSymlink instead; 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, but the directory descend
+ * still opens with opendir (which follows), and the Windows/fallback open is
+ * check-then-open, so a concurrent in-jail writer racing the descend 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 +3027,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 +3088,40 @@ 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 (opendir would follow it); see the
+                 * symlink-handling note in this function's header. */
+                ret = WS_SUCCESS;
+            #ifdef WOLFSSH_HAVE_SYMLINK
+                /* lstat() follows the link when the path ends in a separator,
+                 * so check the root with any trailing separators removed */
+                rootLen = (word32)WSTRLEN(peerRequest);
+                while (rootLen > 1 && (peerRequest[rootLen - 1] == '/' ||
+                                       peerRequest[rootLen - 1] == '\\'))
+                    rootLen--;
+                if (rootLen >= DEFAULT_SCP_FILE_NAME_SZ) {
+                    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 +3135,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;
                 }