From 54dcd50ee87ae96974769a807f3808583369f716 Mon Sep 17 00:00:00 2001 From: Yosuke Shimizu Date: Tue, 16 Jun 2026 12:08:12 +0900 Subject: [PATCH] Bind SCP file timestamps to open descriptor --- configure.ac | 24 +++++ src/wolfscp.c | 84 ++++++++++++++-- tests/unit.c | 265 +++++++++++++++++++++++++++++++++++++++++++++++++ wolfssh/port.h | 41 ++++++++ 4 files changed, 405 insertions(+), 9 deletions(-) diff --git a/configure.ac b/configure.ac index 20f3c2238..07731f0b3 100644 --- a/configure.ac +++ b/configure.ac @@ -88,6 +88,30 @@ 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]) + +# futimens()/utimensat() declarations are feature-test-macro gated, so a plain +# AC_CHECK_FUNCS link test can pass while the prototype stays hidden at compile +# time. Probe through the real header so a positive result is build-safe. +AC_MSG_CHECKING([for futimens]) +AC_LINK_IFELSE( + [AC_LANG_PROGRAM([[#include +#include ]], + [[struct timespec ts[2]; (void)futimens(0, ts);]])], + [AC_DEFINE([HAVE_FUTIMENS], [1], [futimens() available and declared]) + AC_MSG_RESULT([yes])], + [AC_MSG_RESULT([no])]) + +AC_MSG_CHECKING([for utimensat]) +AC_LINK_IFELSE( + [AC_LANG_PROGRAM([[#include +#include +#include ]], + [[struct timespec ts[2]; + (void)utimensat(AT_FDCWD, "x", ts, AT_SYMLINK_NOFOLLOW);]])], + [AC_DEFINE([HAVE_UTIMENSAT], [1], [utimensat() available and declared]) + AC_MSG_RESULT([yes])], + [AC_MSG_RESULT([no])]) + AC_CHECK_DECLS([[pread],[pwrite]],,[unistd.h]) # DEBUG diff --git a/src/wolfscp.c b/src/wolfscp.c index 2113411a2..823e853fb 100644 --- a/src/wolfscp.c +++ b/src/wolfscp.c @@ -2059,9 +2059,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. On the POSIX path-based fallback, fp is NULL because the + * file has already been closed and the timestamp is applied by path; that path + * update uses utimensat() with AT_SYMLINK_NOFOLLOW when available so a swapped + * symlink is not followed, and only drops to plain utimes() otherwise. The + * Windows branch always requires an open fp and rejects fp == NULL with + * WS_BAD_ARGUMENT (there is no path-based fallback there). + * * 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 @@ -2076,18 +2099,43 @@ 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 { + fd = _fileno(fp); + tmp.actime = aTime; + tmp.modtime = mTime; + /* commit buffered data to disk before stamping so the close cannot + * flush a write that bumps the modification time back */ + if (WFFLUSH(fp) != 0 || _commit(fd) != 0 || _futime(fd, &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; + /* no open descriptor to bind to; prefer the no-follow path update + * so a swapped symlink is not followed, falling back to plain + * utimes() only when utimensat() is unavailable */ + #ifdef WUTIMES_NOFOLLOW + if (WUTIMES_NOFOLLOW(fileName, tmp) != 0) + #else + if (WUTIMES(fileName, tmp) != 0) + #endif + ret = WS_FATAL_ERROR; + } #endif } @@ -2306,15 +2354,33 @@ 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; } +#ifdef WOLFSSH_SCP_FD_UTIMES + else if (mTime != 0 || aTime != 0) { + /* descriptor-based update required but the file was never + * opened; do not fall back to a path update that could follow + * a swapped symlink, fail so the handling below aborts */ + ret = WS_FATAL_ERROR; + } +#endif /* 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); +#endif if (ret == WS_SUCCESS) { ret = WS_SCP_CONTINUE; } else { diff --git a/tests/unit.c b/tests/unit.c index d7e4d1675..09a1b81ff 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -73,6 +73,7 @@ #include #include #include +#include #include #include #endif @@ -4353,6 +4354,258 @@ static int test_ScpRecvCallback_SymlinkGuard(void) #endif /* WOLFSSH_HAVE_SYMLINK */ } +/* 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 futimens is detected (HAVE_FUTIMENS) 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 guards 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; + baseReady = 1; + + basePath = realpath(tmpDir, NULL); + if (basePath == NULL) { + result = -852; + goto cleanup; + } + + 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; + } + +#ifdef HAVE_FUTIMENS + /* Descriptor build only: a FILE_DONE carrying timestamps but no open file + * (NULL ctx) must abort rather than silently fall back to a path-based + * update that could follow a swapped symlink. Use distinct times and + * confirm the existing file's modification time is left untouched. */ + ret = wsScpRecvCallback(ssh, WOLFSSH_SCP_FILE_DONE, basePath, + "ts_file.txt", 0644, mTime + 100, aTime + 100, 0, NULL, 0, 0, + NULL); + if (ret != WS_SCP_ABORT) { + result = -864; + goto cleanup; + } + if (stat(filePath, &st) != 0) { + result = -865; + goto cleanup; + } + if ((word64)st.st_mtime != mTime) { + result = -866; + goto cleanup; + } +#endif + +cleanup: + if (ssh != NULL) + wolfSSH_free(ssh); + if (ctx != NULL) + wolfSSH_CTX_free(ctx); + if (filePath[0] != '\0') + (void)remove(filePath); + free(basePath); + if (origCwdSaved && chdir(origCwd) != 0 && result == 0) + result = -863; + if (baseReady) + (void)rmdir(tmpDir); + return result; +} + +#if defined(HAVE_UTIMENSAT) && defined(WOLFSSH_HAVE_SYMLINK) +/* Exercise the no-follow path fallback (no descriptor-based call available). + * Setting times through a symlink must land on the symlink itself, not the + * target, so a swapped symlink cannot redirect a peer-supplied timestamp. */ +static int test_ScpTimestamp_NoFollow(void) +{ + char tmpDir[] = "/tmp/wolfssh_scpnfXXXXXX"; + char canaryPath[PATH_MAX]; + char linkPath[PATH_MAX]; + const word64 mTime = 1222333444; /* distinct from the canary's own time */ + const word64 aTime = 1100000000; + struct timeval tv[2]; + struct stat st; + time_t canaryOrig; + int baseReady = 0; + int canaryReady = 0; + int linkReady = 0; + int ret; + int result = 0; + int fd; + + canaryPath[0] = '\0'; + linkPath[0] = '\0'; + + if (mkdtemp(tmpDir) == NULL) + return -870; + baseReady = 1; + + ret = snprintf(canaryPath, sizeof(canaryPath), "%s/canary.txt", tmpDir); + if (!scpTestSnprintfOk(ret, sizeof(canaryPath))) { + result = -871; + goto cleanup; + } + ret = snprintf(linkPath, sizeof(linkPath), "%s/link", tmpDir); + if (!scpTestSnprintfOk(ret, sizeof(linkPath))) { + result = -872; + goto cleanup; + } + + fd = open(canaryPath, O_CREAT | O_WRONLY | O_TRUNC, 0644); + if (fd < 0) { + result = -873; + goto cleanup; + } + canaryReady = 1; + if (write(fd, "canary\n", 7) != 7) { + close(fd); + result = -881; + goto cleanup; + } + close(fd); + + if (stat(canaryPath, &st) != 0) { + result = -874; + goto cleanup; + } + canaryOrig = st.st_mtime; + + if (symlink(canaryPath, linkPath) != 0) { + result = -875; + goto cleanup; + } + linkReady = 1; + + tv[0].tv_sec = (time_t)aTime; + tv[0].tv_usec = 0; + tv[1].tv_sec = (time_t)mTime; + tv[1].tv_usec = 0; + if (WUTIMES_NOFOLLOW(linkPath, tv) != 0) { + result = -876; + goto cleanup; + } + + /* the symlink's own modification time must carry the supplied value */ + if (lstat(linkPath, &st) != 0) { + result = -877; + goto cleanup; + } + if ((word64)st.st_mtime != mTime) { + result = -878; + goto cleanup; + } + + /* the target the symlink points at must be left untouched */ + if (stat(canaryPath, &st) != 0) { + result = -879; + goto cleanup; + } + if (st.st_mtime != canaryOrig) { + result = -880; + goto cleanup; + } + +cleanup: + if (linkReady) + (void)remove(linkPath); + if (canaryReady) + (void)remove(canaryPath); + if (baseReady) + (void)rmdir(tmpDir); + return result; +} +#endif /* HAVE_UTIMENSAT && WOLFSSH_HAVE_SYMLINK */ + #endif /* WOLFSSH_SCP recv callback depth guard test */ @@ -5338,6 +5591,18 @@ int wolfSSH_UnitTest(int argc, char** argv) printf("ScpRecvCallback_SymlinkGuard: %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; + +#if defined(HAVE_UTIMENSAT) && defined(WOLFSSH_HAVE_SYMLINK) + unitResult = test_ScpTimestamp_NoFollow(); + printf("ScpTimestamp_NoFollow: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif #endif #if defined(WOLFSSH_TEST_INTERNAL) && defined(WOLFSSH_SCP) && \ diff --git a/wolfssh/port.h b/wolfssh/port.h index 9661bf3e9..983818378 100644 --- a/wolfssh/port.h +++ b/wolfssh/port.h @@ -461,6 +461,47 @@ extern "C" { #include #else #define WUTIMES(f,t) utimes((f),(t)) + /* Bind timestamp updates to the open descriptor with futimens() + * (POSIX.1-2008) when the build detected it, immune to a symlink + * swapped in over the path. The SCP code passes a timeval pair + * (matching WUTIMES), so convert it to the timespec pair futimens() + * expects. Falls back to the path-based WUTIMES when futimens() is not + * present. */ + #ifdef HAVE_FUTIMENS + #include + #include + #include + static inline int wFutimes(int fd, const struct timeval t[2]) + { + struct timespec ts[2]; + ts[0].tv_sec = t[0].tv_sec; + ts[0].tv_nsec = (long)t[0].tv_usec * 1000; + ts[1].tv_sec = t[1].tv_sec; + ts[1].tv_nsec = (long)t[1].tv_usec * 1000; + return futimens(fd, ts); + } + #define WFUTIMES(fd,t) wFutimes((fd),(t)) + #endif + /* Path-based fallback for builds without futimens(): utimensat() with + * AT_SYMLINK_NOFOLLOW so a swapped symlink is not followed. Converts + * the timeval pair (matching WUTIMES) to the timespec utimensat wants. */ + #ifdef HAVE_UTIMENSAT + #include + #include + #include + #include + static inline int wUtimesNoFollow(const char* f, + const struct timeval t[2]) + { + struct timespec ts[2]; + ts[0].tv_sec = t[0].tv_sec; + ts[0].tv_nsec = (long)t[0].tv_usec * 1000; + ts[1].tv_sec = t[1].tv_sec; + ts[1].tv_nsec = (long)t[1].tv_usec * 1000; + return utimensat(AT_FDCWD, f, ts, AT_SYMLINK_NOFOLLOW); + } + #define WUTIMES_NOFOLLOW(f,t) wUtimesNoFollow((f),(t)) + #endif #endif #ifndef USE_WINDOWS_API