sftp: refuse dir REMOVE and replacing RENAME

SSH_FXP_REMOVE does not take directories and SSH_FXP_RENAME fails on an
existing newpath (draft-ietf-secsh-filexfer-02 6.5). The non-Windows
handlers now check the target with an lstat first, and REMOVE unlinks.
A regular file renames by linkat() and unlink(), which refuses an
existing name atomically; anything else checks then calls rename().

- configure: probe for linkat()
- port.h: WUNLINK on POSIX, WLINK as linkat() when configure finds it
- FatFs, Nucleus: only "/" and "/." stand in for the root
- regress: test both against files, directories, symlinks and a
  read-only directory

Issue: F-15535, F-15536

Author: ejohnstown

configure.ac

@@ -148,6 +148,15 @@ AC_LINK_IFELSE(
    AC_MSG_RESULT([yes])],
   [AC_MSG_RESULT([no])])
 
+AC_MSG_CHECKING([for linkat])
+AC_LINK_IFELSE(
+  [AC_LANG_PROGRAM([[#include <fcntl.h>
+#include <unistd.h>]],
+                   [[(void)linkat(AT_FDCWD, "x", AT_FDCWD, "y", 0);]])],
+  [AC_DEFINE([HAVE_LINKAT], [1], [linkat() available and declared])
+   AC_MSG_RESULT([yes])],
+  [AC_MSG_RESULT([no])])
+
 AC_CHECK_DECLS([[pread],[pwrite]],,[unistd.h])
 
 # DEBUG

src/wolfsftp.c

@@ -4981,6 +4981,9 @@ int wolfSSH_SFTP_RecvRemove(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
     char   name[WOLFSSH_MAX_FILENAME];
     word32 idx = 0;
     int    ret = WS_SUCCESS;
+#ifndef USE_WINDOWS_API
+    WS_SFTP_FILEATRB atr;
+#endif
 
     byte type = WOLFSSH_FTP_OK;
     int  rc;
@@ -5006,7 +5009,20 @@ int wolfSSH_SFTP_RecvRemove(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
 
     if (ret == WS_SUCCESS) {
     #ifndef USE_WINDOWS_API
-        if (WREMOVE(ssh->fs, name) < 0)
+        /* REMOVE never takes a directory (draft-ietf-secsh-filexfer-02 6.5) */
+        WMEMSET(&atr, 0, sizeof(atr));
+        if (SFTP_GetAttributes(ssh->fs, name, &atr, 1, ssh->ctx->heap)
+                    == WS_SUCCESS
+                && (atr.per & FILEATRB_PER_MASK_TYPE) == FILEATRB_PER_DIR) {
+            WLOG(WS_LOG_SFTP, "Not removing a directory");
+            ret = WS_BAD_FILE_E;
+        }
+    #ifdef WUNLINK
+        /* unlink() refuses a directory swapped in after the check */
+        else if (WUNLINK(ssh->fs, name) < 0)
+    #else
+        else if (WREMOVE(ssh->fs, name) < 0)
+    #endif
     #else /* USE_WINDOWS_API */
         if (WS_DeleteFileA(name, ssh->ctx->heap) == 0)
     #endif /* USE_WINDOWS_API */
@@ -5050,6 +5066,52 @@ int wolfSSH_SFTP_RecvRemove(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
 }
 
 
+#ifndef USE_WINDOWS_API
+/* Rename without replacing an existing name (draft-ietf-secsh-filexfer-02
+ * 6.5). linkat() refuses one atomically; other types, builds without linkat(),
+ * and filesystems without hard links check first and race a concurrent
+ * create.
+ *
+ * returns WS_SUCCESS on success
+ */
+static int SFTP_RenameNoReplace(WOLFSSH* ssh, char* old, char* name)
+{
+    WS_SFTP_FILEATRB atr;
+
+#ifdef WLINK
+    WMEMSET(&atr, 0, sizeof(atr));
+    if (SFTP_GetAttributes(ssh->fs, old, &atr, 1, ssh->ctx->heap)
+                == WS_SUCCESS
+            && (atr.per & FILEATRB_PER_MASK_TYPE) == FILEATRB_PER_FILE) {
+        if (WLINK(ssh->fs, old, name) == 0) {
+            if (WUNLINK(ssh->fs, old) == 0) {
+                return WS_SUCCESS;
+            }
+            /* undo the link */
+            (void)WUNLINK(ssh->fs, name);
+            return WS_BAD_FILE_E;
+        }
+        if (errno == EEXIST) {
+            WLOG(WS_LOG_SFTP, "Rename target exists");
+            return WS_BAD_FILE_E;
+        }
+    }
+#endif
+
+    WMEMSET(&atr, 0, sizeof(atr));
+    if (SFTP_GetAttributes(ssh->fs, name, &atr, 1, ssh->ctx->heap)
+            == WS_SUCCESS) {
+        WLOG(WS_LOG_SFTP, "Rename target exists");
+        return WS_BAD_FILE_E;
+    }
+    if (WRENAME(ssh->fs, old, name) < 0) {
+        return WS_BAD_FILE_E;
+    }
+    return WS_SUCCESS;
+}
+#endif /* USE_WINDOWS_API */
+
+
 /* Handles packet to rename a file
  *
  * returns WS_SUCCESS on success
@@ -5098,8 +5160,9 @@ int wolfSSH_SFTP_RecvRename(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
 
     if (ret == WS_SUCCESS) {
     #ifndef USE_WINDOWS_API
-        if (WRENAME(ssh->fs, old, name) < 0)
+        if (SFTP_RenameNoReplace(ssh, old, name) != WS_SUCCESS)
     #else /* USE_WINDOWS_API */
+        /* MoveFile never replaces */
         if (WS_MoveFileA(old, name, ssh->ctx->heap) == 0)
     #endif /* USE_WINDOWS_API */
         {
@@ -5212,8 +5275,10 @@ static int SFTP_GetAttributes(void* fs, const char* fileName,
         return WS_SUCCESS;
     }
 
-    /* handle case of "/" */
-    if (sz < 3 && fileName[0] == WS_DELIM && ret == NUF_NOFILE) {
+    /* handle case of "/" or "/.", not a root-level name such as "/a" */
+    if (fileName[0] == WS_DELIM
+            && (sz == 1 || (sz == 2 && fileName[1] == '.'))
+            && ret == NUF_NOFILE) {
         atr->flags |= WOLFSSH_FILEATRB_PERM;
         atr->per |= 040000;
         return WS_SUCCESS;
@@ -5597,8 +5662,10 @@ static int SFTP_GetAttributes(void* fs, const char* fileName,
     }
 
     /* handle case of "/" */
-    /* Calling f_stat for "/" returns FR_INVALID_NAME. So we simulate the result. */
-    if (sz < 3 && fileName[0] == WS_DELIM) {
+    /* Calling f_stat for "/" returns FR_INVALID_NAME. So we simulate the result.
+     * Only "/" and "/." though; "/a" is a root-level name for f_stat. */
+    if (fileName[0] == WS_DELIM
+            && (sz == 1 || (sz == 2 && fileName[1] == '.'))) {
         atr->flags |= WOLFSSH_FILEATRB_PERM;
         atr->per |= 040000;
         return WS_SUCCESS;

tests/regress.c

@@ -16021,6 +16021,167 @@ static void TestSftpAppendWritesAtEof(void)
     wolfSSH_free(ssh);
     wolfSSH_CTX_free(ctx);
 }
+
+/* FXP_RENAME path to newPath, or FXP_REMOVE path when newPath is NULL. */
+static int SftpPathRequest(WOLFSSH* ssh, int reqId, const char* path,
+        const char* newPath)
+{
+    byte   pkt[2 * (WOLFSSH_MAX_FILENAME + UINT32_SZ)];
+    word32 idx = 0;
+    word32 sz = (word32)WSTRLEN(path);
+
+    SftpPutU32(sz, pkt + idx); idx += UINT32_SZ;
+    WMEMCPY(pkt + idx, path, sz); idx += sz;
+    if (newPath == NULL) {
+        return wolfSSH_SFTP_RecvRemove(ssh, reqId, pkt, idx);
+    }
+    sz = (word32)WSTRLEN(newPath);
+    SftpPutU32(sz, pkt + idx); idx += UINT32_SZ;
+    WMEMCPY(pkt + idx, newPath, sz); idx += sz;
+    return wolfSSH_SFTP_RecvRename(ssh, reqId, pkt, idx);
+}
+
+static void SftpWriteTestFile(const char* path, const char* content)
+{
+    WFILE* file;
+
+    AssertIntEQ(WFOPEN(NULL, &file, path, "wb"), 0);
+    AssertTrue(file != WBADFILE);
+    AssertIntEQ((int)WFWRITE(NULL, content, 1, WSTRLEN(content), file),
+            (int)WSTRLEN(content));
+    WFCLOSE(NULL, file);
+}
+
+/* REMOVE refuses a directory, RENAME an existing newpath
+ * (draft-ietf-secsh-filexfer-02 6.5). */
+static void TestSftpRemoveRenameSemantics(void)
+{
+    WOLFSSH_CTX* ctx;
+    WOLFSSH* ssh;
+    int rid = 700;
+    WSTAT_T st;
+    char cwd[WOLFSSH_MAX_FILENAME];
+    char fileA[64], fileB[64], fileC[64];
+    char dirA[64], dirB[64], dirC[64];
+#if defined(WOLFSSH_HAVE_SYMLINK) && !defined(USE_WINDOWS_API)
+    char lnk[64];
+#endif
+#ifndef USE_WINDOWS_API
+    char roDir[64], roFile[128], roDest[128];
+#endif
+
+    ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL);
+    AssertNotNull(ctx);
+    ssh = wolfSSH_new(ctx);
+    AssertNotNull(ssh);
+    AssertIntEQ(wolfSSH_SFTP_TestRecvStateInit(ssh), WS_SUCCESS);
+
+    WSNPRINTF(fileA, sizeof(fileA), "wolfssh_rm_a_%lu.tmp", SFTP_TEST_PID());
+    WSNPRINTF(fileB, sizeof(fileB), "wolfssh_rm_b_%lu.tmp", SFTP_TEST_PID());
+    WSNPRINTF(fileC, sizeof(fileC), "wolfssh_rm_c_%lu.tmp", SFTP_TEST_PID());
+    WSNPRINTF(dirA, sizeof(dirA), "wolfssh_rm_da_%lu", SFTP_TEST_PID());
+    WSNPRINTF(dirB, sizeof(dirB), "wolfssh_rm_db_%lu", SFTP_TEST_PID());
+    WSNPRINTF(dirC, sizeof(dirC), "wolfssh_rm_dc_%lu", SFTP_TEST_PID());
+    WMEMSET(cwd, 0, sizeof(cwd));
+    AssertNotNull(WGETCWD(ssh->fs, cwd, sizeof(cwd) - 1));
+    AssertIntEQ(wolfSSH_SFTP_SetDefaultPath(ssh, cwd), WS_SUCCESS);
+
+    SftpWriteTestFile(fileA, "aaaa");
+    SftpWriteTestFile(fileB, "bb");
+    (void)WREMOVE(ssh->fs, fileC);
+    AssertIntEQ(WMKDIR(ssh->fs, dirA, 0755), 0);
+    AssertIntEQ(WMKDIR(ssh->fs, dirB, 0755), 0);
+    (void)WRMDIR(ssh->fs, dirC);
+
+    /* REMOVE refuses an empty directory */
+    AssertIntEQ(SftpPathRequest(ssh, rid, dirA, NULL), WS_BAD_FILE_E);
+    AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_FAILURE);
+    AssertIntEQ(WSTAT(ssh->fs, dirA, &st), 0);
+
+    /* RENAME refuses an existing file; neither changes */
+    AssertIntEQ(SftpPathRequest(ssh, rid, fileA, fileB), WS_BAD_FILE_E);
+    AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_FAILURE);
+    AssertSftpFileContents(fileA, "aaaa", 4);
+    AssertSftpFileContents(fileB, "bb", 2);
+
+    /* RENAME refuses an existing empty directory */
+    AssertIntEQ(SftpPathRequest(ssh, rid, dirA, dirB), WS_BAD_FILE_E);
+    AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_FAILURE);
+    AssertIntEQ(WSTAT(ssh->fs, dirA, &st), 0);
+    AssertIntEQ(WSTAT(ssh->fs, dirB, &st), 0);
+
+#if defined(WOLFSSH_HAVE_SYMLINK) && !defined(USE_WINDOWS_API)
+    /* a dangling symlink is an existing newpath too */
+    WSNPRINTF(lnk, sizeof(lnk), "wolfssh_rm_ln_%lu", SFTP_TEST_PID());
+    (void)WREMOVE(ssh->fs, lnk);
+    AssertIntEQ(symlink(fileC, lnk), 0);
+    AssertIntEQ(SftpPathRequest(ssh, rid, fileA, lnk), WS_BAD_FILE_E);
+    AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_FAILURE);
+    AssertIntEQ(WLSTAT(ssh->fs, lnk, &st), 0);
+    AssertTrue(S_ISLNK(st.st_mode));
+    AssertSftpFileContents(fileA, "aaaa", 4);
+    /* REMOVE takes the link, not its target */
+    AssertIntEQ(SftpPathRequest(ssh, rid, lnk, NULL), WS_SUCCESS);
+    AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_OK);
+    AssertTrue(WLSTAT(ssh->fs, lnk, &st) != 0);
+    /* a link to a directory is not a directory to REMOVE */
+    AssertIntEQ(symlink(dirB, lnk), 0);
+    AssertIntEQ(SftpPathRequest(ssh, rid, lnk, NULL), WS_SUCCESS);
+    AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_OK);
+    AssertTrue(WLSTAT(ssh->fs, lnk, &st) != 0);
+    AssertIntEQ(WSTAT(ssh->fs, dirB, &st), 0);
+#endif
+
+#ifndef USE_WINDOWS_API
+    /* a failed move leaves both names as they were: out of a read-only
+     * directory takes the link rollback, into one the link fallback */
+    if (geteuid() != 0) {
+        WSNPRINTF(roDir, sizeof(roDir), "wolfssh_rm_ro_%lu",
+                SFTP_TEST_PID());
+        WSNPRINTF(roFile, sizeof(roFile), "%s/%s", roDir, fileC);
+        WSNPRINTF(roDest, sizeof(roDest), "%s/%s", roDir, fileB);
+        AssertIntEQ(WMKDIR(ssh->fs, roDir, 0755), 0);
+        SftpWriteTestFile(roFile, "cc");
+        AssertIntEQ(chmod(roDir, 0555), 0);
+
+        AssertIntEQ(SftpPathRequest(ssh, rid, roFile, fileC), WS_BAD_FILE_E);
+        AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_FAILURE);
+        AssertSftpFileContents(roFile, "cc", 2);
+        AssertTrue(WLSTAT(ssh->fs, fileC, &st) != 0);
+
+        AssertIntEQ(SftpPathRequest(ssh, rid, fileB, roDest), WS_BAD_FILE_E);
+        AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_FAILURE);
+        AssertSftpFileContents(fileB, "bb", 2);
+        AssertTrue(WLSTAT(ssh->fs, roDest, &st) != 0);
+
+        AssertIntEQ(chmod(roDir, 0755), 0);
+        AssertIntEQ(WREMOVE(ssh->fs, roFile), 0);
+        AssertIntEQ(WRMDIR(ssh->fs, roDir), 0);
+    }
+#endif
+
+    /* RENAME to a free name moves a file and a directory */
+    AssertIntEQ(SftpPathRequest(ssh, rid, fileA, fileC), WS_SUCCESS);
+    AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_OK);
+    AssertTrue(WSTAT(ssh->fs, fileA, &st) != 0);
+    AssertSftpFileContents(fileC, "aaaa", 4);
+    AssertIntEQ(SftpPathRequest(ssh, rid, dirA, dirC), WS_SUCCESS);
+    AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_OK);
+    AssertTrue(WSTAT(ssh->fs, dirA, &st) != 0);
+    AssertIntEQ(WSTAT(ssh->fs, dirC, &st), 0);
+
+    /* REMOVE still removes a file */
+    AssertIntEQ(SftpPathRequest(ssh, rid, fileC, NULL), WS_SUCCESS);
+    AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_OK);
+    AssertTrue(WSTAT(ssh->fs, fileC, &st) != 0);
+
+    (void)WREMOVE(ssh->fs, fileB);
+    (void)WRMDIR(ssh->fs, dirB);
+    (void)WRMDIR(ssh->fs, dirC);
+    wolfSSH_SFTP_TestRecvStateFree(ssh);
+    wolfSSH_free(ssh);
+    wolfSSH_CTX_free(ctx);
+}
 #endif /* !NO_WOLFSSH_SERVER && !NO_FILESYSTEM */
 
 #if !defined(NO_WOLFSSH_SERVER) && !defined(USE_WINDOWS_API) && \
