From 13877ac3c2ccedf9c6f763a24920fe1891e64491 Mon Sep 17 00:00:00 2001 From: AnarchistHoneybun Date: Fri, 19 Dec 2025 09:39:54 +0530 Subject: [PATCH] cp: fix -p to not preserve xattrs by default Fixes #9704 The -p flag should only preserve mode, ownership, and timestamps, matching GNU cp behavior. Extended attributes require explicit --preserve=xattr or -a (archive mode). Changed Attributes::DEFAULT to set xattr preservation to No, preventing unintended security risks and filesystem compatibility issues. Adds regression test. --- Cargo.lock | 1 + src/uu/cp/Cargo.toml | 1 + src/uu/cp/src/cp.rs | 16 +++++- tests/by-util/test_cp.rs | 106 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 123 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 1e61b0ecff9..c7c4d0325fc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3411,6 +3411,7 @@ dependencies = [ "thiserror 2.0.18", "uucore", "walkdir", + "xattr", ] [[package]] diff --git a/src/uu/cp/Cargo.toml b/src/uu/cp/Cargo.toml index bea4afc67c2..5f2cb0003df 100644 --- a/src/uu/cp/Cargo.toml +++ b/src/uu/cp/Cargo.toml @@ -42,6 +42,7 @@ fluent = { workspace = true } [target.'cfg(unix)'.dependencies] exacl = { workspace = true, optional = true } nix = { workspace = true, features = ["fs"] } +xattr = { workspace = true } [[bin]] name = "cp" diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 9f023a3937f..312cff73371 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -927,7 +927,7 @@ impl Attributes { ownership: Preserve::Yes { required: true }, mode: Preserve::Yes { required: true }, timestamps: Preserve::Yes { required: true }, - xattr: Preserve::Yes { required: true }, + xattr: Preserve::No { explicit: false }, ..Self::NONE }; @@ -1831,6 +1831,20 @@ pub(crate) fn copy_attributes( exacl::getfacl(source, None) .and_then(|acl| exacl::setfacl(&[dest], &acl, None)) .map_err(|err| CpError::Error(err.to_string()))?; + // POSIX ACLs live in system.posix_acl_{access,default} xattrs and are + // part of "mode" in GNU cp semantics, so copy them even when the + // general xattr preservation is off and feat_acl is not compiled in. + #[cfg(all(unix, not(feature = "feat_acl")))] + for name in ["system.posix_acl_access", "system.posix_acl_default"] { + let value = xattr::get(source, name) + .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; + if let Some(value) = value + && let Err(e) = xattr::set(dest, name, &value) + && e.raw_os_error() != Some(libc::ENOTSUP) + { + return Err(CpError::IoErrContext(e, context.to_owned())); + } + } } Ok(()) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 28af3075eef..ca1c86029f9 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1802,6 +1802,112 @@ fn test_cp_preserve_xattr() { } } +// -p preserves mode, ownership, and timestamps, but not xattrs +// xattrs require explicit --preserve=xattr or -a +#[test] +#[cfg(all( + unix, + not(any(target_os = "android", target_os = "macos", target_os = "openbsd")) +))] +fn test_cp_preserve_default_no_xattr() { + use std::process::Command; + use uutests::util::compare_xattrs; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let source_file = "source_with_xattr"; + let dest_file_p = "dest_with_p_flag"; + let dest_file_explicit = "dest_with_explicit_xattr"; + + at.touch(source_file); + + // set xattr on source + let xattr_key = "user.test_preserve"; + let xattr_value = "test_value"; + match Command::new("setfattr") + .args([ + "-n", + xattr_key, + "-v", + xattr_value, + &at.plus_as_string(source_file), + ]) + .status() + .map(|status| status.code()) + { + Ok(Some(0)) => {} + Ok(_) => { + println!("test skipped: setfattr failed"); + return; + } + Err(e) => { + println!("test skipped: setfattr failed with {e}"); + return; + } + } + + // verify xattr was set + let getfattr_output = Command::new("getfattr") + .args([&at.plus_as_string(source_file)]) + .output() + .expect("failed to run getfattr"); + + assert!( + getfattr_output.status.success(), + "getfattr failed: {}", + String::from_utf8_lossy(&getfattr_output.stderr) + ); + + let stdout = String::from_utf8_lossy(&getfattr_output.stdout); + assert!( + stdout.contains(xattr_key), + "xattr not found on source: {xattr_key}" + ); + + // -p should not preserve xattrs + scene + .ucmd() + .args(&[ + "-p", + &at.plus_as_string(source_file), + &at.plus_as_string(dest_file_p), + ]) + .succeeds() + .no_output(); + + // xattrs should not be copied + assert!( + !compare_xattrs(&at.plus(source_file), &at.plus(dest_file_p)), + "cp -p incorrectly preserved xattrs" + ); + + // mode, ownership, and timestamps should be preserved + #[cfg(not(any(target_os = "freebsd", target_os = "macos")))] + { + let metadata_src = at.metadata(source_file); + let metadata_dst = at.metadata(dest_file_p); + assert_metadata_eq!(metadata_src, metadata_dst); + } + + // --preserve=xattr should explicitly preserve xattrs + scene + .ucmd() + .args(&[ + "--preserve=xattr", + &at.plus_as_string(source_file), + &at.plus_as_string(dest_file_explicit), + ]) + .succeeds() + .no_output(); + + // xattrs should be copied + assert!( + compare_xattrs(&at.plus(source_file), &at.plus(dest_file_explicit)), + "cp --preserve=xattr did not preserve xattrs" + ); +} + #[test] #[cfg(all(target_os = "linux", not(feature = "feat_selinux")))] fn test_cp_preserve_all_context_fails_on_non_selinux() {