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)
     };
@@ -2591,7 +2600,40 @@ 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 need to preserve ownership but can't re-access the source as it may block.
+        // Use the metadata we already have.
+        #[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);
+                }
+            }
+        }
+        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 +2647,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,14 +6,13 @@
 
 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::translate;
 
 use crate::{
@@ -50,6 +49,25 @@ enum CopyMethod {
     SparseCopyWithoutHole,
 }
 
+/// Copy file contents with restrictive permissions initially to prevent race conditions.
+/// Creates the destination file with mode 0600 & !umask, copies the content,
+/// and lets the caller set the final permissions.
+fn copy_file_with_secure_permissions<P>(source: P, dest: P) -> io::Result<u64>
+where
+    P: AsRef<Path>,
+{
+    let mut src_file = File::open(&source)?;
+    // Create destination with restrictive permissions initially (to prevent race conditions)
+    // Mode 0o600 means read/write for owner only
+    let mut dst_file = OpenOptions::new()
+        .create(true)
+        .write(true)
+        .truncate(true)
+        .mode(0o600)
+        .open(&dest)?;
+    io::copy(&mut src_file, &mut dst_file)
+}
+
 /// Use the Linux `ioctl_ficlone` API to do a copy-on-write clone.
 ///
 /// `fallback` controls what to do if the system call fails.
@@ -59,7 +77,13 @@ 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)
+        .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) };
@@ -68,7 +92,7 @@ where
     }
     match fallback {
         CloneFallback::Error => Err(std::io::Error::last_os_error()),
-        CloneFallback::FSCopy => std::fs::copy(source, dest).map(|_| ()),
+        CloneFallback::FSCopy => copy_file_with_secure_permissions(source, dest).map(|_| ()),
         CloneFallback::SparseCopy => sparse_copy(source, dest),
         CloneFallback::SparseCopyWithoutHole => sparse_copy_without_hole(source, dest),
     }
@@ -125,7 +149,13 @@ 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)
+        .mode(0o600)
+        .open(dest)?;
     let dst_fd = dst_file.as_raw_fd();
 
     let size = src_file.metadata()?.size();
@@ -175,7 +205,13 @@ 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)
+        .mode(0o600)
+        .open(dest)?;
     let dst_fd = dst_file.as_raw_fd();
 
     let size: usize = src_file.metadata()?.size().try_into().unwrap();
@@ -237,18 +273,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)?;
-    }
+    // No need to set_len(0) since we're already using truncate(true)
 
     let num_bytes_copied = buf_copy::copy_stream(&mut src_file, &mut dst_file)
         .map_err(|e| std::io::Error::other(format!("{e}")))?;
@@ -287,7 +323,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 +341,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 +360,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(|_| ()),
                 }
             }
         }

src/uu/cp/src/platform/macos.rs

@@ -5,6 +5,7 @@
 // spell-checker:ignore reflink
 use std::ffi::CString;
 use std::fs::{self, File, OpenOptions};
+use std::io;
 use std::os::unix::ffi::OsStrExt;
 use std::os::unix::fs::OpenOptionsExt;
 use std::path::Path;
@@ -13,13 +14,30 @@ use uucore::buf_copy;
 use uucore::display::Quotable;
 use uucore::translate;
 
-use uucore::mode::get_umask;
-
 use crate::{
     CopyDebug, CopyResult, CpError, OffloadReflinkDebug, ReflinkMode, SparseDebug, SparseMode,
     is_stream,
 };
 
+/// Copy file contents with restrictive permissions initially to prevent race conditions.
+/// Creates the destination file with mode 0600 & !umask, copies the content,
+/// and lets the caller set the final permissions.
+fn copy_file_with_secure_permissions<P>(source: P, dest: P) -> io::Result<u64>
+where
+    P: AsRef<Path>,
+{
+    let mut src_file = File::open(&source)?;
+    // Create destination with restrictive permissions initially (to prevent race conditions)
+    // Mode 0o600 means read/write for owner only
+    let mut dst_file = OpenOptions::new()
+        .create(true)
+        .write(true)
+        .truncate(true)
+        .mode(0o600)
+        .open(&dest)?;
+    io::copy(&mut src_file, &mut dst_file)
+}
+
 /// Copies `source` to `dest` using copy-on-write if possible.
 pub(crate) fn copy_on_write(
     source: &Path,
@@ -92,24 +110,24 @@ pub(crate) fn copy_on_write(
         copy_debug.reflink = OffloadReflinkDebug::Yes;
         if source_is_stream {
             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
             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)?;
-            }
+            // No need to set_len(0) since we're already using truncate(true)
 
             buf_copy::copy_stream(&mut src_file, &mut dst_file)
                 .map_err(|_| std::io::Error::from(std::io::ErrorKind::Other))
                 .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?;
         } else {
-            fs::copy(source, dest).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?;
+            copy_file_with_secure_permissions(source, dest)
+                .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?;
         }
     }
 

src/uu/cp/src/platform/other_unix.rs

@@ -4,18 +4,37 @@
 // file that was distributed with this source code.
 // spell-checker:ignore reflink
 use std::fs::{self, File, OpenOptions};
+use std::io;
 use std::os::unix::fs::OpenOptionsExt;
 use std::path::Path;
 
 use uucore::buf_copy;
-use uucore::mode::get_umask;
 use uucore::translate;
 
 use crate::{
     CopyDebug, CopyResult, CpError, OffloadReflinkDebug, ReflinkMode, SparseDebug, SparseMode,
     is_stream,
 };
 
+/// Copy file contents with restrictive permissions initially to prevent race conditions.
+/// Creates the destination file with mode 0600 & !umask, copies the content,
+/// and lets the caller set the final permissions.
+fn copy_file_with_secure_permissions<P>(source: P, dest: P) -> io::Result<u64>
+where
+    P: AsRef<Path>,
+{
+    let mut src_file = File::open(&source)?;
+    // Create destination with restrictive permissions initially (to prevent race conditions)
+    // Mode 0o600 means read/write for owner only
+    let mut dst_file = OpenOptions::new()
+        .create(true)
+        .write(true)
+        .truncate(true)
+        .mode(0o600)
+        .open(&dest)?;
+    io::copy(&mut src_file, &mut dst_file)
+}
+
 /// Copies `source` to `dest` for systems without copy-on-write
 pub(crate) fn copy_on_write(
     source: &Path,
@@ -43,18 +62,17 @@ pub(crate) fn copy_on_write(
 
     if source_is_stream {
         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
         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)?;
-        }
+        // No need to set_len(0) since we're already using truncate(true)
 
         buf_copy::copy_stream(&mut src_file, &mut dst_file)
             .map_err(|_| std::io::Error::from(std::io::ErrorKind::Other))
@@ -63,7 +81,8 @@ pub(crate) fn copy_on_write(
         return Ok(copy_debug);
     }
 
-    fs::copy(source, dest).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?;
+    copy_file_with_secure_permissions(source, dest)
+        .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?;
 
     Ok(copy_debug)
 }