From 24852cda3db38e2f2cd78a13703373c77f75f4d5 Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Mon, 4 May 2026 21:53:14 +1000 Subject: [PATCH 39/60] syscall+receiver: secure receiver-side do_chmod against symlink-race TOCTOU CVE-2026-29518's fix routed the receiver's open() through secure_relative_open(), but every other path-based syscall the receiver runs on sender-controllable paths is vulnerable to the same TOCTOU primitive. This commit closes the chmod variant. Add do_chmod_at() that opens the parent of fname under secure_relative_open() and uses fchmodat() against the resulting dirfd. Gate the secure path on am_daemon && !am_chrooted (the same gate use_secure_symlinks already uses for the receiver basis-file open), so non-daemon callers and chrooted daemons keep the original do_chmod() fast path. Migrate the receiver-side do_chmod() call sites in delete.c, generator.c, rsync.c, and xattrs.c. Adds testsuite/chmod-symlink-race.test (with t_chmod_secure helper) as regression coverage. Co-Authored-By: Claude Opus 4.7 (1M context) --- Makefile.in | 10 ++- delete.c | 4 +- generator.c | 4 +- rsync.c | 2 +- syscall.c | 80 ++++++++++++++++++++ t_chmod_secure.c | 117 ++++++++++++++++++++++++++++++ t_stub.c | 2 + testsuite/chmod-symlink-race.test | 68 +++++++++++++++++ xattrs.c | 6 +- 9 files changed, 282 insertions(+), 11 deletions(-) create mode 100644 t_chmod_secure.c create mode 100755 testsuite/chmod-symlink-race.test diff --git a/Makefile.in b/Makefile.in index be3e0fc5..481f3ece 100644 --- a/Makefile.in +++ b/Makefile.in @@ -57,13 +57,13 @@ TLS_OBJ = tls.o syscall.o util2.o t_stub.o lib/compat.o lib/snprintf.o lib/perms # Programs we must have to run the test cases CHECK_PROGS = rsync$(EXEEXT) tls$(EXEEXT) getgroups$(EXEEXT) getfsdev$(EXEEXT) \ - testrun$(EXEEXT) trimslash$(EXEEXT) t_unsafe$(EXEEXT) wildtest$(EXEEXT) \ - simdtest$(EXEEXT) + testrun$(EXEEXT) trimslash$(EXEEXT) t_unsafe$(EXEEXT) t_chmod_secure$(EXEEXT) \ + wildtest$(EXEEXT) simdtest$(EXEEXT) CHECK_SYMLINKS = testsuite/chown-fake.test testsuite/devices-fake.test testsuite/xattrs-hlink.test # Objects for CHECK_PROGS to clean -CHECK_OBJS=tls.o testrun.o getgroups.o getfsdev.o t_stub.o t_unsafe.o trimslash.o wildtest.o +CHECK_OBJS=tls.o testrun.o getgroups.o getfsdev.o t_stub.o t_unsafe.o t_chmod_secure.o trimslash.o wildtest.o # note that the -I. is needed to handle config.h when using VPATH .c.o: @@ -179,6 +179,10 @@ T_UNSAFE_OBJ = t_unsafe.o syscall.o util1.o util2.o t_stub.o lib/compat.o lib/sn t_unsafe$(EXEEXT): $(T_UNSAFE_OBJ) $(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(T_UNSAFE_OBJ) $(LIBS) +T_CHMOD_SECURE_OBJ = t_chmod_secure.o syscall.o util1.o util2.o t_stub.o lib/compat.o lib/snprintf.o lib/wildmatch.o lib/permstring.o +t_chmod_secure$(EXEEXT): $(T_CHMOD_SECURE_OBJ) + $(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(T_CHMOD_SECURE_OBJ) $(LIBS) + .PHONY: conf conf: configure.sh config.h.in diff --git a/delete.c b/delete.c index 4a294853..3a625610 100644 --- a/delete.c +++ b/delete.c @@ -98,7 +98,7 @@ static enum delret delete_dir_contents(char *fname, uint16 flags) strlcpy(p, fp->basename, remainder); if (!(fp->mode & S_IWUSR) && !am_root && fp->flags & FLAG_OWNED_BY_US) - do_chmod(fname, fp->mode | S_IWUSR); + do_chmod_at(fname, fp->mode | S_IWUSR); /* Save stack by recursing to ourself directly. */ if (S_ISDIR(fp->mode)) { if (delete_dir_contents(fname, flags | DEL_RECURSE) != DR_SUCCESS) @@ -139,7 +139,7 @@ enum delret delete_item(char *fbuf, uint16 mode, uint16 flags) } if (flags & DEL_NO_UID_WRITE) - do_chmod(fbuf, mode | S_IWUSR); + do_chmod_at(fbuf, mode | S_IWUSR); if (S_ISDIR(mode) && !(flags & DEL_DIR_IS_EMPTY)) { /* This only happens on the first call to delete_item() since diff --git a/generator.c b/generator.c index a890a43e..c3ace1c2 100644 --- a/generator.c +++ b/generator.c @@ -1499,7 +1499,7 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx, #ifdef HAVE_CHMOD if (!am_root && (file->mode & S_IRWXU) != S_IRWXU && dir_tweaking) { mode_t mode = file->mode | S_IRWXU; - if (do_chmod(fname, mode) < 0) { + if (do_chmod_at(fname, mode) < 0) { rsyserr(FERROR_XFER, errno, "failed to modify permissions on %s", full_fname(fname)); @@ -2107,7 +2107,7 @@ static void touch_up_dirs(struct file_list *flist, int ndx) continue; fname = f_name(file, NULL); if (fix_dir_perms) - do_chmod(fname, file->mode); + do_chmod_at(fname, file->mode); if (need_retouch_dir_times) { STRUCT_STAT st; if (link_stat(fname, &st, 0) == 0 && mtime_differs(&st, file)) { diff --git a/rsync.c b/rsync.c index b130aba5..cc46a2f9 100644 --- a/rsync.c +++ b/rsync.c @@ -657,7 +657,7 @@ int set_file_attrs(const char *fname, struct file_struct *file, stat_x *sxp, #ifdef HAVE_CHMOD if (!BITS_EQUAL(sxp->st.st_mode, new_mode, CHMOD_BITS)) { - int ret = am_root < 0 ? 0 : do_chmod(fname, new_mode); + int ret = am_root < 0 ? 0 : do_chmod_at(fname, new_mode); if (ret < 0) { rsyserr(FERROR_XFER, errno, "failed to set permissions on %s", diff --git a/syscall.c b/syscall.c index bfaaaa63..167aae0e 100644 --- a/syscall.c +++ b/syscall.c @@ -281,6 +281,86 @@ int do_chmod(const char *path, mode_t mode) return code; return 0; } + +/* + Symlink-race-safe variant of do_chmod() for receiver-side use. + + Threat model: on a daemon running with "use chroot = no" (the prerequisite + for CVE-2026-29518), a local attacker can race a symlink swap of one of + the parent directory components of a path the receiver is about to chmod. + Because chmod() resolves symlinks at every component, the swap redirects + the chmod outside the receiver's confinement. + + Defence: open the *parent* directory of fname under secure_relative_open() + (which uses openat2(RESOLVE_BENEATH) on Linux 5.6+, openat() with + O_RESOLVE_BENEATH on FreeBSD 13+ and macOS 15+ (Sequoia), or a per-component + O_NOFOLLOW walk elsewhere) and do fchmodat() against that dirfd. A symlink + substituted into one of the parent components is then either followed + within the tree (legitimate dir-symlinks still work) or rejected by the + kernel (escape attempts fail). + + Final-component handling matches do_chmod(): fchmodat() with flag 0 + follows a symlink at the final component, which is the same behaviour as + chmod() and matches every current call site (the file being chmod'd is + one the receiver itself just created or transferred). For the rare case + where the caller wants to chmod a symlink-as-an-object (S_ISLNK in the + mode bits), we fall through to do_chmod() which has portability code for + that case. + + Falls back to do_chmod() for absolute paths and for paths with no parent + component, where there is nothing to protect against. +*/ +int do_chmod_at(const char *fname, mode_t mode) +{ +#ifdef AT_FDCWD + extern int am_daemon, am_chrooted; + char dirpath[MAXPATHLEN]; + const char *bname; + const char *slash; + int dfd, ret, e; + size_t dlen; + + if (dry_run) return 0; + RETURN_ERROR_IF_RO_OR_LO; + + /* Only the daemon-without-chroot case is exposed to the symlink- + * race attack: a chroot already confines the receiver, and a + * non-daemon rsync runs with the user's own authority so a + * symlink they planted can only redirect to files they could + * already access. Everywhere else, fall through to plain + * do_chmod() to avoid the dirfd-open overhead on every call. */ + if (!am_daemon || am_chrooted) + return do_chmod(fname, mode); + + if (!fname || !*fname || *fname == '/' || S_ISLNK(mode)) + return do_chmod(fname, mode); + + slash = strrchr(fname, '/'); + if (!slash) + return do_chmod(fname, mode); + + dlen = slash - fname; + if (dlen >= sizeof dirpath) { + errno = ENAMETOOLONG; + return -1; + } + memcpy(dirpath, fname, dlen); + dirpath[dlen] = '\0'; + bname = slash + 1; + + dfd = secure_relative_open(NULL, dirpath, O_RDONLY | O_DIRECTORY, 0); + if (dfd < 0) + return -1; + + ret = fchmodat(dfd, bname, mode, 0); + e = errno; + close(dfd); + errno = e; + return ret; +#else + return do_chmod(fname, mode); +#endif +} #endif int do_rename(const char *old_path, const char *new_path) diff --git a/t_chmod_secure.c b/t_chmod_secure.c new file mode 100644 index 00000000..114dfb2d --- /dev/null +++ b/t_chmod_secure.c @@ -0,0 +1,117 @@ +/* + * Test harness for do_chmod_at(). Confirms the symlink-TOCTOU + * primitive used by CVE-2026-29518 (and its incomplete-fix follow-up + * for chmod) is closed by do_chmod_at(): a parent directory component + * being a symlink that escapes the receiver's confinement must be + * rejected, while a parent symlink that resolves *within* the tree + * must still work (so legitimate dir-symlinks are not regressed). + * + * Not linked into rsync itself. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include "rsync.h" + +#include + +int dry_run = 0; +int am_root = 0; +int am_sender = 0; +int read_only = 0; +int list_only = 0; +int copy_links = 0; +int copy_unsafe_links = 0; +extern int am_daemon, am_chrooted; + +short info_levels[COUNT_INFO], debug_levels[COUNT_DEBUG]; + +static int errs = 0; + +static void check(const char *label, int actual_rc, int expect_ok, + const char *path, mode_t expected_mode) +{ + struct stat st; + int got_ok = (actual_rc == 0); + if (got_ok != expect_ok) { + fprintf(stderr, "FAIL [%s]: rc=%d errno=%d (%s), expected %s\n", + label, actual_rc, errno, strerror(errno), + expect_ok ? "success" : "rejection"); + errs++; + return; + } + if (path && stat(path, &st) < 0) { + fprintf(stderr, "FAIL [%s]: stat(%s) failed: %s\n", + label, path, strerror(errno)); + errs++; + return; + } + if (path && (st.st_mode & 07777) != expected_mode) { + fprintf(stderr, + "FAIL [%s]: %s mode is 0%o, expected 0%o\n", + label, path, st.st_mode & 07777, expected_mode); + errs++; + return; + } + fprintf(stderr, "OK [%s]\n", label); +} + +int main(int argc, char **argv) +{ + if (argc != 2) { + fprintf(stderr, "usage: %s \n", argv[0]); + return 2; + } + if (chdir(argv[1]) < 0) { + perror("chdir"); + return 2; + } + + /* Simulate the daemon-without-chroot deployment that do_chmod_at() + * defends. With am_daemon=0 or am_chrooted=1 the wrapper falls + * through to plain do_chmod() and the symlink-race test would be + * meaningless. */ + am_daemon = 1; + am_chrooted = 0; + + /* Test layout (all inside the directory we just chdir'd to): + * + * ./realdir/sentinel -- regular target file + * ./inside_link -> realdir -- legitimate dir-symlink within the tree + * ./escape_link -> ../trap -- attacker swap, target outside tree + * ../trap/sentinel -- the file the attacker wants to alter + * + * The shell wrapper that calls this helper has set both sentinel + * files to mode 0600 so we have a clean baseline to compare. + */ + + /* Scenario A: legitimate parent dir-symlink, chmod must succeed. */ + int rc = do_chmod_at("inside_link/sentinel", 0640); + check("A: legit dir-symlink within tree", + rc, 1, "realdir/sentinel", 0640); + + /* Scenario B: parent symlink escapes the tree -- chmod must be + * rejected and the outside file's mode must be unchanged. */ + rc = do_chmod_at("escape_link/sentinel", 0666); + check("B: parent symlink escapes tree (the attack)", + rc, 0, "../trap/sentinel", 0600); + + /* Scenario C: plain relative path with no symlink components, + * regression check that the safe wrapper doesn't break the + * normal case. */ + rc = do_chmod_at("realdir/sentinel", 0644); + check("C: plain relative path (regression check)", + rc, 1, "realdir/sentinel", 0644); + + /* Scenario D: top-level file, no parent directory component. + * Falls back to do_chmod(); should succeed. */ + rc = do_chmod_at("topfile", 0640); + check("D: top-level file, no parent component", + rc, 1, "topfile", 0640); + + if (errs) + fprintf(stderr, "%d failure(s)\n", errs); + return errs ? 1 : 0; +} diff --git a/t_stub.c b/t_stub.c index 085378a8..904dac99 100644 --- a/t_stub.c +++ b/t_stub.c @@ -23,6 +23,8 @@ int do_fsync = 0; int inplace = 0; +int am_daemon = 0; +int am_chrooted = 0; int modify_window = 0; int preallocate_files = 0; int protect_args = 0; diff --git a/testsuite/chmod-symlink-race.test b/testsuite/chmod-symlink-race.test new file mode 100755 index 00000000..48bbfbb4 --- /dev/null +++ b/testsuite/chmod-symlink-race.test @@ -0,0 +1,68 @@ +#!/bin/sh + +# Copyright (C) 2026 by Andrew Tridgell + +# This program is distributable under the terms of the GNU GPL (see +# COPYING). + +# Regression test for the symlink-TOCTOU class of bug applied to +# chmod() on the receiver side. The CVE-2026-29518 fix used +# secure_relative_open() for the basis-file open, but every other +# path-based syscall the receiver runs on sender-controllable paths +# is vulnerable to the same primitive: a local attacker swaps a +# symlink into one of the parent directory components between the +# receiver's check and its act, and the syscall escapes the module. +# +# This test exercises the new do_chmod_at() wrapper via the +# t_chmod_secure helper. The helper sets up two scenarios: +# - a parent dir-symlink that resolves WITHIN the module tree +# (legitimate -K-style use, must continue to work) +# - a parent dir-symlink that escapes the module tree (the +# attack, must be rejected) +# plus two regression scenarios (plain relative path, top-level +# file) that just confirm the safe wrapper doesn't break the +# normal case. +# +# The kernel-enforced "stay below dirfd" path resolution is +# only available on Linux 5.6+, FreeBSD 13+, and macOS 15+. +# Skip on platforms that fall back to per-component O_NOFOLLOW +# (Solaris, OpenBSD, NetBSD, Cygwin); the per-component fallback +# would also reject the attack but the legitimate dir-symlink +# scenario would fail there. + +. "$suitedir/rsync.fns" + +case "$(uname -s)" in + SunOS|OpenBSD|NetBSD|CYGWIN*) + test_skipped "do_chmod_at relies on RESOLVE_BENEATH-equivalent kernel support not available on $(uname -s)" + ;; +esac + +mod="$scratchdir/module" +trap_outside="$scratchdir/trap" +rm -rf "$mod" "$trap_outside" +mkdir -p "$mod/realdir" "$trap_outside" + +# Set up the four file-system objects the helper expects: +echo bystander > "$mod/realdir/sentinel" +chmod 0600 "$mod/realdir/sentinel" +echo target > "$trap_outside/sentinel" +chmod 0600 "$trap_outside/sentinel" +ln -s realdir "$mod/inside_link" +ln -s ../trap "$mod/escape_link" +echo top > "$mod/topfile" +chmod 0600 "$mod/topfile" + +"$TOOLDIR/t_chmod_secure" "$mod" || \ + test_fail "t_chmod_secure reported failures (see stderr above)" + +# Sanity-check from the shell side too: the outside file's mode must +# still be 0600 -- the helper checked this, but a second look from +# the shell guards against a helper-internal stat() bug. +mode=$(stat -c '%a' "$trap_outside/sentinel" 2>/dev/null \ + || stat -f '%Lp' "$trap_outside/sentinel" 2>/dev/null) +if [ "$mode" != "600" ]; then + test_fail "outside sentinel mode changed from 600 to $mode -- chmod escaped the module" +fi + +exit 0 diff --git a/xattrs.c b/xattrs.c index 65166eed..e5d0dd43 100644 --- a/xattrs.c +++ b/xattrs.c @@ -1086,7 +1086,7 @@ int set_xattr(const char *fname, const struct file_struct *file, const char *fna && !S_ISLNK(sxp->st.st_mode) #endif && access(fname, W_OK) < 0 - && do_chmod(fname, (sxp->st.st_mode & CHMOD_BITS) | S_IWUSR) == 0) + && do_chmod_at(fname, (sxp->st.st_mode & CHMOD_BITS) | S_IWUSR) == 0) added_write_perm = 1; ndx = F_XATTR(file); @@ -1094,7 +1094,7 @@ int set_xattr(const char *fname, const struct file_struct *file, const char *fna lst = &glst->xa_items; int return_value = rsync_xal_set(fname, lst, fnamecmp, sxp); if (added_write_perm) /* remove the temporary write permission */ - do_chmod(fname, sxp->st.st_mode); + do_chmod_at(fname, sxp->st.st_mode); return return_value; } @@ -1211,7 +1211,7 @@ int set_stat_xattr(const char *fname, struct file_struct *file, mode_t new_mode) mode = (fst.st_mode & _S_IFMT) | (fmode & ACCESSPERMS) | (S_ISDIR(fst.st_mode) ? 0700 : 0600); if (fst.st_mode != mode) - do_chmod(fname, mode); + do_chmod_at(fname, mode); if (!IS_DEVICE(fst.st_mode)) fst.st_rdev = 0; /* just in case */ -- 2.51.0