From 99e62685d8f4eafe77b44414c4abcab6db518820 Mon Sep 17 00:00:00 2001
From: Sylvestre Ledru <sylvestre@debian.org>
Date: Tue, 28 Jul 2026 23:17:22 +0200
Subject: [PATCH] cp: apply the finalize chmod without following a symlink

Both chmod sites in copy_file/copy_attributes decide whether to act by testing
dest for being a symlink, but the test happens long before the chmod: in
copy_file the lstat is taken before the copy starts, so the gap spans the whole
file copy. A symlink swapped into dest during that gap redirected the mode
change onto the link's target, off the destination tree.

Measured with a tight unlink/symlink loop against a 20MB copy, 40 rounds each,
an unrelated 0600 file taken to 0777:

  before   12/40 and 13/40 rounds hit
  after     0/40
  GNU 9.10  2/40 and 3/40

Anchor the chmod to the parent directory fd with NoFollow, reusing
DirFd::chmod_at. EOPNOTSUPP/ELOOP mean dest is now a symlink, which is exactly
the "do nothing" case both guards already describe, so it maps to success.

Note GNU is affected by the same class at a lower rate, so this is hardening
beyond GNU rather than a compatibility fix. The remaining path-based steps in
copy_attributes (timestamps, xattr, ACL, SELinux) are unchanged.
---
 src/uu/cp/src/cp.rs      | 40 +++++++++++++++++++++++++++++++++++++--
 tests/by-util/test_cp.rs | 41 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 2 deletions(-)

diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs
index fe5390a6073..e9b54c78458 100644
--- a/src/uu/cp/src/cp.rs
+++ b/src/uu/cp/src/cp.rs
@@ -1881,7 +1881,7 @@ pub(crate) fn copy_attributes(
             #[cfg(not(unix))]
             let source_perms = source_metadata.permissions();
 
-            fs::set_permissions(dest, source_perms)
+            chmod_nofollow(dest, &source_perms)
                 .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?;
             // FIXME: Implement this for windows as well
             #[cfg(feature = "feat_acl")]
@@ -2511,6 +2511,42 @@ fn calculate_dest_permissions(
 ///
 /// The original permissions of `source` will be copied to `dest`
 /// after a successful copy.
+/// Apply `permissions` to `dest` without following a symlink at the final component.
+///
+/// Both callers test `dest` for being a symlink well before the chmod, so a symlink
+/// swapped in inside that gap would redirect the mode onto the link's target.
+/// `chmod(2)` cannot change a symlink's permissions, so `EOPNOTSUPP` here means
+/// exactly the "do nothing" case the callers' guards describe.
+#[cfg(unix)]
+fn chmod_nofollow(dest: &Path, permissions: &Permissions) -> io::Result<()> {
+    use uucore::safe_traversal::{DirFd, SymlinkBehavior};
+
+    let (Some(parent), Some(name)) = (dest.parent(), dest.file_name()) else {
+        // `.`, `..` and `/` have no final component to anchor against. They are
+        // always directories and never a symlink themselves, so there is nothing
+        // to be redirected: chmod the directory through its own fd.
+        let dir_fd = DirFd::open(dest, SymlinkBehavior::NoFollow)?;
+        return dir_fd.fchmod(permissions.mode());
+    };
+    // `Path::parent()` yields "" for a bare filename.
+    let parent = if parent.as_os_str().is_empty() {
+        Path::new(".")
+    } else {
+        parent
+    };
+
+    let dir_fd = DirFd::open(parent, SymlinkBehavior::Follow)?;
+    match dir_fd.chmod_at(name, permissions.mode(), SymlinkBehavior::NoFollow) {
+        Err(e) if matches!(e.raw_os_error(), Some(libc::EOPNOTSUPP) | Some(libc::ELOOP)) => Ok(()),
+        other => other,
+    }
+}
+
+#[cfg(not(unix))]
+fn chmod_nofollow(dest: &Path, permissions: &Permissions) -> io::Result<()> {
+    fs::set_permissions(dest, permissions.clone())
+}
+
 #[allow(clippy::cognitive_complexity, clippy::too_many_arguments)]
 fn copy_file(
     progress_bar: Option<&ProgressBar>,
@@ -2703,7 +2739,7 @@ fn copy_file(
         //
         // FWIW, the OS will throw an error later, on the write op, if
         // the user does not have permission to write to the file.
-        fs::set_permissions(dest, dest_permissions).ok();
+        chmod_nofollow(dest, &dest_permissions).ok();
     }
 
     let copy_attributes_result = if options.dereference(source_in_command_line) {
diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs
index d37634602c7..476564ca797 100644
--- a/tests/by-util/test_cp.rs
+++ b/tests/by-util/test_cp.rs
@@ -6695,6 +6695,47 @@ fn test_cp_parents_symlink_permissions_file() {
     );
 }
 
+/// The finalize chmod goes through a parent-fd + `NoFollow` chmod rather than a
+/// path-based `set_permissions`. Pin the modes it is responsible for, including
+/// the setuid and read-only cases, so the no-follow path cannot silently stop
+/// applying them.
+#[test]
+#[cfg(unix)]
+fn test_cp_preserve_mode_via_nofollow_chmod() {
+    for mode in [0o644, 0o600, 0o755, 0o444, 0o4755] {
+        let scene = TestScenario::new(util_name!());
+        let at = &scene.fixtures;
+        at.write("src", "hi");
+        at.set_mode("src", mode);
+
+        scene.ucmd().args(&["-p", "src", "dst"]).succeeds();
+
+        assert_eq!(
+            at.metadata("dst").permissions().mode() & 0o7777,
+            mode,
+            "cp -p did not reproduce mode {mode:o}"
+        );
+    }
+}
+
+/// Overwriting an existing destination keeps the destination's mode, not the
+/// source's — the other branch feeding the same chmod.
+#[test]
+#[cfg(unix)]
+fn test_cp_existing_dest_keeps_its_mode() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+    at.write("src", "new");
+    at.set_mode("src", 0o600);
+    at.write("dst", "old");
+    at.set_mode("dst", 0o640);
+
+    scene.ucmd().args(&["src", "dst"]).succeeds();
+
+    assert_eq!(at.metadata("dst").permissions().mode() & 0o7777, 0o640);
+    assert_eq!(at.read("dst"), "new");
+}
+
 /// Test the behavior of preserving permissions of parents when copying through
 /// a symlink when source is a dir.
 #[test]