Can Redox OS implement a TOCTOU-free fs::remove_dir_all yet?

[workingjubilee]

Location

fs::remove_dir_all

Summary

See https://blog.rust-lang.org/2022/01/20/cve-2022-21658/ if you want an explanation of what I mean.

Noticed it has been a hot minute since this happened after I noticed the FORTRANization of Redox's name.

Classifying as a documentation issue for now since I have no idea if the docs are even up-to-date on this point.

cc @jackpot51

[bjorn3]

RedoxOS doesn't have openat support yet. It is planned though: https://gitlab.redox-os.org/redox-os/rfcs/-/merge_requests/25 After that we would need to add more *at syscalls.

[jackpot51]

We may already support it in a different way, can you point to the Linux implementation so I can check?

[bjorn3]

rust/library/std/src/sys/fs/unix.rs

Lines 2162 to 2320 in 0e517d3

	// Modern implementation using openat(), unlinkat() and fdopendir()
	#[cfg(not(any(
	target_os = "redox",
	target_os = "espidf",
	target_os = "horizon",
	target_os = "vita",
	target_os = "nto",
	target_os = "vxworks",
	miri
	)))]
	mod remove_dir_impl {
	#[cfg(not(all(target_os = "linux", target_env = "gnu")))]
	use libc::{fdopendir, openat, unlinkat};
	#[cfg(all(target_os = "linux", target_env = "gnu"))]
	use libc::{fdopendir, openat64 as openat, unlinkat};
	use super::{Dir, DirEntry, InnerReadDir, ReadDir, lstat};
	use crate::ffi::CStr;
	use crate::io;
	use crate::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd};
	use crate::os::unix::prelude::{OwnedFd, RawFd};
	use crate::path::{Path, PathBuf};
	use crate::sys::common::small_c_string::run_path_with_cstr;
	use crate::sys::{cvt, cvt_r};
	use crate::sys_common::ignore_notfound;
	pub fn openat_nofollow_dironly(parent_fd: Option<RawFd>, p: &CStr) -> io::Result<OwnedFd> {
	let fd = cvt_r(|| unsafe {
	openat(
	parent_fd.unwrap_or(libc::AT_FDCWD),
	p.as_ptr(),
	libc::O_CLOEXEC | libc::O_RDONLY | libc::O_NOFOLLOW | libc::O_DIRECTORY,
	)
	})?;
	Ok(unsafe { OwnedFd::from_raw_fd(fd) })
	}
	fn fdreaddir(dir_fd: OwnedFd) -> io::Result<(ReadDir, RawFd)> {
	let ptr = unsafe { fdopendir(dir_fd.as_raw_fd()) };
	if ptr.is_null() {
	return Err(io::Error::last_os_error());
	}
	let dirp = Dir(ptr);
	// file descriptor is automatically closed by libc::closedir() now, so give up ownership
	let new_parent_fd = dir_fd.into_raw_fd();
	// a valid root is not needed because we do not call any functions involving the full path
	// of the `DirEntry`s.
	let dummy_root = PathBuf::new();
	let inner = InnerReadDir { dirp, root: dummy_root };
	Ok((ReadDir::new(inner), new_parent_fd))
	}
	#[cfg(any(
	target_os = "solaris",
	target_os = "illumos",
	target_os = "haiku",
	target_os = "vxworks",
	target_os = "aix",
	))]
	fn is_dir(_ent: &DirEntry) -> Option<bool> {
	None
	}
	#[cfg(not(any(
	target_os = "solaris",
	target_os = "illumos",
	target_os = "haiku",
	target_os = "vxworks",
	target_os = "aix",
	)))]
	fn is_dir(ent: &DirEntry) -> Option<bool> {
	match ent.entry.d_type {
	libc::DT_UNKNOWN => None,
	libc::DT_DIR => Some(true),
	_ => Some(false),
	}
	}
	fn is_enoent(result: &io::Result<()>) -> bool {
	if let Err(err) = result
	&& matches!(err.raw_os_error(), Some(libc::ENOENT))
	{
	true
	} else {
	false
	}
	}
	fn remove_dir_all_recursive(parent_fd: Option<RawFd>, path: &CStr) -> io::Result<()> {
	// try opening as directory
	let fd = match openat_nofollow_dironly(parent_fd, &path) {
	Err(err) if matches!(err.raw_os_error(), Some(libc::ENOTDIR | libc::ELOOP)) => {
	// not a directory - don't traverse further
	// (for symlinks, older Linux kernels may return ELOOP instead of ENOTDIR)
	return match parent_fd {
	// unlink...
	Some(parent_fd) => {
	cvt(unsafe { unlinkat(parent_fd, path.as_ptr(), 0) }).map(drop)
	}
	// ...unless this was supposed to be the deletion root directory
	None => Err(err),
	};
	}
	result => result?,
	};
	// open the directory passing ownership of the fd
	let (dir, fd) = fdreaddir(fd)?;
	for child in dir {
	let child = child?;
	let child_name = child.name_cstr();
	// we need an inner try block, because if one of these
	// directories has already been deleted, then we need to
	// continue the loop, not return ok.
	let result: io::Result<()> = try {
	match is_dir(&child) {
	Some(true) => {
	remove_dir_all_recursive(Some(fd), child_name)?;
	}
	Some(false) => {
	cvt(unsafe { unlinkat(fd, child_name.as_ptr(), 0) })?;
	}
	None => {
	// POSIX specifies that calling unlink()/unlinkat(..., 0) on a directory can succeed
	// if the process has the appropriate privileges. This however can causing orphaned
	// directories requiring an fsck e.g. on Solaris and Illumos. So we try recursing
	// into it first instead of trying to unlink() it.
	remove_dir_all_recursive(Some(fd), child_name)?;
	}
	}
	};
	if result.is_err() && !is_enoent(&result) {
	return result;
	}
	}
	// unlink the directory after removing its contents
	ignore_notfound(cvt(unsafe {
	unlinkat(parent_fd.unwrap_or(libc::AT_FDCWD), path.as_ptr(), libc::AT_REMOVEDIR)
	}))?;
	Ok(())
	}
	fn remove_dir_all_modern(p: &CStr) -> io::Result<()> {
	// We cannot just call remove_dir_all_recursive() here because that would not delete a passed
	// symlink. No need to worry about races, because remove_dir_all_recursive() does not recurse
	// into symlinks.
	let attr = lstat(p)?;
	if attr.file_type().is_symlink() {
	super::unlink(p)
	} else {
	remove_dir_all_recursive(None, &p)
	}
	}
	pub fn remove_dir_all(p: &Path) -> io::Result<()> {
	run_path_with_cstr(p, &remove_dir_all_modern)
	}
	}

[jackpot51]

I think unlinkat would also be important.

[jackpot51]

We are working to implement all the *at functions and eliminate the other functions at the syscall level but it will take time

[jackpot51]

@workingjubilee I'm working through this in the Redox OS chat, can you assign me to the issue?

[workingjubilee]

Yes, I saw that the Redox GitLab had recent commotion around the openat API, which I know is part of the minimal requirement here if we use any POSIX-like APIs for this. Thus I thought to myself "is this even up-to-date?"

Note that we do not require Redox OS to implement POSIX APIs for the purpose of the stdlib. It can be nice from the code review perspective (if the POSIX code for the OS does not require many custom exceptions for that OS, anyways), but it's optional. Any API that allows us to avoid a TOCTOU race and thus satisfy the relevant security requirement will allow us to update the stdlib and remove the special note about Redox in the the security notice from the fs::remove_dir_all docs. Though it might also mean we list out the functions we call for that OS, I suppose.