diff --git a/crates/daemon/src/daemon/sections/module_access/client_args/path_resolution.rs b/crates/daemon/src/daemon/sections/module_access/client_args/path_resolution.rs index 82746f6fd..ccf9a1a2f 100644 --- a/crates/daemon/src/daemon/sections/module_access/client_args/path_resolution.rs +++ b/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 `/etc/...` and cannot enumerate outside the -/// module root, which is the same containment upstream gets from -/// `sanitize_path` at depth 0. +/// therefore resolves to `/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() { diff --git a/crates/daemon/src/daemon/sections/module_access/client_args/server_config.rs b/crates/daemon/src/daemon/sections/module_access/client_args/server_config.rs index 5873060d9..34cceb560 100644 --- a/crates/daemon/src/daemon/sections/module_access/client_args/server_config.rs +++ b/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 = if role == ServerRole::Receiver { let dest = resolve_receiver_dest( std::path::Path::new(&module.path), diff --git a/crates/fast_io/src/dir_sandbox/at_syscalls/metadata_ops.rs b/crates/fast_io/src/dir_sandbox/at_syscalls/metadata_ops.rs index 806602a33..ff34f3889 100644 --- a/crates/fast_io/src/dir_sandbox/at_syscalls/metadata_ops.rs +++ b/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); + } +} diff --git a/crates/fast_io/src/pinned_root.rs b/crates/fast_io/src/pinned_root.rs index 9ca9ef4b4..74bb46873 100644 --- a/crates/fast_io/src/pinned_root.rs +++ b/crates/fast_io/src/pinned_root.rs @@ -107,6 +107,68 @@ pub fn read_dir(path: &Path) -> io::Result { 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 { + 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. /// diff --git a/crates/transfer/src/generator/file_list/walk.rs b/crates/transfer/src/generator/file_list/walk.rs index c445102ab..c50aa29b7 100644 --- a/crates/transfer/src/generator/file_list/walk.rs +++ b/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 { + #[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", ...) diff --git a/tools/ci/upstream-3.5.0-expect.macos.nonroot.txt b/tools/ci/upstream-3.5.0-expect.macos.nonroot.txt index 765ea49df..036cfadf0 100644 --- a/tools/ci/upstream-3.5.0-expect.macos.nonroot.txt +++ b/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