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 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