Conversation
Introduce a vfs/ subtree that will house rsync's filesystem-handling code (the do_* syscall wrappers, the race-safe path resolver, the held-dirfd cache, the operator-path ownership walk and daemon module confinement), separating those security-critical details from the protocol/transfer logic. This first commit only stands up the layer; no code is moved yet, so behavior is identical. - vfs/vfs.h: public interface, included by rsync.h just after proto.h. Declares "struct vfs" -- the single global that will hold the state currently scattered across syscall.c statics (the dirfd cache, curr_dir, operator_path_resolve) and the clientserver.c module_* globals -- plus vfs_init(). - vfs/vfs.c: defines the global instance with a designated initializer so the cache and module snapshot are safe by construction (a plain definition would zero base/module_dirfd, making fd 0 look valid); the t_*_secure harnesses never run main(), so this must not rely on vfs_init(). vfs_init() resets between transfers (inert for now). - Build: bundle the VFS into a static libvfs.a linked last on rsync and every test harness, so later commits can move code out of syscall.o without breaking a harness link (the linker pulls only what it needs). AC_CHECK_TOOL(AR)/AC_PROG_RANLIB added for portable archiving; a vfs/dummy config-file output creates vfs/ in VPATH builds. - main.c calls vfs_init() early (establishes the call site).
First step of relocating the security core. The component-walk dirfd stack (struct dirstack + the ds_* helpers) and the module-confinement helpers (path_has_dotdot_component, abspath_excluded_by_module, open_anchor_dirfd) move verbatim out of syscall.c into vfs/dirstack.c. The secure resolver (still in syscall.c) and the held-dirfd cache reach them through a new private header vfs/vfs_internal.h. The functions are byte-identical to before; only their linkage changes (the ones syscall.c still calls become non-static; ds_path_push/ ds_path_pop/ds_push stay file-local). struct dirstack and SECURE_OPEN_MAXSYMLINKS now live in vfs_internal.h so both sides see one definition. vfs_internal.h also centralizes the option/daemon externs the VFS internals read. No behavior change.
Relocate the race-safe resolver and its policy gates out of syscall.c into vfs/secure_open.c, and give them the vfs_* public names that the mainline code will use going forward: secure_relative_open -> vfs_resolve_open secure_relative_open_at -> vfs_resolve_open_at secure_relpath_active -> vfs_relpath_active symlink_optout_allowed -> vfs_symlink_optout_allowed secure_walk_at stays file-local. The function bodies are unchanged; the call sites across receiver/sender/generator/flist/util1/clientserver/ main/options and the test harnesses are updated to the new names, and the four entry points are declared in vfs/vfs.h. The resolver's only consumer of am_chrooted in syscall.c left with it, so the now-dead `am_chrooted` is dropped from the do_*_at wrappers' local externs (and the file-scope extern), which is otherwise unused. No behavior change.
Relocate the operator-supplied-path resolver out of syscall.c into vfs/owner_walk.c with the vfs_* public names: open_no_attacker_symlinks -> vfs_open_owner_walk owner_walk_parent -> vfs_owner_walk_parent The static helpers ona_open and abspath_step move with them, as does the operator_path_resolve flag definition (commit-8 will fold it into the vfs struct). Function bodies are unchanged; the call sites across the daemon and option-parsing files are updated and the two entry points declared in vfs/vfs.h. With the owner-walk gone, syscall.c no longer references am_daemon, so the now-dead `extern int am_daemon` declarations (the do_*_at wrappers delegate to vfs_relpath_active) are dropped. No behavior change.
Relocate the persistent ancestor-dirfd cache out of syscall.c into vfs/dircache.c with the vfs_* public names: open_dir_secure -> vfs_opendir get_dir_fd -> vfs_get_dirfd held_dir_path_fd -> vfs_path_dirfd held_dfd_for -> vfs_cached_dirfd reset_dir_fd_cache -> vfs_dircache_reset The dpc_* cache statics and dpc_dir_fd stay file-local (they fold into the vfs struct in a later commit). Function bodies are unchanged; the callers in generator/receiver/sender/delete/util1/rsync are updated and the five entry points declared in vfs/vfs.h. This completes moving the security core (resolver, owner-walk, dirfd cache) out of syscall.c. No behavior change.
Post-relocation cleanup (no behavior change): - fix a doubled-prefix typo "vfs_vfs_owner_walk_parent" in the vfs/owner_walk.c header comment - vfs/vfs.c comment now points at vfs/dircache.c (not syscall.c) for the live dpc_* cache - drop unused extern decls left from assembling the moved files (am_sender/insecure_links in owner_walk.c, curr_dir/curr_dir_len in dircache.c)
Replace the file-local dpc_* statics in vfs/dircache.c (anchor, base, fd[], name[][], depth) with the vfs.dpc fields already declared in struct vfs and initialized by the designated initializer in vfs/vfs.c. Pure encapsulation: the cache logic is unchanged, DPC_MAXDEPTH becomes the shared VFS_DPC_MAXDEPTH, and vfs_dircache_reset() now resets vfs.dpc (consistent with vfs_init()). No behavior change.
Move the logical-cwd globals into vfs.curr_dir / vfs.curr_dir_len. The definition leaves syscall.c, the per-file `extern char curr_dir[]` / `extern unsigned int curr_dir_len` declarations are dropped (the struct is reached via the vfs.h `extern struct vfs vfs`), and the uses across exclude/flist/log/main/util1 and vfs/secure_open are updated directly -- no compatibility alias macros. change_dir() writes vfs.curr_dir. The separate curr_dir_depth global is untouched. No behavior change.
Move the operator-path resolver-mode flag into vfs.operator_path_resolve. Its definition leaves vfs/owner_walk.c, the extern declarations (in backup/generator/receiver/util1 and vfs_internal.h) are dropped, and the uses across the do_*_at wrappers and the vfs/ internals are updated to the struct field directly. This completes moving the scattered VFS state (dirfd cache, curr_dir, operator_path_resolve) into struct vfs. No behavior change.
Relocate the stat/lstat/fstat wrappers out of syscall.c into vfs/stat.c
with the vfs_* names:
do_stat -> vfs_stat do_stat_at -> vfs_stat_at
do_lstat -> vfs_lstat do_lstat_at -> vfs_lstat_at
do_fstat -> vfs_fstat do_stat_atfd -> vfs_stat_atfd
do_lstat_atfd-> vfs_lstat_atfd
do_xstat_at stays a file-local helper. The x_stat/x_lstat/x_fstat
fallback macros in rsync.h now expand to the vfs_* names. First Phase-3
family move, so the shared RETURN_ERROR_IF* dry-run/read-only guard
macros (and the read_only/list_only externs they expand to) move from
syscall.c into vfs/vfs_internal.h where every vfs/ source can use them.
Function bodies unchanged; no behavior change.
Relocate do_rename / do_rename_at / do_rename_atfd out of syscall.c into vfs/rename.c as vfs_rename / vfs_rename_at / vfs_rename_atfd, declared in vfs/vfs.h. Function bodies unchanged. Also centralize the option-global externs (dry_run, am_root, am_sender, inplace, preserve_*, open_noatime, copy_*, insecure_links, module_id, …) in vfs/vfs_internal.h, replacing syscall.c's local extern block, so each relocated family picks them up from one place. No behavior change.
Relocate do_unlink/do_unlink_at/do_unlink_atfd and do_rmdir/do_rmdir_at out of syscall.c into vfs/unlink.c as the vfs_* names, declared in vfs/vfs.h. Function bodies unchanged; no behavior change.
Relocate do_open/do_open_at/do_open_atfd/do_open_nofollow/ do_open_checklinks out of syscall.c into vfs/open.c as the vfs_* names, declared in vfs/vfs.h. Function bodies unchanged; no behavior change.
Relocate do_chmod/do_chmod_at/do_chmod_atfd (and the leaf-safe do_fchmodat_nofollow helper) out of syscall.c into vfs/chmod.c as the vfs_* names, declared in vfs/vfs.h. The HAVE_CHMOD / HAVE_LCHMOD / HAVE_SETATTRLIST / SYS_fchmodat2 platform guards travel verbatim. Function bodies unchanged; no behavior change. (Portability-sensitive family; wants a fleettest with the rest of the #ifdef-heavy moves.)
Relocate do_symlink/do_symlink_at/do_symlink_atfd and do_readlink/ do_readlink_atfd out of syscall.c into vfs/symlink.c as the vfs_* names. The fake-super (NO_SYMLINK_*XATTRS) placeholder handling travels verbatim. vfs_readlink stays a function only in fake-super builds and a macro -> readlink() otherwise (the rsync.h macro is renamed to match); its vfs.h declaration is guarded accordingly. No behavior change.
Relocate do_link/do_link_at/do_link_atfd out of syscall.c into vfs/link.c as the vfs_* names, declared in vfs/vfs.h. The HAVE_LINK/HAVE_LINKAT guards travel verbatim. No behavior change.
Relocate do_mkdir/do_mkdir_at/do_mkdir_atfd and do_mkstemp/ do_mkstemp_atfd/secure_mkstemp out of syscall.c into vfs/mkdir.c as the vfs_* names (secure_mkstemp -> vfs_secure_mkstemp). The trim_trailing_slashes path helper (used by trimslash and the mkdir wrappers) moves with them, keeping its name and now declared in vfs.h. The static rand_bytes helper, used only by the mkstemp create loop, moves along too. No behavior change.
Relocate do_lchown/do_lchown_at/do_lchown_atfd out of syscall.c into vfs/chown.c as the vfs_* names, declared in vfs/vfs.h. The HAVE_LCHOWN fallback guard travels verbatim. No behavior change.
Relocate do_mknod/do_mknod_at/do_mknod_atfd out of syscall.c into vfs/mknod.c as the vfs_* names, declared in vfs/vfs.h. The HAVE_MKNOD/HAVE_MKNODAT/HAVE_MKFIFO guards and the AF_UNIX socket-bind fallback (with its <sys/un.h> include) travel verbatim. No behavior change. (Portability-sensitive family; wants a fleettest.)
Relocate the timestamp wrappers (do_utimensat/_at/_atfd, do_lutimes, do_utimes, do_utime) and the crtime paths (do_setattrlist_times/_crtime, get_create_time, do_SetFileTime) out of syscall.c into vfs/times.c as the vfs_* names. The struct create_time / #pragma pack / Cygwin windows.h and sys/attr.h includes travel with them, as do the SUPPORT_CRTIMES / HAVE_SETATTRLIST / HAVE_GETATTRLIST / HAVE_UTIMENSAT / HAVE_LUTIMES / HAVE_UTIMES / HAVE_UTIME guards. No behavior change. (Portability- sensitive; wants a fleettest.)
Relocate do_ftruncate/do_lseek/do_fallocate/do_punch_hole out of syscall.c into vfs/fileio.c as the vfs_* names, declared in vfs/vfs.h. The SUPPORT_PREALLOCATION / HAVE_FALLOCATE / HAVE_SYS_FALLOCATE / FALLOC_FL_PUNCH_HOLE guards travel verbatim. This was the last operation family: syscall.c no longer defines any filesystem wrapper. No behavior change. (Portability-sensitive; wants a fleettest.)
The chmod family carries macOS setattrlist() and the Linux SYS_fchmodat2 raw-syscall fast path, but the move into vfs/chmod.c dropped the two platform headers that syscall.c had included at file scope: <sys/attr.h> (macOS) and <sys/syscall.h> (Linux). A Linux build hid this -- with SYS_fchmodat2 undefined the code silently fell back to fchmodat() -- but macOS failed to compile setattrlist(). Re-add both, matching the original syscall.c includes. Found by fleettest (mac2 BUILD-FAIL).
Every filesystem wrapper has moved into vfs/, so syscall.c held nothing but its includes and an orphaned comment. Delete it and drop syscall.o from the rsync link and from each test-harness object list (they reach the vfs_* symbols through libvfs.a now). The S_BLKSIZE fallback define, used by vfs/fileio.c, moves there. No behavior change.
Refresh the header comment now that the wrappers are vfs_* and syscall.c is gone, and document the three operation forms callers choose between: the plain path wrapper, the parent-resolved _at form (race-safe receiver path), and the _atfd form (single component under a pinned dirfd). Comment-only; no code change. Validated at protocol 30 and 29.
Fold the served module root (module_dir / module_dirlen / module_dirfd) into the vfs.module_* snapshot so the confinement checks read the VFS's own state rather than implicit clientserver.c externs. clientserver.c calls vfs_set_module_root() in two stages, matching when the values become final: once right after the module path is settled (before the daemon opens any operator path -- filter/include files, the log file), with the root dirfd still -1, and again once that dirfd is pinned by identity. The dirfd is borrowed (open_anchor_dirfd dup()s it); the VFS never closes it. vfs_init() now clears the whole snapshot so a forked connection can never inherit a stale module root. The vfs/ readers (dirstack, secure_open, owner_walk) switch from the externs to vfs.module_*; clientserver/flist/main keep their own globals. Behavior is identical: between the two calls vfs.module_dirfd is -1, so open_anchor_dirfd re-resolves the path exactly as before the pin.
First step of the API redesign. Define the per-call VFS_* flags (VFS_ALLOW_SYMLINK / VFS_OPERATOR_PATH / VFS_REMOVEDIR) and stop abspath_excluded_by_module() from reading the vfs.operator_path_resolve global directly -- it now takes an explicit is_operator argument. The strict resolver stays confined beneath its anchor (always within the module), so its two call sites pass is_operator=0 (the check never fires there anyway). The ownership walk is reached in BOTH operator context (--backup-dir/--temp-dir/--partial-dir) and non-operator context (daemon log-file/motd/config opens, which may legitimately live outside the module), so vfs_open_owner_walk()/vfs_owner_walk_parent() capture the global and pass it down through ona_open(). No behavior change -- the value still comes from the global; later commits replace that source with the explicit flag and delete the global.
Collapse vfs_mkdir / vfs_mkdir_at / vfs_mkdir_atfd into one call. dirfd ==
VFS_AT_FDCWD resolves the path argument; a real held dirfd makes path a
single component created directly under it (the held-fd form validates
that path is a lone harmless component -- rejects empty, any '/', "." and
".." -- so it cannot reintroduce path resolution under the pinned dir).
The plain-vs-secure choice is now an explicit per-call flag instead of a
function-name choice plus the vfs.operator_path_resolve global:
VFS_ALLOW_SYMLINK the call site asserts it is safe to follow symlinks
(the old plain vfs_mkdir); checked first, wins over
VFS_OPERATOR_PATH (no caller passes both)
VFS_OPERATOR_PATH operator-supplied path: ownership walk + module
confinement (the old global)
default 0 secure receiver resolve (the old vfs_mkdir_at)
VFS_AT_FDCWD is a VFS-owned sentinel (maps to AT_FDCWD where available,
else a value that routes the held-fd form to ENOSYS) so mainline code
never has to mention AT_FDCWD directly -- avoids a compile break on
platforms lacking it.
mkdir stops reading vfs.operator_path_resolve. vfs_owner_walk_parent now
takes the operator context as an is_operator parameter; its non-mkdir
callers (the other _at wrappers, generator hard-link) still pass the
global verbatim and convert in later steps. make_path() gains MKP_OPERATOR
(translated to VFS_OPERATOR_PATH) so the backup-dir creation stays an
operator path by path-semantics, not by relying on the global being set.
Call sites classified: dest dir (main.c) follows symlinks (user's local
dest); transfer dirs (generator, make_path transfer callers) secure;
backup-dir (backup.c, get_backup_name->make_path) and partial-dir
(handle_partial_dir) operator. The operator_path_resolve=1 blocks stay
for now since they still wrap non-mkdir ops; the global value and the
explicit flag agree at every converted mkdir site.
Full suite 190/50; backup/partial/daemon/exclude/filter/symlink-race
subset green.
…ags) Collapse vfs_mknod / vfs_mknod_at / vfs_mknod_atfd into one call, mirroring vfs_mkdir. dirfd == VFS_AT_FDCWD resolves the path (secure receiver resolve by default, ownership walk under VFS_OPERATOR_PATH, plain follow under VFS_ALLOW_SYMLINK); a real held dirfd makes path a single validated component. The old plain/secure/atfd bodies become static helpers (vfs__mknod_plain/_secure/_atfd) behind the public dispatcher; the secure body reads the operator context from VFS_OPERATOR_PATH instead of the vfs.operator_path_resolve global. Call sites: backup file node (make_backup_inner, operator) -> VFS_OPERATOR_PATH; transfer device/fifo (generator) -> secure / held-fd. The t_symlink_secure harness's plain PoC call becomes VFS_ALLOW_SYMLINK (the vulnerable follow it demonstrates) and its secure calls flags=0. Full suite 190/50.
Collapse vfs_symlink / vfs_symlink_at / vfs_symlink_atfd into one call. dirfd == VFS_AT_FDCWD resolves `path` (secure receiver resolve by default, plain follow under VFS_ALLOW_SYMLINK); a real held dirfd makes `path` a single validated component. The old bodies become static helpers behind the public dispatcher. Unlike mkdir/mknod, the symlink secure path has NO ownership-walk branch -- it never read vfs.operator_path_resolve. That pre-existing asymmetry is preserved: VFS_OPERATOR_PATH is accepted but resolves the same as the default secure walk (only the parent dir is confined; the link target is stored verbatim and never resolved at creation). The backup-symlink site passes flags=0, which exactly reproduces the old vfs_symlink_at behavior. Full suite 190/50.
First step of the VFS three-layer architecture: filesystem-mechanic helpers that compose the vfs_* primitives belong inside vfs/, so the operator-path resolution policy travels as an explicit argument instead of leaking across the vfs<->mainline boundary as ambient state. make_path() (recursive mkdir over vfs_mkdir/vfs_stat) moves verbatim from util1.c to vfs/make_path.c and becomes vfs_make_path(fname, mkp_flags, vfs_flags): mkp_flags keeps the path-handling bits (MKP_DROP_NAME/ MKP_SKIP_SLASH), vfs_flags carries the resolution policy (VFS_OPERATOR_PATH for the operator-supplied --backup-dir tree, else 0). The transitional MKP_OPERATOR flag added in the mkdir step is dropped in favour of the explicit vfs_flags argument. Callers: get_backup_name (backup dir) passes VFS_OPERATOR_PATH; the transfer callers (main/receiver/generator) pass 0. Full suite 190/50.
The stat/lstat threading added a vfs_flags argument to x_stat()/x_lstat(),
but on a build without xattr support those are 3-arg macros in rsync.h
(mapping to plain vfs_stat/vfs_lstat), so the 4-arg call sites failed to
compile ("too many arguments to macro"). Give the macros the extra
(ignored) parameter. Caught by the fleettest on openbsd; verified with a
local --disable-xattr build.
Collapse the remaining three-form ops into one call each, matching
mkdir/mknod/symlink/unlink:
vfs_stat(int dirfd, path, st, flags)
vfs_lstat(int dirfd, path, st, flags)
vfs_chmod(int dirfd, path, mode, flags)
vfs_lchown(int dirfd, path, owner, group, flags)
dirfd == VFS_AT_FDCWD resolves the path (VFS_ALLOW_SYMLINK = plain libc op,
default 0 = secure receiver resolve, VFS_OPERATOR_PATH = ownership walk for
stat); a real held dirfd operates on a single component under it. The old
plain/_at/_atfd bodies become static helpers behind the dispatchers;
vfs_fstat stays (fd-based). chmod/lchown keep their no-owner-walk behaviour
(VFS_OPERATOR_PATH resolves the same as the default secure walk).
Pure API-narrowing -- the operator policy was already explicit; no global is
involved. This is behaviour-preserving (plain sites -> VFS_ALLOW_SYMLINK,
_at -> the flag they already carried, _atfd -> held-fd 0).
Held-fd validation differs by op: mkdir/mknod/symlink/unlink reject "."/".."
(creating/removing them is nonsensical), but stat/chmod/lchown ALLOW "."
(a read or metadata op on the dir itself is legitimate -- link_stat_at and
set_file_attrs do it for directory entries); only empty and multi-component
("/") names are rejected there. Caught by the suite (chmod/metadata/
ownership-depth + the implied-"." dir mkdir path).
Full suite 190/50.
Rewrite the vfs.h header contract to describe the current model: the two-layer vfs/ structure (primitives + compounds), the single vfs_<op>(int dirfd, ..., int flags) form, the meaning of VFS_AT_FDCWD / VFS_ALLOW_SYMLINK / VFS_OPERATOR_PATH / VFS_REMOVEDIR, that the operator policy is an explicit per-call flag (not ambient state), and which ops keep explicit forms (rename/link two-path, open variants, fd-based fstat/fileio). Update the stale threat-model cross-references that pointed at the old vfs_chmod_at() (now the static vfs__chmod_secure()) and the two vfs_mknod_at()/vfs_symlink_at() mentions left by the unification.
… stat vfs_stat/vfs_lstat are now vfs_<op>(int dirfd, path, st, int flags); the no-xattr fallback macros still expanded to the old 2-arg vfs_stat(fn,fst), breaking the build without xattr support. Map them to the unified plain form: vfs_stat(VFS_AT_FDCWD, fn, fst, VFS_ALLOW_SYMLINK) (and lstat), which preserves the prior plain-stat behaviour. Caught by the fleettest on openbsd; verified with a local --disable-xattr build.
Three issues from the post-unification review: - The !SUPPORT_XATTRS x_stat/x_lstat macros hardcoded VFS_ALLOW_SYMLINK, silently downgrading an operator-path stat (backup-dir) from the ownership walk + module confinement to plain follow-stat in no-xattr builds (a pre-existing gap the unified vfs_stat now lets us close). Forward the caller's vfs_flags instead, so no-xattr builds get the same policy as xattr builds. - Held-fd ".." was accepted by the stat/chmod/lchown dispatchers, which would operate on the PARENT of the pinned dirfd -- the wrong default for a security boundary even though no current call site passes it. Reject ".." while still allowing "." (a stat/chmod/chown of the dir itself, which link_stat_at and set_file_attrs legitimately do for directory entries). - Two stale comment/test strings still said vfs_chmod_at (now vfs_chmod). Verified with a local --disable-xattr build; full suite 190/50.
Linear-rebase counterpart of the two conflict resolutions made when the sec-fixes base was integrated (see the merge for reference): - Port 8a7e9a16's fixed-array dirstack into the relocated VFS code: vfs/vfs_internal.h (DS_MAXDEPTH + inline int fds[]) and vfs/dirstack.c (drop malloc/realloc/free; ENOMEM past the cap). 8a7e9a16 changed the dirstack in syscall.c, which this branch deleted, so the fix is re-expressed in vfs/. - Map the base's new F_XATTR ndx<0 guard (1d36a565) from the old do_chmod_at() to the unified vfs_chmod(VFS_AT_FDCWD, fname, ..., 0). Tree is byte-identical to the validated merge result.
Linear-rebase counterpart of the round-2 merge resolution: 37dbb263 fixed do_mknod_at()'s operator branch in syscall.c (deleted on this branch) to fall back to mkfifoat() for a FIFO and EOPNOTSUPP for a nested socket when mknodat() can't make a special file on the BSDs/macOS/Solaris. Re-express it in the VFS_OPERATOR_PATH branch of vfs__mknod_secure() in vfs/mknod.c, mirroring the secure-relpath branch already in that function. Tree is byte-identical to the validated round-2 merge result.
The -X theirs rebase replayed the 47 VFS commits onto the new base, letting each VFS commit win its refactor on conflict; that drops the round-3 base changes that overlap the operator-path code (they were authored against the deleted syscall.c and the pre-vfs_flags model). This commit snaps those files byte-for-byte from the validated merge oracle (merge-reference-3 = 6afdd389), so the branch tip is tree-identical to the merge that built clean +/-xattr and passed the suite + root operator-path PoC tests. Reconciled: the syscall.c->vfs/ ports (secure_open/owner_walk/dirstack/rename/ link/chmod/chown/times/copy_file + vfs.h/vfs_internal.h), the set_file_attrs op_pin via ATTRS_OPERATOR_PATH (rsync.c/rsync.h + backup.c callers), the secure_basis_open/basis_link_stat operator branches (receiver.c/generator.c), the copy_file/gen_entry_copy_xattrs held-fd source reads, the change_dir/sender opt-out sites (util1.c/sender.c/clientserver.c), and Makefile.in.
A backup-tree delete (delete_item with DEL_FOR_BACKUP -- removing an existing leaf under an absolute --backup-dir before the new backup is placed) fell through del_unlink() to robust_unlink(fbuf, 0), which resolves the leaf's parent by path with no ownership walk. A local attacker who flips a backup-path parent to a symlink in that window could redirect the unlink outside the backup tree. The base confines this: make_backup() wraps make_backup_inner() in operator_path_resolve, so the same unlink reaches do_unlink_at()'s owner-walk branch. The VFS refactor replaced the global with explicit vfs_flags but left del_unlink() passing 0, dropping the confinement. Thread VFS_OPERATOR_PATH into del_unlink()'s path-based fallback when DEL_FOR_BACKUP is set (the exact cases that ran under the base's operator wrap); a held-dirfd delete is already confined and ignores it. The rmdir path keeps no owner-walk branch, matching base.
vfs_rename_at() and vfs_link_at() took a single vfs_flags applied to BOTH operands, so VFS_OPERATOR_PATH (ownership walk: follow uid0/euid symlinks, refuse foreign) was applied to a transfer operand even when only one side was operator- supplied -- relaxing the secure receiver resolve that a transfer/file-list path should get. The default branch confined each side by absolute-vs-relative, but that missed a *relative* operator path (--backup-dir=bdir, --partial-dir=.rsync, relative --link-dest). Reported in the rsync-private PR #30 review. Take old_flags and new_flags separately and resolve each operand under its own policy via a shared vfs_twopath_side() helper (operator walk if the side is VFS_OPERATOR_PATH or absolute; secure receiver resolve if relative-with-slash; AT_FDCWD for a bare name). An operator basis/backup path on one side can no longer relax the other side's confinement. Callers now pass the correct per-side policy: - backup link/rename: source (live dest file) = transfer (0), backup target = operator. - receiver partial-dir rename: partialptr = operator, final dest = transfer. - hard_link_one / generator link-dest: basis source = operator (non-daemon), dest = transfer. - finish_transfer / gen_entry_rename / robust fallback: both transfer (0,0; default per-side). - robust_unlink ETXTBSY sibling rename: both share the caller's policy. Also drops the operator flag on backup's "just in case" robust_unlink of the transfer-side source. Behaviour-preserving for the operator side (the backup/partial-dir parents still owner-walk); it only tightens the transfer side back to the secure resolve. Builds +/-xattr; suite 210/0/66; root operator-path PoC tests all pass. Suggested-by: Zen Dodd <mail@steadytao.com>
Extend the t_rename_secure harness with PS-refuse/PS-follow: rename to the SAME operator-owned escaping symlink path (oplink -> ../trap) with the SAME operator old-side flag, differing ONLY in the new-side flag. The ownership walk (operator) follows the operator's own uid0/euid symlink; the secure receiver resolve (transfer, flag 0) refuses it because it leaves the cwd anchor. PS-refuse must be rejected and PS-follow must succeed -- proving the new side's confinement is independent of the old side's policy. Deterministic, non-root, no race. Verified RED on the old whole-call behaviour (emulated by OR-ing both operands' flags, the new side then follows oplink and escapes to ../trap) and GREEN with the per-operand split. The driver adds the oplink fixture and the per-side source files. Suggested-by: Zen Dodd <mail@steadytao.com>
…-xattrs race) set_file_attrs() pins the entry via the cached held dir fd and drives the xattr /ACL ops off that fd (fsetxattr). But when the pin missed -- vfs_cached_dirfd() returns -1 (its dir isn't the held one, or the path is deeper than the dirfd cache), or the leaf openat() loses a race -- held_fd stayed -1 and set_stat_xattr()/set_xattr()/get_acl_fdat()/set_acl_fdat() fell through to the path-based branch (sys_lsetxattr(fname,...)). Unlike the chmod/chown/times path wrappers (which secure-resolve), that raw lsetxattr re-resolves the parent, so a concurrent flip of a dest parent component to a symlink->outside lands the xattr OUTSIDE the destination tree (the intermittent copy-xattrs-symlink-race escape that surfaces under -j load, which widens the open->setxattr window). Re-pin through the secure resolver when the cached pin misses on a confined, non-operator receiver path, so the xattr/ACL ops always use a confined fd -- NOT a raw path lsetxattr; if the re-pin also fails (a genuinely raced parent/leaf symlink) skip the path-based ops (xattr_refuse) rather than redirecting them. The re-pin passes O_DIRECTORY for a directory leaf (the secure resolver refuses a bare dir open with EISDIR), so dir xattrs/ACLs are still preserved on a cache miss. Apply the same re-pin/refuse to gen_entry_copy_xattrs() (the dir xattr copy), whose dfd<0 path likewise fell to copy_xattrs() with dest_fd==-1. chmod/chown/times are unchanged (already confined via their *at wrappers); operator paths keep op_pin/op_refuse. Confirmed: under heavy parallel load the dangerous lsetxattr fallback was reached 52+ times before and 0 times after; dir xattrs preserved; suite 210/0/66, root metadata PoC tests pass, builds +/-xattr.
cleanups from review comments by Sam James. Thanks!
Add a CI/dev hardening mode (--enable-strict-confinement) that turns any confined-regime raw path-based metadata op into a hard abort, so a reintroduced copy-xattrs-class fallback fails the test suite instead of silently escaping through a flipped parent symlink. vfs_must_be_confined() is the predicate: the modern *at/O_NOFOLLOW primitives are present, the path is non-operator, relative and multi-component (a parent the attacker could flip), and vfs_relpath_active(). vfs_strict_confine_fail() logs and aborts. Both are absent/no-op without STRICT_CONFINEMENT, so there is no production behaviour change.
…INEMENT Guard the two receiver decision points that drive the xattr/ACL setters down a raw path-based branch -- set_file_attrs() and gen_entry_copy_xattrs(). In a strict build, a confined pinnable non-operator leaf that reached the setters without a confined fd (held_fd/xfd < 0 and not refused) aborts: that is exactly the copy-xattrs fallback class. The guards mirror the pin/re-pin conditions, so they cannot false-abort a legitimate transfer. Verified: under --enable-strict-confinement the suite is 210/0 with zero aborts; neutering the re-pin makes copy-xattrs-symlink-race abort at the guard.
Enable the strict confinement assertion in the ASan/UBSan CI build so the suite enforces "no confined-regime raw path metadata op" on every run.
The HAVE_SOLARIS_ACLS facl(2) paths called secure_relpath_active() -- the base-branch name -- which does not exist on the VFS branch (here the gate is vfs_relpath_active()). Linux/BSD never compile that branch, so it stayed latent; a real Solaris build fails with an implicit-declaration error. Rename both call sites (set_rsync_acl default-ACL delete and the access/default set fallback).
Linear-rebase counterpart of the conflict resolutions made when the sec-fixes base was integrated (see the merge for reference): - Port ddda7ba5's operator-path confinement of do_symlink_at and do_rmdir_at into the relocated VFS code. vfs__symlink_secure gains the VFS_OPERATOR_PATH ownership-walk branch (parent confined via vfs_owner_walk_parent, shared leaf-creation preserved so fake-super emulation still applies); vfs__unlink_secure extends its existing operator branch to the rmdir/AT_REMOVEDIR case. Callers pass the policy explicitly where the base set operator_path_resolve: the keep_backup symlink create (backup.c), the backup-tree rmdir in delete_item (delete.c, DEL_FOR_BACKUP), and handle_partial_dir's rmdir (util1.c). - Port 1f8f89c2's robust_rename EXDEV-fallback confinement into vfs/robust.c: an absolute --temp-dir/--partial-dir operand routes the copy_file dest-write and the source-unlink through the ownership walk (VFS_OPERATOR_PATH), so a raced parent symlink can't redirect either out of the module. - Drop the stale "no ownership-walk branch" notes in vfs/vfs.h and vfs/symlink.c now that symlink and rmdir carry the branch. Tree is byte-identical to the validated merge result.
The #else arm for platforms without AT_FDCWD called the four-argument vfs_symlink() with two arguments -- a compile error on such systems. Call vfs__symlink_plain(), matching the base's do_symlink() fallback. Found by codex review; predates this rebase round.
daemon-exclude-namebased bound its daemon on 13010, and the setup_chroot_inner helper hashed into 12940-13139 -- both reach into 13000+, where ASUS Armoury Crate on the Cygwin CI host parks localhost listeners (13010, 13030-13032), making the port probe fail the test. The helper also used str hash(), which is per-process randomized (PYTHONHASHSEED), so its port wandered run to run. Move the fixed port to 12931 and the helper to a deterministic crc32-based slot in the otherwise-unused 12800-12859 band.
t_hashtable_overflow, t_iwildmatch, t_clean_fname, and t_safe_arg were built by the suite but missing from .gitignore, so a stray git add -A sweeps them into a commit (as happened during this rebase round). List them alongside the other t_* harnesses.
Linear-rebase counterpart of the conflict resolutions made when the sec-fixes base was re-integrated (see the merge for reference). The base gained 5cb4b82 ("syscall: build without AT_SYMLINK_NOFOLLOW") and its CI compile-check, which touch code this branch relocated into vfs/: - Move syscall.c's RSYNC_TEST_NO_AT_FDCWD undef block into vfs/vfs.h, before the VFS_AT_FDCWD sentinel binds (so the sentinel takes its no-AT_FDCWD value rather than dangling on the undefined AT_FDCWD). - Port the AT_SYMLINK_NOFOLLOW-absent fallbacks into the relocated code: vfs/chown.c (vfs__lchown_secure + held-fd vfs_lchown gate on AT_SYMLINK_NOFOLLOW), vfs/stat.c (do_xstat_at's unused-arg casts, the vfs_lstat AT_SYMLINK_NOFOLLOW-absent arm, held-fd gate), vfs/mkdir.c (guard rand_bytes on AT_FDCWD, its only caller). - Retarget the CHECK_COMPILE_OBJS compile-check from syscall.c to a portable shell loop over the vfs sources built with -DRSYNC_TEST_NO_AT_FDCWD (Makefile.in). Tree is byte-identical to the validated merge result.
…the compile-check atomic Two fixes from codex review of the no-AT_FDCWD port: - vfs/stat.c: the held-dirfd vfs_lstat branch fell back to fstatat(dirfd, path, st, 0) when AT_SYMLINK_NOFOLLOW is unavailable, which FOLLOWS the leaf and breaks lstat's no-follow contract. On a system with SUPPORT_LINKS but no AT_SYMLINK_NOFOLLOW, return ENOSYS instead (mirroring the held-fd vfs_lchown arm) so a symlink-sensitive caller fails loud rather than silently following; the !SUPPORT_LINKS arm keeps fstatat(...,0) since there is nothing to follow. The CI compile-check config also undefines AT_FDCWD so no held fd is produced there; this hardens the standalone "no AT_SYMLINK_NOFOLLOW" shape. - Makefile.in: the vfs-no-at-fdcwd.o compile loop wrote every object to $@, so a mid-loop failure left a fresh-timestamped $@ and a retry could skip the check. Compile to $@.tmp and mv to $@ only after the whole loop succeeds (rm the stale target up front); clean the .tmp too.
Linear-rebase counterpart of the conflict resolutions made when the sec-fixes base was re-integrated (see the merge for reference). Three of the base's new commits touch code this branch relocated or reworked: - options.c (3fe1ed5 "rsync: confine the daemon files-from open to the module root"): the base wraps the files-from open in the operator_path_resolve global, which no longer exists here. Pass the operator context explicitly instead: vfs_open_owner_walk(..., 1). - receiver.c (cfd40f5 "receiver: confine peer-selected partial basis paths"): the new relative-partial-basis branch uses the VFS resolver name, vfs_resolve_open(). - vfs/chmod.c (7b16872 "syscall: silence scan-build dead-store in do_fchmodat_nofollow fallback"): syscall.c is deleted here, so the mode masking move and the unused-arg casts land in the relocated do_fchmodat_nofollow. Tree is byte-identical to the validated merge result.
The base gained 99 commits since the last rebase. Nine files needed a hand-port because the change lands on code the VFS split moved or renamed, and syscall.c no longer exists here: - backup.c: make_path() now runs on a private copy of backup_dir_buf (c933f62), so vfs_make_path() takes dirbuf and drops the restore. - clientserver.c: keep both the module-root snapshot and the new daemon_config_filter_file window. - exclude.c: the peer-driven merge-file confinement is the vfs_open_owner_walk() is_operator argument, not a global. - fileio.c: the coalesced --sparse writer's new helpers use vfs_lseek()/vfs_punch_hole(). - generator.c/vfs/mknod.c: gen_entry_mknod() falls back through vfs_mknod(); the atfd path keys its mknodat() off HAVE_MKNODAT. - receiver.c: secure_recv_open() passes VFS_OPERATOR_PATH instead of toggling operator_path_resolve; open_readonly_inplace() uses the VFS stat/chmod/open wrappers. - sender.c: absolute --relative cleanup anchors at "/" via vfs_resolve_open(), the copy-links walk uses vfs_resolve_open_at_beneath(), and the source removal goes through vfs_unlink(). - vfs/chmod.c, vfs/chown.c: VFS_OPERATOR_PATH now takes the ownership walk, and the no-follow chmod grows the non-Linux fd path. - vfs/secure_open.c: the fd-anchored resolver splits into a shared internal with an allow-dotdot entry point, and secure_walk_at() routes a literal "."/".." through ds_descend() before the leaf fast paths. Tree is byte-identical to the merge oracle (tag merge-reference-10).
Base commit 0bfcd3b taught do_chmod_at()/do_lchown_at() to resolve an operator-supplied path through the ownership walk. The previous commit ported that into vfs_chmod()/vfs_lchown(), but on this branch the policy is a per-call argument rather than the ambient operator_path_resolve global, and set_file_attrs() still passed 0 -- so the ported branch was unreachable and the fix was inert. op_pin already covers a reg/dir/fifo leaf with a pinned fd, which is stronger than the walk. What it does not cover reaches the path-based fallbacks: a symlink or device leaf never enters op_pin, and a non-root operator can fail the pin open with an ordinary EACCES and fall through with op_refuse clear. Both then resolved the full operator path with a bare lchown()/chmod(). Derive the flag from ATTRS_OPERATOR_PATH and pass it to the two VFS_AT_FDCWD fallbacks only; the held-dirfd arms stay at 0, since a pinned parent already confines them. vfs_chmod()'s operator branch skips S_ISLNK on its own, so a symlink-as-object keeps the lchmod()/setattrlist() path, matching the base. Also drop ten .gitignore entries the rebase duplicated: this branch's own test-helper block and the base's new one list the same binaries. Suite 259/0/83; the operator-path, backup and temp-dir families pass as root over --use-tcp. Instrumenting the branch shows it is now reached 92 times across those tests (the chmod side); the lchown side has no test exercising it, which is how the gap survived upstream.
The previous commit gave set_file_attrs()' path-based chmod/chown the
operator ownership walk, but nothing exercised the chown side: the
instrumented branch was reached 92 times by the existing suite, all of
them chmod. That is the same gap that let the upstream fix ship inert.
The uncovered path is a SYMLINK backup. op_pin cannot pin a symlink --
there is no O_NOFOLLOW open of one -- so make_backup()'s set_file_attrs()
falls through to the full-path lchown. Without the walk, a backup parent
flipped to an attacker-owned symlink redirects that lchown onto a victim
outside the backup tree and retags it as the attacker's.
Two things make this awkward to test, and both are why it was missed:
- On one filesystem make_backup() hard-links or renames the item into
the backup dir and never calls set_file_attrs() at all. The fixture
therefore puts the backup dir on tmpfs so link and rename fail EXDEV
and the recreate path runs. backup-crossdev-copy probes the same way.
- A statically planted symlink proves nothing: rsync's own backup-dir
validation deletes a non-directory component before using it. The
plant has to be a live flip, as in operator-path-backup-symlink.
The positive control checks both that the symlink reached the backup tree
and that the backup copy carries the attacker's uid -- i.e. that an lchown
actually ran. Without that second assertion the race would pass
vacuously on any build where the chown never happens.
RED on the parent of the previous commit (victim outside/f94 retagged
away from root); GREEN here. Runs on Linux CI as root; registered as an
expected skip on Cygwin (root-only) and macOS (root-only, and no
cross-device tmpfs -- backup-crossdev-copy is listed there for the same
reason). Full expected-skip oracle passes exactly.
The base gained 13 commits. Three needed hand-porting because the change lands on code the VFS split moved or renamed: - 3113011 (--confine-root) is almost entirely in syscall.c, which does not exist here. abspath_excluded_by_module() becomes abspath_outside_confinement() in vfs/dirstack.c, taking its root from vfs.module_dir when we are a daemon and from confine_root otherwise, plus the fd-pin helpers; ona_open() in vfs/owner_walk.c gains the getcwd() seed and the pin_transit exemption. The VFS passes is_operator as an argument where the base reads operator_path_resolve, so the refusal takes it from there rather than the deleted global. - d09edb8 (--link-dest hard-link fallback) keeps the VFS call vfs_link_at(cmpbuf, fname, !am_daemon ? VFS_OPERATOR_PATH : 0, 0) and adopts the base's cannot_hardlink/match_level fallback around it. - t_stub.c gains the confine_root/confine_rootlen stubs beside the VFS curr_dir note. Tree is byte-identical to the merge oracle (tag merge-reference-11).
operator-path-backup-chown probes for a cross-device directory, because make_backup() renames into a same-filesystem backup dir and never reaches the set_file_attrs() path the test is about. On macOS there is normally no such directory, so the test is a macOS-wide expected skip -- but this target puts the scratch trees on a separate HFS+ volume, which supplies exactly the condition it was looking for. Same reason backup-crossdev-copy and chmod-temp-dir are already omitted here. The test PASSES on the target (10.9s); only the expected-skip bookkeeping was wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a rewrite of the filesystem backend for rsync that came out of the work on the 3.5.0 security release.
The aim of this work is to separate out the filesystem portability work from the core rsync protocol in order to make maintaining (and understanding!) rsync easier
Many thanks to @seks99x @steadytao and others for their feedback on this work