Merge pull request from GHSA-mc8h-8q98-g5hr

* Fix TOCTOU races

This involves a new safe API that works from a file descriptor rather than a path.

The existing APIs are preserved but no longer recommended.

* Bump version to indicate API breaks

The crate features have changed, in particular parallel is now not
defaulted-on.

* Rearrange changelog

CHANGELOG.md

@@ -1,7 +1,68 @@
-# unreleased
+# 0.8.0
 
-- Added feature to enable use of rayon.
-- Migrated from winapi to windows-sys
+## Security changes
+
+- Fix TOCTOU race conditions both inside the implementation of functions and the
+  contract: functions now only operate on directories. Callers wanting to
+  process the contents of a symlink (e.g. for remove_dir_contents) should
+  resolve the symlink themselves. This is an API break from 0.7.0, but the previous behaviour was insecure.
+
+  This is due to the same code pattern as caused CVE-2022-21658 in Rust itself:
+  it was possible to trick a privileged process doing a recursive delete in an
+  attacker controlled directory into deleting privileged files, on all operating
+  systems.
+
+  For instance, consider deleting a tree called 'etc' in a parent directory
+  called 'p'. Between calling `remove_dir_all("a")` and remove_dir_all("a")
+  actually starting its work, the attacker can move 'p' to 'p-prime', and
+  replace 'p' with a symlink to '/'. Then the privileged process deletes 'p/etc'
+  which is actually /etc, and now your system is broken. There are some
+  mitigations for this exact scenario, such as CWD relative file lookup, but
+  they are not guaranteed - any code using absolute paths will not have that
+  protection in place.
+
+  The same attack could be performed at any point in the directory tree being
+  deleted: if 'a' contains a child directory called 'etc', attacking the
+  deletion by replacing 'a' with a link is possible.
+
+  The new code in this release mitigates the attack within the directory tree
+  being deleted by using file-handle relative operations: to open 'a/etc', the
+  path 'etc' relative to 'a' is opened, where 'a' is represented by a file
+  descriptor (Unix) or handle (Windows). With the exception of the entry points
+  into the directory deletion logic, this is robust against manipulation of the
+  directory hierarchy, and remove_dir_all will only delete files and directories
+  contained in the tree it is deleting.
+
+  The entry path however is a challenge - as described above, there are some
+  potential mitigations, but since using them must be done by the calling code,
+  it is hard to be confident about the security properties of the path based
+  interface.
+
+  The new extension trait `RemoveDir` provides an interface where it is much
+  harder to get it wrong.
+
+  `somedir.remove_dir_contents("name-of-child")`.
+
+  Callers can then make their own security evaluation about how to securely get
+  a directory handle. That is still not particularly obvious, and we're going to
+  follow up with a helper of some sort (probably in the `fs_at` crate). Once
+  that is available, the path based entry points will get deprecated.
+
+  In the interim, processes that might run with elevated privileges should
+  figure out how to securely identify the directory they are going to delete, to
+  avoid the initial race. Pragmatically, other processes should be fine with the
+  path based entry points : this is the same interface `std::fs::remove_dir_all`
+  offers, and an unprivileged process running in an attacker controlled
+  directory can't do anything that the attacker can't already do.
+
+  tl;dr: state shared with threat actors makes things dangerous; library
+  functions cannot assume anything about the particular threat model of a
+  program and must err on the side of caution.
+
+## Other changes
+
+- Made feature to control use of rayon off-by-default for easier integration by
+  other crates.
 
 # 0.5.2
 

Cargo.toml

@@ -1,48 +1,59 @@
 [package]
 authors = [
-  "Erin P. <xampprocky@gmail.com>",
-  "Robert C. <robertc@robertcollins.net>",
+    "Erin P. <xampprocky@gmail.com>",
+    "Robert C. <robertc@robertcollins.net>",
 ]
 categories = ["filesystem"]
 description = "A safe, reliable implementation of remove_dir_all for Windows"
 edition = "2018"
 include = [
-  "Cargo.toml",
-  "LICENCE-APACHE",
-  "LICENCE-MIT",
-  "src/**/*",
-  "README.md",
+    "Cargo.toml",
+    "LICENCE-APACHE",
+    "LICENCE-MIT",
+    "src/**/*",
+    "README.md",
 ]
 keywords = ["utility", "filesystem", "remove_dir", "windows"]
 license = "MIT OR Apache-2.0"
 name = "remove_dir_all"
 readme = "README.md"
 repository = "https://github.com/XAMPPRocky/remove_dir_all.git"
-version = "0.7.1-alpha.0"
+version = "0.8.0-alpha.0"
 
 [features]
