cp: open source and dest with O_NOFOLLOW in no-dereference mode (#10017)

In `-P` / no-dereference mode, cp now opens the source file with
`O_NOFOLLOW`, matching GNU cp. This closes a TOCTOU window where an
attacker who can swap the source path between cp's `lstat` check and
the subsequent open could redirect the read through a symlink to a
sensitive file (e.g. /etc/shadow). With `O_NOFOLLOW` the open fails
with `ELOOP` instead.

The same flag is propagated to `safe_copy::create_dest_restrictive`,
so the destination open also refuses to follow a symlink in
no-dereference mode. Without that, an attacker who plants the dest
path as a symlink between the caller's check and the open could
redirect the truncate (and the subsequent write) to any file the
caller has permission to write — the symmetric attack to the source
side. With `nofollow=true` the dest open returns `ELOOP` and the
victim file is left untouched.

`copy_on_write` gains a `nofollow` parameter threaded from
`copy_helper`, set to `!options.dereference(source_in_command_line)`.
In deref mode the flag is false and behavior is unchanged — cp still
follows symlinks, matching GNU.

Extends `util/check-safe-traversal.sh` with a cp -P strace check so
the invariant is locked in: future changes that drop `O_NOFOLLOW`
here will fail the smoke test.

src/uu/cp/src/cp.rs

@@ -2274,6 +2274,7 @@ fn handle_copy_mode(
                 context,
                 source_metadata,
                 symlinked_files,
+                source_in_command_line,
                 created_parent_dirs,
             )?;
         }
@@ -2294,6 +2295,7 @@ fn handle_copy_mode(
                             context,
                             source_metadata,
                             symlinked_files,
+                            source_in_command_line,
                             created_parent_dirs,
                         )?;
                     }
@@ -2327,6 +2329,7 @@ fn handle_copy_mode(
                             context,
                             source_metadata,
                             symlinked_files,
+                            source_in_command_line,
                             created_parent_dirs,
                         )?;
                     }
@@ -2339,6 +2342,7 @@ fn handle_copy_mode(
                     context,
                     source_metadata,
                     symlinked_files,
+                    source_in_command_line,
                     created_parent_dirs,
                 )?;
             }
