[libcxx-commits] [PATCH] D118134: [libc++] Fix TOCTOU issue with std::filesystem::remove_all

Tue Jan 25 18:11:56 PST 2022

Quuxplusone added inline comments.

================
Comment at: libcxx/src/filesystem/directory_iterator.cpp:77
-#else
-// defined(_LIBCPP_WIN32API)

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

================
Comment at: libcxx/src/filesystem/filesystem_common.h:570
+  ec.clear();
+  if ((dir_entry_ptr = ::readdir(dir_stream)) == nullptr) {
+    if (errno)
----------------
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.

================
Comment at: libcxx/src/filesystem/operations.cpp:1351
+
+  ~scope_exit() { cleanup_(); }
+
----------------
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.

================
Comment at: libcxx/src/filesystem/operations.cpp:1366
+    DIR* stream = ::fdopendir(fd);
+    scope_exit close_stream([stream] { if (stream != nullptr) ::closedir(stream); });
+    if (stream == nullptr) {
----------------
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.

================
Comment at: libcxx/src/filesystem/operations.cpp:1372
+
+    intmax_t count = 0;
+    while (true) {
----------------
Any reason for `intmax_t` here but `uintmax_t` in the function return type? I suggest consistency, but don't care which.

================
Comment at: libcxx/src/filesystem/operations.cpp:1374
+    while (true) {
+      auto [str, type] = detail::posix_readdir(stream, ec);
+      if (str == "." || str == "..") {
----------------
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.

================
Comment at: libcxx/src/filesystem/operations.cpp:1385
+    // Then, remove the directory itself.
+    if (::unlinkat(parent_directory, p.c_str(), AT_REMOVEDIR) == -1) {
+      ec = detail::capture_errno();
----------------
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.

================
Comment at: libcxx/src/filesystem/operations.cpp:1416
   }
-  if (!__remove(p, &ec))
-    return npos;
-  return count;
 }

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

================
Comment at: libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.remove_all/toctou.pass.cpp:37-38
+int main() {
+  fs::path tmpdir = "/tmp/mydir";
+  fs::path victim_del_path = tmpdir / "victim_del";
+
----------------
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.

================
Comment at: libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.remove_all/toctou.pass.cpp:78
+    fs::create_directory_symlink(attack_dest_dir, victim_del_path);
+  }
+  stop = true;
----------------
(If this is intended for commit at all) Consider adding another test for the subdirectory case I brought up in my previous review.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118134/new/

https://reviews.llvm.org/D118134