Reject symlinks in default SCP send callback

Author: yosuke-wolfssl

examples/client/client.c

@@ -129,6 +129,11 @@ static void ShowUsage(void)
     printf(" -k            set the list of key algos\n");
     printf(" -C            set the list of encrypt algos\n");
     printf(" -q            turn off debugging output\n");
+#ifndef WOLFSSH_HAVE_SYMLINK
+    /* report disabled symlink checking so test scripts can skip the
+     * symlink-rejection cases the server would otherwise follow */
+    printf("               WOLFSSH_NO_SYMLINK_CHECK\n");
+#endif
 }
 
 

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,58 @@ if test $RESULT -eq 0; then
     exit 1
 fi
 
+# The symlink-rejection guard is compiled out by WOLFSSH_NO_SYMLINK_CHECK, which
+# the example client reports in -h.  Skip this test when it is set: the server
+# then follows symlinks by design and the check below would false-fail.
+./examples/client/client -h | grep WOLFSSH_NO_SYMLINK_CHECK
+if [ $? -eq 0 ]; then
+    echo "symlink checking disabled, skipping symlink test"
+else
+    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
+fi
+
 echo -e "\nALL Tests Passed"
 
 exit 0

src/internal.c

@@ -1415,6 +1415,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,82 @@ 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;
+}
+
+#ifndef USE_WINDOWS_API
+/* Open a directory without following a final-component symlink: POSIX uses
+ * O_DIRECTORY|O_NOFOLLOW + fdopendir (atomic), else falls back to wIsSymlink +
+ * opendir.  Returns 0 on success, non-zero on failure (matches WOPENDIR). */
+int wOpendirNoFollow(void* fs, WDIR* dir, const char* path)
+{
+    int ret = 0;
+#if defined(O_NOFOLLOW) && defined(O_DIRECTORY)
+    WFD fd = -1;
+#endif
+
+    if (dir != NULL)
+        *dir = NULL;
+    if (dir == NULL || path == NULL)
+        ret = 1;
+
+#if defined(O_NOFOLLOW) && defined(O_DIRECTORY)
+    if (ret == 0) {
+        fd = WOPEN(fs, path, WOLFSSH_O_RDONLY | WOLFSSH_O_DIRECTORY |
+                   WOLFSSH_O_NOFOLLOW, 0);
+        if (fd < 0)
+            ret = 1;
+    }
+    if (ret == 0 && WFDOPENDIR(fs, dir, fd) != 0) {
+        WCLOSE(fs, fd);
+        ret = 1;
+    }
+#else
+    if (ret == 0 && wIsSymlink(path))
+        ret = 1;
+    if (ret == 0 && WOPENDIR(fs, NULL, dir, path) != 0)
+        ret = 1;
+#endif
+    WOLFSSH_UNUSED(fs);
+    return ret;
+}
+#endif /* !USE_WINDOWS_API */
 #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 &&
@@ -2543,11 +2546,16 @@ static ScpDir* ScpNewDir(void *fs, const char* path, void* heap)
         }
     }
 #else
+    #ifdef WOLFSSH_HAVE_SYMLINK
+    /* refuse a symlinked directory leaf atomically (closes the descend race) */
+    if (wOpendirNoFollow(fs, &entry->dir, path) != 0) {
+    #else
     if (WOPENDIR(fs, heap, &entry->dir, path) != 0
         #if !defined(WOLFSSL_NUCLEUS) && !defined(WOLFSSH_ZEPHYR)
             || entry->dir == NULL
         #endif
             ) {
+    #endif
         WFREE(entry, heap, DYNTYPE_SCPDIR);
         WLOG(WS_LOG_ERROR, scpError, "opendir failed on directory",
              WS_INVALID_PATH_E);
@@ -2626,6 +2634,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 +2839,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 +2868,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 +2895,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 +2984,22 @@ 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 and directory
+ * opens (both the recursive root and every descend) go through wOpendirNoFollow.
+ * Both are atomic against a swapped link on POSIX (O_NOFOLLOW, plus O_DIRECTORY
+ * for the directory open) and fall back to a wIsSymlink check-then-open on
+ * Windows or where O_NOFOLLOW is absent.  The root also gets an explicit
+ * wIsSymlink pre-check because a trailing separator (open("link/", O_NOFOLLOW))
+ * can still follow the link; 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.  On the
+ * Windows/fallback path the open is check-then-open, so a concurrent in-jail
+ * writer racing it 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 +3033,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 +3094,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 */
-                ret = ScpPushDir(ssh->fs, sendCtx, peerRequest, ssh->ctx->heap);
+                /* first request, keep track of request directory.  Reject a
+                 * symlink root here (a trailing separator can still follow);
+                 * 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 */
+
+                if (ret == WS_SUCCESS)
+                    ret = ScpPushDir(ssh->fs, sendCtx, peerRequest,
+                                     ssh->ctx->heap);
 
                 if (ret == WS_SUCCESS) {
                     /* get file name from request */
@@ -3053,7 +3141,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;
                 }