diff --git a/src/shared/file.c b/src/shared/file.c index 9514e87..c7e357c 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -183,15 +183,29 @@ bool file_stat_secure(const char* path, struct stat* st) { int parent_fd = file_open_secure_parent(path, &leaf, false); if (parent_fd < 0) return false; - int fd = openat(parent_fd, leaf, O_RDONLY | O_NONBLOCK | O_CLOEXEC | O_NOFOLLOW); - bool exists = fd >= 0 && fstat(fd, st) == 0 && S_ISREG(st->st_mode); - if (fd >= 0) - close(fd); + bool exists = fstatat(parent_fd, leaf, st, AT_SYMLINK_NOFOLLOW) == 0 && S_ISREG(st->st_mode); close(parent_fd); free(leaf); return exists; } +static bool stat_is_newer(const struct stat* st, const FileMetadata* metadata) { + if (!st || !metadata) + return false; +#ifdef __linux__ + long mtime_nsec = st->st_mtim.tv_nsec; +#else + long mtime_nsec = 0; +#endif + return st->st_mtime > metadata->mtime_sec || + (st->st_mtime == metadata->mtime_sec && mtime_nsec > metadata->mtime_nsec); +} + +bool file_destination_is_newer_secure(const char* path, const FileMetadata* metadata) { + struct stat st; + return file_stat_secure(path, &st) && stat_is_newer(&st, metadata); +} + int file_open_secure_parent(const char* path, char** leaf_out, bool create_dirs) { char* copy = str_dup(path); if (!copy) @@ -302,8 +316,9 @@ bool file_rename_secure(const char* old_path, const char* new_path) { return ok; } -bool file_to_disk_secure(const char* path, const void* data, unsigned long long data_size, - bool inplace, bool sparse, const FileMetadata* metadata) { +static bool file_to_disk_secure_impl(const char* path, const void* data, + unsigned long long data_size, bool inplace, bool sparse, + const FileMetadata* metadata, bool update) { char* leaf = NULL; int dirfd = file_open_secure_parent(path, &leaf, true); if (dirfd < 0) @@ -311,15 +326,37 @@ bool file_to_disk_secure(const char* path, const void* data, unsigned long long int fd = -1; bool ok = false; if (inplace) { - fd = openat(dirfd, leaf, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC | O_NOFOLLOW, 0644); + fd = openat(dirfd, leaf, O_WRONLY | O_CREAT | O_CLOEXEC | O_NOFOLLOW, 0644); if (fd >= 0) { - if (!sparse || data_size == 0 || ftruncate(fd, (off_t)data_size) == 0) - ok = write_all(fd, data, data_size); - if (ok && metadata) - ok = file_restore_metadata_fd(fd, metadata); + struct stat destination_stat; + bool newer = false; + if (update && metadata && fstat(fd, &destination_stat) == 0 && + S_ISREG(destination_stat.st_mode)) { + newer = stat_is_newer(&destination_stat, metadata); + } + if (newer) { + ok = true; + } else { + if (ftruncate(fd, 0) == 0 && + (!sparse || data_size == 0 || ftruncate(fd, (off_t)data_size) == 0)) + ok = write_all(fd, data, data_size); + if (ok && metadata) + ok = file_restore_metadata_fd(fd, metadata); + } } } else { char tmp[NAME_MAX]; + if (update && metadata) { + /* This check protects the normal atomic path as far as possible. A + concurrent replacement can still occur before the final rename. */ + struct stat destination_stat; + if (fstatat(dirfd, leaf, &destination_stat, AT_SYMLINK_NOFOLLOW) == 0 && + S_ISREG(destination_stat.st_mode) && stat_is_newer(&destination_stat, metadata)) { + close(dirfd); + free(leaf); + return true; + } + } for (unsigned int i = 0; i < 100 && !ok; ++i) { snprintf(tmp, sizeof(tmp), ".%s.tmp.%ld.%u", leaf, (long)getpid(), i); fd = openat(dirfd, tmp, O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC | O_NOFOLLOW, 0600); @@ -347,6 +384,16 @@ bool file_to_disk_secure(const char* path, const void* data, unsigned long long return ok; } +bool file_to_disk_secure(const char* path, const void* data, unsigned long long data_size, + bool inplace, bool sparse, const FileMetadata* metadata) { + return file_to_disk_secure_impl(path, data, data_size, inplace, sparse, metadata, false); +} + +bool file_to_disk_secure_update(const char* path, const void* data, unsigned long long data_size, + bool inplace, bool sparse, const FileMetadata* metadata) { + return file_to_disk_secure_impl(path, data, data_size, inplace, sparse, metadata, true); +} + bool file_write_to_disk(const char* path, const void* data, unsigned long long data_size, bool inplace, bool sparse) { if (!path || (!data && data_size != 0) || has_path_traversal(path)) diff --git a/src/shared/file.h b/src/shared/file.h index 23dac26..67a471c 100644 --- a/src/shared/file.h +++ b/src/shared/file.h @@ -27,10 +27,16 @@ bool file_set_authorized_root(int fd, const char* canonical_path); /* Secure path/filesystem primitives (symlink-safe, O_NOFOLLOW, root-confined). */ bool file_path_exists_secure(const char* path); bool file_stat_secure(const char* path, struct stat* st); +bool file_destination_is_newer_secure(const char* path, const FileMetadata* metadata); int file_open_secure_parent(const char* path, char** leaf_out, bool create_dirs); bool file_ensure_directory_secure(const char* path); bool file_rename_secure(const char* old_path, const char* new_path); bool file_to_disk_secure(const char* path, const void* data, unsigned long long data_size, bool inplace, bool sparse, const FileMetadata* metadata); +/* With update enabled, an existing newer destination is left untouched. The + check is descriptor-based for inplace writes; atomic replacement still has + an unavoidable final rename race without filesystem locking. */ +bool file_to_disk_secure_update(const char* path, const void* data, unsigned long long data_size, + bool inplace, bool sparse, const FileMetadata* metadata); #endif diff --git a/src/shared/file_receive.c b/src/shared/file_receive.c index 93edb1e..a51f39d 100644 --- a/src/shared/file_receive.c +++ b/src/shared/file_receive.c @@ -21,17 +21,6 @@ #define MAX_SERVER_DELETE_COUNT 100000U #define MAX_FILE_DATA_SIZE MAX_RECEIVE_FILE_SIZE -static bool destination_is_newer(const char* path, const FileMetadata* source_metadata) { - struct stat destination_stat; - if (!source_metadata || !file_stat_secure(path, &destination_stat)) - return false; - time_t destination_sec = destination_stat.st_mtim.tv_sec; - long destination_nsec = destination_stat.st_mtim.tv_nsec; - return destination_sec > source_metadata->mtime_sec || - (destination_sec == source_metadata->mtime_sec && - destination_nsec > source_metadata->mtime_nsec); -} - bool file_save_to_disk(const char* root_directory, const File* file, const Config* config) { bool backup_enabled = config && config->backup; bool inplace = config && config->inplace; @@ -72,8 +61,9 @@ bool file_save_to_disk(const char* root_directory, const File* file, const Confi return false; } - /* --update is receiver-side policy: never replace a newer destination. */ - if (config && config->update && destination_is_newer(disk_path, file->metadata)) { + /* --update is receiver-side policy: never replace a newer destination. + The secure stat does not require read permission on the destination. */ + if (config && config->update && file_destination_is_newer_secure(disk_path, file->metadata)) { free(confined_backup); free(confined_partial); free(disk_path); @@ -110,8 +100,11 @@ bool file_save_to_disk(const char* root_directory, const File* file, const Confi } } - bool ok = file_to_disk_secure(disk_path, file->data->data, file->data->size, inplace, sparse, - file->metadata); + bool ok = config && config->update + ? file_to_disk_secure_update(disk_path, file->data->data, file->data->size, inplace, + sparse, file->metadata) + : file_to_disk_secure(disk_path, file->data->data, file->data->size, inplace, + sparse, file->metadata); free(parent_copy); free(backup_path); free(confined_backup); diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index 60d4427..6dbb333 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -227,21 +227,42 @@ class TestUpdate: with open(received_file, "wb") as f: f.write(b"newer destination\n") - os.utime(received_file, (source_stat.st_mtime + 10, source_stat.st_mtime + 10)) + os.utime(received_file, ns=(source_stat.st_atime_ns, source_stat.st_mtime_ns + 10_000_000_000)) result, _ = run_client(SOURCE_DIR, DEST_DIR, flags=["-u"], port=shared_server.port) assert result.returncode == 0 with open(received_file, "rb") as f: assert f.read() == b"newer destination\n" - os.utime(received_file, (source_stat.st_atime, source_stat.st_mtime)) + os.utime(received_file, ns=(source_stat.st_atime_ns, source_stat.st_mtime_ns)) result, _ = run_client(SOURCE_DIR, DEST_DIR, flags=["-u"], port=shared_server.port) assert result.returncode == 0 with open(received_file, "rb") as f: assert f.read() == b"hello world\n" + def test_update_skips_unreadable_newer_destination(self, shared_server): + clean_dir(DEST_DIR) + result, _ = run_client(SOURCE_DIR, DEST_DIR, flags=["-u"], port=shared_server.port) + assert result.returncode == 0 + + received_file = os.path.join(get_dest_received_dir(DEST_DIR, SOURCE_DIR), "small.txt") + source_stat = os.stat(os.path.join(SOURCE_DIR, "small.txt")) + with open(received_file, "wb") as f: + f.write(b"protected destination\n") + os.utime(received_file, ns=(source_stat.st_atime_ns, source_stat.st_mtime_ns + 10_000_000_000)) + original_mode = os.stat(received_file).st_mode + try: + os.chmod(received_file, 0) + result, _ = run_client(SOURCE_DIR, DEST_DIR, flags=["-u"], port=shared_server.port) + assert result.returncode == 0 + os.chmod(received_file, original_mode) + with open(received_file, "rb") as f: + assert f.read() == b"protected destination\n" + finally: + os.chmod(received_file, original_mode) + with open(received_file, "wb") as f: f.write(b"older destination\n") - os.utime(received_file, (source_stat.st_atime, source_stat.st_mtime - 10)) + os.utime(received_file, ns=(source_stat.st_atime_ns, source_stat.st_mtime_ns - 10_000_000_000)) result, _ = run_client(SOURCE_DIR, DEST_DIR, flags=["-u"], port=shared_server.port) assert result.returncode == 0 with open(received_file, "rb") as f: