Reject symlinks in default SCP send callback

yosuke-wolfssl · yosuke-wolfssl · commit 4f9761c320ee · 2026-06-22T11:46:10.000+09:00
diff --git a/scripts/scp.test b/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
diff --git a/src/port.c b/src/port.c
@@ -719,6 +719,43 @@ 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(WFILE** f, const char* path)
+{
+    int ret = 0;
+#if !defined(USE_WINDOWS_API) && defined(O_NOFOLLOW)
+    WFD fd;
+#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(NULL, path, WOLFSSH_O_RDONLY | WOLFSSH_O_NOFOLLOW, 0);
+        if (fd < 0)
+            ret = 1;
+    }
+    if (ret == 0 && WFDOPEN(NULL, f, fd, "rb") != 0) {
+        WCLOSE(NULL, fd);
+        ret = 1;
+    }
+#else
+    if (ret == 0 && wIsSymlink(path))
+        ret = 1;
+    if (ret == 0 && WFOPEN(NULL, f, path, "rb") != 0)
+        ret = 1;
+#endif
+    return ret;
+}
 #endif /* WOLFSSH_HAVE_SYMLINK && (WOLFSSH_SFTP || WOLFSSH_SCP) */
 
 #ifndef WSTRING_USER
diff --git a/src/wolfscp.c b/src/wolfscp.c
@@ -2820,6 +2820,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 +2849,11 @@ static int ScpProcessEntry(WOLFSSH* ssh, char* fileName, word64* mTime,
             }
 
         } else if (ScpFileIsFile(sendCtx)) {
+        #ifdef WOLFSSH_HAVE_SYMLINK
+            if (wFopenNoFollow(&(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 +2876,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 +2965,20 @@ 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.  The metadata WSTAT in GetFileStats
+ * still follows links and the Windows/fallback open is check-then-open, so
+ * those remain best-effort: a concurrent in-jail writer running as the server
+ * user could race the metadata stat.  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 +3012,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(&(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;
@@ -3038,8 +3074,23 @@ int wsScpSendCallback(WOLFSSH* ssh, int state, const char* peerRequest,
 
             if (ScpDirStackIsEmpty(sendCtx)) {
 
-                /* 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 (opendir would follow it); see the
+                 * symlink-handling note in this function's header. */
+                ret = WS_SUCCESS;
+            #ifdef WOLFSSH_HAVE_SYMLINK
+                if (wIsSymlink(peerRequest)) {
+                    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 +3104,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;
                 }
diff --git a/tests/api.c b/tests/api.c
@@ -40,6 +40,13 @@
     #include <stdlib.h>
     #include <unistd.h>
 #endif
+#if defined(WOLFSSH_SCP) && defined(WOLFSSH_HAVE_SYMLINK) && \
+    !defined(SINGLE_THREADED) && !defined(WOLFSSH_ZEPHYR) && \
+    !defined(USE_WINDOWS_API)
+    /* mkdtemp()/symlink() for the SCP send symlink-rejection test */
+    #include <stdlib.h>
+    #include <unistd.h>
+#endif
 #include <wolfssh/ssh.h>
 #include <wolfssh/internal.h>
 #include <wolfssh/log.h>
@@ -1045,6 +1052,88 @@ static void test_wolfSSH_SCP_CB(void)
 static void test_wolfSSH_SCP_CB(void) { ; }
 #endif /* WOLFSSH_SCP */
 
+#if defined(WOLFSSH_SCP) && defined(WOLFSSH_HAVE_SYMLINK) && \
+    !defined(SINGLE_THREADED) && !defined(WOLFSSH_ZEPHYR) && \
+    !defined(USE_WINDOWS_API)
+/* The default SCP send callback must refuse a symlink so its target is not
+ * streamed to the peer.  The example SCP client cannot issue a recursive
+ * ("scp -f -r") request, so the recursive-root guard in wsScpSendCallback is
+ * unreachable through scripts/scp.test; drive it here at the callback level
+ * with a planted symlink directory.  wFopenNoFollow, shared by the single-file
+ * and recursive file opens, is exercised directly for both the accept (real
+ * file) and reject (symlink) cases. */
+static void test_wolfSSH_SCP_SendSymlinkReject(void)
+{
+    WOLFSSH_CTX* ctx = NULL;
+    WOLFSSH*     ssh = NULL;
+    ScpSendCtx   sendCtx;
+    WFILE*       fp = NULL;
+    char   scpRoot[]  = "/tmp/wolfssh_scp_sym_XXXXXX";
+    char   realFile[WOLFSSH_MAX_FILENAME];
+    char   symToFile[WOLFSSH_MAX_FILENAME];
+    char   symToDir[WOLFSSH_MAX_FILENAME];
+    char   fileName[DEFAULT_SCP_FILE_NAME_SZ];
+    byte   buf[256];
+    word64 mTime = 0;
+    word64 aTime = 0;
+    int    fileMode = 0;
+    word32 totalSz = 0;
+
+    AssertNotNull(mkdtemp(scpRoot));
+
+    WSNPRINTF(realFile,  sizeof(realFile),  "%s/secret",    scpRoot);
+    WSNPRINTF(symToFile, sizeof(symToFile), "%s/link_file", scpRoot);
+    WSNPRINTF(symToDir,  sizeof(symToDir),  "%s/link_dir",  scpRoot);
+
+    /* stage an empty real file plus a symlink to it and to the temp dir */
+    AssertIntEQ(WFOPEN(NULL, &fp, realFile, "wb"), 0);
+    WFCLOSE(NULL, fp);
+    fp = NULL;
+    AssertIntEQ(symlink(realFile, symToFile), 0);
+    AssertIntEQ(symlink(scpRoot,  symToDir),  0);
+
+    /* the guard's discriminator: a real path is not a link, the planted ones
+     * are - so a genuine directory would not trip the recursive-root check */
+    AssertIntEQ(wIsSymlink(scpRoot),  0);
+    AssertIntEQ(wIsSymlink(symToDir), 1);
+
+    /* wFopenNoFollow opens a real file but refuses a symlink */
+    AssertIntEQ(wFopenNoFollow(&fp, realFile), 0);
+    AssertNotNull(fp);
+    WFCLOSE(NULL, fp);
+    fp = NULL;
+    AssertIntNE(wFopenNoFollow(&fp, symToFile), 0);
+    AssertNull(fp);
+
+    AssertNotNull(ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL));
+    AssertNotNull(ssh = wolfSSH_new(ctx));
+    WMEMSET(&sendCtx, 0, sizeof(sendCtx));
+    WMEMSET(fileName, 0, sizeof(fileName));
+
+    /* a benign state returns success (the callback does not blanket-abort) */
+    AssertIntEQ(wsScpSendCallback(ssh, WOLFSSH_SCP_NEW_REQUEST, symToDir,
+            fileName, (word32)sizeof(fileName), &mTime, &aTime, &fileMode, 0,
+            &totalSz, buf, (word32)sizeof(buf), &sendCtx), WS_SUCCESS);
+
+    /* a recursive root that is a symlink aborts before any directory is
+     * opened, so nothing is pushed onto the stack and nothing leaks */
+    AssertIntEQ(wsScpSendCallback(ssh, WOLFSSH_SCP_RECURSIVE_REQUEST, symToDir,
+            fileName, (word32)sizeof(fileName), &mTime, &aTime, &fileMode, 0,
+            &totalSz, buf, (word32)sizeof(buf), &sendCtx), WS_SCP_ABORT);
+    AssertNull(sendCtx.currentDir);
+
+    wolfSSH_free(ssh);
+    wolfSSH_CTX_free(ctx);
+
+    WREMOVE(NULL, symToFile);
+    WREMOVE(NULL, symToDir);
+    WREMOVE(NULL, realFile);
+    WRMDIR(NULL, scpRoot);
+}
+#else
+static void test_wolfSSH_SCP_SendSymlinkReject(void) { ; }
+#endif
+
 #ifdef WOLFSSH_AGENT
 typedef struct AgentTestCtx {
     int partialWrite;
@@ -3610,6 +3699,7 @@ int wolfSSH_ApiTest(int argc, char** argv)
 
     /* SCP tests */
     test_wolfSSH_SCP_CB();
+    test_wolfSSH_SCP_SendSymlinkReject();
     test_wolfSSH_SCP_ReKey();
     test_wolfSSH_SCP_ReKey_NonBlock();
     test_wolfSSH_SCP_ReKey_ToServer();
diff --git a/wolfssh/port.h b/wolfssh/port.h
@@ -1325,6 +1325,7 @@ extern "C" {
     #define WGETCWD(fs,r,rSz)    _getcwd((r),(rSz))
     #define WOPEN(fs,f,m,p)      _open((f),(m),(p))
     #define WCLOSE(fs,fd)        _close((fd))
+    #define WFDOPEN(fs,f,fd,m)   ((*(f) = _fdopen((fd),(m))) == NULL)
 
     #define WFD int
     int wPwrite(WFD fd, unsigned char* buf, unsigned int sz,
@@ -1495,6 +1496,7 @@ extern "C" {
 
     #define WOPEN(fs,f,m,p) open((f),(m),(p))
     #define WCLOSE(fs,fd) close((fd))
+    #define WFDOPEN(fs,f,fd,m) ((*(f) = fdopen((fd),(m))) == NULL)
     int wPwrite(WFD fd, unsigned char* buf, unsigned int sz,
             const unsigned int* shortOffset);
     int wPread(WFD fd, unsigned char* buf, unsigned int sz,
@@ -1534,10 +1536,27 @@ 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);
+
+    /* Open flag that refuses to follow a final-component symbolic link, so the
+     * open is atomic against a swapped link.  Maps to the platform primitive
+     * where available (POSIX O_NOFOLLOW) and to 0 elsewhere (Windows and any
+     * filesystem lacking it), where wFopenNoFollow falls back to a wIsSymlink
+     * check before the open. */
+    #ifdef O_NOFOLLOW
+        #define WOLFSSH_O_NOFOLLOW O_NOFOLLOW
+    #else
+        #define WOLFSSH_O_NOFOLLOW 0
+    #endif
+
+    /* Open a file for reading without following a final-component symlink.
+     * Returns 0 on success, non-zero on failure, matching the wfopen
+     * convention.  Shared, un-followed read-open used by the SCP send guards. */
+    WOLFSSH_LOCAL int wFopenNoFollow(WFILE** f, const char* path);
 #endif
 
 #ifndef WS_MAYBE_UNUSED
