From 2234879b2f229bb1ec39835b5d53c47ff4bda371 Mon Sep 17 00:00:00 2001 From: TapTap Date: Sat, 5 Sep 2026 12:29:46 +0200 Subject: [PATCH] fix: #258 normalize mode and truncate on --inplace overwrite The --inplace branch opened the destination with O_WRONLY|O_CREAT (no O_TRUNC) and only restored metadata when the sender supplied it. Two flaws resulted: 1. An existing destination file kept its original mode when no metadata was sent, so setuid/setgid/sticky bits survived an overwrite (a root sync could leave a root-owned setuid binary controlled by a client). 2. A shorter payload left stale trailing bytes from the previous version because the file was never truncated to the new length. In the inplace branch of file_to_disk_secure_impl: - Always trim the file to the new payload length (ftruncate after the write) so stale trailing bytes can never survive; sparse targets keep their pre-size ftruncate. - Always normalize the mode after a successful overwrite: apply the metadata-derived safe mode when metadata is present (as before), else fchmod to a safe default 0644, so setuid/setgid/sticky are cleared in both cases. - The --update newer-destination check still runs before any truncation or chmod, preserving the skip semantics. Adds unit tests in test_file.c: (a) setuid/sticky bits on an existing destination are cleared after an inplace write with and without metadata, (b) a shorter inplace payload leaves no trailing stale bytes. --- src/shared/file.c | 21 +++++- tests/test_file.c | 159 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 177 insertions(+), 3 deletions(-) diff --git a/src/shared/file.c b/src/shared/file.c index f8a7f84..cd87726 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -348,10 +348,25 @@ static bool file_to_disk_secure_impl(const char* path, const void* data, if (newer) { ok = true; } else { - if (!sparse || data_size == 0 || ftruncate(fd, (off_t)data_size) == 0) + /* In-place overwrites: pre-size sparse targets and always trim the + file to the new payload length afterwards so shorter payloads can + never leave stale trailing bytes from a previous version. */ + if (sparse && data_size > 0) + ok = ftruncate(fd, (off_t)data_size) == 0; + if (ok || !sparse || data_size == 0) ok = write_all(fd, data, data_size); - if (ok && metadata) - ok = file_restore_metadata_fd(fd, metadata, preserve_executability); + if (ok) + ok = ftruncate(fd, (off_t)data_size) == 0; + /* Normalize the mode: apply the metadata-derived safe mode when the + sender supplied metadata (setuid/setgid/sticky are never honored); + otherwise fall back to a safe default so dangerous bits on an + existing destination cannot survive an overwrite. */ + if (ok) { + if (metadata) + ok = file_restore_metadata_fd(fd, metadata, preserve_executability); + else if (fchmod(fd, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH) != 0) + ok = false; + } if (ok && use_fsync) ok = fsync(fd) == 0; } diff --git a/tests/test_file.c b/tests/test_file.c index 787a60e..b936e98 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -5,6 +5,7 @@ #include "utils.h" #include "protocol.h" #include "test_utils.h" +#include #include #include #include @@ -624,6 +625,161 @@ static void test_file_send_single_calls_metadata_and_path() { } } +static void test_inplace_overwrite_clears_special_mode_bits() { + const char* root = "test_inplace_tmp"; + const char* path = "test_inplace_tmp/priv.txt"; + const char* content = "olddata"; + unlink(path); + rmdir(root); + EXPECT_EQ_INT(mkdir(root, 0700), 0); + + /* Create a destination carrying setuid + sticky bits. */ + int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC, 0644); + EXPECT_TRUE(fd >= 0); + // cppcheck-suppress knownConditionTrueFalse + if (fd < 0) { + rmdir(root); + return; + } + EXPECT_EQ_INT((int)write(fd, content, strlen(content)), (int)strlen(content)); + EXPECT_EQ_INT(fchmod(fd, S_ISUID | S_ISVTX | 0755), 0); + EXPECT_EQ_INT(close(fd), 0); + + /* Overwrite in place without metadata: the mode must be normalized to a + safe default (0644) and the setuid/sticky bits must be gone. */ + File* f = file_create("priv.txt"); + EXPECT_NOT_NULL(f); + const char* new_content = "newdata"; + f->data->data = malloc(strlen(new_content)); + EXPECT_NOT_NULL(f->data->data); + memcpy(f->data->data, new_content, strlen(new_content)); + f->data->size = strlen(new_content); + + Config* cfg = config_create(); + EXPECT_NOT_NULL(cfg); + cfg->inplace = true; + EXPECT_TRUE(file_save_to_disk(root, f, cfg)); + file_destroy(f); + config_delete(cfg); + + struct stat st; + EXPECT_EQ_INT(stat(path, &st), 0); + EXPECT_EQ_INT((int)(st.st_mode & (S_ISUID | S_ISGID | S_ISVTX)), 0); + EXPECT_EQ_INT((int)(st.st_mode & 0777), 0644); + FILE* stream = fopen(path, "rb"); + char buf[16] = {0}; + EXPECT_NOT_NULL(stream); + // cppcheck-suppress knownConditionTrueFalse + if (stream) { + size_t nread = fread(buf, 1, sizeof(buf) - 1, stream); + fclose(stream); + EXPECT_EQ_INT((int)nread, (int)strlen(new_content)); + } + EXPECT_EQ_STR(buf, new_content); + + unlink(path); + rmdir(root); +} + +static void test_inplace_overwrite_metadata_strips_special_bits() { + const char* root = "test_inplace_meta_tmp"; + const char* path = "test_inplace_meta_tmp/meta.txt"; + const char* source = "test_inplace_meta_source.txt"; + unlink(path); + unlink(source); + rmdir(root); + EXPECT_EQ_INT(mkdir(root, 0700), 0); + + /* Existing destination with setuid+sticky set. */ + int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC, 0644); + EXPECT_TRUE(fd >= 0); + // cppcheck-suppress knownConditionTrueFalse + if (fd < 0) { + rmdir(root); + return; + } + EXPECT_EQ_INT((int)write(fd, "olddata", 7), 7); + EXPECT_EQ_INT(fchmod(fd, S_ISUID | S_ISVTX | 0755), 0); + EXPECT_EQ_INT(close(fd), 0); + + /* Build source metadata carrying a plain executable mode (no specials). */ + EXPECT_TRUE(file_write_to_disk(source, "source", 6, false, false)); + EXPECT_EQ_INT(chmod(source, 0755), 0); + struct stat source_st; + EXPECT_EQ_INT(stat(source, &source_st), 0); + + File* f = file_create("meta.txt"); + EXPECT_NOT_NULL(f); + const char* new_content = "meta"; + f->data->data = malloc(strlen(new_content)); + EXPECT_NOT_NULL(f->data->data); + memcpy(f->data->data, new_content, strlen(new_content)); + f->data->size = strlen(new_content); + f->metadata = file_metadata_create(&source_st); + EXPECT_NOT_NULL(f->metadata); + + Config* cfg = config_create(); + EXPECT_NOT_NULL(cfg); + cfg->inplace = true; + EXPECT_TRUE(file_save_to_disk(root, f, cfg)); + file_destroy(f); + config_delete(cfg); + unlink(source); + + struct stat st; + EXPECT_EQ_INT(stat(path, &st), 0); + /* Metadata-derived mode is applied and never includes setuid/setgid/sticky. */ + EXPECT_EQ_INT((int)(st.st_mode & (S_ISUID | S_ISGID | S_ISVTX)), 0); + EXPECT_EQ_INT((int)(st.st_mode & 0777), 0755); + + unlink(path); + rmdir(root); +} + +static void test_inplace_overwrite_truncates_shorter_payload() { + const char* root = "test_inplace_trunc_tmp"; + const char* path = "test_inplace_trunc_tmp/big.txt"; + unlink(path); + rmdir(root); + EXPECT_EQ_INT(mkdir(root, 0700), 0); + + const char* old_content = "0123456789abcdef"; /* 16 bytes */ + EXPECT_TRUE(file_write_to_disk(path, old_content, strlen(old_content), false, false)); + + File* f = file_create("big.txt"); + EXPECT_NOT_NULL(f); + const char* new_content = "hi"; + f->data->data = malloc(strlen(new_content)); + EXPECT_NOT_NULL(f->data->data); + memcpy(f->data->data, new_content, strlen(new_content)); + f->data->size = strlen(new_content); + + Config* cfg = config_create(); + EXPECT_NOT_NULL(cfg); + cfg->inplace = true; + EXPECT_TRUE(file_save_to_disk(root, f, cfg)); + file_destroy(f); + config_delete(cfg); + + /* A shorter payload must truncate the file: no stale trailing bytes. */ + struct stat st; + EXPECT_EQ_INT(stat(path, &st), 0); + EXPECT_EQ_INT((int)st.st_size, (int)strlen(new_content)); + FILE* stream = fopen(path, "rb"); + char buf[32] = {0}; + EXPECT_NOT_NULL(stream); + // cppcheck-suppress knownConditionTrueFalse + if (stream) { + size_t nread = fread(buf, 1, sizeof(buf) - 1, stream); + fclose(stream); + EXPECT_EQ_INT((int)nread, (int)strlen(new_content)); + } + EXPECT_EQ_STR(buf, new_content); + + unlink(path); + rmdir(root); +} + void test_file() { test_file_create(); test_file_destroy_null(); @@ -654,4 +810,7 @@ void test_file() { test_file_send_single_calls_metadata_and_path(); } test_file_metadata_create(); + test_inplace_overwrite_clears_special_mode_bits(); + test_inplace_overwrite_metadata_strips_special_bits(); + test_inplace_overwrite_truncates_shorter_payload(); }