From 41d076ace558cfae01e233f9746dc955dcf78dba Mon Sep 17 00:00:00 2001 From: Andrey Semashev Date: Sun, 30 Jan 2022 23:41:06 +0300 Subject: [PATCH] Added protection for CVE-2022-21658 in remove_all on POSIX systems. Another process could replace the directory being processed by remove_all with a symlink after remove_all called symlink_status but before it creates a directory iterator. As a result, remove_all would remove the linked directory contents instead of removing the symlink. On POSIX systems that support fdopendir and O_NOFOLLOW flag for open(2), this can be prevented by opening the directory with O_NOFOLLOW before iterating. This will fail if the directory was replaced with a symlink. No protection on other systems. Reported in https://github.com/boostorg/filesystem/issues/224. --- libs/filesystem/CMakeLists.txt | 4 ++ libs/filesystem/build/Jamfile.v2 | 1 + libs/filesystem/config/Jamfile.v2 | 2 + libs/filesystem/config/has_fdopendir_nofollow.cpp | 20 ++++++ boost/filesystem/directory.hpp | 3 +- libs/filesystem/src/directory.cpp | 53 ++++++++++++++-- libs/filesystem/src/operations.cpp | 86 ++++++++++++++++---------- 7 files changed, 131 insertions(+), 38 deletions(-) create mode 100644 config/has_fdopendir_nofollow.cpp diff --git a/libs/filesystem/CMakeLists.txt b/libs/filesystem/CMakeLists.txt index 5a8678309..37ef7e161 100644 --- a/libs/filesystem/CMakeLists.txt +++ b/libs/filesystem/CMakeLists.txt @@ -59,6 +59,7 @@ if(NOT BOOST_FILESYSTEM_DISABLE_STATX) check_cxx_source_compiles("#include <${CMAKE_CURRENT_SOURCE_DIR}/config/has_statx_syscall.cpp>" BOOST_FILESYSTEM_HAS_STATX_SYSCALL) endif() endif() +check_cxx_source_compiles("#include <${CMAKE_CURRENT_SOURCE_DIR}/config/has_fdopendir_nofollow.cpp>" BOOST_FILESYSTEM_HAS_FDOPENDIR_NOFOLLOW) if(WIN32 AND NOT BOOST_FILESYSTEM_DISABLE_BCRYPT) set(CMAKE_REQUIRED_LIBRARIES bcrypt) check_cxx_source_compiles("#include <${CMAKE_CURRENT_SOURCE_DIR}/config/has_bcrypt.cpp>" BOOST_FILESYSTEM_HAS_BCRYPT) @@ -170,6 +171,9 @@ endif() if(BOOST_FILESYSTEM_HAS_STATX_SYSCALL) target_compile_definitions(boost_filesystem PRIVATE BOOST_FILESYSTEM_HAS_STATX_SYSCALL) endif() +if(BOOST_FILESYSTEM_HAS_FDOPENDIR_NOFOLLOW) + target_compile_definitions(boost_filesystem PRIVATE BOOST_FILESYSTEM_HAS_FDOPENDIR_NOFOLLOW) +endif() target_link_libraries(boost_filesystem PUBLIC diff --git a/libs/filesystem/build/Jamfile.v2 b/libs/filesystem/build/Jamfile.v2 index 6ab58ddb8..02e3b8971 100644 --- a/libs/filesystem/build/Jamfile.v2 +++ b/libs/filesystem/build/Jamfile.v2 @@ -112,6 +112,7 @@ project boost/filesystem [ check-target-builds ../config//has_stat_st_birthtim "has stat::st_birthtim" : BOOST_FILESYSTEM_HAS_STAT_ST_BIRTHTIM ] [ check-target-builds ../config//has_stat_st_birthtimensec "has stat::st_birthtimensec" : BOOST_FILESYSTEM_HAS_STAT_ST_BIRTHTIMENSEC ] [ check-target-builds ../config//has_stat_st_birthtimespec "has stat::st_birthtimespec" : BOOST_FILESYSTEM_HAS_STAT_ST_BIRTHTIMESPEC ] + [ check-target-builds ../config//has_fdopendir_nofollow "has fdopendir(O_NOFOLLOW)" : BOOST_FILESYSTEM_HAS_FDOPENDIR_NOFOLLOW ] @check-statx @select-windows-crypto-api @check-cxx20-atomic-ref diff --git a/libs/filesystem/config/Jamfile.v2 b/libs/filesystem/config/Jamfile.v2 index 2e9ee4bd6..5ea7dc6c7 100644 --- a/libs/filesystem/config/Jamfile.v2 +++ b/libs/filesystem/config/Jamfile.v2 @@ -27,6 +27,8 @@ obj has_stat_st_birthtimensec : has_stat_st_birthtimensec.cpp : ../src explicit has_stat_st_birthtimensec ; obj has_stat_st_birthtimespec : has_stat_st_birthtimespec.cpp : ../src ; explicit has_stat_st_birthtimespec ; +obj has_fdopendir_nofollow : has_fdopendir_nofollow.cpp : ../src ; +explicit has_fdopendir_nofollow ; lib bcrypt ; explicit bcrypt ; diff --git a/libs/filesystem/config/has_fdopendir_nofollow.cpp b/libs/filesystem/config/has_fdopendir_nofollow.cpp new file mode 100644 index 000000000..009a73084 --- /dev/null +++ b/libs/filesystem/config/has_fdopendir_nofollow.cpp @@ -0,0 +1,20 @@ +// Copyright 2022 Andrey Semashev + +// Distributed under the Boost Software License, Version 1.0. +// See http://www.boost.org/LICENSE_1_0.txt + +// See library home page at http://www.boost.org/libs/filesystem + +#include "platform_config.hpp" + +#include +#include +#include +#include + +int main() +{ + int fd = open(".", O_DIRECTORY | O_RDONLY | O_NOFOLLOW); + DIR* dir = fdopendir(fd); + return dir != 0; +} diff --git a/boost/filesystem/directory.hpp b/boost/filesystem/directory.hpp index f487633fc..fa1e69be5 100644 --- a/boost/filesystem/directory.hpp +++ b/boost/filesystem/directory.hpp @@ -247,7 +247,8 @@ BOOST_SCOPED_ENUM_UT_DECLARE_BEGIN(directory_options, unsigned int) skip_dangling_symlinks = 1u << 2, // non-standard extension for recursive_directory_iterator: don't follow dangling directory symlinks, pop_on_error = 1u << 3, // non-standard extension for recursive_directory_iterator: instead of producing an end iterator on errors, // repeatedly invoke pop() until it succeeds or the iterator becomes equal to end iterator - _detail_no_push = 1u << 4 // internal use only + _detail_no_follow = 1u << 4, // internal use only + _detail_no_push = 1u << 5 // internal use only } BOOST_SCOPED_ENUM_DECLARE_END(directory_options) diff --git a/libs/filesystem/src/directory.cpp b/libs/filesystem/src/directory.cpp index 554277580..8b41ae5e1 100644 --- a/libs/filesystem/src/directory.cpp +++ b/libs/filesystem/src/directory.cpp @@ -2,7 +2,7 @@ // Copyright 2002-2009, 2014 Beman Dawes // Copyright 2001 Dietmar Kuehl -// Copyright 2019 Andrey Semashev +// Copyright 2019, 2022 Andrey Semashev // Distributed under the Boost Software License, Version 1.0. // See http://www.boost.org/LICENSE_1_0.txt @@ -33,6 +33,8 @@ #ifdef BOOST_POSIX_API +#include +#include #include #include #include @@ -47,6 +49,14 @@ #define BOOST_FILESYSTEM_USE_READDIR_R #endif +// At least Mac OS X 10.6 and older doesn't support O_CLOEXEC +#ifndef O_CLOEXEC +#define O_CLOEXEC 0 +#define BOOST_FILESYSTEM_NO_O_CLOEXEC +#endif + +#include "posix_tools.hpp" + #else // BOOST_WINDOWS_API #include @@ -206,13 +216,46 @@ inline std::size_t path_max() #endif // BOOST_FILESYSTEM_USE_READDIR_R -error_code dir_itr_first(void*& handle, void*& buffer, const char* dir, std::string& target, fs::file_status&, fs::file_status&) +error_code dir_itr_first(void*& handle, void*& buffer, const char* dir, std::string& target, unsigned int opts, fs::file_status&, fs::file_status&) { - if ((handle = ::opendir(dir)) == 0) +#if defined(BOOST_FILESYSTEM_HAS_FDOPENDIR_NOFOLLOW) + int flags = O_DIRECTORY | O_RDONLY | O_NDELAY | O_CLOEXEC; + if ((opts & static_cast< unsigned int >(directory_options::_detail_no_follow)) != 0u) + flags |= O_NOFOLLOW; + + int fd = ::open(dir, flags); + if (BOOST_UNLIKELY(fd < 0)) + { + const int err = errno; + return error_code(err, system_category()); + } + +#if defined(BOOST_FILESYSTEM_NO_O_CLOEXEC) && defined(FD_CLOEXEC) + int res = ::fcntl(fd, F_SETFD, FD_CLOEXEC); + if (BOOST_UNLIKELY(res < 0)) + { + const int err = errno; + close_fd(fd); + return error_code(err, system_category()); + } +#endif + + handle = ::fdopendir(fd); + if (BOOST_UNLIKELY(!handle)) { const int err = errno; + close_fd(fd); return error_code(err, system_category()); } +#else // defined(BOOST_FILESYSTEM_HAS_FDOPENDIR_NOFOLLOW) + handle = ::opendir(dir); + if (BOOST_UNLIKELY(!handle)) + { + const int err = errno; + return error_code(err, system_category()); + } +#endif // defined(BOOST_FILESYSTEM_HAS_FDOPENDIR_NOFOLLOW) + target.assign("."); // string was static but caused trouble // when iteration called from dtor, after // static had already been destroyed @@ -342,7 +385,7 @@ error_code dir_itr_increment(void*& handle, void*& buffer, std::string& target, #else // BOOST_WINDOWS_API -error_code dir_itr_first(void*& handle, fs::path const& dir, std::wstring& target, fs::file_status& sf, fs::file_status& symlink_sf) +error_code dir_itr_first(void*& handle, fs::path const& dir, std::wstring& target, unsigned int opts, fs::file_status& sf, fs::file_status& symlink_sf) // Note: an empty root directory has no "." or ".." entries, so this // causes a ERROR_FILE_NOT_FOUND error which we do not considered an // error. It is treated as eof instead. @@ -512,7 +555,7 @@ void directory_iterator_construct(directory_iterator& it, path const& p, unsigne #if defined(BOOST_POSIX_API) imp->buffer, #endif - p.c_str(), filename, file_stat, symlink_file_stat); + p.c_str(), filename, opts, file_stat, symlink_file_stat); if (result) { diff --git a/libs/filesystem/src/operations.cpp b/libs/filesystem/src/operations.cpp index e14b598b7..bf42105b9 100644 --- a/libs/filesystem/src/operations.cpp +++ b/libs/filesystem/src/operations.cpp @@ -945,51 +945,73 @@ inline bool remove_impl(path const& p, error_code* ec) //! remove_all() implementation uintmax_t remove_all_impl(path const& p, error_code* ec) { - fs::file_type type; + for (unsigned int attempt = 0u; attempt < 5u; ++attempt) { - error_code local_ec; - type = fs::detail::symlink_status(p, &local_ec).type(); + fs::file_type type; + { + error_code local_ec; + type = fs::detail::symlink_status(p, &local_ec).type(); - if (type == fs::file_not_found) - return 0u; + if (type == fs::file_not_found) + return 0u; - if (BOOST_UNLIKELY(type == fs::status_error)) - { - if (!ec) - BOOST_FILESYSTEM_THROW(filesystem_error("boost::filesystem::remove_all", p, local_ec)); + if (BOOST_UNLIKELY(type == fs::status_error)) + { + if (!ec) + BOOST_FILESYSTEM_THROW(filesystem_error("boost::filesystem::remove_all", p, local_ec)); - *ec = local_ec; - return static_cast< uintmax_t >(-1); + *ec = local_ec; + return static_cast< uintmax_t >(-1); + } } - } - uintmax_t count = 0u; + uintmax_t count = 0u; - if (type == fs::directory_file) // but not a directory symlink - { - fs::directory_iterator itr; - fs::detail::directory_iterator_construct(itr, p, static_cast< unsigned int >(directory_options::none), ec); - if (ec && *ec) - return static_cast< uintmax_t >(-1); - - const fs::directory_iterator end_dit; - while (itr != end_dit) + if (type == fs::directory_file) // but not a directory symlink { - count += fs::detail::remove_all_impl(itr->path(), ec); - if (ec && *ec) - return static_cast< uintmax_t >(-1); + fs::directory_iterator itr; + error_code local_ec; + fs::detail::directory_iterator_construct(itr, p, static_cast< unsigned int >(directory_options::_detail_no_follow), &local_ec); + if (BOOST_UNLIKELY(!!local_ec)) + { +#if defined(BOOST_FILESYSTEM_HAS_FDOPENDIR_NOFOLLOW) + // If open(2) with O_NOFOLLOW fails with ELOOP, this means that either the path contains a loop + // of symbolic links, or the last element of the path is a symbolic link. Given that lstat(2) above + // did not fail, most likely it is the latter case. I.e. between the lstat above and this open call + // the filesystem was modified so that the path no longer refers to a directory file (as opposed to a symlink). + if (local_ec == error_code(ELOOP, system_category())) + continue; +#endif // defined(BOOST_FILESYSTEM_HAS_FDOPENDIR_NOFOLLOW) - fs::detail::directory_iterator_increment(itr, ec); - if (ec && *ec) + if (!ec) + BOOST_FILESYSTEM_THROW(filesystem_error("boost::filesystem::remove_all", p, local_ec)); + + *ec = local_ec; return static_cast< uintmax_t >(-1); + } + + const fs::directory_iterator end_dit; + while (itr != end_dit) + { + count += fs::detail::remove_all_impl(itr->path(), ec); + if (ec && *ec) + return static_cast< uintmax_t >(-1); + + fs::detail::directory_iterator_increment(itr, ec); + if (ec && *ec) + return static_cast< uintmax_t >(-1); + } } - } - count += fs::detail::remove_impl(p, type, ec); - if (ec && *ec) - return static_cast< uintmax_t >(-1); + count += fs::detail::remove_impl(p, type, ec); + if (ec && *ec) + return static_cast< uintmax_t >(-1); - return count; + return count; + } + + emit_error(ELOOP, p, ec, "boost::filesystem::remove_all: path cannot be opened as a directory"); + return static_cast< uintmax_t >(-1); } #else // defined(BOOST_POSIX_API)