chmod, chown: check --preserve-root on the directory actually descended into

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,31 +662,67 @@ 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,
             TraverseSymlinks::First => is_command_line_arg, // Only follow symlinks that are command line args
             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.

src/uucore/src/lib/features/fs.rs

@@ -631,6 +631,19 @@ pub fn path_is_root_dir<P: AsRef<Path>>(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()));
+    }
 }

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();