@@ -20771,6 +20932,8 @@ int main(int argc, char** argv)
     #if !defined(NO_WOLFSSH_SERVER) && !defined(NO_FILESYSTEM)
     /* FXF_APPEND writes land at EOF regardless of the request offset */
     TestSftpAppendWritesAtEof();
+    /* REMOVE refuses directories, RENAME refuses an existing newpath */
+    TestSftpRemoveRenameSemantics();
     #endif
     #if !defined(NO_WOLFSSH_SERVER) && defined(USE_WINDOWS_API) && \
             !defined(NO_FILESYSTEM)

wolfssh/port.h

@@ -1613,7 +1613,7 @@ extern "C" {
 
 #else
 
-    #include <unistd.h>   /* used for rmdir */
+    #include <unistd.h>   /* used for rmdir, linkat, and unlink */
     #include <sys/stat.h> /* used for mkdir, stat, and lstat */
     #include <stdio.h>    /* used for remove and rename */
 
@@ -1622,6 +1622,15 @@ extern "C" {
     #define WSTAT(fs,p,b)   stat((p),(b))
     #ifndef USE_OSE_API
         #define WLSTAT(fs,p,b) lstat((p),(b))
+        #define WUNLINK(fs,p)  unlink((p))
+        #ifdef HAVE_LINKAT
+            /* SFTP no-replace rename; it reads errno. linkat() without
+             * AT_SYMLINK_FOLLOW links a symlink itself, where link() may
+             * follow it. */
+            #include <errno.h>
+            #include <fcntl.h>
+            #define WLINK(fs,o,n) linkat(AT_FDCWD, (o), AT_FDCWD, (n), 0)
+        #endif
     #else
         #define WLSTAT(fs,p,b) stat((p),(b))
     #endif