From 41cd49fc1b74ce2d9490b9598dcde6baa3a86eb5 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Wed, 28 Jan 2026 22:04:23 +0100 Subject: [PATCH] mv,cp: fix xattr TOCTOU by using file descriptor-based operations Closes: #10014 --- src/uu/cp/src/cp.rs | 13 ++- src/uu/mv/src/mv.rs | 27 +++++- src/uucore/src/lib/features/fsxattr.rs | 122 +++++++++++++++++++++++++ 3 files changed, 157 insertions(+), 5 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index fa6ddcfc6..b7fbe79cd 100644 --- a/src/uu/cp/src/cp.rs +++ b/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}; @@ -1713,8 +1713,12 @@ pub(crate) fn set_selinux_context(path: &Path, context: Option<&String>) -> Copy /// 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, 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 +1738,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) }; diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 517e70a4f..f73ffee34 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -967,8 +967,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(|_| 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 +989,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?; @@ -1215,12 +1227,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), diff --git a/src/uucore/src/lib/features/fsxattr.rs b/src/uucore/src/lib/features/fsxattr.rs index d96832646..e2fc76d9c 100644 --- a/src/uucore/src/lib/features/fsxattr.rs +++ b/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>(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>(source: P) -> std::io::Result std::io::Result>> { + 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>( 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>) -> 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 + ); + } }