diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index d7a49a261b4..aa46033208a 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. diff --git a/src/uucore/src/lib/features/fs.rs b/src/uucore/src/lib/features/fs.rs index ee4feab59d8..ee1f6f404de 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 29f5c7aacac..a83ebca97de 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();