mv: strip setuid/setgid when the cross-device copy cannot preserve ownership

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<bool> {
     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 {

tests/by-util/test_mv.rs

@@ -712,8 +712,8 @@
     for target in ["./test_mv_spell_a", "test_mv_spell_a"] {
         let (at, mut ucmd) = at_and_ucmd!();
         let source = "test_mv_spell_a~";
         at.write(source, "SRCDATA");
         at.write("test_mv_spell_a", "DSTDATA");
 
         ucmd.arg(source)
             .arg(target)
@@ -723,7 +723,7 @@
 
         assert_eq!(
             at.read(source),
             "SRCDATA",
             "mv destroyed the source when the target was spelled {target}"
         );
     }
@@ -734,8 +734,8 @@
 #[cfg(all(unix, not(target_os = "android")))]
 fn test_mv_backup_simple_guard_allows_unrelated_source() {
     let (at, mut ucmd) = at_and_ucmd!();
     at.write("test_mv_spell_src", "SRCDATA");
     at.write("test_mv_spell_dst", "DSTDATA");
 
     ucmd.arg("test_mv_spell_src")
         .arg("test_mv_spell_dst")
@@ -1985,6 +1985,40 @@
     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() {