From 938886829e4b22c0c0f418009e5c133c6091c4c7 Mon Sep 17 00:00:00 2001 From: TapTap Date: Sat, 12 Sep 2026 12:54:45 +0200 Subject: [PATCH] fix(p7-privilege): close re-review gaps (implicit dir ownership, daemon --super, write-devices gate) --- src/server/server.c | 23 +++++++++++++++++++-- src/shared/file.c | 14 ++++++++++++- src/shared/file_receive.c | 26 +++++++++++++++++------- src/shared/identity.c | 5 ++--- tests/integration/test_daemon.py | 22 ++++++++++++++++++++ tests/integration/test_features.py | 32 ++++++++++++++++++++++++++++++ 6 files changed, 109 insertions(+), 13 deletions(-) diff --git a/src/server/server.c b/src/server/server.c index 0648660..e8edb13 100644 --- a/src/server/server.c +++ b/src/server/server.c @@ -195,6 +195,17 @@ static const char* server_module_gate(const Config* config, void* context) { "client-chosen ownership); refusing"); return "--copy-as is not permitted by this daemon"; } + /* --super (SUPER_MODE_ON) with no explicit identity policy implies raw + numeric-id ownership, i.e. a client-chosen owner. A daemon has no + per-module opt-in, so refuse the explicit ON request for the same reason it + refuses --copy-as; the pre-existing --numeric-ids/--chown/--usermap surfaces + are unchanged (documented daemon trust model). --no-super still works. */ + if (g_daemon_conf != NULL && config->super_mode == SUPER_MODE_ON) { + log_message(LOG_LEVEL_ERROR, + "--super is refused by the daemon (no per-module opt-in for client-chosen " + "ownership); refusing"); + return "--super is not permitted by this daemon"; + } /* Operator veto: --no-super forces SUPER_MODE_OFF for this connection before the copy-as gate is evaluated, and the caller clamps the accepted config again after this returns so the ownership/device gates see it too. */ @@ -202,8 +213,13 @@ static const char* server_module_gate(const Config* config, void* context) { if (server_no_super) effective->super_mode = SUPER_MODE_OFF; if (identity_copy_as_refused(effective)) { - log_message(LOG_LEVEL_ERROR, "--copy-as requires a privileged receiver (root); refusing"); - return "--copy-as requires a privileged receiver (root)"; + if (geteuid() != 0) + log_message(LOG_LEVEL_ERROR, "--copy-as requires a privileged receiver (root); refusing"); + else + log_message(LOG_LEVEL_ERROR, + "--copy-as refused: super-user activities are disabled by the server " + "(--no-super); refusing"); + return "cannot perform --copy-as on this receiver"; } /* --iconv (protocol 2.16.0): the receiver's exact conversion direction (the client spec's wire charset into this server's local charset, including a @@ -606,6 +622,9 @@ static void print_server_usage(void) { printf(" -6, --ipv6 Bind an IPv6 socket\n"); printf(" --allow-delete Permit manifest deletion\n"); printf(" --trust-sender Trust the remote sender's file list\n"); + printf(" --no-super Operator veto: never attempt super-user activities\n"); + printf(" (ownership, device nodes) even as root, and refuse\n"); + printf(" any client --copy-as/--super request\n"); printf(" --iconv=LOCAL[,REMOTE] Declare this server's LOCAL charset for file-name\n"); printf(" conversion: received names are translated to this\n"); printf(" charset (the wire charset still comes from the\n"); diff --git a/src/shared/file.c b/src/shared/file.c index 8905cf2..f402763 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -16,6 +16,7 @@ #include "data.h" #include "delta.h" #include "file.h" +#include "identity.h" #include "log.h" #include "metadata.h" #include "utils.h" @@ -572,8 +573,19 @@ int file_open_secure_parent(const char* path, char** leaf_out, bool create_dirs) if (strcmp(component, ".") != 0) { int next = openat(fd, component, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); if (next < 0 && create_dirs && errno == ENOENT) { - if (mkdirat(fd, component, 0755) == 0 || errno == EEXIST) + bool created = mkdirat(fd, component, 0755) == 0; + if (created || errno == EEXIST) { + /* P7 Wave E: --copy-as owns EVERY entry, including the intermediate + directories this walk creates implicitly. Its target ids are a + global policy, so they are available here without per-entry source + metadata. Only a directory this walk actually created is chowned + (a pre-existing destination directory is left alone, matching + rsync's transferred-entry scope); the helper is a no-op unless an + identity policy is active. */ + if (created && identity_copy_as_active()) + identity_apply_ownership_link(fd, component, 0, 0); next = openat(fd, component, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + } } /* --keep-dirlinks (-K): a path component that is an existing symlink to an in-root directory is used as THAT directory rather than failing the diff --git a/src/shared/file_receive.c b/src/shared/file_receive.c index d4ca072..10037c1 100644 --- a/src/shared/file_receive.c +++ b/src/shared/file_receive.c @@ -357,13 +357,15 @@ static FileSaveResult file_save_special_to_disk(const char* root_directory, cons if (!config || !config->preserve_devices) return FILE_SAVE_SKIPPED; /* --super / --no-super (P7 Wave E): char/block device-node creation is a - super-user activity. --no-super forbids it even for a root receiver; the - default AUTO only attempts it when already root. Pure FIFO creation is - unprivileged and deliberately NOT gated here. */ - if (!privilege_super_permitted()) { + super-user activity. --no-super forbids it even for a root receiver; + AUTO and --super attempt it (an unprivileged attempt is refused by the + kernel and skipped). The helper is evaluated against THIS config's mode + so the policy does not depend on a prior identity_set_active(). Pure + 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 " - "(--no-super, or the receiver is not privileged)", + "(super-user activities disabled by --no-super)", file->path); return FILE_SAVE_SKIPPED; } @@ -586,9 +588,19 @@ FileSaveResult file_save_to_disk_full(const char* root_directory, const File* fi writing content (privilege-gated, confined, rdev-validated). */ if (file->is_special) return file_save_special_to_disk(root_directory, file, config); - /* --write-devices: write straight into an existing device node. */ - if (config && config->write_devices) + /* --write-devices: write straight into an existing device node. Writing + into a device is a super-user activity, so --no-super must suppress it just + like device-node creation; the default AUTO/--super attempt it (the wide + open below keeps its own confinement and best-effort skip semantics). */ + 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", + file->path ? file->path : "(null)"); + return FILE_SAVE_SKIPPED; + } return file_save_write_device(root_directory, file); + } /* Explicit directory entries (--dirs) carry an empty payload; the entry is created as a directory under the receive root, applying the same secure diff --git a/src/shared/identity.c b/src/shared/identity.c index 3bf6ad6..023d530 100644 --- a/src/shared/identity.c +++ b/src/shared/identity.c @@ -93,7 +93,6 @@ void identity_set_active(const Config* config) { g_identity.groupmap_count = config->groupmap_count; } } - g_identity.super_mode = config->super_mode; g_identity.set = true; /* A root receiver would honor any client-supplied ownership request (a --usermap/--groupmap/--chown/--copy-as, or raw ids under --numeric-ids). @@ -467,7 +466,7 @@ int identity_parse_copy_as(Config* config, const char* value) { return -1; } char* user_token = spec; - char* group_token = NULL; + const char* group_token = NULL; char* colon = strchr(spec, ':'); if (colon) { *colon = '\0'; @@ -726,5 +725,5 @@ void identity_apply_ownership_link(int parent_fd, const char* leaf, int32_t sour if (!identity_resolve_targets(&st, source_uid, source_gid, &uid, &gid)) return; if (fchownat(parent_fd, leaf, uid, gid, AT_SYMLINK_NOFOLLOW) != 0) - identity_log_chown_failure("symlink", uid, gid); + identity_log_chown_failure("no-follow entry", uid, gid); } diff --git a/tests/integration/test_daemon.py b/tests/integration/test_daemon.py index 386a3be..03c5057 100644 --- a/tests/integration/test_daemon.py +++ b/tests/integration/test_daemon.py @@ -341,6 +341,28 @@ class TestDaemonRejection: f"daemon did not log the copy-as refusal: {tail[-400:]!r}" ) + def test_super_refused_by_daemon(self, daemon): + """P7 Wave E: --super (SUPER_MODE_ON) implies raw numeric-id ownership + with no explicit identity flag, so a daemon refuses it for the same + reason it refuses --copy-as: there is no per-module opt-in for + client-chosen ownership. The refusal happens at the config handshake, + before any data lands.""" + 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() + result, _ = run_client(SOURCE_DIR, "127.0.0.1::files", port=daemon.port, + flags=["--super", "--preserve"]) + assert result.returncode != 0, "the daemon must refuse --super" + assert self._tree_files() == before_files, \ + "--super refusal wrote under the module root" + time.sleep(0.3) + with open(log_path, "rb") as f: + f.seek(before) + tail = f.read().decode("utf-8", "replace") + assert "super is refused by the daemon" in tail, ( + f"daemon did not log the --super refusal: {tail[-400:]!r}" + ) + @pytest.mark.daemon_detach def test_real_detach_path(self): """--daemon WITHOUT --no-detach double-forks a real background daemon; diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index 9a18de5..311e0ce 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -5291,6 +5291,38 @@ class TestCopyAs: f"--copy-as did not own the directory: uid={st.st_uid} gid={st.st_gid}" ) + @pytest.mark.ci + @pytest.mark.skipif(os.geteuid() != 0, reason="requires a root receiver to chown") + def test_root_copy_as_owns_implicit_parent_dirs(self, shared_server): + """--copy-as must also own the intermediate directories that the receiver + creates implicitly while writing a nested file (the scanner does not emit + STATUS_MKDIR entries for ordinary traversal directories), not just the + file itself.""" + source = os.path.join(TEST_DATA_DIR, "copyas_nested_src") + dest = os.path.join(TEST_DATA_DIR, "copyas_nested_dst") + clean_dir(source) + clean_dir(dest) + nested = os.path.join(source, "top", "mid", "leaf") + os.makedirs(nested, exist_ok=True) + with open(os.path.join(nested, "deep.txt"), "wb") as fh: + fh.write(b"nested copy-as ownership\n") + + result, _ = run_client(source, dest, + flags=["--copy-as=@65534:@65534"], + port=shared_server.port) + assert result.returncode == 0, ( + f"--copy-as nested transfer failed: {(result.stderr or result.stdout)[:400]}" + ) + received = get_dest_received_dir(dest, source) + for rel in ("top", os.path.join("top", "mid"), os.path.join("top", "mid", "leaf")): + target = os.path.join(received, rel) + assert os.path.isdir(target), f"implicit directory missing at {target}" + st = os.stat(target) + assert (st.st_uid, st.st_gid) == (65534, 65534), ( + f"--copy-as did not own implicit directory {rel}: " + f"uid={st.st_uid} gid={st.st_gid}" + ) + @pytest.mark.ci @pytest.mark.skipif(os.geteuid() != 0, reason="requires a root receiver to chown") def test_root_copy_as_owns_fifo(self, shared_server):