From ea0a0e2eafff45366567ff5896fe65faaff6c51c Mon Sep 17 00:00:00 2001 From: TapTap Date: Sat, 12 Sep 2026 14:33:42 +0200 Subject: [PATCH] fix(p8-security): make --copy-as directory ownership airtight; harden tests/logs - file_ensure_directory_secure() now chowns a final directory it creates under --copy-as and fails on error; the symlink parent-creation call site propagates it. The is_dir branch fails when the confined parent cannot be opened under --copy-as. Closes the residual wrong-owner gap for synthesized/symlink parent directories. - file_restore_symlink_metadata() early NULL return is copy-as-aware. - Preserve errno across the implicit-parent failure cleanup. - Neutral skip messages (the clamp, not --no-super, may be responsible). - Daemon copy-as test tolerates the non-root privilege refusal; usage text lists --copy-as. --- src/client/usage.c | 2 +- src/shared/file.c | 20 ++++++++++++++++++-- src/shared/file_receive.c | 17 ++++++++++++----- src/shared/metadata.c | 2 +- tests/integration/test_daemon.py | 13 +++++++++---- 5 files changed, 41 insertions(+), 13 deletions(-) diff --git a/src/client/usage.c b/src/client/usage.c index 9d793d3..0da87ba 100644 --- a/src/client/usage.c +++ b/src/client/usage.c @@ -173,7 +173,7 @@ void print_usage(void) { printf(" within the confined receive root. Never elevates\n"); printf(" privileges and never bypasses confinement; ownership\n"); printf(" is still applied only with an explicit identity flag\n"); - printf(" (--numeric-ids/--chown/--usermap/--groupmap)\n"); + printf(" (--numeric-ids/--chown/--usermap/--groupmap/--copy-as)\n"); printf(" --no-super Forbid those super-user activities even when the\n"); printf(" receiver is running as root\n"); printf(" --chmod Modify transferred permissions (rsync syntax)\n"); diff --git a/src/shared/file.c b/src/shared/file.c index c6bc4ef..58e2753 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -586,10 +586,14 @@ int file_open_secure_parent(const char* path, char** leaf_out, bool create_dirs) !identity_apply_ownership_link(fd, component, 0, 0)) { /* A REQUIRED --copy-as ownership that cannot be applied to a directory this walk just created must fail the entry rather than - leave that implicit parent owned by the receiver. */ + leave that implicit parent owned by the receiver. Preserve the + failing errno across the cleanup so the caller logs the real + reason. */ + int saved_errno = errno; close(fd); free(copy); free(leaf); + errno = saved_errno; return -1; } next = openat(fd, component, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); @@ -699,11 +703,23 @@ bool file_ensure_directory_secure(const char* path) { return false; int dir_fd = openat(parent_fd, leaf, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + bool created = false; if (dir_fd < 0 && errno == ENOENT) { - if (mkdirat(parent_fd, leaf, 0755) == 0 || errno == EEXIST) + if (mkdirat(parent_fd, leaf, 0755) == 0) { + created = true; dir_fd = openat(parent_fd, leaf, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + } else if (errno == EEXIST) { + dir_fd = openat(parent_fd, leaf, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + } } bool ok = dir_fd >= 0; + /* --copy-as owns a directory this call just created (the final component; + intermediate components were handled by file_open_secure_parent above). A + failed REQUIRED ownership fails the call rather than leaving the directory + owned by the receiver. */ + if (ok && created && identity_copy_as_active() && + !identity_apply_ownership_link(parent_fd, leaf, 0, 0)) + ok = false; if (dir_fd >= 0) close(dir_fd); close(parent_fd); diff --git a/src/shared/file_receive.c b/src/shared/file_receive.c index 9ce4d90..7059596 100644 --- a/src/shared/file_receive.c +++ b/src/shared/file_receive.c @@ -364,8 +364,7 @@ static FileSaveResult file_save_special_to_disk(const char* root_directory, cons FIFO creation is unprivileged and deliberately NOT gated here. */ if (!privilege_super_mode_permitted(config->super_mode)) { log_message(LOG_LEVEL_WARNING, - "skipping %s: super-user device-node creation is not permitted " - "(super-user activities disabled by --no-super)", + "skipping %s: super-user device-node creation is not permitted on this receiver", file->path); return FILE_SAVE_SKIPPED; } @@ -598,7 +597,8 @@ FileSaveResult file_save_to_disk_full(const char* root_directory, const File* fi if (config && config->write_devices) { if (!privilege_super_mode_permitted(config->super_mode)) { log_message(LOG_LEVEL_WARNING, - "write-devices: %s skipped: super-user activities disabled by --no-super", + "write-devices: %s skipped: super-user activities are not permitted on this " + "receiver", file->path ? file->path : "(null)"); return FILE_SAVE_SKIPPED; } @@ -633,6 +633,10 @@ FileSaveResult file_save_to_disk_full(const char* root_directory, const File* fi (int32_t)file->metadata->gid)) ok = false; close(parent_fd); + } else if (identity_copy_as_active()) { + /* The directory exists (ok) but its required --copy-as ownership could + not be applied because the confined parent could not be opened. */ + ok = false; } free(leaf); } @@ -679,10 +683,13 @@ FileSaveResult file_save_to_disk_full(const char* root_directory, const File* fi } char* parent = str_dup(link_path); if (parent) { - file_ensure_directory_secure(dirname(parent)); + /* Propagate a failed --copy-as ownership of the parent directory this + creates; every other failure mode stays best-effort as before. */ + ok = file_ensure_directory_secure(dirname(parent)); free(parent); } - ok = file_symlink_at_secure(link_path, target); + if (ok) + ok = file_symlink_at_secure(link_path, target); free(target); /* P7 Wave D: apply the symlink's own metadata with no-follow primitives (utimensat/lchown/fchmodat AT_SYMLINK_NOFOLLOW). -J/--omit-link-times diff --git a/src/shared/metadata.c b/src/shared/metadata.c index 949d365..b1cff1e 100644 --- a/src/shared/metadata.c +++ b/src/shared/metadata.c @@ -360,7 +360,7 @@ void file_restore_metadata(const char* path, const FileMetadata* metadata, bool file_restore_symlink_metadata(const char* path, const FileMetadata* metadata, bool omit_link_times) { if (path == NULL || metadata == NULL) - return true; + return !identity_copy_as_active(); char* leaf = NULL; int parent_fd = file_open_secure_parent(path, &leaf, false); if (parent_fd < 0) diff --git a/tests/integration/test_daemon.py b/tests/integration/test_daemon.py index d0ea7ca..d0781e0 100644 --- a/tests/integration/test_daemon.py +++ b/tests/integration/test_daemon.py @@ -343,10 +343,13 @@ class TestDaemonRejection: assert result.returncode != 0 assert _tree_file_count(AUTH_MODULE) == 0 - def _assert_ownership_refused(self, daemon, module, flags): + def _assert_ownership_refused(self, daemon, module, flags, + accept=("client-chosen ownership",)): """A daemon module without `client owner = yes` refuses every client-chosen ownership / super-user request at the config handshake, - before any data lands.""" + before any data lands. `accept` lists the log phrases that count as the + refusal (a non-root daemon refuses --copy-as earlier, at the privilege + check, so the caller accepts that phrase too).""" log_path = os.path.join(TEST_DATA_DIR, "fastsyncd.log") before = os.path.getsize(log_path) if os.path.exists(log_path) else 0 before_files = self._tree_files() @@ -358,7 +361,7 @@ class TestDaemonRejection: with open(log_path, "rb") as f: f.seek(before) tail = f.read().decode("utf-8", "replace") - assert "client-chosen ownership" in tail, ( + assert any(phrase in tail for phrase in accept), ( f"daemon did not log the ownership refusal: {tail[-400:]!r}" ) @@ -368,7 +371,9 @@ class TestDaemonRejection: so even a root daemon must not honor an arbitrary client-selected owner by default. The refusal happens at the config handshake, before any data lands.""" - self._assert_ownership_refused(daemon, "files", ["--copy-as=@65534:@65534"]) + self._assert_ownership_refused( + daemon, "files", ["--copy-as=@65534:@65534"], + accept=("client-chosen ownership", "requires a privileged receiver")) def test_super_refused_by_daemon(self, daemon): """An explicit --super is a super-user activity request, so a daemon