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, copy_xattrs_skip_selinux};
+use uucore::fsxattr::{copy_xattrs_fd, copy_xattrs_skip_selinux};
 use uucore::translate;
 
 use clap::{Arg, ArgAction, ArgMatches, Command, builder::ValueParser, value_parser};
@@ -1715,6 +1715,8 @@ pub(crate) fn set_selinux_context(path: &Path, context: Option<&String>) -> Copy
 /// or if xattr copying fails.
 #[cfg(all(unix, not(target_os = "android")))]
 fn copy_extended_attrs(source: &Path, dest: &Path, skip_selinux: bool) -> 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.
@@ -1734,6 +1736,13 @@ fn copy_extended_attrs(source: &Path, dest: &Path, skip_selinux: bool) -> CopyRe
         // When -Z is used, skip copying security.selinux xattr so that
         // the default context can be set instead of preserving from source
         copy_xattrs_skip_selinux(source, dest)
+    } else if metadata.is_file() {
+        // Use file descriptor-based operations for regular files to avoid TOCTOU races.
+        // Directories cannot be opened with write mode for xattr operations
+        // Symlinks (especially dangling ones) cannot be opened via File::open
+        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)
     };

src/uu/mv/src/mv.rs

@@ -967,8 +967,15 @@ fn rename_dir_fallback(
         (_, _) => None,
     };
 
+    // Retrieve xattrs before copying (directories use path-based operations
+    // since they cannot be opened in write mode for xattr operations)
     #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
-    let xattrs = fsxattr::retrieve_xattrs(from).unwrap_or_else(|_| FxHashMap::default());
+    let xattrs = {
+        use std::fs::File;
+        File::open(from)
+            .and_then(|f| fsxattr::retrieve_xattrs_fd(&f))
+            .unwrap_or_else(|_| FxHashMap::default())
+    };
 
     // Use directory copying (with or without hardlink support)
     let result = copy_dir_contents(
@@ -983,8 +990,14 @@ fn rename_dir_fallback(
         display_manager,
     );
 
+    // Apply xattrs after directory contents are copied, ignoring ENOTSUP errors
+    // (filesystem doesn't support xattrs, which is acceptable for cross-device moves)
     #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
-    fsxattr::apply_xattrs(to, xattrs)?;
+    if let Err(e) = fsxattr::apply_xattrs(to, xattrs) {
+        if e.raw_os_error() != Some(libc::EOPNOTSUPP) {
+            return Err(e);
+        }
+    }
 
     result?;
 
@@ -1082,7 +1095,16 @@ fn copy_dir_contents_recursive(
                 rename_symlink_fallback(&from_path, &to_path)?;
             }
 
-            print_verbose(&from_path, &to_path);
+            // Print verbose message for symlink
+            if verbose {
+                let message = translate!("mv-verbose-renamed", "from" => from_path.quote(), "to" => to_path.quote());
+                match display_manager {
+                    Some(pb) => pb.suspend(|| {
+                        println!("{message}");
+                    }),
+                    None => println!("{message}"),
+                }
+            }
         } else if from_path.is_dir() {
             // Recursively copy subdirectory (only real directories, not symlinks)
             fs::create_dir_all(&to_path)?;
@@ -1215,12 +1237,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 rustc_hash::FxHashMap;
 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.
 ///
@@ -45,6 +47,29 @@ pub fn copy_xattrs_skip_selinux<P: AsRef<Path>>(source: P, dest: P) -> std::io::
     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
@@ -64,6 +89,28 @@ pub fn retrieve_xattrs<P: AsRef<Path>>(source: P) -> std::io::Result<FxHashMap<O
     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<FxHashMap<OsString, Vec<u8>>> {
+    let mut attrs = FxHashMap::default();
+    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
@@ -84,6 +131,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: FxHashMap<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
@@ -299,4 +366,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 = FxHashMap::default();
+        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
+        );
+    }
 }

tests/by-util/test_cp.rs

@@ -7856,3 +7856,75 @@ fn test_cp_recursive_non_utf8_source() {
 
     assert!(at.plus("dir2").join("a").exists());
 }
+
+#[test]
+#[cfg(all(
+    unix,
+    not(any(target_os = "android", target_os = "macos", target_os = "openbsd"))
+))]
+fn test_cp_xattr_implicit_vs_explicit() {
+    // Test the difference between implicit (-a) and explicit (--preserve=xattr) xattr handling
+    use std::process::Command;
+
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let source_file = "source.txt";
+    at.write(source_file, "test content");
+
+    // Try to set an xattr on the source file
+    let xattr_key = "user.test";
+    let xattr_value = "test_value";
+
+    let xattr_set = Command::new("setfattr")
+        .args([
+            "-n",
+            xattr_key,
+            "-v",
+            xattr_value,
+            &at.plus_as_string(source_file),
+        ])
+        .output()
+        .map(|o| o.status.success())
+        .unwrap_or(false);
+
+    if !xattr_set {
+        eprintln!("Skipping test: Cannot set xattrs");
+        return;
+    }
+
+    // Test 1: cp -a (implicit xattr preservation - should never fail)
+    scene
+        .ucmd()
+        .args(&["-a", source_file, "dest_implicit.txt"])
+        .succeeds()
+        .no_stderr(); // Should not produce errors for implicit preservation
+
+    assert!(at.file_exists("dest_implicit.txt"));
+
+    // Test 2: cp --preserve=xattr (explicit - may fail if filesystem doesn't support)
+    // This should succeed on normal filesystems but the behavior differs from -a
+    // in that explicit --preserve=xattr is "required" while -a is "best effort"
+    let result = scene
+        .ucmd()
+        .args(&["--preserve=xattr", source_file, "dest_explicit.txt"])
+        .run();
+
+    // The file should be created either way
+    if at.file_exists("dest_explicit.txt") {
+        // Check if xattrs were actually preserved
+        let getfattr_check = Command::new("getfattr")
+            .args(["-n", xattr_key, &at.plus_as_string("dest_explicit.txt")])
+            .output()
+            .unwrap();
+
+        if getfattr_check.status.success() {
+            // xattrs were preserved successfully
+            assert!(
+                result.succeeded(),
+                "cp --preserve=xattr should succeed when xattrs can be preserved"
+            );
+        }
+        // If xattrs weren't preserved, the behavior depends on the filesystem
+    }
+}