fix(metadata): apply the set-group-ID mode chmod(2) would apply

crates/daemon/src/daemon/sections/module_access/client_args/path_resolution.rs

@@ -208,15 +208,30 @@ fn resolve_receiver_dest(
 /// Sub-paths containing `..`, and host-absolute sub-paths, are COLLAPSED
 /// against the module root rather than refused - see
 /// [`collapse_module_relative`]. A crafted `rsync://host/mod/../etc/...` URL
-/// therefore resolves to `<module>/etc/...` and cannot enumerate outside the
-/// module root, which is the same containment upstream gets from
-/// `sanitize_path` at depth 0.
+/// therefore resolves to `<module>/etc/...`, which is the same containment
+/// upstream gets from `sanitize_path` at depth 0.
+///
+/// ⚠ That collapse is LEXICAL, so it contains exactly the shapes it folds -
+/// `.`, `..` and a host-absolute spelling - and nothing else. It is NOT a
+/// statement about where the returned path RESOLVES: a symlink is resolved by
+/// the kernel at syscall time and no string operation can see it. Upstream
+/// does not rely on the collapse alone either; it also enters the module with
+/// `change_dir(module_chdir, CD_NORMAL)` (clientserver.c:992) and scans
+/// through the confined `secure_opendir()` (flist.c:2028-2059).
+///
+/// Resolved containment for these operands is therefore owned by the sender,
+/// not by this function: the content open goes through the confined source
+/// open, and the file-list scan is anchored on the transfer's confinement root
+/// rather than read by absolute path. Do not read this collapse as a reason to
+/// leave a downstream operation unconfined - it was read that way once, and an
+/// out-of-module directory reached by a module-root symlink was enumerated
+/// into the file list.
 ///
 /// # Upstream Reference
 ///
-/// - `util1.c:804 glob_expand_module()` - strips the module name from each arg
-/// - `clientserver.c:992 change_dir(module_chdir, CD_NORMAL)` - relativises args
-/// - `flist.c:2338-2349 send_file_list()` - `dir/fn` split per positional
+/// - `util1.c:881 glob_expand_module()` - strips the module name from each arg
+/// - `clientserver.c:1059 change_dir(module_chdir, CD_NORMAL)` - relativises args
+/// - `flist.c:2610-2621 send_file_list()` - `dir/fn` split per positional
 fn resolve_sender_sources(
     module_path: &std::path::Path,
     client_args: &[String],
@@ -236,8 +251,10 @@ fn resolve_sender_sources(
         }
         all_empty = false;
         // Collapse `.` and `..` exactly as upstream's `sanitize_path` does at
-        // depth 0 - see [`collapse_module_relative`]. The result cannot escape
-        // the module root, so there is nothing left to refuse.
+        // depth 0 - see [`collapse_module_relative`]. The result is LEXICALLY
+        // beneath the module root, so there is no traversing spelling left to
+        // refuse; where it RESOLVES is decided by the sender's confined open
+        // and its anchored directory scan, not here.
         let collapsed = collapse_module_relative(tail.trim_start_matches('/'));
         let trimmed = collapsed.as_str();
         if trimmed.is_empty() {

crates/daemon/src/daemon/sections/module_access/client_args/server_config.rs

@@ -154,19 +154,25 @@ fn build_server_config(
         .cloned()
         .unwrap_or_default();
 
-    // upstream: main.c:1203-1204 + util1.c:804 (glob_expand_module) - receivers
+    // upstream: io.c:1497 + util1.c:881 (glob_expand_module) - receivers
     // resolve their destination by joining the module path with the client's
     // module-relative tail (e.g. `upload/realdir/` -> module + `realdir/`).
     // Senders (pull requests) split each positional the same way so the
-    // sender's per-source `dir/fn` (flist.c:2338-2349) walks the requested
+    // sender's per-source `dir/fn` (flist.c:2610-2621) walks the requested
     // sub-tree instead of the entire module root. The original argv[0] is
     // always the module root; legacy tests that push straight into the module
     // root keep that behaviour.
     // Both resolvers are total: they collapse `..` the way upstream's
-    // `sanitize_path` does at depth 0, so the result is confined to the module
-    // root by construction and there is no "resolves outside module root"
-    // rejection to represent. Upstream has no such daemon error either - a
-    // traversing tail is rewritten and served (util1.c:1183).
+    // `sanitize_path` does at depth 0, so no traversing SPELLING survives and
+    // there is no "resolves outside module root" rejection to represent.
+    // Upstream has no such daemon error either - a traversing tail is
+    // rewritten and served (util1.c:1183).
+    // ⚠ Total is not confined. The collapse is a string operation and says
+    // nothing about where the path resolves; a symlink beneath the module root
+    // still points wherever it points. Resolved containment is enforced where
+    // the syscalls are issued - the confined source open and the file-list
+    // scan anchored on the transfer's confinement root - not by these
+    // resolvers.
     let positional_args: Vec<OsString> = if role == ServerRole::Receiver {
         let dest = resolve_receiver_dest(
             std::path::Path::new(&module.path),

crates/fast_io/src/dir_sandbox/at_syscalls/metadata_ops.rs

@@ -278,7 +278,60 @@ pub fn secure_chmod_at(path: &Path, mode: u32, follow_symlinks: bool) -> io::Res
         .file_name()
         .ok_or_else(|| io::Error::from_raw_os_error(libc::EINVAL))?;
     let dirfd = crate::secure_open_dir(parent)?;
-    fchmodat(dirfd.as_fd(), leaf, mode, follow_symlinks)
+    chmodat_with_chmod_setgid_semantics(dirfd.as_fd(), leaf, mode, follow_symlinks)
+}
+
+/// `fchmodat` with the set-group-ID semantics `chmod(2)` has on this platform.
+///
+/// Upstream reaches every mode change through `do_chmod()` -> `chmod(2)`
+/// (syscall.c:761). oc anchors the same change on a walked parent dirfd for
+/// symlink-race safety, which means it issues `fchmodat(2)` instead - and the
+/// two syscalls do NOT agree about `S_ISGID` for an unprivileged caller.
+///
+/// MEASURED on macOS 26.5 (euid 501, caller owns the target), same inode and
+/// same mode word for both calls:
+///
+/// | requested            | `chmod(2)`         | `fchmodat(2)` |
+/// |----------------------|--------------------|---------------|
+/// | `0755`               | -                  | ok            |
+/// | `0755 \| S_ISUID`    | ok -> `04755`      | ok -> `04755` |
+/// | `0755 \| S_ISGID`    | ok -> `00755`      | **EPERM**     |
+/// | `0755 \| both`       | ok -> `04755`      | **EPERM**     |
+///
+/// So the kernel silently MASKS a set-group-ID bit it will not grant when the
+/// request arrives through `chmod`, but REFUSES the identical request through
+/// `fchmodat`. Linux masks on both, which is why this is not observable there.
+///
+/// Left unhandled, that turns `--chmod=a+s` into a fatal `EPERM` for an
+/// ordinary user on macOS where upstream completes with exit 0 - measured
+/// against the real rsync 3.5.0 binary, which leaves `drwsr-xr-x`.
+///
+/// This helper restores upstream's semantics WITHOUT giving up the anchor:
+/// on refusal it retries the same anchored `fchmodat` once with `S_ISGID`
+/// cleared, which is exactly the mode the platform's own `chmod` would have
+/// applied. The retry cannot mask a genuine permission error - a caller that
+/// does not own the target is refused for the setgid-free mode too, and that
+/// error propagates.
+///
+/// upstream: rsync-3.5.0 syscall.c:761 `do_chmod()` -> `chmod(2)`; rsync.c:658-668
+/// `set_file_attrs()` treats the chmod result as fatal for a non-symlink, which
+/// is why the refusal must not reach it.
+fn chmodat_with_chmod_setgid_semantics(
+    dirfd: BorrowedFd<'_>,
+    leaf: &OsStr,
+    mode: u32,
+    follow_symlinks: bool,
+) -> io::Result<()> {
+    // The set-group-ID bit, spelled as the POSIX-fixed literal: `libc::S_ISGID`
+    // is `u16` on macOS but already `u32` on Linux, so neither `u32::from` nor
+    // an `as` cast is lint-clean on both platforms.
+    const SETGID: u32 = 0o2000;
+    match fchmodat(dirfd, leaf, mode, follow_symlinks) {
+        Err(error) if error.raw_os_error() == Some(libc::EPERM) && mode & SETGID != 0 => {
+            fchmodat(dirfd, leaf, mode & !SETGID, follow_symlinks)
+        }
+        other => other,
+    }
 }
 
 /// Path-based `chown(2)` / `lchown(2)` on `link_path` through the libc
@@ -534,3 +587,17 @@ pub fn utimensat_via_sandbox_or_fallback(
         filetime::set_symlink_file_times(link_path, atime, mtime)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    /// `chmodat_with_chmod_setgid_semantics` spells the set-group-ID bit as a
+    /// literal because `libc::S_ISGID` has a different width per platform. Pin
+    /// the literal against libc so a wrong value cannot go unnoticed.
+    ///
+    /// Both sides widen to `u64`, which is a real conversion on every supported
+    /// platform - comparing in `u32` would be a no-op cast on Linux.
+    #[test]
+    fn setgid_literal_matches_libc() {
+        assert_eq!(u64::from(libc::S_ISGID), 0o2000);
+    }
+}

crates/fast_io/src/pinned_root.rs

@@ -107,6 +107,68 @@ pub fn read_dir(path: &Path) -> io::Result<ReadDir> {
     Ok(ReadDir(Source::Std(std::fs::read_dir(path)?)))
 }
 
+/// Like [`read_dir`], but anchored on a root the CALLER supplies instead of the
+/// ambient session pin.
+///
+/// [`read_dir`] reads its anchor from the process-global session root, which is
+/// only ever correct when the process serves one confinement domain at a time -
+/// upstream's situation, because it forks a child per connection. oc serves
+/// each daemon connection on a worker thread of one process, so a per-connection
+/// root cannot live in a global without two concurrent connections overwriting
+/// each other's boundary. Passing the root in makes the anchor a parameter of
+/// the call rather than ambient state, which is what keeps it correct under
+/// threads.
+///
+/// The walk beneath `root` is the confined one: an absolute symlink target, a
+/// `..` above the anchor, or an excluded component is REFUSED rather than
+/// followed, so a refused scan can never be mistaken for an empty directory.
+///
+/// ⚠ `strip_prefix` is LEXICAL, so "does not lie beneath `root`" here means
+/// NOT ANCHORABLE, never "escapes the root". A relative operand resolves
+/// against the process cwd - which is typically inside the root, since that is
+/// how a restricted shell invokes rsync - and an absolute path can be spelled
+/// differently from the root it is under. Both fall back to the ordinary read,
+/// exactly as [`crate::confinement::pinned_root_relative`] already decides for
+/// the ambient pin: "Returns `None` - meaning resolve `path` the ordinary way -
+/// when no root is pinned, or when `path` does not lie beneath the pinned
+/// root."
+///
+/// Treating a failed `strip_prefix` as an escape and refusing would be the same
+/// error this anchoring exists to fix: a lexical test standing in for a
+/// resolution decision. It refuses directories that were never outside
+/// anything - measured, as `opendir "sub" failed: Cross-device link`.
+///
+/// # Errors
+///
+/// - The walk's refusal (`ELOOP` for an absolute or escaping symlink target,
+///   `..` above the anchor), or the underlying `opendir` error.
+///
+/// # Upstream Reference
+///
+/// - `rsync-3.5.0/flist.c:2028-2059` `secure_opendir()` - the confined open that
+///   produces `scan_dirfd` for a daemon's directory scan.
+/// - `rsync-3.5.0/syscall.c:136` `confinement_root()` - `am_daemon ? module_dir
+///   : confine_root`, the value this parameter carries.
+#[cfg(unix)]
+pub fn read_dir_under(root: &Path, path: &Path) -> io::Result<ReadDir> {
+    let Ok(relative) = path.strip_prefix(root) else {
+        return read_dir(path);
+    };
+    // `strip_prefix` yields an empty path when `path` IS the root; the walk
+    // spells that directory `.`, matching what upstream's post-`change_dir`
+    // code uses for the same directory (`flist.c:2059`).
+    let relative = if relative.as_os_str().is_empty() {
+        Path::new(".")
+    } else {
+        relative
+    };
+    let names = crate::confined_readdir::read_dir_confined(root, relative)?;
+    Ok(ReadDir(Source::Names {
+        dir: path.to_path_buf(),
+        names: names.into_iter(),
+    }))
+}
+
 /// Open `path` as an `O_PATH` descriptor, anchored on the pinned root when it
 /// applies.
 ///

crates/transfer/src/generator/file_list/walk.rs

@@ -122,6 +122,39 @@ impl<'a> WalkScope<'a> {
 }
 
 impl GeneratorContext {
+    /// Enumerates a source directory, confined beneath this transfer's root
+    /// when it has one.
+    ///
+    /// Sole owner of the enumeration-anchor decision for the walk, so the two
+    /// scan sites cannot disagree about whether a directory read is confined -
+    /// the same reason [`Self::confine_root`] is the sole owner of the boundary
+    /// it reads.
+    ///
+    /// The root is passed to `fast_io` rather than left ambient. `read_dir`'s
+    /// own anchor is the process-global session pin, which oc never installs
+    /// for a daemon, so this walk silently degraded to an ordinary
+    /// absolute-path read, while [`Self::confine_root`] (already
+    /// daemon-correct) was consulted only by the CONTENT open. That split is
+    /// exactly what let a module symlink be enumerated but not read.
+    /// Installing the global per
+    /// connection is not the alternative: oc serves each connection on a worker
+    /// thread of one process, so a per-connection value in a global would race
+    /// between concurrent connections on different modules.
+    ///
+    /// # Upstream Reference
+    ///
+    /// - `rsync-3.5.0/flist.c:2028-2059` `secure_opendir()` - the confined open
+    ///   producing `scan_dirfd` for a daemon's scan.
+    /// - `rsync-3.5.0/flist.c:1878` - the `opendir` failure diagnostic both call
+    ///   sites report, unchanged by the anchoring.
+    fn scan_source_dir(&self, path: &Path) -> io::Result<fast_io::pinned_root::ReadDir> {
+        #[cfg(unix)]
+        if let Some(root) = self.confine_root() {
+            return fast_io::pinned_root::read_dir_under(&root, path);
+        }
+        fast_io::pinned_root::read_dir(path)
+    }
+
     /// Pre-checks a top-level source entry and walks it if it exists.
     ///
     /// Returns `true` if the entry was processed (exists or was handled as a
@@ -512,7 +545,7 @@ impl GeneratorContext {
         let should_recurse =
             metadata.is_dir() && self.config.flags.recursive && scope.descends_into_subdirs();
         let dir_read = if should_recurse {
-            match fast_io::pinned_root::read_dir(&path) {
+            match self.scan_source_dir(&path) {
                 Ok(entries) => Some(entries),
                 Err(e) => {
                     // upstream: flist.c:1878 - rsyserr(FERROR_XFER, errno, "opendir %s failed", ...)
@@ -685,7 +718,7 @@ impl GeneratorContext {
     ) -> io::Result<()> {
         let entries = match opened {
             Some(entries) => entries,
-            None => match fast_io::pinned_root::read_dir(dir_path) {
+            None => match self.scan_source_dir(dir_path) {
                 Ok(entries) => entries,
                 Err(e) => {
                     // upstream: flist.c:1878 - rsyserr(FERROR_XFER, errno, "opendir %s failed", ...)

tools/ci/upstream-3.5.0-expect.macos.nonroot.txt

@@ -129,7 +129,7 @@ daemon-refuse-compress pass
 daemon-refuse-compress-threads-alias skip
 daemon-refuse-delete-alias pass
 daemon-scan-cwd-desync pass
-daemon-scan-dir-escape fail
+daemon-scan-dir-escape pass
 daemon-secrets-file-symlink skip
 daemon-size-arg-overflow pass
 daemon-standalone-detach skip
@@ -200,7 +200,7 @@ links pass
 log-control-chars pass
 log-file-symlink skip
 longdir pass
-macos-setgid-ordinary-mode-regression fail
+macos-setgid-ordinary-mode-regression pass
 malicious-dot-dir-delete-scope skip
 malicious-dot-file-delete-scope skip
 malicious-sender-delete-scope pass
@@ -320,7 +320,7 @@ scanner-batch-flag-mismatch pass
 scanner-daemon-log-checksum pass
 scanner-delete-delay-overread pass
 secure-relpath-validation pass
-sender-flist-symlink-leak fail
+sender-flist-symlink-leak pass
 sender-readlink-atfd pass
 sender-remove-source-relative-anchor pass
 sender-remove-source-root-anchor skip