[libcxx-commits] [libcxx] 4f67a90 - [libc++] Fix TOCTOU issue with std::filesystem::remove_all
Louis Dionne via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Feb 1 12:31:38 PST 2022
Author: Louis Dionne
Date: 2022-02-01T15:31:28-05:00
New Revision: 4f67a909902d8ab9e24e171201db189b661700bf
URL: https://github.com/llvm/llvm-project/commit/4f67a909902d8ab9e24e171201db189b661700bf
DIFF: https://github.com/llvm/llvm-project/commit/4f67a909902d8ab9e24e171201db189b661700bf.diff
LOG: [libc++] Fix TOCTOU issue with std::filesystem::remove_all
https://bugs.chromium.org/p/llvm/issues/detail?id=19
rdar://87912416
Differential Revision: https://reviews.llvm.org/D118134
Added:
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.remove_all/toctou.pass.cpp
Modified:
libcxx/src/filesystem/operations.cpp
Removed:
################################################################################
diff --git a/libcxx/src/filesystem/operations.cpp b/libcxx/src/filesystem/operations.cpp
index 62bcfbff097f2..7aeeffaae8f38 100644
--- a/libcxx/src/filesystem/operations.cpp
+++ b/libcxx/src/filesystem/operations.cpp
@@ -24,9 +24,10 @@
# 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 */
@@ -1338,6 +1339,19 @@ bool __remove(const path& p, error_code* ec) {
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) {
@@ -1377,6 +1391,97 @@ uintmax_t __remove_all(const path& p, error_code* ec) {
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)
diff --git a/libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.remove_all/toctou.pass.cpp b/libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.remove_all/toctou.pass.cpp
new file mode 100644
index 0000000000000..f99ce1072bfb8
--- /dev/null
+++ b/libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.remove_all/toctou.pass.cpp
@@ -0,0 +1,89 @@
+//===----------------------------------------------------------------------===//
+//
+// 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;
+}
More information about the libcxx-commits
mailing list