diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index c48cfda52..b1f81ea56 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -2274,6 +2274,7 @@ fn handle_copy_mode( context, source_metadata, symlinked_files, + source_in_command_line, created_parent_dirs, )?; } @@ -2294,6 +2295,7 @@ fn handle_copy_mode( context, source_metadata, symlinked_files, + source_in_command_line, created_parent_dirs, )?; } @@ -2327,6 +2329,7 @@ fn handle_copy_mode( context, source_metadata, symlinked_files, + source_in_command_line, created_parent_dirs, )?; } @@ -2339,6 +2342,7 @@ fn handle_copy_mode( context, source_metadata, symlinked_files, + source_in_command_line, created_parent_dirs, )?; } @@ -2716,6 +2720,7 @@ fn handle_no_preserve_mode(options: &Options, org_mode: u32) -> u32 { /// Copy the file from `source` to `dest` either using the normal `fs::copy` or a /// copy-on-write scheme if --reflink is specified and the filesystem supports it. +#[allow(clippy::too_many_arguments)] fn copy_helper( source: &Path, dest: &Path, @@ -2723,6 +2728,7 @@ fn copy_helper( context: &str, source_metadata: &Metadata, symlinked_files: &mut HashSet, + #[cfg_attr(not(unix), allow(unused_variables))] source_in_command_line: bool, created_parent_dirs: &mut HashSet, ) -> CopyResult<()> { if options.parents { @@ -2753,6 +2759,14 @@ fn copy_helper( if source_metadata.is_symlink() { copy_link(source, dest, symlinked_files, options)?; } else { + // Use O_NOFOLLOW on the source open iff cp is in no-dereference mode. + // In that case source_metadata was obtained via lstat, so a path swap + // to a symlink between lstat and open must be refused to close the + // TOCTOU window described in issue #10017. In deref mode cp + // intentionally follows symlinks, matching GNU cp's behavior of + // applying O_NOFOLLOW here only with `-P`. + #[cfg(unix)] + let nofollow = !options.dereference(source_in_command_line); let copy_debug = copy_on_write( source, dest, @@ -2761,6 +2775,8 @@ fn copy_helper( context, #[cfg(unix)] is_stream(source_metadata), + #[cfg(unix)] + nofollow, )?; if !options.attributes_only && options.debug { diff --git a/src/uu/cp/src/platform/linux.rs b/src/uu/cp/src/platform/linux.rs index 694979220..328c3f4a1 100644 --- a/src/uu/cp/src/platform/linux.rs +++ b/src/uu/cp/src/platform/linux.rs @@ -2,17 +2,17 @@ // // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore ficlone reflink ftruncate pwrite fiemap lseek +// spell-checker:ignore ficlone reflink ftruncate pwrite fiemap lseek nofollow use rustix::fs::{SeekFrom, ftruncate, ioctl_ficlone, seek}; -use std::fs::File; use std::io::Read; use std::os::unix::fs::FileExt; use std::os::unix::fs::FileTypeExt; use std::os::unix::fs::MetadataExt; use std::path::Path; + use uucore::buf_copy; -use uucore::safe_copy::{create_dest_restrictive, safe_copy_file}; +use uucore::safe_copy::{create_dest_restrictive, open_source}; use uucore::translate; use crate::{ @@ -20,6 +20,20 @@ use crate::{ is_stream, }; +// Replacement for `std::fs::copy` that uses the safe-copy primitives but +// only applies `O_NOFOLLOW` to the *source* open. The destination is +// followed if it is a pre-existing symlink, matching GNU cp -d/-P which +// only forbid dereferencing on the source side. +fn fs_copy(source: P, dest: Q, source_nofollow: bool) -> std::io::Result +where + P: AsRef, + Q: AsRef, +{ + let mut src = open_source(source, source_nofollow)?; + let mut dst = create_dest_restrictive(dest, false)?; + std::io::copy(&mut src, &mut dst) +} + /// The fallback behavior for [`clone`] on failed system call. #[derive(Clone, Copy)] enum CloneFallback { @@ -53,18 +67,20 @@ enum CopyMethod { /// /// `fallback` controls what to do if the system call fails. #[cfg(any(target_os = "linux", target_os = "android"))] -fn clone

(source: P, dest: P, fallback: CloneFallback) -> std::io::Result<()> +fn clone

(source: P, dest: P, fallback: CloneFallback, nofollow: bool) -> std::io::Result<()> where P: AsRef, { - let src_file = File::open(&source)?; + let src_file = open_source(&source, nofollow)?; let dst_file = create_dest_restrictive(&dest, false)?; if ioctl_ficlone(dst_file, src_file).is_err() { return match fallback { CloneFallback::Error => Err(std::io::Error::last_os_error()), - CloneFallback::FSCopy => safe_copy_file(source, dest, false).map(|_| ()), - CloneFallback::SparseCopy => sparse_copy(source, dest), - CloneFallback::SparseCopyWithoutHole => sparse_copy_without_hole(source, dest), + CloneFallback::FSCopy => fs_copy(source, dest, nofollow).map(|_| ()), + CloneFallback::SparseCopy => sparse_copy(source, dest, nofollow), + CloneFallback::SparseCopyWithoutHole => { + sparse_copy_without_hole(source, dest, nofollow) + } }; } Ok(()) @@ -74,8 +90,8 @@ where /// This function returns a tuple of (bool, u64, u64) signifying a tuple of (whether a file has /// data, its size, no of blocks it has allocated in disk) #[cfg(any(target_os = "linux", target_os = "android"))] -fn check_for_data(source: &Path) -> Result<(bool, u64, u64), std::io::Error> { - let mut src_file = File::open(source)?; +fn check_for_data(source: &Path, nofollow: bool) -> Result<(bool, u64, u64), std::io::Error> { + let mut src_file = open_source(source, nofollow)?; let metadata = src_file.metadata()?; let size = metadata.size(); @@ -94,8 +110,8 @@ fn check_for_data(source: &Path) -> Result<(bool, u64, u64), std::io::Error> { #[cfg(any(target_os = "linux", target_os = "android"))] /// Checks whether a file is sparse i.e. it contains holes, uses the crude heuristic blocks < size / 512 /// Reference:`` -fn check_sparse_detection(source: &Path) -> Result { - let src_file = File::open(source)?; +fn check_sparse_detection(source: &Path, nofollow: bool) -> Result { + let src_file = open_source(source, nofollow)?; let metadata = src_file.metadata()?; let size = metadata.size(); let blocks = metadata.blocks(); @@ -109,11 +125,11 @@ fn check_sparse_detection(source: &Path) -> Result { /// Optimized [`sparse_copy`] doesn't create holes for large sequences of zeros in non `sparse_files` /// Used when `--sparse=auto` #[cfg(any(target_os = "linux", target_os = "android"))] -fn sparse_copy_without_hole

(source: P, dest: P) -> std::io::Result<()> +fn sparse_copy_without_hole

(source: P, dest: P, nofollow: bool) -> std::io::Result<()> where P: AsRef, { - let src_file = File::open(source)?; + let src_file = open_source(source, nofollow)?; let dst_file = create_dest_restrictive(dest, false)?; let size = src_file.metadata()?.size(); @@ -144,11 +160,11 @@ where /// Perform a sparse copy from one file to another. /// Creates a holes for large sequences of zeros in `non_sparse_files`, used for `--sparse=always` #[cfg(any(target_os = "linux", target_os = "android"))] -fn sparse_copy

(source: P, dest: P) -> std::io::Result<()> +fn sparse_copy

(source: P, dest: P, nofollow: bool) -> std::io::Result<()> where P: AsRef, { - let mut src_file = File::open(source)?; + let mut src_file = open_source(source, nofollow)?; let dst_file = create_dest_restrictive(dest, false)?; let size: usize = src_file.metadata()?.size().try_into().unwrap(); @@ -185,7 +201,7 @@ fn check_dest_is_fifo(dest: &Path) -> bool { } /// Copy the contents of a stream from `source` to `dest`. -fn copy_stream

(source: P, dest: P) -> std::io::Result<()> +fn copy_stream

(source: P, dest: P, nofollow: bool) -> std::io::Result<()> where P: AsRef, { @@ -207,7 +223,7 @@ where // // TODO Update the code below to respect the case where // `--preserve=ownership` is not true. - let mut src_file = File::open(&source)?; + let mut src_file = open_source(&source, nofollow)?; // Use the same restrictive initial mode as the regular file path so that // the dest does not momentarily sit with broader perms. The `0o622 & // !umask` form previously used here could still allow group/other write @@ -234,6 +250,7 @@ pub(crate) fn copy_on_write( sparse_mode: SparseMode, context: &str, source_is_stream: bool, + nofollow: bool, ) -> CopyResult { let mut copy_debug = CopyDebug { offload: OffloadReflinkDebug::Unknown, @@ -247,18 +264,18 @@ pub(crate) fn copy_on_write( copy_debug.reflink = OffloadReflinkDebug::No; if source_is_stream { copy_debug.offload = OffloadReflinkDebug::Avoided; - copy_stream(source, dest).map(|_| ()) + copy_stream(source, dest, nofollow).map(|_| ()) } else { let mut copy_method = CopyMethod::Default; - let result = handle_reflink_never_sparse_always(source, dest); + let result = handle_reflink_never_sparse_always(source, dest, nofollow); if let Ok((debug, method)) = result { copy_debug = debug; copy_method = method; } match copy_method { - CopyMethod::FSCopy => safe_copy_file(source, dest, false).map(|_| ()), - _ => sparse_copy(source, dest), + CopyMethod::FSCopy => fs_copy(source, dest, nofollow).map(|_| ()), + _ => sparse_copy(source, dest, nofollow), } } } @@ -267,13 +284,13 @@ pub(crate) fn copy_on_write( if source_is_stream { copy_debug.offload = OffloadReflinkDebug::Avoided; - copy_stream(source, dest).map(|_| ()) + copy_stream(source, dest, nofollow).map(|_| ()) } else { - let result = handle_reflink_never_sparse_never(source); + let result = handle_reflink_never_sparse_never(source, nofollow); if let Ok(debug) = result { copy_debug = debug; } - safe_copy_file(source, dest, false).map(|_| ()) + fs_copy(source, dest, nofollow).map(|_| ()) } } (ReflinkMode::Never, SparseMode::Auto) => { @@ -281,18 +298,20 @@ pub(crate) fn copy_on_write( if source_is_stream { copy_debug.offload = OffloadReflinkDebug::Avoided; - copy_stream(source, dest).map(|_| ()) + copy_stream(source, dest, nofollow).map(|_| ()) } else { let mut copy_method = CopyMethod::Default; - let result = handle_reflink_never_sparse_auto(source, dest); + let result = handle_reflink_never_sparse_auto(source, dest, nofollow); if let Ok((debug, method)) = result { copy_debug = debug; copy_method = method; } match copy_method { - CopyMethod::SparseCopyWithoutHole => sparse_copy_without_hole(source, dest), - _ => safe_copy_file(source, dest, false).map(|_| ()), + CopyMethod::SparseCopyWithoutHole => { + sparse_copy_without_hole(source, dest, nofollow) + } + _ => fs_copy(source, dest, nofollow).map(|_| ()), } } } @@ -301,18 +320,18 @@ pub(crate) fn copy_on_write( // SparseMode::Always if source_is_stream { copy_debug.offload = OffloadReflinkDebug::Avoided; - copy_stream(source, dest).map(|_| ()) + copy_stream(source, dest, nofollow).map(|_| ()) } else { let mut copy_method = CopyMethod::Default; - let result = handle_reflink_auto_sparse_always(source, dest); + let result = handle_reflink_auto_sparse_always(source, dest, nofollow); if let Ok((debug, method)) = result { copy_debug = debug; copy_method = method; } match copy_method { - CopyMethod::FSCopy => clone(source, dest, CloneFallback::FSCopy), - _ => clone(source, dest, CloneFallback::SparseCopy), + CopyMethod::FSCopy => clone(source, dest, CloneFallback::FSCopy, nofollow), + _ => clone(source, dest, CloneFallback::SparseCopy, nofollow), } } } @@ -321,23 +340,23 @@ pub(crate) fn copy_on_write( copy_debug.reflink = OffloadReflinkDebug::No; if source_is_stream { copy_debug.offload = OffloadReflinkDebug::Avoided; - copy_stream(source, dest).map(|_| ()) + copy_stream(source, dest, nofollow).map(|_| ()) } else { - let result = handle_reflink_auto_sparse_never(source); + let result = handle_reflink_auto_sparse_never(source, nofollow); if let Ok(debug) = result { copy_debug = debug; } - clone(source, dest, CloneFallback::FSCopy) + clone(source, dest, CloneFallback::FSCopy, nofollow) } } (ReflinkMode::Auto, SparseMode::Auto) => { if source_is_stream { copy_debug.offload = OffloadReflinkDebug::Unsupported; - copy_stream(source, dest).map(|_| ()) + copy_stream(source, dest, nofollow).map(|_| ()) } else { let mut copy_method = CopyMethod::Default; - let result = handle_reflink_auto_sparse_auto(source, dest); + let result = handle_reflink_auto_sparse_auto(source, dest, nofollow); if let Ok((debug, method)) = result { copy_debug = debug; copy_method = method; @@ -345,9 +364,9 @@ pub(crate) fn copy_on_write( match copy_method { CopyMethod::SparseCopyWithoutHole => { - clone(source, dest, CloneFallback::SparseCopyWithoutHole) + clone(source, dest, CloneFallback::SparseCopyWithoutHole, nofollow) } - _ => clone(source, dest, CloneFallback::FSCopy), + _ => clone(source, dest, CloneFallback::FSCopy, nofollow), } } } @@ -356,7 +375,7 @@ pub(crate) fn copy_on_write( copy_debug.sparse_detection = SparseDebug::No; copy_debug.reflink = OffloadReflinkDebug::Yes; - clone(source, dest, CloneFallback::Error) + clone(source, dest, CloneFallback::Error, nofollow) } (ReflinkMode::Always, _) => { return Err(translate!("cp-error-reflink-always-sparse-auto").into()); @@ -371,6 +390,7 @@ pub(crate) fn copy_on_write( fn handle_reflink_auto_sparse_always( source: &Path, dest: &Path, + nofollow: bool, ) -> Result<(CopyDebug, CopyMethod), std::io::Error> { let mut copy_debug = CopyDebug { offload: OffloadReflinkDebug::Unknown, @@ -378,8 +398,8 @@ fn handle_reflink_auto_sparse_always( sparse_detection: SparseDebug::Zeros, }; let mut copy_method = CopyMethod::Default; - let (data_flag, size, blocks) = check_for_data(source)?; - let sparse_flag = check_sparse_detection(source)?; + let (data_flag, size, blocks) = check_for_data(source, nofollow)?; + let sparse_flag = check_sparse_detection(source, nofollow)?; if data_flag || size < 512 { copy_debug.offload = OffloadReflinkDebug::Avoided; @@ -408,14 +428,17 @@ fn handle_reflink_auto_sparse_always( /// Handles debug results when flags are "--reflink=auto" and "--sparse=auto" and specifies what /// type of copy should be used -fn handle_reflink_never_sparse_never(source: &Path) -> Result { +fn handle_reflink_never_sparse_never( + source: &Path, + nofollow: bool, +) -> Result { let mut copy_debug = CopyDebug { offload: OffloadReflinkDebug::Unknown, reflink: OffloadReflinkDebug::No, sparse_detection: SparseDebug::No, }; - let (data_flag, size, _blocks) = check_for_data(source)?; - let sparse_flag = check_sparse_detection(source)?; + let (data_flag, size, _blocks) = check_for_data(source, nofollow)?; + let sparse_flag = check_sparse_detection(source, nofollow)?; if sparse_flag { copy_debug.sparse_detection = SparseDebug::SeekHole; @@ -429,15 +452,18 @@ fn handle_reflink_never_sparse_never(source: &Path) -> Result Result { +fn handle_reflink_auto_sparse_never( + source: &Path, + nofollow: bool, +) -> Result { let mut copy_debug = CopyDebug { offload: OffloadReflinkDebug::Unknown, reflink: OffloadReflinkDebug::No, sparse_detection: SparseDebug::No, }; - let (data_flag, size, _blocks) = check_for_data(source)?; - let sparse_flag = check_sparse_detection(source)?; + let (data_flag, size, _blocks) = check_for_data(source, nofollow)?; + let sparse_flag = check_sparse_detection(source, nofollow)?; if sparse_flag { copy_debug.sparse_detection = SparseDebug::SeekHole; @@ -454,6 +480,7 @@ fn handle_reflink_auto_sparse_never(source: &Path) -> Result Result<(CopyDebug, CopyMethod), std::io::Error> { let mut copy_debug = CopyDebug { offload: OffloadReflinkDebug::Unknown, @@ -462,8 +489,8 @@ fn handle_reflink_auto_sparse_auto( }; let mut copy_method = CopyMethod::Default; - let (data_flag, size, blocks) = check_for_data(source)?; - let sparse_flag = check_sparse_detection(source)?; + let (data_flag, size, blocks) = check_for_data(source, nofollow)?; + let sparse_flag = check_sparse_detection(source, nofollow)?; if (data_flag && size != 0) || (size > 0 && size < 512) { copy_debug.offload = OffloadReflinkDebug::Yes; @@ -497,6 +524,7 @@ fn handle_reflink_auto_sparse_auto( fn handle_reflink_never_sparse_auto( source: &Path, dest: &Path, + nofollow: bool, ) -> Result<(CopyDebug, CopyMethod), std::io::Error> { let mut copy_debug = CopyDebug { offload: OffloadReflinkDebug::Unknown, @@ -504,8 +532,8 @@ fn handle_reflink_never_sparse_auto( sparse_detection: SparseDebug::No, }; - let (data_flag, size, blocks) = check_for_data(source)?; - let sparse_flag = check_sparse_detection(source)?; + let (data_flag, size, blocks) = check_for_data(source, nofollow)?; + let sparse_flag = check_sparse_detection(source, nofollow)?; let mut copy_method = CopyMethod::Default; if data_flag || size < 512 { @@ -533,6 +561,7 @@ fn handle_reflink_never_sparse_auto( fn handle_reflink_never_sparse_always( source: &Path, dest: &Path, + nofollow: bool, ) -> Result<(CopyDebug, CopyMethod), std::io::Error> { let mut copy_debug = CopyDebug { offload: OffloadReflinkDebug::Unknown, @@ -541,8 +570,8 @@ fn handle_reflink_never_sparse_always( }; let mut copy_method = CopyMethod::SparseCopy; - let (data_flag, size, blocks) = check_for_data(source)?; - let sparse_flag = check_sparse_detection(source)?; + let (data_flag, size, blocks) = check_for_data(source, nofollow)?; + let sparse_flag = check_sparse_detection(source, nofollow)?; if data_flag || size < 512 { copy_debug.offload = OffloadReflinkDebug::Avoided; diff --git a/src/uu/cp/src/platform/macos.rs b/src/uu/cp/src/platform/macos.rs index a545c552b..54e04b2bc 100644 --- a/src/uu/cp/src/platform/macos.rs +++ b/src/uu/cp/src/platform/macos.rs @@ -28,6 +28,7 @@ pub(crate) fn copy_on_write( sparse_mode: SparseMode, context: &str, source_is_stream: bool, + _nofollow: bool, ) -> CopyResult { if sparse_mode != SparseMode::Auto { return Err(translate!("cp-error-sparse-not-supported") @@ -69,8 +70,19 @@ pub(crate) fn copy_on_write( { // clonefile(2) fails if the destination exists. Remove it and try again. Do not // bother to check if removal worked because we're going to try to clone again. - // first lets make sure the dest file is not read only - if fs::metadata(dest).is_ok_and(|md| !md.permissions().readonly()) { + // first lets make sure the dest file is not read only. + // + // If dest is a symlink, GNU cp follows it and writes through to + // the target rather than replacing the link itself. Removing + // dest here would unlink the symlink and the retry would + // clonefile a regular file in its place. Skip the retry — the + // AlreadyExists error stays in `error` and we fall through to + // fs::copy below, which follows the symlink via O_TRUNC. + let dest_is_symlink = + fs::symlink_metadata(dest).is_ok_and(|md| md.file_type().is_symlink()); + if !dest_is_symlink + && fs::metadata(dest).is_ok_and(|md| !md.permissions().readonly()) + { // remove and copy again // TODO: rewrite this to better match linux behavior // linux first opens the source file and destination file then uses the file diff --git a/src/uu/cp/src/platform/other_unix.rs b/src/uu/cp/src/platform/other_unix.rs index ee87c2ac6..cfd3c716d 100644 --- a/src/uu/cp/src/platform/other_unix.rs +++ b/src/uu/cp/src/platform/other_unix.rs @@ -3,11 +3,10 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. // spell-checker:ignore reflink -use std::fs::File; use std::path::Path; use uucore::buf_copy; -use uucore::safe_copy::create_dest_restrictive; +use uucore::safe_copy::{create_dest_restrictive, open_source}; use uucore::translate; use crate::{ @@ -23,6 +22,7 @@ pub(crate) fn copy_on_write( sparse_mode: SparseMode, context: &str, source_is_stream: bool, + nofollow: bool, ) -> CopyResult { if reflink_mode != ReflinkMode::Never { return Err(translate!("cp-error-reflink-not-supported") @@ -41,8 +41,10 @@ pub(crate) fn copy_on_write( }; if source_is_stream { - let mut src_file = File::open(source)?; - let mut dst_file = create_dest_restrictive(dest, false)?; + let mut src_file = open_source(source, nofollow) + .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; + let mut dst_file = create_dest_restrictive(dest, false) + .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; let dest_is_stream = is_stream(&dst_file.metadata()?); if !dest_is_stream { @@ -57,13 +59,14 @@ pub(crate) fn copy_on_write( return Ok(copy_debug); } - // Equivalent of fs::copy but creates dest with DEST_INITIAL_MODE rather - // than the default umask-derived 0o666, closing the window where another - // user could read/write dest before cp applies the final permissions. + // Replacement for fs::copy: restrictive 0o600 dest mode (#10011) and + // O_NOFOLLOW on the *source* under -P (#10017). The destination open + // intentionally does not use O_NOFOLLOW so that an existing symlink at + // dest is followed, matching GNU cp. let mut src_file = - File::open(source).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; - let mut dst_file = - create_dest_restrictive(dest, false).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; + open_source(source, nofollow).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; + let mut dst_file = create_dest_restrictive(dest, false) + .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; std::io::copy(&mut src_file, &mut dst_file) .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 3eda89c6c..8ca8d1889 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -3036,8 +3036,6 @@ fn test_cp_symlink_overwrite_detection() { .fails() .stderr_only(if cfg!(target_os = "windows") { "cp: will not copy 'good/README' through just-created symlink 'tmp\\README'\n" - } else if cfg!(target_os = "macos") { - "cp: will not overwrite just-created 'tmp/README' with 'good/README'\n" } else { "cp: will not copy 'good/README' through just-created symlink 'tmp/README'\n" }); @@ -7899,3 +7897,92 @@ fn test_cp_final_mode_unchanged_after_restrictive_create() { "dst final mode should match source & ~umask (got {mode:o})" ); } + +// Sanity check for the `-P` happy path: a symlink source is copied as a +// symlink, not by following it. The actual `O_NOFOLLOW` invariant for +// issue #10017 (path swap to a symlink between lstat and open) cannot be +// raced deterministically from a unit test; that is locked in by the +// strace check in util/check-safe-traversal.sh, which fails if a future +// change drops `O_NOFOLLOW` from the source open under `-P`. +#[test] +#[cfg(unix)] +fn test_cp_no_dereference_copies_symlink_as_symlink() { + let (at, mut ucmd) = at_and_ucmd!(); + at.write("target", "secret target contents"); + at.symlink_file("target", "src_link"); + + ucmd.arg("-P").arg("src_link").arg("dst").succeeds(); + assert!(at.symlink_exists("dst")); + assert!(at.read_symlink("dst").ends_with("target")); +} + +// Regression for GNU tests/cp/deref-slink: when the destination exists as +// a symlink, `cp -d` (which implies --no-dereference for the source) must +// still follow the destination symlink and overwrite the link's target. +// `-P`/`-d` only forbids dereferencing on the source side; applying +// O_NOFOLLOW to the dest open broke this and surfaced as ELOOP. +#[test] +#[cfg(unix)] +fn test_cp_d_overwrites_existing_symlink_dest() { + let (at, mut ucmd) = at_and_ucmd!(); + at.touch("f"); + at.touch("slink-target"); + at.symlink_file("slink-target", "slink"); + + ucmd.arg("-d").arg("f").arg("slink").succeeds(); + + // The destination symlink itself remains a symlink (GNU follows it + // through to the target rather than replacing it). + assert!(at.symlink_exists("slink")); + assert!(at.read_symlink("slink").ends_with("slink-target")); +} + +// Regression for GNU tests/cp/acl: `cp -p` must preserve POSIX ACLs on +// Linux. ACLs are part of GNU's `mode` preservation, not its `xattr` +// preservation, so the default-no-xattr change in #9704 must not strip +// them. Only runs when `setfacl` is available so non-ACL filesystems and +// non-Linux CI do not flag spurious failures. +#[test] +#[cfg(target_os = "linux")] +fn test_cp_p_preserves_posix_acls() { + use std::process::Command; + + if Command::new("setfacl").arg("--version").output().is_err() { + return; + } + if Command::new("getfacl").arg("--version").output().is_err() { + return; + } + + let (at, mut ucmd) = at_and_ucmd!(); + at.touch("src"); + + let setfacl = Command::new("setfacl") + .arg("-m") + .arg("user:bin:rw-") + .arg(at.plus("src")) + .status(); + let Ok(status) = setfacl else { return }; + if !status.success() { + // Filesystem doesn't support ACLs; skip. + return; + } + + ucmd.arg("-p").arg("src").arg("dst").succeeds(); + + let src_acl = Command::new("getfacl") + .arg("--omit-header") + .arg(at.plus("src")) + .output() + .unwrap(); + let dst_acl = Command::new("getfacl") + .arg("--omit-header") + .arg(at.plus("dst")) + .output() + .unwrap(); + assert_eq!( + String::from_utf8_lossy(&src_acl.stdout), + String::from_utf8_lossy(&dst_acl.stdout), + "cp -p must preserve POSIX ACLs (GNU tests/cp/acl regression)", + ); +} diff --git a/util/check-safe-traversal.sh b/util/check-safe-traversal.sh index f4bb783e5..ae9dc28d8 100755 --- a/util/check-safe-traversal.sh +++ b/util/check-safe-traversal.sh @@ -233,27 +233,40 @@ if echo "$AVAILABLE_UTILS" | grep -q "mv"; then check_utility "mv" "openat,renameat,newfstatat,rename" "openat" "test_mv_src test_mv_dst" "move_directory" fi -# Test cp destination creation - must use a restrictive initial mode (0o600) -# so the destination is never briefly readable by other users on a shared -# directory before cp applies the final permissions. See issue #10011. +# cp invariant checks. Both #10011 (restrictive 0600 destination mode) and +# #10017 (O_NOFOLLOW on the -P source) need to hold; verify each on its own +# strace. if echo "$AVAILABLE_UTILS" | grep -q "cp"; then - echo "cp_perm_test" > test_cp_src_perm if [ "$USE_MULTICALL" -eq 1 ]; then cp_cmd="$COREUTILS_BIN cp" else cp_cmd="$PROJECT_ROOT/target/${PROFILE}/cp" fi + + # #10011: destination created with mode 0600 so other users cannot open + # the file through its umask-derived initial mode before cp narrows it. + echo "cp_perm_test" > test_cp_src_perm rm -f test_cp_dst_perm strace -f -e trace=openat -o strace_cp_dest_perm.log \ $cp_cmd test_cp_src_perm test_cp_dst_perm 2>/dev/null || true - # The creation openat should carry mode 0600. Any wider mode (e.g. 0666 - # masked by umask) reopens the window #10011 closed. if ! grep -qE 'openat\(AT_FDCWD, "test_cp_dst_perm".*O_CREAT.*, 0600\)' strace_cp_dest_perm.log; then cat strace_cp_dest_perm.log fail_immediately "cp must create the destination with mode 0600 (issue #10011)" fi echo "✓ cp creates destination with restrictive 0600 mode" rm -f test_cp_src_perm test_cp_dst_perm + + # #10017: -P opens source with O_NOFOLLOW so a path swap to a symlink + # between the lstat check and the open cannot redirect the copy. + echo "cp_nofollow_test" > test_cp_src + strace -f -e trace=openat -o strace_cp_nofollow.log \ + $cp_cmd -P test_cp_src test_cp_dst 2>/dev/null || true + if ! grep -qE 'openat\(AT_FDCWD, "test_cp_src".*O_NOFOLLOW' strace_cp_nofollow.log; then + cat strace_cp_nofollow.log + fail_immediately "cp -P must open the source with O_NOFOLLOW (issue #10017)" + fi + echo "✓ cp -P opens source with O_NOFOLLOW" + rm -f test_cp_src test_cp_dst fi echo ""