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

[ldionne]

Summary

https://bugs.chromium.org/p/llvm/issues/detail?id=19
rdar://87912416

[• Quuxplusone]

Further discussion is on /r/cpp: https://old.reddit.com/r/cpp/comments/s8ok0h/possible_toctou_vulnerabilities_in/hti8jyt/
(Remember I told you Niall Douglas was once working on a thing for secure filesystem? There he is talking about it!)

libcxx/include/__filesystem/directory_options.h

26 ↗

(On Diff #402865)

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

libcxx/src/filesystem/directory_iterator.cpp

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

libcxx/src/filesystem/operations.cpp

1380

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](https://linux.die.net/man/2/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?)

[ldionne]

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.

libcxx/src/filesystem/directory_iterator.cpp

207 ↗

(On Diff #402865)

Fixed in a separate NFC commit.

libcxx/src/filesystem/operations.cpp

1380

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.

[ldionne]

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]

libcxx/src/filesystem/filesystem_common.h

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?

[• Quuxplusone]

libcxx/src/filesystem/directory_iterator.cpp

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.

libcxx/src/filesystem/filesystem_common.h

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 ;)) https://lwn.net/Articles/696474/ 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.

libcxx/src/filesystem/operations.cpp

1373

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

1375

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.

1375

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.

1377

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.
  ec.clear();
  return 0;
} else if (ec == errc::not_a_directory) {
  // Remove `p` as a normal file instead.
  ec.clear();
  ~~~

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

1386

FWIW, this line is still racey:
https://stackoverflow.com/questions/28517236/can-posix-linux-unlink-file-entries-completely-race-free
https://bugzilla.kernel.org/show_bug.cgi?id=93441
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.

1390

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.

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.remove_all/toctou.pass.cpp

38–39

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.

79

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

[ldionne]

libcxx/src/filesystem/directory_iterator.cpp

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.

libcxx/src/filesystem/filesystem_common.h

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.

libcxx/src/filesystem/operations.cpp

1373

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

1375

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.

1386

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]

Address all comments except for adding a second test.

[ldionne]

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

[ldionne]

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]

Add some UNSUPPORTED for Windows and AIX.

[ldionne]

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.

[mati865]

Looking at Rust's fix over https://github.com/rust-lang/rust/commit/4f0ad1c92ca08da6e8dc17838070975762f59714 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.

[mstorsjo]

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.

In D118134#3277706, @mati865 wrote:

Looking at Rust's fix over https://github.com/rust-lang/rust/commit/4f0ad1c92ca08da6e8dc17838070975762f59714 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.)

libcxx/src/filesystem/operations.cpp

50

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

[ldionne]

Don't include <dirent.h> unconditionally.

[daltenty]

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.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html

O_DIRECTORY
If path resolves to a non-directory file, fail and set errno to [ENOTDIR].
O_NOFOLLOW
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) {
	  printf("ENOTDIR\n");
	} else if (errno==ELOOP) {
	  printf("ELOOP\n");
	}
	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).

libcxx/src/filesystem/operations.cpp

1458

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

[ldionne]

Address failure caused by symlink on AIX.

[ldionne]

In D118134#3281500, @daltenty wrote:

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]

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).

[jwakely]

In D118134#3281500, @daltenty wrote:

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.

libcxx/src/filesystem/operations.cpp

 
 #include "posix_compat.h"
 
 #if defined(_LIBCPP_WIN32API)
 # define WIN32_LEAN_AND_MEAN
 # define NOMINMAX
 # include <windows.h>
 #else
-# include <unistd.h>
+# include <dirent.h>
 # include <sys/stat.h>
 # include <sys/statvfs.h>
