mv,cp: fix xattr TOCTOU by using file descriptor-based operations

Closes: #10014

Author: sylvestre

src/uu/cp/src/cp.rs

@@ -16,7 +16,7 @@ use std::os::unix::net::UnixListener;
 use std::path::{Path, PathBuf, StripPrefixError};
 use std::{fmt, io};
 #[cfg(all(unix, not(target_os = "android")))]
-use uucore::fsxattr::copy_xattrs;
+use uucore::fsxattr::copy_xattrs_fd;
 use uucore::translate;
 
 use clap::{Arg, ArgAction, ArgMatches, Command, builder::ValueParser, value_parser};
@@ -1675,8 +1675,13 @@ fn handle_preserve<F: Fn() -> CopyResult<()>>(p: &Preserve, f: F) -> CopyResult<
 /// user-writable if needed and restoring its original permissions afterward. This avoids "Operation
 /// not permitted" errors on read-only files. Returns an error if permission or metadata operations fail,
 /// or if xattr copying fails.
+///
+/// Uses file descriptor-based operations to avoid TOCTOU races during xattr copying.
 #[cfg(all(unix, not(target_os = "android")))]
 fn copy_extended_attrs(source: &Path, dest: &Path) -> CopyResult<()> {
+    use std::fs::File;
+    use uucore::fsxattr::copy_xattrs;
+
     let metadata = fs::symlink_metadata(dest)?;
 
     // Check if the destination file is currently read-only for the user.
@@ -1690,9 +1695,18 @@ fn copy_extended_attrs(source: &Path, dest: &Path) -> CopyResult<()> {
         fs::set_permissions(dest, perms)?;
     }
 
-    // Perform the xattr copy and capture any potential error,
-    // so we can restore permissions before returning.
-    let copy_xattrs_result = copy_xattrs(source, dest);
+    // Use file descriptor-based operations for regular files to avoid TOCTOU races.
+    // For directories and symlinks, we must use path-based operations since:
+    // - Directories cannot be opened with write mode for xattr operations
+    // - Symlinks (especially dangling ones) cannot be opened via File::open
+    let copy_xattrs_result = if metadata.is_file() {
+        // Open both files to pin their inodes, avoiding TOCTOU races during xattr operations
+        let source_file = File::open(source)?;
+        let dest_file = OpenOptions::new().write(true).open(dest)?;
+        copy_xattrs_fd(&source_file, &dest_file)
+    } else {
+        copy_xattrs(source, dest)
+    };
 
     // Restore read-only if we changed it.
     if was_readonly {

src/uu/mv/src/mv.rs

@@ -975,8 +975,14 @@ fn rename_dir_fallback(
         (_, _) => None,
     };
 
+    // Retrieve xattrs using file descriptor to avoid TOCTOU races
     #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
-    let xattrs = fsxattr::retrieve_xattrs(from).unwrap_or_else(|_| HashMap::new());
+    let xattrs = {
+        use std::fs::File;
+        File::open(from)
+            .and_then(|f| fsxattr::retrieve_xattrs_fd(&f))
+            .unwrap_or_else(|_| HashMap::new())
+    };
 
     // Use directory copying (with or without hardlink support)
     let result = copy_dir_contents(
@@ -991,8 +997,14 @@ fn rename_dir_fallback(
         display_manager,
     );
 
+    // Apply xattrs using file descriptor to avoid TOCTOU races
     #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
-    fsxattr::apply_xattrs(to, xattrs)?;
+    {
+        use std::fs::OpenOptions;
+        if let Ok(f) = OpenOptions::new().write(true).open(to) {
+            fsxattr::apply_xattrs_fd(&f, xattrs)?;
+        }
+    }
 
     result?;
 
@@ -1212,12 +1224,19 @@ fn rename_file_fallback(
     Ok(())
 }
 
-/// Copy xattrs from source to destination, ignoring ENOTSUP/EOPNOTSUPP errors.
+/// Copy xattrs from source to destination using file descriptors, ignoring ENOTSUP/EOPNOTSUPP errors.
+/// This version avoids TOCTOU races by operating on file descriptors rather than paths.
 /// These errors indicate the filesystem doesn't support extended attributes,
 /// which is acceptable when moving files across filesystems.
 #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
 fn copy_xattrs_if_supported(from: &Path, to: &Path) -> io::Result<()> {
-    match fsxattr::copy_xattrs(from, to) {
+    use std::fs::{File, OpenOptions};
+
+    // Open both files to pin their inodes, avoiding TOCTOU races during xattr operations
+    let source_file = File::open(from)?;
+    let dest_file = OpenOptions::new().write(true).open(to)?;
+
+    match fsxattr::copy_xattrs_fd(&source_file, &dest_file) {
         Ok(()) => Ok(()),
         Err(e) if e.raw_os_error() == Some(libc::EOPNOTSUPP) => Ok(()),
         Err(e) => Err(e),

src/uucore/src/lib/features/fsxattr.rs

@@ -9,9 +9,11 @@
 use itertools::Itertools;
 use std::collections::HashMap;
 use std::ffi::{OsStr, OsString};
+use std::fs::File;
 #[cfg(unix)]
 use std::os::unix::ffi::OsStrExt;
 use std::path::Path;
+use xattr::FileExt;
 
 /// Copies extended attributes (xattrs) from one file or directory to another.
 ///
@@ -32,6 +34,29 @@ pub fn copy_xattrs<P: AsRef<Path>>(source: P, dest: P) -> std::io::Result<()> {
     Ok(())
 }
 
+/// Copies extended attributes (xattrs) from one file to another using file descriptors.
+///
+/// This version avoids TOCTOU (time-of-check to time-of-use) races by operating
+/// on open file descriptors rather than paths, ensuring all operations target
+/// the same inodes throughout.
+///
+/// # Arguments
+///
+/// * `source` - A reference to the source file (open file descriptor).
+/// * `dest` - A reference to the destination file (open file descriptor).
+///
+/// # Returns
+///
+/// A result indicating success or failure.
+pub fn copy_xattrs_fd(source: &File, dest: &File) -> std::io::Result<()> {
+    for attr_name in source.list_xattr()? {
+        if let Some(value) = source.get_xattr(&attr_name)? {
+            dest.set_xattr(&attr_name, &value)?;
+        }
+    }
+    Ok(())
+}
+
 /// Retrieves the extended attributes (xattrs) of a given file or directory.
 ///
 /// # Arguments
@@ -51,6 +76,28 @@ pub fn retrieve_xattrs<P: AsRef<Path>>(source: P) -> std::io::Result<HashMap<OsS
     Ok(attrs)
 }
 
+/// Retrieves the extended attributes (xattrs) of a given file using a file descriptor.
+///
+/// This version avoids TOCTOU races by operating on an open file descriptor
+/// rather than a path, ensuring all operations target the same inode.
+///
+/// # Arguments
+///
+/// * `source` - A reference to the file (open file descriptor).
+///
+/// # Returns
+///
+/// A result containing a HashMap of attribute names and values, or an error.
+pub fn retrieve_xattrs_fd(source: &File) -> std::io::Result<HashMap<OsString, Vec<u8>>> {
+    let mut attrs = HashMap::new();
+    for attr_name in source.list_xattr()? {
+        if let Some(value) = source.get_xattr(&attr_name)? {
+            attrs.insert(attr_name, value);
+        }
+    }
+    Ok(attrs)
+}
+
 /// Applies extended attributes (xattrs) to a given file or directory.
 ///
 /// # Arguments
@@ -71,6 +118,26 @@ pub fn apply_xattrs<P: AsRef<Path>>(
     Ok(())
 }
 
+/// Applies extended attributes (xattrs) to a given file using a file descriptor.
+///
+/// This version avoids TOCTOU races by operating on an open file descriptor
+/// rather than a path, ensuring all operations target the same inode.
+///
+/// # Arguments
+///
+/// * `dest` - A reference to the file (open file descriptor).
+/// * `xattrs` - A HashMap containing attribute names and their corresponding values.
+///
+/// # Returns
+///
+/// A result indicating success or failure.
+pub fn apply_xattrs_fd(dest: &File, xattrs: HashMap<OsString, Vec<u8>>) -> std::io::Result<()> {
+    for (attr, value) in xattrs {
+        dest.set_xattr(&attr, &value)?;
+    }
+    Ok(())
+}
+
 /// Checks if a file has an Access Control List (ACL) based on its extended attributes.
 ///
 /// # Arguments
@@ -286,4 +353,59 @@ mod tests {
             assert!(has_security_cap_acl(&file_path));
         }
     }
+
+    #[test]
+    fn test_copy_xattrs_fd() {
+        use std::fs::OpenOptions;
+
+        let temp_dir = tempdir().unwrap();
+        let source_path = temp_dir.path().join("source.txt");
+        let dest_path = temp_dir.path().join("dest.txt");
+
+        File::create(&source_path).unwrap();
+        File::create(&dest_path).unwrap();
+
+        let test_attr = "user.test_fd";
+        let test_value = b"test value fd";
+        xattr::set(&source_path, test_attr, test_value).unwrap();
+
+        let source_file = File::open(&source_path).unwrap();
+        let dest_file = OpenOptions::new().write(true).open(&dest_path).unwrap();
+
+        copy_xattrs_fd(&source_file, &dest_file).unwrap();
+
+        let copied_value = xattr::get(&dest_path, test_attr).unwrap().unwrap();
+        assert_eq!(copied_value, test_value);
+    }
+
+    #[test]
+    fn test_apply_and_retrieve_xattrs_fd() {
+        use std::fs::OpenOptions;
+
+        let temp_dir = tempdir().unwrap();
+        let file_path = temp_dir.path().join("test_file.txt");
+
+        File::create(&file_path).unwrap();
+
+        let mut test_xattrs = HashMap::new();
+        let test_attr = "user.test_attr_fd";
+        let test_value = b"test value fd";
+        test_xattrs.insert(OsString::from(test_attr), test_value.to_vec());
+
+        // Apply using file descriptor
+        let file = OpenOptions::new().write(true).open(&file_path).unwrap();
+        apply_xattrs_fd(&file, test_xattrs).unwrap();
+        drop(file);
+
+        // Retrieve using file descriptor
+        let file = File::open(&file_path).unwrap();
+        let retrieved_xattrs = retrieve_xattrs_fd(&file).unwrap();
+        assert!(retrieved_xattrs.contains_key(OsString::from(test_attr).as_os_str()));
+        assert_eq!(
+            retrieved_xattrs
+                .get(OsString::from(test_attr).as_os_str())
+                .unwrap(),
+            test_value
+        );
+    }
 }