diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 337ad4e0043..52b27bcc947 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -17,7 +17,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_acls, copy_xattrs, copy_xattrs_skip_selinux}; +use uucore::fsxattr::{copy_acls, copy_xattrs_fd, copy_xattrs_skip_selinux}; use uucore::translate; use clap::{Arg, ArgAction, ArgMatches, Command, builder::ValueParser, value_parser}; @@ -1737,8 +1737,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. @@ -1758,6 +1762,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 1b92c520eeb..393cc74d19a 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -5,6 +5,7 @@ // spell-checker:ignore (ToDO) sourcepath targetpath nushell canonicalized unwriteable // spell-checker:ignore renameat symlinkat unlinkat unguessability RDONLY CLOEXEC +// spell-checker:ignore renamer fsetxattr mod error; #[cfg(unix)] @@ -1166,8 +1167,15 @@ fn rename_dir_fallback( (_, _) => None, }; + // Retrieve xattrs through a file descriptor so a concurrent renamer cannot + // redirect the list/get calls to a different inode. #[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( @@ -1182,8 +1190,18 @@ fn rename_dir_fallback( display_manager, ); + // Apply xattrs using a file descriptor to avoid TOCTOU races, ignoring + // ENOTSUP/EOPNOTSUPP (filesystem without xattr support, which is expected + // for cross-device moves). + // + // The fd is opened read-only: a directory cannot be opened for writing, and + // fsetxattr checks write permission on the inode, not the open mode. #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] - fsxattr::apply_xattrs(to, xattrs)?; + { + use std::fs::File; + let dest = File::open(to)?; + fsxattr::apply_xattrs_fd_ignore_unsupported(&dest, xattrs)?; + } result?; diff --git a/src/uucore/src/lib/features/fsxattr.rs b/src/uucore/src/lib/features/fsxattr.rs index b42ea91e345..34214b4d96e 100644 --- a/src/uucore/src/lib/features/fsxattr.rs +++ b/src/uucore/src/lib/features/fsxattr.rs @@ -129,6 +129,30 @@ pub fn retrieve_xattrs>(source: P) -> std::io::Result std::io::Result>> { + use xattr::FileExt; + 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 @@ -149,6 +173,43 @@ 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 map of attribute names to their corresponding values. +/// +/// # Returns +/// +/// A result indicating success or failure. +#[cfg(unix)] +pub fn apply_xattrs_fd( + dest: &std::fs::File, + xattrs: FxHashMap>, +) -> std::io::Result<()> { + use xattr::FileExt; + for (attr, value) in xattrs { + dest.set_xattr(&attr, &value)?; + } + Ok(()) +} + +/// Like [`apply_xattrs_fd`], but maps `ENOTSUP` / `EOPNOTSUPP` to `Ok(())`. +#[cfg(unix)] +pub fn apply_xattrs_fd_ignore_unsupported( + dest: &std::fs::File, + xattrs: FxHashMap>, +) -> std::io::Result<()> { + match apply_xattrs_fd(dest, xattrs) { + Err(e) if is_xattr_unsupported(&e) => Ok(()), + res => res, + } +} + /// Checks if a file has an Access Control List (ACL) based on its extended attributes. /// /// # Arguments @@ -393,4 +454,35 @@ mod tests { assert!(has_security_cap_acl(&file_path)); } } + + #[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 + ); + } } diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index 2b58b3c3875..872610564b5 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -3,7 +3,8 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. // -// spell-checker:ignore mydir hardlinked tmpfs notty unwriteable myfolder SRCDATA DSTDATA +// spell-checker:ignore mydir hardlinked tmpfs notty unwriteable myfolder SRCDATA DSTDATA REALDATA +// spell-checker:ignore dirattr dirvalue setfattr getfattr use filetime::FileTime; use rstest::rstest; @@ -3055,6 +3056,64 @@ fn test_mv_xattr_enotsup_silent() { } } +/// Cross-device mv of a directory must preserve the directory's own xattrs. +/// The fd-based xattr path has to open the destination read-only: a directory +/// cannot be opened for writing, so a write-mode open would silently drop them. +#[test] +#[cfg(target_os = "linux")] +fn test_mv_cross_device_dir_xattr_preserved() { + use std::process::Command; + use tempfile::TempDir; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.mkdir("src_dir"); + at.write("src_dir/file.txt", "content"); + + if !Command::new("setfattr") + .args([ + "-n", + "user.dirattr", + "-v", + "dirvalue", + &at.plus_as_string("src_dir"), + ]) + .status() + .is_ok_and(|s| s.success()) + { + println!("test skipped: setfattr failed"); + return; + } + + let other_fs_tempdir = + TempDir::new_in("/dev/shm/").expect("Unable to create temp directory in /dev/shm"); + let dst_path = other_fs_tempdir.path().join("dst_dir"); + + scene + .ucmd() + .arg(at.plus_as_string("src_dir")) + .arg(dst_path.to_str().unwrap()) + .succeeds() + .no_stderr(); + + let out = Command::new("getfattr") + .args([ + "-n", + "user.dirattr", + "--only-values", + dst_path.to_str().unwrap(), + ]) + .output() + .expect("failed to run getfattr on the moved directory"); + assert!( + out.status.success(), + "directory xattr was not preserved across devices: {}", + String::from_utf8_lossy(&out.stderr) + ); + assert_eq!(out.stdout, b"dirvalue"); +} + /// Cross-device mv of a symlink onto an existing file must replace the /// destination atomically, matching GNU. #[test]