+# include <unistd.h>
 #endif
 #include <time.h>
 #include <fcntl.h> /* values for fchmodat */
 
 #if __has_include(<sys/sendfile.h>)
 # include <sys/sendfile.h>
 # define _LIBCPP_FILESYSTEM_USE_SENDFILE
 #elif defined(__APPLE__) || __has_include(<copyfile.h>)
 # include <copyfile.h>
 # define _LIBCPP_FILESYSTEM_USE_COPYFILE
 #else
 # include "fstream"
 # define _LIBCPP_FILESYSTEM_USE_FSTREAM
 #endif
 
 #if !defined(CLOCK_REALTIME) && !defined(_LIBCPP_WIN32API)
 # include <sys/time.h> // for gettimeofday and timeval
 #endif
 
 #if defined(__ELF__) && defined(_LIBCPP_LINK_RT_LIB)
 # pragma comment(lib, "rt")
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_FILESYSTEM
 
 namespace {
 
 bool isSeparator(path::value_type C) {
bool __remove(const path& p, error_code* ec) {
   if (detail::remove(p.c_str()) == -1) {
     if (errno != ENOENT)
       err.report(capture_errno());
     return false;
   }
   return true;
 }
 
+// We currently have two implementations of `__remove_all`. The first one is general and
+// used on platforms where we don't have access to the `openat()` family of POSIX functions.
+// That implementation uses `directory_iterator`, however it is vulnerable to some race
+// conditions, see https://reviews.llvm.org/D118134 for details.
+//
+// The second implementation is used on platforms where `openat()` & friends are available,
+// and it threads file descriptors through recursive calls to avoid such race conditions.
+#if defined(_LIBCPP_WIN32API)
+# define REMOVE_ALL_USE_DIRECTORY_ITERATOR
+#endif
+
+#if defined(REMOVE_ALL_USE_DIRECTORY_ITERATOR)
+
 namespace {
 
 uintmax_t remove_all_impl(path const& p, error_code& ec) {
   const auto npos = static_cast<uintmax_t>(-1);
   const file_status st = __symlink_status(p, &ec);
   if (ec)
     return npos;
   uintmax_t count = 1;
   if (is_directory(st)) {
     for (directory_iterator it(p, ec); !ec && it != directory_iterator();
          it.increment(ec)) {
       auto other_count = remove_all_impl(it->path(), ec);
       if (ec)
         return npos;
       count += other_count;
     }
     if (ec)
       return npos;
   }
   if (!__remove(p, &ec))
     return npos;
   return count;
 }
 
 } // end namespace
 
 uintmax_t __remove_all(const path& p, error_code* ec) {
   ErrorHandler<uintmax_t> err("remove_all", ec, &p);
 
   error_code mec;
   auto count = remove_all_impl(p, mec);
   if (mec) {
     if (mec == errc::no_such_file_or_directory)
       return 0;
     return err.report(mec);
   }
   return count;
 }
 
+#else // !REMOVE_ALL_USE_DIRECTORY_ITERATOR
+
+namespace {
+
+template <class Cleanup>
+struct scope_exit {
+  explicit scope_exit(Cleanup const& cleanup)
+    : cleanup_(cleanup)
+  { }
+
+  ~scope_exit() { cleanup_(); }
+
+private:
+  Cleanup cleanup_;
+};
+
+uintmax_t remove_all_impl(int parent_directory, const path& p, error_code& ec) {
+  // First, try to open the path as a directory.
+  const int options = O_CLOEXEC | O_RDONLY | O_DIRECTORY | O_NOFOLLOW;
+  int fd = ::openat(parent_directory, p.c_str(), options);
+  if (fd != -1) {
+    // If that worked, iterate over the contents of the directory and
+    // remove everything in it, recursively.
+    scope_exit close_fd([=] { ::close(fd); });
+    DIR* stream = ::fdopendir(fd);
+    if (stream == nullptr) {
+      ec = detail::capture_errno();
+      return 0;
+    }
+    scope_exit close_stream([=] { ::closedir(stream); });
+
+    uintmax_t count = 0;
+    while (true) {
+      auto [str, type] = detail::posix_readdir(stream, ec);
+      static_assert(std::is_same_v<decltype(str), std::string_view>);
+      if (str == "." || str == "..") {
+        continue;
+      } else if (ec || str.empty()) {
+        break; // we're done iterating through the directory
+      } else {
+        count += remove_all_impl(fd, str, ec);
+      }
+    }
+
+    // Then, remove the now-empty directory itself.
+    if (::unlinkat(parent_directory, p.c_str(), AT_REMOVEDIR) == -1) {
+      ec = detail::capture_errno();
+      return count;
+    }
+
+    return count + 1; // the contents of the directory + the directory itself
+  }
+
+  ec = detail::capture_errno();
+
+  // If we failed to open `p` because it didn't exist, it's not an
+  // error -- it might have moved or have been deleted already.
+  if (ec == errc::no_such_file_or_directory) {
+    ec.clear();
+    return 0;
+  }
+
+  // If opening `p` failed because it wasn't a directory, remove it as
+  // a normal file instead. Note that `openat()` can return either ENOTDIR
+  // or ELOOP depending on the exact reason of the failure.
+  if (ec == errc::not_a_directory || ec == errc::too_many_symbolic_link_levels) {
+    ec.clear();
+    if (::unlinkat(parent_directory, p.c_str(), /* flags = */0) == -1) {
+      ec = detail::capture_errno();
+      return 0;
+    }
+    return 1;
+  }
+
+  // Otherwise, it's a real error -- we don't remove anything.
+  return 0;
+}
+
+} // end namespace
+
+uintmax_t __remove_all(const path& p, error_code* ec) {
+  ErrorHandler<uintmax_t> err("remove_all", ec, &p);
+  error_code mec;
+  uintmax_t count = remove_all_impl(AT_FDCWD, p, mec);
+  if (mec)
+    return err.report(mec);
+  return count;
+}
+
+#endif // REMOVE_ALL_USE_DIRECTORY_ITERATOR
+
 void __rename(const path& from, const path& to, error_code* ec) {
   ErrorHandler<void> err("rename", ec, &from, &to);
   if (detail::rename(from.c_str(), to.c_str()) == -1)
     err.report(capture_errno());
 }
 
 void __resize_file(const path& p, uintmax_t size, error_code* ec) {
   ErrorHandler<void> err("resize_file", ec, &p);

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.remove_all/toctou.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03
+// UNSUPPORTED: libcpp-has-no-localization
+// UNSUPPORTED: libcpp-has-no-threads
+
+// <filesystem>
+
+// Test for a time-of-check to time-of-use issue with std::filesystem::remove_all.
+//
+// Scenario:
+// The attacker wants to get directory contents deleted, to which he does not have access.
+// He has a way to get a privileged binary call `std::filesystem::remove_all()` on a
+// directory he controls, e.g. in his home directory.
+//
+// The POC sets up the `attack_dest/attack_file` which the attacker wants to have deleted.
+// The attacker repeatedly creates a directory and replaces it with a symlink from
+// `victim_del` to `attack_dest` while the victim code calls `std::filesystem::remove_all()`
+// on `victim_del`. After a few seconds the attack has succeeded and
+// `attack_dest/attack_file` is deleted.
+//
+// This is taken from https://github.com/rust-lang/wg-security-response/blob/master/patches/CVE-2022-21658/0002-Fix-CVE-2022-21658-for-UNIX-like.patch
+
+// This test requires a dylib containing the fix shipped in https://reviews.llvm.org/D118134.
+// We use UNSUPPORTED instead of XFAIL because the test might not fail reliably.
+// UNSUPPORTED: use_system_cxx_lib && target={{.+}}-apple-macosx10.{{9|10|11|12|13|14|15}}
+// UNSUPPORTED: use_system_cxx_lib && target={{.+}}-apple-macosx11
+// UNSUPPORTED: use_system_cxx_lib && target={{.+}}-apple-macosx12.{{0|1|2}}
+
+// Windows doesn't support the necessary APIs to mitigate this issue.
+// UNSUPPORTED: target={{.+}}-windows-{{.+}}
+
+#include <cstdio>
+#include <filesystem>
+#include <system_error>
+#include <thread>
+
+#include "filesystem_include.h"
+#include "filesystem_test_helper.h"
+
+int main() {
+  scoped_test_env env;
+  fs::path const tmpdir = env.create_dir("mydir");
+  fs::path const victim_del_path = tmpdir / "victim_del";
+  fs::path const attack_dest_dir = env.create_dir(tmpdir / "attack_dest");
+  fs::path const attack_dest_file = env.create_file(attack_dest_dir / "attack_file", 42);
+
+  // victim just continuously removes `victim_del`
+  bool stop = false;
+  std::thread t{[&]() {
+    while (!stop) {
+        std::error_code ec;
+        fs::remove_all(victim_del_path, ec); // ignore any error
+    }
+  }};
+
+  // attacker (could of course be in a separate process)
+  auto start_time = std::chrono::system_clock::now();
+  auto elapsed_since = [](std::chrono::system_clock::time_point const& time_point) {
+      return std::chrono::duration_cast<std::chrono::seconds>(std::chrono::system_clock::now() - time_point);
+  };
+  bool attack_succeeded = false;
+  while (elapsed_since(start_time) < std::chrono::seconds(5)) {
+    if (!fs::exists(attack_dest_file)) {
+      std::printf("Victim deleted symlinked file outside of victim_del. Attack succeeded in %lld seconds.\n",
+                  elapsed_since(start_time).count());
+      attack_succeeded = true;
+      break;
+    }
+    std::error_code ec;
+    fs::create_directory(victim_del_path, ec);
+    if (ec) {
+      continue;
+    }
+
+    fs::remove(victim_del_path);
+    fs::create_directory_symlink(attack_dest_dir, victim_del_path);
+  }
+  stop = true;
+  t.join();
+
+  return attack_succeeded ? 1 : 0;
+}