Fix for CVE-2022-21658 is unsafe on some file systems

[jclulow]

The fix for CVE-2022-21658 (#93112, 54e22eb) appears to have introduced a regression that can cause file system corruption on some UNIX systems.

The new code will attempt to use unlinkat(2) in any case where it does not know what type of file a directory entry represents. Some systems do not provide that information along with the entry names returned while reading a directory, and even on systems which provide a type hint, that hint may be DT_UNKNOWN. In cases where we do not know the type of the entry, the code tries first to unlink it as if it were something other than a directory:

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

Lines 1700 to 1712 in 4b043fa

	None => match cvt(unsafe { unlinkat(fd, child.name_cstr().as_ptr(), 0) }) {
	// type unknown - try to unlink
	Err(err)
	if err.raw_os_error() == Some(libc::EISDIR)
	|| err.raw_os_error() == Some(libc::EPERM) =>
	{
	// if the file is a directory unlink fails with EISDIR on Linux and EPERM everyhwere else
	remove_dir_all_recursive(Some(fd), Path::new(&child.file_name()))?;
	}
	result => {
	result?;
	}
	},

The code assumes that unlinkat() on a directory will fail with EPERM, or on Linux with the non-standard EISDIR, but this is not always the case. POSIX does suggests EPERM as a failure in some, but not all cases:

[EPERM]
The file named by path is a directory, and either the calling process does not have appropriate privileges, or the implementation prohibits using unlink() on directories.

(See unlink in The Open Group Base Specifications Issue 7, 2018 edition)

Implementations are not required to prohibit unlink() on directories, and indeed UFS file systems on present day illumos and presumably at least some Solaris systems allow unlink() of a directory if the user has sufficient privileges. Other platforms may as well.

Unfortunately unlinking a directory on UFS causes a sort of file system corruption that requires an unmount and a trip through fsck to correct. Any code that uses std::fs::remove_dir_all() on UFS has since started causing that kind of corruption, which took a bit of work to track down.

I think the fix is probably relatively simple, and has the benefit of not touching the code path where we believe we know what sort of entry we are removing:

None => match cvt(unsafe { unlinkat(fd, child.name_cstr().as_ptr(), libc::AT_REMOVEDIR) }) {
    // type unknown - try to rmdir
    Err(err)
        if err.raw_os_error() == Some(libc::EEXIST)
            || err.raw_os_error() == Some(libc::ENOTEMPTY) =>
    {
        // this is a directory that contains files to remove
        remove_dir_all_recursive(Some(fd), Path::new(&child.file_name()))?;
    }
    Err(err) if err.raw_os_error() == Some(libc::ENOTDIR) {
        // this is some other type of entry that we can just unlink
        cvt(unsafe { unlinkat(fd, child.name_cstr().as_ptr(), 0) })?;
    }
    result => {
        result?;
    }
}

In short, we should try to rmdir() first, rather than unlink() first, as that always fails in a safe way that we can detect.

[Urgau]

cc @hkratz (as you're the one how wrote the new code)

[hkratz]

@Urgau Thanks for the ping

@rustbot claim

[apiraino]

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-critical

[nagisa]

Was this issue reported upstream? Filesystems corrupting themselves seem like a poor failure mode in any case, and in some ways is a violation of the sandboxing that the kernel ought to provide for the user-space applications.