From 32bab9014a89bd0c07c169677e5b4fe0856e41bd Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Tue, 29 Sep 2026 18:53:48 -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::dev_ino_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 | 36 +++++++++++++++++++++++++--- 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/src/uucore/src/lib/features/fs.rs b/src/uucore/src/lib/features/fs.rs index ee4feab59d..ee1f6f404d 100644 --- a/src/uucore/src/lib/features/fs.rs +++ b/src/uucore/src/lib/features/fs.rs @@ -631,6 +631,19 @@ pub fn path_is_root_dir>(path: P, dereference: bool) -> bool { } } +/// Whether the file a `stat` already taken reports as `(dev, ino)` 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. +#[cfg(unix)] +pub fn dev_ino_is_root_dir(dev: u64, ino: u64) -> 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 == dev && root.0.st_ino as u64 == 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. @@ -1604,4 +1617,15 @@ mod tests { assert!(path_is_root_dir(&link, true)); assert!(!path_is_root_dir(&link, false)); } + + #[cfg(unix)] + #[test] + fn test_dev_ino_is_root_dir() { + let root = fs::metadata("/").unwrap(); + assert!(dev_ino_is_root_dir(root.dev(), root.ino())); + + let dir = tempdir().unwrap(); + let other = fs::metadata(dir.path()).unwrap(); + assert!(!dev_ino_is_root_dir(other.dev(), other.ino())); + } } diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 29f5c7aaca..a83ebca97d 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -27,7 +27,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::{dev_ino_is_root_dir, path_is_root_dir}; #[cfg(target_os = "linux")] use crate::features::safe_traversal::{DirFd, FileInfo, SymlinkBehavior}; @@ -246,7 +246,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 !dev_ino_is_root_dir(meta.dev(), meta.ino()) { + 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 { @@ -256,7 +270,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. @@ -308,9 +321,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; @@ -1121,6 +1138,19 @@ mod tests { assert!(fd_is(&link_fd, &meta).unwrap()); } + /// The operand's `--preserve-root` verdict must come from the stat the + /// descent is pinned to, whatever the path points at by the time it is asked. + #[cfg(unix)] + #[test] + fn test_meta_is_root_ignores_the_path() { + let temp_dir = tempdir().unwrap(); + let root = std::fs::metadata("/").unwrap(); + let other = std::fs::metadata(temp_dir.path()).unwrap(); + + assert!(meta_is_root(temp_dir.path(), &root)); + assert!(!meta_is_root(Path::new("/"), &other)); + } + #[test] fn test_empty_string() { let path = PathBuf::new(); From ab7438f06670f0b4ab9e63234e1fcf32e0bc7493 Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Tue, 29 Sep 2026 18:53:48 -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. A directory that cannot be read before its mode change, and -H with --no-dereference, still go by path. --- src/uu/chmod/src/chmod.rs | 140 ++++++++++++++++++++++++++++++++------ 1 file changed, 120 insertions(+), 20 deletions(-) diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index d7a49a261b..aa46033208 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)] @@ -46,6 +46,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), + #[error("{}", translate!("perms-cannot-access-replaced", "file" => _0.quote()))] + #[cfg_attr( + any(target_os = "aix", target_os = "hurd", target_os = "redox"), + allow(dead_code) + )] + Replaced(PathBuf), } impl UError for ChmodError {} @@ -545,6 +551,20 @@ impl Chmoder { path_is_root_dir(file, true) } + /// [`Self::is_root`] for a directory already open, which no rename can re-point. + #[cfg(not(any(target_os = "aix", target_os = "hurd", target_os = "redox")))] + fn is_root_fd(dir_fd: &DirFd) -> bool { + dir_fd + .metadata() + .is_ok_and(|meta| uucore::fs::dev_ino_is_root_dir(meta.dev(), meta.ino())) + } + + #[cfg(not(any(target_os = "aix", target_os = "hurd", target_os = "redox")))] + fn same_dir(a: &DirFd, b: &DirFd) -> bool { + FileInformation::from_file(a) + .is_ok_and(|a| FileInformation::from_file(b).is_ok_and(|b| a == b)) + } + /// `--preserve-root` guard re-checked at every descent: a symlink to `/` /// (under `-L`) or a bind mount of `/` met inside the tree is still `/`, so /// the operand-only check is not enough. GNU re-checks every entry too. @@ -642,8 +662,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, @@ -651,22 +669,60 @@ 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() { + if !((!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir()) { + return self.chmod_file(file_path); + } + + // Change the mode through a descriptor, checked not to be "/", rather than by + // pathname: after a rename that lookup could land on something else, "/" + // included once the check above has passed. Under `-P` (the `-R` default) + // O_NOFOLLOW fails the open rather than following a swapped-in symlink; + // `-H`/`-L` follow, which is what they ask for. `-H --no-dereference` asks + // for the mode of what `chmod_file` resolves, and a directory we cannot read + // yet opens only once its mode change allows it: both still go by path, + // neither being what a privileged `chmod -R` meets. + let pinned = if should_follow_symlink == self.dereference { match DirFd::open(file_path, should_follow_symlink.into()) { - Ok(dir_fd) => { - r = self.safe_traverse_dir(&dir_fd, file_path, ancestors).and(r); + Ok(dir_fd) => Some(dir_fd), + Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => None, + Err(err) => return Err(err.into()), + } + } else { + None + }; + let mut r = match &pinned { + Some(dir_fd) => { + if self.preserve_root && Self::is_root_fd(dir_fd) { + 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())); - } + self.chmod_dir_fd(dir_fd, file_path) + } + None => self.chmod_file(file_path), + }; + + // Open again to descend, as GNU does, so the mode just set decides whether + // the directory can be read; it must still be the one changed above. + match DirFd::open(file_path, should_follow_symlink.into()) { + Ok(dir_fd) => { + if pinned + .as_ref() + .is_some_and(|pinned| !Self::same_dir(pinned, &dir_fd)) + { + return r.and(Err(ChmodError::Replaced(file_path.into()).into())); + } + if self.preserve_root && Self::is_root_fd(&dir_fd) { + 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(err.into())); } } } @@ -895,8 +951,35 @@ 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. + #[cfg(not(any(target_os = "aix", target_os = "hurd", target_os = "redox")))] + fn chmod_dir_fd(&self, dir_fd: &DirFd, file: &Path) -> UResult<()> { + let fperm = match dir_fd.metadata() { + Ok(meta) => meta.mode() & 0o7777, + Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => { + return Err(ChmodError::PermissionDenied(file.into()).into()); + } + Err(_) => return Err(ChmodError::CannotStat(file.into()).into()), + }; + 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 @@ -917,11 +1000,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)); } @@ -946,6 +1036,16 @@ impl Chmoder { mod tests { use super::*; + #[cfg(not(any(target_os = "aix", target_os = "hurd", target_os = "redox")))] + #[test] + fn test_is_root_fd() { + let root = DirFd::open(Path::new("/"), SymlinkBehavior::Follow).unwrap(); + assert!(Chmoder::is_root_fd(&root)); + + let cwd = DirFd::open(Path::new("."), SymlinkBehavior::Follow).unwrap(); + assert!(!Chmoder::is_root_fd(&cwd)); + } + #[test] fn test_extract_negative_modes() { // "chmod -w -r file" becomes "chmod -w,-r file". clap does not accept "-w,-r" as MODE.