Bind SCP file timestamps to open descriptor

Author: yosuke-wolfssl

configure.ac

@@ -87,7 +87,7 @@ AC_ARG_WITH(wolfssl,
 )
 
 AC_CHECK_LIB([wolfssl],[wolfCrypt_Init],,[AC_MSG_ERROR([libwolfssl is required for ${PACKAGE}. It can be obtained from https://www.wolfssl.com/download.html/ .])])
-AC_CHECK_FUNCS([gethostbyname getaddrinfo gettimeofday inet_ntoa memset socket wc_ecc_set_rng])
+AC_CHECK_FUNCS([gethostbyname getaddrinfo gettimeofday inet_ntoa memset socket wc_ecc_set_rng futimes])
 AC_CHECK_DECLS([[pread],[pwrite]],,[unistd.h])
 
 # DEBUG

src/wolfscp.c

@@ -1857,14 +1857,32 @@ static INLINE int wolfSSH_LastError(void)
 }
 
 
+/* WOLFSSH_SCP_FD_UTIMES is defined for platforms that can set file timestamps
+ * on the still-open descriptor, binding the update to the inode. On those the
+ * timestamp is applied before the file is closed. Other platforms fall back to
+ * applying the timestamp by path after the file is closed. */
+#if defined(USE_WINDOWS_API) || defined(WFUTIMES)
+    #define WOLFSSH_SCP_FD_UTIMES
+#endif
+
 /* set file access and modification times
+ *
+ * On descriptor-capable platforms (fp != NULL and WOLFSSH_SCP_FD_UTIMES) the
+ * timestamps are applied to the open descriptor so the update is bound to the
+ * inode. This avoids a race where the path could be replaced by a symlink
+ * between closing the file and a path-based time update, which would let a
+ * peer-supplied timestamp be applied to an arbitrary target. Buffered file
+ * data is flushed first so the later close does not write and overwrite the
+ * modification time. When fp is NULL the file has already been closed and the
+ * timestamp is applied by path.
+ *
  * Returns WS_SUCCESS on success, or negative upon error */
-static int SetTimestampInfo(const char* fileName, word64 mTime, word64 aTime)
+static int SetTimestampInfo(WFILE* fp, const char* fileName,
+        word64 mTime, word64 aTime)
 {
     int ret = WS_SUCCESS;
 #ifdef USE_WINDOWS_API
     struct _utimbuf tmp;
-    int fd;
 #else
     struct timeval tmp[2];
 #endif
@@ -1874,18 +1892,33 @@ static int SetTimestampInfo(const char* fileName, word64 mTime, word64 aTime)
 
     if (ret == WS_SUCCESS) {
 #ifdef USE_WINDOWS_API
-        tmp.actime  = aTime;
-        tmp.modtime = mTime;
-        _sopen_s(&fd, fileName, _O_RDWR, _SH_DENYNO, 0);
-        _futime(fd, &tmp);
-        _close(fd);
+        if (fp == NULL) {
+            ret = WS_BAD_ARGUMENT;
+        }
+        else {
+            tmp.actime  = aTime;
+            tmp.modtime = mTime;
+            if (WFFLUSH(fp) != 0 || _futime(_fileno(fp), &tmp) != 0)
+                ret = WS_FATAL_ERROR;
+        }
 #else
         tmp[0].tv_sec = (time_t)aTime;
         tmp[0].tv_usec = 0;
         tmp[1].tv_sec = (time_t)mTime;
         tmp[1].tv_usec = 0;
 
-        ret = WUTIMES(fileName, tmp);
+    #ifdef WFUTIMES
+        if (fp != NULL) {
+            if (WFFLUSH(fp) != 0 || WFUTIMES(fileno(fp), tmp) != 0)
+                ret = WS_FATAL_ERROR;
+        }
+        else
+    #endif
+        {
+            (void)fp;
+            if (WUTIMES(fileName, tmp) != 0)
+                ret = WS_FATAL_ERROR;
+        }
 #endif
     }
 
@@ -2093,15 +2126,30 @@ int wsScpRecvCallback(WOLFSSH* ssh, int state, const char* basePath,
                 (void)WFFLUSH(fp);
                 (void)fsync(fileno(fp));
                 flush_bytes = 0;
+#endif
+#ifdef WOLFSSH_SCP_FD_UTIMES
+                /* set timestamp info on the open file, before closing, so the
+                 * update is bound to the inode and cannot be redirected to a
+                 * symlink swapped in over the path */
+                if (mTime != 0 || aTime != 0)
+                    ret = SetTimestampInfo(fp, fileName, mTime, aTime);
 #endif
                 WFCLOSE(ssh->fs, fp);
                 fp = NULL;
             }
 
             /* set timestamp info */
             if (mTime != 0 || aTime != 0) {
-                ret = SetTimestampInfo(fileName, mTime, aTime);
-
+#ifndef WOLFSSH_SCP_FD_UTIMES
+                /* no descriptor-based update available, set by path now that
+                 * the file is closed so the close cannot overwrite the time */
+                ret = SetTimestampInfo(NULL, fileName, mTime, aTime);
+#else
+                /* on descriptor-based platforms the timestamp was applied
+                 * above while the file was open; if fp was NULL here (file
+                 * never opened) we intentionally abort rather than fall back
+                 * to a path-based update that could follow a swapped symlink */
+#endif
                 if (ret == WS_SUCCESS) {
                     ret = WS_SCP_CONTINUE;
                 } else {

tests/unit.c

@@ -3932,6 +3932,132 @@ static int test_ScpRecvCallback_NewDirChdirFail(void)
     return result;
 }
 
+/* Drive the default SCP receive callback through a full single-file receive
+ * and confirm the peer-supplied modification/access times end up on the
+ * written file.
+ *
+ * Which code path applies the timestamp is decided at build time, not by this
+ * test: when futimes is detected (HAVE_FUTIMES) the callback sets it on the
+ * open descriptor before the closing flush, otherwise it sets it by path after
+ * close. Both must yield the peer-supplied times, which is what this end-to-end
+ * test asserts. When the descriptor path is compiled in, this also regresses
+ * its ordering relative to the closing flush: applying the timestamp before the
+ * buffered data was flushed would leave the modification time at the current
+ * time rather than the peer value. */
+static int test_ScpRecvCallback_Timestamp(void)
+{
+    char tmpDir[] = "/tmp/wolfssh_scptsXXXXXX";
+    char filePath[PATH_MAX];
+    char origCwd[PATH_MAX];
+    const char data[] = "wolfssh scp timestamp regression\n";
+    const word64 mTime = 1234567890; /* 2009-02-13 23:31:30 UTC */
+    const word64 aTime = 1000000000; /* 2001-09-09 01:46:40 UTC */
+    struct stat st;
+    WOLFSSH_CTX* ctx = NULL;
+    WOLFSSH* ssh = NULL;
+    char* basePath = NULL;
+    int origCwdSaved = 0;
+    int baseReady = 0;
+    int ret;
+    int result = 0;
+
+    filePath[0] = '\0';
+
+    if (getcwd(origCwd, sizeof(origCwd)) == NULL)
+        return -850;
+    origCwdSaved = 1;
+
+    if (mkdtemp(tmpDir) == NULL)
+        return -851;
+
+    basePath = realpath(tmpDir, NULL);
+    if (basePath == NULL) {
+        result = -852;
+        goto cleanup;
+    }
+    baseReady = 1;
+
+    ret = snprintf(filePath, sizeof(filePath), "%s/ts_file.txt", basePath);
+    if (!scpTestSnprintfOk(ret, sizeof(filePath))) {
+        result = -853;
+        goto cleanup;
+    }
+
+    ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL);
+    if (ctx == NULL) {
+        result = -854;
+        goto cleanup;
+    }
+    ssh = wolfSSH_new(ctx);
+    if (ssh == NULL) {
+        result = -855;
+        goto cleanup;
+    }
+
+    /* enter the destination directory */
+    ret = wsScpRecvCallback(ssh, WOLFSSH_SCP_NEW_REQUEST, basePath,
+            NULL, 0, 0, 0, 0, NULL, 0, 0, NULL);
+    if (ret != WS_SCP_CONTINUE) {
+        result = -856;
+        goto cleanup;
+    }
+
+    /* open the destination file */
+    ret = wsScpRecvCallback(ssh, WOLFSSH_SCP_NEW_FILE, basePath,
+            "ts_file.txt", 0644, mTime, aTime, sizeof(data) - 1, NULL, 0, 0,
+            NULL);
+    if (ret != WS_SCP_CONTINUE) {
+        result = -857;
+        goto cleanup;
+    }
+
+    /* write the file contents */
+    ret = wsScpRecvCallback(ssh, WOLFSSH_SCP_FILE_PART, basePath,
+            "ts_file.txt", 0644, mTime, aTime, sizeof(data) - 1,
+            (byte*)data, sizeof(data) - 1, 0, wolfSSH_GetScpRecvCtx(ssh));
+    if (ret != WS_SCP_CONTINUE) {
+        result = -858;
+        goto cleanup;
+    }
+
+    /* close the file and apply the peer-supplied timestamps */
+    ret = wsScpRecvCallback(ssh, WOLFSSH_SCP_FILE_DONE, basePath,
+            "ts_file.txt", 0644, mTime, aTime, sizeof(data) - 1, NULL, 0, 0,
+            wolfSSH_GetScpRecvCtx(ssh));
+    if (ret != WS_SCP_CONTINUE) {
+        result = -859;
+        goto cleanup;
+    }
+
+    /* the written file must carry the peer-supplied timestamps */
+    if (stat(filePath, &st) != 0) {
+        result = -860;
+        goto cleanup;
+    }
+    if ((word64)st.st_mtime != mTime) {
+        result = -861;
+        goto cleanup;
+    }
+    if ((word64)st.st_atime != aTime) {
+        result = -862;
+        goto cleanup;
+    }
+
+cleanup:
+    if (ssh != NULL)
+        wolfSSH_free(ssh);
+    if (ctx != NULL)
+        wolfSSH_CTX_free(ctx);
+    if (filePath[0] != '\0')
+        (void)remove(filePath);
+    free(basePath);
+    if (baseReady)
+        (void)rmdir(tmpDir);
+    if (origCwdSaved && chdir(origCwd) != 0 && result == 0)
+        result = -863;
+    return result;
+}
+
 #endif /* WOLFSSH_SCP recv callback depth guard test */
 
 
@@ -4552,6 +4678,11 @@ int wolfSSH_UnitTest(int argc, char** argv)
     printf("ScpRecvCallback_NewDirChdirFail: %s\n",
             (unitResult == 0 ? "SUCCESS" : "FAILED"));
     testResult = testResult || unitResult;
+
+    unitResult = test_ScpRecvCallback_Timestamp();
+    printf("ScpRecvCallback_Timestamp: %s\n",
+            (unitResult == 0 ? "SUCCESS" : "FAILED"));
+    testResult = testResult || unitResult;
 #endif
 
 #ifdef WOLFSSH_TEST_CAPTURING_ALLOCATOR

wolfssh/port.h

@@ -461,6 +461,11 @@ extern "C" {
         #include <sys/utime.h>
     #else
         #define WUTIMES(f,t)      utimes((f),(t))
+        /* futimes() is a BSD extension, not POSIX; only use it when the build
+         * detected it, otherwise fall back to the path-based WUTIMES */
+        #ifdef HAVE_FUTIMES
+            #define WFUTIMES(fd,t)    futimes((fd),(t))
+        #endif
     #endif
 
     #ifndef USE_WINDOWS_API