@@ -2716,6 +2720,7 @@ fn handle_no_preserve_mode(options: &Options, org_mode: u32) -> u32 {
 
 /// Copy the file from `source` to `dest` either using the normal `fs::copy` or a
 /// copy-on-write scheme if --reflink is specified and the filesystem supports it.
+#[allow(clippy::too_many_arguments)]
 fn copy_helper(
     source: &Path,
     dest: &Path,
@@ -2723,6 +2728,7 @@ fn copy_helper(
     context: &str,
     source_metadata: &Metadata,
     symlinked_files: &mut HashSet<FileInformation>,
+    #[cfg_attr(not(unix), allow(unused_variables))] source_in_command_line: bool,
     created_parent_dirs: &mut HashSet<PathBuf>,
 ) -> CopyResult<()> {
     if options.parents {
@@ -2753,6 +2759,14 @@ fn copy_helper(
     if source_metadata.is_symlink() {
         copy_link(source, dest, symlinked_files, options)?;
     } else {
+        // Use O_NOFOLLOW on the source open iff cp is in no-dereference mode.
+        // In that case source_metadata was obtained via lstat, so a path swap
+        // to a symlink between lstat and open must be refused to close the
+        // TOCTOU window described in issue #10017. In deref mode cp
+        // intentionally follows symlinks, matching GNU cp's behavior of
+        // applying O_NOFOLLOW here only with `-P`.
+        #[cfg(unix)]
+        let nofollow = !options.dereference(source_in_command_line);
         let copy_debug = copy_on_write(
             source,
             dest,
@@ -2761,6 +2775,8 @@ fn copy_helper(
             context,
             #[cfg(unix)]
             is_stream(source_metadata),
+            #[cfg(unix)]
+            nofollow,
         )?;
 
         if !options.attributes_only && options.debug {

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

@@ -2,17 +2,17 @@
 //
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
-// spell-checker:ignore ficlone reflink ftruncate pwrite fiemap lseek
+// spell-checker:ignore ficlone reflink ftruncate pwrite fiemap lseek nofollow
 
 use rustix::fs::{SeekFrom, ftruncate, ioctl_ficlone, seek};
-use std::fs::File;
 use std::io::Read;
 use std::os::unix::fs::FileExt;
 use std::os::unix::fs::FileTypeExt;
 use std::os::unix::fs::MetadataExt;
 use std::path::Path;
+
 use uucore::buf_copy;
-use uucore::safe_copy::{create_dest_restrictive, safe_copy_file};
+use uucore::safe_copy::{create_dest_restrictive, open_source};
 use uucore::translate;
 
 use crate::{
@@ -20,6 +20,20 @@ use crate::{
     is_stream,
 };
 
+// Replacement for `std::fs::copy` that uses the safe-copy primitives but
+// only applies `O_NOFOLLOW` to the *source* open. The destination is
+// followed if it is a pre-existing symlink, matching GNU cp -d/-P which
+// only forbid dereferencing on the source side.
+fn fs_copy<P, Q>(source: P, dest: Q, source_nofollow: bool) -> std::io::Result<u64>
+where
+    P: AsRef<Path>,
+    Q: AsRef<Path>,
+{
+    let mut src = open_source(source, source_nofollow)?;
+    let mut dst = create_dest_restrictive(dest, false)?;
+    std::io::copy(&mut src, &mut dst)
+}
+
 /// The fallback behavior for [`clone`] on failed system call.
 #[derive(Clone, Copy)]
 enum CloneFallback {
@@ -53,18 +67,20 @@ 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, nofollow: bool) -> std::io::Result<()>
 where
     P: AsRef<Path>,
 {
-    let src_file = File::open(&source)?;
+    let src_file = open_source(&source, nofollow)?;
     let dst_file = create_dest_restrictive(&dest, false)?;
     if ioctl_ficlone(dst_file, src_file).is_err() {
         return match fallback {
             CloneFallback::Error => Err(std::io::Error::last_os_error()),
-            CloneFallback::FSCopy => safe_copy_file(source, dest, false).map(|_| ()),
-            CloneFallback::SparseCopy => sparse_copy(source, dest),
-            CloneFallback::SparseCopyWithoutHole => sparse_copy_without_hole(source, dest),
+            CloneFallback::FSCopy => fs_copy(source, dest, nofollow).map(|_| ()),
+            CloneFallback::SparseCopy => sparse_copy(source, dest, nofollow),
+            CloneFallback::SparseCopyWithoutHole => {
+                sparse_copy_without_hole(source, dest, nofollow)
+            }
         };
     }
     Ok(())
@@ -74,8 +90,8 @@ 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> {
-    let mut src_file = File::open(source)?;
+fn check_for_data(source: &Path, nofollow: bool) -> Result<(bool, u64, u64), std::io::Error> {
+    let mut src_file = open_source(source, nofollow)?;
     let metadata = src_file.metadata()?;
 
     let size = metadata.size();
@@ -94,8 +110,8 @@ fn check_for_data(source: &Path) -> Result<(bool, u64, u64), std::io::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> {
-    let src_file = File::open(source)?;
+fn check_sparse_detection(source: &Path, nofollow: bool) -> Result<bool, std::io::Error> {
+    let src_file = open_source(source, nofollow)?;
     let metadata = src_file.metadata()?;
     let size = metadata.size();
     let blocks = metadata.blocks();
@@ -109,11 +125,11 @@ 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, nofollow: bool) -> std::io::Result<()>
 where
     P: AsRef<Path>,
 {
-    let src_file = File::open(source)?;
+    let src_file = open_source(source, nofollow)?;
     let dst_file = create_dest_restrictive(dest, false)?;
 
     let size = src_file.metadata()?.size();
@@ -144,11 +160,11 @@ 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, nofollow: bool) -> std::io::Result<()>
 where
     P: AsRef<Path>,
 {
-    let mut src_file = File::open(source)?;
+    let mut src_file = open_source(source, nofollow)?;
     let dst_file = create_dest_restrictive(dest, false)?;
 
     let size: usize = src_file.metadata()?.size().try_into().unwrap();
@@ -185,7 +201,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<()>
+fn copy_stream<P>(source: P, dest: P, nofollow: bool) -> std::io::Result<()>
 where
     P: AsRef<Path>,
 {
@@ -207,7 +223,7 @@ where
     //
     // TODO Update the code below to respect the case where
     // `--preserve=ownership` is not true.
-    let mut src_file = File::open(&source)?;
+    let mut src_file = open_source(&source, nofollow)?;
     // Use the same restrictive initial mode as the regular file path so that
     // the dest does not momentarily sit with broader perms. The `0o622 &
     // !umask` form previously used here could still allow group/other write
@@ -234,6 +250,7 @@ pub(crate) fn copy_on_write(
     sparse_mode: SparseMode,
     context: &str,
     source_is_stream: bool,
+    nofollow: bool,
 ) -> CopyResult<CopyDebug> {
     let mut copy_debug = CopyDebug {
         offload: OffloadReflinkDebug::Unknown,
@@ -247,18 +264,18 @@ pub(crate) fn copy_on_write(
             copy_debug.reflink = OffloadReflinkDebug::No;
             if source_is_stream {
                 copy_debug.offload = OffloadReflinkDebug::Avoided;
-                copy_stream(source, dest).map(|_| ())
+                copy_stream(source, dest, nofollow).map(|_| ())
             } else {
                 let mut copy_method = CopyMethod::Default;
-                let result = handle_reflink_never_sparse_always(source, dest);
+                let result = handle_reflink_never_sparse_always(source, dest, nofollow);
                 if let Ok((debug, method)) = result {
                     copy_debug = debug;
                     copy_method = method;
                 }
 
                 match copy_method {
-                    CopyMethod::FSCopy => safe_copy_file(source, dest, false).map(|_| ()),
-                    _ => sparse_copy(source, dest),
+                    CopyMethod::FSCopy => fs_copy(source, dest, nofollow).map(|_| ()),
+                    _ => sparse_copy(source, dest, nofollow),
                 }
             }
         }
@@ -267,13 +284,13 @@ pub(crate) fn copy_on_write(
 
             if source_is_stream {
                 copy_debug.offload = OffloadReflinkDebug::Avoided;
-                copy_stream(source, dest).map(|_| ())
+                copy_stream(source, dest, nofollow).map(|_| ())
             } else {
-                let result = handle_reflink_never_sparse_never(source);
+                let result = handle_reflink_never_sparse_never(source, nofollow);
                 if let Ok(debug) = result {
                     copy_debug = debug;
                 }
-                safe_copy_file(source, dest, false).map(|_| ())
+                fs_copy(source, dest, nofollow).map(|_| ())
             }
         }
         (ReflinkMode::Never, SparseMode::Auto) => {
@@ -281,18 +298,20 @@ pub(crate) fn copy_on_write(
 
             if source_is_stream {
                 copy_debug.offload = OffloadReflinkDebug::Avoided;
-                copy_stream(source, dest).map(|_| ())
+                copy_stream(source, dest, nofollow).map(|_| ())
             } else {
                 let mut copy_method = CopyMethod::Default;
-                let result = handle_reflink_never_sparse_auto(source, dest);
+                let result = handle_reflink_never_sparse_auto(source, dest, nofollow);
                 if let Ok((debug, method)) = result {
                     copy_debug = debug;
                     copy_method = method;
                 }
 
                 match copy_method {
-                    CopyMethod::SparseCopyWithoutHole => sparse_copy_without_hole(source, dest),
-                    _ => safe_copy_file(source, dest, false).map(|_| ()),
+                    CopyMethod::SparseCopyWithoutHole => {
+                        sparse_copy_without_hole(source, dest, nofollow)
+                    }
+                    _ => fs_copy(source, dest, nofollow).map(|_| ()),
                 }
             }
         }
@@ -301,18 +320,18 @@ pub(crate) fn copy_on_write(
             // SparseMode::Always
             if source_is_stream {
                 copy_debug.offload = OffloadReflinkDebug::Avoided;
-                copy_stream(source, dest).map(|_| ())
+                copy_stream(source, dest, nofollow).map(|_| ())
             } else {
                 let mut copy_method = CopyMethod::Default;
-                let result = handle_reflink_auto_sparse_always(source, dest);
+                let result = handle_reflink_auto_sparse_always(source, dest, nofollow);
                 if let Ok((debug, method)) = result {
                     copy_debug = debug;
                     copy_method = method;
                 }
 
                 match copy_method {
-                    CopyMethod::FSCopy => clone(source, dest, CloneFallback::FSCopy),
-                    _ => clone(source, dest, CloneFallback::SparseCopy),
+                    CopyMethod::FSCopy => clone(source, dest, CloneFallback::FSCopy, nofollow),
+                    _ => clone(source, dest, CloneFallback::SparseCopy, nofollow),
                 }
             }
         }
@@ -321,23 +340,23 @@ pub(crate) fn copy_on_write(
             copy_debug.reflink = OffloadReflinkDebug::No;
             if source_is_stream {
                 copy_debug.offload = OffloadReflinkDebug::Avoided;
-                copy_stream(source, dest).map(|_| ())
+                copy_stream(source, dest, nofollow).map(|_| ())
             } else {
-                let result = handle_reflink_auto_sparse_never(source);
+                let result = handle_reflink_auto_sparse_never(source, nofollow);
                 if let Ok(debug) = result {
                     copy_debug = debug;
                 }
 
-                clone(source, dest, CloneFallback::FSCopy)
+                clone(source, dest, CloneFallback::FSCopy, nofollow)
             }
         }
         (ReflinkMode::Auto, SparseMode::Auto) => {
             if source_is_stream {
                 copy_debug.offload = OffloadReflinkDebug::Unsupported;
-                copy_stream(source, dest).map(|_| ())
+                copy_stream(source, dest, nofollow).map(|_| ())
             } else {
                 let mut copy_method = CopyMethod::Default;
-                let result = handle_reflink_auto_sparse_auto(source, dest);
+                let result = handle_reflink_auto_sparse_auto(source, dest, nofollow);
                 if let Ok((debug, method)) = result {
                     copy_debug = debug;
                     copy_method = method;
@@ -345,9 +364,9 @@ pub(crate) fn copy_on_write(
 
                 match copy_method {
                     CopyMethod::SparseCopyWithoutHole => {
-                        clone(source, dest, CloneFallback::SparseCopyWithoutHole)
+                        clone(source, dest, CloneFallback::SparseCopyWithoutHole, nofollow)
                     }
-                    _ => clone(source, dest, CloneFallback::FSCopy),
+                    _ => clone(source, dest, CloneFallback::FSCopy, nofollow),
                 }
             }
         }
@@ -356,7 +375,7 @@ pub(crate) fn copy_on_write(
             copy_debug.sparse_detection = SparseDebug::No;
             copy_debug.reflink = OffloadReflinkDebug::Yes;
 
-            clone(source, dest, CloneFallback::Error)
+            clone(source, dest, CloneFallback::Error, nofollow)
         }
         (ReflinkMode::Always, _) => {
             return Err(translate!("cp-error-reflink-always-sparse-auto").into());
@@ -371,6 +390,7 @@ pub(crate) fn copy_on_write(
 fn handle_reflink_auto_sparse_always(
     source: &Path,
     dest: &Path,
+    nofollow: bool,
 ) -> Result<(CopyDebug, CopyMethod), std::io::Error> {
     let mut copy_debug = CopyDebug {
         offload: OffloadReflinkDebug::Unknown,
@@ -378,8 +398,8 @@ fn handle_reflink_auto_sparse_always(
         sparse_detection: SparseDebug::Zeros,
     };
     let mut copy_method = CopyMethod::Default;
-    let (data_flag, size, blocks) = check_for_data(source)?;
-    let sparse_flag = check_sparse_detection(source)?;
+    let (data_flag, size, blocks) = check_for_data(source, nofollow)?;
+    let sparse_flag = check_sparse_detection(source, nofollow)?;
 
     if data_flag || size < 512 {
         copy_debug.offload = OffloadReflinkDebug::Avoided;
@@ -408,14 +428,17 @@ 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,
+    nofollow: bool,
+) -> Result<CopyDebug, std::io::Error> {
     let mut copy_debug = CopyDebug {
         offload: OffloadReflinkDebug::Unknown,
         reflink: OffloadReflinkDebug::No,
         sparse_detection: SparseDebug::No,
     };
-    let (data_flag, size, _blocks) = check_for_data(source)?;
-    let sparse_flag = check_sparse_detection(source)?;
+    let (data_flag, size, _blocks) = check_for_data(source, nofollow)?;
+    let sparse_flag = check_sparse_detection(source, nofollow)?;
 
     if sparse_flag {
         copy_debug.sparse_detection = SparseDebug::SeekHole;
@@ -429,15 +452,18 @@ 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,
+    nofollow: bool,
+) -> Result<CopyDebug, std::io::Error> {
     let mut copy_debug = CopyDebug {
         offload: OffloadReflinkDebug::Unknown,
         reflink: OffloadReflinkDebug::No,
         sparse_detection: SparseDebug::No,
     };
 
-    let (data_flag, size, _blocks) = check_for_data(source)?;
-    let sparse_flag = check_sparse_detection(source)?;
+    let (data_flag, size, _blocks) = check_for_data(source, nofollow)?;
+    let sparse_flag = check_sparse_detection(source, nofollow)?;
 
     if sparse_flag {
         copy_debug.sparse_detection = SparseDebug::SeekHole;
@@ -454,6 +480,7 @@ fn handle_reflink_auto_sparse_never(source: &Path) -> Result<CopyDebug, std::io:
 fn handle_reflink_auto_sparse_auto(
     source: &Path,
     dest: &Path,
+    nofollow: bool,
 ) -> Result<(CopyDebug, CopyMethod), std::io::Error> {
     let mut copy_debug = CopyDebug {
         offload: OffloadReflinkDebug::Unknown,
@@ -462,8 +489,8 @@ fn handle_reflink_auto_sparse_auto(
     };
 
     let mut copy_method = CopyMethod::Default;
-    let (data_flag, size, blocks) = check_for_data(source)?;
-    let sparse_flag = check_sparse_detection(source)?;
+    let (data_flag, size, blocks) = check_for_data(source, nofollow)?;
+    let sparse_flag = check_sparse_detection(source, nofollow)?;
 
     if (data_flag && size != 0) || (size > 0 && size < 512) {
         copy_debug.offload = OffloadReflinkDebug::Yes;
@@ -497,6 +524,7 @@ fn handle_reflink_auto_sparse_auto(
 fn handle_reflink_never_sparse_auto(
     source: &Path,
     dest: &Path,
+    nofollow: bool,
 ) -> Result<(CopyDebug, CopyMethod), std::io::Error> {
     let mut copy_debug = CopyDebug {
         offload: OffloadReflinkDebug::Unknown,
@@ -504,8 +532,8 @@ fn handle_reflink_never_sparse_auto(
         sparse_detection: SparseDebug::No,
     };
 
-    let (data_flag, size, blocks) = check_for_data(source)?;
-    let sparse_flag = check_sparse_detection(source)?;
+    let (data_flag, size, blocks) = check_for_data(source, nofollow)?;
+    let sparse_flag = check_sparse_detection(source, nofollow)?;
 
     let mut copy_method = CopyMethod::Default;
     if data_flag || size < 512 {
@@ -533,6 +561,7 @@ fn handle_reflink_never_sparse_auto(
 fn handle_reflink_never_sparse_always(
     source: &Path,
     dest: &Path,
+    nofollow: bool,
 ) -> Result<(CopyDebug, CopyMethod), std::io::Error> {
     let mut copy_debug = CopyDebug {
         offload: OffloadReflinkDebug::Unknown,
@@ -541,8 +570,8 @@ fn handle_reflink_never_sparse_always(
     };
     let mut copy_method = CopyMethod::SparseCopy;
 
-    let (data_flag, size, blocks) = check_for_data(source)?;
-    let sparse_flag = check_sparse_detection(source)?;
+    let (data_flag, size, blocks) = check_for_data(source, nofollow)?;
+    let sparse_flag = check_sparse_detection(source, nofollow)?;
 
     if data_flag || size < 512 {
         copy_debug.offload = OffloadReflinkDebug::Avoided;

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

@@ -28,6 +28,7 @@ pub(crate) fn copy_on_write(
     sparse_mode: SparseMode,
     context: &str,
     source_is_stream: bool,
+    _nofollow: bool,
 ) -> CopyResult<CopyDebug> {
     if sparse_mode != SparseMode::Auto {
         return Err(translate!("cp-error-sparse-not-supported")
@@ -69,8 +70,19 @@ pub(crate) fn copy_on_write(
             {
                 // clonefile(2) fails if the destination exists.  Remove it and try again.  Do not
                 // bother to check if removal worked because we're going to try to clone again.
-                // first lets make sure the dest file is not read only
-                if fs::metadata(dest).is_ok_and(|md| !md.permissions().readonly()) {
+                // first lets make sure the dest file is not read only.
+                //
+                // If dest is a symlink, GNU cp follows it and writes through to
+                // the target rather than replacing the link itself. Removing
+                // dest here would unlink the symlink and the retry would
+                // clonefile a regular file in its place. Skip the retry — the
+                // AlreadyExists error stays in `error` and we fall through to
+                // fs::copy below, which follows the symlink via O_TRUNC.
+                let dest_is_symlink =
+                    fs::symlink_metadata(dest).is_ok_and(|md| md.file_type().is_symlink());
+                if !dest_is_symlink
+                    && fs::metadata(dest).is_ok_and(|md| !md.permissions().readonly())
+                {
                     // remove and copy again
                     // TODO: rewrite this to better match linux behavior
                     // linux first opens the source file and destination file then uses the file

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

@@ -3,11 +3,10 @@
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
 // spell-checker:ignore reflink
-use std::fs::File;
 use std::path::Path;
 
 use uucore::buf_copy;
-use uucore::safe_copy::create_dest_restrictive;
+use uucore::safe_copy::{create_dest_restrictive, open_source};
 use uucore::translate;
 
 use crate::{
@@ -23,6 +22,7 @@ pub(crate) fn copy_on_write(
     sparse_mode: SparseMode,
     context: &str,
     source_is_stream: bool,
+    nofollow: bool,
 ) -> CopyResult<CopyDebug> {
     if reflink_mode != ReflinkMode::Never {
         return Err(translate!("cp-error-reflink-not-supported")
@@ -41,8 +41,10 @@ pub(crate) fn copy_on_write(
     };
 
     if source_is_stream {
-        let mut src_file = File::open(source)?;
-        let mut dst_file = create_dest_restrictive(dest, false)?;
+        let mut src_file = open_source(source, nofollow)
+            .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?;
+        let mut dst_file = create_dest_restrictive(dest, false)
+            .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?;
 
         let dest_is_stream = is_stream(&dst_file.metadata()?);
         if !dest_is_stream {
@@ -57,13 +59,14 @@ pub(crate) fn copy_on_write(
         return Ok(copy_debug);
     }
 
-    // Equivalent of fs::copy but creates dest with DEST_INITIAL_MODE rather
-    // than the default umask-derived 0o666, closing the window where another
-    // user could read/write dest before cp applies the final permissions.
+    // Replacement for fs::copy: restrictive 0o600 dest mode (#10011) and
+    // O_NOFOLLOW on the *source* under -P (#10017). The destination open
+    // intentionally does not use O_NOFOLLOW so that an existing symlink at
+    // dest is followed, matching GNU cp.
     let mut src_file =
-        File::open(source).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?;
-    let mut dst_file =
-        create_dest_restrictive(dest, false).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?;
+        open_source(source, nofollow).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?;
+    let mut dst_file = create_dest_restrictive(dest, false)
+        .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?;
     std::io::copy(&mut src_file, &mut dst_file)
         .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?;
 

tests/by-util/test_cp.rs

@@ -3036,8 +3036,6 @@ fn test_cp_symlink_overwrite_detection() {
         .fails()
         .stderr_only(if cfg!(target_os = "windows") {
             "cp: will not copy 'good/README' through just-created symlink 'tmp\\README'\n"
-        } else if cfg!(target_os = "macos") {
-            "cp: will not overwrite just-created 'tmp/README' with 'good/README'\n"
         } else {
             "cp: will not copy 'good/README' through just-created symlink 'tmp/README'\n"
         });
@@ -7899,3 +7897,92 @@ fn test_cp_final_mode_unchanged_after_restrictive_create() {
         "dst final mode should match source & ~umask (got {mode:o})"
     );
 }
+
+// Sanity check for the `-P` happy path: a symlink source is copied as a
+// symlink, not by following it. The actual `O_NOFOLLOW` invariant for
+// issue #10017 (path swap to a symlink between lstat and open) cannot be
+// raced deterministically from a unit test; that is locked in by the
+// strace check in util/check-safe-traversal.sh, which fails if a future
+// change drops `O_NOFOLLOW` from the source open under `-P`.
+#[test]
+#[cfg(unix)]
+fn test_cp_no_dereference_copies_symlink_as_symlink() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.write("target", "secret target contents");
+    at.symlink_file("target", "src_link");
+
+    ucmd.arg("-P").arg("src_link").arg("dst").succeeds();
+    assert!(at.symlink_exists("dst"));
+    assert!(at.read_symlink("dst").ends_with("target"));
+}
+
+// Regression for GNU tests/cp/deref-slink: when the destination exists as
+// a symlink, `cp -d` (which implies --no-dereference for the source) must
+// still follow the destination symlink and overwrite the link's target.
+// `-P`/`-d` only forbids dereferencing on the source side; applying
+// O_NOFOLLOW to the dest open broke this and surfaced as ELOOP.
+#[test]
+#[cfg(unix)]
+fn test_cp_d_overwrites_existing_symlink_dest() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.touch("f");
+    at.touch("slink-target");
+    at.symlink_file("slink-target", "slink");
+
+    ucmd.arg("-d").arg("f").arg("slink").succeeds();
+
+    // The destination symlink itself remains a symlink (GNU follows it
+    // through to the target rather than replacing it).
+    assert!(at.symlink_exists("slink"));
+    assert!(at.read_symlink("slink").ends_with("slink-target"));
+}
+
+// Regression for GNU tests/cp/acl: `cp -p` must preserve POSIX ACLs on
+// Linux. ACLs are part of GNU's `mode` preservation, not its `xattr`
+// preservation, so the default-no-xattr change in #9704 must not strip
+// them. Only runs when `setfacl` is available so non-ACL filesystems and
+// non-Linux CI do not flag spurious failures.
+#[test]
+#[cfg(target_os = "linux")]
+fn test_cp_p_preserves_posix_acls() {
+    use std::process::Command;
+
+    if Command::new("setfacl").arg("--version").output().is_err() {
+        return;
+    }
+    if Command::new("getfacl").arg("--version").output().is_err() {
+        return;
+    }
+
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.touch("src");
+
+    let setfacl = Command::new("setfacl")
+        .arg("-m")
+        .arg("user:bin:rw-")
+        .arg(at.plus("src"))
+        .status();
+    let Ok(status) = setfacl else { return };
+    if !status.success() {
+        // Filesystem doesn't support ACLs; skip.
+        return;
+    }
+
+    ucmd.arg("-p").arg("src").arg("dst").succeeds();
+
+    let src_acl = Command::new("getfacl")
+        .arg("--omit-header")
+        .arg(at.plus("src"))
+        .output()
+        .unwrap();
+    let dst_acl = Command::new("getfacl")
+        .arg("--omit-header")
+        .arg(at.plus("dst"))
+        .output()
+        .unwrap();
+    assert_eq!(
+        String::from_utf8_lossy(&src_acl.stdout),
+        String::from_utf8_lossy(&dst_acl.stdout),
+        "cp -p must preserve POSIX ACLs (GNU tests/cp/acl regression)",
+    );
+}

util/check-safe-traversal.sh

@@ -233,27 +233,40 @@ if echo "$AVAILABLE_UTILS" | grep -q "mv"; then
     check_utility "mv" "openat,renameat,newfstatat,rename" "openat" "test_mv_src test_mv_dst" "move_directory"
 fi
 
-# Test cp destination creation - must use a restrictive initial mode (0o600)
-# so the destination is never briefly readable by other users on a shared
-# directory before cp applies the final permissions. See issue #10011.
+# cp invariant checks. Both #10011 (restrictive 0600 destination mode) and
+# #10017 (O_NOFOLLOW on the -P source) need to hold; verify each on its own
+# strace.
 if echo "$AVAILABLE_UTILS" | grep -q "cp"; then
-    echo "cp_perm_test" > test_cp_src_perm
     if [ "$USE_MULTICALL" -eq 1 ]; then
         cp_cmd="$COREUTILS_BIN cp"
     else
         cp_cmd="$PROJECT_ROOT/target/${PROFILE}/cp"
     fi
+
+    # #10011: destination created with mode 0600 so other users cannot open
+    # the file through its umask-derived initial mode before cp narrows it.
+    echo "cp_perm_test" > test_cp_src_perm
     rm -f test_cp_dst_perm
     strace -f -e trace=openat -o strace_cp_dest_perm.log \
         $cp_cmd test_cp_src_perm test_cp_dst_perm 2>/dev/null || true
-    # The creation openat should carry mode 0600. Any wider mode (e.g. 0666
-    # masked by umask) reopens the window #10011 closed.
     if ! grep -qE 'openat\(AT_FDCWD, "test_cp_dst_perm".*O_CREAT.*, 0600\)' strace_cp_dest_perm.log; then
         cat strace_cp_dest_perm.log
         fail_immediately "cp must create the destination with mode 0600 (issue #10011)"
     fi
     echo "✓ cp creates destination with restrictive 0600 mode"
     rm -f test_cp_src_perm test_cp_dst_perm
+
+    # #10017: -P opens source with O_NOFOLLOW so a path swap to a symlink
+    # between the lstat check and the open cannot redirect the copy.
+    echo "cp_nofollow_test" > test_cp_src
+    strace -f -e trace=openat -o strace_cp_nofollow.log \
+        $cp_cmd -P test_cp_src test_cp_dst 2>/dev/null || true
+    if ! grep -qE 'openat\(AT_FDCWD, "test_cp_src".*O_NOFOLLOW' strace_cp_nofollow.log; then
+        cat strace_cp_nofollow.log
+        fail_immediately "cp -P must open the source with O_NOFOLLOW (issue #10017)"
+    fi
+    echo "✓ cp -P opens source with O_NOFOLLOW"
+    rm -f test_cp_src test_cp_dst
 fi
 
 echo ""