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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/src/filesystem/directory_iterator.cpp:207
       ec = detail::capture_errno();
       const bool allow_eacess =
           bool(opts & directory_options::skip_permission_denied);
----------------
Quuxplusone wrote:
> Pre-existing: `allow_eacces`
Fixed in a separate NFC commit.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118134