diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 2c3202fbf9..0e2aa73c56 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -1324,8 +1324,18 @@ fn rename_file_fallback( // chown before chmod: chown(2) clears setuid/setgid for non-root, // so the final mode must be applied last to preserve those bits. - let _ = preserve_ownership(from, to); - let _ = dst_file.set_permissions(Permissions::from_mode(src_mode)); + // + // If the chown did not take, the destination belongs to whoever ran + // `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 dest_mode = if ownership_preserved { + src_mode + } else { + src_mode & !0o6000 + }; + let _ = dst_file.set_permissions(Permissions::from_mode(dest_mode)); } #[cfg(not(unix))] @@ -1342,8 +1352,13 @@ fn rename_file_fallback( /// Preserve ownership (uid/gid) from source to destination. /// Uses lchown so it works on symlinks without following them. /// Errors are silently ignored for non-root users who cannot chown. +/// +/// Returns whether the destination ended up with the source's uid and gid. +/// Callers that re-apply the source mode must strip setuid/setgid when this is +/// false: those bits on a file that changed hands grant the new owner's +/// identity, not the original one's, which is why GNU drops them. #[cfg(unix)] -fn preserve_ownership(from: &Path, to: &Path) -> io::Result<()> { +fn preserve_ownership(from: &Path, to: &Path) -> io::Result { use std::os::unix::fs::MetadataExt; let source_meta = from.symlink_metadata()?; @@ -1360,7 +1375,7 @@ fn preserve_ownership(from: &Path, to: &Path) -> io::Result<()> { // Use follow=false so lchown is used (works on symlinks) // Silently ignore errors: non-root users typically cannot chown to // arbitrary uid, matching GNU mv behavior which also uses best-effort. - let _ = wrap_chown( + if wrap_chown( to, &dest_meta, Some(uid), @@ -1370,10 +1385,14 @@ fn preserve_ownership(from: &Path, to: &Path) -> io::Result<()> { groups_only: false, level: VerbosityLevel::Silent, }, - ); + ) + .is_err() + { + return Ok(false); + } } - Ok(()) + Ok(true) } fn is_empty_dir(path: &Path) -> bool { diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index cbbe737922..a546eaa02d 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -1985,6 +1985,40 @@ mod inter_partition_copying { use uutests::util::TestScenario; use uutests::util_name; + // setuid/setgid must survive a cross-device move when ownership is + // preserved. The mover owns the file here, so the chown is a no-op and the + // bits are legitimate; only a failed chown justifies stripping them, and + // that needs a second uid, so it is exercised out of band rather than here. + #[test] + pub(crate) fn test_mv_inter_partition_keeps_setuid_when_ownership_preserved() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.write("src", "src contents"); + set_permissions(at.plus("src"), PermissionsExt::from_mode(0o6755)) + .expect("Unable to set setuid/setgid on src"); + + let other_fs_tempdir = + TempDir::new_in("/dev/shm/").expect("Unable to create temp directory"); + let dest = other_fs_tempdir.path().join("dest"); + + scene + .ucmd() + .arg("src") + .arg(dest.to_str().unwrap()) + .succeeds(); + + let mode = fs::metadata(&dest) + .expect("destination should exist") + .permissions() + .mode() + & 0o7777; + assert_eq!( + mode, 0o6755, + "setuid/setgid must be kept when ownership was preserved, got {mode:o}" + ); + } + // Ensure that the copying code used in an inter-partition move unlinks the destination symlink. #[test] pub(crate) fn test_mv_unlinks_dest_symlink() {