This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fix TOCTOU issue with std::filesystem::remove_all

Authored by ldionne on Jan 25 2022, 5:15 AM.

Diff Detail

Event Timeline

ldionne requested review of this revision.Jan 25 2022, 5:15 AM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2022, 5:15 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Jan 25 2022, 8:37 AM
Quuxplusone added a subscriber: Quuxplusone.

Further discussion is on /r/cpp:
(Remember I told you Niall Douglas was once working on a thing for secure filesystem? There he is talking about it!)

26 ↗(On Diff #402865)

Please add a trailing comma here too, so you don't have to touch this line next time. :)

200 ↗(On Diff #402865)

Surely you should check if (fd == -1) (or even if (fd < 0)) here, and bail down to line 206 if needed.

207 ↗(On Diff #402865)

Pre-existing: allow_eacces


Doesn't this flatten/round-trip everything back through path, which means you have the symlink vulnerability again at this level?

Suppose I ask to remove_all("/tmp/foo"). The STL securely/atomically opens /tmp/foo as a directory (detecting and rejecting any attempt by me to rm -rf /tmp/foo ; ln -sf /root /tmp/foo). Then it starts iterating over that open directory. (At this point I can ln -sf /root /tmp/foo if I want, but it won't matter because the STL is already iterating over the real inode and no longer cares what's at that path in the filesystem.)
The STL removes /tmp/foo/a.txt. Then it sees a subdirectory /tmp/foo/bin. So it... uh... goes back to open the path /tmp/foo/bin?? But in the meantime, I have done rm -rf /tmp/foo ; ln -sf /usr /tmp/foo. So now when the STL opens the path /tmp/foo/bin, it's secretly opening /usr/bin, and will happily delete everything out of it. (Notice that bin there is not a symlink, so O_NOFOLLOW is happy.)

I believe that a proper fix for this issue requires using [openat]( at every level. As soon as the code touches fs::path, it's game over. (Bonus: fs::path does a ton of heap allocation, but with openat I suspect you never need to allocate, do you?)

This revision now requires changes to proceed.Jan 25 2022, 8:37 AM
ldionne updated this revision to Diff 403052.Jan 25 2022, 3:26 PM
ldionne marked 4 inline comments as done.

Completely new approach using parent file descriptors. I'm aware this patch as-is won't compile on Windows -- I'll fix that once I'm confident the base patch is alright. Windows is not affected by this issue anyway.

207 ↗(On Diff #402865)

Fixed in a separate NFC commit.


Yeah, I think you're right. I don't know how to retrofit that on top of directory_iterator though. We could technically do it using recursive_directory_iterator, but it would be way more complicated.

I want to get this patch landed ASAP, so I'm going to upload another approach based entirely on top of the openat, unlinkat & friends API, without using directory_iterator at all. Please take a look -- once we're confident we're solving the problem properly, we can try to figure out how to polish the rough edges after landing it.

Also adding @jwakely @STL_MSFT for awareness. In particular, @jwakely it looks like the initial approach I (we?) had taken using directory_iterator was too naive.

ojhunt added a subscriber: ojhunt.Jan 25 2022, 4:12 PM
ojhunt added inline comments.
572 ↗(On Diff #403052)

(non-libc++ expert) Sorry if this is a dumb question: is capture_errno() API? is it safe to have in a header?

77 ↗(On Diff #403052)

I wonder if it would be simpler to just move remove_all into this .cpp file. A priori (without seeing that this diff is where they came from) it's weird to see static functions in a .h file.

570 ↗(On Diff #403052)

Seems like a perfect place for if (struct dirent *dir_entry_ptr = ::readdir(dir_stream)) { ... (and swap the if/else branches).

Btw, for anyone else who's confused why we aren't using ::readdir_r, apparently ::readdir is equally thread-safe on modern systems (at least, those modern systems documented by LWN ;)) We should expect that dir_entry_ptr will point to some memory located physically inside the footprint of *dir_stream (as opposed to some static buffer or something).

However, I now also see that this code simply moved from directory_iterator.cpp (and have suggested that maybe remove_all should move over there instead), in which case there's no need to drive-by refactor any of this particular function.


Any reason for intmax_t here but uintmax_t in the function return type? I suggest consistency, but don't care which.


If anything below were actually to throw an exception, then our count would be wrong. But this simplifies the early-return cases even in the absence of exceptions, so I assume that's why you did it.


Consider a documentary static_assert(std::is_same_v<decltype(str), std::string_view> here, because comparing auto to a string literal smells like it might be a pointer comparison, and that would suck.


Style: For this if-else ladder, I'd prefer either what-you've-got-minus-all-the-else-keywords, or

if (ec == errc::no_such_file_or_directory) {
  // Not an error; `p` might have moved or been deleted already.
  return 0;
} else if (ec == errc::not_a_directory) {
  // Remove `p` as a normal file instead.

i.e., put the commentary for each branch on the branch, and cuddle the elses as usual.


FWIW, this line is still racey:
The primitive that we need here is "remove-parent_directory's-child-named-p-iff-it-still-refers-to-the-same-inode-as-fd", i.e., a sort of compare-exchange primitive, which Linux does not provide and which is impossible to emulate in userspace.

So, this line right here is already the state of the art, and I can't think of anything better to do than to be OK with it.

Also, at first glance I can't see any way to really "exploit" this race. Either (happy path) you remove the now-empty directory you intended to; or the attacker moves that now-empty directory out of the way and puts their own thing in its place which you fail to delete (because it's not a directory, or because it's a non-empty directory); or the attacker moves that now-empty directory out of the way and puts their own empty directory in its place and you delete it (but it was an empty directory, and it wasn't a symlink (because it was a directory), so who cares).
Also, we will always have this same race on the front side of the transaction: the programmer gives us a fs::path and we start removing whatever's there now, not whatever was there when the programmer decided to call into us.


It would seem simpler to move this down below the if, and get rid of the if (stream != nullptr) inside the lambda. (Also personally I'd capture [&] rather than [stream], because nothing weird is going on here.)
Analogous comments may apply to fd above.


Running this test deletes whatever I have in /tmp/mydir/victim_del? I suggest not committing this as-is. 😛
Looks like the existing tests use things like env.create_dir and env.create_file to avoid messing with the user's own files.


(If this is intended for commit at all) Consider adding another test for the subdirectory case I brought up in my previous review.

ldionne marked 10 inline comments as done.Jan 26 2022, 8:08 AM
ldionne added inline comments.
77 ↗(On Diff #403052)

I hadn't noticed they were static. I think it would be entirely fine to make them non-static and leave them in the header, but I'd rather not move remove_all into this file, it seems pretty weird to have that operation (and only that one) inside directory_iterator.cpp.

Edit: After looking more at the contents of the file, there's a bunch of static functions and almost everything is defined in an anonymous namespace. Honestly, it's just weird. I'll make the move as-is in a separate commit and we can take an action item to go back and refactor this later.

570 ↗(On Diff #403052)

Right, I think I'll move the code in a separate commit to make this less confusing.

572 ↗(On Diff #403052)

filesystem_common.h is only used inside the sources of libc++ used for building the shared library. But yeah, I think it would be fine regardless, it's just a helper function.


Sorry, that was a typo. It should have been uintmax_t, thanks for spotting.


Yes, simplifying the cleanup (without worrying about exceptions) is the purpose of this class. I am assuming that nothing throws in the filesystem code because we are always using the error_code version of functions.


Right, I agree -- I hadn't thought about this race but I think we're both on the same page that it is benign (and also unavoidable without different OS APIs).

ldionne updated this revision to Diff 403274.Jan 26 2022, 8:09 AM
ldionne marked 6 inline comments as done.

Address all comments except for adding a second test.

ldionne updated this revision to Diff 403672.Jan 27 2022, 8:37 AM

Use the old implementation on systems that don't have openat.

ldionne updated this revision to Diff 403674.Jan 27 2022, 8:40 AM
ldionne added a subscriber: daltenty.

Use _AIX instead of MVS to guard.

@daltenty Note that it looks like AIX does provide those POSIX functions, but the tests fail
when I start using my implementation on AIX. If someone who works on AIX has an appetite to
take a look, it might be good to enable the new (safe) implementation on AIX too.

ldionne updated this revision to Diff 403799.Jan 27 2022, 2:15 PM

Add some UNSUPPORTED for Windows and AIX.

ldionne added a subscriber: Restricted Project.Jan 27 2022, 2:17 PM

I'd like to ship this before LLVM 14, so if folks can take a look, it would be awesome.

Also, adding vendors so they are aware of this. In particular, we don't implement this fix on AIX, Windows and MinGW . On AIX it's because somehow the remove_all test starts failing when we use the implementation based on openat(), and on MinGW/Windows it's because the OS doesn't implement the right APIs AFAICT. If people who manage these platforms want to take a look, this is the notice.

Looking at Rust's fix over seems like there is API added in Windows 10 to solve it (I don't know how effective though).
I'm not going to work on this myself but leaving the link in case somebody else wants to give it a try.

Thanks, noted. I don't believe I have the bandwidth to fix this right now before the 14.x branch early next week though, so the current form of the patch seems sensible wrt Windows I think.

Looking at Rust's fix over seems like there is API added in Windows 10 to solve it (I don't know how effective though).

The use of SetFileInformationByHandle(FileDispositionInfoEx) with FILE_DISPOSITION_FLAG_POSIX_SEMANTICS doesn't seem to be the core of the fix. The core of the fix is to rewrite directory iteration with something that operates on an open handle, instead of something given a path. In the Rust fix, this is done with GetFileInformationByHandleEx(FileIdBothDirectoryInfo) (which I would believe exists earlier).

So we could open a handle to the intended path with FILE_FLAG_OPEN_REPARSE_POINT (so it doesn't follow symlinks), then inspect whether it's a regular file or a directory, and if a directory, iterate over its contents without closing the handle and using a path name again. (Currently we use the generic directory iterators, which are built on top of FindFirstFileW/FindNextFileW.)


This header can't be included unconditionally on all OSes.

ldionne updated this revision to Diff 404108.Jan 28 2022, 11:27 AM
ldionne marked an inline comment as done.

Don't include <dirent.h> unconditionally.

Thanks for pinging us on this. After taking a look at the AIX test failure, and dumping the error_code we get back from the new implementation, I think this is actually due to some ambiguity in the expected errno when the combination of O_DIRECTORY and O_NOFOLLOW is used and the path is a symlink.

If path resolves to a non-directory file, fail and set errno to [ENOTDIR].
If path names a symbolic link, fail and set errno to [ELOOP].

See the following test program:

#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <stdio.h>
#include <errno.h>

int main() {
	mkdir("foo", S_IRWXU);
	symlink("foo", "bar");
	int ret=openat(AT_FDCWD, "bar", O_CLOEXEC | O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
	if (errno==ENOTDIR) {
	} else if (errno==ELOOP) {
	return 0;

Which will it seems will give you ENOTDIR on MacOS and some Linux, but gives ELOOP on AIX (and interestingly RHEL Linux on Power).


Seems like we need to address the too_many_symbolic_link_levels error case with O_NOFOLLOW

ldionne updated this revision to Diff 404508.Jan 31 2022, 7:10 AM
ldionne marked an inline comment as done.

Address failure caused by symlink on AIX.

Thanks for pinging us on this. After taking a look at the AIX test failure, and dumping the error_code we get back from the new implementation, I think this is actually due to some ambiguity in the expected errno when the combination of O_DIRECTORY and O_NOFOLLOW is used and the path is a symlink.

Sure thing, thanks a lot for investigating. I'm re-uploading the patch with a fix and I'm enabling the fix on AIX -- let's see if CI is happy (I can't test on that platform locally).

ldionne accepted this revision.Feb 1 2022, 12:31 PM

The release branch is apparently being cut today. I'm going to ship this now because I don't want to miss the deadline with this fix, but please feel free to comment if you see potential issues/improvements and I'll implement them (and cherry-pick onto LLVM 14).

This revision was not accepted when it landed; it landed in state Needs Review.Feb 1 2022, 12:31 PM
This revision was automatically updated to reflect the committed changes.

Thanks for pinging us on this. After taking a look at the AIX test failure, and dumping the error_code we get back from the new implementation, I think this is actually due to some ambiguity in the expected errno when the combination of O_DIRECTORY and O_NOFOLLOW is used and the path is a symlink.

I've just hit the same case for GCC on AIX. Arguably POSIX is clear and AIX is right. For the case being discussed we have a symlink to a directory, so the path *names* a symlink, but *resolves* to a directory. So ELOOP is right.

I'll raise this with glibc.