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

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)
     };
@@ -2591,7 +2600,45 @@ fn copy_file(
         fs::set_permissions(dest, dest_permissions).ok();
     }
 
-    let copy_attributes_result = if options.dereference(source_in_command_line) {
+    let copy_attributes_result = if source_is_stream && options.copy_contents {
+        // When copying contents from a stream (like a FIFO with --copy-contents),
+        // we can't re-access the source as it may block. Use the metadata we
+        // already have to preserve ownership and permissions.
+        #[cfg(unix)]
+        {
+            if let Preserve::Yes { .. } = options.attributes.ownership {
+                use std::os::unix::prelude::MetadataExt;
+                use uucore::perms::{Verbosity, VerbosityLevel, wrap_chown};
+
+                let dest_uid = source_metadata.uid();
+                let dest_gid = source_metadata.gid();
+                if let Ok(dest_meta) = dest.symlink_metadata() {
+                    let try_chown = |uid| {
+                        wrap_chown(
+                            dest,
+                            &dest_meta,
+                            uid,
+                            Some(dest_gid),
+                            false,
+                            Verbosity {
+                                groups_only: false,
+                                level: VerbosityLevel::Silent,
+                            },
+                        )
+                    };
+                    // gnu compatibility: cp doesn't report an error if it fails to set the
+                    // ownership, and will fall back to changing only the gid if possible.
+                    if try_chown(Some(dest_uid)).is_err() {
+                        let _ = try_chown(None);
+                    }
+                }
+            }
+            if let Preserve::Yes { .. } = options.attributes.mode {
+                fs::set_permissions(dest, source_metadata.permissions()).ok();
+            }
+        }
+        Ok(())
+    } else if options.dereference(source_in_command_line) {
         // Try to canonicalize, but if it fails (e.g., due to inaccessible parent directories),
         // fall back to the original source path
         let src_for_attrs = canonicalize(source, MissingHandling::Normal, ResolveMode::Physical)
@@ -2605,12 +2652,6 @@ fn copy_file(
             false,
             options.set_selinux_context,
         )
-    } else if source_is_stream && !source.exists() {
-        // Some stream files may not exist after we have copied it,
-        // like anonymous pipes. Thus, we can't really copy its
-        // attributes. However, this is already handled in the stream
-        // copy function (see `copy_stream` under platform/linux.rs).
-        Ok(())
     } else {
         copy_attributes(
             source,

src/uu/cp/src/platform/linux.rs

@@ -6,19 +6,18 @@
 
 use libc::{SEEK_DATA, SEEK_HOLE};
 use std::fs::{File, OpenOptions};
-use std::io::Read;
+use std::io::{self, Read};
 use std::os::unix::fs::FileExt;
 use std::os::unix::fs::MetadataExt;
 use std::os::unix::fs::{FileTypeExt, OpenOptionsExt};
 use std::os::unix::io::AsRawFd;
 use std::path::Path;
 use uucore::buf_copy;
-use uucore::mode::get_umask;
+use uucore::fs::copy_file_with_secure_permissions;
 use uucore::translate;
 
 use crate::{
     CopyDebug, CopyResult, CpError, OffloadReflinkDebug, ReflinkMode, SparseDebug, SparseMode,
-    is_stream,
 };
 
 /// The fallback behavior for [`clone`] on failed system call.
@@ -54,21 +53,41 @@ enum CopyMethod {
 ///
 /// `fallback` controls what to do if the system call fails.
 #[cfg(any(target_os = "linux", target_os = "android"))]
-fn clone<P>(source: P, dest: P, fallback: CloneFallback) -> std::io::Result<()>
+fn clone<P>(source: P, dest: P, fallback: CloneFallback) -> io::Result<()>
 where
     P: AsRef<Path>,
 {
     let src_file = File::open(&source)?;
-    let dst_file = File::create(&dest)?;
+    // Create destination with restrictive permissions initially (to prevent race conditions)
+    // Mode 0o600 means read/write for owner only
+    let dst_file = OpenOptions::new()
+        .create(true)
+        .write(true)
+        .truncate(false)
+        .mode(0o600)
+        .open(&dest)?;
     let src_fd = src_file.as_raw_fd();
     let dst_fd = dst_file.as_raw_fd();
     let result = unsafe { libc::ioctl(dst_fd, libc::FICLONE, src_fd) };
     if result == 0 {
         return Ok(());
     }
+    let err = io::Error::last_os_error();
     match fallback {
-        CloneFallback::Error => Err(std::io::Error::last_os_error()),
-        CloneFallback::FSCopy => std::fs::copy(source, dest).map(|_| ()),
+        CloneFallback::Error => {
+            // FICLONE fails with EINVAL if the destination is not empty.
+            // Truncate and retry once before giving up.
+            if err.raw_os_error() == Some(libc::EINVAL) {
+                dst_file.set_len(0)?;
+                let retry = unsafe { libc::ioctl(dst_fd, libc::FICLONE, src_fd) };
+                if retry == 0 {
+                    return Ok(());
+                }
+                return Err(io::Error::last_os_error());
+            }
+            Err(err)
+        }
+        CloneFallback::FSCopy => copy_file_with_secure_permissions(source, dest).map(|_| ()),
         CloneFallback::SparseCopy => sparse_copy(source, dest),
         CloneFallback::SparseCopyWithoutHole => sparse_copy_without_hole(source, dest),
     }
@@ -78,7 +97,7 @@ where
 /// This function returns a tuple of (bool, u64, u64) signifying a tuple of (whether a file has
 /// data, its size, no of blocks it has allocated in disk)
 #[cfg(any(target_os = "linux", target_os = "android"))]
-fn check_for_data(source: &Path) -> Result<(bool, u64, u64), std::io::Error> {
+fn check_for_data(source: &Path) -> Result<(bool, u64, u64), io::Error> {
     let mut src_file = File::open(source)?;
     let metadata = src_file.metadata()?;
 
@@ -98,14 +117,14 @@ fn check_for_data(source: &Path) -> Result<(bool, u64, u64), std::io::Error> {
     match result {
         -1 => Ok((false, size, blocks)), // No data found or end of file
         _ if result >= 0 => Ok((true, size, blocks)), // Data found
-        _ => Err(std::io::Error::last_os_error()),
+        _ => Err(io::Error::last_os_error()),
     }
 }
 
 #[cfg(any(target_os = "linux", target_os = "android"))]
 /// Checks whether a file is sparse i.e. it contains holes, uses the crude heuristic blocks < size / 512
 /// Reference:`<https://doc.rust-lang.org/std/os/unix/fs/trait.MetadataExt.html#tymethod.blocks>`
-fn check_sparse_detection(source: &Path) -> Result<bool, std::io::Error> {
+fn check_sparse_detection(source: &Path) -> Result<bool, io::Error> {
     let src_file = File::open(source)?;
     let metadata = src_file.metadata()?;
     let size = metadata.size();
@@ -120,17 +139,24 @@ fn check_sparse_detection(source: &Path) -> Result<bool, std::io::Error> {
 /// Optimized [`sparse_copy`] doesn't create holes for large sequences of zeros in non `sparse_files`
 /// Used when `--sparse=auto`
 #[cfg(any(target_os = "linux", target_os = "android"))]
-fn sparse_copy_without_hole<P>(source: P, dest: P) -> std::io::Result<()>
+fn sparse_copy_without_hole<P>(source: P, dest: P) -> io::Result<()>
 where
     P: AsRef<Path>,
 {
     let src_file = File::open(source)?;
-    let dst_file = File::create(dest)?;
+    // Create destination with restrictive permissions initially (to prevent race conditions)
+    // Mode 0o600 means read/write for owner only
+    let dst_file = OpenOptions::new()
+        .create(true)
+        .write(true)
+        .truncate(true)
+        .mode(0o600)
+        .open(dest)?;
     let dst_fd = dst_file.as_raw_fd();
 
     let size = src_file.metadata()?.size();
     if unsafe { libc::ftruncate(dst_fd, size.try_into().unwrap()) } < 0 {
-        return Err(std::io::Error::last_os_error());
+        return Err(io::Error::last_os_error());
     }
     let src_fd = src_file.as_raw_fd();
     let mut current_offset: isize = 0;
@@ -152,7 +178,7 @@ where
             break;
         }
         if result <= -2 || hole <= -2 {
-            return Err(std::io::Error::last_os_error());
+            return Err(io::Error::last_os_error());
         }
         let len: isize = hole - current_offset;
         // Read and write data in chunks of `step` while reusing the same buffer
@@ -170,17 +196,24 @@ where
 /// Perform a sparse copy from one file to another.
 /// Creates a holes for large sequences of zeros in `non_sparse_files`, used for `--sparse=always`
 #[cfg(any(target_os = "linux", target_os = "android"))]
-fn sparse_copy<P>(source: P, dest: P) -> std::io::Result<()>
+fn sparse_copy<P>(source: P, dest: P) -> io::Result<()>
 where
     P: AsRef<Path>,
 {
     let mut src_file = File::open(source)?;
-    let dst_file = File::create(dest)?;
+    // Create destination with restrictive permissions initially (to prevent race conditions)
+    // Mode 0o600 means read/write for owner only
+    let dst_file = OpenOptions::new()
+        .create(true)
+        .write(true)
+        .truncate(true)
+        .mode(0o600)
+        .open(dest)?;
     let dst_fd = dst_file.as_raw_fd();
 
     let size: usize = src_file.metadata()?.size().try_into().unwrap();
     if unsafe { libc::ftruncate(dst_fd, size.try_into().unwrap()) } < 0 {
-        return Err(std::io::Error::last_os_error());
+        return Err(io::Error::last_os_error());
     }
 
     let blksize = dst_file.metadata()?.blksize();
@@ -214,7 +247,7 @@ fn check_dest_is_fifo(dest: &Path) -> bool {
 }
 
 /// Copy the contents of a stream from `source` to `dest`.
-fn copy_stream<P>(source: P, dest: P) -> std::io::Result<u64>
+fn copy_stream<P>(source: P, dest: P) -> io::Result<u64>
 where
     P: AsRef<Path>,
 {
@@ -237,21 +270,18 @@ where
     // TODO Update the code below to respect the case where
     // `--preserve=ownership` is not true.
     let mut src_file = File::open(&source)?;
-    let mode = 0o622 & !get_umask();
+    // Create with restrictive permissions initially to prevent race conditions
+    // Mode 0o600 means read/write for owner only
+    // Use truncate(true) to ensure we overwrite existing content
     let mut dst_file = OpenOptions::new()
         .create(true)
         .write(true)
-        .mode(mode)
+        .truncate(true)
+        .mode(0o600)
         .open(&dest)?;
 
-    let dest_is_stream = is_stream(&dst_file.metadata()?);
-    if !dest_is_stream {
-        // `copy_stream` doesn't clear the dest file, if dest is not a stream, we should clear it manually.
-        dst_file.set_len(0)?;
-    }
-
     let num_bytes_copied = buf_copy::copy_stream(&mut src_file, &mut dst_file)
-        .map_err(|e| std::io::Error::other(format!("{e}")))?;
+        .map_err(|e| io::Error::other(format!("{e}")))?;
 
     Ok(num_bytes_copied)
 }
@@ -287,7 +317,9 @@ pub(crate) fn copy_on_write(
                 }
 
                 match copy_method {
-                    CopyMethod::FSCopy => std::fs::copy(source, dest).map(|_| ()),
+                    CopyMethod::FSCopy => {
+                        copy_file_with_secure_permissions(source, dest).map(|_| ())
+                    }
                     _ => sparse_copy(source, dest),
                 }
             }
@@ -303,7 +335,7 @@ pub(crate) fn copy_on_write(
                 if let Ok(debug) = result {
                     copy_debug = debug;
                 }
-                std::fs::copy(source, dest).map(|_| ())
+                copy_file_with_secure_permissions(source, dest).map(|_| ())
             }
         }
         (ReflinkMode::Never, SparseMode::Auto) => {
@@ -322,7 +354,7 @@ pub(crate) fn copy_on_write(
 
                 match copy_method {
                     CopyMethod::SparseCopyWithoutHole => sparse_copy_without_hole(source, dest),
-                    _ => std::fs::copy(source, dest).map(|_| ()),
+                    _ => copy_file_with_secure_permissions(source, dest).map(|_| ()),
                 }
             }
         }
@@ -401,7 +433,7 @@ pub(crate) fn copy_on_write(
 fn handle_reflink_auto_sparse_always(
     source: &Path,
     dest: &Path,
-) -> Result<(CopyDebug, CopyMethod), std::io::Error> {
+) -> Result<(CopyDebug, CopyMethod), io::Error> {
     let mut copy_debug = CopyDebug {
         offload: OffloadReflinkDebug::Unknown,
         reflink: OffloadReflinkDebug::Unsupported,
@@ -438,7 +470,7 @@ fn handle_reflink_auto_sparse_always(
 
 /// Handles debug results when flags are "--reflink=auto" and "--sparse=auto" and specifies what
 /// type of copy should be used
-fn handle_reflink_never_sparse_never(source: &Path) -> Result<CopyDebug, std::io::Error> {
+fn handle_reflink_never_sparse_never(source: &Path) -> Result<CopyDebug, io::Error> {
     let mut copy_debug = CopyDebug {
         offload: OffloadReflinkDebug::Unknown,
         reflink: OffloadReflinkDebug::No,
@@ -459,7 +491,7 @@ fn handle_reflink_never_sparse_never(source: &Path) -> Result<CopyDebug, std::io
 
 /// Handles debug results when flags are "--reflink=auto" and "--sparse=never", files will be copied
 /// through cloning them with fallback switching to [`std::fs::copy`]
-fn handle_reflink_auto_sparse_never(source: &Path) -> Result<CopyDebug, std::io::Error> {
+fn handle_reflink_auto_sparse_never(source: &Path) -> Result<CopyDebug, io::Error> {
     let mut copy_debug = CopyDebug {
         offload: OffloadReflinkDebug::Unknown,
         reflink: OffloadReflinkDebug::No,
@@ -484,7 +516,7 @@ fn handle_reflink_auto_sparse_never(source: &Path) -> Result<CopyDebug, std::io:
 fn handle_reflink_auto_sparse_auto(
     source: &Path,
     dest: &Path,
-) -> Result<(CopyDebug, CopyMethod), std::io::Error> {
+) -> Result<(CopyDebug, CopyMethod), io::Error> {
     let mut copy_debug = CopyDebug {
         offload: OffloadReflinkDebug::Unknown,
         reflink: OffloadReflinkDebug::Unsupported,
@@ -527,7 +559,7 @@ fn handle_reflink_auto_sparse_auto(
 fn handle_reflink_never_sparse_auto(
     source: &Path,
     dest: &Path,
-) -> Result<(CopyDebug, CopyMethod), std::io::Error> {
+) -> Result<(CopyDebug, CopyMethod), io::Error> {
     let mut copy_debug = CopyDebug {
         offload: OffloadReflinkDebug::Unknown,
         reflink: OffloadReflinkDebug::No,
@@ -563,7 +595,7 @@ fn handle_reflink_never_sparse_auto(
 fn handle_reflink_never_sparse_always(
     source: &Path,
     dest: &Path,
-) -> Result<(CopyDebug, CopyMethod), std::io::Error> {
+) -> Result<(CopyDebug, CopyMethod), io::Error> {
     let mut copy_debug = CopyDebug {
         offload: OffloadReflinkDebug::Unknown,
         reflink: OffloadReflinkDebug::No,