From 679ad50a99fd3efe0f2a029502a606520b38c34f Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Wed, 20 May 2026 11:28:28 +1000 Subject: [PATCH 01/11] fileflags: portable BSD chflags + Linux chattr implementation Squash of pr-fileflags work, rebased onto 3.4.3. Original branch had 12 commits walking through: 1. apply rsync-patches/fileflags.diff (BSD-only --fileflags option) 2. testsuite/fileflags.test 3. CI fixes 4. portable stat -f %f readback 5. phase-1 security: fd-based chflags, sender filter, daemon refuse 6. phase-2 security: dirfd-anchored force_change recovery 7. phase-3 security: secure_relative_open bound to dest subtree 8. --force-change rebalance: default to USR_IMMUTABLE only 9. set_refuse_options POPT_BIT_SET fix 10. user-visible regressions on non-chflags builds 11. t_stub fd-based stubs + weak curr_dir 12. Linux port via FS_IOC_{GET,SET}FLAGS Squashed and rebased because 3.4.3 (CVE-2026-29518 + family) overlaps heavily with the security work in phases 1-3 -- master added do_chmod_at / do_lchown_at / secure_relative_open hardening that duplicates the bits the fileflags patch needed. The fileflags work keeps the original do_chmod / do_lchown signatures (no UNUSED arg addition), so master's _at variants and call sites are unchanged. Resulting feature set: --fileflags preserve file flags (chflags on BSD, chattr {+d, +i, +a} on Linux). SAFE_FILEFLAGS mask (UF_NODUMP|UF_IMMUTABLE|UF_APPEND[|UF_HIDDEN]) applied by default; sender-supplied SF_* and UF_NOUNLINK dropped to avoid DoS where a hostile source pins permanent flags on the receiver. --unsafe-fileflags widens the mask to the full sender value. --force-change clear USR_IMMUTABLE (UF_*) on dest files being updated/deleted so the op can proceed. --force-uchange alias for --force-change. --force-schange also clear SYS_IMMUTABLE (SF_*); separate opt-in. --no-force-{u,s,}change to clear bits. Implementation: - lib/fileflags.c: portable rsync_fchflags / rsync_fgetflags / rsync_lgetflags / stat_x_get_fileflags + BSD<->Linux bit translation. Wire format is BSD bits. Linux side does read-modify-write of just LINUX_WIRE_MASK so the kernel doesn't reject the call when fs-internal bits like FS_EXTENT_FL would otherwise be cleared. - stat_x grows a (fileflags, fileflags_cached) pair so the per-file open()+ioctl cost on Linux happens at most once per stat_x life. init_stat_x() in ifuncs.h zeroes both. - syscall.c do_unlink / do_rmdir / do_chmod / do_lchown / do_rename grow dirfd-anchored force_change recovery using force_change_open_parent / force_change_open_target / fchflags / fchmod / fchown / unlinkat / renameat. The recovery opens via secure_relative_open(curr_dir, dirpart, O_RDONLY|O_DIRECTORY| O_NOFOLLOW) so RESOLVE_BENEATH (where available) bounds the operation to the destination subtree. Symlinks rejected at open. - Daemon mode refuses fileflags / unsafe-fileflags / force-change / force-uchange / force-schange / no-force-uchange / no-force-schange by default; opt-in per-module via "refuse options = !fileflags". - set_refuse_options POPT_BIT_SET / POPT_BIT_CLR fix: the original rsync check was `op->argInfo == POPT_ARG_VAL` literal, which missed POPT_BIT_SET (= POPT_ARG_VAL|POPT_ARGFLAG_OR). Refused bit-set options slipped through. Now masks via POPT_ARG_MASK. - testsuite/fileflags.test picks chflags(1) or chattr(1) depending on what's available; on Linux it parses lsattr down to the transferable letters (a, d, i, u). On Linux non-root the uchg portion self-skips (CAP_LINUX_IMMUTABLE required). - CI: fileflags removed from RSYNC_EXPECT_SKIPPED on Linux jobs; Cygwin keeps it (no chattr). - Linux ioctl path is gated behind autoconf check for FS_IOC_GETFLAGS / FS_IOC_SETFLAGS / FS_NODUMP_FL / FS_IMMUTABLE_FL / FS_APPEND_FL in ; falls through to "no fileflags support" if absent (older kernels, non-Linux non-BSD). Verified on Linux/ext4: 59 passed / 2 skipped / 0 failed. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/cygwin-build.yml | 2 +- Makefile.in | 13 +- compat.c | 10 +- configure.ac | 31 ++- delete.c | 15 ++ flist.c | 39 ++++ generator.c | 43 ++++ ifuncs.h | 4 + lib/fileflags.c | 202 +++++++++++++++++ log.c | 9 + main.c | 28 +++ options.c | 67 +++++- rsync.1.md | 61 +++++- rsync.c | 129 ++++++++++- rsync.h | 95 +++++++- syscall.c | 333 ++++++++++++++++++++++++++++- t_stub.c | 38 ++++ testsuite/fileflags.test | 148 +++++++++++++ testsuite/rsync.fns | 16 +- usage.c | 5 + util1.c | 40 ++++ 21 files changed, 1302 insertions(+), 26 deletions(-) create mode 100644 lib/fileflags.c create mode 100755 testsuite/fileflags.test diff --git a/.github/workflows/cygwin-build.yml b/.github/workflows/cygwin-build.yml index 781e46953..2beef871b 100644 --- a/.github/workflows/cygwin-build.yml +++ b/.github/workflows/cygwin-build.yml @@ -39,7 +39,7 @@ jobs: - name: info run: bash -c '/usr/local/bin/rsync --version' - name: check - run: bash -c 'RSYNC_EXPECT_SKIPPED=acls-default,acls,bare-do-open-symlink-race,chdir-symlink-race,chmod-symlink-race,chown,daemon-chroot-acl,devices,dir-sgid,open-noatime,protected-regular,sender-flist-symlink-leak,simd-checksum,symlink-dirlink-basis make check' + run: bash -c 'RSYNC_EXPECT_SKIPPED=acls-default,acls,bare-do-open-symlink-race,chdir-symlink-race,chmod-symlink-race,chown,daemon-chroot-acl,devices,dir-sgid,fileflags,open-noatime,protected-regular,sender-flist-symlink-leak,simd-checksum,symlink-dirlink-basis make check' - name: ssl file list run: bash -c 'PATH="/usr/local/bin:$PATH" rsync-ssl --no-motd download.samba.org::rsyncftp/ || true' - name: save artifact diff --git a/Makefile.in b/Makefile.in index 699d99562..961f48ed8 100644 --- a/Makefile.in +++ b/Makefile.in @@ -40,7 +40,8 @@ GENFILES=configure.sh aclocal.m4 config.h.in rsync.1 rsync.1.html \ HEADERS=byteorder.h config.h errcode.h proto.h rsync.h ifuncs.h itypes.h inums.h \ lib/pool_alloc.h lib/mdigest.h lib/md-defines.h LIBOBJ=lib/wildmatch.o lib/compat.o lib/snprintf.o lib/mdfour.o lib/md5.o \ - lib/permstring.o lib/pool_alloc.o lib/sysacls.o lib/sysxattrs.o @LIBOBJS@ + lib/permstring.o lib/pool_alloc.o lib/sysacls.o lib/sysxattrs.o \ + lib/fileflags.o @LIBOBJS@ zlib_OBJS=zlib/deflate.o zlib/inffast.o zlib/inflate.o zlib/inftrees.o \ zlib/trees.o zlib/zutil.o zlib/adler32.o zlib/compress.o zlib/crc32.o OBJS1=flist.o rsync.o generator.o receiver.o cleanup.o sender.o exclude.o \ @@ -53,7 +54,7 @@ popt_OBJS= popt/popt.o popt/poptconfig.o \ popt/popthelp.o popt/poptparse.o popt/poptint.o OBJS=$(OBJS1) $(OBJS2) $(OBJS3) $(DAEMON_OBJ) $(LIBOBJ) @BUILD_ZLIB@ @BUILD_POPT@ -TLS_OBJ = tls.o syscall.o util2.o t_stub.o lib/compat.o lib/snprintf.o lib/permstring.o lib/sysxattrs.o @BUILD_POPT@ +TLS_OBJ = tls.o syscall.o util2.o t_stub.o lib/compat.o lib/snprintf.o lib/permstring.o lib/sysxattrs.o lib/fileflags.o @BUILD_POPT@ # Programs we must have to run the test cases CHECK_PROGS = rsync$(EXEEXT) tls$(EXEEXT) getgroups$(EXEEXT) getfsdev$(EXEEXT) \ @@ -171,19 +172,19 @@ getgroups$(EXEEXT): getgroups.o getfsdev$(EXEEXT): getfsdev.o $(CC) $(CFLAGS) $(LDFLAGS) -o $@ getfsdev.o $(LIBS) -TRIMSLASH_OBJ = trimslash.o syscall.o util2.o t_stub.o lib/compat.o lib/snprintf.o +TRIMSLASH_OBJ = trimslash.o syscall.o util2.o t_stub.o lib/compat.o lib/snprintf.o lib/fileflags.o trimslash$(EXEEXT): $(TRIMSLASH_OBJ) $(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(TRIMSLASH_OBJ) $(LIBS) -T_UNSAFE_OBJ = t_unsafe.o syscall.o util1.o util2.o t_stub.o lib/compat.o lib/snprintf.o lib/wildmatch.o +T_UNSAFE_OBJ = t_unsafe.o syscall.o util1.o util2.o t_stub.o lib/compat.o lib/snprintf.o lib/wildmatch.o lib/fileflags.o 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_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 lib/fileflags.o t_chmod_secure$(EXEEXT): $(T_CHMOD_SECURE_OBJ) $(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(T_CHMOD_SECURE_OBJ) $(LIBS) -T_SECURE_RELPATH_OBJ = t_secure_relpath.o syscall.o util1.o util2.o t_stub.o lib/compat.o lib/snprintf.o lib/wildmatch.o lib/permstring.o +T_SECURE_RELPATH_OBJ = t_secure_relpath.o syscall.o util1.o util2.o t_stub.o lib/compat.o lib/snprintf.o lib/wildmatch.o lib/permstring.o lib/fileflags.o t_secure_relpath$(EXEEXT): $(T_SECURE_RELPATH_OBJ) $(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(T_SECURE_RELPATH_OBJ) $(LIBS) diff --git a/compat.c b/compat.c index d1ca31614..787688988 100644 --- a/compat.c +++ b/compat.c @@ -40,6 +40,7 @@ extern int checksum_seed; extern int basis_dir_cnt; extern int prune_empty_dirs; extern int protocol_version; +extern int force_change; extern int protect_args; extern int preserve_uid; extern int preserve_gid; @@ -47,6 +48,7 @@ extern int preserve_atimes; extern int preserve_crtimes; extern int preserve_acls; extern int preserve_xattrs; +extern int preserve_fileflags; extern int xfer_flags_as_varint; extern int need_messages_from_generator; extern int delete_mode, delete_before, delete_during, delete_after; @@ -87,7 +89,7 @@ struct name_num_item *xattr_sum_nni; int xattr_sum_len = 0; /* These index values are for the file-list's extra-attribute array. */ -int pathname_ndx, depth_ndx, atimes_ndx, crtimes_ndx, uid_ndx, gid_ndx, acls_ndx, xattrs_ndx, unsort_ndx; +int pathname_ndx, depth_ndx, atimes_ndx, crtimes_ndx, uid_ndx, gid_ndx, fileflags_ndx, acls_ndx, xattrs_ndx, unsort_ndx; int receiver_symlink_times = 0; /* receiver can set the time on a symlink */ int sender_symlink_iconv = 0; /* sender should convert symlink content */ @@ -589,6 +591,8 @@ void setup_protocol(int f_out,int f_in) uid_ndx = ++file_extra_cnt; if (preserve_gid) gid_ndx = ++file_extra_cnt; + if (preserve_fileflags || (force_change && !am_sender)) + fileflags_ndx = ++file_extra_cnt; if (preserve_acls && !am_sender) acls_ndx = ++file_extra_cnt; if (preserve_xattrs) @@ -752,6 +756,10 @@ void setup_protocol(int f_out,int f_in) fprintf(stderr, "Both rsync versions must be at least 3.2.0 for --crtimes.\n"); exit_cleanup(RERR_PROTOCOL); } + if (!xfer_flags_as_varint && preserve_fileflags) { + fprintf(stderr, "Both rsync versions must be at least 3.2.0 for --fileflags.\n"); + exit_cleanup(RERR_PROTOCOL); + } if (am_sender) { receiver_symlink_times = am_server ? strchr(client_info, 'L') != NULL diff --git a/configure.ac b/configure.ac index 4062651df..6370b6a93 100644 --- a/configure.ac +++ b/configure.ac @@ -13,7 +13,7 @@ AC_CHECK_HEADERS(sys/fcntl.h sys/select.h fcntl.h sys/time.h sys/unistd.h \ sys/acl.h acl/libacl.h attr/xattr.h sys/xattr.h sys/extattr.h dl.h \ popt.h popt/popt.h linux/falloc.h netinet/in_systm.h netgroup.h \ zlib.h xxhash.h openssl/md4.h openssl/md5.h zstd.h lz4.h sys/file.h \ - bsd/string.h) + bsd/string.h linux/fs.h) AC_CHECK_HEADERS([netinet/ip.h], [], [], [[#include ]]) AC_HEADER_MAJOR_FIXED @@ -928,6 +928,35 @@ AC_PREPROC_IFELSE([AC_LANG_SOURCE([[ ] ) +AC_MSG_CHECKING([for FS_IOC_GETFLAGS]) +AC_PREPROC_IFELSE([AC_LANG_SOURCE([[ + #include + #ifdef HAVE_LINUX_FS_H + #include + #endif + #ifndef FS_IOC_GETFLAGS + #error FS_IOC_GETFLAGS is missing + #endif + #ifndef FS_IOC_SETFLAGS + #error FS_IOC_SETFLAGS is missing + #endif + #ifndef FS_NODUMP_FL + #error FS_NODUMP_FL is missing + #endif + #ifndef FS_IMMUTABLE_FL + #error FS_IMMUTABLE_FL is missing + #endif + #ifndef FS_APPEND_FL + #error FS_APPEND_FL is missing + #endif + ]])], [ + AC_MSG_RESULT([yes]) + AC_DEFINE([HAVE_FS_IOC_GETFLAGS], [1], [Define if FS_IOC_GETFLAGS / FS_IOC_SETFLAGS ioctls are available (Linux chattr-style file flags).]) + ], [ + AC_MSG_RESULT([no]) + ] +) + AC_MSG_CHECKING([for FALLOC_FL_ZERO_RANGE]) AC_PREPROC_IFELSE([AC_LANG_SOURCE([[ #define _GNU_SOURCE 1 diff --git a/delete.c b/delete.c index 4a52122d3..d8408ae21 100644 --- a/delete.c +++ b/delete.c @@ -25,6 +25,7 @@ extern int am_root; extern int make_backups; extern int max_delete; +extern int force_change; extern char *backup_dir; extern char *backup_suffix; extern int backup_suffix_len; @@ -97,6 +98,10 @@ static enum delret delete_dir_contents(char *fname, uint16 flags) } strlcpy(p, fp->basename, remainder); +#ifdef SUPPORT_FORCE_CHANGE + if (force_change) + make_mutable(fname, fp->mode, F_FFLAGS(fp), force_change); +#endif if (!(fp->mode & S_IWUSR) && !am_root && fp->flags & FLAG_OWNED_BY_US) do_chmod_at(fname, fp->mode | S_IWUSR); /* Save stack by recursing to ourself directly. */ @@ -144,6 +149,16 @@ enum delret delete_item(char *fbuf, uint16 mode, uint16 flags) if (S_ISDIR(mode) && !(flags & DEL_DIR_IS_EMPTY)) { /* This only happens on the first call to delete_item() since * delete_dir_contents() always calls us w/DEL_DIR_IS_EMPTY. */ +#ifdef SUPPORT_FORCE_CHANGE + if (force_change) { + STRUCT_STAT st; + if (x_lstat(fbuf, &st, NULL) == 0) { + uint32 ff = rsync_lgetflags(fbuf, st.st_mode, &st); + if (ff != NO_FFLAGS) + make_mutable(fbuf, st.st_mode, ff, force_change); + } + } +#endif ignore_perishable = 1; /* If DEL_RECURSE is not set, this just reports emptiness. */ ret = delete_dir_contents(fbuf, flags); diff --git a/flist.c b/flist.c index 2ec07f54a..adab40505 100644 --- a/flist.c +++ b/flist.c @@ -52,6 +52,7 @@ extern int preserve_links; extern int preserve_hard_links; extern int preserve_devices; extern int preserve_specials; +extern int preserve_fileflags; extern int delete_during; extern int missing_args; extern int eol_nulls; @@ -388,6 +389,9 @@ static void send_file_entry(int f, const char *fname, struct file_struct *file, static time_t crtime; #endif static mode_t mode; +#ifdef SUPPORT_FILEFLAGS + static uint32 fileflags; +#endif #ifdef SUPPORT_HARD_LINKS static int64 dev; #endif @@ -431,6 +435,14 @@ static void send_file_entry(int f, const char *fname, struct file_struct *file, xflags |= XMIT_SAME_MODE; else mode = file->mode; +#ifdef SUPPORT_FILEFLAGS + if (preserve_fileflags) { + if (F_FFLAGS(file) == fileflags) + xflags |= XMIT_SAME_FLAGS; + else + fileflags = F_FFLAGS(file); + } +#endif if (preserve_devices && IS_DEVICE(mode)) { if (protocol_version < 28) { @@ -592,6 +604,10 @@ static void send_file_entry(int f, const char *fname, struct file_struct *file, #endif if (!(xflags & XMIT_SAME_MODE)) write_int(f, to_wire_mode(mode)); +#ifdef SUPPORT_FILEFLAGS + if (preserve_fileflags && !(xflags & XMIT_SAME_FLAGS)) + write_int(f, (int)fileflags); +#endif if (atimes_ndx && !S_ISDIR(mode) && !(xflags & XMIT_SAME_ATIME)) write_varlong(f, atime, 4); if (preserve_uid && !(xflags & XMIT_SAME_UID)) { @@ -686,6 +702,9 @@ static struct file_struct *recv_file_entry(int f, struct file_list *flist, int x static time_t crtime; #endif static mode_t mode; +#ifdef SUPPORT_FILEFLAGS + static uint32 fileflags; +#endif #ifdef SUPPORT_HARD_LINKS static int64 dev; #endif @@ -803,6 +822,10 @@ static struct file_struct *recv_file_entry(int f, struct file_list *flist, int x #ifdef SUPPORT_CRTIMES if (crtimes_ndx) crtime = F_CRTIME(first); +#endif +#ifdef SUPPORT_FILEFLAGS + if (preserve_fileflags) + fileflags = F_FFLAGS(first); #endif if (preserve_uid) uid = F_OWNER(first); @@ -887,6 +910,10 @@ static struct file_struct *recv_file_entry(int f, struct file_list *flist, int x if (chmod_modes && !S_ISLNK(mode) && mode) mode = tweak_mode(mode, chmod_modes); +#ifdef SUPPORT_FILEFLAGS + if (preserve_fileflags && !(xflags & XMIT_SAME_FLAGS)) + fileflags = (uint32)read_int(f); +#endif if (preserve_uid && !(xflags & XMIT_SAME_UID)) { if (protocol_version < 30) @@ -1068,6 +1095,10 @@ static struct file_struct *recv_file_entry(int f, struct file_list *flist, int x } #endif file->mode = mode; +#ifdef SUPPORT_FILEFLAGS + if (preserve_fileflags) + F_FFLAGS(file) = fileflags; +#endif if (preserve_uid) F_OWNER(file) = uid; if (preserve_gid) { @@ -1481,6 +1512,14 @@ struct file_struct *make_file(const char *fname, struct file_list *flist, } #endif file->mode = st.st_mode; +#if defined SUPPORT_FILEFLAGS || defined SUPPORT_FORCE_CHANGE + if (fileflags_ndx) { + /* On BSD st.st_flags is free; on Linux the helper does an + * open()+ioctl() per file -- but only when fileflags_ndx is + * set, which only happens with --fileflags / --force-change. */ + F_FFLAGS(file) = rsync_lgetflags(fname, st.st_mode, &st); + } +#endif if (preserve_uid) F_OWNER(file) = st.st_uid; if (preserve_gid) diff --git a/generator.c b/generator.c index 4d4ae72e3..9199739a7 100644 --- a/generator.c +++ b/generator.c @@ -43,10 +43,13 @@ extern int preserve_devices; extern int preserve_specials; extern int preserve_hard_links; extern int preserve_executability; +extern int preserve_fileflags; +extern int preserve_unsafe_fileflags; extern int preserve_perms; extern int preserve_mtimes; extern int omit_dir_times; extern int omit_link_times; +extern int force_change; extern int delete_mode; extern int delete_before; extern int delete_during; @@ -492,6 +495,13 @@ int unchanged_attrs(const char *fname, struct file_struct *file, stat_x *sxp) return 0; if (perms_differ(file, sxp)) return 0; +#ifdef SUPPORT_FILEFLAGS + if (preserve_fileflags) { + uint32 current = stat_x_get_fileflags(sxp, fname); + if (current != filter_recv_fileflags(F_FFLAGS(file), current)) + return 0; + } +#endif if (ownership_differs(file, sxp)) return 0; #ifdef SUPPORT_ACLS @@ -553,6 +563,13 @@ void itemize(const char *fnamecmp, struct file_struct *file, int ndx, int statre iflags |= ITEM_REPORT_OWNER; if (gid_ndx && !(file->flags & FLAG_SKIP_GROUP) && sxp->st.st_gid != (gid_t)F_GROUP(file)) iflags |= ITEM_REPORT_GROUP; +#ifdef SUPPORT_FILEFLAGS + if (preserve_fileflags && !S_ISLNK(file->mode)) { + uint32 current = stat_x_get_fileflags(sxp, fnamecmp); + if (current != filter_recv_fileflags(F_FFLAGS(file), current)) + iflags |= ITEM_REPORT_FFLAGS; + } +#endif #ifdef SUPPORT_ACLS if (preserve_acls && !S_ISLNK(file->mode)) { if (!ACL_READY(*sxp)) @@ -1460,6 +1477,10 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx, if (!preserve_perms) { /* See comment in non-dir code below. */ file->mode = dest_mode(file->mode, sx.st.st_mode, dflt_perms, statret == 0); } +#ifdef SUPPORT_FORCE_CHANGE + if (force_change && !preserve_fileflags) + F_FFLAGS(file) = stat_x_get_fileflags(&sx, fname); +#endif if (statret != 0 && basis_dir[0] != NULL) { int j = try_dests_non(file, fname, ndx, fnamecmpbuf, &sx, itemizing, code); if (j == -2) { @@ -1502,6 +1523,11 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx, * readable and writable permissions during the time we are * putting files within them. This is then restored to the * former permissions after the transfer is done. */ +#ifdef SUPPORT_FORCE_CHANGE + if (force_change && F_FFLAGS(file) & force_change + && make_mutable(fname, file->mode, F_FFLAGS(file), force_change)) + need_retouch_dir_perms = 1; +#endif #ifdef HAVE_CHMOD if (!am_root && (file->mode & S_IRWXU) != S_IRWXU && dir_tweaking) { mode_t mode = file->mode | S_IRWXU; @@ -1540,6 +1566,10 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx, int exists = statret == 0 && stype != FT_DIR; file->mode = dest_mode(file->mode, sx.st.st_mode, dflt_perms, exists); } +#ifdef SUPPORT_FORCE_CHANGE + if (force_change && !preserve_fileflags) + F_FFLAGS(file) = stat_x_get_fileflags(&sx, fname); +#endif #ifdef SUPPORT_HARD_LINKS if (preserve_hard_links && F_HLINK_NOT_FIRST(file) @@ -2124,10 +2154,23 @@ static void touch_up_dirs(struct file_list *flist, int ndx) st.st_mtime = file->modtime; #ifdef ST_MTIME_NSEC st.ST_MTIME_NSEC = F_MOD_NSEC_or_0(file); +#endif +#ifdef SUPPORT_FORCE_CHANGE + st.st_mode = file->mode; +#ifdef HAVE_CHFLAGS + /* Tell set_times' try_a_force_change there's nothing + * to bypass yet; the real flags come from rsync_lgetflags. + * On Linux STRUCT_STAT has no st_flags so this is a no-op. */ + st.st_flags = 0; +#endif #endif set_times(fname, &st); } } +#ifdef SUPPORT_FORCE_CHANGE + if (force_change && F_FFLAGS(file) & force_change) + undo_make_mutable(fname, F_FFLAGS(file)); +#endif if (counter >= loopchk_limit) { if (allowed_lull) maybe_send_keepalive(time(NULL), MSK_ALLOW_FLUSH); diff --git a/ifuncs.h b/ifuncs.h index 956fc22eb..4f1187066 100644 --- a/ifuncs.h +++ b/ifuncs.h @@ -76,6 +76,10 @@ static inline void init_stat_x(stat_x *sx_p) { sx_p->crtime = 0; +#ifdef SUPPORT_FILEFLAGS + sx_p->fileflags = 0; + sx_p->fileflags_cached = 0; +#endif #ifdef SUPPORT_ACLS sx_p->acc_acl = sx_p->def_acl = NULL; #endif diff --git a/lib/fileflags.c b/lib/fileflags.c new file mode 100644 index 000000000..c9baf06ca --- /dev/null +++ b/lib/fileflags.c @@ -0,0 +1,202 @@ +/* + * Portable BSD-st_flags / Linux-chattr abstraction. + * + * The rsync --fileflags / --force-change family was originally written + * for the BSD chflags(2) family. This file provides a small wrapper + * layer so that Linux's FS_IOC_GETFLAGS / FS_IOC_SETFLAGS ioctls + * present the same interface to the rest of rsync, with a single + * translation point between the two on-disk bit conventions: + * + * BSD UF_NODUMP <-> Linux FS_NODUMP_FL (chattr +d) + * BSD UF_IMMUTABLE <-> Linux FS_IMMUTABLE_FL (chattr +i) + * BSD SF_IMMUTABLE -- mapped to FS_IMMUTABLE_FL on Linux + * BSD UF_APPEND <-> Linux FS_APPEND_FL (chattr +a) + * BSD SF_APPEND -- mapped to FS_APPEND_FL on Linux + * BSD UF_NOUNLINK / SF_NOUNLINK -- no Linux equivalent (the chattr + * 'u' flag exists in the kernel API but no mainline filesystem + * implements it). Dropped on Linux receivers. + * + * The BSD bit layout is the on-the-wire canonical: senders fill BSD + * bits, receivers consume BSD bits, and the translation lives at the + * platform boundary inside this file. That keeps interop with the + * already-shipped BSD --fileflags rsync. + * + * SECURITY: all functions that touch flags operate on an open fd + * (callers use openat(O_NOFOLLOW) or secure_relative_open first), so + * the TOCTOU race that the path-based chflags(2) exposed is gone. + * Symlinks aren't a meaningful target for either chattr or BSD + * chflags semantics here, so they short-circuit to a no-op. + */ + +#include "rsync.h" + +#ifdef SUPPORT_FILEFLAGS + +#include +#ifdef HAVE_LINUX_FS_H +#include +#endif + +#ifdef HAVE_CHFLAGS +/* ---------- BSD / macOS path: native chflags(2) family ---------- */ + +int rsync_fchflags(int fd, UNUSED(mode_t mode), uint32 bsd_flags) +{ + return fchflags(fd, bsd_flags); +} + +uint32 rsync_fgetflags(int fd, UNUSED(mode_t mode), const STRUCT_STAT *hint) +{ + STRUCT_STAT st; + if (hint) + return hint->st_flags; + if (fstat(fd, &st) != 0) + return NO_FFLAGS; + return st.st_flags; +} + +uint32 rsync_lgetflags(UNUSED(const char *path), UNUSED(mode_t mode), const STRUCT_STAT *hint) +{ + /* On BSD the value is free from lstat -- the caller already + * has it in the stat buffer. */ + if (hint) + return hint->st_flags; + { + STRUCT_STAT st; + if (lstat(path, &st) != 0) + return NO_FFLAGS; + return st.st_flags; + } +} + +#elif defined HAVE_FS_IOC_GETFLAGS +/* ---------- Linux path: FS_IOC_{GET,SET}FLAGS ioctls ---------- */ + +static long bsd_flags_to_linux(uint32 bf) +{ + long lf = 0; + if (bf & UF_NODUMP) lf |= FS_NODUMP_FL; + if (bf & (UF_IMMUTABLE | SF_IMMUTABLE)) lf |= FS_IMMUTABLE_FL; + if (bf & (UF_APPEND | SF_APPEND)) lf |= FS_APPEND_FL; + /* UF_NOUNLINK / SF_NOUNLINK have no Linux equivalent -- dropped. */ + return lf; +} + +static uint32 linux_flags_to_bsd(long lf) +{ + uint32 bf = 0; + /* Map back into UF_* (user class). Linux has no user/system split + * and most fs implementations require CAP_LINUX_IMMUTABLE for the + * immutable/append flags anyway, but UF_* is the safer wire choice + * for cross-platform interop -- a BSD receiver getting UF_* will + * honour SAFE_FILEFLAGS, while SF_* would require explicit + * --unsafe-fileflags on a non-Linux receiver. */ + if (lf & FS_NODUMP_FL) bf |= UF_NODUMP; + if (lf & FS_IMMUTABLE_FL) bf |= UF_IMMUTABLE; + if (lf & FS_APPEND_FL) bf |= UF_APPEND; + return bf; +} + +/* Filesystems that don't support the chattr ioctls return ENOTTY, + * EOPNOTSUPP, EINVAL, or sometimes ENOSYS depending on kernel/fs. + * Treat any of those as "no flags here" rather than as an error. */ +static int errno_is_no_chattr(int e) +{ + return e == ENOTTY || e == EOPNOTSUPP || e == EINVAL +#ifdef ENOSYS + || e == ENOSYS +#endif + ; +} + +/* Mask of the Linux flag bits we manage (and that map to the BSD wire). + * Read-modify-write: we read the current ioctl flags, clear these bits, + * then OR in our translated bits. Bits outside this mask (FS_EXTENT_FL, + * FS_HUGE_FILE_FL, FS_INLINE_DATA_FL, FS_INDEX_FL, FS_ENCRYPT_FL, ...) are + * fs-internal / read-only and will be rejected by FS_IOC_SETFLAGS if we + * try to flip them. Preserving them is mandatory. */ +#define LINUX_WIRE_MASK (FS_NODUMP_FL | FS_IMMUTABLE_FL | FS_APPEND_FL) + +int rsync_fchflags(int fd, mode_t mode, uint32 bsd_flags) +{ + long cur_lf, new_lf; + /* chattr doesn't apply to symlinks / devices / fifos / sockets. */ + if (S_ISLNK(mode) || !(S_ISREG(mode) || S_ISDIR(mode))) { + errno = EINVAL; + return -1; + } + if (ioctl(fd, FS_IOC_GETFLAGS, &cur_lf) != 0) { + if (errno_is_no_chattr(errno) && bsd_flags == 0) + return 0; /* nothing to do, fs doesn't support flags */ + return -1; + } + new_lf = (cur_lf & ~LINUX_WIRE_MASK) | bsd_flags_to_linux(bsd_flags); + if (new_lf == cur_lf) + return 0; /* no managed bits change */ + if (ioctl(fd, FS_IOC_SETFLAGS, &new_lf) == 0) + return 0; + if (errno_is_no_chattr(errno) && (new_lf & LINUX_WIRE_MASK) == (cur_lf & LINUX_WIRE_MASK)) + return 0; /* fs refuses but nothing we manage actually differs */ + return -1; +} + +uint32 rsync_fgetflags(int fd, mode_t mode, UNUSED(const STRUCT_STAT *hint)) +{ + long lf; + if (S_ISLNK(mode) || !(S_ISREG(mode) || S_ISDIR(mode))) + return 0; + if (ioctl(fd, FS_IOC_GETFLAGS, &lf) != 0) { + if (errno_is_no_chattr(errno)) + return 0; + return NO_FFLAGS; + } + return linux_flags_to_bsd(lf); +} + +uint32 rsync_lgetflags(const char *path, mode_t mode, UNUSED(const STRUCT_STAT *hint)) +{ + int fd; + uint32 bf; + int oflags = O_RDONLY | O_NOFOLLOW; + + if (S_ISLNK(mode) || !(S_ISREG(mode) || S_ISDIR(mode))) + return 0; +#ifdef O_NONBLOCK + oflags |= O_NONBLOCK; +#endif +#ifdef O_DIRECTORY + if (S_ISDIR(mode)) + oflags |= O_DIRECTORY; +#endif + fd = open(path, oflags); + if (fd < 0) + return 0; /* can't open -> treat as no flags rather than failing */ + bf = rsync_fgetflags(fd, mode, NULL); + close(fd); + return bf == NO_FFLAGS ? 0 : bf; +} + +#endif /* HAVE_CHFLAGS / HAVE_FS_IOC_GETFLAGS */ + +/* ---------- Cached accessor on stat_x (both platforms) ---------- */ + +/* Read the cached fileflags off a stat_x, populating from the lstat + * (BSD) or via an open()+ioctl (Linux) on first access. */ +uint32 stat_x_get_fileflags(stat_x *sxp, const char *path) +{ + if (sxp->fileflags_cached) + return sxp->fileflags; + sxp->fileflags = rsync_lgetflags(path, sxp->st.st_mode, &sxp->st); + sxp->fileflags_cached = 1; + return sxp->fileflags; +} + +/* Invalidate the cache after a successful fchflags / chmod / chown + * that may have changed (or freshly-applied) the inode's flags. */ +void stat_x_invalidate_fileflags(stat_x *sxp) +{ + sxp->fileflags_cached = 0; + sxp->fileflags = 0; +} + +#endif /* SUPPORT_FILEFLAGS */ diff --git a/log.c b/log.c index b948f16a1..dc6c05b20 100644 --- a/log.c +++ b/log.c @@ -731,7 +731,16 @@ static void log_formatted(enum logcode code, const char *format, const char *op, : iflags & ITEM_REPORT_ATIME ? 'u' : 'n'; c[9] = !(iflags & ITEM_REPORT_ACL) ? '.' : 'a'; c[10] = !(iflags & ITEM_REPORT_XATTR) ? '.' : 'x'; +#ifdef SUPPORT_FILEFLAGS + /* The 'f' column is only emitted when the build has + * fileflags support, so non-chflags builds keep the + * historical 11-char %i format and existing scripts + * that parse --itemize-changes don't see a wider line. */ + c[11] = !(iflags & ITEM_REPORT_FFLAGS) ? '.' : 'f'; + c[12] = '\0'; +#else c[11] = '\0'; +#endif if (iflags & (ITEM_IS_NEW|ITEM_MISSING_DATA)) { char ch = iflags & ITEM_IS_NEW ? '+' : '?'; diff --git a/main.c b/main.c index 78f0b8331..be9abc920 100644 --- a/main.c +++ b/main.c @@ -31,6 +31,13 @@ #ifdef __TANDEM #include #endif +#if defined SUPPORT_FORCE_CHANGE && defined HAVE_CHFLAGS +/* The securelevel check below uses BSD sysctl(KERN_SECURELVL) -- only + * relevant on systems with the BSD chflags(2) family. Linux has no + * equivalent of securelevel for the immutable bits, so this whole + * block is BSD-only. */ +#include +#endif extern int dry_run; extern int list_only; @@ -49,6 +56,7 @@ extern int need_messages_from_generator; extern int kluge_around_eof; extern int got_xfer_error; extern int old_style_args; +extern int force_change; extern int msgs2stderr; extern int module_id; extern int read_only; @@ -986,6 +994,26 @@ static int do_recv(int f_in, int f_out, char *local_name) * points to an identical file won't be replaced by the referent. */ copy_links = copy_dirlinks = copy_unsafe_links = 0; +#if defined SUPPORT_FORCE_CHANGE && defined HAVE_CHFLAGS + if (force_change & SYS_IMMUTABLE) { + /* BSD-only: determine whether we'll be able to unlock a system + * immutable item. Linux has no equivalent of securelevel for + * its chattr +i (it requires CAP_LINUX_IMMUTABLE rather than + * a kernel-wide securelevel knob), and CAP_LINUX_IMMUTABLE + * either is or is not set on this process. */ + int mib[2]; + int securityLevel = 0; + size_t len = sizeof securityLevel; + + mib[0] = CTL_KERN; + mib[1] = KERN_SECURELVL; + if (sysctl(mib, 2, &securityLevel, &len, NULL, 0) == 0 && securityLevel > 0) { + rprintf(FERROR, "System security level is too high to force mutability on system immutable files and directories.\n"); + exit_cleanup(RERR_UNSUPPORTED); + } + } +#endif + #ifdef SUPPORT_HARD_LINKS if (preserve_hard_links && !inc_recurse) match_hard_links(first_flist); diff --git a/options.c b/options.c index 3c2d23526..0d6e7d7ca 100644 --- a/options.c +++ b/options.c @@ -56,6 +56,8 @@ int preserve_hard_links = 0; int preserve_acls = 0; int preserve_xattrs = 0; int preserve_perms = 0; +int preserve_fileflags = 0; +int preserve_unsafe_fileflags = 0; int preserve_executability = 0; int preserve_devices = 0; int preserve_specials = 0; @@ -99,6 +101,7 @@ int msgs2stderr = 2; /* Default: send errors to stderr for local & remote-shell int saw_stderr_opt = 0; int allow_8bit_chars = 0; int force_delete = 0; +int force_change = 0; int io_timeout = 0; int prune_empty_dirs = 0; int use_qsort = 0; @@ -633,6 +636,10 @@ static struct poptOption long_options[] = { {"perms", 'p', POPT_ARG_VAL, &preserve_perms, 1, 0, 0 }, {"no-perms", 0, POPT_ARG_VAL, &preserve_perms, 0, 0, 0 }, {"no-p", 0, POPT_ARG_VAL, &preserve_perms, 0, 0, 0 }, + {"fileflags", 0, POPT_ARG_VAL, &preserve_fileflags, 1, 0, 0 }, + {"no-fileflags", 0, POPT_ARG_VAL, &preserve_fileflags, 0, 0, 0 }, + {"unsafe-fileflags", 0, POPT_ARG_VAL, &preserve_unsafe_fileflags, 1, 0, 0 }, + {"no-unsafe-fileflags",0,POPT_ARG_VAL, &preserve_unsafe_fileflags, 0, 0, 0 }, {"executability", 'E', POPT_ARG_NONE, &preserve_executability, 0, 0, 0 }, {"acls", 'A', POPT_ARG_NONE, 0, 'A', 0, 0 }, {"no-acls", 0, POPT_ARG_VAL, &preserve_acls, 0, 0, 0 }, @@ -731,6 +738,19 @@ static struct poptOption long_options[] = { {"remove-source-files",0,POPT_ARG_VAL, &remove_source_files, 1, 0, 0 }, {"force", 0, POPT_ARG_VAL, &force_delete, 1, 0, 0 }, {"no-force", 0, POPT_ARG_VAL, &force_delete, 0, 0, 0 }, + {"force-delete", 0, POPT_ARG_VAL, &force_delete, 1, 0, 0 }, + {"no-force-delete", 0, POPT_ARG_VAL, &force_delete, 0, 0, 0 }, + /* --force-change defaults to USR_IMMUTABLE only (the safer set -- + * UF_*). System-class flags require an explicit --force-schange. + * USR + SYS are additive: combine with "--force-change --force-schange" + * (or "--force-uchange --force-schange"); use --no-force-change to clear + * everything. */ + {"force-change", 0, POPT_BIT_SET, &force_change, USR_IMMUTABLE, 0, 0 }, + {"no-force-change", 0, POPT_ARG_VAL, &force_change, 0, 0, 0 }, + {"force-uchange", 0, POPT_BIT_SET, &force_change, USR_IMMUTABLE, 0, 0 }, + {"no-force-uchange", 0, POPT_BIT_CLR, &force_change, USR_IMMUTABLE, 0, 0 }, + {"force-schange", 0, POPT_BIT_SET, &force_change, SYS_IMMUTABLE, 0, 0 }, + {"no-force-schange", 0, POPT_BIT_CLR, &force_change, SYS_IMMUTABLE, 0, 0 }, {"ignore-errors", 0, POPT_ARG_VAL, &ignore_errors, 1, 0, 0 }, {"no-ignore-errors", 0, POPT_ARG_VAL, &ignore_errors, 0, 0, 0 }, {"max-delete", 0, POPT_ARG_INT, &max_delete, 0, 0, 0 }, @@ -984,6 +1004,22 @@ static void set_refuse_options(void) if (am_daemon) { /* Refused by default, but can be accepted via a negated exact match. */ parse_one_refuse_match(0, "copy-devices", list_end); parse_one_refuse_match(0, "write-devices", list_end); +#ifdef SUPPORT_FILEFLAGS + /* A daemon shouldn't apply sender-controlled st_flags or clear + * receiver-side immutability by default -- both are foot-guns + * for the daemon admin and DoS vectors for the daemon's + * filesystem. Admin can opt-in per-module with "refuse + * options = !fileflags" etc. */ + parse_one_refuse_match(0, "fileflags", list_end); + parse_one_refuse_match(0, "unsafe-fileflags", list_end); +#endif +#ifdef SUPPORT_FORCE_CHANGE + parse_one_refuse_match(0, "force-change", list_end); + parse_one_refuse_match(0, "force-uchange", list_end); + parse_one_refuse_match(0, "force-schange", list_end); + parse_one_refuse_match(0, "no-force-uchange", list_end); + parse_one_refuse_match(0, "no-force-schange", list_end); +#endif } while (1) { @@ -1028,6 +1064,17 @@ static void set_refuse_options(void) #ifndef SUPPORT_CRTIMES parse_one_refuse_match(0, "crtimes", list_end); #endif +#ifndef SUPPORT_FILEFLAGS + parse_one_refuse_match(0, "fileflags", list_end); + parse_one_refuse_match(0, "unsafe-fileflags", list_end); +#endif +#ifndef SUPPORT_FORCE_CHANGE + parse_one_refuse_match(0, "force-change", list_end); + parse_one_refuse_match(0, "force-uchange", list_end); + parse_one_refuse_match(0, "force-schange", list_end); + parse_one_refuse_match(0, "no-force-uchange", list_end); + parse_one_refuse_match(0, "no-force-schange", list_end); +#endif /* Now we use the descrip values to actually mark the options for refusal. */ for (op = long_options; op != list_end; op++) { @@ -1035,7 +1082,12 @@ static void set_refuse_options(void) op->descrip = NULL; if (!refused) continue; - if (op->argInfo == POPT_ARG_VAL) + /* Mask off any POPT_ARGFLAG_* bits so we also catch + * POPT_BIT_SET (POPT_ARG_VAL|POPT_ARGFLAG_OR) and + * POPT_BIT_CLR (POPT_ARG_VAL|POPT_ARGFLAG_NAND); otherwise + * those slip through the refuse path because popt silently + * performs the bit op and never returns the overridden val. */ + if ((op->argInfo & POPT_ARG_MASK) == POPT_ARG_VAL) op->argInfo = POPT_ARG_NONE; op->val = (op - long_options) + OPT_REFUSED_BASE; /* The following flags are set to let us easily check an implied option later in the code. */ @@ -2752,6 +2804,11 @@ void server_options(char **args, int *argc_p) if (xfer_dirs && !recurse && delete_mode && am_sender) args[ac++] = "--no-r"; + if (preserve_fileflags) + args[ac++] = "--fileflags"; + if (preserve_unsafe_fileflags) + args[ac++] = "--unsafe-fileflags"; + if (do_compression && do_compression_level != CLVL_NOT_SPECIFIED) { if (asprintf(&arg, "--compress-level=%d", do_compression_level) < 0) goto oom; @@ -2847,6 +2904,14 @@ void server_options(char **args, int *argc_p) args[ac++] = "--delete-excluded"; if (force_delete) args[ac++] = "--force"; +#ifdef SUPPORT_FORCE_CHANGE + /* Propagate USR/SYS bits independently -- the receiver-side + * options are additive, so both can be passed together. */ + if (force_change & USR_IMMUTABLE) + args[ac++] = "--force-uchange"; + if (force_change & SYS_IMMUTABLE) + args[ac++] = "--force-schange"; +#endif if (write_batch < 0) args[ac++] = "--only-write-batch=X"; if (am_root > 1) diff --git a/rsync.1.md b/rsync.1.md index 2b4b75087..dd3b50670 100644 --- a/rsync.1.md +++ b/rsync.1.md @@ -446,6 +446,7 @@ has its own detailed description later in this manpage. --keep-dirlinks, -K treat symlinked dir on receiver as dir --hard-links, -H preserve hard links --perms, -p preserve permissions +--fileflags preserve file-flags (aka chflags) --executability, -E preserve executability --chmod=CHMOD affect file and/or directory permissions --acls, -A preserve ACLs (implies --perms) @@ -488,6 +489,8 @@ has its own detailed description later in this manpage. --delete-missing-args delete missing source args from destination --ignore-errors delete even if there are I/O errors --force force deletion of dirs even if not empty +--force-change affect user-immutable files/dirs (alias --force-uchange) +--force-schange also affect system-immutable files/dirs --max-delete=NUM don't delete more than NUM files --max-size=SIZE don't transfer any file larger than SIZE --min-size=SIZE don't transfer any file smaller than SIZE @@ -832,6 +835,7 @@ expand it. recursion and want to preserve almost everything. Be aware that it does **not** include preserving ACLs (`-A`), xattrs (`-X`), atimes (`-U`), crtimes (`-N`), nor the finding and preserving of hardlinks (`-H`). + It also does **not** imply [`--fileflags`](#opt). The only exception to the above equivalence is when [`--files-from`](#opt) is specified, in which case [`-r`](#opt) is not implied. @@ -1491,6 +1495,51 @@ expand it. those used by [`--fake-super`](#opt)) unless you repeat the option (e.g. `-XX`). This "copy all xattrs" mode cannot be used with [`--fake-super`](#opt). +0. `--fileflags` + + This option causes rsync to update the file-flags to be the same as the + source files and directories (if your OS supports the **chflags**(2) system + call). Some flags can only be altered by the super-user and some might + only be unset below a certain secure-level (usually single-user mode). It + will not make files alterable that are set to immutable on the receiver. + To do that, see [`--force-change`](#opt), [`--force-uchange`](#opt), and + [`--force-schange`](#opt). + +0. `--force-change`, `--force-uchange` + + This option causes rsync to temporarily clear the **user-immutable** + flags (`UF_IMMUTABLE`, `UF_APPEND`, `UF_NOUNLINK`) on files and + directories on the receiving side that are being updated or deleted, + so that the update can proceed. The original flags are restored + after the operation. By default it does **not** affect + system-immutable (`SF_*`) flags -- those require root, are + securelevel-locked on BSD, and a misdirected clear can permanently + weaken receiver protections; combine with [`--force-schange`](#opt) + if you really want that behaviour. + `--force-uchange` is an alias for `--force-change`. + Use `--no-force-change` to clear both classes back to off. + Use `--no-force-uchange` to clear only the user-class bit, + leaving any [`--force-schange`](#opt) in effect. + + Note: earlier patched rsync versions had `--force-change` clear both + user- and system-immutable flags. The current default is user-class + only; pass `--force-change --force-schange` to recover the previous + behaviour. + +0. `--force-schange` + + This option causes rsync to temporarily clear the **system-immutable** + flags (`SF_IMMUTABLE`, `SF_APPEND`, `SF_NOUNLINK`) on files and + directories on the receiving side that are being updated or deleted, + restoring the original flags afterwards. Operating on system flags + requires root, and on BSD systems running at securelevel >= 1 these + flags cannot be cleared at all -- the option will fail at startup + if it can't do its job. Use with care: a misdirected clear on a + security-relevant file is harder to recover from than the + user-immutable equivalent. + Use `--no-force-schange` to clear only this bit, leaving + any [`--force-change`](#opt) / [`--force-uchange`](#opt) in effect. + 0. `--chmod=CHMOD` This option tells rsync to apply one or more comma-separated "chmod" modes @@ -2020,8 +2069,8 @@ expand it. [`--ignore-missing-args`](#opt) option a step farther: each missing arg will become a deletion request of the corresponding destination file on the receiving side (should it exist). If the destination file is a non-empty - directory, it will only be successfully deleted if [`--force`](#opt) or - [`--delete`](#opt) are in effect. Other than that, this option is + directory, it will only be successfully deleted if [`--force`](#opt) + or [`--delete`](#opt) are in effect. Other than that, this option is independent of any other type of delete processing. The missing source files are represented by special file-list entries which @@ -2032,14 +2081,14 @@ expand it. Tells [`--delete`](#opt) to go ahead and delete files even when there are I/O errors. -0. `--force` +0. `--force`, `--force-delete` This option tells rsync to delete a non-empty directory when it is to be replaced by a non-directory. This is only relevant if deletions are not active (see [`--delete`](#opt) for details). - Note for older rsync versions: `--force` used to still be required when - using [`--delete-after`](#opt), and it used to be non-functional unless the + Note that some older rsync versions used to require `--force` when using + [`--delete-after`](#opt), and it used to be non-functional unless the [`--recursive`](#opt) option was also enabled. 0. `--max-delete=NUM` @@ -3116,7 +3165,7 @@ expand it. also turns on the output of other verbose messages). The "%i" escape has a cryptic output that is 11 letters long. The general - format is like the string `YXcstpoguax`, where **Y** is replaced by the type + format is like the string `YXcstpoguaxf`, where **Y** is replaced by the type of update being done, **X** is replaced by the file-type, and the other letters represent attributes that may be output if they are being modified. diff --git a/rsync.c b/rsync.c index 1d2ae82a1..28301b472 100644 --- a/rsync.c +++ b/rsync.c @@ -31,6 +31,8 @@ extern int dry_run; extern int preserve_acls; extern int preserve_xattrs; extern int preserve_perms; +extern int preserve_fileflags; +extern int preserve_unsafe_fileflags; extern int preserve_executability; extern int preserve_mtimes; extern int omit_dir_times; @@ -471,6 +473,100 @@ mode_t dest_mode(mode_t flist_mode, mode_t stat_mode, int dflt_perms, return new_mode; } +#if defined SUPPORT_FILEFLAGS || defined SUPPORT_FORCE_CHANGE +/* Project the sender-supplied fileflags onto the bits this receiver is + * willing to apply, preserving the bits the receiver already has outside + * that mask. By default the safe-to-apply set is SAFE_FILEFLAGS; the + * --unsafe-fileflags option widens it to all bits. See rsync.h comment + * on SAFE_FILEFLAGS for the threat model. */ +uint32 filter_recv_fileflags(uint32 received, uint32 current_on_dest) +{ + uint32 mask = preserve_unsafe_fileflags ? (uint32)-1 : SAFE_FILEFLAGS; + return (received & mask) | (current_on_dest & ~mask); +} + +/* Set a file's BSD-canonical fileflags, via the portable wrapper that + * handles both BSD fchflags(fd, ...) and Linux ioctl(FS_IOC_SETFLAGS). + * Opens the path with O_NOFOLLOW first to avoid the symlink-follow + * TOCTOU class: chflags(2)/chattr-via-path would re-resolve the path + * each call, letting an attacker swap a regular file for a symlink to + * /etc/master.passwd between the lstat-and-chflags pair the original + * patch used. Operating on an O_NOFOLLOW fd pins the change to the + * inode we actually opened. */ +static int set_fileflags(const char *fname, uint32 fileflags) +{ + int fd, rc, save_errno; + int oflags = O_RDONLY | O_NOFOLLOW; + STRUCT_STAT st; + +#ifdef O_NONBLOCK + oflags |= O_NONBLOCK; +#endif + fd = open(fname, oflags); + if (fd < 0) + goto fail; + if (fstat(fd, &st) != 0) { + save_errno = errno; + close(fd); + errno = save_errno; + goto fail; + } + rc = rsync_fchflags(fd, st.st_mode, fileflags); + save_errno = errno; + close(fd); + errno = save_errno; + if (rc != 0) + goto fail; + return 1; +fail: + rsyserr(FERROR_XFER, errno, + "failed to set file flags on %s", + full_fname(fname)); + return 0; +} + +/* Remove immutable flags from an object, so it can be altered/removed. */ +int make_mutable(const char *fname, mode_t mode, uint32 fileflags, uint32 iflags) +{ + if (S_ISLNK(mode) || !(fileflags & iflags)) + return 0; + if (!set_fileflags(fname, fileflags & ~iflags)) + return -1; + return 1; +} + +/* Undo a prior make_mutable() call that returned a 1. */ +int undo_make_mutable(const char *fname, uint32 fileflags) +{ + if (!set_fileflags(fname, fileflags)) + return -1; + return 1; +} + +/* fd-based variants for use from the syscall.c force_change recovery + * paths, where we hold an fd we just openat()'d. No path is touched + * after open, eliminating the TOCTOU window between an lstat and the + * chflags. */ +int make_mutable_fd(int fd, mode_t mode, uint32 fileflags, uint32 iflags) +{ + if (S_ISLNK(mode) || !(fileflags & iflags)) + return 0; + if (rsync_fchflags(fd, mode, fileflags & ~iflags) != 0) + return -1; + return 1; +} + +int undo_make_mutable_fd(int fd, uint32 fileflags) +{ + /* mode unknown here; pass S_IFREG so the Linux rsync_fchflags + * doesn't refuse based on file type (we've already validated in + * make_mutable_fd that the fd is regular/dir). */ + if (rsync_fchflags(fd, S_IFREG, fileflags) != 0) + return -1; + return 1; +} +#endif + static int same_mtime(struct file_struct *file, STRUCT_STAT *st, int extra_accuracy) { #ifdef ST_MTIME_NSEC @@ -669,6 +765,25 @@ int set_file_attrs(const char *fname, struct file_struct *file, stat_x *sxp, } #endif +#ifdef SUPPORT_FILEFLAGS + if (preserve_fileflags && !S_ISLNK(sxp->st.st_mode)) { + uint32 current = stat_x_get_fileflags(sxp, fname); + uint32 wanted = filter_recv_fileflags(F_FFLAGS(file), current); + if (flags & ATTRS_DELAY_IMMUTABLE) + wanted &= ~ALL_IMMUTABLE; + if (current != wanted) { + if (!set_fileflags(fname, wanted)) + goto cleanup; + /* Deliberately do NOT invalidate the cache here: the + * itemize() call that follows compares pre-change + * (cached) against wanted to decide whether to emit + * the 'f' column. Invalidating would refresh to the + * post-change value and itemize would see no diff. */ + updated = 1; + } + } +#endif + if (INFO_GTE(NAME, 2) && flags & ATTRS_REPORT) { if (updated) rprintf(FCLIENT, "%s\n", fname); @@ -746,7 +861,8 @@ int finish_transfer(const char *fname, const char *fnametmp, /* Change permissions before putting the file into place. */ set_file_attrs(fnametmp, file, NULL, fnamecmp, - ok_to_set_time ? ATTRS_ACCURATE_TIME : ATTRS_SKIP_MTIME | ATTRS_SKIP_ATIME | ATTRS_SKIP_CRTIME); + ATTRS_DELAY_IMMUTABLE + | (ok_to_set_time ? ATTRS_ACCURATE_TIME : ATTRS_SKIP_MTIME | ATTRS_SKIP_ATIME | ATTRS_SKIP_CRTIME)); /* move tmp file over real file */ if (DEBUG_GTE(RECV, 1)) @@ -763,6 +879,17 @@ int finish_transfer(const char *fname, const char *fnametmp, } if (ret == 0) { /* The file was moved into place (not copied), so it's done. */ +#ifdef SUPPORT_FILEFLAGS + if (preserve_fileflags) { + /* We delayed applying the immutable bits until after + * the rename; do so now -- but only the bits the + * receiver's policy lets through (see + * filter_recv_fileflags). */ + uint32 wanted = filter_recv_fileflags(F_FFLAGS(file), 0); + if (wanted & ALL_IMMUTABLE) + set_fileflags(fname, wanted); + } +#endif return 1; } /* The file was copied, so tweak the perms of the copied file. If it diff --git a/rsync.h b/rsync.h index cdc2d2c0d..38281f7e1 100644 --- a/rsync.h +++ b/rsync.h @@ -69,7 +69,7 @@ /* The following XMIT flags require an rsync that uses a varint for the flag values */ -#define XMIT_RESERVED_16 (1<<16) /* reserved for future fileflags use */ +#define XMIT_SAME_FLAGS (1<<16) /* any protocol - restricted by command-line option */ #define XMIT_CRTIME_EQ_MTIME (1<<17) /* any protocol - restricted by command-line option */ /* These flags are used in the live flist data. */ @@ -216,6 +216,7 @@ #define ATTRS_SKIP_MTIME (1<<1) #define ATTRS_ACCURATE_TIME (1<<2) #define ATTRS_SKIP_ATIME (1<<3) +#define ATTRS_DELAY_IMMUTABLE (1<<4) #define ATTRS_SKIP_CRTIME (1<<5) #define MSG_FLUSH 2 @@ -244,6 +245,7 @@ #define ITEM_REPORT_GROUP (1<<6) #define ITEM_REPORT_ACL (1<<7) #define ITEM_REPORT_XATTR (1<<8) +#define ITEM_REPORT_FFLAGS (1<<9) #define ITEM_REPORT_CRTIME (1<<10) #define ITEM_BASIS_TYPE_FOLLOWS (1<<11) #define ITEM_XNAME_FOLLOWS (1<<12) @@ -611,6 +613,74 @@ typedef unsigned int size_t; #define SUPPORT_CRTIMES 1 #endif +#define NO_FFLAGS ((uint32)-1) + +/* SUPPORT_FILEFLAGS / SUPPORT_FORCE_CHANGE: enabled on BSD-family systems + * with chflags(2) and on Linux systems with the FS_IOC_{GET,SET}FLAGS + * ioctls (chattr). The on-the-wire bit values are the BSD UF_xxx and + * SF_xxx set; the Linux side (in lib/fileflags.c) translates to/from + * FS_xxx_FL at the platform boundary. */ +#if defined HAVE_CHFLAGS || defined HAVE_FS_IOC_GETFLAGS +#define SUPPORT_FILEFLAGS 1 +#define SUPPORT_FORCE_CHANGE 1 +#endif + +#if defined SUPPORT_FILEFLAGS || defined SUPPORT_FORCE_CHANGE +/* BSD bit values that may be missing in some headers (and on Linux, + * which has none of them in ). These are the canonical + * wire values regardless of platform. */ +#ifndef UF_NODUMP +#define UF_NODUMP 0x00000001 +#endif +#ifndef UF_IMMUTABLE +#define UF_IMMUTABLE 0x00000002 +#endif +#ifndef UF_APPEND +#define UF_APPEND 0x00000004 +#endif +#ifndef UF_NOUNLINK +#define UF_NOUNLINK 0 +#endif +#ifndef UF_HIDDEN +#define UF_HIDDEN 0 +#endif +#ifndef SF_IMMUTABLE +#define SF_IMMUTABLE 0x00020000 +#endif +#ifndef SF_APPEND +#define SF_APPEND 0x00040000 +#endif +#ifndef SF_NOUNLINK +#define SF_NOUNLINK 0 +#endif +#define USR_IMMUTABLE (UF_IMMUTABLE|UF_NOUNLINK|UF_APPEND) +#define SYS_IMMUTABLE (SF_IMMUTABLE|SF_NOUNLINK|SF_APPEND) +#define ALL_IMMUTABLE (USR_IMMUTABLE|SYS_IMMUTABLE) +/* SAFE_FILEFLAGS: the bits --fileflags will apply by default from an + * untrusted source. Excludes SF_* (root-only, kernel securelevel-locked, + * can permanently brick a receiver) and UF_NOUNLINK (DoS: locks the + * receiver out of cleanup even for the file owner). Use + * --unsafe-fileflags to apply the full sender value. */ +#define SAFE_FILEFLAGS (UF_NODUMP|UF_IMMUTABLE|UF_APPEND|UF_HIDDEN) +#else +#define USR_IMMUTABLE 0 +#define SYS_IMMUTABLE 0 +#define ALL_IMMUTABLE 0 +#define SAFE_FILEFLAGS 0 +#endif + +/* ST_FLAGS reads the current fileflags off an already-known stat. On + * BSD struct stat has st_flags; on Linux it doesn't, so the access + * needs an extra ioctl (which the code arranges via stat_x's cached + * fileflags or via an explicit helper). ST_FLAGS therefore only + * applies on BSD-with-chflags builds; elsewhere it returns NO_FFLAGS + * to force the caller into the explicit-helper path. */ +#ifdef HAVE_CHFLAGS +#define ST_FLAGS(st) ((st).st_flags) +#else +#define ST_FLAGS(st) NO_FFLAGS +#endif + /* Find a variable that is either exactly 32-bits or longer. * If some code depends on 32-bit truncation, it will need to * take special action in a "#if SIZEOF_INT32 > 4" section. */ @@ -842,6 +912,7 @@ extern int pathname_ndx; extern int depth_ndx; extern int uid_ndx; extern int gid_ndx; +extern int fileflags_ndx; extern int acls_ndx; extern int xattrs_ndx; extern int file_sum_extra_cnt; @@ -897,6 +968,11 @@ extern int file_sum_extra_cnt; /* When the associated option is on, all entries will have these present: */ #define F_OWNER(f) REQ_EXTRA(f, uid_ndx)->unum #define F_GROUP(f) REQ_EXTRA(f, gid_ndx)->unum +#if defined SUPPORT_FILEFLAGS || defined SUPPORT_FORCE_CHANGE +#define F_FFLAGS(f) REQ_EXTRA(f, fileflags_ndx)->unum +#else +#define F_FFLAGS(f) NO_FFLAGS +#endif #define F_ACL(f) REQ_EXTRA(f, acls_ndx)->num #define F_XATTR(f) REQ_EXTRA(f, xattrs_ndx)->num #define F_NDX(f) REQ_EXTRA(f, unsort_ndx)->num @@ -1159,6 +1235,14 @@ typedef struct { typedef struct { STRUCT_STAT st; time_t crtime; +#ifdef SUPPORT_FILEFLAGS + /* Cached BSD-canonical fileflags for this inode. On BSD this is + * just sxp->st.st_flags (populated when fileflags_cached != 0); on + * Linux it requires an open()+ioctl(FS_IOC_GETFLAGS), so we cache + * it after the first read for the lifetime of the stat_x. */ + uint32 fileflags; + int fileflags_cached; +#endif #ifdef SUPPORT_ACLS struct rsync_acl *acc_acl; /* access ACL */ struct rsync_acl *def_acl; /* default ACL */ @@ -1171,6 +1255,15 @@ typedef struct { #define ACL_READY(sx) ((sx).acc_acl != NULL) #define XATTR_READY(sx) ((sx).xattr != NULL) +/* Portable fileflags helpers (lib/fileflags.c). */ +#ifdef SUPPORT_FILEFLAGS +int rsync_fchflags(int fd, mode_t mode, uint32 bsd_flags); +uint32 rsync_fgetflags(int fd, mode_t mode, const STRUCT_STAT *hint); +uint32 rsync_lgetflags(const char *path, mode_t mode, const STRUCT_STAT *hint); +uint32 stat_x_get_fileflags(stat_x *sxp, const char *path); +void stat_x_invalidate_fileflags(stat_x *sxp); +#endif + #define CLVL_NOT_SPECIFIED INT_MIN #define CPRES_AUTO (-1) diff --git a/syscall.c b/syscall.c index e317bccc3..151266d5f 100644 --- a/syscall.c +++ b/syscall.c @@ -45,6 +45,7 @@ extern int am_root; extern int am_sender; extern int read_only; extern int list_only; +extern int force_change; extern int inplace; extern int preallocate_files; extern int preserve_perms; @@ -52,6 +53,7 @@ extern int preserve_executability; extern int open_noatime; extern int copy_links; extern int copy_unsafe_links; +extern char curr_dir[MAXPATHLEN]; #ifndef S_BLKSIZE # if defined hpux || defined __hpux__ || defined __hpux @@ -86,11 +88,165 @@ struct create_time { #define RETURN_ERROR_IF_RO_OR_LO RETURN_ERROR_IF(read_only || list_only, EROFS) +#ifdef SUPPORT_FORCE_CHANGE +/* Split path into dirname (copied into dirbuf) and basename (pointer into + * the original path). Used by the force_change recovery paths to obtain a + * dirfd we can anchor openat()/unlinkat()/renameat()/fchownat() onto, so + * the actual operation runs on the inode we opened rather than re-resolving + * the path each call (which is the TOCTOU primitive). Returns 0 on + * success, -1 with errno set on failure. */ +static int split_dir_base(const char *path, char *dirbuf, size_t dirbuf_sz, + const char **base_out) +{ + const char *slash = strrchr(path, '/'); + size_t dirlen; + + if (slash == NULL) { + /* No slash: bare filename, parent is "." */ + if (dirbuf_sz < 2) { errno = ENAMETOOLONG; return -1; } + dirbuf[0] = '.'; + dirbuf[1] = '\0'; + *base_out = path; + return 0; + } + dirlen = slash - path; + if (dirlen == 0) { + /* "/file" -> dir="/" */ + if (dirbuf_sz < 2) { errno = ENAMETOOLONG; return -1; } + dirbuf[0] = '/'; + dirbuf[1] = '\0'; + } else { + if (dirlen + 1 > dirbuf_sz) { errno = ENAMETOOLONG; return -1; } + memcpy(dirbuf, path, dirlen); + dirbuf[dirlen] = '\0'; + } + *base_out = slash + 1; + if (**base_out == '\0') { + /* Trailing slash -- not a normal target. */ + errno = EISDIR; + return -1; + } + return 0; +} + +/* Open the parent directory of path, returning the dirfd and the + * basename pointer. Caller closes dirfd. + * + * SECURITY (phase 3): when path is relative we anchor the parent-dir + * open at curr_dir via secure_relative_open(), which uses + * openat2(RESOLVE_BENEATH) on Linux 5.6+, openat(O_RESOLVE_BENEATH) on + * FreeBSD 13+ / macOS 15+, and a per-component O_NOFOLLOW walk + * elsewhere. All of these reject any path that escapes curr_dir via + * ".." or via a symlink that points outside the subtree. On the + * receiver curr_dir IS the destination root (set via change_dir() + * before the file-ops phase), so the effect is to bound the + * force_change recovery to the destination tree -- a sender that + * managed to inject a path like "../etc/master.passwd" still gets the + * openat refused at the kernel. + * + * For absolute paths we fall back to the phase-2 path-based open + * (O_NOFOLLOW on the parent dir, no RESOLVE_BENEATH). Absolute paths + * are rare in the force_change recovery context (receiver normally + * operates on relative paths) but a few configurations -- partial-dir, + * backup-dir -- can pass them. */ +static int force_change_open_parent(const char *path, const char **base_out) +{ + char dirbuf[MAXPATHLEN]; + int dirfd; + + if (split_dir_base(path, dirbuf, sizeof dirbuf, base_out) < 0) + return -1; +#ifndef O_DIRECTORY +#define O_DIRECTORY 0 +#endif + if (dirbuf[0] != '/') { + dirfd = secure_relative_open(curr_dir, dirbuf, + O_RDONLY | O_DIRECTORY | O_NOFOLLOW, 0); + /* Do NOT fall back to a path-based open on failure -- that + * would re-enable the escape route we just closed. */ + return dirfd; + } + dirfd = open(dirbuf, O_RDONLY | O_DIRECTORY | O_NOFOLLOW); + return dirfd; +} + +/* Open the target via dirfd+basename with O_NOFOLLOW (so we never operate + * on a symlink the attacker substituted), then fstat to learn its current + * mode and read its current BSD-canonical fileflags (via rsync_fgetflags, + * which abstracts BSD fstat-st_flags vs Linux ioctl(FS_IOC_GETFLAGS)). + * Returns fd on success, -1 with errno set on failure. */ +static int force_change_open_target(int dirfd, const char *base, STRUCT_STAT *stp, uint32 *flags_out) +{ + int oflags = O_RDONLY | O_NOFOLLOW; + int fd; + +#ifdef O_NONBLOCK + oflags |= O_NONBLOCK; /* don't block on fifos */ +#endif + fd = openat(dirfd, base, oflags); + if (fd < 0) + return -1; + if (fstat(fd, stp) != 0) { + int e = errno; + close(fd); + errno = e; + return -1; + } + *flags_out = rsync_fgetflags(fd, stp->st_mode, stp); + if (*flags_out == NO_FFLAGS) { + int e = errno; + close(fd); + errno = e; + return -1; + } + return fd; +} +#endif /* SUPPORT_FORCE_CHANGE */ + int do_unlink(const char *path) { if (dry_run) return 0; RETURN_ERROR_IF_RO_OR_LO; - return unlink(path); + if (unlink(path) == 0) + return 0; +#ifdef SUPPORT_FORCE_CHANGE + if (force_change && errno == EPERM) { + const char *base; + int dirfd, fd; + STRUCT_STAT st; + uint32 fileflags; + + /* Dirfd-anchored recovery: open the parent + the target with + * O_NOFOLLOW, fchflags-clear via the fd, then unlinkat via + * the dirfd. This prevents an attacker from swapping the + * path under us between the lstat and the unlink (which the + * earlier path-based code allowed, giving an arbitrary + * make-mutable / unlink primitive when rsync is running as + * root). */ + dirfd = force_change_open_parent(path, &base); + if (dirfd < 0) + goto unlink_fail; + fd = force_change_open_target(dirfd, base, &st, &fileflags); + if (fd < 0) { + close(dirfd); + goto unlink_fail; + } + if (make_mutable_fd(fd, st.st_mode, fileflags, force_change) > 0) { + if (unlinkat(dirfd, base, 0) == 0) { + close(fd); + close(dirfd); + return 0; + } + undo_make_mutable_fd(fd, fileflags); + } + close(fd); + close(dirfd); + /* TODO: handle immutable directories */ + unlink_fail: + errno = EPERM; + } +#endif + return -1; } /* @@ -406,7 +562,44 @@ int do_lchown(const char *path, uid_t owner, gid_t group) #ifndef HAVE_LCHOWN #define lchown chown #endif - return lchown(path, owner, group); + if (lchown(path, owner, group) == 0) + return 0; +#ifdef SUPPORT_FORCE_CHANGE + if (force_change && errno == EPERM) { + const char *base; + int dirfd, fd; + STRUCT_STAT st; + uint32 fileflags; + int ret; + + dirfd = force_change_open_parent(path, &base); + if (dirfd < 0) + goto lchown_fail; + fd = force_change_open_target(dirfd, base, &st, &fileflags); + if (fd < 0) { + close(dirfd); + goto lchown_fail; + } + if (make_mutable_fd(fd, st.st_mode, fileflags, force_change) > 0) { + /* lchown semantics on the fd: fchownat with + * AT_SYMLINK_NOFOLLOW operates on the symlink itself, + * but we opened with O_NOFOLLOW so it's not a symlink + * by construction. Use fchown for clarity. */ + ret = fchown(fd, owner, group); + undo_make_mutable_fd(fd, fileflags); + if (ret == 0) { + close(fd); + close(dirfd); + return 0; + } + } + close(fd); + close(dirfd); + lchown_fail: + errno = EPERM; + } +#endif + return -1; } /* @@ -616,7 +809,38 @@ int do_rmdir(const char *pathname) { if (dry_run) return 0; RETURN_ERROR_IF_RO_OR_LO; - return rmdir(pathname); + if (rmdir(pathname) == 0) + return 0; +#ifdef SUPPORT_FORCE_CHANGE + if (force_change && errno == EPERM) { + const char *base; + int dirfd, fd; + STRUCT_STAT st; + uint32 fileflags; + + dirfd = force_change_open_parent(pathname, &base); + if (dirfd < 0) + goto rmdir_fail; + fd = force_change_open_target(dirfd, base, &st, &fileflags); + if (fd < 0) { + close(dirfd); + goto rmdir_fail; + } + if (make_mutable_fd(fd, st.st_mode, fileflags, force_change) > 0) { + if (unlinkat(dirfd, base, AT_REMOVEDIR) == 0) { + close(fd); + close(dirfd); + return 0; + } + undo_make_mutable_fd(fd, fileflags); + } + close(fd); + close(dirfd); + rmdir_fail: + errno = EPERM; + } +#endif + return -1; } /* @@ -795,6 +1019,39 @@ int do_chmod(const char *path, mode_t mode) code = chmod(path, mode & CHMOD_BITS); /* DISCOURAGED FUNCTION */ break; } +#ifdef SUPPORT_FORCE_CHANGE + if (code < 0 && force_change && errno == EPERM && !S_ISLNK(mode)) { + const char *base; + int dirfd, fd; + STRUCT_STAT st; + uint32 fileflags; + + dirfd = force_change_open_parent(path, &base); + if (dirfd < 0) { + errno = EPERM; + goto chmod_done; + } + fd = force_change_open_target(dirfd, base, &st, &fileflags); + if (fd < 0) { + close(dirfd); + errno = EPERM; + goto chmod_done; + } + if (make_mutable_fd(fd, mode, fileflags, force_change) > 0) { + code = fchmod(fd, mode & CHMOD_BITS); + undo_make_mutable_fd(fd, fileflags); + if (code == 0) { + close(fd); + close(dirfd); + return 0; + } + } + close(fd); + close(dirfd); + errno = EPERM; + } + chmod_done: +#endif if (code != 0 && (preserve_perms || preserve_executability)) return code; return 0; @@ -881,11 +1138,79 @@ int do_chmod_at(const char *fname, mode_t mode) } #endif +/* (do_chflags removed -- callers now go through rsync.c's + * set_fileflags(), which opens with O_NOFOLLOW + fstat and routes + * via the portable rsync_fchflags() in lib/fileflags.c. The same + * symlink-follow defence is in place there; this just centralizes + * the path-to-fd transition for both BSD and Linux.) */ + int do_rename(const char *old_path, const char *new_path) { if (dry_run) return 0; RETURN_ERROR_IF_RO_OR_LO; - return rename(old_path, new_path); + if (rename(old_path, new_path) == 0) + return 0; +#ifdef SUPPORT_FORCE_CHANGE + if (force_change && errno == EPERM) { + const char *old_base = NULL, *new_base = NULL; + int old_dirfd = -1, new_dirfd = -1; + int old_fd = -1, new_fd = -1; + STRUCT_STAT old_st, new_st; + uint32 old_fileflags = 0, new_fileflags = 0; + int old_mutated = 0, new_mutated = 0; + + /* Open old_path's parent + target. */ + old_dirfd = force_change_open_parent(old_path, &old_base); + if (old_dirfd < 0) + goto rename_fail; + old_fd = force_change_open_target(old_dirfd, old_base, &old_st, &old_fileflags); + if (old_fd < 0) + goto rename_fail; + old_mutated = make_mutable_fd(old_fd, old_st.st_mode, + old_fileflags, force_change) > 0; + if (old_mutated && renameat(old_dirfd, old_base, + AT_FDCWD, new_path) == 0) + goto rename_success; + + /* Old side wasn't the blocker (or rename still failed): + * try to make new_path mutable too. */ + new_dirfd = force_change_open_parent(new_path, &new_base); + if (new_dirfd < 0) + goto rename_fail; + new_fd = force_change_open_target(new_dirfd, new_base, &new_st, &new_fileflags); + if (new_fd < 0) + goto rename_fail; + new_mutated = make_mutable_fd(new_fd, new_st.st_mode, + new_fileflags, force_change) > 0; + if (new_mutated + && renameat(old_dirfd, old_base, new_dirfd, new_base) == 0) { + rename_success: + /* After a successful rename, the inode that used to be + * at old_path now lives at new_path. Restore its + * original flags on the new location via the still-open + * fd (the fd survives the rename). */ + if (old_mutated) + undo_make_mutable_fd(old_fd, old_fileflags); + close(old_fd); + close(old_dirfd); + if (new_fd != -1) close(new_fd); + if (new_dirfd != -1) close(new_dirfd); + return 0; + } + if (new_mutated) + undo_make_mutable_fd(new_fd, new_fileflags); + if (old_mutated) + undo_make_mutable_fd(old_fd, old_fileflags); + rename_fail: + if (old_fd != -1) close(old_fd); + if (old_dirfd != -1) close(old_dirfd); + if (new_fd != -1) close(new_fd); + if (new_dirfd != -1) close(new_dirfd); + /* TODO: handle immutable directories */ + errno = EPERM; + } +#endif + return -1; } /* diff --git a/t_stub.c b/t_stub.c index 63bc144c5..9b7691fd8 100644 --- a/t_stub.c +++ b/t_stub.c @@ -31,6 +31,9 @@ int protect_args = 0; int module_id = -1; int relative_paths = 0; unsigned int module_dirlen = 0; +int force_change = 0; +int preserve_acls = 0; +int preserve_unsafe_fileflags = 0; int preserve_xattrs = 0; int preserve_perms = 0; int preserve_executability = 0; @@ -39,6 +42,11 @@ int open_noatime = 0; size_t max_alloc = 0; /* max_alloc is needed when combined with util2.o */ char *partial_dir; char *module_dir; +/* curr_dir is the rsync-managed cwd, normally defined in util1.c. + * Some test helpers (tls, trimslash) don't link util1.o; define it + * here as weak so the real def in util1.o still wins for helpers + * that DO link it (t_unsafe). */ +__attribute__((weak)) char curr_dir[MAXPATHLEN]; filter_rule_list daemon_filter_list; void rprintf(UNUSED(enum logcode code), const char *format, ...) @@ -113,3 +121,33 @@ filter_rule_list daemon_filter_list; { return cst ? 0 : 0; } + +#if defined SUPPORT_FILEFLAGS || defined SUPPORT_FORCE_CHANGE + int make_mutable(UNUSED(const char *fname), UNUSED(mode_t mode), UNUSED(uint32 fileflags), UNUSED(uint32 iflags)) +{ + return 0; +} + +/* Undo a prior make_mutable() call that returned a 1. */ + int undo_make_mutable(UNUSED(const char *fname), UNUSED(uint32 fileflags)) +{ + return 0; +} + + int make_mutable_fd(UNUSED(int fd), UNUSED(mode_t mode), UNUSED(uint32 fileflags), UNUSED(uint32 iflags)) +{ + return 0; +} + + int undo_make_mutable_fd(UNUSED(int fd), UNUSED(uint32 fileflags)) +{ + return 0; +} +#endif + +#ifdef SUPPORT_XATTRS + int x_lstat(UNUSED(const char *fname), UNUSED(STRUCT_STAT *fst), UNUSED(STRUCT_STAT *xst)) +{ + return -1; +} +#endif diff --git a/testsuite/fileflags.test b/testsuite/fileflags.test new file mode 100755 index 000000000..9ce4329d8 --- /dev/null +++ b/testsuite/fileflags.test @@ -0,0 +1,148 @@ +#!/bin/sh + +# Test rsync preserving file flags via --fileflags. +# +# rsync's --fileflags rides on top of: +# - BSD/macOS: chflags(2) / fchflags(2) +# - Linux: ioctl(FS_IOC_GETFLAGS / FS_IOC_SETFLAGS), aka chattr(1) +# +# This test exercises both, using whichever userland tool is available +# (chflags on BSD/macOS, chattr on Linux). Test skips cleanly on +# systems where rsync wasn't built with fileflags support, where +# neither tool is installed, or where the test scratch filesystem +# doesn't support the flags we need. + +. "$suitedir/rsync.fns" + +# Skip if rsync wasn't built with fileflags support. +$RSYNC -VV | grep '"file_flags": true' >/dev/null \ + || test_skipped "Rsync is configured without fileflags support" + +# Pick the platform-appropriate userland tool and readback function. +if command -v chflags >/dev/null 2>&1; then + # BSD/macOS path + set_nodump() { chflags nodump "$1"; } + set_uchg() { chflags uchg "$1" 2>/dev/null; } + clear_flags() { chflags 0 "$@" 2>/dev/null; } + if stat -f '%f' . >/dev/null 2>&1; then + show_flags() { stat -f '%f' "$1"; } + elif ls -lod . >/dev/null 2>&1; then + show_flags() { ls -lod "$1" | awk '{print $5}'; } + else + test_skipped "No way to read st_flags on this host (no stat -f or ls -lo)" + fi +elif command -v chattr >/dev/null 2>&1 && command -v lsattr >/dev/null 2>&1; then + # Linux path -- chattr 'd' is owner-settable on filesystems that + # support it (ext2/3/4, xfs, btrfs, f2fs, ...). 'i' (immutable) + # requires CAP_LINUX_IMMUTABLE so set_uchg will fail and the uchg + # portion of the test will be skipped for ordinary users. + set_nodump() { chattr +d "$1"; } + set_uchg() { chattr +i "$1" 2>/dev/null; } + clear_flags() { chattr = "$@" 2>/dev/null; } + # lsattr's first column is the 20-char attribute string -- but + # most of those bits are fs-internal (extent format, htree, inline + # data, ...) and aren't part of what rsync transfers. Strip down + # to just the transferable letters (a, d, i, u) so we don't fail + # on e.g. a small dest inode that didn't get the 'e' bit set when + # ext4 inlined its data. + show_flags() { lsattr -d "$1" 2>/dev/null | awk '{ + s = $1; out = ""; + for (i = 1; i <= length(s); i++) { + c = substr(s, i, 1); + if (c == "a" || c == "d" || c == "i" || c == "u") out = out c; + } + if (out == "") out = "-"; + print out; + }'; } +else + test_skipped "No chflags(1) or chattr(1) command on this host" +fi + +mkdir "$fromdir" +echo hi >"$fromdir/plain" +echo hi >"$fromdir/nodump" +echo hi >"$fromdir/uchg" +mkdir "$fromdir/dir" +echo hi >"$fromdir/dir/inner" + +# Set the user-settable "nodump" flag on one file. +set_nodump "$fromdir/nodump" \ + || test_skipped "Filesystem does not support the nodump flag" + +# Try the user-immutable flag too; on Linux this needs +# CAP_LINUX_IMMUTABLE (root), on BSDs some filesystems (e.g. tmpfs on +# some kernels) don't allow it -- if so, just drop the uchg part. +if set_uchg "$fromdir/uchg"; then + have_uchg=1 +else + have_uchg=0 + rm -f "$fromdir/uchg" +fi + +# Also set nodump on the directory to check that dir flags travel. +set_nodump "$fromdir/dir" || true + +src_nodump=`show_flags "$fromdir/nodump"` +src_plain=`show_flags "$fromdir/plain"` +src_dir=`show_flags "$fromdir/dir"` + +echo "source flags: plain=$src_plain nodump=$src_nodump dir=$src_dir" + +TLS_ARGS=--fileflags + +echo "Running: $RSYNC -rtgvvv --fileflags \"$fromdir/\" \"$todir/\"" +$RSYNC -rtgvvv --fileflags "$fromdir/" "$todir/" \ + || test_fail "rsync --fileflags failed" + +for f in plain nodump dir/inner dir; do + s=`show_flags "$fromdir/$f"` + d=`show_flags "$todir/$f"` + if [ "x$s" != "x$d" ]; then + test_fail "flags mismatch on $f: source=$s dest=$d" + fi + echo "ok: $f flags=$d" +done + +if [ "$have_uchg" = 1 ]; then + s=`show_flags "$fromdir/uchg"` + d=`show_flags "$todir/uchg"` + if [ "x$s" != "x$d" ]; then + clear_flags "$fromdir/uchg" "$todir/uchg" || true + test_fail "flags mismatch on uchg: source=$s dest=$d" + fi + echo "ok: uchg flags=$d" + # Clear so the scratchdir can be removed. + clear_flags "$fromdir/uchg" "$todir/uchg" || true +fi + +# Confirm the itemized output reports an 'f' change on a second run +# after we clear the flag on the destination. +clear_flags "$todir/nodump" || test_fail "could not clear flags on dest nodump" + +$RSYNC -rtgi --fileflags "$fromdir/" "$todir/" >"$outfile" 2>&1 \ + || test_fail "second rsync --fileflags run failed" +cat "$outfile" +if ! grep -E '^\.f\.\.\.\.\.\.\.\.\.f.* nodump$' "$outfile" >/dev/null; then + test_fail "expected itemized 'f' (flags) change on nodump in second run" +fi + + +######################################################################## +# SECURITY: --unsafe-fileflags option +######################################################################## +# Smoke test only: confirm the option parses and that --fileflags +# --unsafe-fileflags round-trips a SAFE_FILEFLAGS bit cleanly. + +rm -rf "$fromdir" "$todir" +mkdir "$fromdir" "$todir" +echo hi > "$fromdir/plain" +set_nodump "$fromdir/plain" 2>/dev/null || true + +$RSYNC -rt --fileflags --unsafe-fileflags "$fromdir/" "$todir/" \ + || test_fail "rsync --fileflags --unsafe-fileflags failed on simple tree" + +if [ "`show_flags "$fromdir/plain"`" != "`show_flags "$todir/plain"`" ]; then + test_fail "unsafe-fileflags should still propagate flags within SAFE_FILEFLAGS" +fi + +exit 0 diff --git a/testsuite/rsync.fns b/testsuite/rsync.fns index 4caec58b5..0c209ee20 100644 --- a/testsuite/rsync.fns +++ b/testsuite/rsync.fns @@ -25,10 +25,18 @@ chkdir="$tmpdir/chk" chkfile="$scratchdir/rsync.chk" outfile="$scratchdir/rsync.out" -# For itemized output: -all_plus='+++++++++' -allspace=' ' -dots='.....' # trailing dots after changes +# For itemized output: %i is 12 wide on chflags-supporting builds (the +# 12th column is the 'f' flag), 11 wide everywhere else. Detect from the +# rsync binary we're testing rather than hard-coding either width. +if "$RSYNC" -VV 2>/dev/null | grep '"file_flags": true' >/dev/null 2>&1; then + all_plus='++++++++++' + allspace=' ' + dots='......' # trailing dots after changes +else + all_plus='+++++++++' + allspace=' ' + dots='.....' # trailing dots after changes +fi tab_ch=' ' # a single tab character # Berkley's nice. diff --git a/usage.c b/usage.c index f346385f4..e248b871c 100644 --- a/usage.c +++ b/usage.c @@ -138,6 +138,11 @@ static void print_info_flags(enum logcode f) #endif "crtimes", +#ifndef SUPPORT_FILEFLAGS + "no " +#endif + "file-flags", + "*Optimizations", #ifndef USE_ROLL_SIMD diff --git a/util1.c b/util1.c index 36c1b68cd..7fefdc86d 100644 --- a/util1.c +++ b/util1.c @@ -34,6 +34,7 @@ extern int relative_paths; extern int preserve_xattrs; extern int omit_link_times; extern int preallocate_files; +extern int force_change; extern char *module_dir; extern unsigned int module_dirlen; extern char *partial_dir; @@ -116,6 +117,33 @@ void print_child_argv(const char *prefix, char **cmd) rprintf(FCLIENT, " (%d args)\n", cnt); } +#ifdef SUPPORT_FORCE_CHANGE +static int try_a_force_change(const char *fname, STRUCT_STAT *stp) +{ + uint32 fileflags = ST_FLAGS(*stp); + if (fileflags == NO_FFLAGS) { + STRUCT_STAT st; + if (x_lstat(fname, &st, NULL) == 0) + fileflags = rsync_lgetflags(fname, st.st_mode, &st); + } + if (fileflags != NO_FFLAGS && make_mutable(fname, stp->st_mode, fileflags, force_change) > 0) { + int ret, save_force_change = force_change; + + force_change = 0; /* Make certain we can't come back here. */ + ret = set_times(fname, stp); + force_change = save_force_change; + + undo_make_mutable(fname, fileflags); + + return ret; + } + + errno = EPERM; + + return -1; +} +#endif + /* This returns 0 for success, 1 for a symlink if symlink time-setting * is not possible, or -1 for any other error. */ int set_times(const char *fname, STRUCT_STAT *stp) @@ -143,6 +171,10 @@ int set_times(const char *fname, STRUCT_STAT *stp) #include "case_N.h" if (do_utimensat_at(fname, stp) == 0) break; +#ifdef SUPPORT_FORCE_CHANGE + if (force_change && errno == EPERM && try_a_force_change(fname, stp) == 0) + break; +#endif if (errno != ENOSYS) return -1; switch_step++; @@ -152,6 +184,10 @@ int set_times(const char *fname, STRUCT_STAT *stp) #include "case_N.h" if (do_lutimes(fname, stp) == 0) break; +#ifdef SUPPORT_FORCE_CHANGE + if (force_change && errno == EPERM && try_a_force_change(fname, stp) == 0) + break; +#endif if (errno != ENOSYS) return -1; switch_step++; @@ -173,6 +209,10 @@ int set_times(const char *fname, STRUCT_STAT *stp) if (do_utime(fname, stp) == 0) break; #endif +#ifdef SUPPORT_FORCE_CHANGE + if (force_change && errno == EPERM && try_a_force_change(fname, stp) == 0) + break; +#endif return -1; } From 1f1514302b6b3d2f4cef35dbc926623178b9dfdd Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Wed, 20 May 2026 12:22:01 +1000 Subject: [PATCH 02/11] fileflags: address codex review findings #1 and #4 #1 (high, security): set_fileflags() in rsync.c was using bare open(fname, O_NOFOLLOW), which only protects the *final* path component. Parent components could still be swapped to symlinks between calls -- the exact attack class CVE-2026-29518 was about, just on the --fileflags / --force-change code path that the previous hardening missed. Switch to secure_relative_open(curr_dir, fname, ...) for relative paths so the full path chain is bounded by RESOLVE_BENEATH (openat2 on Linux 5.6+, openat() with O_RESOLVE_BENEATH on FreeBSD 13+ / macOS 15+, per-component O_NOFOLLOW walk elsewhere). On the receiver, curr_dir is the destination root. Absolute paths still fall back to a plain open(O_NOFOLLOW) -- no basedir context for them and they don't occur on the normal receiver path. #4: drop the --force-delete alias entirely. Operators who already have "refuse options = force" in rsyncd.conf would silently fail to refuse --force-delete (it sets the same force_delete variable but under a different popt name). --force is and stays the documented name, so the alias was only ever a back-compat hold-over from the original fileflags patch's renaming. Remove from both options.c and the man-page detailed entry header. Verified on Linux/ext4: full suite 59 passed / 2 skipped / 0 failed. Co-Authored-By: Claude Opus 4.7 (1M context) --- options.c | 2 -- rsync.1.md | 2 +- rsync.c | 29 ++++++++++++++++++++++------- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/options.c b/options.c index 0d6e7d7ca..eedeb6eec 100644 --- a/options.c +++ b/options.c @@ -738,8 +738,6 @@ static struct poptOption long_options[] = { {"remove-source-files",0,POPT_ARG_VAL, &remove_source_files, 1, 0, 0 }, {"force", 0, POPT_ARG_VAL, &force_delete, 1, 0, 0 }, {"no-force", 0, POPT_ARG_VAL, &force_delete, 0, 0, 0 }, - {"force-delete", 0, POPT_ARG_VAL, &force_delete, 1, 0, 0 }, - {"no-force-delete", 0, POPT_ARG_VAL, &force_delete, 0, 0, 0 }, /* --force-change defaults to USR_IMMUTABLE only (the safer set -- * UF_*). System-class flags require an explicit --force-schange. * USR + SYS are additive: combine with "--force-change --force-schange" diff --git a/rsync.1.md b/rsync.1.md index dd3b50670..eb1da1391 100644 --- a/rsync.1.md +++ b/rsync.1.md @@ -2081,7 +2081,7 @@ expand it. Tells [`--delete`](#opt) to go ahead and delete files even when there are I/O errors. -0. `--force`, `--force-delete` +0. `--force` This option tells rsync to delete a non-empty directory when it is to be replaced by a non-directory. This is only relevant if deletions are not diff --git a/rsync.c b/rsync.c index 28301b472..32bdc12e6 100644 --- a/rsync.c +++ b/rsync.c @@ -57,6 +57,7 @@ extern int make_backups; extern int sanitize_paths; extern struct file_list *cur_flist, *first_flist, *dir_flist; extern struct chmod_mode_struct *daemon_chmod_modes; +extern char curr_dir[MAXPATHLEN]; #ifdef ICONV_OPTION extern char *iconv_opt; #endif @@ -487,12 +488,23 @@ uint32 filter_recv_fileflags(uint32 received, uint32 current_on_dest) /* Set a file's BSD-canonical fileflags, via the portable wrapper that * handles both BSD fchflags(fd, ...) and Linux ioctl(FS_IOC_SETFLAGS). - * Opens the path with O_NOFOLLOW first to avoid the symlink-follow - * TOCTOU class: chflags(2)/chattr-via-path would re-resolve the path - * each call, letting an attacker swap a regular file for a symlink to - * /etc/master.passwd between the lstat-and-chflags pair the original - * patch used. Operating on an O_NOFOLLOW fd pins the change to the - * inode we actually opened. */ + * + * SECURITY: relative paths are opened via secure_relative_open(curr_dir, + * fname, ...) so the full path chain is bounded -- on Linux 5.6+, + * FreeBSD 13+ and macOS 15+ this uses openat2/openat with + * RESOLVE_BENEATH (or O_RESOLVE_BENEATH), and elsewhere falls back to a + * per-component O_NOFOLLOW walk. This closes the same daemon + * (use chroot = no) symlink-race attack class as CVE-2026-29518: a + * plain open(O_NOFOLLOW) only protects the *final* path component, so + * a local attacker with write access to one of the parent directories + * can swap a parent dir to a symlink between calls and redirect the + * fchflags outside the receiver's confinement. curr_dir is the + * receiver's destination root (set by change_dir() before the + * file-ops phase), so we anchor to that. + * + * Absolute paths still fall back to a plain open(O_NOFOLLOW): we have + * no basedir context for them and they don't occur on the normal + * receiver path (only via --partial-dir=/abs and similar). */ static int set_fileflags(const char *fname, uint32 fileflags) { int fd, rc, save_errno; @@ -502,7 +514,10 @@ static int set_fileflags(const char *fname, uint32 fileflags) #ifdef O_NONBLOCK oflags |= O_NONBLOCK; #endif - fd = open(fname, oflags); + if (fname[0] != '/') + fd = secure_relative_open(curr_dir, fname, oflags, 0); + else + fd = open(fname, oflags); if (fd < 0) goto fail; if (fstat(fd, &st) != 0) { From b8d0bd92d77880db575d4319e554d1bb89095b7d Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Wed, 20 May 2026 12:35:02 +1000 Subject: [PATCH 03/11] fileflags: address codex review findings #2 and #3 #2 (medium, correctness): the do_*_at() wrappers used by the receiver side bypassed the force_change EPERM-recovery I'd added in plain do_unlink / do_rmdir / do_chmod / do_lchown / do_rename. - do_unlink_at / do_rmdir_at: - Secure dirfd path: on EPERM + force_change, call new shared helper force_change_retry_unlinkat() which uses the existing dirfd to openat(O_NOFOLLOW) the target, fchflags-clear via the fd, retry unlinkat with the same dirfd, restore on failure. - Non-secure fallback paths (non-daemon / chrooted / no-parent / absolute / AT_FDCWD missing) were calling bare unlink() / rmdir(); rerouted through do_unlink() / do_rmdir() so the recovery there still fires. - do_chmod_at / do_lchown_at: - Secure dirfd path: inline the same fd-based recovery on EPERM +force_change. fd was opened O_NOFOLLOW so fchmod/fchown is safe (not a symlink). - Non-secure fallback already went through do_chmod / do_lchown which have recovery -- unchanged. - do_rename_at: secure-path recovery is involved (two-side make- mutable plus restore-on-the-renamed-inode bookkeeping that do_rename already implements), so on EPERM + force_change just fall back to do_rename(old_path, new_path). The fallback's force_change_open_parent uses secure_relative_open(curr_dir, ...) which on the receiver bounds to the same destination subtree as the dirfds we just closed -- error-recovery path only. #3 (medium, security): make_mutable() calls in delete.c were not paired with undo_make_mutable() on the paths where the delete didn't actually take place (max-delete cap, non-empty dir, do_rmdir EPERM after recovery, etc.), leaking files in a less-protected state. - delete_dir_contents per-entry make_mutable: track entry_unmuted + entry_saved_flags; if delete_item returns anything other than DR_SUCCESS, undo_make_mutable on the entry. - delete_item per-directory make_mutable (the eager clear before delete_dir_contents recursion): track dir_unmuted + dir_saved_flags hoisted to function scope; check_ret restores on the !DR_SUCCESS path. Changed the early `return DR_AT_LIMIT` to `goto check_ret` so the max-delete skip also flows through the restore. Verified on Linux/ext4: 59 passed / 2 skipped / 0 failed. Co-Authored-By: Claude Opus 4.7 (1M context) --- delete.c | 54 +++++++++++++++++++++++--- syscall.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 155 insertions(+), 12 deletions(-) diff --git a/delete.c b/delete.c index d8408ae21..5b5ab0d69 100644 --- a/delete.c +++ b/delete.c @@ -99,8 +99,21 @@ static enum delret delete_dir_contents(char *fname, uint16 flags) strlcpy(p, fp->basename, remainder); #ifdef SUPPORT_FORCE_CHANGE - if (force_change) - make_mutable(fname, fp->mode, F_FFLAGS(fp), force_change); + /* If force_change is in effect and the entry has immutable + * bits set, clear them so the recursive delete (for dirs) or + * the per-entry delete_item below can proceed. Track whether + * we actually made the change so we can undo on failure -- if + * delete_item ends up NOT removing the entry (max-delete cap, + * non-empty dir, EPERM after retry, ...) we'd otherwise leak + * a less-protected file behind. */ + int entry_unmuted = 0; + uint32 entry_saved_flags = 0; + if (force_change && (F_FFLAGS(fp) & force_change)) { + if (make_mutable(fname, fp->mode, F_FFLAGS(fp), force_change) > 0) { + entry_unmuted = 1; + entry_saved_flags = F_FFLAGS(fp); + } + } #endif if (!(fp->mode & S_IWUSR) && !am_root && fp->flags & FLAG_OWNED_BY_US) do_chmod_at(fname, fp->mode | S_IWUSR); @@ -109,8 +122,17 @@ static enum delret delete_dir_contents(char *fname, uint16 flags) if (delete_dir_contents(fname, flags | DEL_RECURSE) != DR_SUCCESS) ret = DR_NOT_EMPTY; } - if (delete_item(fname, fp->mode, flags) != DR_SUCCESS) + enum delret item_ret = delete_item(fname, fp->mode, flags); + if (item_ret != DR_SUCCESS) ret = DR_NOT_EMPTY; +#ifdef SUPPORT_FORCE_CHANGE + /* Restore the entry's flags if the delete didn't actually take + * place. DR_SUCCESS means the inode is gone, no need to + * restore; everything else (DR_NOT_EMPTY, DR_AT_LIMIT, + * DR_FAILURE) leaves the file in place. */ + if (entry_unmuted && item_ret != DR_SUCCESS) + undo_make_mutable(fname, entry_saved_flags); +#endif } fname[dlen] = '\0'; @@ -137,6 +159,15 @@ enum delret delete_item(char *fbuf, uint16 mode, uint16 flags) enum delret ret; char *what; int ok; +#ifdef SUPPORT_FORCE_CHANGE + /* Track whether we cleared the dir's immutable bits so we can + * restore them if the directory ends up NOT being removed (delete + * skipped, contents non-empty, rmdir failed). Without this the + * receiver would be left with a less-protected directory than it + * started with. */ + int dir_unmuted = 0; + uint32 dir_saved_flags = 0; +#endif if (DEBUG_GTE(DEL, 2)) { rprintf(FINFO, "delete_item(%s) mode=%o flags=%d\n", @@ -154,8 +185,10 @@ enum delret delete_item(char *fbuf, uint16 mode, uint16 flags) STRUCT_STAT st; if (x_lstat(fbuf, &st, NULL) == 0) { uint32 ff = rsync_lgetflags(fbuf, st.st_mode, &st); - if (ff != NO_FFLAGS) - make_mutable(fbuf, st.st_mode, ff, force_change); + if (ff != NO_FFLAGS && make_mutable(fbuf, st.st_mode, ff, force_change) > 0) { + dir_unmuted = 1; + dir_saved_flags = ff; + } } } #endif @@ -170,7 +203,8 @@ enum delret delete_item(char *fbuf, uint16 mode, uint16 flags) if (!(flags & DEL_MAKE_ROOM) && max_delete >= 0 && stats.deleted_files >= max_delete) { skipped_deletes++; - return DR_AT_LIMIT; + ret = DR_AT_LIMIT; + goto check_ret; } if (S_ISDIR(mode)) { @@ -222,6 +256,14 @@ enum delret delete_item(char *fbuf, uint16 mode, uint16 flags) } check_ret: +#ifdef SUPPORT_FORCE_CHANGE + /* If we made the directory mutable but it's still present (delete + * skipped, contents non-empty, rmdir EPERM after recovery, etc.), + * restore its original flags so we don't leave the receiver with + * weaker protection than it started with. */ + if (dir_unmuted && ret != DR_SUCCESS) + undo_make_mutable(fbuf, dir_saved_flags); +#endif if (ret != DR_SUCCESS && flags & DEL_MAKE_ROOM) { const char *desc; switch (flags & DEL_MAKE_ROOM) { diff --git a/syscall.c b/syscall.c index 151266d5f..c263cd7b1 100644 --- a/syscall.c +++ b/syscall.c @@ -201,6 +201,34 @@ static int force_change_open_target(int dirfd, const char *base, STRUCT_STAT *st } return fd; } + +/* Shared force_change EPERM-recovery helper for do_unlink_at / do_rmdir_at: + * make the target temporarily mutable via fchflags, retry unlinkat with the + * given AT_REMOVEDIR-or-0 flag, restore flags on failure. Reuses the + * caller's already-opened secure dirfd so the recovery stays bounded to + * the same secure-relative-open context. Returns 0 on successful + * removal, -1 otherwise (with errno reset to EPERM as the original + * unlinkat would have left it). */ +static int force_change_retry_unlinkat(int dirfd, const char *base, int rm_flags) +{ + STRUCT_STAT st; + uint32 fileflags; + int fd = force_change_open_target(dirfd, base, &st, &fileflags); + if (fd < 0) { + errno = EPERM; + return -1; + } + if (make_mutable_fd(fd, st.st_mode, fileflags, force_change) > 0) { + if (unlinkat(dirfd, base, rm_flags) == 0) { + close(fd); + return 0; + } + undo_make_mutable_fd(fd, fileflags); + } + close(fd); + errno = EPERM; + return -1; +} #endif /* SUPPORT_FORCE_CHANGE */ int do_unlink(const char *path) @@ -273,15 +301,17 @@ int do_unlink_at(const char *path) if (dry_run) return 0; RETURN_ERROR_IF_RO_OR_LO; + /* In the non-secure fallback cases, go through do_unlink() (not bare + * unlink()) so the force_change EPERM-recovery in do_unlink fires. */ if (!am_daemon || am_chrooted) - return unlink(path); + return do_unlink(path); if (!path || !*path || *path == '/') - return unlink(path); + return do_unlink(path); slash = strrchr(path, '/'); if (!slash) - return unlink(path); + return do_unlink(path); dlen = slash - path; if (dlen >= sizeof dirpath) { @@ -298,6 +328,17 @@ int do_unlink_at(const char *path) ret = unlinkat(dfd, bname, 0); e = errno; +#ifdef SUPPORT_FORCE_CHANGE + /* Secure-path force_change recovery: reuse the dirfd, fchflags-clear + * the target via the fd, retry unlinkat against the same dirfd. + * Keeps the operation bounded to the secure-relative-open context. */ + if (ret < 0 && e == EPERM && force_change) { + if (force_change_retry_unlinkat(dfd, bname, 0) == 0) { + close(dfd); + return 0; + } + } +#endif close(dfd); errno = e; return ret; @@ -654,6 +695,26 @@ int do_lchown_at(const char *fname, uid_t owner, gid_t group) ret = fchownat(dfd, bname, owner, group, AT_SYMLINK_NOFOLLOW); e = errno; +#ifdef SUPPORT_FORCE_CHANGE + if (ret < 0 && e == EPERM && force_change) { + STRUCT_STAT st; + uint32 fileflags; + int fd = force_change_open_target(dfd, bname, &st, &fileflags); + if (fd >= 0) { + if (make_mutable_fd(fd, st.st_mode, fileflags, force_change) > 0) { + /* fd was opened O_NOFOLLOW so it's not a symlink. */ + int chown_rc = fchown(fd, owner, group); + undo_make_mutable_fd(fd, fileflags); + if (chown_rc == 0) { + close(fd); + close(dfd); + return 0; + } + } + close(fd); + } + } +#endif close(dfd); errno = e; return ret; @@ -861,15 +922,17 @@ int do_rmdir_at(const char *pathname) if (dry_run) return 0; RETURN_ERROR_IF_RO_OR_LO; + /* Route fallback paths through do_rmdir() so its force_change + * recovery still applies. */ if (!am_daemon || am_chrooted) - return rmdir(pathname); + return do_rmdir(pathname); if (!pathname || !*pathname || *pathname == '/') - return rmdir(pathname); + return do_rmdir(pathname); slash = strrchr(pathname, '/'); if (!slash) - return rmdir(pathname); + return do_rmdir(pathname); dlen = slash - pathname; if (dlen >= sizeof dirpath) { @@ -886,6 +949,14 @@ int do_rmdir_at(const char *pathname) ret = unlinkat(dfd, bname, AT_REMOVEDIR); e = errno; +#ifdef SUPPORT_FORCE_CHANGE + if (ret < 0 && e == EPERM && force_change) { + if (force_change_retry_unlinkat(dfd, bname, AT_REMOVEDIR) == 0) { + close(dfd); + return 0; + } + } +#endif close(dfd); errno = e; return ret; @@ -1129,6 +1200,25 @@ int do_chmod_at(const char *fname, mode_t mode) ret = fchmodat(dfd, bname, mode, 0); e = errno; +#ifdef SUPPORT_FORCE_CHANGE + if (ret < 0 && e == EPERM && force_change && !S_ISLNK(mode)) { + STRUCT_STAT st; + uint32 fileflags; + int fd = force_change_open_target(dfd, bname, &st, &fileflags); + if (fd >= 0) { + if (make_mutable_fd(fd, mode, fileflags, force_change) > 0) { + int chmod_rc = fchmod(fd, mode & CHMOD_BITS); + undo_make_mutable_fd(fd, fileflags); + if (chmod_rc == 0) { + close(fd); + close(dfd); + return 0; + } + } + close(fd); + } + } +#endif close(dfd); errno = e; return ret; @@ -1289,6 +1379,17 @@ int do_rename_at(const char *old_path, const char *new_path) if (new_dfd != old_dfd) close(new_dfd); close(old_dfd); +#ifdef SUPPORT_FORCE_CHANGE + /* Secure-path force_change recovery for rename is involved (two-side + * make-mutable plus restore-on-the-renamed-inode bookkeeping that + * do_rename already implements); fall back to do_rename() rather than + * duplicate the logic. This costs another pair of secure_relative_open + * calls (via force_change_open_parent) on the error-recovery path + * only, and stays bounded to curr_dir which on the receiver is the + * destination root -- same scope as the dirfds we just closed. */ + if (ret < 0 && e == EPERM && force_change) + return do_rename(old_path, new_path); +#endif errno = e; return ret; #else From ac1d6a952f3b7fcd60c9e3045a7075350ec92593 Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Wed, 20 May 2026 13:07:26 +1000 Subject: [PATCH 04/11] fileflags: address codex round 2 review findings Three follow-ups after the round-1 codex review caught new issues introduced by the round-1 fixes. #3 (medium, correctness): stat_x_get_fileflags(&sx, fname) in generator.c:1481 was being called without checking statret == 0. For missing destinations sx.st has not been populated; on BSD, rsync_lgetflags() returns hint->st_flags, which is uninitialized stack memory. That garbage was being stashed in F_FFLAGS(file) and later applied/restored by make_mutable / undo_make_mutable on the freshly-created dir. Gate on statret == 0, zero F_FFLAGS otherwise. Also tightened the recv_generator dir-creation make_mutable call from truthy-check to `> 0` so a make_mutable failure (-1) doesn't incorrectly flag need_retouch_dir_perms and trigger a bogus undo_make_mutable in touch_up_dirs. #1 (high, security): the do_rename_at force_change EPERM-recovery that fell back to do_rename(old_path, new_path) reopened the symlink-race attack class on daemon "use chroot = no": do_rename's first action is bare rename(old, new) with raw paths, before its own recovery runs, so an attacker who swaps a parent dir between the secure dirfd close and the fallback rename can redirect the retry outside the module. Replaced with a proper two-side recovery that stays entirely on the already-open secure dirfds: openat(O_NOFOLLOW) the old-side target, fchflags-clear, renameat through the held dirfds; if that fails, do the same on the new-side target. On success, restore the original flags via old_fd (the fd survives the rename and still refers to the inode that now lives at new_bname). Mirrors do_rename's recovery logic but with every operation anchored on the secure dirfds. #2 (medium/high, security): delete_dir_contents's per-entry make_mutable cleared immutable flags before delete_item ran, but delete_item's backup path (make_backup -> do_rename_at or do_link_at + unlink) moves the inode out from under us -- and my undo_make_mutable(fname, ...) is path-based so it can only restore flags at the now-empty original path, never at the backup location the inode actually ended up at. Net effect: a successful backed-up delete of a uchg'd file silently strips the uchg from the backup copy. Fix: only pre-clear flags on SUB-DIRECTORIES (where the +i on the dir blocks delete_dir_contents from recursing). Regular files, symlinks, etc. are left alone -- the underlying do_unlink_at / do_rmdir_at recovery uses fd-based fchflags, and that fd survives a rename or hardlink performed by make_backup, so the recovery correctly preserves the immutable flag on the inode after it lands at its backup location. Verified on Linux/ext4: full suite still 59 passed / 2 skipped / 0 failed. Co-Authored-By: Claude Opus 4.7 (1M context) --- delete.c | 24 ++++++++++------ generator.c | 22 ++++++++++++--- syscall.c | 79 +++++++++++++++++++++++++++++++++++++++++++++-------- 3 files changed, 102 insertions(+), 23 deletions(-) diff --git a/delete.c b/delete.c index 5b5ab0d69..e424d48b8 100644 --- a/delete.c +++ b/delete.c @@ -99,16 +99,24 @@ static enum delret delete_dir_contents(char *fname, uint16 flags) strlcpy(p, fp->basename, remainder); #ifdef SUPPORT_FORCE_CHANGE - /* If force_change is in effect and the entry has immutable - * bits set, clear them so the recursive delete (for dirs) or - * the per-entry delete_item below can proceed. Track whether - * we actually made the change so we can undo on failure -- if - * delete_item ends up NOT removing the entry (max-delete cap, - * non-empty dir, EPERM after retry, ...) we'd otherwise leak - * a less-protected file behind. */ + /* For SUB-DIRECTORIES only: clear the immutable bits so the + * recursive delete_dir_contents below can unlink entries + * inside (the dir's +i blocks all add/remove operations on + * its children). Track so we can undo on failure. + * + * Non-dirs are deliberately NOT pre-cleared here: the + * underlying do_unlink_at / do_rmdir_at force_change recovery + * uses fd-based fchflags, and that fd survives a rename or + * hardlink performed by make_backup() -- so the recovery + * correctly preserves the immutable flag on the inode after + * it lands at its backup location. A path-based pre-clear + * here would lose the flag because our undo only knows the + * original path, which is gone after make_backup moves the + * inode into the backup tree. */ int entry_unmuted = 0; uint32 entry_saved_flags = 0; - if (force_change && (F_FFLAGS(fp) & force_change)) { + if (S_ISDIR(fp->mode) && force_change + && (F_FFLAGS(fp) & force_change)) { if (make_mutable(fname, fp->mode, F_FFLAGS(fp), force_change) > 0) { entry_unmuted = 1; entry_saved_flags = F_FFLAGS(fp); diff --git a/generator.c b/generator.c index 9199739a7..54bace311 100644 --- a/generator.c +++ b/generator.c @@ -1478,8 +1478,17 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx, file->mode = dest_mode(file->mode, sx.st.st_mode, dflt_perms, statret == 0); } #ifdef SUPPORT_FORCE_CHANGE - if (force_change && !preserve_fileflags) - F_FFLAGS(file) = stat_x_get_fileflags(&sx, fname); + if (force_change && !preserve_fileflags) { + /* Only consult the dest's current flags when the + * lstat actually succeeded. For missing entries + * sx.st has not been populated; on BSD, + * rsync_lgetflags() would return hint->st_flags + * which is uninitialized stack memory. */ + if (statret == 0) + F_FFLAGS(file) = stat_x_get_fileflags(&sx, fname); + else + F_FFLAGS(file) = 0; + } #endif if (statret != 0 && basis_dir[0] != NULL) { int j = try_dests_non(file, fname, ndx, fnamecmpbuf, &sx, itemizing, code); @@ -1524,8 +1533,13 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx, * putting files within them. This is then restored to the * former permissions after the transfer is done. */ #ifdef SUPPORT_FORCE_CHANGE - if (force_change && F_FFLAGS(file) & force_change - && make_mutable(fname, file->mode, F_FFLAGS(file), force_change)) + /* make_mutable returns >0 on success, 0 if there was nothing + * to clear, -1 on failure. Only flag need_retouch when we + * actually cleared bits -- otherwise the matching + * undo_make_mutable in touch_up_dirs would try to restore + * flags that were never modified. */ + if (force_change && (F_FFLAGS(file) & force_change) + && make_mutable(fname, file->mode, F_FFLAGS(file), force_change) > 0) need_retouch_dir_perms = 1; #endif #ifdef HAVE_CHMOD diff --git a/syscall.c b/syscall.c index c263cd7b1..843189100 100644 --- a/syscall.c +++ b/syscall.c @@ -1376,20 +1376,77 @@ int do_rename_at(const char *old_path, const char *new_path) ret = renameat(old_dfd, old_bname, new_dfd, new_bname); e = errno; +#ifdef SUPPORT_FORCE_CHANGE + /* Secure-path force_change recovery -- done entirely on the held + * dirfds so the symlink-race guarantees of the outer + * secure_relative_open()s are preserved. Falling back to do_rename() + * here would be wrong: do_rename() starts with a bare rename(old,new) + * before its own recovery (syscall.c:1241), which re-resolves the + * path components through whatever the filesystem says NOW -- a daemon + * (use chroot = no) attacker can swap a parent dir to a symlink after + * our dirfds close and redirect the retry outside the module. + * + * The logic mirrors do_rename's recovery but every operation goes + * through the dirfds: openat(O_NOFOLLOW), fchflags, renameat, + * unlinkat (implicit via renameat's replace), restore via the fd that + * survives the rename. */ + if (ret < 0 && e == EPERM && force_change) { + STRUCT_STAT old_st, new_st; + uint32 old_fileflags = 0, new_fileflags = 0; + int old_fd = -1, new_fd = -1; + int old_mutated = 0, new_mutated = 0; + + /* Try to make the old-side target mutable -- some uchg/uappnd + * flags block rename of the source inode. */ + old_fd = force_change_open_target(old_dfd, old_bname, &old_st, &old_fileflags); + if (old_fd >= 0) { + old_mutated = make_mutable_fd(old_fd, old_st.st_mode, + old_fileflags, force_change) > 0; + if (old_mutated && renameat(old_dfd, old_bname, + new_dfd, new_bname) == 0) { + ret = 0; + goto rename_success; + } + } + + /* Try to make the new-side target mutable -- a uchg/uappnd on + * the file being replaced blocks rename's overwrite. */ + new_fd = force_change_open_target(new_dfd, new_bname, &new_st, &new_fileflags); + if (new_fd >= 0) { + new_mutated = make_mutable_fd(new_fd, new_st.st_mode, + new_fileflags, force_change) > 0; + if (new_mutated && renameat(old_dfd, old_bname, + new_dfd, new_bname) == 0) { + ret = 0; + goto rename_success; + } + } + + /* Rename still failed -- restore both sides. */ + if (new_mutated) + undo_make_mutable_fd(new_fd, new_fileflags); + if (old_mutated) + undo_make_mutable_fd(old_fd, old_fileflags); + goto rename_cleanup; + + rename_success: + /* After a successful rename, the inode that was at old now + * lives at new_bname under new_dfd. Restore its original + * flags via old_fd (the fd survives the rename and still + * refers to that inode). Any new-side mutation can be + * discarded -- the new-side inode is gone. */ + if (old_mutated) + undo_make_mutable_fd(old_fd, old_fileflags); + e = 0; + + rename_cleanup: + if (old_fd != -1) close(old_fd); + if (new_fd != -1) close(new_fd); + } +#endif if (new_dfd != old_dfd) close(new_dfd); close(old_dfd); -#ifdef SUPPORT_FORCE_CHANGE - /* Secure-path force_change recovery for rename is involved (two-side - * make-mutable plus restore-on-the-renamed-inode bookkeeping that - * do_rename already implements); fall back to do_rename() rather than - * duplicate the logic. This costs another pair of secure_relative_open - * calls (via force_change_open_parent) on the error-recovery path - * only, and stays bounded to curr_dir which on the receiver is the - * destination root -- same scope as the dirfds we just closed. */ - if (ret < 0 && e == EPERM && force_change) - return do_rename(old_path, new_path); -#endif errno = e; return ret; #else From b2b6d156ea018c38aca3edc27b84cc36ee92fe07 Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Wed, 20 May 2026 13:57:34 +1000 Subject: [PATCH 05/11] fileflags: fix CI failures from the codex-round-2 push Three independent CI failures on PR #894 commit ac1d6a95: 1. Ubuntu / Ubuntu 22.04 (5 test failures each in make check30): testsuite/rsync.fns quoted "$RSYNC" when probing -VV to detect whether the build supports fileflags. $RSYNC is a command STRING (e.g. "/path/to/rsync --protocol=30"), not a single binary path -- so the quoted form tries to exec a file whose name contains a space, fails silently, and the all_plus/dots widths fall through to the 9-char non-fileflags defaults. On Linux+chattr the rsync build emits 10-char (12-column %i) output, so devices, exclude, itemize, etc. mismatched their expected output. Fix: drop the quotes so shell word-splitting separates the binary from --protocol. Local check30 now clean. 2. NetBSD / OpenBSD (fileflags FAIL): set_fileflags() opened with O_RDONLY|O_NOFOLLOW|O_NONBLOCK and then fchflags'd. Both NetBSD and OpenBSD reject open() with O_NONBLOCK on a directory, returning EISDIR even though POSIX allows opening directories O_RDONLY. Drop O_NONBLOCK: it isn't needed -- rsync_fchflags() rejects everything except S_IFREG and S_IFDIR, and callers already short-circuit on S_ISLNK, so the open target is always a plain file or directory. Neither blocks on plain O_RDONLY. 3. AlmaLinux 8 (RSYNC_EXPECT_SKIPPED mismatch): The base image doesn't have chattr(1), so the fileflags test self-skips with "No chflags(1) or chattr(1) command on this host" even though the rsync build has FS_IOC_GETFLAGS support. Add fileflags back to the AlmaLinux skip-allowlist; comment notes the reason. (Could alternatively dnf install e2fsprogs in the workflow, but the skip is also fine -- Ubuntu / FreeBSD / macOS cover the test on every other CI job.) The remaining 4 CI jobs (Cygwin, FreeBSD, Solaris, macOS) were green on the round-2 push and unaffected by any of these fixes. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/almalinux-8-build.yml | 8 +++++--- rsync.c | 10 +++++++--- testsuite/rsync.fns | 9 ++++++++- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/.github/workflows/almalinux-8-build.yml b/.github/workflows/almalinux-8-build.yml index 9d7ea782e..2e4131004 100644 --- a/.github/workflows/almalinux-8-build.yml +++ b/.github/workflows/almalinux-8-build.yml @@ -58,9 +58,11 @@ jobs: - name: info run: ./rsync --version - name: check - # In the container we already run as root, so no sudo. The - # crtimes-not-supported skip matches the other Linux jobs. - run: RSYNC_EXPECT_SKIPPED=crtimes make check + # In the container we already run as root, so no sudo. crtimes + # has no kernel support here; chattr(1) isn't installed in the + # AlmaLinux 8 base image so the fileflags test self-skips even + # though the rsync build has FS_IOC_GETFLAGS support. + run: RSYNC_EXPECT_SKIPPED=crtimes,fileflags make check - name: ssl file list run: ./rsync-ssl --no-motd download.samba.org::rsyncftp/ || true - name: save artifact diff --git a/rsync.c b/rsync.c index 32bdc12e6..e74b4c8c6 100644 --- a/rsync.c +++ b/rsync.c @@ -511,9 +511,13 @@ static int set_fileflags(const char *fname, uint32 fileflags) int oflags = O_RDONLY | O_NOFOLLOW; STRUCT_STAT st; -#ifdef O_NONBLOCK - oflags |= O_NONBLOCK; -#endif + /* Do NOT add O_NONBLOCK: NetBSD's open() rejects directories + * when O_NONBLOCK is set in the flags, returning EISDIR even + * though POSIX allows opening directories O_RDONLY. We don't + * need O_NONBLOCK here -- rsync_fchflags() rejects everything + * except S_IFREG and S_IFDIR, and callers already short-circuit + * on S_ISLNK, so the open target is always a regular file or + * directory. Neither blocks on plain O_RDONLY. */ if (fname[0] != '/') fd = secure_relative_open(curr_dir, fname, oflags, 0); else diff --git a/testsuite/rsync.fns b/testsuite/rsync.fns index 0c209ee20..952ea9988 100644 --- a/testsuite/rsync.fns +++ b/testsuite/rsync.fns @@ -28,7 +28,14 @@ outfile="$scratchdir/rsync.out" # For itemized output: %i is 12 wide on chflags-supporting builds (the # 12th column is the 'f' flag), 11 wide everywhere else. Detect from the # rsync binary we're testing rather than hard-coding either width. -if "$RSYNC" -VV 2>/dev/null | grep '"file_flags": true' >/dev/null 2>&1; then +# +# $RSYNC is a command STRING (e.g. "/path/to/rsync --protocol=30"), not a +# single binary path -- so it must be expanded WITHOUT quoting so shell +# word-splitting separates the binary from any options. Quoting it as +# "$RSYNC" would make the shell try to exec a binary whose name contains +# a space, which fails, the grep finds nothing, and we'd silently fall +# through to the 9-char defaults even on chflags-capable builds. +if $RSYNC -VV 2>/dev/null | grep '"file_flags": true' >/dev/null 2>&1; then all_plus='++++++++++' allspace=' ' dots='......' # trailing dots after changes From 2ca4200e2106ad11ddea76b4457d0477236fdd08 Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Wed, 20 May 2026 14:03:45 +1000 Subject: [PATCH 06/11] fileflags: address codex round 3 review findings Three more issues caught by codex post-round-2. #1 (high, security): the force_change EPERM-recovery in do_unlink / do_unlink_at / do_rmdir / do_rmdir_at / do_rename_at cleared the target's immutable flags, performed the unlink/rename, then closed the fd without restoring flags. For an inode with st_nlink > 1, the unlink/rename only removed one of several references -- the inode itself survives via its other hardlinks, and those hardlinks were left with the cleared flags. Same shape for rename's replaced-destination inode: renameat() unlinks the old new-side inode; if it had st_nlink > 1, the remaining links to it carry forward whatever flag state we put on it during the make_mutable_fd(). Fix: after a successful unlink/rename, check st_nlink on the pre-operation stat. If > 1, the fd still refers to a surviving inode; restore the original flags via the fd before closing. For the AT_REMOVEDIR (rmdir) case the check is skipped -- a dir's st_nlink counts "." and child-".." references, never additional hardlinks, and after rmdir the inode is gone for good. Applied to: - force_change_retry_unlinkat (do_unlink_at / do_rmdir_at) - do_unlink dirfd-anchored recovery - do_rename_at secure-path recovery (the new_st.st_nlink > 1 case) #2 (high, correctness): the --fileflags / --crtimes protocol gate at compat.c:759 only ran inside the `else if (protocol_version >= 30)` branch. For protocol < 30 (e.g. --protocol=29) xfer_flags_as_varint stays 0 and the check never runs, so --fileflags --protocol=29 would silently let flist.c write the extra fileflags word (and use the XMIT_SAME_FLAGS bit in the 1<<16 position that requires varint flag encoding), desyncing the file-list stream. Move the --fileflags rejection out of the protocol-30 block to right after the if/else if cascade, so it fires for every negotiated protocol where xfer_flags_as_varint is still 0. (The --crtimes gate stays in the protocol-30 block: crtimes support also requires HAVE_GETATTRLIST or __CYGWIN__ and the v-flag dance, which only happens in the >= 30 path.) #3 (medium, correctness): the same uninitialized-stat_x.st bug I fixed for the directory path in generator.c:1481 still existed for the non-directory path at generator.c:1584. stat_x_get_fileflags(&sx, fname) was being called without a statret == 0 guard; for missing destinations sx.st has not been populated, and on BSD rsync_lgetflags() returns hint->st_flags which is uninitialized stack memory. Add the same `statret == 0 ? flags : 0` guard at the non-dir site. Verified on Linux/ext4: 59 passed / 2 skipped / 0 failed. Co-Authored-By: Claude Opus 4.7 (1M context) --- compat.c | 20 ++++++++++++++++---- generator.c | 12 ++++++++++-- syscall.c | 31 ++++++++++++++++++++++++++++--- 3 files changed, 54 insertions(+), 9 deletions(-) diff --git a/compat.c b/compat.c index 787688988..daa08e611 100644 --- a/compat.c +++ b/compat.c @@ -756,10 +756,10 @@ void setup_protocol(int f_out,int f_in) fprintf(stderr, "Both rsync versions must be at least 3.2.0 for --crtimes.\n"); exit_cleanup(RERR_PROTOCOL); } - if (!xfer_flags_as_varint && preserve_fileflags) { - fprintf(stderr, "Both rsync versions must be at least 3.2.0 for --fileflags.\n"); - exit_cleanup(RERR_PROTOCOL); - } + /* --fileflags rejection moved to after the if/else if cascade + * so it also runs for protocol < 30 (where xfer_flags_as_varint + * stays 0 and the receiver wouldn't understand the extra + * fileflags word that flist.c would write). */ if (am_sender) { receiver_symlink_times = am_server ? strchr(client_info, 'L') != NULL @@ -791,6 +791,18 @@ void setup_protocol(int f_out,int f_in) #endif } + /* --fileflags writes an extra word per file in the file-list stream + * and uses the XMIT_SAME_FLAGS bit (1<<16), both of which require the + * varint flag-encoding negotiated by CF_VARINT_FLIST_FLAGS in + * compat_flags. That negotiation only happens in the protocol >= 30 + * branch above, so for protocol < 30 xfer_flags_as_varint stays 0. + * Without this check, --fileflags --protocol=29 would silently emit + * bytes the receiver doesn't expect and desync the file-list stream. */ + if (!xfer_flags_as_varint && preserve_fileflags) { + fprintf(stderr, "Both rsync versions must be at least 3.2.0 for --fileflags.\n"); + exit_cleanup(RERR_PROTOCOL); + } + if (read_batch) do_negotiated_strings = 0; diff --git a/generator.c b/generator.c index 54bace311..c5d957a03 100644 --- a/generator.c +++ b/generator.c @@ -1581,8 +1581,16 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx, file->mode = dest_mode(file->mode, sx.st.st_mode, dflt_perms, exists); } #ifdef SUPPORT_FORCE_CHANGE - if (force_change && !preserve_fileflags) - F_FFLAGS(file) = stat_x_get_fileflags(&sx, fname); + if (force_change && !preserve_fileflags) { + /* Same guard as the dir-path branch above: sx.st is only + * populated when statret == 0; for missing entries it's + * uninitialized stack memory, and on BSD rsync_lgetflags() + * would return that garbage via hint->st_flags. */ + if (statret == 0) + F_FFLAGS(file) = stat_x_get_fileflags(&sx, fname); + else + F_FFLAGS(file) = 0; + } #endif #ifdef SUPPORT_HARD_LINKS diff --git a/syscall.c b/syscall.c index 843189100..ff208b0c8 100644 --- a/syscall.c +++ b/syscall.c @@ -208,7 +208,15 @@ static int force_change_open_target(int dirfd, const char *base, STRUCT_STAT *st * caller's already-opened secure dirfd so the recovery stays bounded to * the same secure-relative-open context. Returns 0 on successful * removal, -1 otherwise (with errno reset to EPERM as the original - * unlinkat would have left it). */ + * unlinkat would have left it). + * + * Hardlink-aware restore: if the inode had st_nlink > 1, the unlinked + * name was just one of several references to the same inode and the + * inode itself survives. Restore the original flags via the still-open + * fd so the *other* hardlinks don't end up less-protected than they + * started. (For AT_REMOVEDIR / dirs this is moot: a dir's st_nlink + * counts "." and child-".." references, never additional hardlinks, + * and after rmdir the inode is gone.) */ static int force_change_retry_unlinkat(int dirfd, const char *base, int rm_flags) { STRUCT_STAT st; @@ -220,6 +228,8 @@ static int force_change_retry_unlinkat(int dirfd, const char *base, int rm_flags } if (make_mutable_fd(fd, st.st_mode, fileflags, force_change) > 0) { if (unlinkat(dirfd, base, rm_flags) == 0) { + if (!(rm_flags & AT_REMOVEDIR) && st.st_nlink > 1) + undo_make_mutable_fd(fd, fileflags); close(fd); return 0; } @@ -261,6 +271,14 @@ int do_unlink(const char *path) } if (make_mutable_fd(fd, st.st_mode, fileflags, force_change) > 0) { if (unlinkat(dirfd, base, 0) == 0) { + /* Hardlink-aware: if this name was one of + * several hardlinks to the inode, the inode + * survives and the other names still hold the + * cleared flags. Restore via the still-open + * fd before close so the surviving links + * aren't left less-protected. */ + if (st.st_nlink > 1) + undo_make_mutable_fd(fd, fileflags); close(fd); close(dirfd); return 0; @@ -1433,10 +1451,17 @@ int do_rename_at(const char *old_path, const char *new_path) /* After a successful rename, the inode that was at old now * lives at new_bname under new_dfd. Restore its original * flags via old_fd (the fd survives the rename and still - * refers to that inode). Any new-side mutation can be - * discarded -- the new-side inode is gone. */ + * refers to that inode). */ if (old_mutated) undo_make_mutable_fd(old_fd, old_fileflags); + /* The new-side inode that the rename replaced was unlinked + * by renameat(); usually that means it's gone. But if it had + * st_nlink > 1, the inode survives via its other hardlinks -- + * and those hardlinks would be left with the cleared flags + * unless we restore via new_fd (which still refers to the + * old-new-inode since the fd was opened before the rename). */ + if (new_mutated && new_st.st_nlink > 1) + undo_make_mutable_fd(new_fd, new_fileflags); e = 0; rename_cleanup: From 56a69a32c46bb6430a13ffa8018b3d4927db5235 Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Wed, 20 May 2026 14:36:30 +1000 Subject: [PATCH 07/11] fileflags: align man page with BSD-patch behaviour; daemon-refuse test Codex round 4 #1 highlighted a contradiction: my man page promised that --fileflags would not make receiver-immutable files alterable without --force-change, but the code (preserved from the original rsync-patches/fileflags.diff) makes destination flags match source flags within SAFE_FILEFLAGS in both directions -- so a source without uchg can clear uchg on a destination that has it. Resolved by matching the manpage to the long-standing BSD code rather than rewriting the receiver-side semantics: - --fileflags is "make dest match source," same as -p / -o / -g. - This is justified because the daemon-default refuse list rejects --fileflags out of the box; a daemon admin has to opt in per-module before clients can hand the daemon's filesystem sender-controlled st_flags. Without that opt-in, the "receiver-immutable bits silently cleared" foot-gun isn't reachable from a hostile remote sender. - SAFE_FILEFLAGS still drops SF_* and UF_NOUNLINK; the --unsafe-fileflags opt-in widens to the full sender value (its own detailed entry now exists too). New testsuite/daemon-refuse-fileflags.test enforces the daemon-default refuse contract: - --fileflags / --unsafe-fileflags refused on a default-policy module (download) by name in the error. - --force-uchange / --force-schange refused on a default-policy module (upload, since the --force-* family only propagates over the wire from a sender via server_options). - --fileflags accepted on a module with "refuse options = !fileflags". Verified Linux/ext4 full suite 60 passed / 2 skipped / 0 failed (was 59 / 2 / 0; +1 for the new test). Co-Authored-By: Claude Opus 4.7 (1M context) --- rsync.1.md | 49 +++++++++++-- testsuite/daemon-refuse-fileflags.test | 96 ++++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 7 deletions(-) create mode 100755 testsuite/daemon-refuse-fileflags.test diff --git a/rsync.1.md b/rsync.1.md index eb1da1391..9a4301bf5 100644 --- a/rsync.1.md +++ b/rsync.1.md @@ -447,6 +447,7 @@ has its own detailed description later in this manpage. --hard-links, -H preserve hard links --perms, -p preserve permissions --fileflags preserve file-flags (aka chflags) +--unsafe-fileflags widen --fileflags to include system flags --executability, -E preserve executability --chmod=CHMOD affect file and/or directory permissions --acls, -A preserve ACLs (implies --perms) @@ -1497,13 +1498,47 @@ expand it. 0. `--fileflags` - This option causes rsync to update the file-flags to be the same as the - source files and directories (if your OS supports the **chflags**(2) system - call). Some flags can only be altered by the super-user and some might - only be unset below a certain secure-level (usually single-user mode). It - will not make files alterable that are set to immutable on the receiver. - To do that, see [`--force-change`](#opt), [`--force-uchange`](#opt), and - [`--force-schange`](#opt). + This option causes rsync to update the file flags on the receiver to be + the same as on the source, the same way it updates mode and ownership + under [`--perms`](#opt) / [`--owner`](#opt) / [`--group`](#opt) -- that + is, "make the destination match the source" rather than "only ever set + flags." If the source has `nodump` set and the destination doesn't, + the destination gets `nodump`; if the source has `uchg` cleared and the + destination has it set, the destination has `uchg` cleared. This + matches the long-standing behaviour of the BSD `--fileflags` patch. + Requires **chflags**(2) (BSD/macOS) or the chattr ioctls + (Linux ext2/3/4, xfs, btrfs, ...) on both sides. + + By default only the bits in a small "safe" set are honoured on the + receiver: `UF_NODUMP`, `UF_IMMUTABLE`, `UF_APPEND`, plus `UF_HIDDEN` + on macOS. Sender-supplied `SF_*` (system-immutable) and `UF_NOUNLINK` + bits are dropped silently, because they're root-only, kernel- + securelevel-locked on BSD, and have caused real foot-guns where a + hostile source could pin files in a way the receiver couldn't easily + clean up. Use [`--unsafe-fileflags`](#opt) to widen the mask to the + full sender value. + + Once a destination has bits in the "safe" set set on it, that + immutable bit blocks rsync from modifying or deleting the file. Use + [`--force-change`](#opt) (or [`--force-uchange`](#opt) / + [`--force-schange`](#opt)) to let rsync clear the bits transparently + around the update. + + Daemon mode refuses `--fileflags`, `--unsafe-fileflags`, and the + `--force-*change` family by default. An admin must explicitly opt + each module in via `refuse options = !fileflags` (etc.) before + clients may use them -- handing a daemon's filesystem over to + sender-controlled immutable bits is dangerous and we want it to be + a deliberate choice. + +0. `--unsafe-fileflags` + + Used together with [`--fileflags`](#opt), this widens the + receiver's "safe" mask to accept the full sender value -- including + the system-immutable (`SF_*`) and `UF_NOUNLINK` bits that + `--fileflags` alone drops. Setting these bits requires root on the + receiver and, on BSD, may be effectively permanent depending on the + running kernel securelevel. Use only when you trust the source. 0. `--force-change`, `--force-uchange` diff --git a/testsuite/daemon-refuse-fileflags.test b/testsuite/daemon-refuse-fileflags.test new file mode 100755 index 000000000..a6d6aad99 --- /dev/null +++ b/testsuite/daemon-refuse-fileflags.test @@ -0,0 +1,96 @@ +#!/bin/sh + +# Copyright (C) 2026 by Andrew Tridgell + +# This program is distributable under the terms of the GNU GPL (see +# COPYING). + +# Test that the rsync daemon refuses --fileflags / --unsafe-fileflags / +# --force-change / --force-uchange / --force-schange by default, even +# on a module that has no explicit "refuse options" setting. These +# options are foot-guns when handed to a daemon -- sender-controlled +# st_flags applied to the daemon's filesystem (--fileflags) or +# receiver-side immutability cleared on demand (--force-change family) +# -- so we want the policy that "you have to explicitly opt in +# per-module before clients can use them," not "you have to explicitly +# refuse them per-module." +# +# Also verify the opt-in works: a module that lists +# "refuse options = !fileflags" accepts --fileflags. + +. "$suitedir/rsync.fns" + +$RSYNC -VV | grep '"file_flags": true' >/dev/null \ + || test_skipped "Rsync is configured without fileflags support" + +build_rsyncd_conf + +# Writable module for the force_change tests (push), read-only for the +# fileflags ones (download). +uploaddir="$scratchdir/upload" +mkdir -p "$uploaddir" +cat >>"$conf" </dev/null 2>"$errlog"; then + cat "$errlog" >&2 + test_fail "$opt was accepted by the daemon (expected refuse)" + fi + if ! grep -- "$opt" "$errlog" >/dev/null; then + cat "$errlog" >&2 + test_fail "$opt: refuse error did not name the option" + fi + echo "ok: daemon refused $opt by default" +done + +# The --force-* family is a sender-side propagation, so it only reaches +# the daemon when the client is uploading (am_sender == 1). Hit a +# writable module with --force-uchange / --force-schange (server_options +# emits the USR/SYS bits as those names rather than --force-change). +for opt in --force-uchange --force-schange; do + echo data > "$scratchdir/push_src" + if $RSYNC $opt "$scratchdir/push_src" localhost::default-rw/ >/dev/null 2>"$errlog"; then + cat "$errlog" >&2 + test_fail "$opt was accepted by the daemon (expected refuse)" + fi + if ! grep -- "$opt" "$errlog" >/dev/null; then + cat "$errlog" >&2 + test_fail "$opt: refuse error did not name the option" + fi + echo "ok: daemon refused $opt by default" +done + +# --fileflags works against the explicit opt-in module. +rm -rf "$todir"; mkdir "$todir" +if ! $RSYNC -a --fileflags localhost::opt-in/ "$todir/" >/dev/null 2>"$errlog"; then + cat "$errlog" >&2 + test_fail "--fileflags refused by opt-in module despite refuse-options = !fileflags" +fi +echo "ok: opt-in module accepts --fileflags" + +# The script would have aborted on error, so getting here means we've won. +exit 0 From ae99ba5288b025cc503a4a5886aa9aaa449c5dc5 Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Wed, 20 May 2026 14:50:49 +1000 Subject: [PATCH 08/11] fileflags: codex round 4 #2 #4 #5 (defer dir immutable, batch flag) #5 (medium): batch.c didn't record preserve_fileflags, so --write-batch --fileflags emits an extra word per file in the file-list stream (XMIT_SAME_FLAGS bit + fileflags word) that --read-batch wouldn't expect without the matching option. Added preserve_fileflags at slot 15 of flag_ptr/flag_name, marked "protocol 30+, requires varint flags" so the bit-position remains stable. Older rsync readers ignore unknown high bits, so this is backwards-readable; newer readers of an old batch see bit 15 unset and correctly leave preserve_fileflags at 0. #4 (medium): if a source directory has immutable / append bits in SAFE_FILEFLAGS (e.g. chflags uchg dir/), set_file_attrs in recv_generator applied them to the dest dir BEFORE the children inside the dir were populated -- and the +i then blocked rsync from creating those children at all. Pass ATTRS_DELAY_IMMUTABLE in the recv_generator set_file_attrs call for dirs, mirroring how finish_transfer already defers immutable bits for temp-then- rename files. touch_up_dirs now re-applies the deferred bits after all children have been processed. #2 (high): the early `continue` in touch_up_dirs at "no times to retouch and no perms to fix" skipped the per-directory force_change restore (undo_make_mutable) too -- so --force-change --omit-dir-times on a +i source dir was leaving the dest dir permanently mutable. Same issue would now affect the new --fileflags deferred-apply path without the fix. Split the continue condition: never skip if there's a force_change restore pending OR a deferred --fileflags apply pending OR the existing times/perms work. Side effect of the #4 fix: set_fileflags() is no longer static because the new touch_up_dirs path calls it directly (cleaner than overloading undo_make_mutable's name). Testsuite addition: fileflags.test now exercises the deferred- immutable-on-dir scenario when the test user can set uchg (only on BSD/macOS or as root on Linux). Creates a uchg source dir with children, rsyncs, asserts the children are present AND the dest dir ends up with uchg set -- which means rsync deferred the bit until after the children populated. Verified Linux/ext4 full suite 60 passed / 2 skipped / 0 failed (the new test block self-skips on non-root Linux since chattr +i needs CAP_LINUX_IMMUTABLE). Co-Authored-By: Claude Opus 4.7 (1M context) --- batch.c | 8 ++++++ generator.c | 55 +++++++++++++++++++++++++++++++++++++-- rsync.c | 2 +- testsuite/fileflags.test | 56 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+), 3 deletions(-) diff --git a/batch.c b/batch.c index 878b6dd1d..bac8ed486 100644 --- a/batch.c +++ b/batch.c @@ -56,6 +56,12 @@ static int tweaked_append; static int tweaked_append_verify; static int tweaked_iconv; +/* preserve_fileflags is referenced by flag_ptr even when SUPPORT_FILEFLAGS + * is not defined (so the bit position in the batch stream is stable + * across builds with and without chflags support). It's defined in + * options.c on every build. */ +extern int preserve_fileflags; + static int *flag_ptr[] = { &recurse, /* 0 */ &preserve_uid, /* 1 */ @@ -72,6 +78,7 @@ static int *flag_ptr[] = { &inplace, /* 12 (protocol 30) */ &tweaked_append, /* 13 (protocol 30) */ &tweaked_append_verify, /* 14 (protocol 30) */ + &preserve_fileflags, /* 15 (protocol 30+, requires varint flags) */ NULL }; @@ -91,6 +98,7 @@ static const char *const flag_name[] = { "--inplace", "--append", "--append-verify", + "--fileflags", NULL }; diff --git a/generator.c b/generator.c index c5d957a03..2be41e3de 100644 --- a/generator.c +++ b/generator.c @@ -1524,7 +1524,13 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx, if (preserve_xattrs && statret == 1) copy_xattrs(fnamecmpbuf, fname); #endif - if (set_file_attrs(fname, file, real_ret ? NULL : &real_sx, NULL, 0) + /* ATTRS_DELAY_IMMUTABLE: hold back any UF_IMMUTABLE / UF_APPEND + * etc that the sender wants on this dir, otherwise applying + * them now would block populating the children we're about + * to recv_generator into the dir. The immutable bits get + * re-applied by touch_up_dirs after children are in place. */ + if (set_file_attrs(fname, file, real_ret ? NULL : &real_sx, NULL, + ATTRS_DELAY_IMMUTABLE) && INFO_GTE(NAME, 1) && code != FNONE && f_out != -1) rprintf(code, "%s/\n", fname); @@ -2165,8 +2171,31 @@ static void touch_up_dirs(struct file_list *flist, int ndx) } /* Be sure not to retouch permissions with --fake-super. */ fix_dir_perms = !am_root && !(file->mode & S_IWUSR); - if (file->flags & FLAG_MISSING_DIR || !(need_retouch_dir_times || fix_dir_perms)) + if (file->flags & FLAG_MISSING_DIR) continue; + /* The body below may need to run for any of: + * - retouch dir times (--times etc) + * - fix dir perms (set above) + * - restore force_change bits we cleared in recv_generator + * - apply deferred --fileflags immutable bits we held back + * in recv_generator so children could populate + * Skip the dir only if there's truly nothing to do. Without + * the SUPPORT_* clauses here, --omit-dir-times + --force-change + * (or --omit-dir-times + --fileflags on an immutable source dir) + * would silently leave the dir mutable / never apply the bits. */ + { + BOOL has_work = need_retouch_dir_times || fix_dir_perms; +#ifdef SUPPORT_FORCE_CHANGE + if (force_change && (F_FFLAGS(file) & force_change)) + has_work = True; +#endif +#ifdef SUPPORT_FILEFLAGS + if (preserve_fileflags && (F_FFLAGS(file) & ALL_IMMUTABLE)) + has_work = True; +#endif + if (!has_work) + continue; + } fname = f_name(file, NULL); if (fix_dir_perms) do_chmod_at(fname, file->mode); @@ -2192,6 +2221,28 @@ static void touch_up_dirs(struct file_list *flist, int ndx) #ifdef SUPPORT_FORCE_CHANGE if (force_change && F_FFLAGS(file) & force_change) undo_make_mutable(fname, F_FFLAGS(file)); +#endif +#ifdef SUPPORT_FILEFLAGS + /* Apply the immutable bits we held back at recv_generator time. + * undo_make_mutable above already restored the dir's pre-clear + * state for the force_change-affected bits; this handles the + * non-force_change case where the sender simply wants the dest + * dir to end up with immutable bits set (e.g. --fileflags on a + * source dir that was chflags +uchg, no --force-change). For + * dirs whose effective wanted-state already matches dest, this + * is a no-op via the current==wanted check inside set_fileflags's + * caller. */ + if (preserve_fileflags && (F_FFLAGS(file) & ALL_IMMUTABLE)) { + stat_x sx_dir; + init_stat_x(&sx_dir); + if (link_stat(fname, &sx_dir.st, 0) == 0) { + uint32 current = stat_x_get_fileflags(&sx_dir, fname); + uint32 wanted = filter_recv_fileflags(F_FFLAGS(file), current); + if (current != wanted) + set_fileflags(fname, wanted); + } + free_stat_x(&sx_dir); + } #endif if (counter >= loopchk_limit) { if (allowed_lull) diff --git a/rsync.c b/rsync.c index e74b4c8c6..01093c537 100644 --- a/rsync.c +++ b/rsync.c @@ -505,7 +505,7 @@ uint32 filter_recv_fileflags(uint32 received, uint32 current_on_dest) * Absolute paths still fall back to a plain open(O_NOFOLLOW): we have * no basedir context for them and they don't occur on the normal * receiver path (only via --partial-dir=/abs and similar). */ -static int set_fileflags(const char *fname, uint32 fileflags) +int set_fileflags(const char *fname, uint32 fileflags) { int fd, rc, save_errno; int oflags = O_RDONLY | O_NOFOLLOW; diff --git a/testsuite/fileflags.test b/testsuite/fileflags.test index 9ce4329d8..1df219676 100755 --- a/testsuite/fileflags.test +++ b/testsuite/fileflags.test @@ -145,4 +145,60 @@ if [ "`show_flags "$fromdir/plain"`" != "`show_flags "$todir/plain"`" ]; then test_fail "unsafe-fileflags should still propagate flags within SAFE_FILEFLAGS" fi + +######################################################################## +# DEFERRED IMMUTABLE BITS ON DIRECTORIES +######################################################################## +# If the source has a uchg (or similar immutable) directory with +# children inside, --fileflags must populate the children BEFORE the +# immutable bit is set on the dest dir -- otherwise the dest dir's +i +# blocks creating the children inside it. rsync should hold back +# immutable bits during recv_generator (via ATTRS_DELAY_IMMUTABLE) +# and re-apply them in touch_up_dirs after all children land. +# +# Only run this part if the test user can set uchg (would have been +# detected by the earlier set_uchg probe). + +if [ "$have_uchg" = 1 ]; then + rm -rf "$fromdir" "$todir" + mkdir "$fromdir" "$todir" + mkdir "$fromdir/locked_dir" + echo inner1 > "$fromdir/locked_dir/inner1" + echo inner2 > "$fromdir/locked_dir/inner2" + set_uchg "$fromdir/locked_dir" \ + || { rm -rf "$fromdir/locked_dir"; have_uchg=0; } +fi + +if [ "$have_uchg" = 1 ]; then + src_locked=`show_flags "$fromdir/locked_dir"` + echo "source locked_dir flags: $src_locked (should contain uchg)" + + # The transfer must succeed even though source dir is uchg -- + # rsync must defer the uchg apply until after children populate. + if ! $RSYNC -rt --fileflags "$fromdir/" "$todir/" >"$outfile" 2>&1; then + cat "$outfile" >&2 + clear_flags "$fromdir/locked_dir" "$todir/locked_dir" || true + test_fail "rsync --fileflags failed on uchg source dir with children" + fi + + # Children must be present and readable + for f in inner1 inner2; do + if [ ! -r "$todir/locked_dir/$f" ]; then + clear_flags "$fromdir/locked_dir" "$todir/locked_dir" || true + test_fail "child $f missing under uchg dest dir (defer didn't fire)" + fi + done + + # Dest dir must have the uchg bit set after the transfer + dst_locked=`show_flags "$todir/locked_dir"` + if [ "x$src_locked" != "x$dst_locked" ]; then + clear_flags "$fromdir/locked_dir" "$todir/locked_dir" || true + test_fail "dest locked_dir flags=$dst_locked != source $src_locked (touch_up_dirs didn't re-apply)" + fi + echo "ok: deferred-immutable on dir: src=$src_locked dst=$dst_locked, children present" + + # Clear so scratchdir teardown can rm the tree + clear_flags "$fromdir/locked_dir" "$todir/locked_dir" || true +fi + exit 0 From 51a088238a7977a2d29b65e102cddb261a2e092c Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Wed, 20 May 2026 14:59:14 +1000 Subject: [PATCH 09/11] fileflags: codex round 4 #3 (do_rename hardlinks) and #6 (chmod mode arg) These two small syscall.c fixes were made at the start of the round-4 work but got dropped on the floor when I split the commit -- only the docs (#1) and the bigger #2/#4/#5 deferred-immutable-dir series ended up landed. The tree was left dirty. #3: do_rename (the non-_at variant) was missing the hardlink-aware restore I added to do_rename_at last round. Same shape -- when renameat replaces a destination inode that had st_nlink > 1, the remaining hardlinks survive carrying the cleared flags. Restore via new_fd before close (the fd still refers to the surviving inode). #6: do_chmod and do_chmod_at force_change recovery were calling make_mutable_fd(fd, mode, ...) where mode was the caller-supplied chmod-target mode -- some callers (notably xattrs.c's set_xattr recovery path) pass perm bits only, no S_IFREG / S_IFDIR, so on Linux rsync_fchflags rejects the call as neither regular file nor directory and recovery silently fails. Use st.st_mode from the freshly-fstatted target instead, which always has the right S_IFx bits. Co-Authored-By: Claude Opus 4.7 (1M context) --- syscall.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/syscall.c b/syscall.c index ff208b0c8..b70c347de 100644 --- a/syscall.c +++ b/syscall.c @@ -1126,7 +1126,12 @@ int do_chmod(const char *path, mode_t mode) errno = EPERM; goto chmod_done; } - if (make_mutable_fd(fd, mode, fileflags, force_change) > 0) { + /* Use st.st_mode (from the fstat we just did) for the + * file-type-classification arg to make_mutable_fd, not the + * caller-supplied chmod mode. Some callers (e.g. xattrs.c) + * pass only permission bits, no S_IFx, so rsync_fchflags() + * would reject it as neither regular file nor directory. */ + if (make_mutable_fd(fd, st.st_mode, fileflags, force_change) > 0) { code = fchmod(fd, mode & CHMOD_BITS); undo_make_mutable_fd(fd, fileflags); if (code == 0) { @@ -1224,7 +1229,10 @@ int do_chmod_at(const char *fname, mode_t mode) uint32 fileflags; int fd = force_change_open_target(dfd, bname, &st, &fileflags); if (fd >= 0) { - if (make_mutable_fd(fd, mode, fileflags, force_change) > 0) { + /* st.st_mode (not the caller-supplied chmod mode): + * callers like xattrs.c pass perm bits only, no S_IFx, + * which would fail rsync_fchflags's type check. */ + if (make_mutable_fd(fd, st.st_mode, fileflags, force_change) > 0) { int chmod_rc = fchmod(fd, mode & CHMOD_BITS); undo_make_mutable_fd(fd, fileflags); if (chmod_rc == 0) { @@ -1299,6 +1307,13 @@ int do_rename(const char *old_path, const char *new_path) * fd (the fd survives the rename). */ if (old_mutated) undo_make_mutable_fd(old_fd, old_fileflags); + /* If the replaced new-side inode had st_nlink > 1 the + * renameat only removed one of its names; the inode + * itself survives via the other hardlinks and would be + * left with the cleared flags unless we restore via + * new_fd (which still refers to that surviving inode). */ + if (new_mutated && new_st.st_nlink > 1) + undo_make_mutable_fd(new_fd, new_fileflags); close(old_fd); close(old_dirfd); if (new_fd != -1) close(new_fd); From 9cf6c659fcd982efc8933735eecb5c11dbb2fc90 Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Wed, 20 May 2026 15:58:51 +1000 Subject: [PATCH 10/11] fileflags: address codex round 5 review findings Five findings from another codex pass: 1. set_fileflags() and force_change_open_parent() were anchoring relative paths at curr_dir-as-a-path-string via secure_relative_open(curr_dir, ...). A path-string basedir is re-traversed component-by-component on every call -- which reopens the same symlink-race attack window the phase-3 work was supposed to close, this time for the daemon "use chroot = no" case where module-root parents are reachable by other users. Switch both call sites to secure_relative_open(NULL, ...), which anchors at AT_FDCWD: a stable kernel-held reference the receiver set up via change_dir() at startup, not a path that gets re-walked. 2. --force-change without --fileflags promises in the manpage that the dest's original flags are restored after the transfer, but finish_transfer() only re-applied flags when preserve_fileflags was set. For the common single-link case this silently dropped immutable bits after every update. The F_FFLAGS() stash the generator does in recv_generator() isn't visible to the receiver process, so capture the dest's flags ourselves via do_lstat() at finish_transfer entry (before any rename) and re-apply the force_change-affected subset after the rename succeeds. Add testsuite coverage. 3. set_file_attrs() chmods first and then calls set_fileflags(), which open(O_RDONLY)s the file -- a non-root receiver transferring a mode-000 source with chflags+immutable would fail at the open with EACCES. Add an EACCES retry that do_lstat()s the path, temporarily widens the mode to add S_IRUSR via path-chmod, re-opens, then restores via fchmod through the fd we just acquired (before the fchflags so the restore isn't blocked by a freshly-set immutable bit). 4. force_change_open_target() still set O_NONBLOCK even though set_fileflags() had already dropped it after the NetBSD/OpenBSD EISDIR trap was discovered in round 2. The same trap fires on the recovery path for directories (rmdir/chmod force_change). Drop O_NONBLOCK and document why neither site needs it. 5. The %i itemize manpage prose still said "11 letters long" and never documented the new 'f' (fileflags) attribute despite the example already being updated to `YXcstpoguaxf`. Update the prose to 12 letters and add an 'f' bullet next to 'a'/'x'. Also fix a misleading-indentation lint that snuck in with the do_rename_at hardlink-restore commit: the rename_fail: label was indented two spaces instead of being at column 0. Linux: 60 passed / 2 skipped / 0 failed. FreeBSD: 51 passed / 8 skipped / 0 failed (uchg block self-skips on ZFS as before). macOS: fileflags test passes including the new --force-change restore subtest ("ok: --force-change restored flags=2 after rename"). Co-Authored-By: Claude Opus 4.7 (1M context) --- rsync.1.md | 4 +- rsync.c | 108 +++++++++++++++++++++++++++++++++++---- syscall.c | 37 +++++++++----- testsuite/fileflags.test | 55 ++++++++++++++++++++ 4 files changed, 178 insertions(+), 26 deletions(-) diff --git a/rsync.1.md b/rsync.1.md index 9a4301bf5..0cf3470c4 100644 --- a/rsync.1.md +++ b/rsync.1.md @@ -3199,7 +3199,7 @@ expand it. version 2.6.7 (you can use `-vv` with older versions of rsync, but that also turns on the output of other verbose messages). - The "%i" escape has a cryptic output that is 11 letters long. The general + The "%i" escape has a cryptic output that is 12 letters long. The general format is like the string `YXcstpoguaxf`, where **Y** is replaced by the type of update being done, **X** is replaced by the file-type, and the other letters represent attributes that may be output if they are being modified. @@ -3262,6 +3262,8 @@ expand it. - `b` means that both the access and create times are being updated - The `a` means that the ACL information is being changed. - The `x` means that the extended attribute information is being changed. + - The `f` means that the BSD/Linux file flags are being changed (requires + [`--fileflags`](#opt)). One other output is possible: when deleting files, the "%i" will output the string "`*deleting`" for each item that is being removed (assuming that you diff --git a/rsync.c b/rsync.c index 01093c537..89da4fa2e 100644 --- a/rsync.c +++ b/rsync.c @@ -33,6 +33,7 @@ extern int preserve_xattrs; extern int preserve_perms; extern int preserve_fileflags; extern int preserve_unsafe_fileflags; +extern int force_change; extern int preserve_executability; extern int preserve_mtimes; extern int omit_dir_times; @@ -489,18 +490,19 @@ uint32 filter_recv_fileflags(uint32 received, uint32 current_on_dest) /* Set a file's BSD-canonical fileflags, via the portable wrapper that * handles both BSD fchflags(fd, ...) and Linux ioctl(FS_IOC_SETFLAGS). * - * SECURITY: relative paths are opened via secure_relative_open(curr_dir, + * SECURITY: relative paths are opened via secure_relative_open(NULL, * fname, ...) so the full path chain is bounded -- on Linux 5.6+, * FreeBSD 13+ and macOS 15+ this uses openat2/openat with - * RESOLVE_BENEATH (or O_RESOLVE_BENEATH), and elsewhere falls back to a - * per-component O_NOFOLLOW walk. This closes the same daemon - * (use chroot = no) symlink-race attack class as CVE-2026-29518: a - * plain open(O_NOFOLLOW) only protects the *final* path component, so - * a local attacker with write access to one of the parent directories - * can swap a parent dir to a symlink between calls and redirect the - * fchflags outside the receiver's confinement. curr_dir is the - * receiver's destination root (set by change_dir() before the - * file-ops phase), so we anchor to that. + * RESOLVE_BENEATH (or O_RESOLVE_BENEATH), and elsewhere falls back to + * a per-component O_NOFOLLOW walk. This closes the same daemon + * (use chroot = no) symlink-race attack class as CVE-2026-29518. + * + * basedir = NULL means "anchor at AT_FDCWD" -- the cwd the receiver + * already set via change_dir() before the file-ops phase, kernel-level + * confined and not re-traversed component-by-component on each call. + * The earlier basedir=curr_dir variant re-opened curr_dir-as-a-path- + * string each call, which made the basedir itself re-resolvable through + * any swapped components above the module root. * * Absolute paths still fall back to a plain open(O_NOFOLLOW): we have * no basedir context for them and they don't occur on the normal @@ -509,6 +511,8 @@ int set_fileflags(const char *fname, uint32 fileflags) { int fd, rc, save_errno; int oflags = O_RDONLY | O_NOFOLLOW; + int restore_mode = -1; /* mode_t to restore after, -1 = no restore */ + STRUCT_STAT lst; STRUCT_STAT st; /* Do NOT add O_NONBLOCK: NetBSD's open() rejects directories @@ -519,17 +523,57 @@ int set_fileflags(const char *fname, uint32 fileflags) * on S_ISLNK, so the open target is always a regular file or * directory. Neither blocks on plain O_RDONLY. */ if (fname[0] != '/') - fd = secure_relative_open(curr_dir, fname, oflags, 0); + fd = secure_relative_open(NULL, fname, oflags, 0); else fd = open(fname, oflags); + if (fd < 0 && errno == EACCES) { + /* set_file_attrs chmods before set_fileflags, so if the + * caller's target mode lacks owner-read (e.g. 000 backup of + * a chattr+i secret file), the O_RDONLY open above fails + * for non-root users. Recover by lstat-ing the path, + * temporarily widening the mode to add owner-read, retrying + * the open, then restoring via fchmod through the fd we + * just acquired (which closes the path-based TOCTOU window + * on the restore step). Refuse anything that isn't a + * regular file or directory, both to keep within the same + * set_fileflags target types and to avoid chmod-ing through + * a setuid program file we don't own. */ + if (do_lstat(fname, &lst) == 0 + && (S_ISREG(lst.st_mode) || S_ISDIR(lst.st_mode)) + && !(lst.st_mode & S_IRUSR) + && do_chmod_at(fname, lst.st_mode | S_IRUSR) == 0) { + restore_mode = lst.st_mode & CHMOD_BITS; + if (fname[0] != '/') + fd = secure_relative_open(NULL, fname, oflags, 0); + else + fd = open(fname, oflags); + if (fd < 0) { + save_errno = errno; + do_chmod_at(fname, restore_mode); + errno = save_errno; + } + } + } if (fd < 0) goto fail; if (fstat(fd, &st) != 0) { save_errno = errno; + if (restore_mode != -1) + (void)fchmod(fd, restore_mode); close(fd); errno = save_errno; goto fail; } + if (restore_mode != -1) { + /* Restore via fd, BEFORE the fchflags below -- if the + * fchflags sets immutable, a later fchmod would be + * rejected by the kernel even through the existing fd. + * fchmod here works because the inode is still mutable and + * we own it; the already-open fd retains its read access + * across the mode drop (POSIX checks read perm at open(), + * not at use). */ + (void)fchmod(fd, restore_mode); + } rc = rsync_fchflags(fd, st.st_mode, fileflags); save_errno = errno; close(fd); @@ -862,6 +906,27 @@ int finish_transfer(const char *fname, const char *fnametmp, { int ret; const char *temp_copy_name = partialptr && *partialptr != '/' ? partialptr : NULL; +#ifdef SUPPORT_FORCE_CHANGE + /* For force_change without preserve_fileflags, the generator's + * F_FFLAGS stash isn't visible to us (the receiver runs in a + * separate process from the generator and we never decoded the + * dest's flags off the wire -- they're a local-only fact). Stat + * the dest ourselves before any rename so we can re-apply the + * force_change-affected bits afterwards. Without this, the + * manpage's "original flags are restored" promise silently fails + * for single-link rename-replaced destinations. */ + uint32 fc_pre_flags = 0; + if (force_change && !preserve_fileflags && !inplace) { + stat_x sx_dst; + init_stat_x(&sx_dst); + if (do_lstat(fname, &sx_dst.st) == 0 + && (S_ISREG(sx_dst.st.st_mode) || S_ISDIR(sx_dst.st.st_mode))) + fc_pre_flags = stat_x_get_fileflags(&sx_dst, fname); + if (fc_pre_flags == NO_FFLAGS) + fc_pre_flags = 0; + free_stat_x(&sx_dst); + } +#endif if (inplace) { if (DEBUG_GTE(RECV, 1)) @@ -908,6 +973,18 @@ int finish_transfer(const char *fname, const char *fnametmp, if (wanted & ALL_IMMUTABLE) set_fileflags(fname, wanted); } +#endif +#ifdef SUPPORT_FORCE_CHANGE + /* Restore the dest's pre-rename force_change-affected bits. + * The temp inode now sits at fname with no flags, so + * without this the manpage's promise that the "original + * flags are restored" silently fails for the common + * single-link case. fc_pre_flags was captured at function + * entry via lstat (preserve_fileflags already took the + * separate restoration path above). */ + if (force_change && !preserve_fileflags + && (fc_pre_flags & force_change)) + set_fileflags(fname, fc_pre_flags & force_change); #endif return 1; } @@ -926,6 +1003,15 @@ int finish_transfer(const char *fname, const char *fnametmp, return 0; } handle_partial_dir(temp_copy_name, PDIR_DELETE); +#ifdef SUPPORT_FORCE_CHANGE + /* Same rename-replaced-the-inode hazard as the move path + * above: the temp inode now sits at fname with no flags, + * so restore the force_change-affected bits captured at + * function entry. */ + if (force_change && !preserve_fileflags + && (fc_pre_flags & force_change)) + set_fileflags(fname, fc_pre_flags & force_change); +#endif } return 1; } diff --git a/syscall.c b/syscall.c index b70c347de..c9bf304d0 100644 --- a/syscall.c +++ b/syscall.c @@ -133,16 +133,18 @@ static int split_dir_base(const char *path, char *dirbuf, size_t dirbuf_sz, * basename pointer. Caller closes dirfd. * * SECURITY (phase 3): when path is relative we anchor the parent-dir - * open at curr_dir via secure_relative_open(), which uses + * open at AT_FDCWD via secure_relative_open(NULL, ...), which uses * openat2(RESOLVE_BENEATH) on Linux 5.6+, openat(O_RESOLVE_BENEATH) on * FreeBSD 13+ / macOS 15+, and a per-component O_NOFOLLOW walk - * elsewhere. All of these reject any path that escapes curr_dir via - * ".." or via a symlink that points outside the subtree. On the - * receiver curr_dir IS the destination root (set via change_dir() - * before the file-ops phase), so the effect is to bound the - * force_change recovery to the destination tree -- a sender that - * managed to inject a path like "../etc/master.passwd" still gets the - * openat refused at the kernel. + * elsewhere. All of these reject any path that escapes the anchor via + * ".." or via a symlink that points outside the subtree. The receiver + * change_dir()s into the destination root before the file-ops phase, + * so AT_FDCWD IS the destination root -- and unlike a basedir passed + * as a path string (which would be re-traversed component-by-component + * on every call, re-exposing any swapped parent dirs above the module + * root), AT_FDCWD is a stable kernel-held reference set up once at + * startup. A sender that managed to inject a path like + * "../etc/master.passwd" still gets the openat refused at the kernel. * * For absolute paths we fall back to the phase-2 path-based open * (O_NOFOLLOW on the parent dir, no RESOLVE_BENEATH). Absolute paths @@ -160,7 +162,7 @@ static int force_change_open_parent(const char *path, const char **base_out) #define O_DIRECTORY 0 #endif if (dirbuf[0] != '/') { - dirfd = secure_relative_open(curr_dir, dirbuf, + dirfd = secure_relative_open(NULL, dirbuf, O_RDONLY | O_DIRECTORY | O_NOFOLLOW, 0); /* Do NOT fall back to a path-based open on failure -- that * would re-enable the escape route we just closed. */ @@ -174,15 +176,22 @@ static int force_change_open_parent(const char *path, const char **base_out) * on a symlink the attacker substituted), then fstat to learn its current * mode and read its current BSD-canonical fileflags (via rsync_fgetflags, * which abstracts BSD fstat-st_flags vs Linux ioctl(FS_IOC_GETFLAGS)). - * Returns fd on success, -1 with errno set on failure. */ + * Returns fd on success, -1 with errno set on failure. + * + * NB: we don't set O_NONBLOCK here even though the target may be a fifo -- + * the force_change recovery path also fires on directories (rmdir, + * chmod), and NetBSD/OpenBSD reject O_NONBLOCK|O_RDONLY on directory + * open() with EISDIR/EPERM. This is the same trap that set_fileflags + * documents at rsync.c. A fifo open *without* O_NONBLOCK can block in + * theory, but openat with O_NOFOLLOW on a fifo with no other end behaves + * the same as the unlinkat would have -- in practice the receiver only + * reaches this path after its own unlinkat or chmod failed on a node it + * just generated, and the kernel doesn't make us wait. */ static int force_change_open_target(int dirfd, const char *base, STRUCT_STAT *stp, uint32 *flags_out) { int oflags = O_RDONLY | O_NOFOLLOW; int fd; -#ifdef O_NONBLOCK - oflags |= O_NONBLOCK; /* don't block on fifos */ -#endif fd = openat(dirfd, base, oflags); if (fd < 0) return -1; @@ -1324,7 +1333,7 @@ int do_rename(const char *old_path, const char *new_path) undo_make_mutable_fd(new_fd, new_fileflags); if (old_mutated) undo_make_mutable_fd(old_fd, old_fileflags); - rename_fail: + rename_fail: if (old_fd != -1) close(old_fd); if (old_dirfd != -1) close(old_dirfd); if (new_fd != -1) close(new_fd); diff --git a/testsuite/fileflags.test b/testsuite/fileflags.test index 1df219676..8ac79827a 100755 --- a/testsuite/fileflags.test +++ b/testsuite/fileflags.test @@ -201,4 +201,59 @@ if [ "$have_uchg" = 1 ]; then clear_flags "$fromdir/locked_dir" "$todir/locked_dir" || true fi +######################################################################## +# --force-change restores flags after the transfer +######################################################################## +# The manpage says "--force-change ... The original flags are restored +# after the update." Without preserve_fileflags, finish_transfer used +# to silently skip that restore for the common single-link case +# (codex round 5 #2). Verify the restore actually happens: dest has +# uchg, source has different content + no flags, after the transfer +# the dest content matches the source AND the dest still has uchg. +# +# Only runs if the test user can set uchg. + +if [ "$have_uchg" = 1 ]; then + rm -rf "$fromdir" "$todir" + mkdir "$fromdir" "$todir" + # Make source content distinctly different size from dest so the + # transfer happens regardless of quick-check timestamps. + echo "new content -- forced through uchg via --force-change" > "$fromdir/forced" + echo "old" > "$todir/forced" + if ! set_uchg "$todir/forced"; then + # Filesystem doesn't allow uchg here even though set_uchg + # worked earlier on $fromdir -- skip this block rather + # than fail the whole test. + rm -f "$todir/forced" + else + dst_pre=`show_flags "$todir/forced"` + + # --force-change without --fileflags: rsync should bully + # through the uchg, replace the inode via temp+rename, and + # put uchg back. + if ! $RSYNC --force-change -t "$fromdir/forced" "$todir/forced" \ + >"$outfile" 2>&1; then + cat "$outfile" >&2 + clear_flags "$todir/forced" || true + test_fail "rsync --force-change failed on uchg dest" + fi + + dst_post=`show_flags "$todir/forced"` + if [ "x$dst_pre" != "x$dst_post" ]; then + clear_flags "$todir/forced" || true + test_fail "force_change did not restore flags: pre=$dst_pre post=$dst_post" + fi + + # Content must have been transferred + if ! cmp "$fromdir/forced" "$todir/forced" >/dev/null 2>&1; then + clear_flags "$todir/forced" || true + test_fail "force_change transfer left dest content stale" + fi + echo "ok: --force-change restored flags=$dst_post after rename" + + # Clear so teardown can remove the file + clear_flags "$todir/forced" || true + fi +fi + exit 0 From 92eafc60e06cf21efba80057d36563b4010efefc Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Wed, 20 May 2026 16:33:26 +1000 Subject: [PATCH 11/11] fileflags: fix CI failures on Cygwin, NetBSD, OpenBSD, Ubuntu Four classes of failure surfaced after the round-5 push: NetBSD/OpenBSD ("Is a directory" on set_fileflags for dirs): secure_relative_open()'s per-component-walk fallback (used on hosts without RESOLVE_BENEATH-equivalent kernel support) returns EISDIR when the resolved path is a directory and the caller didn't pass O_DIRECTORY -- it doesn't second-guess the caller's intent. set_fileflags() is happy with either a regular file or a directory (rsync_fchflags handles both), so introduce an open_for_fileflags() helper that retries with O_DIRECTORY when the first open returns EISDIR. Linux/FreeBSD/macOS take the kernel RESOLVE_BENEATH path which allows the first call through, so the retry is dead on those hosts and only fires where the fallback is in use. The same helper is reused on the EACCES retry path so the round-5 mode-000 fix covers both regular files and directories correctly. Ubuntu/Ubuntu 22.04 (protocol incompatibility under check29): --fileflags requires the varint flag encoding which is only negotiated at protocol >= 30 (CF_VARINT_FLIST_FLAGS); under make check29 (--protocol=29) the compat.c handshake correctly aborts. The test scripts already had the right behaviour at runtime -- they just weren't recognising that the abort under proto 29 was expected. Have fileflags.test and daemon-refuse-fileflags.test inspect $RSYNC for "--protocol=2N" where N <= 9 and self-skip cleanly. Update the Ubuntu CI workflows so the check29 step expects these two tests to skip (check and check30 are unchanged). Cygwin (daemon-refuse-fileflags newly-skipped, not in expected list): Cygwin builds without fileflags support, so the new daemon-refuse-fileflags test now self-skips with the file_flags=true probe just like fileflags.test does. Add it to the Cygwin RSYNC_EXPECT_SKIPPED list. Linux check / check29 / check30 all clean locally. FreeBSD check29 clean: SKIPs both fileflags tests as expected. macOS fileflags test still PASS, including the new --force-change restore subtest from round 5. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/cygwin-build.yml | 2 +- .github/workflows/ubuntu-22.04-build.yml | 2 +- .github/workflows/ubuntu-build.yml | 2 +- rsync.c | 39 ++++++++++++++++++------ testsuite/daemon-refuse-fileflags.test | 9 ++++++ testsuite/fileflags.test | 11 +++++++ 6 files changed, 53 insertions(+), 12 deletions(-) diff --git a/.github/workflows/cygwin-build.yml b/.github/workflows/cygwin-build.yml index 2beef871b..1892a2d6b 100644 --- a/.github/workflows/cygwin-build.yml +++ b/.github/workflows/cygwin-build.yml @@ -39,7 +39,7 @@ jobs: - name: info run: bash -c '/usr/local/bin/rsync --version' - name: check - run: bash -c 'RSYNC_EXPECT_SKIPPED=acls-default,acls,bare-do-open-symlink-race,chdir-symlink-race,chmod-symlink-race,chown,daemon-chroot-acl,devices,dir-sgid,fileflags,open-noatime,protected-regular,sender-flist-symlink-leak,simd-checksum,symlink-dirlink-basis make check' + run: bash -c 'RSYNC_EXPECT_SKIPPED=acls-default,acls,bare-do-open-symlink-race,chdir-symlink-race,chmod-symlink-race,chown,daemon-chroot-acl,daemon-refuse-fileflags,devices,dir-sgid,fileflags,open-noatime,protected-regular,sender-flist-symlink-leak,simd-checksum,symlink-dirlink-basis make check' - name: ssl file list run: bash -c 'PATH="/usr/local/bin:$PATH" rsync-ssl --no-motd download.samba.org::rsyncftp/ || true' - name: save artifact diff --git a/.github/workflows/ubuntu-22.04-build.yml b/.github/workflows/ubuntu-22.04-build.yml index 0e608279e..bf0a0ea82 100644 --- a/.github/workflows/ubuntu-22.04-build.yml +++ b/.github/workflows/ubuntu-22.04-build.yml @@ -43,7 +43,7 @@ jobs: - name: check30 run: sudo RSYNC_EXPECT_SKIPPED=crtimes make check30 - name: check29 - run: sudo RSYNC_EXPECT_SKIPPED=crtimes make check29 + run: sudo RSYNC_EXPECT_SKIPPED=crtimes,daemon-refuse-fileflags,fileflags make check29 - name: ssl file list run: rsync-ssl --no-motd download.samba.org::rsyncftp/ || true - name: save artifact diff --git a/.github/workflows/ubuntu-build.yml b/.github/workflows/ubuntu-build.yml index 5efadce5b..d2fbde78d 100644 --- a/.github/workflows/ubuntu-build.yml +++ b/.github/workflows/ubuntu-build.yml @@ -39,7 +39,7 @@ jobs: - name: check30 run: sudo RSYNC_EXPECT_SKIPPED=crtimes make check30 - name: check29 - run: sudo RSYNC_EXPECT_SKIPPED=crtimes make check29 + run: sudo RSYNC_EXPECT_SKIPPED=crtimes,daemon-refuse-fileflags,fileflags make check29 - name: ssl file list run: rsync-ssl --no-motd download.samba.org::rsyncftp/ || true - name: save artifact diff --git a/rsync.c b/rsync.c index 89da4fa2e..a4ae64035 100644 --- a/rsync.c +++ b/rsync.c @@ -487,6 +487,34 @@ uint32 filter_recv_fileflags(uint32 received, uint32 current_on_dest) return (received & mask) | (current_on_dest & ~mask); } +/* Try to open fname O_RDONLY|O_NOFOLLOW via secure_relative_open + * (relative) or plain open() (absolute). If the per-component-walk + * fallback returns EISDIR (the resolved target is a directory and we + * didn't ask for O_DIRECTORY), retry with O_DIRECTORY -- set_fileflags + * is happy with either type and the fallback enforces O_DIRECTORY + * stricter than the kernel RESOLVE_BENEATH path does. Returns fd on + * success, -1 with errno preserved on failure. */ +static int open_for_fileflags(const char *fname) +{ + int oflags = O_RDONLY | O_NOFOLLOW; + int fd; + + if (fname[0] != '/') + fd = secure_relative_open(NULL, fname, oflags, 0); + else + fd = open(fname, oflags); +#ifdef O_DIRECTORY + if (fd < 0 && errno == EISDIR) { + oflags |= O_DIRECTORY; + if (fname[0] != '/') + fd = secure_relative_open(NULL, fname, oflags, 0); + else + fd = open(fname, oflags); + } +#endif + return fd; +} + /* Set a file's BSD-canonical fileflags, via the portable wrapper that * handles both BSD fchflags(fd, ...) and Linux ioctl(FS_IOC_SETFLAGS). * @@ -510,7 +538,6 @@ uint32 filter_recv_fileflags(uint32 received, uint32 current_on_dest) int set_fileflags(const char *fname, uint32 fileflags) { int fd, rc, save_errno; - int oflags = O_RDONLY | O_NOFOLLOW; int restore_mode = -1; /* mode_t to restore after, -1 = no restore */ STRUCT_STAT lst; STRUCT_STAT st; @@ -522,10 +549,7 @@ int set_fileflags(const char *fname, uint32 fileflags) * except S_IFREG and S_IFDIR, and callers already short-circuit * on S_ISLNK, so the open target is always a regular file or * directory. Neither blocks on plain O_RDONLY. */ - if (fname[0] != '/') - fd = secure_relative_open(NULL, fname, oflags, 0); - else - fd = open(fname, oflags); + fd = open_for_fileflags(fname); if (fd < 0 && errno == EACCES) { /* set_file_attrs chmods before set_fileflags, so if the * caller's target mode lacks owner-read (e.g. 000 backup of @@ -543,10 +567,7 @@ int set_fileflags(const char *fname, uint32 fileflags) && !(lst.st_mode & S_IRUSR) && do_chmod_at(fname, lst.st_mode | S_IRUSR) == 0) { restore_mode = lst.st_mode & CHMOD_BITS; - if (fname[0] != '/') - fd = secure_relative_open(NULL, fname, oflags, 0); - else - fd = open(fname, oflags); + fd = open_for_fileflags(fname); if (fd < 0) { save_errno = errno; do_chmod_at(fname, restore_mode); diff --git a/testsuite/daemon-refuse-fileflags.test b/testsuite/daemon-refuse-fileflags.test index a6d6aad99..e3c96ba54 100755 --- a/testsuite/daemon-refuse-fileflags.test +++ b/testsuite/daemon-refuse-fileflags.test @@ -23,6 +23,15 @@ $RSYNC -VV | grep '"file_flags": true' >/dev/null \ || test_skipped "Rsync is configured without fileflags support" +# Daemon mode for fileflags negotiates the varint flag encoding which +# only exists at protocol >= 30. Under check29 the daemon handshake +# fails before we can exercise the refuse-options policy. +case " $RSYNC " in + *" --protocol=29 "*|*" --protocol=2"[0-8]" "*) + test_skipped "fileflags daemon test requires protocol >= 30" + ;; +esac + build_rsyncd_conf # Writable module for the force_change tests (push), read-only for the diff --git a/testsuite/fileflags.test b/testsuite/fileflags.test index 8ac79827a..af9eec164 100755 --- a/testsuite/fileflags.test +++ b/testsuite/fileflags.test @@ -18,6 +18,17 @@ $RSYNC -VV | grep '"file_flags": true' >/dev/null \ || test_skipped "Rsync is configured without fileflags support" +# --fileflags requires the varint flag encoding negotiated by +# CF_VARINT_FLIST_FLAGS, which only happens at protocol >= 30. Under +# check29 the $RSYNC string includes --protocol=29 and compat.c will +# abort with "Both rsync versions must be at least 3.2.0 for --fileflags" +# -- skip the test rather than fail. +case " $RSYNC " in + *" --protocol=29 "*|*" --protocol=2"[0-8]" "*) + test_skipped "--fileflags requires protocol >= 30" + ;; +esac + # Pick the platform-appropriate userland tool and readback function. if command -v chflags >/dev/null 2>&1; then # BSD/macOS path