From b88acdbd3c03882af40009c622db850d30f9b1f1 Mon Sep 17 00:00:00 2001 From: TapTap Date: Wed, 23 Sep 2026 01:01:18 +0200 Subject: [PATCH] fix(xattr): whitelist path-based symlink apply; strengthen symlink xattr tests --- src/shared/file_save.c | 8 ++-- src/shared/xattr.c | 20 +++++++--- src/shared/xattr.h | 32 +++++++++++---- tests/integration/test_features.py | 16 +++++++- tests/test_xattr.c | 63 +++++++++++++++++++++++++++--- 5 files changed, 116 insertions(+), 23 deletions(-) diff --git a/src/shared/file_save.c b/src/shared/file_save.c index 3c49ebd..f51215b 100644 --- a/src/shared/file_save.c +++ b/src/shared/file_save.c @@ -926,12 +926,14 @@ static FileSaveResult file_save_symlink_to_disk(const FileSavePlan* plan, bool* /* -X/-A: apply the symlink's OWN xattrs with a no-follow primitive. The confined parent directory is the anchor and the final component is applied with lsetxattr, so the referent is never touched. Best-effort: on Linux - the VFS refuses xattrs on symlinks, so this is normally a no-op. */ - if (ok && config && config->use_xattrs && file->xattrs) { + the VFS refuses xattrs on symlinks, so this is normally a no-op. Hoist the + empty-list check so the common Linux case (NULL/empty xattrs) does not pay + an open/close of the parent per symlink. */ + if (ok && config && config->use_xattrs && file->xattrs && file->xattrs->count > 0) { char* leaf = NULL; int parent_fd = file_open_secure_parent(link_path, &leaf, false); if (parent_fd >= 0) { - xattr_apply_path_nofollow(parent_fd, leaf, file->xattrs); + xattr_apply_path_nofollow(parent_fd, leaf, file->xattrs, config->preserve_acls); close(parent_fd); } free(leaf); diff --git a/src/shared/xattr.c b/src/shared/xattr.c index e1ca859..aa0e747 100644 --- a/src/shared/xattr.c +++ b/src/shared/xattr.c @@ -383,8 +383,17 @@ bool xattr_apply_fd(int fd, const FileXattrList* list) { * referent. fsetxattr cannot be used (no *at xattr syscall exists, and the * kernel rejects xattr syscalls on an O_PATH descriptor), so the already-open, * confinement-checked parent directory is addressed through /proc/self/fd and - * the final component is applied with lsetxattr, which does not follow it. */ -bool xattr_apply_path_nofollow(int parent_fd, const char* leaf, const FileXattrList* list) { + * the final component is applied with lsetxattr, which does not follow it. + * + * The list is trusted to come from xattr_receive() (already whitelisted), but + * every name is re-validated here so this path-based primitive is confined on + * its own -- this is the only apply primitive that addresses a path, and the + * header promises a whitelisted apply. The apply is best-effort: if /proc is + * not mounted (the anchor cannot be formed) or the kernel refuses the set, the + * failure is skipped and never fails the transfer. See xattr.h for the bounded + * residual TOCTOU between link creation and lsetxattr. */ +bool xattr_apply_path_nofollow(int parent_fd, const char* leaf, const FileXattrList* list, + bool preserve_acls) { if (parent_fd < 0 || !leaf || leaf[0] == '\0' || strchr(leaf, '/') != NULL || !list) return false; if (list->count == 0) @@ -403,9 +412,10 @@ bool xattr_apply_path_nofollow(int parent_fd, const char* leaf, const FileXattrL int first_errno = 0; for (int i = 0; i < list->count; i++) { const FileXattr* xa = &list->items[i]; - /* Defense in depth: even a hand-crafted list can never apply the reserved - --fake-super key (only fake_super_store_fd may write it). */ - if (strcmp(xa->name, FAKESUPER_XATTR) == 0) + /* Defense in depth: re-validate against the receiver's full whitelist, so a + hand-crafted list can never apply a privileged namespace or the reserved + --fake-super key through this path-based primitive. */ + if (!xattr_name_appliable(xa->name, preserve_acls)) continue; if (lsetxattr(path, xa->name, xa->value, xa->value_len, 0) != 0) { if (!warned) { diff --git a/src/shared/xattr.h b/src/shared/xattr.h index 6628aeb..bbaa045 100644 --- a/src/shared/xattr.h +++ b/src/shared/xattr.h @@ -110,16 +110,32 @@ bool xattr_apply_fd(int fd, const FileXattrList* list); /* Receiver: apply every entry to the symlink named by (parent_fd, leaf) WITHOUT * following it, via lsetxattr() on the confined path - * "/proc/self/fd//". A symlink cannot be targeted by the - * fd-relative fsetxattr() path: there is no *at() xattr syscall and the kernel - * rejects xattr syscalls on an O_PATH descriptor, so the already-opened, + * "/proc/self/fd//". Every incoming name is independently + * re-validated against xattr_name_appliable() with `preserve_acls`, exactly like + * xattr_apply_fd(): a non-whitelisted namespace (including the reserved + * --fake-super key) is skipped, so this primitive stays confined even if handed + * a hand-crafted list. A symlink cannot be targeted by the fd-relative + * fsetxattr() path: there is no *at() xattr syscall and the kernel rejects + * xattr syscalls on an O_PATH descriptor, so the already-opened, * confinement-checked parent directory is the anchor and only the final * component is the (no-follow) link. `leaf` must be a single path component. - * Best-effort exactly like xattr_apply_fd(): a per-attribute failure (on Linux - * every set on a symlink fails with EPERM) is logged once and skipped, never - * fatal. Returns false only for an invalid anchor/list; true when an apply was - * attempted. The reserved --fake-super key is never applied. */ -bool xattr_apply_path_nofollow(int parent_fd, const char* leaf, const FileXattrList* list); + * + * Portability: the "/proc/self/fd/" anchor requires a mounted /proc. + * Where /proc is unavailable (or the fd cannot be addressed that way) the + * lsetxattr simply fails and is skipped -- the apply is best-effort exactly like + * xattr_apply_fd(), so no error is propagated and the transfer continues. A + * per-attribute failure (on Linux every set on a symlink fails with EPERM) is + * logged once and skipped, never fatal. Returns false only for an invalid + * anchor/list; true when an apply was attempted. + * + * Residual TOCTOU: `leaf` is a caller-supplied name resolved by path in the + * parent, so a local writer could replace the just-created symlink between its + * creation and lsetxattr(). This is bounded: it requires write access to the + * confinement-checked destination directory (already trusted), can only install + * a whitelisted user namespace or POSIX-ACL name, and never follows the link (a + * replacement symlink is still applied to as the final, no-follow component). */ +bool xattr_apply_path_nofollow(int parent_fd, const char* leaf, const FileXattrList* list, + bool preserve_acls); /* --fake-super: write the source uid/gid/mode/rdev record into the reserved * FAKESUPER_XATTR on `fd`, using rsync 3.4.1's exact grammar (see the key diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index d14f58a..5730af0 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -6675,13 +6675,25 @@ class TestExtendedAttributes: assert os.path.islink(dst_link), "destination link entry is not a symlink" assert os.readlink(dst_link) == "target.txt" - # The no-follow guarantee: the referent's attribute must never leak onto - # the symlink entry. + # The no-follow guarantee. Checking only the link's own xattr list is + # vacuous on Linux (lsetxattr on a symlink always fails EPERM), so also + # prove the apply never followed the link: the destination REFERENT must + # keep its own user.* value untouched. + dst_target = os.path.join(received, "target.txt") + assert os.getxattr(dst_target, "user.referent-only") == b"referent-value", ( + "the destination symlink apply followed the link and rewrote the " + "referent's xattr" + ) link_names = os.listxattr(dst_link, follow_symlinks=False) assert "user.referent-only" not in link_names, ( "the destination symlink captured its REFERENT's xattr " "(path-following capture bug)" ) + if sys.platform.startswith("linux"): + assert link_names == [], ( + "Linux associates no xattrs with a symlink; the link entry must " + "carry none" + ) if link_xattr_supported: assert os.getxattr( dst_link, "user.link-own", follow_symlinks=False diff --git a/tests/test_xattr.c b/tests/test_xattr.c index 8fc680a..314fbaf 100644 --- a/tests/test_xattr.c +++ b/tests/test_xattr.c @@ -8,6 +8,7 @@ #include "identity.h" #include "metadata.h" #include "protocol.h" +#include "scanner_internal.h" #include "test_utils.h" #include #include @@ -741,16 +742,16 @@ static void test_xattr_apply_path_nofollow_does_not_follow() { EXPECT_TRUE(xattr_list_append(list, "user.added", "x", 1)); /* Invalid anchors are refused before any syscall (no fd/leaf/list). */ - EXPECT_FALSE(xattr_apply_path_nofollow(-1, "link", list)); - EXPECT_FALSE(xattr_apply_path_nofollow(0, "", list)); - EXPECT_FALSE(xattr_apply_path_nofollow(0, "a/b", list)); - EXPECT_FALSE(xattr_apply_path_nofollow(0, "link", NULL)); + EXPECT_FALSE(xattr_apply_path_nofollow(-1, "link", list, false)); + EXPECT_FALSE(xattr_apply_path_nofollow(0, "", list, false)); + EXPECT_FALSE(xattr_apply_path_nofollow(0, "a/b", list, false)); + EXPECT_FALSE(xattr_apply_path_nofollow(0, "link", NULL, false)); /* The confined parent directory is the anchor; the final component is the link. Best-effort: returns true even when the kernel refuses. */ int dir_fd = open(root, O_RDONLY | O_DIRECTORY); EXPECT_TRUE(dir_fd >= 0); - EXPECT_TRUE(xattr_apply_path_nofollow(dir_fd, "link", list)); + EXPECT_TRUE(xattr_apply_path_nofollow(dir_fd, "link", list, false)); close(dir_fd); /* The referent must be untouched: a following apply would have set user.orig @@ -777,6 +778,57 @@ static void test_xattr_apply_path_nofollow_does_not_follow() { rmdir(root); } +/* Protocol 2.29.0 scanner wiring: scanner_capture_xattrs() must choose the + * NO-FOLLOW capture for a symlink entry, so the link's FileXattrList never + * carries the REFERENT's user.* attributes. xattr_capture_path_nofollow() is + * already covered directly above; this exercises the scanner CALL SITE, which is + * what makes the no-follow variant actually reach symlink entries. If the + * scanner regressed to the path-following capture, file->xattrs would contain + * user.symref and this test fails. Guarded on filesystem xattr support. */ +static void test_scanner_symlink_capture_is_nofollow() { + const char* target = "test_scanner_symlink_xattr_target"; + const char* link = "test_scanner_symlink_xattr_link"; + unlink(link); + unlink(target); + int fd = open(target, O_WRONLY | O_CREAT | O_TRUNC, 0600); + if (fd < 0) + return; + bool has_xattr = setxattr(target, "user.symref", "referent", 8, 0) == 0; + close(fd); + if (!has_xattr) { + unlink(target); + return; /* filesystem without xattr support */ + } + if (symlink(target, link) != 0) { + unlink(target); + return; + } + + DirectoryScanner scanner; + memset(&scanner, 0, sizeof(scanner)); + scanner.options.preserve_xattrs = true; + File* file = file_create(link); + EXPECT_NOT_NULL(file); + file->is_symlink = true; + + scanner_capture_xattrs(&scanner, file); + + /* The referent's attribute must not appear on the symlink's captured list. */ + bool leaked = false; + for (int i = 0; file->xattrs && i < file->xattrs->count; i++) { + if (strcmp(file->xattrs->items[i].name, "user.symref") == 0) + leaked = true; + } + EXPECT_FALSE(leaked); + /* On Linux the VFS associates no xattrs with a symlink, so the capture is + NULL (never an empty-but-valid list). */ + EXPECT_NULL(file->xattrs); + + file_destroy(file); + unlink(link); + unlink(target); +} + /* Protocol 2.29.0: a STATUS_SYMLINK frame followed by an -X/-A xattr block is * decoded by file_receive_symlink() with the block attached to the File. This * is the wire round-trip for the new trailing symlink xattr block. */ @@ -843,6 +895,7 @@ static void test_symlink_frame_carries_xattrs() { void test_xattr() { test_xattr_list_clone(); test_xattr_capture_symlink_nofollow(); + test_scanner_symlink_capture_is_nofollow(); test_xattr_apply_path_nofollow_does_not_follow(); test_symlink_frame_carries_xattrs(); test_xattr_wire_roundtrip();