diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index 81114c5f0..43ca9f998 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -2,7 +2,7 @@ // // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore TODO canonicalizes direntry pathbuf symlinked +// spell-checker:ignore TODO canonicalizes direntry pathbuf symlinked IRWXO IRWXG //! Recursively copy the contents of a directory. //! //! See the [`copy_directory`] function for more information. @@ -246,12 +246,7 @@ fn copy_direntry( if target_is_file { return Err("cannot overwrite non-directory with directory".into()); } else { - // TODO Since the calling code is traversing from the root - // of the directory structure, I don't think - // `create_dir_all()` will have any benefit over - // `create_dir()`, since all the ancestor directories - // should have already been created. - fs::create_dir_all(&local_to_target)?; + build_dir(options, &local_to_target, false)?; if options.verbose { println!("{}", context_for(&source_relative, &local_to_target)); } @@ -376,8 +371,7 @@ pub(crate) fn copy_directory( let tmp = if options.parents { if let Some(parent) = root.parent() { let new_target = target.join(parent); - std::fs::create_dir_all(&new_target)?; - + build_dir(options, &new_target, true)?; if options.verbose { // For example, if copying file `a/b/c` and its parents // to directory `d/`, then print @@ -470,6 +464,42 @@ pub fn path_has_prefix(p1: &Path, p2: &Path) -> io::Result { Ok(pathbuf1.starts_with(pathbuf2)) } +/// Builds a directory at the specified path with the given options. +/// +/// # Notes +/// - It excludes certain permissions if ownership or special mode bits could +/// potentially change. +/// - The `recursive` flag determines whether parent directories should be created +/// if they do not already exist. +// we need to allow unused_variable since `options` might be unused in non unix systems +#[allow(unused_variables)] +fn build_dir(options: &Options, path: &PathBuf, recursive: bool) -> CopyResult<()> { + let mut builder = fs::DirBuilder::new(); + builder.recursive(recursive); + // To prevent unauthorized access before the folder is ready, + // exclude certain permissions if ownership or special mode bits + // could potentially change. + #[cfg(unix)] + { + // we need to allow trivial casts here because some systems like linux have u32 constants in + // in libc while others don't. + #[allow(clippy::unnecessary_cast)] + let mut excluded_perms = + if matches!(options.attributes.ownership, crate::Preserve::Yes { .. }) { + libc::S_IRWXG | libc::S_IRWXO // exclude rwx for group and other + } else if matches!(options.attributes.mode, crate::Preserve::Yes { .. }) { + libc::S_IWGRP | libc::S_IWOTH //exclude w for group and other + } else { + 0 + } as u32; + excluded_perms |= uucore::mode::get_umask(); + let mode = !excluded_perms & 0o777; //use only the last three octet bits + std::os::unix::fs::DirBuilderExt::mode(&mut builder, mode); + } + builder.create(path)?; + Ok(()) +} + #[cfg(test)] mod tests { use super::ends_with_slash_dot; diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 4d0602f63..a9032f215 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.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 (flags) reflink (fs) tmpfs (linux) rlimit Rlim NOFILE clob btrfs neve ROOTDIR USERDIR procfs outfile uufs xattrs -// spell-checker:ignore bdfl hlsl +// spell-checker:ignore bdfl hlsl IRWXO IRWXG use crate::common::util::TestScenario; #[cfg(not(windows))] use std::fs::set_permissions; @@ -5438,3 +5438,67 @@ mod link_deref { } } } + +// The cp command might create directories with excessively permissive permissions temporarily, +// which could be problematic if we aim to preserve ownership or mode. For example, when +// copying a directory, the destination directory could temporarily be setgid on some filesystems. +// This temporary setgid status could grant access to other users who share the same group +// ownership as the newly created directory.To mitigate this issue, when creating a directory we +// disable these excessive permissions. +#[test] +#[cfg(unix)] +fn test_dir_perm_race_with_preserve_mode_and_ownership() { + const SRC_DIR: &str = "src"; + const DEST_DIR: &str = "dest"; + const FIFO: &str = "src/fifo"; + for attr in ["mode", "ownership"] { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir(SRC_DIR); + at.mkdir(DEST_DIR); + at.set_mode(SRC_DIR, 0o775); + at.set_mode(DEST_DIR, 0o2775); + at.mkfifo(FIFO); + let child = scene + .ucmd() + .args(&[ + format!("--preserve={}", attr).as_str(), + "-R", + "--copy-contents", + "--parents", + SRC_DIR, + DEST_DIR, + ]) + // make sure permissions weren't disabled because of umask. + .umask(0) + .run_no_wait(); + // while cp wait for fifo we could check the dirs created by cp + let timeout = Duration::from_secs(10); + let start_time = std::time::Instant::now(); + // wait for cp to create dirs + loop { + if start_time.elapsed() >= timeout { + panic!("timed out: cp took too long to create destination directory") + } + if at.dir_exists(&format!("{}/{}", DEST_DIR, SRC_DIR)) { + break; + } + std::thread::sleep(Duration::from_millis(100)); + } + let mode = at.metadata(&format!("{}/{}", DEST_DIR, SRC_DIR)).mode(); + #[allow(clippy::unnecessary_cast)] + let mask = if attr == "mode" { + libc::S_IWGRP | libc::S_IWOTH + } else { + libc::S_IRWXG | libc::S_IRWXO + } as u32; + assert_eq!( + (mode & mask), + 0, + "unwanted permissions are present - {}", + attr + ); + at.write(FIFO, "done"); + child.wait().unwrap().succeeded(); + } +}