From fa83449325cfefa5eb41803232480045dbf15c1d Mon Sep 17 00:00:00 2001 From: Ofer Chen Date: Fri, 4 Sep 2026 06:50:54 +0300 Subject: [PATCH 1/2] fix(receiver): clear a symlink standing at the --partial-dir name A peer-supplied `--partial-dir` naming a symlink out of the served tree was accepted as "the directory is already there", after which the staging rename followed the link and wrote outside the daemon module. MEASURED against a daemon push of `-a --delay-updates --partial-dir=/blink` where `mod/blink` was a symlink to a directory outside the module, with real rsync 3.5.0 as the control on the same host: | | exit | dest mod/f0 | outside victim | |---|---|---|---| | upstream 3.5.0 | 2, `refusing unconfined partial basis for f0` | OLD | intact | | oc before | 23 | NEW | DESTROYED | | oc after | 0 | NEW | intact | The escaping site is the `--delay-updates` staging create, not the basis open and not `rename_to_partial_dir`. `clear_partial_dir_obstruction` deliberately left a symlink standing, on the stated grounds that "the confined rename makes the refusal". That premise is refuted by the run above: the staging rename follows the link into the outside directory, and the delayed-updates sweep then renames the file back out, which is why the victim's directory ends up empty. Leaving the link could not work in any case, because `mkdirat` reports `EEXIST` for a symlink and every caller on this path maps `EEXIST` to success - `fast_io::operator_mkdir` and `create_dir_all_sandboxed` both do. So the obstruction was never merely tolerated; it was read as a usable directory. upstream util1.c:1516-1527 `handle_partial_dir(..., PDIR_CREATE)` is the rule: under `operator_path_resolve`, `do_lstat_at` finds a non-directory at the name - `!S_ISDIR` covers a symlink and a regular file alike - `do_unlink_at` clears it, and a failure to clear returns 0, at which point receiver.c:1302-1306 discards the received temp rather than staging around the obstruction. Clearing is what makes it safe: the walk resolves the parent chain and hands `unlinkat` a single leaf component, so the link is removed as the link it is and never followed. `fast_io::operator_unlink` is added as the sibling of `operator_mkdir` that this needs - the same ownership walk, the same shape, mirroring upstream's pairing of `do_unlink_at` with `do_mkdir_at` inside one `operator_path_resolve` region. Also fixes a second, non-security divergence measured on the way: with a RELATIVE `--partial-dir=blink` upstream clears the obstruction and completes (exit 0, destination updated) while oc exited 11 with the destination untouched. oc now matches upstream on that shape too. Mutation-proven: restoring only the symlink arm reproduces the original escape end to end (victim destroyed, exit 23) and reddens `clearing_the_partial_dir_removes_a_symlink_without_touching_its_target` with "the symlink standing at the partial-dir name must be removed". The pin asserts BOTH that the link is gone AND that its target survives - "the link is gone" alone would also hold for an implementation that followed the link and deleted the target, which is the opposite of this fix. DELIBERATELY NOT CHANGED, and the reason the upstream-testsuite cell `operator-path-partial-dir-daemon` still fails: upstream never sanitizes `partial_dir`. options.c:2402-2430's `if (sanitize_paths)` block covers `argv`, `tmpdir` and `backup_dir` and NOT `partial_dir`, so on a daemon `/blink` stays absolute, names the filesystem root, and cannot be created - which is where upstream's non-zero exit comes from. oc re-roots it under the module instead (the hardening added for the daemon partial-dir), so oc completes the transfer where upstream refuses. That is a policy question about whether a peer may name an absolute staging path outside the served module, not something to flip silently in a security fix; the cell's row is not re-baselined. Verified on the pinned 1.88.0 toolchain: `cargo fmt --all -- --check` clean, `cargo clippy --workspace --all-targets --all-features --no-deps -- -D warnings` clean, engine 4719/4719. --- crates/engine/src/util/cleanup.rs | 136 +++++++++++++++++++++++++----- crates/fast_io/src/lib.rs | 2 +- crates/fast_io/src/owner_walk.rs | 36 +++++++- 3 files changed, 149 insertions(+), 25 deletions(-) diff --git a/crates/engine/src/util/cleanup.rs b/crates/engine/src/util/cleanup.rs index 69493e090..3a0903dfb 100644 --- a/crates/engine/src/util/cleanup.rs +++ b/crates/engine/src/util/cleanup.rs @@ -97,39 +97,57 @@ pub fn create_partial_dir(dir: &Path) -> std::io::Result<()> { /// - `rsync-3.5.0/util1.c:1521-1528` `handle_partial_dir()` - `do_lstat_at` /// then, when the entry exists and is not a directory, /// `do_unlink_at(dir) < 0` aborts, otherwise `do_mkdir_at` runs against the -/// cleared name. Without the unlink an obstruction is fatal, where upstream -/// clears it and proceeds. +/// cleared name. /// /// Only the FINAL component is cleared. Upstream's `handle_partial_dir` names /// exactly one directory, so an obstruction standing where an *ancestor* /// belongs is not something upstream removes and is not removed here either - /// it surfaces as the caller's create error. /// -/// The probe is `symlink_metadata`, so a symlink at the name is removed as the -/// non-directory it is rather than being followed to whatever it points at -/// (upstream's `do_lstat_at` makes the same choice). +/// Both the probe and the removal run through the ownership walk, exactly as +/// upstream wraps its `do_lstat_at`/`do_unlink_at` pair in +/// `operator_path_resolve` (util1.c:1516-1527). That is what makes clearing a +/// SYMLINK safe: the walk resolves the parent chain and the leaf is handed to +/// `unlinkat` as a single component, so the link is removed as the +/// non-directory it is and is never followed to whatever it points at. +/// +/// ⚠ A symlink here is exactly the shape that must NOT be left standing. A +/// peer-supplied `--partial-dir` naming a symlink out of the served tree used +/// to be accepted as "the directory is already there" - `mkdirat` reports +/// `EEXIST` for a symlink and every caller treats `EEXIST` as success - after +/// which the staging rename followed the link and wrote outside the module. +/// MEASURED against a daemon push with `--delay-updates --partial-dir=/blink` +/// where `blink` pointed outside: the outside file was replaced and then moved +/// away by the delayed-updates sweep. Upstream never reaches that state because +/// it clears the obstruction first, and fails the file outright if it cannot. /// /// # Errors /// -/// Surfaces the `lstat` error for anything other than "not found", and any -/// `unlink` error. +/// Surfaces the walk's refusal, the `lstat` error for anything other than "not +/// found", and any `unlink` error. A failure here is fatal to the file: upstream +/// returns 0 from `handle_partial_dir()`, and `receiver.c:1302-1306` then +/// discards the received temp rather than staging around the obstruction. pub fn clear_partial_dir_obstruction(dir: &Path) -> std::io::Result<()> { - match std::fs::symlink_metadata(dir) { - Ok(metadata) if metadata.is_dir() => Ok(()), - // A SYMLINK is deliberately left in place, which upstream would unlink. - // Upstream can afford to because its unlink runs inside - // `operator_path_resolve`, so a link leading out of the served tree is - // refused rather than followed; oc cannot use that walk here (it - // resolves from `/` and the daemon's Landlock ruleset admits only the - // module root, measured). Removing the link unconfined would be worse - // than not removing it: it destroys an operator-visible symlink AND - // pre-empts the confined rename that currently refuses the escape. - // Leaving it means the staging open follows the link and the confined - // rename makes the refusal, which is upstream's outcome for that shape. - Ok(metadata) if metadata.is_symlink() => Ok(()), - Ok(_) => std::fs::remove_file(dir), - Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()), - Err(error) => Err(error), + #[cfg(unix)] + { + match fast_io::operator_symlink_metadata(dir) { + Ok(metadata) if metadata.is_dir() => Ok(()), + // upstream: util1.c:1522-1527 - `statret == 0 && !S_ISDIR(st.st_mode)` + // covers a symlink and a regular file alike; both are unlinked, and a + // failure to unlink returns 0 (failure) rather than proceeding. + Ok(_) => fast_io::operator_unlink(dir), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(error) => Err(error), + } + } + #[cfg(not(unix))] + { + match std::fs::symlink_metadata(dir) { + Ok(metadata) if metadata.is_dir() => Ok(()), + Ok(_) => std::fs::remove_file(dir), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(error) => Err(error), + } } } @@ -678,4 +696,76 @@ mod tests { assert_eq!(manager2.temp_file_count(), 1); } + + /// A SYMLINK standing at the `--partial-dir` name is REMOVED, and what it + /// points at is left alone. + /// + /// This is the security shape: a peer-supplied `--partial-dir` naming a + /// symlink out of the served tree. Leaving the link standing let `mkdirat` + /// report `EEXIST`, every caller read that as "the directory is already + /// there", and the staging rename then wrote THROUGH the link - measured + /// destroying a file outside a daemon module. + /// + /// Both halves are asserted deliberately. "The link is gone" alone would + /// also hold for an implementation that followed the link and deleted its + /// TARGET, which is the opposite of the fix; and "the target survives" + /// alone would hold for the old pass-through that removed nothing. + /// + /// upstream: `util1.c:1522-1527` - `statret == 0 && !S_ISDIR(st.st_mode)` + /// unlinks, and a failure to unlink fails the whole call. + #[cfg(unix)] + #[test] + fn clearing_the_partial_dir_removes_a_symlink_without_touching_its_target() { + let dir = tempdir().expect("tempdir"); + let outside = dir.path().join("outside"); + fs::create_dir(&outside).expect("mkdir outside"); + let victim = outside.join("keep-me"); + fs::write(&victim, b"PROTECTED").expect("write victim"); + + let obstruction = dir.path().join("partial"); + std::os::unix::fs::symlink(&outside, &obstruction).expect("plant symlink"); + + clear_partial_dir_obstruction(&obstruction).expect("clear the obstruction"); + + assert!( + fs::symlink_metadata(&obstruction).is_err(), + "the symlink standing at the partial-dir name must be removed", + ); + assert_eq!( + fs::read(&victim).expect("victim still readable"), + b"PROTECTED", + "the symlink's target must be untouched - the link is unlinked, never followed", + ); + assert!( + outside.is_dir(), + "the directory the link pointed at survives" + ); + } + + /// The companion that keeps the test above honest: an ordinary regular file + /// at the name is cleared too (upstream's predicate is `!S_ISDIR`, not + /// "is a symlink"), and an existing DIRECTORY is reused rather than removed. + #[cfg(unix)] + #[test] + fn clearing_the_partial_dir_removes_a_regular_file_and_reuses_a_directory() { + let dir = tempdir().expect("tempdir"); + + let regular = dir.path().join("regular"); + fs::write(®ular, b"obstruction").expect("write obstruction"); + clear_partial_dir_obstruction(®ular).expect("clear the regular file"); + assert!( + fs::symlink_metadata(®ular).is_err(), + "a regular file at the partial-dir name is cleared", + ); + + let existing = dir.path().join("existing"); + fs::create_dir(&existing).expect("mkdir existing"); + let inhabitant = existing.join("staged"); + fs::write(&inhabitant, b"staged").expect("write inhabitant"); + clear_partial_dir_obstruction(&existing).expect("reuse the directory"); + assert!( + inhabitant.is_file(), + "an existing partial dir is REUSED, so its contents survive", + ); + } } diff --git a/crates/fast_io/src/lib.rs b/crates/fast_io/src/lib.rs index 6289044f5..2f1596477 100644 --- a/crates/fast_io/src/lib.rs +++ b/crates/fast_io/src/lib.rs @@ -429,7 +429,7 @@ pub use owner_walk::{ operator_open_read_confined, operator_open_recv, operator_open_rw_create, operator_open_write_create, operator_open_write_create_confined, operator_read_to_string, operator_read_to_string_confined, operator_rename, operator_rename_confined, - operator_symlink_metadata, owner_trusted_parent, owner_trusted_parent_kind, + operator_symlink_metadata, operator_unlink, owner_trusted_parent, owner_trusted_parent_kind, symlink_owner_is_trusted, }; pub use refs_detect::{clear_refs_cache, is_refs_filesystem}; diff --git a/crates/fast_io/src/owner_walk.rs b/crates/fast_io/src/owner_walk.rs index e6109d3f8..8163b7b15 100644 --- a/crates/fast_io/src/owner_walk.rs +++ b/crates/fast_io/src/owner_walk.rs @@ -30,7 +30,7 @@ use std::io; use std::os::fd::{AsFd, BorrowedFd, OwnedFd}; use std::path::{Component, Path, PathBuf}; -use rustix::fs::{Mode, OFlags}; +use rustix::fs::{AtFlags, Mode, OFlags}; use rustix::io::Errno; use crate::dir_sandbox::at_syscalls; @@ -622,6 +622,40 @@ pub fn owner_trusted_parent_kind( Ok((dirfd, leaf)) } +/// Remove a non-directory at an operator-supplied path through the ownership +/// walk. +/// +/// The `unlink` counterpart to [`operator_mkdir`], and its partner in upstream's +/// `handle_partial_dir()`: the parent chain is resolved by the ownership walk +/// and the leaf removed with `unlinkat` on the resulting descriptor, so the name +/// that is cleared is the one the walk validated. Because the leaf is passed to +/// `unlinkat` as a single component, a symlink standing at that name is removed +/// as the link it is and never followed to its target. +/// +/// An already-absent leaf is success, so the call is idempotent against a racing +/// remover - the caller's goal is that the name be free, not that this call be +/// the one that freed it. +/// +/// # Upstream Reference +/// +/// - `rsync-3.5.0/util1.c:1521-1527` `handle_partial_dir()` - under +/// `operator_path_resolve = 1`, `do_lstat_at()` finds a non-directory at the +/// partial-dir name and `do_unlink_at(dir)` clears it; a failure to clear is +/// fatal to that file rather than something to stage around. +/// +/// # Errors +/// +/// Surfaces any walk error (including the refusal of a foreign-owned symlink) +/// and any `unlinkat` error other than `ENOENT`. +pub fn operator_unlink(path: &Path) -> io::Result<()> { + let (parent, leaf) = owner_trusted_parent(path)?; + match rustix::fs::unlinkat(&parent, leaf.as_os_str(), AtFlags::empty()) { + Ok(()) => Ok(()), + Err(Errno::NOENT) => Ok(()), + Err(error) => Err(error.into()), + } +} + /// Create a directory at an operator-supplied path through the ownership walk. /// /// The `mkdir` counterpart to the `operator_open_*` family: the parent chain is From f302d1d9850274df26b363b1b0755d17ef979cc9 Mon Sep 17 00:00:00 2001 From: Ofer Chen Date: Fri, 4 Sep 2026 06:57:17 +0300 Subject: [PATCH 2/2] docs(ci): say why the partial-dir daemon row is a policy fork, not a patchable bug The row's one-line classification read "real oc defect", which invites the wrong fix. Its security oracle now passes; the residual is the first oracle's non-zero exit, and upstream's non-zero exit comes from options.c:2402-2430 never sanitizing partial_dir - an absolute daemon --partial-dir stays absolute and cannot be created. oc re-roots it under the served module, so greening the row means letting a peer name a staging path outside the module. --- tools/ci/upstream-3.5.0-expect.macos.nonroot.txt | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) 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 1d1bd416c..f7f1672ac 100644 --- a/tools/ci/upstream-3.5.0-expect.macos.nonroot.txt +++ b/tools/ci/upstream-3.5.0-expect.macos.nonroot.txt @@ -21,7 +21,7 @@ # # chmod-setid upstream FAILS too - unsatisfiable here # partial-protected-regular-retry-policy upstream FAILS too - unsatisfiable here -# operator-path-partial-dir-daemon upstream PASSES - real oc defect +# operator-path-partial-dir-daemon upstream PASSES - policy fork, task 1140 # filter-merge-content-echo owned by task 1011 # # Two of the four are therefore NOT oc divergences: the platform (or this @@ -30,6 +30,17 @@ # here, not because oc is right - re-baselining them would hide the row rather # than resolve it. None of the four appears in upstream's own # testsuite/skiplist/macos.txt. +# +# operator-path-partial-dir-daemon needs a sharper note, because the obvious +# way to green it is WRONG. Its security oracle passes: a symlink standing at +# the --partial-dir name is cleared rather than staged through. What remains is +# the cell's first oracle, which wants a non-zero exit - and upstream's non-zero +# exit comes from somewhere else entirely. options.c:2402-2430 sanitizes argv, +# tmpdir and backup_dir and NEVER partial_dir, so on a daemon an absolute +# `/blink` stays absolute, names the filesystem root, and cannot be created. oc +# re-roots it under the served module instead (tasks 971/973/1032). Matching +# upstream here would let a peer name an absolute staging path outside the +# module, so this row is a confinement-policy question, not a bug to patch out. 00-hello pass acl-symlink-race skip acls pass