From 4b4320113f0a4c409930ef2ae0776bb580009737 Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Mon, 5 Oct 2026 18:59:11 -0700 Subject: [PATCH 1/2] chown, chgrp: decide --from on the file that is changed --from was checked on a stat of the name and the change was then made by name, so a rename landing in between changed a file that never had the required owner. On Linux, hold the file open with O_PATH, check that it is the one that was stat'd and still passes --from, and change it through that descriptor with fchownat(AT_EMPTY_PATH). This covers operands and -R entries. A file found replaced is left alone; as with GNU, that fails without a message, and -v reports the change that was not made. --- src/uucore/src/lib/features/perms.rs | 198 +++++++++++++++--- src/uucore/src/lib/features/safe_traversal.rs | 63 +++++- tests/by-util/test_chown.rs | 18 ++ 3 files changed, 247 insertions(+), 32 deletions(-) diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 3eb3dca9fae..31e1a2714eb 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -18,6 +18,8 @@ use libc::{gid_t, uid_t}; use options::traverse; #[cfg(target_os = "linux")] use std::collections::HashSet; +#[cfg(target_os = "linux")] +use std::ffi::OsStr; use std::ffi::OsString; #[cfg(not(target_os = "linux"))] @@ -27,7 +29,9 @@ use walkdir::WalkDir; use crate::features::fs::FileInformation; use crate::features::fs::path_is_root_dir; #[cfg(target_os = "linux")] -use crate::features::safe_traversal::{DirFd, FileInfo, SymlinkBehavior}; +use crate::features::safe_traversal::{ + DirFd, FileInfo, Metadata as TraversalMetadata, PinnedFile, SymlinkBehavior, +}; use std::ffi::CString; use std::fs::Metadata; @@ -96,13 +100,28 @@ pub fn wrap_chown>( dest_gid: Option, follow: bool, verbosity: Verbosity, +) -> Result { + let path = path.as_ref(); + report_chown(path, meta, dest_uid, dest_gid, verbosity, |uid, gid| { + chown(path, uid, gid, follow) + }) +} + +/// Change the owner of the file `meta` describes through `change`, and +/// describe the outcome as [`wrap_chown`] does. +fn report_chown( + path: &Path, + meta: &Metadata, + dest_uid: Option, + dest_gid: Option, + verbosity: Verbosity, + change: impl FnOnce(uid_t, gid_t) -> IOResult<()>, ) -> Result { let dest_uid = dest_uid.unwrap_or_else(|| meta.uid()); let dest_gid = dest_gid.unwrap_or_else(|| meta.gid()); - let path = path.as_ref(); let mut out: String = String::new(); - if let Err(e) = chown(path, dest_uid, dest_gid, follow) { + if let Err(e) = change(dest_uid, dest_gid) { match verbosity.level { VerbosityLevel::Silent => (), level => { @@ -117,26 +136,13 @@ pub fn wrap_chown>( strip_errno(&e), ); if level == VerbosityLevel::Verbose { - out = if verbosity.groups_only { - let gid = meta.gid(); - format!( - "{out}\nfailed to change group of {} from {} to {}", - path.quote(), - entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string()), - entries::gid2grp(dest_gid).unwrap_or_else(|_| dest_gid.to_string()) - ) - } else { - let uid = meta.uid(); - let gid = meta.gid(); - format!( - "{out}\nfailed to change ownership of {} from {}:{} to {}:{}", - path.quote(), - entries::uid2usr(uid).unwrap_or_else(|_| uid.to_string()), - entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string()), - entries::uid2usr(dest_uid).unwrap_or_else(|_| dest_uid.to_string()), - entries::gid2grp(dest_gid).unwrap_or_else(|_| dest_gid.to_string()) - ) - }; + let failed = failed_change_line( + path, + (meta.uid(), meta.gid()), + (dest_uid, dest_gid), + verbosity.groups_only, + ); + out = format!("{out}\n{failed}"); } } } @@ -190,6 +196,32 @@ pub fn wrap_chown>( Ok(out) } +/// The verbose line for a change of the owner of `path` that did not happen. +fn failed_change_line( + path: &Path, + (uid, gid): (uid_t, gid_t), + (dest_uid, dest_gid): (uid_t, gid_t), + groups_only: bool, +) -> String { + if groups_only { + format!( + "failed to change group of {} from {} to {}", + path.quote(), + entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string()), + entries::gid2grp(dest_gid).unwrap_or_else(|_| dest_gid.to_string()) + ) + } else { + format!( + "failed to change ownership of {} from {}:{} to {}:{}", + path.quote(), + entries::uid2usr(uid).unwrap_or_else(|_| uid.to_string()), + entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string()), + entries::uid2usr(dest_uid).unwrap_or_else(|_| dest_uid.to_string()), + entries::gid2grp(dest_gid).unwrap_or_else(|_| dest_gid.to_string()) + ) + } +} + pub enum IfFrom { All, User(u32), @@ -363,6 +395,24 @@ impl ChownExecutor { // The open failed; safe_dive_into reports it. None => Ok(String::new()), } + } else if !matches!(self.filter, IfFrom::All) { + // `--from` was judged on `meta`, so change that very file, held + // open, and not whatever a rename may have put under `path` since. + match self.hold(&meta, PinnedFile::open(path, self.dereference.into())) { + Ok(Some(file)) => report_chown( + path, + &meta, + self.dest_uid, + self.dest_gid, + self.verbosity.clone(), + |uid, gid| file.chown(Some(uid), Some(gid)), + ), + // Not a directory, so there is nothing to recurse into. + Ok(None) => return self.report_replaced(path, (meta.uid(), meta.gid())), + Err(e) => Err( + translate!("perms-cannot-access", "file" => path.quote(), "error" => strip_errno(&e)), + ), + } } else { // For non-directories (files, symlinks), use the regular wrap_chown method wrap_chown( @@ -473,6 +523,58 @@ impl ChownExecutor { Ok(()) } + /// Hold open, through `open`, the file `meta` describes, which passed + /// `--from`, so that the change goes to that file and not to one a rename + /// may have put under its name since. `None` if the file held is not that + /// one, or no longer passes `--from`. + #[cfg(target_os = "linux")] + fn hold( + &self, + meta: &impl MetadataExt, + open: IOResult, + ) -> IOResult> { + let file = open?; + let held = file.metadata()?; + let same = (held.dev(), held.ino()) == (meta.dev(), meta.ino()); + Ok((same && self.matched(held.uid(), held.gid())).then_some(file)) + } + + /// Change the entry `name` of `dir_fd`, which `meta` describes and which + /// passed `--from`, through [`Self::hold`] if `--from` was given. Returns + /// whether it was changed. + #[cfg(target_os = "linux")] + fn chown_entry( + &self, + dir_fd: &DirFd, + name: &OsStr, + meta: &TraversalMetadata, + follow: bool, + ) -> IOResult { + // Under -H a symlink is stat'd itself but changed through: the file + // changed is not the one `meta` describes, so there is none to hold. + if matches!(self.filter, IfFrom::All) || follow && meta.file_type().is_symlink() { + dir_fd.chown_at(name, self.dest_uid, self.dest_gid, follow.into())?; + return Ok(true); + } + let Some(file) = self.hold(meta, dir_fd.pin_at(name, follow.into()))? else { + return Ok(false); + }; + file.chown(self.dest_uid, self.dest_gid)?; + Ok(true) + } + + /// Leave alone a file found replaced after it passed `--from`. As with + /// GNU, that fails without an error message, and only -v says so. + #[cfg(target_os = "linux")] + fn report_replaced(&self, path: &Path, (uid, gid): (uid_t, gid_t)) -> i32 { + if self.verbosity.level == VerbosityLevel::Verbose { + let dest = (self.dest_uid.unwrap_or(uid), self.dest_gid.unwrap_or(gid)); + let line = failed_change_line(path, (uid, gid), dest, self.verbosity.groups_only); + self.write_verbose_line(&line); + } + 1 + } + /// `operand_fd` is the descriptor `traverse` opened and verified against `meta`. #[cfg(target_os = "linux")] fn safe_dive_into>( @@ -579,17 +681,13 @@ impl ChownExecutor { // Check if we should chown this entry if self.matched(meta.uid(), meta.gid()) { - // Use fchownat for the actual ownership change let follow_symlinks = self.dereference || self.traverse_symlinks == TraverseSymlinks::All; - // Only pass the IDs that should actually be changed - let chown_uid = self.dest_uid; - let chown_gid = self.dest_gid; - - if let Err(e) = - dir_fd.chown_at(&entry_name, chown_uid, chown_gid, follow_symlinks.into()) - { + let changed = self.chown_entry(dir_fd, &entry_name, &meta, follow_symlinks); + if let Ok(false) = changed { + *ret = self.report_replaced(&entry_path, (meta.uid(), meta.gid())); + } else if let Err(e) = changed { *ret = 1; if self.verbosity.level != VerbosityLevel::Silent { let msg = format!( @@ -1119,6 +1217,44 @@ mod tests { assert!(fd_is(&link_fd, &meta).unwrap()); } + /// `hold` must refuse a file renamed over the one that passed `--from`, + /// even one that would pass too, and a file that no longer passes. + #[cfg(target_os = "linux")] + #[test] + fn test_hold_refuses_another_file() { + let temp_dir = tempdir().unwrap(); + let item = temp_dir.path().join("item"); + std::fs::write(&item, "").unwrap(); + std::fs::write(temp_dir.path().join("other"), "").unwrap(); + let meta = std::fs::metadata(&item).unwrap(); + let dir_fd = DirFd::open(temp_dir.path(), SymlinkBehavior::Follow).unwrap(); + let name = OsStr::new("item"); + let entry_meta = dir_fd.metadata_at(name, SymlinkBehavior::NoFollow).unwrap(); + let open = || PinnedFile::open(&item, SymlinkBehavior::Follow); + let mut executor = ChownExecutor { + dest_uid: None, + dest_gid: None, + raw_owner: String::new(), + traverse_symlinks: TraverseSymlinks::None, + verbosity: Verbosity::default(), + filter: IfFrom::Group(meta.gid()), + files: Vec::new(), + recursive: false, + preserve_root: false, + dereference: true, + }; + assert!(executor.hold(&meta, open()).unwrap().is_some()); + + std::fs::rename(temp_dir.path().join("other"), &item).unwrap(); + assert!(executor.hold(&meta, open()).unwrap().is_none()); + let pin = dir_fd.pin_at(name, SymlinkBehavior::NoFollow); + assert!(executor.hold(&entry_meta, pin).unwrap().is_none()); + + let meta = std::fs::metadata(&item).unwrap(); + executor.filter = IfFrom::Group(meta.gid().wrapping_add(1)); + assert!(executor.hold(&meta, open()).unwrap().is_none()); + } + #[test] fn test_empty_string() { let path = PathBuf::new(); diff --git a/src/uucore/src/lib/features/safe_traversal.rs b/src/uucore/src/lib/features/safe_traversal.rs index 2ebc460f1d6..bf5b78901ac 100644 --- a/src/uucore/src/lib/features/safe_traversal.rs +++ b/src/uucore/src/lib/features/safe_traversal.rs @@ -6,7 +6,7 @@ // spell-checker:ignore CLOEXEC RDONLY TOCTOU closedir dirp fdopendir fstatat openat REMOVEDIR unlinkat smallfile // spell-checker:ignore RAII dirfd fchownat fchown FchmodatFlags fchmodat fchmod mkdirat CREAT WRONLY ELOOP ENOTDIR EXCL EEXIST // spell-checker:ignore atimensec mtimensec ctimensec opath chmods fakeroot fakechroot EOVERFLOW chowned chmoded -// spell-checker:ignore LARGEFILE getdents atim mtim ctim statat +// spell-checker:ignore LARGEFILE getdents atim mtim ctim statat FDCWD // Safe directory traversal using openat() and related syscalls // This module provides TOCTOU-safe filesystem operations for recursive traversal @@ -327,6 +327,16 @@ impl DirFd { Ok(Self { fd }) } + /// Hold the file `name` relative to this directory, see [`PinnedFile`] + #[cfg(target_os = "linux")] + pub fn pin_at( + &self, + name: &OsStr, + symlink_behavior: SymlinkBehavior, + ) -> io::Result { + PinnedFile::open_at(&self.fd, name, symlink_behavior) + } + /// Get raw stat data for a file relative to this directory pub fn stat_at(&self, name: &OsStr, symlink_behavior: SymlinkBehavior) -> io::Result { let name_cstr = @@ -806,6 +816,57 @@ impl AsFd for DirFd { } } +/// A descriptor holding one file of any type without opening it (O_PATH), so +/// that what is learned about the file and what is done to it concern that +/// same file, whatever its name is re-pointed to in between. +#[cfg(target_os = "linux")] +pub struct PinnedFile { + fd: OwnedFd, +} + +#[cfg(target_os = "linux")] +impl PinnedFile { + /// Hold the file at `path` + pub fn open(path: &Path, symlink_behavior: SymlinkBehavior) -> io::Result { + Self::open_at(nix::fcntl::AT_FDCWD, path, symlink_behavior) + } + + fn open_at( + dir: impl AsFd, + path: &P, + symlink_behavior: SymlinkBehavior, + ) -> io::Result { + let mut flags = OFlag::O_PATH | OFlag::O_CLOEXEC; + if !symlink_behavior.should_follow() { + // With O_PATH, this holds a symlink itself. + flags |= OFlag::O_NOFOLLOW; + } + let fd = openat(dir, path, flags, Mode::empty()) + .map_err(|e| io::Error::from_raw_os_error(e as i32))?; + Ok(Self { fd }) + } + + /// Get metadata for the held file + pub fn metadata(&self) -> io::Result { + fstat_fd(&self.fd).map(Metadata::from_stat) + } + + /// Change ownership of the held file + /// Use uid/gid of None to keep the current value + pub fn chown(&self, uid: Option, gid: Option) -> io::Result<()> { + // fchown refuses an O_PATH descriptor; an empty path names the + // descriptor itself. + fchownat( + &self.fd, + c"", + uid.map(Uid::from_raw), + gid.map(Gid::from_raw), + nix::fcntl::AtFlags::AT_EMPTY_PATH, + ) + .map_err(|e| io::Error::from_raw_os_error(e as i32)) + } +} + /// File information for tracking inodes #[derive(Debug, Clone, Hash, PartialEq, Eq)] pub struct FileInfo { diff --git a/tests/by-util/test_chown.rs b/tests/by-util/test_chown.rs index 4d77788792a..d676b47463c 100644 --- a/tests/by-util/test_chown.rs +++ b/tests/by-util/test_chown.rs @@ -1141,3 +1141,21 @@ fn verbose_missing_file_write_error_is_reported_not_panic() { .fails_with_code(1) .stderr_contains("chown: write error: No space left on device"); } + +/// Under `-R -H` a symlink met in the tree is changed through, so the file +/// changed is not the link that was looked at: `--from` must not refuse it. +#[cfg(all(unix, not(target_os = "openbsd")))] +#[test] +fn test_chown_from_changes_through_symlink_in_tree_under_big_h() { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("dir"); + at.touch("target"); + at.relative_symlink_file("../target", "dir/link"); + let meta = at.plus("target").metadata().unwrap(); + let owner = format!("{}:{}", meta.uid(), meta.gid()); + + ucmd.args(&["-R", "-H", "-v", &format!("--from={owner}"), &owner, "dir"]) + .succeeds() + .stdout_contains("ownership of 'dir/link' retained as") + .stdout_does_not_contain("failed"); +} From c17a06ea3d29476b4648e99d02a3d0be34b8df24 Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Mon, 5 Oct 2026 19:03:54 -0700 Subject: [PATCH 2/2] chown, chgrp: change a directory operand that cannot be read On Linux, a directory operand was changed through a descriptor opened for reading, and when that open failed it was skipped without a message. Change it by name then, or through an O_PATH descriptor with --from, as for any other file. Under -R it is still left alone and the failure to read it reported, as with GNU. --- src/uucore/src/lib/features/perms.rs | 20 ++++++++++---------- tests/by-util/test_chown.rs | 25 +++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 31e1a2714eb..74e86d28a3c 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -387,14 +387,13 @@ impl ChownExecutor { let ret = if self.matched(meta.uid(), meta.gid()) { // Use safe syscalls for root directory to prevent TOCTOU attacks on Linux #[cfg(target_os = "linux")] - let chown_result = if meta.is_dir() { - match operand_fd.as_ref() { - Some(dir_fd) => self - .safe_chown_dir(dir_fd, path, &meta) - .map(|_| String::new()), - // The open failed; safe_dive_into reports it. - None => Ok(String::new()), - } + let chown_result = if let Some(dir_fd) = operand_fd.as_ref() { + self.safe_chown_dir(dir_fd, path, &meta) + .map(|_| String::new()) + } else if meta.is_dir() && self.recursive { + // The open failed; safe_dive_into reports it. As with GNU, + // a directory that -R cannot read is left alone. + Ok(String::new()) } else if !matches!(self.filter, IfFrom::All) { // `--from` was judged on `meta`, so change that very file, held // open, and not whatever a rename may have put under `path` since. @@ -407,14 +406,15 @@ impl ChownExecutor { self.verbosity.clone(), |uid, gid| file.chown(Some(uid), Some(gid)), ), - // Not a directory, so there is nothing to recurse into. + // Nothing to recurse into, so return now. Ok(None) => return self.report_replaced(path, (meta.uid(), meta.gid())), Err(e) => Err( translate!("perms-cannot-access", "file" => path.quote(), "error" => strip_errno(&e)), ), } } else { - // For non-directories (files, symlinks), use the regular wrap_chown method + // Change anything else, including a directory that could not + // be opened, by name. wrap_chown( path, &meta, diff --git a/tests/by-util/test_chown.rs b/tests/by-util/test_chown.rs index d676b47463c..b44f8e4cbe8 100644 --- a/tests/by-util/test_chown.rs +++ b/tests/by-util/test_chown.rs @@ -1159,3 +1159,28 @@ fn test_chown_from_changes_through_symlink_in_tree_under_big_h() { .stdout_contains("ownership of 'dir/link' retained as") .stdout_does_not_contain("failed"); } + +/// A directory operand that cannot be read is still changed without -R. +#[cfg(all(unix, not(target_os = "openbsd")))] +#[test] +fn test_chown_unreadable_dir_operand() { + use std::os::unix::fs::PermissionsExt; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir("dir"); + let meta = at.plus("dir").metadata().unwrap(); + let owner = format!("{}:{}", meta.uid(), meta.gid()); + std::fs::set_permissions(at.plus("dir"), std::fs::Permissions::from_mode(0o000)).unwrap(); + + for from in [None, Some(format!("--from={owner}"))] { + scene + .ucmd() + .arg("-v") + .args(from.as_slice()) + .args(&[owner.as_str(), "dir"]) + .succeeds() + .stdout_contains("ownership of 'dir' retained as"); + } + std::fs::set_permissions(at.plus("dir"), std::fs::Permissions::from_mode(0o755)).unwrap(); +}