-default = ["parallel"]
-parallel = ["rayon"]
+default = []
+log = ["dep:log"]
+parallel = ["dep:rayon"]
 
 [dependencies]
 cfg-if = "1.0.0"
+fs_at = { version = "0.1" }
+lazy_static = "1.4.0"
+log = { version = "0.4.11", optional = true }
+normpath = "1.0.1"
+rayon = { version = "1.4", optional = true }
 
 [target.'cfg(windows)'.dependencies]
-log = "0.4.11"
-rayon = { version = "1.4", optional = true }
+aligned = "0.4.1"
 
 [target.'cfg(windows)'.dependencies.windows-sys]
-version = "0.45.0"
 features = [
     "Win32_Foundation",
     "Win32_Storage_FileSystem",
+    "Win32_System_IO",
+    "Win32_System_Ioctl",
     "Win32_System_SystemServices",
+    "Win32_System_Threading",
 ]
+version = "0.45.0"
 
 [target.'cfg(not(windows))'.dependencies]
 libc = "0.2"
+cvt = "0.1.1"
 
 [dev-dependencies]
 doc-comment = "0.3"
-env_logger = "0.9.0"
+env_logger = "0.10.0"
 tempfile = "3.1"
+test-log = "0.2"
+log = "0.4.11"

src/_impl.rs

@@ -0,0 +1,157 @@
+use std::{
+    ffi::OsStr,
+    fs::File,
+    io::{ErrorKind, Result},
+    path::Path,
+};
+
+#[cfg(feature = "parallel")]
+use rayon::prelude::*;
+
+mod io;
+mod path_components;
+
+cfg_if::cfg_if! {
+    if #[cfg(windows)] {
+        mod win;
+        pub(crate) use win::WindowsIo as OsIo;
+    } else {
+        mod unix;
+        pub(crate) use unix::UnixIo as OsIo;
+    }
+}
+
+impl super::RemoveDir for std::fs::File {
+    fn remove_dir_contents(&mut self, debug_root: Option<&Path>) -> Result<()> {
+        // thunk over to the free version adding in the os-specific IO trait impl
+        let debug_root = match debug_root {
+            None => PathComponents::Path(Path::new("")),
+            Some(debug_root) => PathComponents::Path(debug_root),
+        };
+        _remove_dir_contents::<OsIo>(self, &debug_root)
+    }
+}
+
+/// Entry point for deprecated function
+pub(crate) fn _ensure_empty_dir_path<I: io::Io, P: AsRef<Path>>(path: P) -> Result<()> {
+    // This is as TOCTOU safe as we can make it. Attacks via link replacements
+    // in interior components of the path is still possible. if the create
+    // succeeds, mission accomplished. if the create fails, open the dir
+    // (subject to races again), and then proceed to delete the contents via the
+    // descriptor.
+    match std::fs::create_dir(&path) {
+        Err(e) if e.kind() == ErrorKind::AlreadyExists => {
+            // Exists and is a dir. Open it
+            let mut existing_dir = I::open_dir(path.as_ref())?;
+            existing_dir.remove_dir_contents(Some(path.as_ref()))
+        }
+        otherwise => otherwise,
+    }
+}
+
+// Deprecated entry point
+pub(crate) fn _remove_dir_contents_path<I: io::Io, P: AsRef<Path>>(path: P) -> Result<()> {
+    let mut d = I::open_dir(path.as_ref())?;
+    _remove_dir_contents::<I>(&mut d, &PathComponents::Path(path.as_ref()))
+}
+
+/// exterior lifetime interface to dir removal
+fn _remove_dir_contents<I: io::Io>(d: &mut File, debug_root: &PathComponents<'_>) -> Result<()> {
+    let owned_handle = I::duplicate_fd(d)?;
+    remove_dir_contents_recursive::<I>(owned_handle, debug_root)
+}
+
+/// deprecated interface
+pub(crate) fn remove_dir_all_path<I: io::Io, P: AsRef<Path>>(path: P) -> Result<()> {
+    let p = path.as_ref();
+    // Opportunity 1 for races
+    let d = I::open_dir(p)?;
+    let debug_root = PathComponents::Path(if p.has_root() { p } else { Path::new(".") });
+    remove_dir_contents_recursive::<OsIo>(d, &debug_root)?;
+    // Opportunity 2 for races
+    std::fs::remove_dir(&path)
+}
+
+use crate::RemoveDir;
+
+use self::path_components::PathComponents;
+
+// Core workhorse, heading towards this being able to be tasks.
+#[allow(clippy::map_identity)]
+fn remove_dir_contents_recursive<I: io::Io>(
+    mut d: File,
+    debug_root: &PathComponents<'_>,
+) -> Result<()> {
+    #[cfg(feature = "log")]
+    log::trace!("scanning {}", &debug_root);
+    // We take a os level clone of the FD so that there are no lifetime
+    // concerns. It would *not* be ok to do readdir on one file twice
+    // concurrently because of shared kernel state.
+    let dirfd = I::duplicate_fd(&mut d)?;
+    cfg_if::cfg_if! {
+        if #[cfg(feature = "parallel")] {
+            let iter = fs_at::read_dir(&mut d)?;
+            let iter = iter.par_bridge();
+        } else {
+            let mut iter = fs_at::read_dir(&mut d)?;
+        }
+    }
+
+    iter.try_for_each(|dir_entry| -> Result<()> {
+        let dir_entry = dir_entry?;
+        let name = dir_entry.name();
+        if name == OsStr::new(".") || name == OsStr::new("..") {
+            return Ok(());
+        }
+        let dir_path = Path::new(name);
+        let dir_debug_root = PathComponents::Component(debug_root, dir_path);
+        // Windows optimised: open everything always, which is not bad for
+        // linux, and portable to OS's and FS's that don't expose inode type in
+        // the readdir entries.
+
+        let mut opts = fs_at::OpenOptions::default();
+        opts.read(true)
+            .write(fs_at::OpenOptionsWriteMode::Write)
+            .follow(false);
+
+        let child_result = opts.open_dir_at(&dirfd, name);
+        let is_dir = match child_result {
+            Err(e) if !I::is_eloop(&e) => return Err(e),
+            Err(_) => false,
+            Ok(child_file) => {
+                let metadata = child_file.metadata()?;
+                let is_dir = metadata.is_dir();
+                I::clear_readonly(&child_file, &dir_debug_root, &metadata)?;
+
+                if is_dir {
+                    remove_dir_contents_recursive::<I>(child_file, &dir_debug_root)?;
+                    #[cfg(feature = "log")]
+                    log::trace!("rmdir: {}", &dir_debug_root);
+                    let opts = fs_at::OpenOptions::default();
+                    opts.rmdir_at(&dirfd, name).map_err(|e| {
+                        #[cfg(feature = "log")]
+                        log::debug!("error removing {}", dir_debug_root);
+                        e
+                    })?;
+                }
+                is_dir
+            }
+        };
+        if !is_dir {
+            #[cfg(feature = "log")]
+            log::trace!("unlink: {}", &dir_debug_root);
+            opts.unlink_at(&dirfd, name).map_err(|e| {
+                #[cfg(feature = "log")]
+                log::debug!("error removing {}", dir_debug_root);
+                e
+            })?;
+        }
+
+        #[cfg(feature = "log")]
+        log::trace!("removed {}", dir_debug_root);
+        Ok(())
+    })?;
+    #[cfg(feature = "log")]
+    log::trace!("scanned {}", &debug_root);
+    Ok(())
+}

