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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

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



================
Comment at: libcxx/include/__filesystem/directory_options.h:26
+  skip_permission_denied = 2,
+  __disallow_root_directory_symlink = 4
 };
----------------
Please add a trailing comma here too, so you don't have to touch this line next time. :)


================
Comment at: libcxx/src/filesystem/directory_iterator.cpp:200
+      int fd = ::openat(AT_FDCWD, root.c_str(), O_CLOEXEC | O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
+      __stream_ = ::fdopendir(fd);
+    } else {
----------------
Surely you should check `if (fd == -1)` (or even `if (fd < 0)`) here, and bail down to line 206 if needed.


================
Comment at: libcxx/src/filesystem/directory_iterator.cpp:207
       ec = detail::capture_errno();
       const bool allow_eacess =
           bool(opts & directory_options::skip_permission_denied);
----------------
Pre-existing: `allow_eacces`


================
Comment at: libcxx/src/filesystem/operations.cpp:1377
+    for (; !ec && it != end; it.increment(ec)) {
+      count += remove_all_impl(it->path(), ec);
       if (ec)
----------------
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?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118134