From 20a898c60b16765c14c88a1780a682ab4900aff5 Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Tue, 13 Oct 2026 03:34:12 -0700 Subject: [PATCH 1/2] chown, chgrp: judge --preserve-root on the stat the descent is pinned to The recursive operand's --preserve-root check looked the path up again, separately from the stat that the operand's descriptor is then verified against. A rename landing between the two let that check see a harmless directory while the chown and the descent went into "/" under -H or -L. Also decide on that stat, through a new uucore::fs::metadata_is_root_dir. The path lookup stays, so a symlink to "/" that the stat did not follow is still reported as before. --- src/uucore/src/lib/features/fs.rs | 24 ++++++++++++++++++++++++ src/uucore/src/lib/features/perms.rs | 23 ++++++++++++++++++++--- 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/src/uucore/src/lib/features/fs.rs b/src/uucore/src/lib/features/fs.rs index c17fa99d21d..46007586b79 100644 --- a/src/uucore/src/lib/features/fs.rs +++ b/src/uucore/src/lib/features/fs.rs @@ -638,6 +638,21 @@ pub fn path_is_root_dir>(path: P, dereference: bool) -> bool { } } +/// Whether the file that `meta`, a `stat` already taken, describes is `/`. +/// +/// Unlike [`path_is_root_dir`] this does not look the path up again, so the +/// answer describes the file the caller is about to act on even if the path +/// has been re-pointed since. Callers hold either a [`fs::Metadata`] or, from +/// an `fstat` of a descriptor, a `safe_traversal::Metadata`, hence the generic. +#[cfg(unix)] +pub fn metadata_is_root_dir(meta: &impl MetadataExt) -> bool { + // st_dev and st_ino have different types on different platforms + #[allow(clippy::unnecessary_cast)] + root_file_information().is_some_and(|root| { + root.0.st_dev as u64 == meta.dev() && root.0.st_ino as u64 == meta.ino() + }) +} + /// Check if two files are identical by comparing their contents. /// /// Returns `Ok(true)` if both files exist, are regular files, and have identical contents. @@ -1617,4 +1632,13 @@ mod tests { assert!(path_is_root_dir(&link, true)); assert!(!path_is_root_dir(&link, false)); } + + #[cfg(unix)] + #[test] + fn test_metadata_is_root_dir() { + assert!(metadata_is_root_dir(&fs::metadata("/").unwrap())); + + let dir = tempdir().unwrap(); + assert!(!metadata_is_root_dir(&fs::metadata(dir.path()).unwrap())); + } } diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 3eb3dca9fae..52f93d23fe3 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -25,7 +25,7 @@ use walkdir::WalkDir; #[cfg(target_os = "linux")] use crate::features::fs::FileInformation; -use crate::features::fs::path_is_root_dir; +use crate::features::fs::{metadata_is_root_dir, path_is_root_dir}; #[cfg(target_os = "linux")] use crate::features::safe_traversal::{DirFd, FileInfo, SymlinkBehavior}; @@ -244,7 +244,21 @@ fn is_root(path: &Path, would_traverse_symlink: bool) -> bool { if !path_is_root_dir(path, would_traverse_symlink) { return false; } + report_root(path); + true +} + +/// [`is_root`], judged on `meta` the caller already holds instead of on a new +/// lookup of `path`, which a concurrent rename could have re-pointed. +fn meta_is_root(path: &Path, meta: &Metadata) -> bool { + if !metadata_is_root_dir(meta) { + return false; + } + report_root(path); + true +} +fn report_root(path: &Path) { if path.as_os_str() == "/" { show_error!("it is dangerous to operate recursively on '/'"); } else { @@ -254,7 +268,6 @@ fn is_root(path: &Path, would_traverse_symlink: bool) -> bool { ); } show_error!("use --no-preserve-root to override this failsafe"); - true } /// Whether `dir_fd` refers to the very object `meta` describes. @@ -306,9 +319,13 @@ impl ChownExecutor { return 1; }; + // Also judge `meta`: the descent below is pinned to it, so a swap after the + // path lookup in `is_root` cannot slip "/" past, while that lookup still + // catches a symlink to "/" which `meta` did not follow. if self.recursive && self.preserve_root - && is_root(path, self.traverse_symlinks != TraverseSymlinks::None) + && (is_root(path, self.traverse_symlinks != TraverseSymlinks::None) + || meta_is_root(path, &meta)) { // Fail-fast, do not attempt to recurse. return 1; From 73df2284d32e2507b25e486ac1afc25882bb7e43 Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Tue, 13 Oct 2026 03:34:12 -0700 Subject: [PATCH 2/2] chmod: change the -R operand's mode through a descriptor The operand of a recursive chmod was checked against --preserve-root by path, changed by path and then opened by path to descend, so a rename landing between those steps could send the mode change and the descent into "/" under -H, or through a symlink swapped in under -P. Open the operand first, check that descriptor is not "/", and change the mode through it. The directory is then opened again to descend, as GNU does, so the new mode still decides whether it can be read, and the descent goes ahead only if that is the same directory. If the operand can't be opened because it isn't readable yet or no descriptors are left, or under -h with -H or -L, the mode is changed by path as before; other open errors are reported. --- .../workspace.wordlist.txt | 2 + src/uu/chmod/src/chmod.rs | 126 +++++++++-- tests/by-util/test_chmod.rs | 203 +++++++++++++++++- 3 files changed, 310 insertions(+), 21 deletions(-) diff --git a/.vscode/cspell.dictionaries/workspace.wordlist.txt b/.vscode/cspell.dictionaries/workspace.wordlist.txt index 971e99afcdb..a921c759070 100644 --- a/.vscode/cspell.dictionaries/workspace.wordlist.txt +++ b/.vscode/cspell.dictionaries/workspace.wordlist.txt @@ -118,6 +118,8 @@ EBUSY EEXIST EINVAL EISDIR +EMFILE +ENFILE ENODATA ENOENT ENOSPC diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index f586ef8a681..8907180e6bc 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (ToDO) Chmoder cmode fmode fperm fref ugoa RFILE RFILE's +// spell-checker:ignore (ToDO) Chmoder cmode fmode fperm fref ugoa RFILE RFILE's fchmod #![cfg(unix)] @@ -45,6 +45,12 @@ enum ChmodError { NewPermissions(PathBuf, String, String), #[error("{}", translate!("chmod-error-changing-permissions", "file" => _0.quote(), "err" => strip_errno(_1)))] ChangingPermissions(PathBuf, std::io::Error), + #[cfg(not(target_os = "redox"))] + #[error("{}", translate!("perms-cannot-access-replaced", "file" => _0.quote()))] + Replaced(PathBuf), + #[cfg(not(target_os = "redox"))] + #[error("{}", translate!("perms-cannot-access", "file" => _0.quote(), "error" => strip_errno(_1)))] + CannotAccess(PathBuf, std::io::Error), } impl UError for ChmodError {} @@ -595,8 +601,6 @@ impl Chmoder { return Ok(()); } - let mut r = self.chmod_file(file_path); - // Determine whether to traverse symlinks based on context and traversal mode let should_follow_symlink = match self.traverse_symlinks { TraverseSymlinks::All => true, @@ -604,22 +608,76 @@ impl Chmoder { TraverseSymlinks::None => false, }; - // Recurse via safe traversal, opening under the same symlink policy the checks - // above used: the pathname is resolved again here, so under `-P` (the `-R` - // default) O_NOFOLLOW fails the open rather than redirecting the descent into a - // swapped-in symlink. `-H`/`-L` still follow, which is what they ask for. - if (!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir() { - match DirFd::open(file_path, should_follow_symlink.into()) { - Ok(dir_fd) => { - r = self.safe_traverse_dir(&dir_fd, file_path, ancestors).and(r); + let descend = (!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir(); + if !descend { + return self.chmod_file(file_path); + } + + // Change the mode through a descriptor checked not to be "/", which a rename + // cannot re-point. Under `-h` with `-H`/`-L`, or if it does not open yet, go by + // path as GNU does; the descent below then checks for "/" itself. + let pinned = if should_follow_symlink == self.dereference { + match DirFd::open(file_path, should_follow_symlink.into()) + .and_then(|dir_fd| Ok((dir_fd.metadata()?, dir_fd))) + { + Ok(pinned) => Some(pinned), + Err(e) + if matches!( + e.raw_os_error(), + Some(uucore::libc::EACCES | uucore::libc::EMFILE | uucore::libc::ENFILE) + ) => + { + None + } + // Other errors, such as the name now leading to a file or a symlink, + // leave it alone rather than change whatever is there by path. + Err(e) => return Err(ChmodError::CannotAccess(file_path.into(), e).into()), + } + } else { + None + }; + // The descriptor is closed once the mode is changed, keeping only the + // directory's identity, so the walk holds no more descriptors than without it. + let (mut r, changed) = match pinned { + Some((meta, dir_fd)) => { + if self.preserve_root && uucore::fs::metadata_is_root_dir(&meta) { + show!(ChmodError::PreserveRootSameAs(file_path.into())); + return Ok(()); } - Err(err) => { - // Handle permission denied errors with proper file path context - if err.kind() == std::io::ErrorKind::PermissionDenied { - r = r.and(Err(ChmodError::PermissionDenied(file_path.into()).into())); - } else { - r = r.and(Err(err.into())); + let r = self.chmod_dir_fd(&dir_fd, meta.mode() & 0o7777, file_path); + (r, Some(meta.file_info())) + } + None => (self.chmod_file(file_path), None), + }; + + // Open again to descend, as GNU does, so the mode just set decides whether + // the directory can be read. + match DirFd::open(file_path, should_follow_symlink.into()) { + Ok(dir_fd) => { + if let Some(changed) = changed { + // Descend only into the directory changed above, known not to be "/". + if !dir_fd + .metadata() + .is_ok_and(|meta| meta.file_info() == changed) + { + return r.and(Err(ChmodError::Replaced(file_path.into()).into())); } + } else if self.preserve_root + && dir_fd + .metadata() + .is_ok_and(|meta| uucore::fs::metadata_is_root_dir(&meta)) + { + show!(ChmodError::PreserveRootSameAs(file_path.into())); + return r; + } + r = self.safe_traverse_dir(&dir_fd, file_path, ancestors).and(r); + } + Err(err) => { + // Handle permission denied errors with proper file path context + if err.kind() == std::io::ErrorKind::PermissionDenied { + r = r.and(Err(ChmodError::PermissionDenied(file_path.into()).into())); + } else { + r = r.and(Err(ChmodError::CannotAccess(file_path.into(), err).into())); } } } @@ -838,8 +896,29 @@ impl Chmoder { return Ok(()); } - self.change_file(fperm, self.fmode.unwrap_or(new_mode), file)?; + let mode = self.fmode.unwrap_or(new_mode); + self.change_file(fperm, mode, file, || { + fs::set_permissions(file, fs::Permissions::from_mode(mode)) + })?; + self.check_umask_kept(file, new_mode, naively_expected_new_mode) + } + /// [`Self::chmod_file`] for a directory already open, which no rename can + /// re-point, whose mode bits are `fperm`. + #[cfg(not(target_os = "redox"))] + fn chmod_dir_fd(&self, dir_fd: &DirFd, fperm: u32, file: &Path) -> UResult<()> { + let (new_mode, naively_expected_new_mode) = self.calculate_new_mode(fperm, true)?; + let mode = self.fmode.unwrap_or(new_mode); + self.change_file(fperm, mode, file, || dir_fd.fchmod(mode))?; + self.check_umask_kept(file, new_mode, naively_expected_new_mode) + } + + fn check_umask_kept( + &self, + file: &Path, + new_mode: u32, + naively_expected_new_mode: u32, + ) -> UResult<()> { // A bare mode such as `-w` is umask-relative, so the umask can keep permissions that // the user asked to drop. GNU reports that as an error, but only when the mode was // written in the option-like form (`chmod -w f`), where it doubles as a hint that the @@ -860,11 +939,18 @@ impl Chmoder { Ok(()) } - fn change_file(&self, fperm: u32, mode: u32, file: &Path) -> Result<(), i32> { + /// Report the outcome of `set`, which applies `mode` to `file`. + fn change_file( + &self, + fperm: u32, + mode: u32, + file: &Path, + set: impl FnOnce() -> std::io::Result<()>, + ) -> Result<(), i32> { // Always issue the chmod(2) call, even when the bits are unchanged: the // syscall can still fail (e.g. lacking permission on the file) and that // failure must be reported, matching GNU. - if let Err(err) = fs::set_permissions(file, fs::Permissions::from_mode(mode)) { + if let Err(err) = set() { if !self.quiet { show_error!("{}", ChmodError::ChangingPermissions(file.into(), err)); } diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index f4e07d9bdbf..8aacfb34701 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (words) dirfd subdirs openat FDCWD rwxr +// spell-checker:ignore (words) dirfd subdirs openat FDCWD rwxr Nofile use std::fs::{OpenOptions, Permissions, metadata, set_permissions}; use std::os::unix::fs::{MetadataExt, OpenOptionsExt, PermissionsExt}; @@ -598,6 +598,207 @@ fn test_chmod_preserve_root_symlink_during_recursion() { ); } +#[test] +fn test_chmod_preserve_root_symlink_operand() { + // A symlink operand that resolves to '/' is '/' whenever it is followed: + // by default and with -H or -L, and with -P once a trailing slash makes the + // lookup resolve it. `u+u` changes nothing, should the check ever let it by. + let cases: [(&[&str], &str); 4] = [ + (&["-R"], "root"), + (&["-R", "-H"], "root"), + (&["-R", "-L"], "root"), + (&["-R", "-P"], "root/"), + ]; + for (flags, operand) in cases { + let (at, mut ucmd) = at_and_ucmd!(); + at.symlink_dir("/", "root"); + ucmd.args(flags) + .args(&["--preserve-root", "u+u", operand]) + .fails_with_code(1) + .stderr_is(format!( + "chmod: it is dangerous to operate recursively on '{operand}' (same as '/')\n\ + chmod: use --no-preserve-root to override this failsafe\n" + )); + } +} + +#[test] +fn test_chmod_recursive_operand_that_does_not_open() { + // An operand directory that cannot be opened yet still has its mode changed, + // and is descended into once that change makes it readable, also through a + // symlink operand, which -P leaves alone. + for flags in [&["-R"][..], &["-R", "-H"], &["-R", "-L"], &["-R", "-P"]] { + for operand in ["d", "link"] { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("d"); + at.touch("d/f"); + at.set_mode("d/f", 0o644); + at.set_mode("d", 0o000); + at.relative_symlink_dir("d", "link"); + + ucmd.args(flags) + .args(&["u+rwx", operand]) + .succeeds() + .no_output(); + + let mode = |file| at.metadata(file).permissions().mode() & 0o7777; + if operand == "link" && flags.contains(&"-P") { + assert_eq!(mode("d"), 0o000, "{flags:?} {operand}"); + at.set_mode("d", 0o755); + continue; + } + assert_eq!(mode("d"), 0o700, "{flags:?} {operand}"); + assert_eq!(mode("d/f"), 0o744, "{flags:?} {operand}"); + } + } +} + +#[test] +fn test_chmod_recursive_operand_made_unreadable() { + // An operand that the mode change leaves unreadable keeps its new mode, and + // the failed descent names it. Root reads it regardless. + if rustix::process::geteuid().is_root() { + return; + } + for flags in [&["-R"][..], &["-R", "-H"], &["-R", "-L"], &["-R", "-P"]] { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("d"); + at.touch("d/f"); + at.set_mode("d/f", 0o644); + at.set_mode("d", 0o755); + + ucmd.args(flags) + .args(&["a-r", "d"]) + .fails_with_code(1) + .stderr_is("chmod: cannot access 'd': Permission denied\n"); + + assert_eq!( + at.metadata("d").permissions().mode() & 0o7777, + 0o311, + "{flags:?}" + ); + at.set_mode("d", 0o755); + assert_eq!( + at.metadata("d/f").permissions().mode() & 0o7777, + 0o644, + "{flags:?}" + ); + } +} + +#[test] +fn test_chmod_recursive_no_dereference_operand() { + // With -h, a directory operand followed by -H or -L has its mode changed by + // path rather than through a descriptor; it is still changed and descended. + for flags in [ + &["-R", "-h"][..], + &["-R", "-H", "-h"], + &["-R", "-L", "-h"], + &["-R", "-P", "-h"], + ] { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("d"); + at.touch("d/f"); + at.set_mode("d", 0o755); + at.set_mode("d/f", 0o644); + + ucmd.args(flags).args(&["700", "d"]).succeeds().no_output(); + + assert_eq!(at.metadata("d").permissions().mode() & 0o7777, 0o700); + assert_eq!(at.metadata("d/f").permissions().mode() & 0o7777, 0o700); + } +} + +/// `chmod -R` changes the operand's mode and then opens it again to descend: +/// it must only descend into the directory whose mode it changed, which is the +/// one checked for '/', whatever the operand's name points at by then. A racer +/// re-points the operand between two directories of the fixture, so that a +/// failure never reaches outside it, and a file, which fails the open. +#[test] +#[cfg(not(target_os = "redox"))] +fn test_chmod_recursive_descends_only_into_the_directory_it_changed() { + use std::sync::Arc; + use std::sync::atomic::{AtomicBool, Ordering}; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + for dir in ["a", "b"] { + at.mkdir(dir); + at.touch(format!("{dir}/f")); + } + at.touch("x"); + at.relative_symlink_dir("a", "op"); + + let stop = Arc::new(AtomicBool::new(false)); + let (op, tmp) = (at.plus("op"), at.plus("op.tmp")); + let racer_stop = stop.clone(); + let racer = std::thread::spawn(move || { + for target in ["b", "x", "a", "x"].iter().cycle() { + if racer_stop.load(Ordering::Relaxed) { + break; + } + std::os::unix::fs::symlink(target, &tmp).unwrap(); + std::fs::rename(&tmp, &op).unwrap(); + } + }); + + for _ in 0..200 { + for dir in ["a", "b"] { + at.set_mode(dir, 0o755); + at.set_mode(&format!("{dir}/f"), 0o644); + } + + let result = scene.ucmd().args(&["-R", "700", "op"]).run(); + + for dir in ["a", "b"] { + let mode = |file: &str| at.metadata(file).permissions().mode() & 0o7777; + assert!( + mode(&format!("{dir}/f")) == 0o644 || mode(dir) == 0o700, + "descended into {dir} without changing its mode" + ); + } + if !result.succeeded() { + // One error about the operand. Its cause varies with the timing: + // macOS can also fail a lookup through a symlink being replaced. + let stderr = result.code_is(1).stderr_str(); + assert!( + stderr.starts_with("chmod: ") + && stderr.contains("'op'") + && stderr.lines().count() == 1, + "{stderr}" + ); + } + } + + stop.store(true, Ordering::Relaxed); + racer.join().unwrap(); +} + +/// Each directory reached through a symlink under -L takes one descriptor +/// while the walk is below it, not more. +#[test] +#[cfg(target_os = "linux")] +fn test_chmod_recursive_dereference_one_descriptor_per_level() { + use rustix::process::Resource; + + let (at, mut ucmd) = at_and_ucmd!(); + for i in 0..=10 { + at.mkdir(format!("d{i}")); + } + for i in 0..10 { + at.relative_symlink_dir(&format!("../d{}", i + 1), &format!("d{i}/next")); + } + at.touch("d10/f"); + at.set_mode("d10/f", 0o644); + + ucmd.args(&["-R", "-L", "700", "d0"]) + .limit(Resource::Nofile, 20, 20) + .succeeds() + .no_output(); + + assert_eq!(at.metadata("d10/f").permissions().mode() & 0o7777, 0o700); +} + #[test] fn test_chmod_recursive_reference_does_not_follow_inner_symlink() { // A symlink met inside the tree during `chmod -R` must not have its referent