From 376f2135f60d60fc3c53d55ba58aa4a4af7282ff Mon Sep 17 00:00:00 2001 From: Alessio Attilio Date: Thu, 17 Sep 2026 01:24:14 +0200 Subject: [PATCH] mv: create the cross-device destination exclusively and chown it by fd --- src/uu/cp/src/platform/linux.rs | 4 +- src/uu/cp/src/platform/macos.rs | 2 +- src/uu/cp/src/platform/other_unix.rs | 4 +- src/uu/mv/src/mv.rs | 46 +++++++++++++++++++--- src/uucore/src/lib/features/safe_copy.rs | 50 +++++++++++++++++++++--- tests/by-util/test_mv.rs | 35 +++++++++++++++++ 6 files changed, 124 insertions(+), 17 deletions(-) diff --git a/src/uu/cp/src/platform/linux.rs b/src/uu/cp/src/platform/linux.rs index 0ba2ce30076..77d39c64109 100644 --- a/src/uu/cp/src/platform/linux.rs +++ b/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 { - 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()), diff --git a/src/uu/cp/src/platform/macos.rs b/src/uu/cp/src/platform/macos.rs index b338c2bffbc..7b06cad36ed 100644 --- a/src/uu/cp/src/platform/macos.rs +++ b/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()), diff --git a/src/uu/cp/src/platform/other_unix.rs b/src/uu/cp/src/platform/other_unix.rs index 754fa872e41..b19ecaf6561 100644 --- a/src/uu/cp/src/platform/other_unix.rs +++ b/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()), diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index f17a06ccf5e..11d803ef5ed 100644 --- a/src/uu/mv/src/mv.rs +++ b/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 { 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 { + 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()) } diff --git a/src/uucore/src/lib/features/safe_copy.rs b/src/uucore/src/lib/features/safe_copy.rs index d8cbc36b824..69e6fa0c2bf 100644 --- a/src/uucore/src/lib/features/safe_copy.rs +++ b/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>(path: P, nofollow: bool) -> io::Result /// 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>(path: P, nofollow: bool) -> io::Result { - 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>( + path: P, + nofollow: bool, + exclusive: bool, +) -> io::Result { + 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"); + } } diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index caa8f15b09c..251efc95803 100644 --- a/tests/by-util/test_mv.rs +++ b/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() {