diff --git a/src/uu/mv/Cargo.toml b/src/uu/mv/Cargo.toml index 4a169af0f..c252bb3a3 100644 --- a/src/uu/mv/Cargo.toml +++ b/src/uu/mv/Cargo.toml @@ -31,6 +31,7 @@ uucore = { workspace = true, features = [ "fs", "fsxattr", "perms", + "safe-copy", "update-control", ] } fluent = { workspace = true } diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index d78be6d8d..97dbe3e6e 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -921,7 +921,7 @@ fn rename_symlink_fallback(from: &Path, to: &Path) -> io::Result<()> { } #[cfg(not(any(target_os = "macos", target_os = "redox")))] { - let _ = copy_xattrs_if_supported(from, to); + let _ = fsxattr::copy_xattrs(from, to); } let _ = preserve_ownership(from, to); fs::remove_file(from) @@ -1250,7 +1250,7 @@ fn copy_file_with_hardlinks_helper( // Copy xattrs, ignoring ENOTSUP errors (filesystem doesn't support xattrs) #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] { - let _ = copy_xattrs_if_supported(from, to); + let _ = fsxattr::copy_xattrs(from, to); } // Preserve ownership (uid/gid) from the source let _ = preserve_ownership(from, to); @@ -1293,20 +1293,41 @@ fn rename_file_fallback( } } - // Regular file copy - fs::copy(from, to) - .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?; - - // Copy xattrs, ignoring ENOTSUP errors (filesystem doesn't support xattrs) - #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] - { - let _ = copy_xattrs_if_supported(from, to); - } - - // Preserve ownership (uid/gid) from the source file + // 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. #[cfg(unix)] { + use std::fs::Permissions; + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + use uucore::safe_copy::{create_dest_restrictive, open_source}; + let src_file = open_source(from, /* nofollow */ true) + .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?; + let src_mode = src_file + .metadata() + .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")))?; + io::copy(&mut &src_file, &mut dst_file) + .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?; + + #[cfg(not(any(target_os = "macos", target_os = "redox")))] + { + let _ = fsxattr::copy_xattrs_fd(&src_file, &dst_file); + } + + // 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)); + } + + #[cfg(not(unix))] + { + fs::copy(from, to) + .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?; } fs::remove_file(from) @@ -1351,18 +1372,6 @@ fn preserve_ownership(from: &Path, to: &Path) -> io::Result<()> { Ok(()) } -/// Copy xattrs from source to destination, ignoring ENOTSUP/EOPNOTSUPP errors. -/// These errors indicate the filesystem doesn't support extended attributes, -/// which is acceptable when moving files across filesystems. -#[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] -fn copy_xattrs_if_supported(from: &Path, to: &Path) -> io::Result<()> { - match fsxattr::copy_xattrs(from, to) { - Ok(()) => Ok(()), - Err(e) if e.raw_os_error() == Some(libc::EOPNOTSUPP) => Ok(()), - Err(e) => Err(e), - } -} - 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/Cargo.toml b/src/uucore/Cargo.toml index f164e8655..da2c1b903 100644 --- a/src/uucore/Cargo.toml +++ b/src/uucore/Cargo.toml @@ -138,7 +138,7 @@ extendedbigdecimal = ["bigdecimal", "num-traits"] fast-inc = [] fs = ["dunce", "libc", "winapi-util", "windows-sys"] fsext = ["libc", "windows-sys", "bstr"] -fsxattr = ["xattr", "itertools"] +fsxattr = ["xattr", "itertools", "libc"] hardware = [] lines = [] feat_systemd_logind = ["utmpx", "libc"] diff --git a/src/uucore/src/lib/features/fsxattr.rs b/src/uucore/src/lib/features/fsxattr.rs index 2b42edaa3..0c581690a 100644 --- a/src/uucore/src/lib/features/fsxattr.rs +++ b/src/uucore/src/lib/features/fsxattr.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore getxattr posix_acl_default posix_acl_access +// spell-checker:ignore getxattr posix_acl_default posix_acl_access ENOTSUP EOPNOTSUPP renamer //! Set of functions to manage xattr on files and dirs use itertools::Itertools; @@ -13,20 +13,74 @@ use std::ffi::{OsStr, OsString}; use std::os::unix::ffi::OsStrExt; use std::path::Path; +/// Returns true if the error is the kernel/filesystem signaling that +/// extended attributes are not supported (`ENOTSUP` / `EOPNOTSUPP`). +/// On Linux these are the same errno; on the BSDs they differ, so we +/// match on either. +#[cfg(unix)] +fn is_xattr_unsupported(err: &std::io::Error) -> bool { + matches!( + err.raw_os_error(), + Some(e) if e == libc::ENOTSUP || e == libc::EOPNOTSUPP + ) +} + +#[cfg(not(unix))] +fn is_xattr_unsupported(_err: &std::io::Error) -> bool { + false +} + /// Copies extended attributes (xattrs) from one file or directory to another. /// +/// Returns `Ok(())` if the destination filesystem signals that xattrs are +/// not supported (`ENOTSUP` / `EOPNOTSUPP`), since cross-filesystem moves +/// onto e.g. tmpfs without xattr support are a legitimate scenario. All +/// other errors propagate so the caller can surface (for example) +/// permission failures on `security.*` namespaces. +/// /// # Arguments /// /// * `source` - A reference to the source path. /// * `dest` - A reference to the destination path. -/// -/// # Returns -/// -/// A result indicating success or failure. pub fn copy_xattrs>(source: P, dest: P) -> std::io::Result<()> { - for attr_name in xattr::list(&source)? { + let attrs = match xattr::list(&source) { + Ok(a) => a, + Err(e) if is_xattr_unsupported(&e) => return Ok(()), + Err(e) => return Err(e), + }; + for attr_name in attrs { if let Some(value) = xattr::get(&source, &attr_name)? { - xattr::set(&dest, &attr_name, &value)?; + if let Err(e) = xattr::set(&dest, &attr_name, &value) { + if is_xattr_unsupported(&e) { + return Ok(()); + } + return Err(e); + } + } + } + Ok(()) +} + +/// Copies xattrs between two open file descriptors. Pins both inodes so +/// list/get/set calls cannot be redirected by a concurrent renamer, unlike +/// the path-based [`copy_xattrs`]. `ENOTSUP` / `EOPNOTSUPP` is treated as +/// success. +#[cfg(unix)] +pub fn copy_xattrs_fd(source: &std::fs::File, dest: &std::fs::File) -> std::io::Result<()> { + use xattr::FileExt; + let attrs = match source.list_xattr() { + Ok(a) => a, + Err(e) if is_xattr_unsupported(&e) => return Ok(()), + Err(e) => return Err(e), + }; + for attr_name in attrs { + if let Some(value) = source.get_xattr(&attr_name)? { + if let Err(e) = dest.set_xattr(&attr_name, &value) { + if is_xattr_unsupported(&e) { + return Ok(()); + } + return Err(e); + } } } Ok(()) @@ -222,6 +276,35 @@ mod tests { assert_eq!(copied_value, test_value); } + #[test] + #[cfg(unix)] + fn test_copy_xattrs_fd() { + let temp_dir = tempdir().unwrap(); + let source_path = temp_dir.path().join("source.txt"); + let dest_path = temp_dir.path().join("dest.txt"); + + File::create(&source_path).unwrap(); + File::create(&dest_path).unwrap(); + + let test_attr = "user.fd_test"; + let test_value = b"fd value"; + // Skip silently if the test fs doesn't support user xattrs. + if xattr::set(&source_path, test_attr, test_value).is_err() { + return; + } + + let src = File::open(&source_path).unwrap(); + let dst = std::fs::OpenOptions::new() + .write(true) + .open(&dest_path) + .unwrap(); + + copy_xattrs_fd(&src, &dst).unwrap(); + + let copied = xattr::get(&dest_path, test_attr).unwrap().unwrap(); + assert_eq!(copied, test_value); + } + #[test] fn test_apply_and_retrieve_xattrs() { let temp_dir = tempdir().unwrap(); diff --git a/util/check-safe-traversal.sh b/util/check-safe-traversal.sh index 1b01434d6..97869789d 100755 --- a/util/check-safe-traversal.sh +++ b/util/check-safe-traversal.sh @@ -269,6 +269,53 @@ if echo "$AVAILABLE_UTILS" | grep -q "cp"; then rm -f test_cp_src test_cp_dst fi +# mv cross-device (EXDEV) must use fd-based *xattr ops (issue #10014). +if echo "$AVAILABLE_UTILS" | grep -q "mv" && [ -d /dev/shm ]; then + # Need different filesystems for the EXDEV fallback to fire. + temp_fs_id=$(stat -f -c %i "$TEMP_DIR" 2>/dev/null || echo "") + shm_fs_id=$(stat -f -c %i /dev/shm 2>/dev/null || echo "") + if [ -z "$temp_fs_id" ] || [ -z "$shm_fs_id" ] || [ "$temp_fs_id" = "$shm_fs_id" ]; then + echo "WARN: mv cross-device xattr check: TMPDIR and /dev/shm are on the same filesystem; skipped" + else + shm_probe=$(mktemp -p /dev/shm mv_xattr_probe.XXXXXX) + if setfattr -n user.probe -v ok "$shm_probe" 2>/dev/null; then + rm -f "$shm_probe" + cross_src=$(mktemp -p "$TEMP_DIR" cross_src.XXXXXX) + cross_dst=$(mktemp -u -p /dev/shm cross_dst.XXXXXX) + echo "cross-device payload" > "$cross_src" + if setfattr -n user.tag -v pinned "$cross_src" 2>/dev/null; then + if [ "$USE_MULTICALL" -eq 1 ]; then + mv_cmd="$COREUTILS_BIN mv" + else + mv_cmd="$PROJECT_ROOT/target/${PROFILE}/mv" + fi + strace -f -e trace='%file,fgetxattr,fsetxattr,flistxattr,getxattr,setxattr,listxattr' \ + -o strace_mv_xattr.log \ + $mv_cmd "$cross_src" "$cross_dst" 2>/dev/null || true + + # Path-based xattr calls on src/dst basenames = the TOCTOU pattern. + cross_src_base=$(basename "$cross_src") + cross_dst_base=$(basename "$cross_dst") + if grep -qE "(listxattr|getxattr|setxattr)\([^,]*($cross_src_base|$cross_dst_base)" strace_mv_xattr.log; then + cat strace_mv_xattr.log + fail_immediately "mv cross-device must use fd-based xattr ops (issue #10014)" + fi + if grep -qE 'flistxattr|fgetxattr|fsetxattr' strace_mv_xattr.log; then + echo "OK: mv cross-device uses fd-based xattr syscalls" + else + echo "WARN: mv cross-device xattr check: no xattr syscalls observed (xattr may have been filtered - check filesystem support)" + fi + rm -f "$cross_dst" + else + echo "WARN: mv cross-device xattr check: TMPDIR does not support user xattrs; skipped" + fi + else + rm -f "$shm_probe" + echo "WARN: mv cross-device xattr check: /dev/shm does not support user xattrs; skipped" + fi + fi +fi + # mv cross-device symlink replacement must use *at syscalls against a # pinned parent fd (matches GNU's force_symlinkat) so a concurrent rename # of the parent directory cannot redirect the temp-and-rename dance, and