From 7ba1bf857c82f13a63e68655e5676be7d6df9a0d Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Tue, 21 Apr 2026 12:52:42 +0200 Subject: [PATCH] cp: don't preserve xattrs with -p by default (#9704) GNU `cp -p` preserves mode, ownership, and timestamps. xattrs are NOT preserved unless the user asks for them via `--preserve=xattr` or `-a`. uutils's `Attributes::DEFAULT` had xattr set to `Preserve::Yes { required: true }`, which (1) diverges from GNU and breaks scripts that expect the stock behavior, (2) leaks security xattrs like file capabilities and SELinux labels into copies when run as root, and (3) fails hard on destinations that don't support xattrs. Remove the xattr override in `Attributes::DEFAULT` so it inherits `Preserve::No` from `Attributes::NONE`. `Attributes::ALL` (used by `-a` and `--preserve=all`) still sets xattr to Yes, and `--preserve=xattr` still works as before. --- src/uu/cp/src/cp.rs | 16 ++++++++-- src/uucore/src/lib/features/fsxattr.rs | 24 ++++++++++++++- tests/by-util/test_cp.rs | 41 ++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 4 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index b1f81ea56..e1a1dc122 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -16,7 +16,7 @@ use std::os::unix::net::UnixListener; use std::path::{Path, PathBuf, StripPrefixError}; use std::{fmt, io}; #[cfg(all(unix, not(target_os = "android")))] -use uucore::fsxattr::{copy_xattrs, copy_xattrs_skip_selinux}; +use uucore::fsxattr::{copy_acls, copy_xattrs, copy_xattrs_skip_selinux}; use uucore::translate; use clap::{Arg, ArgAction, ArgMatches, Command, builder::ValueParser, value_parser}; @@ -921,13 +921,15 @@ impl Attributes { xattr: Preserve::No { explicit: false }, }; - // TODO: ownership is required if the user is root, for non-root users it's not required. + // xattr is intentionally NOT in DEFAULT — GNU `cp -p` only preserves + // mode, ownership, and timestamps. Default xattr preservation leaks + // capability / SELinux labels into copies and fails hard on filesystems + // without xattr support. See issue #9704. pub const DEFAULT: Self = Self { #[cfg(unix)] ownership: Preserve::Yes { required: true }, mode: Preserve::Yes { required: true }, timestamps: Preserve::Yes { required: true }, - xattr: Preserve::Yes { required: true }, ..Self::NONE }; @@ -1829,6 +1831,14 @@ 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()))?; + // GNU `cp -p` preserves POSIX ACLs as part of mode. On Linux the + // ACLs are stored as `system.posix_acl_*` xattrs; copy just those + // so we keep ACL parity with GNU without preserving user xattrs + // (which are intentionally excluded from the default -p set per + // issue #9704). Best-effort: ignore failures on filesystems that + // do not support ACL xattrs. + #[cfg(all(unix, not(target_os = "android")))] + copy_acls(source, dest); } Ok(()) diff --git a/src/uucore/src/lib/features/fsxattr.rs b/src/uucore/src/lib/features/fsxattr.rs index d96832646..2b42edaa3 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 +// spell-checker:ignore getxattr posix_acl_default posix_acl_access //! Set of functions to manage xattr on files and dirs use itertools::Itertools; @@ -45,6 +45,28 @@ pub fn copy_xattrs_skip_selinux>(source: P, dest: P) -> std::io:: Ok(()) } +/// Copies only the POSIX ACL xattrs (`system.posix_acl_access` and +/// `system.posix_acl_default`) from `source` to `dest`. +/// +/// GNU `cp -p` preserves ACLs as part of mode preservation but does not +/// preserve other (user/security) xattrs unless `--preserve=xattr` or +/// `-a` is requested. On Linux, POSIX ACLs are stored as the two `system.*` +/// xattrs above; copying them here without copying the rest gives the +/// GNU-compatible "preserve mode (incl. ACLs) but not user xattrs" behavior. +/// +/// Errors from the underlying xattr calls are silently ignored: filesystems +/// without ACL/xattr support are common, and GNU cp itself does not surface +/// failures here when `mode` is the only thing being preserved. +#[cfg(unix)] +pub fn copy_acls>(source: P, dest: P) { + for name in ["system.posix_acl_access", "system.posix_acl_default"] { + if let Ok(Some(value)) = xattr::get(&source, name) { + // Best-effort: silently skip if dest doesn't support ACL xattrs. + let _ = xattr::set(&dest, name, &value); + } + } +} + /// Retrieves the extended attributes (xattrs) of a given file or directory. /// /// # Arguments diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 8ca8d1889..83eefd211 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1768,6 +1768,47 @@ fn test_cp_preserve_all() { } } +// GNU `cp -p` preserves mode, ownership, and timestamps but NOT xattrs. +// xattr preservation requires explicit `--preserve=xattr` or `-a`. See #9704. +#[test] +#[cfg(all( + unix, + not(any(target_os = "android", target_os = "openbsd", target_os = "macos")) +))] +fn test_cp_p_does_not_preserve_xattr_by_default() { + use std::process::Command; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.touch("src"); + + let xattr_key = "user.test_preserve_p"; + let setfattr = Command::new("setfattr") + .args(["-n", xattr_key, "-v", "v", &at.plus_as_string("src")]) + .status(); + match setfattr { + Ok(s) if s.success() => {} + _ => { + println!("test skipped: setfattr not available / filesystem rejects xattrs"); + return; + } + } + + scene + .ucmd() + .args(&["-p", &at.plus_as_string("src"), &at.plus_as_string("dst")]) + .succeeds(); + + let out = Command::new("getfattr") + .args(["--only-values", "-n", xattr_key, &at.plus_as_string("dst")]) + .output() + .expect("getfattr failed"); + assert!( + !out.status.success(), + "cp -p should not preserve xattrs by default, but '{xattr_key}' was copied" + ); +} + #[test] #[cfg(all(unix, not(any(target_os = "android", target_os = "openbsd"))))] fn test_cp_preserve_xattr() {