src/_impl/io.rs

@@ -0,0 +1,27 @@
+//! Private trait to deal with OS variance
+
+use std::{
+    fmt::Debug,
+    fs::{File, Metadata},
+    io,
+    path::Path,
+};
+
+use super::path_components::PathComponents;
+
+pub(crate) trait Io {
+    type UniqueIdentifier: PartialEq + Debug;
+
+    fn duplicate_fd(f: &mut File) -> io::Result<File>;
+
+    fn open_dir(p: &Path) -> io::Result<File>;
+    fn unique_identifier(d: &File) -> io::Result<Self::UniqueIdentifier>;
+
+    fn clear_readonly(
+        f: &File,
+        dir_debug_root: &'_ PathComponents<'_>,
+        metadata: &Metadata,
+    ) -> io::Result<()>;
+
+    fn is_eloop(e: &io::Error) -> bool;
+}

src/_impl/path_components.rs

@@ -0,0 +1,22 @@
+use std::{fmt::Display, path::Path};
+
+/// Print a path that is broken into segments.
+// explicitly typed to avoid type recursion. 'a is the smallest lifetime present
+// : that of the child.
+pub(crate) enum PathComponents<'a> {
+    Path(&'a Path),
+    Component(&'a PathComponents<'a>, &'a Path),
+}
+
+impl<'p> Display for PathComponents<'p> {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            PathComponents::Path(p) => p.display().fmt(f),
+            PathComponents::Component(p, c) => {
+                p.fmt(f)?;
+                f.write_str("/")?;
+                c.display().fmt(f)
+            }
+        }
+    }
+}