cp: parent-perm-race gnu fix (#6403)

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
+            build_dir(options, &local_to_target, false)?;
-            // 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)?;
             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<bool> {
     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;

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();
+    }
+}