wolfsftp: reject symlink leaf in SFTP_RecvOpen

Author: yosuke-wolfssl

src/wolfsftp.c

@@ -1816,6 +1816,22 @@ static int SFTP_IsSymlink(const char* path)
 #endif /* WOLFSSH_HAVE_SYMLINK */
 
 
+/* Length of the confining prefix of an SFTP default path: its length with any
+ * trailing separators stripped (a lone "/" is kept).  A result > 1 means the
+ * session is confined to something deeper than the filesystem root.  Defined
+ * once here so the confinement gate stays consistent between GetAndCleanPath
+ * and SFTP_SessionConfined.  Caller must pass a non-NULL path. */
+static word32 SFTP_ConfinedPathLen(const char* defaultPath)
+{
+    word32 dpLen = (word32)WSTRLEN(defaultPath);
+
+    while (dpLen > 1 && WOLFSSH_SFTP_IS_DELIM(defaultPath[dpLen - 1])) {
+        dpLen--;
+    }
+    return dpLen;
+}
+
+
 /*
  * 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
@@ -1845,11 +1861,7 @@ static int GetAndCleanPath(const char* defaultPath,
         /* defaultPath is stored in canonical form by
          * wolfSSH_SFTP_SetDefaultPath, so a direct prefix compare against the
          * canonical resolved request path enforces confinement. */
