From 423a62e69171ea262ec7261467a5af25a8cbb160 Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 21 Sep 2026 18:45:29 +0200 Subject: [PATCH] fix(receiver): confine --temp-dir scratch dir and gate setuid bits Three receiver security fixes from the audit: 1. --temp-dir symlink escape (High): file_open_temp_dir() opened the client-controlled scratch dir with a bare open(), so a symlink planted under the receive root let a peer redirect receiver scratch files outside the authorized root. The opened dir is now judged by the REAL path of its fd (via /proc/self/fd), and any target outside the authorized receive root is refused with a logged error (EACCES). An in-root symlink (the EXDEV cross-filesystem fallback case) still works, and the no-root local batch path is unchanged. 2. setuid/setgid/sticky under SUPER_MODE_OFF (High): the special bits were applied under --perms (and via --chmod) even when the connection forbade super-user activities. FileAttrPolicy gains super_permitted, set by file_attr_policy_from_config() from privilege_super_mode_permitted(); metadata_mode_for_policy(), the symlink path, the special-node creation path, and the deferred directory-mode apply now strip the special bits when it is false. Exact rsync semantics are preserved when permitted. 3. daemon umask (Low): daemonize() forced umask(0), so implied parent directories created without -p were world-writable 0777. Set the conventional daemon umask 022 instead (rsync never forces 0); -p/-a mode preservation is unaffected because it restores modes via fchmod. Tests: new unit tests for file_open_temp_dir confinement and the masked/unmasked special-bit policy (incl. the --chmod path), a daemon world-writable-dir regression test, an integration escape test, and a root-only integration test asserting special bits are masked without --allow-super. The old cross-filesystem test encoded the vulnerable behavior (symlink target outside the root) and is replaced by the escape test; the EXDEV fallback code is retained for in-root links. --- src/server/server.c | 14 +++-- src/shared/file.c | 52 +++++++++++++--- src/shared/file.h | 8 ++- src/shared/file_attr.h | 6 ++ src/shared/file_receive.c | 12 +++- src/shared/metadata.c | 25 ++++++-- tests/integration/test_daemon.py | 15 +++++ tests/integration/test_features.py | 74 ++++++++++++++-------- tests/test_file.c | 97 ++++++++++++++++++++++++----- tests/test_metadata.c | 98 ++++++++++++++++++++---------- tests/test_xattr.c | 6 +- 11 files changed, 309 insertions(+), 98 deletions(-) diff --git a/src/server/server.c b/src/server/server.c index aebf384..fa19876 100644 --- a/src/server/server.c +++ b/src/server/server.c @@ -1178,13 +1178,19 @@ static bool daemonize(void) { close(devnull); } /* Do not pin the launch CWD (module-relative 'path' entries would resolve - * against an unstable working directory) and drop the restrictive host umask - * so modules can create files/dirs with the modes the config requests. */ + * against an unstable working directory). Set a conservative daemon umask + * of 022 (the conventional service default): rsync never forces umask 0 -- + * it reads and restores the inherited umask and creates new entries as + * 0777 & ~umask / source & ~umask without -p. Forcing 0 here made every + * implied parent directory world-writable (0777) whenever -p metadata was not + * applied. 022 gives 0755 directories and source&~022 files, matching rsync + * under a normal daemon umask; -p/-a still restore the exact source mode via + * fchmod, which is unaffected by the umask. */ if (chdir("/") != 0) log_message(LOG_LEVEL_WARNING, "daemon: chdir to / failed: %s", strerror(errno)); - umask(0); + umask(022); /* Refresh the cached umask: main() captured the launch umask before this - * (single-threaded) umask(0), and file_mode_base() must see the daemon's + * (single-threaded) umask(022), and file_mode_base() must see the daemon's * actual umask. */ file_umask_capture(); return true; diff --git a/src/shared/file.c b/src/shared/file.c index 7e44023..4fdf3fe 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -1136,17 +1136,51 @@ int file_open_private_dir(const char* dir_path) { return fd; } -/* Open a --temp-dir scratch directory exactly as rsync does: the directory must - * already exist and is used as given (an absolute path is used verbatim, a - * relative one was already resolved against the destination root by the - * caller). Unlike file_open_private_dir this neither creates it nor confines - * it below the receive root, because rsync accepts any temp dir -- including - * one outside the destination tree or on another filesystem. Returns an - * O_DIRECTORY|O_CLOEXEC fd, or -1 on error. */ +/* Open a --temp-dir scratch directory. The directory must already exist (rsync + * never creates it); a relative path was already resolved against the + * destination root by the caller. Unlike file_open_private_dir this neither + * creates it nor requires it to be a direct child of the receive root, because + * rsync permits a scratch dir that (via a symlink) lands on another filesystem + * -- but it MUST resolve inside the authorized receive root. The directory is + * opened following symlinks and then judged by the REAL path of the opened fd + * (through /proc/self/fd), so a client-planted symlink under the receive root + * can never redirect receiver scratch files outside the sandbox while an + * in-root link to another filesystem (the EXDEV fallback case) still works. + * Returns an O_DIRECTORY|O_CLOEXEC fd, or -1 on error (errno set; an escaping + * target is reported as EACCES with a logged reason). */ int file_open_temp_dir(const char* dir_path) { if (!dir_path) return -1; - return open(dir_path, O_RDONLY | O_DIRECTORY | O_CLOEXEC); + int fd = open(dir_path, O_RDONLY | O_DIRECTORY | O_CLOEXEC); + if (fd < 0) + return -1; + const char* root = utils_get_authorized_root_path(); + if (!root) { + /* No authorized root (e.g. a local batch apply): nothing to confine + against, so preserve the historical open-as-given behavior. */ + return fd; + } + char fd_path[64]; + int fd_path_length = snprintf(fd_path, sizeof(fd_path), "/proc/self/fd/%d", fd); + char resolved[PATH_MAX]; + if (fd_path_length < 0 || (size_t)fd_path_length >= sizeof(fd_path) || + !realpath(fd_path, resolved)) { + int saved_errno = errno; + close(fd); + errno = saved_errno; + return -1; + } + if (!path_is_within_root(root, resolved)) { + char* escaped = output_escape(dir_path, log_get_8_bit_output()); + log_message(LOG_LEVEL_ERROR, + "--temp-dir '%s' resolves outside the authorized receive root; refusing", + escaped ? escaped : ""); + free(escaped); + close(fd); + errno = EACCES; + return -1; + } + return fd; } /* After the content and mode/times are restored on the just-written file, apply @@ -1859,6 +1893,6 @@ bool file_write_to_disk(const char* path, const void* data, unsigned long long d bool inplace, bool sparse) { if (!path || (!data && data_size != 0) || has_path_traversal(path)) return false; - FileAttrPolicy policy = {false, false, false, false}; + FileAttrPolicy policy = {0}; return file_to_disk_secure(path, data, data_size, inplace, sparse, false, NULL, policy, NULL); } diff --git a/src/shared/file.h b/src/shared/file.h index 612c50b..12654ee 100644 --- a/src/shared/file.h +++ b/src/shared/file.h @@ -103,8 +103,12 @@ bool file_remove_tree_secure(const char* path); the authorized root. Used for the --delay-updates staging directory. */ int file_open_private_dir(const char* dir_path); -/* Open an existing --temp-dir scratch directory as-is (absolute or relative; - no creation, no root confinement), matching rsync's --temp-dir handling. */ +/* Open an existing --temp-dir scratch directory (relative or absolute; no + creation). When an authorized receive root is configured the directory's + REAL path (symlinks resolved) must lie within it, so a client-planted + symlink cannot redirect receiver scratch files outside the sandbox; an + in-root symlink to another filesystem is still allowed for rsync's EXDEV + fallback. */ int file_open_temp_dir(const char* dir_path); /* The file_to_disk_secure* variants write a temporary copy in the destination diff --git a/src/shared/file_attr.h b/src/shared/file_attr.h index d965773..3beb8ab 100644 --- a/src/shared/file_attr.h +++ b/src/shared/file_attr.h @@ -29,6 +29,12 @@ typedef struct FileAttrPolicy { bool times; /* config->preserve_times: apply the source mtime */ bool atimes; /* config->preserve_atimes (-U): apply the source atime */ bool executability; /* config->use_executability (-E): exec-bits-only mode */ + /* privilege_super_mode_permitted(): when false (SUPER_MODE_OFF / --no-super, + or a daemon that did not grant `client owner = yes`), the setuid/setgid/ + sticky bits are stripped from every applied mode (source mode and any + --chmod result) even under --perms. When true, rsync's exact semantics are + preserved: -p copies the special bits and the kernel decides. */ + bool super_permitted; } FileAttrPolicy; /* Build the per-attribute policy from a connection's Config. A NULL config diff --git a/src/shared/file_receive.c b/src/shared/file_receive.c index d41ca1d..3abbda1 100644 --- a/src/shared/file_receive.c +++ b/src/shared/file_receive.c @@ -474,9 +474,13 @@ static FileSaveResult file_save_special_to_disk(const char* root_directory, cons /* Under -p/--perms rsync copies the source's permission and special bits; a * kernel that denies setuid/setgid/sticky reports the failure rather than * having them masked here. Without -p the node is created like any other new - * entry: source_mode & 0777 & ~umask. */ + * entry: source_mode & 0777 & ~umask. When super-user activities are + * forbidden, the special bits are stripped even under -p (they are + * super-user activities just like device-node creation). */ mode_t perms = config->preserve_perms ? (mode & (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0777)) : (mode & 0777 & ~(mode_t)file_process_umask()); + if (!privilege_super_mode_permitted(config->super_mode)) + perms &= ~(mode_t)(S_ISUID | S_ISGID | S_ISVTX); int rc = is_fifo ? mkfifoat(parent_fd, leaf, perms) : mknodat(parent_fd, leaf, create_mode | perms, rdev); @@ -2949,8 +2953,12 @@ void dir_metadata_list_apply(const DirTimeList* list, const char* root_directory } if (mode_ready) { /* rsync -p copies the source directory mode exactly, including - * group/other write and the setgid/sticky bits. */ + * group/other write and the setgid/sticky bits. Setuid/setgid/sticky + * are super-user activities: when the connection forbade them + * (SUPER_MODE_OFF / --no-super), strip them even under -p. */ mode_t safe_mode = dir_mode & (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0777); + if (!privilege_super_mode_permitted(config->super_mode)) + safe_mode &= ~(mode_t)(S_ISUID | S_ISGID | S_ISVTX); if (dir_fd < 0) { char* escaped_path = output_escape(dir_path, log_get_8_bit_output()); log_message(LOG_LEVEL_WARNING, "Failed to open directory %s to set its mode: %s", diff --git a/src/shared/metadata.c b/src/shared/metadata.c index 168d574..f0922b3 100644 --- a/src/shared/metadata.c +++ b/src/shared/metadata.c @@ -211,13 +211,22 @@ FileMetadata* metadata_receive(int file_descriptor, int* ok) { bool metadata_mode_for_policy(mode_t source_mode, mode_t current_mode, FileAttrPolicy policy, mode_t* out_mode) { + const mode_t special_bits = (mode_t)(S_ISUID | S_ISGID | S_ISVTX); const mode_t execute_bits = S_IXUSR | S_IXGRP | S_IXOTH; if (policy.perms) { /* rsync --perms copies the source's permission and special bits exactly, * including group/other write and setuid/setgid/sticky. The kernel may * still clear setgid when the receiver is not in the file's group; the - * caller logs a failed chmod rather than silently masking the bits here. */ - *out_mode = source_mode & (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0777); + * caller logs a failed chmod rather than silently masking the bits here. + * Setuid/setgid/sticky are super-user activities: when the connection did + * not permit them (SUPER_MODE_OFF / --no-super) they are stripped, so a + * client can never install a privileged bit on a receiver that forbade + * super-user activities. This also covers bits introduced by --chmod, + * whose result is fed in as source_mode. */ + mode_t bits = source_mode & (mode_t)(special_bits | 0777); + if (!policy.super_permitted) + bits &= ~special_bits; + *out_mode = bits; return true; } if (policy.executability) { @@ -227,8 +236,11 @@ bool metadata_mode_for_policy(mode_t source_mode, mode_t current_mode, FileAttrP * execute); otherwise clear every execute bit. This runs on the * destination-derived base (pre-existing dest mode, or source&~umask for a * new file), and leaves the special bits untouched. --perms wins when both - * are set (handled above). */ - mode_t base = current_mode & (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0777); + * are set (handled above). The destination's own special bits survive + * unless super-user activities are forbidden. */ + mode_t base = current_mode & (mode_t)(special_bits | 0777); + if (!policy.super_permitted) + base &= ~special_bits; if (source_mode & 0111) *out_mode = base | ((base & 0444) >> 2); else @@ -240,12 +252,13 @@ bool metadata_mode_for_policy(mode_t source_mode, mode_t current_mode, FileAttrP } FileAttrPolicy file_attr_policy_from_config(const Config* config) { - FileAttrPolicy policy = {false, false, false, false}; + FileAttrPolicy policy = {0}; if (config) { policy.perms = config->preserve_perms; policy.times = config->preserve_times; policy.atimes = config->preserve_atimes; policy.executability = config->use_executability; + policy.super_permitted = privilege_super_mode_permitted(config->super_mode); } return policy; } @@ -314,6 +327,8 @@ bool file_restore_symlink_metadata(const char* path, const FileMetadata* metadat transfer never fails over it. */ if (policy.perms) { mode_t link_mode = metadata->mode & (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0777); + if (!policy.super_permitted) + link_mode &= ~(mode_t)(S_ISUID | S_ISGID | S_ISVTX); if (fchmodat(parent_fd, leaf, link_mode, AT_SYMLINK_NOFOLLOW) != 0 && errno != EOPNOTSUPP && errno != ENOTSUP && errno != ENOSYS) { log_message(LOG_LEVEL_DEBUG, "Could not set symlink mode on %s: %s", path, strerror(errno)); diff --git a/tests/integration/test_daemon.py b/tests/integration/test_daemon.py index 00be064..0212616 100644 --- a/tests/integration/test_daemon.py +++ b/tests/integration/test_daemon.py @@ -332,6 +332,21 @@ class TestDaemonModuleSelection: assert not missing, f"missing: {missing[:5]}" assert not mismatches, f"mismatch: {mismatches[:5]}" + def test_daemon_new_dirs_not_world_writable(self, daemon): + """The daemon must not force umask 0: implied parent directories created + without -p are the source default (0755 under a 022 umask), never + world-writable 0777.""" + sub = os.path.join(FILES_MODULE, "umask_check") + shutil.rmtree(sub, ignore_errors=True) + os.makedirs(sub, exist_ok=True) + result = _push("127.0.0.1::files/umask_check", daemon.port) + assert result.returncode == 0, result.stderr or result.stdout + received = get_dest_received_dir(sub, SOURCE_DIR) + nested = os.path.join(received, "nested") + assert os.path.isdir(nested), f"nested dir missing under {received}" + mode = stat.S_IMODE(os.stat(nested).st_mode) + assert (mode & 0o022) == 0, f"implied directory is group/other writable: {oct(mode)}" + class TestDaemonRejection: def _tree_files(self): diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index 1a2c05c..d3371ab 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -2606,35 +2606,30 @@ class TestTimeoutAndAllocLimits: mismatches, missing = verify_transfer(source, received) assert not missing and not mismatches - def test_temp_dir_cross_filesystem_fallback(self, shared_server): - """A confined relative --temp-dir that resolves (via a symlink under the - destination root) to another filesystem must fall back to a non-atomic - copy instead of aborting (rsync parity). Skipped when no second - filesystem is available.""" - shm = "/dev/shm" - if not os.path.isdir(shm): - pytest.skip("/dev/shm not available") - if os.stat(shm).st_dev == os.stat(TEST_DATA_DIR).st_dev: - pytest.skip("/dev/shm is on the same filesystem as the test data") - scratch = os.path.join(shm, f"fastsync_tmp_{os.getpid()}") - shutil.rmtree(scratch, ignore_errors=True) - os.makedirs(scratch) + def test_temp_dir_symlink_escape_rejected(self, shared_server): + """A symlink planted inside the destination root pointing outside it + must not redirect receiver scratch files: --temp-dir= is + refused and nothing is written at the link target. An in-root symlink + (e.g. to a mount point that stays inside the authorized root) is still + accepted, preserving the engine's EXDEV cross-filesystem fallback.""" + source, dest = self._seed("tempdir_escape_src") + outside = "/tmp/fastsync_tempdir_escape_%d" % os.getpid() + shutil.rmtree(outside, ignore_errors=True) + os.makedirs(outside) + link = os.path.join(dest, "escape_scratch") + if os.path.lexists(link): + os.unlink(link) + os.symlink(outside, link) try: - source, dest = self._seed("tempdir_xdev_src") - # The receiver resolves a relative temp dir under the destination - # root; a symlink there points the scratch at the second filesystem. - link = os.path.join(dest, "xdev_scratch") - os.symlink(scratch, link) - result, _ = run_client(source, dest, flags=["--temp-dir", "xdev_scratch"], + result, _ = run_client(source, dest, flags=["--temp-dir", "escape_scratch"], port=shared_server.port) - assert result.returncode == 0, f"cross-fs temp-dir failed: {result.stderr[:300]}" + assert result.returncode != 0, "an escaping --temp-dir symlink must be refused" received = get_dest_received_dir(dest, source) - mismatches, missing = verify_transfer(source, received) - assert not missing, f"Missing: {missing}" - assert not mismatches, f"Mismatch: {mismatches}" - assert os.listdir(scratch) == [], "temp files left behind in the cross-fs scratch" + assert not os.path.exists(os.path.join(received, "f.txt")), \ + "the receiver must not fall back to writing the file" + assert os.listdir(outside) == [], "receiver wrote outside the authorized root" finally: - shutil.rmtree(scratch, ignore_errors=True) + shutil.rmtree(outside, ignore_errors=True) class TestRemoteOptionTransport: @@ -5782,6 +5777,35 @@ class TestStandaloneSuperDefault: "standalone server accepted --copy-as without --allow-super" ) + @pytest.mark.skipif( + os.geteuid() != 0, + reason="root triggers the SUPER_MODE_OFF default and can create setuid sources", + ) + def test_special_bits_masked_without_allow_super(self): + """A root standalone server without --allow-super forces SUPER_MODE_OFF, + so client-supplied setuid/setgid/sticky bits must be stripped even under + -p (they are super-user activities just like device-node creation).""" + source = os.path.join(TEST_DATA_DIR, "super_default_mode_src") + dest = os.path.join(TEST_DATA_DIR, "super_default_mode_dst") + clean_dir(source) + clean_dir(dest) + src_file = os.path.join(source, "priv.sh") + with open(src_file, "wb") as f: + f.write(b"#!/bin/sh\necho hi\n") + os.chmod(src_file, 0o4755) + server = ServerManager() + server.start() # deliberately no --allow-super -> SUPER_MODE_OFF as root + try: + result, _ = run_client(source, dest, flags=["-p"], port=server.port) + finally: + server.stop() + assert result.returncode == 0, f"exit {result.returncode}: {(result.stderr or '')[:200]}" + received = get_dest_received_dir(dest, source) + mode = stat.S_IMODE(os.stat(os.path.join(received, "priv.sh")).st_mode) + assert (mode & (stat.S_ISUID | stat.S_ISGID | stat.S_ISVTX)) == 0, \ + f"--no-super receiver kept a privileged bit: {oct(mode)}" + assert (mode & 0o777) == 0o755, f"ordinary permission bits lost: {oct(mode)}" + @pytest.mark.skipif(os.geteuid() != 0, reason="root can create the source device node") def test_devices_skipped_without_allow_super(self): """Root standalone server without --allow-super must skip device-node diff --git a/tests/test_file.c b/tests/test_file.c index 611b4cc..0546365 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -377,6 +377,70 @@ static void test_file_save_to_disk_temp_dir_confined() { rmdir(outside); } +/* A client-planted symlink under the receive root must never redirect the + * --temp-dir scratch directory outside the authorized root: the REAL path of + * the opened dir is checked. An in-root symlink (the EXDEV cross-filesystem + * case) must still be accepted. */ +static void test_file_open_temp_dir_symlink_confinement() { + const char* root = "test_tempdir_link_root"; + const char* outside = "test_tempdir_link_outside"; + char root_abs[PATH_MAX]; + char outside_abs[PATH_MAX]; + unlink("test_tempdir_link_root/escape"); + unlink("test_tempdir_link_root/inside_link"); + rmdir("test_tempdir_link_root/scratch"); + rmdir(root); + rmdir(outside); + EXPECT_EQ_INT(mkdir(root, 0755), 0); + EXPECT_EQ_INT(mkdir(outside, 0755), 0); + EXPECT_NOT_NULL(realpath(root, root_abs)); + EXPECT_NOT_NULL(realpath(outside, outside_abs)); + int root_fd = open(root_abs, O_RDONLY | O_DIRECTORY | O_CLOEXEC); + EXPECT_TRUE(root_fd >= 0); + // cppcheck-suppress knownConditionTrueFalse + if (root_fd < 0) { + rmdir(root); + rmdir(outside); + return; + } + EXPECT_TRUE(utils_set_authorized_root(root_fd, root_abs)); + + /* An existing in-root scratch dir opens normally. */ + char* scratch = path_cat(root_abs, "scratch"); + EXPECT_NOT_NULL(scratch); + EXPECT_EQ_INT(mkdir(scratch, 0755), 0); + int scratch_fd = file_open_temp_dir(scratch); + EXPECT_TRUE(scratch_fd >= 0); + if (scratch_fd >= 0) + close(scratch_fd); + + /* A symlink whose target is outside the root is refused. */ + char* escape = path_cat(root_abs, "escape"); + EXPECT_NOT_NULL(escape); + EXPECT_EQ_INT(symlink(outside_abs, escape), 0); + EXPECT_EQ_INT(file_open_temp_dir(escape), -1); + + /* A symlink that stays inside the root is accepted (EXDEV fallback path). */ + char* inside_link = path_cat(root_abs, "inside_link"); + EXPECT_NOT_NULL(inside_link); + EXPECT_EQ_INT(symlink(scratch, inside_link), 0); + int link_fd = file_open_temp_dir(inside_link); + EXPECT_TRUE(link_fd >= 0); + if (link_fd >= 0) + close(link_fd); + + free(inside_link); + free(escape); + free(scratch); + utils_set_authorized_root(-1, NULL); + close(root_fd); + unlink("test_tempdir_link_root/escape"); + unlink("test_tempdir_link_root/inside_link"); + rmdir("test_tempdir_link_root/scratch"); + rmdir(root); + rmdir(outside); +} + /* Issue #251: file_save_to_disk_full must distinguish receiver-side skips (--existing/--ignore-existing/--update) from real writes so the sender can decide whether --remove-source-files may unlink its source. */ @@ -1040,8 +1104,8 @@ static void test_atomic_no_perms_preserves_destination_mode() { /* No -p/-E: the pre-existing 0640 survives the atomic overwrite. */ bool ok = file_to_disk_secure_attrs(path, "data", 4, false, false, false, &m, - (FileAttrPolicy){false, false, false, false}, false, false, - false, NULL, false, false, NULL); + (FileAttrPolicy){false, false, false, false, true}, false, + false, false, NULL, false, false, NULL); EXPECT_TRUE(ok); struct stat st; EXPECT_EQ_INT(stat(path, &st), 0); @@ -1049,8 +1113,8 @@ static void test_atomic_no_perms_preserves_destination_mode() { /* -p: the source mode wins. */ ok = file_to_disk_secure_attrs(path, "data2", 5, false, false, false, &m, - (FileAttrPolicy){true, true, false, false}, false, false, false, - NULL, false, false, NULL); + (FileAttrPolicy){true, true, false, false, true}, false, false, + false, NULL, false, false, NULL); EXPECT_TRUE(ok); EXPECT_EQ_INT(stat(path, &st), 0); EXPECT_EQ_INT((int)(st.st_mode & 0777), 0755); @@ -1060,8 +1124,8 @@ static void test_atomic_no_perms_preserves_destination_mode() { source 0755 gives 0750, not 0751 and not the scratch 0711. */ EXPECT_EQ_INT(chmod(path, 0640), 0); ok = file_to_disk_secure_attrs(path, "data3", 6, false, false, false, &m, - (FileAttrPolicy){false, false, false, true}, false, false, false, - NULL, false, false, NULL); + (FileAttrPolicy){false, false, false, true, true}, false, false, + false, NULL, false, false, NULL); EXPECT_TRUE(ok); EXPECT_EQ_INT(stat(path, &st), 0); EXPECT_EQ_INT((int)(st.st_mode & 0777), 0750); @@ -1073,8 +1137,8 @@ static void test_atomic_no_perms_preserves_destination_mode() { const char* fresh = "test_attr_split_fresh.txt"; unlink(fresh); ok = file_to_disk_secure_attrs(fresh, "data", 4, false, false, false, &m, - (FileAttrPolicy){false, false, false, false}, false, false, false, - NULL, false, false, NULL); + (FileAttrPolicy){false, false, false, false, true}, false, false, + false, NULL, false, false, NULL); EXPECT_TRUE(ok); EXPECT_EQ_INT(stat(fresh, &st), 0); EXPECT_EQ_INT((int)(st.st_mode & 0777), (int)(m.mode & 0777 & ~(mode_t)file_process_umask())); @@ -1083,8 +1147,8 @@ static void test_atomic_no_perms_preserves_destination_mode() { /* Without any metadata the historical fixed 0644 default still applies. */ unlink(fresh); ok = file_to_disk_secure_attrs(fresh, "data", 4, false, false, false, NULL, - (FileAttrPolicy){false, false, false, false}, false, false, false, - NULL, false, false, NULL); + (FileAttrPolicy){false, false, false, false, true}, false, false, + false, NULL, false, false, NULL); EXPECT_TRUE(ok); EXPECT_EQ_INT(stat(fresh, &st), 0); EXPECT_EQ_INT((int)(st.st_mode & 0777), 0644); @@ -1104,8 +1168,8 @@ static void test_new_file_mode_honors_source_and_umask() { m.gid = getegid(); bool ok = file_to_disk_secure_attrs(path, "x", 1, false, false, false, &m, - (FileAttrPolicy){false, false, false, false}, false, false, - false, NULL, false, false, NULL); + (FileAttrPolicy){false, false, false, false, true}, false, + false, false, NULL, false, false, NULL); EXPECT_TRUE(ok); struct stat st; EXPECT_EQ_INT(stat(path, &st), 0); @@ -1663,8 +1727,8 @@ static void test_file_write_to_disk_partial_retention() { m.atime_valid = false; m.crtime_valid = false; bool ok = file_to_disk_secure_attrs(path, content, strlen(content), false, false, true, &m, - (FileAttrPolicy){true, true, false, false}, false, false, - false, NULL, false, true, NULL); + (FileAttrPolicy){true, true, false, false, true}, false, + false, false, NULL, false, true, NULL); EXPECT_FALSE(ok); /* the write itself succeeded, but metadata restore failed */ /* Retained: the already-written temp now sits at the destination path. */ int fd = open(path, O_RDONLY); @@ -1683,8 +1747,8 @@ static void test_file_write_to_disk_partial_retention() { /* Same failure with keep_partial=false: temp is unlinked, nothing retained. */ ok = file_to_disk_secure_attrs(path, content, strlen(content), false, false, true, &m, - (FileAttrPolicy){true, true, false, false}, false, false, false, - NULL, false, false, NULL); + (FileAttrPolicy){true, true, false, false, true}, false, false, + false, NULL, false, false, NULL); EXPECT_FALSE(ok); EXPECT_TRUE(access(path, F_OK) == -1); } @@ -2264,6 +2328,7 @@ void test_file() { test_file_save_to_disk_ignore_existing_entry_types(); test_file_save_to_disk_partial_install(); test_file_save_to_disk_temp_dir_confined(); + test_file_open_temp_dir_symlink_confinement(); test_file_save_to_disk_reports_skips(); test_file_write_to_disk_sparse_preserves_holes(); test_file_write_to_disk_partial_retention(); diff --git a/tests/test_metadata.c b/tests/test_metadata.c index e67604f..b8599b2 100644 --- a/tests/test_metadata.c +++ b/tests/test_metadata.c @@ -339,7 +339,7 @@ static void test_file_restore_metadata_applies_atime() { m.crtime_sec = 0; m.crtime_nsec = 0; - file_restore_metadata(path, &m, (FileAttrPolicy){true, true, true, false}); + file_restore_metadata(path, &m, (FileAttrPolicy){true, true, true, false, true}); struct stat st; EXPECT_EQ_INT(stat(path, &st), 0); @@ -378,7 +378,7 @@ static void test_file_restore_metadata() { .atime_valid = false, .crtime_valid = false}; - file_restore_metadata(path, &m, (FileAttrPolicy){true, true, false, false}); + file_restore_metadata(path, &m, (FileAttrPolicy){true, true, false, false, true}); struct stat st; EXPECT_EQ_INT(stat(path, &st), 0); @@ -395,7 +395,7 @@ static void test_file_restore_executability_only() { FileMetadata m = { .mode = 0751, .uid = getuid(), .gid = getgid(), .mtime_sec = 0, .mtime_nsec = 0}; - file_restore_metadata(path, &m, (FileAttrPolicy){false, false, false, true}); + file_restore_metadata(path, &m, (FileAttrPolicy){false, false, false, true, true}); struct stat st; EXPECT_EQ_INT(stat(path, &st), 0); @@ -409,7 +409,7 @@ static void test_directory_restore_executability_only() { FileMetadata m = { .mode = 0755, .uid = getuid(), .gid = getgid(), .mtime_sec = 0, .mtime_nsec = 0}; - file_restore_metadata(path, &m, (FileAttrPolicy){false, false, false, true}); + file_restore_metadata(path, &m, (FileAttrPolicy){false, false, false, true, true}); struct stat st; EXPECT_EQ_INT(stat(path, &st), 0); @@ -437,7 +437,7 @@ static void test_file_restore_executability_rsync_rule() { EXPECT_TRUE(file_write_to_disk(path, "x", 1, false, false)); EXPECT_EQ_INT(chmod(path, cases[i].dest), 0); FileMetadata m = {.mode = cases[i].src, .uid = getuid(), .gid = getgid()}; - file_restore_metadata(path, &m, (FileAttrPolicy){false, false, false, true}); + file_restore_metadata(path, &m, (FileAttrPolicy){false, false, false, true, true}); struct stat st; EXPECT_EQ_INT(stat(path, &st), 0); EXPECT_EQ_INT(st.st_mode & 0777, cases[i].want); @@ -453,34 +453,60 @@ static void test_file_restore_executability_rsync_rule() { * bits from the destination and --perms wins when both are set. */ static void test_metadata_mode_for_policy() { mode_t out = 0xdead; - EXPECT_FALSE( - metadata_mode_for_policy(0777, 0644, (FileAttrPolicy){false, false, false, false}, &out)); + EXPECT_FALSE(metadata_mode_for_policy(0777, 0644, + (FileAttrPolicy){false, false, false, false, true}, &out)); EXPECT_EQ_INT((int)out, 0xdead); /* untouched when no change is requested */ - EXPECT_TRUE( - metadata_mode_for_policy(0777, 0644, (FileAttrPolicy){true, false, false, false}, &out)); + EXPECT_TRUE(metadata_mode_for_policy(0777, 0644, + (FileAttrPolicy){true, false, false, false, true}, &out)); EXPECT_EQ_INT((int)(out & 0777), 0777); /* group/other write is preserved */ mode_t specials = (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0672); - EXPECT_TRUE( - metadata_mode_for_policy(specials, 0644, (FileAttrPolicy){true, false, false, false}, &out)); + EXPECT_TRUE(metadata_mode_for_policy(specials, 0644, + (FileAttrPolicy){true, false, false, false, true}, &out)); EXPECT_EQ_INT((int)(out & (S_ISUID | S_ISGID | S_ISVTX | 0777)), (int)(S_ISUID | S_ISGID | S_ISVTX | 0672)); + /* SUPER_MODE_OFF: the special bits are stripped even under -p (this also + * covers bits introduced by --chmod, whose result is fed in as source_mode), + * while the ordinary permission bits are still copied. */ + EXPECT_TRUE(metadata_mode_for_policy(specials, 0644, + (FileAttrPolicy){true, false, false, false, false}, &out)); + EXPECT_EQ_INT((int)(out & (S_ISUID | S_ISGID | S_ISVTX)), 0); + EXPECT_EQ_INT((int)(out & 0777), 0672); + EXPECT_TRUE(metadata_mode_for_policy(04755, 0644, + (FileAttrPolicy){true, false, false, false, false}, &out)); + EXPECT_EQ_INT((int)(out & 07777), 0755); + /* The same holds for a special bit introduced by --chmod=+s. */ + mode_t chmodded = 0; + EXPECT_TRUE(chmod_apply(0755, "u+s", &chmodded)); + EXPECT_TRUE(metadata_mode_for_policy(chmodded, 0644, + (FileAttrPolicy){true, false, false, false, false}, &out)); + EXPECT_EQ_INT((int)(out & 07777), 0755); + + /* -E: a destination's own special bits survive unless super-user activities + * are forbidden, in which case they are stripped from the derived base. */ + EXPECT_TRUE(metadata_mode_for_policy(0755, (mode_t)(S_ISUID | 0750), + (FileAttrPolicy){false, false, false, true, true}, &out)); + EXPECT_EQ_INT((int)(out & (S_ISUID | 0777)), (int)(S_ISUID | 0750)); + EXPECT_TRUE(metadata_mode_for_policy(0755, (mode_t)(S_ISUID | 0750), + (FileAttrPolicy){false, false, false, true, false}, &out)); + EXPECT_EQ_INT((int)(out & (S_ISUID | 0777)), 0750); + /* -E: exec bits derive from the DESTINATION's read bits. */ - EXPECT_TRUE( - metadata_mode_for_policy(0755, 0644, (FileAttrPolicy){false, false, false, true}, &out)); + EXPECT_TRUE(metadata_mode_for_policy(0755, 0644, + (FileAttrPolicy){false, false, false, true, true}, &out)); EXPECT_EQ_INT((int)(out & 0777), 0755); - EXPECT_TRUE( - metadata_mode_for_policy(0644, 0755, (FileAttrPolicy){false, false, false, true}, &out)); + EXPECT_TRUE(metadata_mode_for_policy(0644, 0755, + (FileAttrPolicy){false, false, false, true, true}, &out)); EXPECT_EQ_INT((int)(out & 0777), 0644); - EXPECT_TRUE( - metadata_mode_for_policy(0755, 0600, (FileAttrPolicy){false, false, false, true}, &out)); + EXPECT_TRUE(metadata_mode_for_policy(0755, 0600, + (FileAttrPolicy){false, false, false, true, true}, &out)); EXPECT_EQ_INT((int)(out & 0777), 0700); /* --perms wins over -E when both are set. */ EXPECT_TRUE( - metadata_mode_for_policy(0700, 0644, (FileAttrPolicy){true, false, false, true}, &out)); + metadata_mode_for_policy(0700, 0644, (FileAttrPolicy){true, false, false, true, true}, &out)); EXPECT_EQ_INT((int)(out & 0777), 0700); } @@ -493,8 +519,8 @@ static void test_new_file_mode_from_source_and_umask() { mode_t want = (mode_t)(0751 & 0777 & ~(mode_t)file_process_umask()); bool ok = file_to_disk_secure_attrs(path, "x", 1, false, false, false, &m, - (FileAttrPolicy){false, false, false, false}, false, false, - false, NULL, false, false, NULL); + (FileAttrPolicy){false, false, false, false, true}, false, + false, false, NULL, false, false, NULL); EXPECT_TRUE(ok); struct stat st; EXPECT_EQ_INT(stat(path, &st), 0); @@ -503,8 +529,8 @@ static void test_new_file_mode_from_source_and_umask() { /* -E on top of the source&~umask base (src 0751, umask 022 -> 0751). */ ok = file_to_disk_secure_attrs(path, "x", 1, false, false, false, &m, - (FileAttrPolicy){false, false, false, true}, false, false, false, - NULL, false, false, NULL); + (FileAttrPolicy){false, false, false, true, true}, false, false, + false, NULL, false, false, NULL); EXPECT_TRUE(ok); EXPECT_EQ_INT(stat(path, &st), 0); mode_t want_e = @@ -529,7 +555,7 @@ static void test_file_restore_attribute_split() { .crtime_valid = false}; /* times only: mtime changes, mode stays 0640. */ - file_restore_metadata(path, &m, (FileAttrPolicy){false, true, false, false}); + file_restore_metadata(path, &m, (FileAttrPolicy){false, true, false, false, true}); struct stat st; EXPECT_EQ_INT(stat(path, &st), 0); EXPECT_EQ_INT(st.st_mode & 0777, 0640); @@ -543,7 +569,7 @@ static void test_file_restore_attribute_split() { FileMetadata m2 = m; m2.mode = 0700; m2.mtime_sec = 1600000000; - file_restore_metadata(path, &m2, (FileAttrPolicy){false, false, false, false}); + file_restore_metadata(path, &m2, (FileAttrPolicy){false, false, false, false, true}); EXPECT_EQ_INT(stat(path, &st), 0); EXPECT_EQ_INT(st.st_mode & 0777, 0640); EXPECT_EQ_INT((int)st.st_mtime, 1000000000); @@ -569,6 +595,11 @@ static void test_file_attr_policy_from_config() { EXPECT_TRUE(p.times); EXPECT_TRUE(p.atimes); EXPECT_TRUE(p.executability); + /* Default super mode (AUTO) permits special bits. */ + EXPECT_TRUE(p.super_permitted); + c->super_mode = SUPER_MODE_OFF; + p = file_attr_policy_from_config(c); + EXPECT_FALSE(p.super_permitted); config_delete(c); } @@ -582,8 +613,8 @@ static void test_perms_preserves_special_bits() { .mode = (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0755), .uid = getuid(), .gid = getgid()}; bool ok = file_to_disk_secure_attrs(path, "x", 1, false, false, false, &m, - (FileAttrPolicy){true, false, false, false}, false, false, - false, NULL, false, false, NULL); + (FileAttrPolicy){true, false, false, false, true}, false, + false, false, NULL, false, false, NULL); EXPECT_TRUE(ok); struct stat st; EXPECT_EQ_INT(stat(path, &st), 0); @@ -670,7 +701,8 @@ static void test_file_restore_symlink_metadata() { /* Positive path: a non-omitted apply stamps the link's own mtime. */ FileMetadata applied = {.mtime_sec = 1000000000, .mtime_nsec = 0}; - file_restore_symlink_metadata(link, &applied, (FileAttrPolicy){false, true, false, false}, false); + file_restore_symlink_metadata(link, &applied, (FileAttrPolicy){false, true, false, false, true}, + false); struct stat st; EXPECT_EQ_INT(lstat(link, &st), 0); EXPECT_TRUE(S_ISLNK(st.st_mode)); @@ -679,7 +711,8 @@ static void test_file_restore_symlink_metadata() { /* -J: a different time must be left untouched. */ FileMetadata newer = {.mtime_sec = 1234567890, .mtime_nsec = 0}; - file_restore_symlink_metadata(link, &newer, (FileAttrPolicy){false, true, false, false}, true); + file_restore_symlink_metadata(link, &newer, (FileAttrPolicy){false, true, false, false, true}, + true); EXPECT_EQ_INT(lstat(link, &st), 0); EXPECT_EQ_INT((int)st.st_mtime, (int)t1); if (symlink_times_supported) @@ -723,14 +756,14 @@ static void test_file_restore_metadata_fd_attribute_split() { /* perms-only: mode applied, mtime untouched. */ EXPECT_EQ_INT(fstat(fd, &before), 0); - EXPECT_TRUE(file_restore_metadata_fd(fd, &m, (FileAttrPolicy){true, false, false, false})); + EXPECT_TRUE(file_restore_metadata_fd(fd, &m, (FileAttrPolicy){true, false, false, false, true})); EXPECT_EQ_INT(fstat(fd, &st), 0); EXPECT_EQ_INT(st.st_mode & 0777, 0755); EXPECT_EQ_INT((int)st.st_mtime, (int)before.st_mtime); /* times-only: mtime applied, mode untouched. */ EXPECT_EQ_INT(chmod(path, 0600), 0); - EXPECT_TRUE(file_restore_metadata_fd(fd, &m, (FileAttrPolicy){false, true, false, false})); + EXPECT_TRUE(file_restore_metadata_fd(fd, &m, (FileAttrPolicy){false, true, false, false, true})); EXPECT_EQ_INT(fstat(fd, &st), 0); EXPECT_EQ_INT(st.st_mode & 0777, 0600); EXPECT_EQ_INT((int)st.st_mtime, 1234567890); @@ -740,7 +773,7 @@ static void test_file_restore_metadata_fd_attribute_split() { {.tv_sec = 1000000000, .tv_nsec = 0}}; EXPECT_EQ_INT(futimens(fd, reset), 0); EXPECT_EQ_INT(fstat(fd, &before), 0); - EXPECT_TRUE(file_restore_metadata_fd(fd, &m, (FileAttrPolicy){false, false, true, false})); + EXPECT_TRUE(file_restore_metadata_fd(fd, &m, (FileAttrPolicy){false, false, true, false, true})); EXPECT_EQ_INT(fstat(fd, &st), 0); EXPECT_EQ_INT((int)st.st_atime, 999999999); EXPECT_EQ_INT((int)st.st_mtime, (int)before.st_mtime); @@ -753,7 +786,8 @@ static void test_file_restore_metadata_fd_attribute_split() { m2.mode = 0700; m2.mtime_sec = 1600000000; m2.atime_sec = 1700000000; - EXPECT_TRUE(file_restore_metadata_fd(fd, &m2, (FileAttrPolicy){false, false, false, false})); + EXPECT_TRUE( + file_restore_metadata_fd(fd, &m2, (FileAttrPolicy){false, false, false, false, true})); EXPECT_EQ_INT(fstat(fd, &st), 0); EXPECT_EQ_INT(st.st_mode & 0777, 0640); EXPECT_EQ_INT((int)st.st_mtime, 1000000000); diff --git a/tests/test_xattr.c b/tests/test_xattr.c index 4e268e5..10447a3 100644 --- a/tests/test_xattr.c +++ b/tests/test_xattr.c @@ -248,7 +248,7 @@ static void test_link_copy_fallback_preserves_xattrs() { m.crtime_valid = false; bool ok = file_to_disk_secure_link_attrs(dest, basis_dir, "payload", 7, false, &m, - (FileAttrPolicy){true, true, false, false}, false, + (FileAttrPolicy){true, true, false, false, true}, false, xattrs, true, NULL); xattr_list_free(xattrs); EXPECT_TRUE(ok); @@ -369,7 +369,7 @@ static void test_fake_super_restore() { } /* No xattr present yet: restore is a silent no-op (returns false, no crash). */ - FileAttrPolicy policy = {true, true, false, false}; + FileAttrPolicy policy = {true, true, false, false, true}; EXPECT_FALSE(fake_super_restore_fd(fd, policy)); fake_super_store_fd(fd, 1001, 1002, 0751, 1700000000, 123456789); @@ -423,7 +423,7 @@ static void test_fake_super_no_real_chown() { fake_super_store_fd(fd, 12345, 12346, 0755, 1700000000, 0); Config* c = config_create(); - FileAttrPolicy policy = {true, true, false, false}; + FileAttrPolicy policy = {true, true, false, false, true}; EXPECT_NOT_NULL(c); /* The strongest ownership request available plus permitted super mode. */ c->preserve_owner = true;