mv: create the cross-device destination exclusively and chown it by fd

Author: krosci

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

@@ -26,7 +26,7 @@ use crate::{
 // Create the destination. It is followed when it is a pre-existing symlink,
 // matching GNU cp -d/-P which only forbid dereferencing on the source side.
 fn create_dest(dest: &Path) -> CopyResult<File> {
-    create_dest_restrictive(dest, false).map_err(|e| {
+    create_dest_restrictive(dest, false, false).map_err(|e| {
         CpError::IoErrContext(
             e,
             translate!("cp-error-cannot-create-regular-file", "path" => dest.quote()),
@@ -260,7 +260,7 @@ where
     // the dest does not momentarily sit with broader perms. The `0o622 &
     // !umask` form previously used here could still allow group/other write
     // under a permissive umask. See #10011.
-    let mut dst_file = create_dest_restrictive(&dest, false).map_err(|e| {
+    let mut dst_file = create_dest_restrictive(&dest, false, false).map_err(|e| {
         CpError::IoErrContext(
             e,
             translate!("cp-error-cannot-create-regular-file", "path" => dest.as_ref().quote()),

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

@@ -141,7 +141,7 @@ pub(crate) fn copy_on_write(
         } else {
             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| {
+            let mut dst_file = create_dest_restrictive(dest, false, false).map_err(|e| {
                 CpError::IoErrContext(
                     e,
                     translate!("cp-error-cannot-create-regular-file", "path" => dest.quote()),

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

@@ -46,7 +46,7 @@ pub(crate) fn copy_on_write(
     if source_is_stream {
         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| {
+        let mut dst_file = create_dest_restrictive(dest, false, false).map_err(|e| {
             CpError::IoErrContext(
                 e,
                 translate!("cp-error-cannot-create-regular-file", "path" => dest.quote()),
@@ -72,7 +72,7 @@ pub(crate) fn copy_on_write(
     // dest is followed, matching GNU cp.
     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| {
+    let mut dst_file = create_dest_restrictive(dest, false, false).map_err(|e| {
         CpError::IoErrContext(
             e,
             translate!("cp-error-cannot-create-regular-file", "path" => dest.quote()),

src/uu/mv/src/mv.rs

@@ -1398,9 +1398,9 @@ fn rename_file_fallback(
         }
     }
 
-    // Open src/dst with O_NOFOLLOW and keep the fds alive across copy,
-    // chown, xattr, and chmod so a concurrent path-swap can't redirect any
-    // step to a different inode.
+    // Open the source with O_NOFOLLOW, create the destination exclusively,
+    // and keep both descriptors alive across copy, xattr, chown, and chmod so
+    // a concurrent path-swap cannot redirect any step to a different inode.
     #[cfg(unix)]
     {
         use std::fs::Permissions;
@@ -1413,8 +1413,10 @@ fn rename_file_fallback(
             .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?
             .mode()
             & 0o7777;
-        let mut dst_file = create_dest_restrictive(to, /* nofollow */ true)
-            .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?;
+        let mut dst_file = create_dest_restrictive(
+            to, /* nofollow */ true, /* exclusive */ true,
+        )
+        .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?;
         uucore::buf_copy::copy_fast(&mut &src_file, &mut dst_file)
             .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?;
 
@@ -1430,7 +1432,7 @@ fn rename_file_fallback(
         // `mv`, and re-applying setuid/setgid would hand them a binary running
         // as themselves that used to run as someone else. GNU strips the bits
         // in that case and so do we.
-        let ownership_preserved = preserve_ownership(from, to).unwrap_or(false);
+        let ownership_preserved = preserve_ownership_fd(&src_file, &dst_file).unwrap_or(false);
         let dest_mode = if ownership_preserved {
             src_mode
         } else {
@@ -1496,6 +1498,38 @@ fn preserve_ownership(from: &Path, to: &Path) -> io::Result<bool> {
     Ok(true)
 }
 
+/// [`preserve_ownership`] on the already-open source and destination
+/// descriptors, via `fchown`.
+///
+/// A cross-device copy holds both descriptors open across the content copy,
+/// yet the chown used to re-resolve the destination by path, so a concurrent
+/// path-swap could redirect it to an inode the copy never touched. `fchown`
+/// acts on the inode behind `to`, which is the one the caller created and
+/// wrote. Returns the same "did the destination keep the source's uid/gid"
+/// answer as the path-based variant.
+#[cfg(unix)]
+fn preserve_ownership_fd(from: &fs::File, to: &fs::File) -> io::Result<bool> {
+    use rustix::fs::{Gid, Uid, fchown};
+    use std::os::unix::fs::MetadataExt;
+
+    let source_meta = from.metadata()?;
+    let uid = source_meta.uid();
+    let gid = source_meta.gid();
+
+    let dest_meta = to.metadata()?;
+
+    // Only chown if ownership actually differs
+    if uid != dest_meta.uid() || gid != dest_meta.gid() {
+        // Silently ignore errors: non-root users typically cannot chown to
+        // arbitrary uid, matching GNU mv behavior which also uses best-effort.
+        if fchown(to, Some(Uid::from_raw(uid)), Some(Gid::from_raw(gid))).is_err() {
+            return Ok(false);
+        }
+    }
+
+    Ok(true)
+}
+
 fn is_empty_dir(path: &Path) -> bool {
     fs::read_dir(path).is_ok_and(|mut contents| contents.next().is_none())
 }

src/uucore/src/lib/features/safe_copy.rs

@@ -19,7 +19,9 @@
 //!   shared directory cannot open the file before the caller narrows the
 //!   final permissions via `set_permissions` (issue #10011). The same
 //!   `nofollow` flag refuses to truncate through a symlink that may have
-//!   been swapped in at the destination path.
+//!   been swapped in at the destination path, and `exclusive` refuses to
+//!   open *any* pre-existing name, including a hard link to a file the
+//!   caller did not create.
 
 use std::fs::File;
 use std::io;
@@ -38,6 +40,10 @@ const DEST_FLAGS: OFlags = OFlags::WRONLY
     .union(OFlags::CREATE)
     .union(OFlags::TRUNC)
     .union(OFlags::CLOEXEC);
+const DEST_EXCL_FLAGS: OFlags = OFlags::WRONLY
+    .union(OFlags::CREATE)
+    .union(OFlags::EXCL)
+    .union(OFlags::CLOEXEC);
 
 /// Open `path` for reading, optionally with `O_NOFOLLOW`.
 ///
@@ -70,8 +76,25 @@ pub fn open_source<P: AsRef<Path>>(path: P, nofollow: bool) -> io::Result<File>
 /// who plants `path` as a symlink between the caller's check and this
 /// open can redirect the truncate (and the subsequent write) to any file
 /// the caller has permission to write.
-pub fn create_dest_restrictive<P: AsRef<Path>>(path: P, nofollow: bool) -> io::Result<File> {
-    let mut flags = DEST_FLAGS;
+///
+/// With `exclusive = true`, the call carries `O_EXCL` and fails with
+/// `EEXIST` instead of opening an existing name at all. Pass `true`
+/// whenever the caller has just unlinked `path` and intends to create a
+/// fresh inode: `nofollow` alone still opens a hard link planted in that
+/// window, which would truncate (and later chown and chmod) a file the
+/// caller did not create. A symlink also fails under `O_EXCL`, so
+/// `nofollow` is subsumed when `exclusive` is set; the reverse is not
+/// true, since `O_NOFOLLOW` still opens a pre-existing regular file.
+pub fn create_dest_restrictive<P: AsRef<Path>>(
+    path: P,
+    nofollow: bool,
+    exclusive: bool,
+) -> io::Result<File> {
+    let mut flags = if exclusive {
+        DEST_EXCL_FLAGS
+    } else {
+        DEST_FLAGS
+    };
     if nofollow {
         flags |= OFlags::NOFOLLOW;
     }
@@ -138,7 +161,7 @@ mod tests {
     fn create_dest_uses_restrictive_initial_mode() {
         let dir = tempdir().unwrap();
         let path = dir.path().join("new");
-        let f = create_dest_restrictive(&path, false).unwrap();
+        let f = create_dest_restrictive(&path, false, false).unwrap();
         let mode = f.metadata().unwrap().mode() & 0o777;
         assert_eq!(mode, DEST_INITIAL_MODE);
     }
@@ -159,7 +182,7 @@ mod tests {
         }
         // Re-open via the helper — mode of the existing inode stays 0o644,
         // only the contents are truncated.
-        create_dest_restrictive(&path, false).unwrap();
+        create_dest_restrictive(&path, false, false).unwrap();
         let mode = std::fs::metadata(&path).unwrap().mode() & 0o777;
         assert_eq!(mode, 0o644);
         assert_eq!(std::fs::metadata(&path).unwrap().len(), 0);
@@ -177,11 +200,26 @@ mod tests {
         std::fs::write(&victim, b"do not truncate me").unwrap();
         symlink(&victim, &dst).unwrap();
 
-        let err = create_dest_restrictive(&dst, true).unwrap_err();
+        let err = create_dest_restrictive(&dst, true, false).unwrap_err();
         assert_eq!(
             err.raw_os_error(),
             Some(rustix::io::Errno::LOOP.raw_os_error())
         );
         assert_eq!(std::fs::read(&victim).unwrap(), b"do not truncate me");
     }
+
+    #[test]
+    fn create_dest_exclusive_refuses_existing_and_hard_linked() {
+        // An attacker who plants a hard link in the window between the
+        // caller's unlink and this create must not get the victim truncated
+        // and later chowned/chmoded. O_EXCL refuses the existing name.
+        let dir = tempdir().unwrap();
+        let victim = dir.path().join("victim");
+        std::fs::write(&victim, b"SECRET").unwrap();
+        std::fs::hard_link(&victim, dir.path().join("planted")).unwrap();
+
+        let err = create_dest_restrictive(dir.path().join("planted"), true, true).unwrap_err();
+        assert_eq!(err.kind(), io::ErrorKind::AlreadyExists);
+        assert_eq!(std::fs::read(&victim).unwrap(), b"SECRET");
+    }
 }

tests/by-util/test_mv.rs

@@ -2074,6 +2074,41 @@ mod inter_partition_copying {
         );
     }
 
+    // A cross-device move onto an existing regular file must replace that name
+    // with a fresh inode created O_EXCL, not truncate whatever the name
+    // pointed at. Here the destination name is a hard link to a victim; the
+    // victim's contents must survive the overwrite.
+    #[test]
+    pub(crate) fn test_mv_inter_partition_existing_dest_replaced() {
+        let scene = TestScenario::new(util_name!());
+        let at = &scene.fixtures;
+
+        at.write("src", "src contents");
+
+        let other_fs_tempdir =
+            TempDir::new_in("/dev/shm/").expect("Unable to create temp directory");
+        let victim = other_fs_tempdir.path().join("victim");
+        write(&victim, "victim contents").expect("Unable to write victim");
+        let dest = other_fs_tempdir.path().join("dest");
+        fs::hard_link(&victim, &dest).expect("Unable to hard link dest to victim");
+
+        scene
+            .ucmd()
+            .arg("src")
+            .arg(dest.to_str().unwrap())
+            .succeeds();
+
+        assert_eq!(
+            fs::read_to_string(&dest).expect("destination should be readable"),
+            "src contents"
+        );
+        assert_eq!(
+            fs::read_to_string(&victim).expect("victim should be readable"),
+            "victim contents",
+            "the hard link's target must not be truncated"
+        );
+    }
+
     // Ensure that the copying code used in an inter-partition move unlinks the destination symlink.
     #[test]
     pub(crate) fn test_mv_unlinks_dest_symlink() {