chown, chgrp: change a directory operand that cannot be read

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<P: AsRef<Path>>(
     dest_gid: Option<u32>,
     follow: bool,
     verbosity: Verbosity,
+) -> Result<String, String> {
+    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<u32>,
+    dest_gid: Option<u32>,
+    verbosity: Verbosity,
+    change: impl FnOnce(uid_t, gid_t) -> IOResult<()>,
 ) -> Result<String, String> {
     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<P: AsRef<Path>>(
                     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<P: AsRef<Path>>(
     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),
@@ -355,16 +387,34 @@ 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.
+                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)),
+                    ),
+                    // 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,
@@ -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<PinnedFile>,
+    ) -> IOResult<Option<PinnedFile>> {
+        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<bool> {
+        // 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<P: AsRef<Path>>(
@@ -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();

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> {
+        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<FileStat> {
         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> {
+        Self::open_at(nix::fcntl::AT_FDCWD, path, symlink_behavior)
+    }
+
+    fn open_at<P: ?Sized + nix::NixPath>(
+        dir: impl AsFd,
+        path: &P,
+        symlink_behavior: SymlinkBehavior,
+    ) -> io::Result<Self> {
+        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<Metadata> {
+        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<u32>, gid: Option<u32>) -> 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 {