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.
This commit is contained in:
+1
-1
@@ -173,7 +173,7 @@ void print_usage(void) {
|
|||||||
printf(" within the confined receive root. Never elevates\n");
|
printf(" within the confined receive root. Never elevates\n");
|
||||||
printf(" privileges and never bypasses confinement; ownership\n");
|
printf(" privileges and never bypasses confinement; ownership\n");
|
||||||
printf(" is still applied only with an explicit identity flag\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(" --no-super Forbid those super-user activities even when the\n");
|
||||||
printf(" receiver is running as root\n");
|
printf(" receiver is running as root\n");
|
||||||
printf(" --chmod <changes> Modify transferred permissions (rsync syntax)\n");
|
printf(" --chmod <changes> Modify transferred permissions (rsync syntax)\n");
|
||||||
|
|||||||
+18
-2
@@ -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)) {
|
!identity_apply_ownership_link(fd, component, 0, 0)) {
|
||||||
/* A REQUIRED --copy-as ownership that cannot be applied to a
|
/* A REQUIRED --copy-as ownership that cannot be applied to a
|
||||||
directory this walk just created must fail the entry rather than
|
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);
|
close(fd);
|
||||||
free(copy);
|
free(copy);
|
||||||
free(leaf);
|
free(leaf);
|
||||||
|
errno = saved_errno;
|
||||||
return -1;
|
return -1;
|
||||||
}
|
}
|
||||||
next = openat(fd, component, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC);
|
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;
|
return false;
|
||||||
|
|
||||||
int dir_fd = openat(parent_fd, leaf, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC);
|
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 (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);
|
dir_fd = openat(parent_fd, leaf, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC);
|
||||||
}
|
}
|
||||||
|
}
|
||||||
bool ok = dir_fd >= 0;
|
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)
|
if (dir_fd >= 0)
|
||||||
close(dir_fd);
|
close(dir_fd);
|
||||||
close(parent_fd);
|
close(parent_fd);
|
||||||
|
|||||||
@@ -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. */
|
FIFO creation is unprivileged and deliberately NOT gated here. */
|
||||||
if (!privilege_super_mode_permitted(config->super_mode)) {
|
if (!privilege_super_mode_permitted(config->super_mode)) {
|
||||||
log_message(LOG_LEVEL_WARNING,
|
log_message(LOG_LEVEL_WARNING,
|
||||||
"skipping %s: super-user device-node creation is not permitted "
|
"skipping %s: super-user device-node creation is not permitted on this receiver",
|
||||||
"(super-user activities disabled by --no-super)",
|
|
||||||
file->path);
|
file->path);
|
||||||
return FILE_SAVE_SKIPPED;
|
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 (config && config->write_devices) {
|
||||||
if (!privilege_super_mode_permitted(config->super_mode)) {
|
if (!privilege_super_mode_permitted(config->super_mode)) {
|
||||||
log_message(LOG_LEVEL_WARNING,
|
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)");
|
file->path ? file->path : "(null)");
|
||||||
return FILE_SAVE_SKIPPED;
|
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))
|
(int32_t)file->metadata->gid))
|
||||||
ok = false;
|
ok = false;
|
||||||
close(parent_fd);
|
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);
|
free(leaf);
|
||||||
}
|
}
|
||||||
@@ -679,9 +683,12 @@ FileSaveResult file_save_to_disk_full(const char* root_directory, const File* fi
|
|||||||
}
|
}
|
||||||
char* parent = str_dup(link_path);
|
char* parent = str_dup(link_path);
|
||||||
if (parent) {
|
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);
|
free(parent);
|
||||||
}
|
}
|
||||||
|
if (ok)
|
||||||
ok = file_symlink_at_secure(link_path, target);
|
ok = file_symlink_at_secure(link_path, target);
|
||||||
free(target);
|
free(target);
|
||||||
/* P7 Wave D: apply the symlink's own metadata with no-follow primitives
|
/* P7 Wave D: apply the symlink's own metadata with no-follow primitives
|
||||||
|
|||||||
@@ -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 file_restore_symlink_metadata(const char* path, const FileMetadata* metadata,
|
||||||
bool omit_link_times) {
|
bool omit_link_times) {
|
||||||
if (path == NULL || metadata == NULL)
|
if (path == NULL || metadata == NULL)
|
||||||
return true;
|
return !identity_copy_as_active();
|
||||||
char* leaf = NULL;
|
char* leaf = NULL;
|
||||||
int parent_fd = file_open_secure_parent(path, &leaf, false);
|
int parent_fd = file_open_secure_parent(path, &leaf, false);
|
||||||
if (parent_fd < 0)
|
if (parent_fd < 0)
|
||||||
|
|||||||
@@ -343,10 +343,13 @@ class TestDaemonRejection:
|
|||||||
assert result.returncode != 0
|
assert result.returncode != 0
|
||||||
assert _tree_file_count(AUTH_MODULE) == 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
|
"""A daemon module without `client owner = yes` refuses every
|
||||||
client-chosen ownership / super-user request at the config handshake,
|
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")
|
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 = os.path.getsize(log_path) if os.path.exists(log_path) else 0
|
||||||
before_files = self._tree_files()
|
before_files = self._tree_files()
|
||||||
@@ -358,7 +361,7 @@ class TestDaemonRejection:
|
|||||||
with open(log_path, "rb") as f:
|
with open(log_path, "rb") as f:
|
||||||
f.seek(before)
|
f.seek(before)
|
||||||
tail = f.read().decode("utf-8", "replace")
|
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}"
|
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
|
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
|
by default. The refusal happens at the config handshake, before any data
|
||||||
lands."""
|
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):
|
def test_super_refused_by_daemon(self, daemon):
|
||||||
"""An explicit --super is a super-user activity request, so a daemon
|
"""An explicit --super is a super-user activity request, so a daemon
|
||||||
|
|||||||
Reference in New Issue
Block a user