-        dpLen = (word32)WSTRLEN(defaultPath);
-        /* strip trailing separator(s), but keep a lone "/" as-is */
-        while (dpLen > 1 && WOLFSSH_SFTP_IS_DELIM(defaultPath[dpLen - 1])) {
-            dpLen--;
-        }
+        dpLen = SFTP_ConfinedPathLen(defaultPath);
         if (dpLen > 1) {
             /* resolved path must equal the default path or be within its
              * subtree. On Windows the filesystem is case-insensitive and the
@@ -1911,6 +1923,22 @@ static int GetAndCleanPath(const char* defaultPath,
 }
 
 
+#ifndef USE_WINDOWS_API
+/* Returns 1 when the session is confined to a default path deeper than the
+ * filesystem root.  This mirrors the gate GetAndCleanPath uses for its
+ * per-component symlink check, so the open-time symlink defenses stay limited
+ * to confined sessions and do not change behavior for servers that
+ * intentionally follow symlinks (the default, like OpenSSH's sftp-server). */
+static int SFTP_SessionConfined(const char* defaultPath)
+{
+    if (defaultPath == NULL) {
+        return 0;
+    }
+    return (SFTP_ConfinedPathLen(defaultPath) > 1) ? 1 : 0;
+}
+#endif /* !USE_WINDOWS_API */
+
+
 /* Builds a status packet and queues it for send.
  * Returns WS_SUCCESS on success; ssh takes ownership of the allocated buffer. */
 static int SFTP_SendStatus(WOLFSSH* ssh, byte type, int reqId, const char* msg)
@@ -2271,6 +2299,7 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
     int rc;
     int fdOpened = 0;
     int outOwnedBySsh = 0;
+    int confined;
 
     word32 outSz = sizeof(WFD) + UINT32_SZ + WOLFSSH_SFTP_HEADER;
     byte*  out = NULL;
@@ -2290,6 +2319,10 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
 
     WLOG(WS_LOG_SFTP, "Receiving WOLFSSH_FTP_OPEN");
 
+    /* only apply the symlink-follow defenses when the session is confined, so
+     * unconfined servers keep following symlinks as they did before */
+    confined = SFTP_SessionConfined(ssh->sftpDefaultPath);
+
     #if defined(MICROCHIP_MPLAB_HARMONY) || defined(FREESCALE_MQX)
         fd = WBADFILE;
     #else
@@ -2369,7 +2402,8 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
         WS_SFTP_FILEATRB fileAtr;
         WMEMSET(&fileAtr, 0, sizeof(fileAtr));
         if (SFTP_GetAttributes(ssh->fs,
-                        dir, &fileAtr, 0, ssh->ctx->heap) == WS_SUCCESS) {
+                        dir, &fileAtr, (byte)confined, ssh->ctx->heap)
+                        == WS_SUCCESS) {
             if ((fileAtr.per & FILEATRB_PER_MASK_TYPE)
                         != FILEATRB_PER_FILE) {
                 ssh->error = WS_SFTP_NOT_FILE_E;
@@ -2391,6 +2425,13 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
             atr.per = 0644;
         }
 
+        /* when confined, refuse to follow a symlink leaf, closing the TOCTOU
+         * window between the type check above and the open below. No-op where
+         * the platform has no O_NOFOLLOW (WOLFSSH_O_NOFOLLOW is 0). */
+        if (confined) {
+            m |= WOLFSSH_O_NOFOLLOW;
+        }
+
     #ifdef MICROCHIP_MPLAB_HARMONY
         {
             WFILE* f = &fd;

tests/api.c

@@ -1702,6 +1702,16 @@ static void test_wolfSSH_SFTP_Confinement(void)
      * run. */
     char             escSymlink[] = "confine_symlink";
     char             escSymThru[WOLFSSH_MAX_FILENAME];
+    /* an in-jail symlink whose leaf points at an in-jail regular file.  The
+     * path resolves and stays inside the jail and the target is a real file.
+     * This block is guarded by WOLFSSH_HAVE_SYMLINK, so GetAndCleanPath's
+     * per-component link check is always present here and rejects the leaf
+     * symlink first; the assertion therefore verifies the end-to-end "open a
+     * symlinked file is refused" behavior.  The new RecvOpen lstat/O_NOFOLLOW
+     * handling is defense-in-depth behind that check, closing the TOCTOU window
+     * between the type check and the open. */
+    char             leafTarget[]  = "confine_leaf_target";
+    char             leafSymlink[] = "confine_leaf_symlink";
 #endif
     WFILE*           fp = NULL;
     int              snLen;
@@ -1792,6 +1802,14 @@ static void test_wolfSSH_SFTP_Confinement(void)
     /* stage an in-jail symlink pointing at the out-of-jail temp root */
     WREMOVE(NULL, escSymlink);
     AssertIntEQ(symlink(escRoot, escSymlink), 0);
+
+    /* stage an in-jail regular file and an in-jail symlink that targets it */
+    WREMOVE(NULL, leafTarget);
+    AssertIntEQ(WFOPEN(NULL, &fp, leafTarget, "wb"), 0);
+    AssertNotNull(fp);
+    WFCLOSE(NULL, fp);
+    WREMOVE(NULL, leafSymlink);
+    AssertIntEQ(symlink(leafTarget, leafSymlink), 0);
 #endif
 #endif
 
@@ -1954,6 +1972,32 @@ static void test_wolfSSH_SFTP_Confinement(void)
     AssertNotNull(ls);
     wolfSSH_SFTPNAME_list_free(ls);
     ls = NULL;
+
+    /* Positive control: opening the in-jail regular file directly must still
+     * succeed, proving the symlink rejection below does not also block plain
+     * regular-file opens. */
+    handleSz = WOLFSSH_MAX_HANDLE;
+    ret = wolfSSH_SFTP_Open(ssh, leafTarget, WOLFSSH_FXF_READ, NULL,
+            handle, &handleSz);
+    AssertIntEQ(ret, WS_SUCCESS);
+    AssertIntEQ(wolfSSH_SFTP_Close(ssh, handle, handleSz), WS_SUCCESS);
+    ls = wolfSSH_SFTP_LS(ssh, curDir);
+    AssertNotNull(ls);
+    wolfSSH_SFTPNAME_list_free(ls);
+    ls = NULL;
+
+    /* opening a leaf that is itself a symlink to an in-jail regular file must
+     * be rejected, not silently followed to the target.  On this build the
+     * GetAndCleanPath per-component check rejects it first; the RecvOpen
+     * lstat/O_NOFOLLOW path is the defense-in-depth backstop. */
+    handleSz = WOLFSSH_MAX_HANDLE;
+    ret = wolfSSH_SFTP_Open(ssh, leafSymlink, WOLFSSH_FXF_READ, NULL,
+            handle, &handleSz);
+    AssertIntNE(ret, WS_SUCCESS);
+    ls = wolfSSH_SFTP_LS(ssh, curDir);
+    AssertNotNull(ls);
+    wolfSSH_SFTPNAME_list_free(ls);
+    ls = NULL;
 #endif
 
     /* Positive case: a relative write op that resolves inside the jail must be
@@ -2005,6 +2049,8 @@ static void test_wolfSSH_SFTP_Confinement(void)
 #if !defined(WOLFSSH_ZEPHYR) && !defined(USE_WINDOWS_API)
 #ifdef WOLFSSH_HAVE_SYMLINK
     WREMOVE(NULL, escSymlink);
+    WREMOVE(NULL, leafSymlink);
+    WREMOVE(NULL, leafTarget);
 #endif
     WRMDIR(NULL, escRoot);
     /* escSibling is non-empty only if this run created it (see staging above),

wolfssh/port.h

@@ -1484,6 +1484,9 @@ extern "C" {
     #define WOLFSSH_O_CREAT  O_CREAT
     #define WOLFSSH_O_TRUNC  O_TRUNC
     #define WOLFSSH_O_EXCL   O_EXCL
+    #ifdef O_NOFOLLOW
+        #define WOLFSSH_O_NOFOLLOW O_NOFOLLOW
+    #endif
 
     #define WOPEN(fs,f,m,p) open((f),(m),(p))
     #define WCLOSE(fs,fd) close((fd))
@@ -1505,6 +1508,12 @@ extern "C" {
 #endif
 #endif /* WOLFSSH_SFTP or WOLFSSH_SCP */
 
+/* Platforms without an open(2)-style O_NOFOLLOW (or without a filesystem) get a
+ * harmless no-op so callers can OR it into the open flags unconditionally. */
+#ifndef WOLFSSH_O_NOFOLLOW
+    #define WOLFSSH_O_NOFOLLOW 0
+#endif
+
 /* SFTP path confinement must reject symbolic links because wolfSSH_RealPath
  * only normalizes the path string and does not resolve them.  Define
  * WOLFSSH_HAVE_SYMLINK on the filesystems that can actually store a link -