[Bug libstdc++/104161] Potential Security Vulnerability: remove_all and symbolic link

["cvs-commit at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104161

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:5e6ce4031ce6eba3ea8b2b75bcacb869b591b92c

commit r11-11037-g5e6ce4031ce6eba3ea8b2b75bcacb869b591b92c
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Feb 1 22:04:46 2022 +0000

    libstdc++: Fix filesystem::remove_all races [PR104161]

    This fixes the remaining filesystem::remove_all race condition by using
    POSIX openat to recurse into sub-directories and using POSIX unlinkat to
    remove files. This avoids the remaining race where the directory being
    removed is replaced with a symlink after the directory has been opened,
    so that the filesystem::remove("subdir/file") resolves to "target/file"
    instead, because "subdir" has been removed and replaced with a symlink.
    The previous patch only fixed the case where the directory was replaced
    with a symlink before we tried to open it, but it still used the full
    (potentially compromised) path as an argument to filesystem::remove.

    The first part of the fix is to use openat when recursing into a
    sub-directory with recursive_directory_iterator. This means that opening
    "dir/subdir" uses the file descriptor for "dir", and so is sure to open
    "dir/subdir" and not "symlink/subdir". (The previous patch to use
    O_NOFOLLOW already ensured we won't open "dir/symlink/" here.)

    The second part of the fix is to use unlinkat for the remove_all
    operation. Previously we used a directory_iterator to get the name of
    each file in a directory and then used filesystem::remove(iter->path())
    on that name. This meant that any checks (e.g. O_NOFOLLOW) done by the
    iterator could be invalidated before the remove operation on that
    pathname. The directory iterator contains an open DIR stream, which we
    can use to obtain a file descriptor to pass to unlinkat. This ensures
    that the file being deleted really is contained within the directory
    we're iterating over, rather than using a pathname that could resolve to
    some other file.

    The filesystem::remove_all function previously used a (non-recursive)
    filesystem::directory_iterator for each directory, and called itself
    recursively for sub-directories. The new implementation uses a single
    filesystem::recursive_directory_iterator object, and calls a new __erase
    member function on that iterator. That new __erase member function does
    the actual work of removing a file (or a directory after its contents
    have been iterated over and removed) using unlinkat. That means we don't
    need to expose the DIR stream or its file descriptor to the remove_all
    function, it's still encapuslated by the iterator class.

    It would be possible to add a __rewind member to directory iterators
    too, to call rewinddir after each modification to the directory. That
    would make it more likely for filesystem::remove_all to successfully
    remove everything even if files are being written to the directory tree
    while removing it. It's unclear if that is actually prefereable, or if
    it's better to fail and report an error at the first opportunity.

    The necessary APIs (openat, unlinkat, fdopendir, dirfd) are defined in
    POSIX.1-2008, and in Glibc since 2.10. But if the target doesn't provide
    them, the original code (with race conditions) is still used.

    This also reduces the number of small memory allocations needed for
    std::filesystem::remove_all, because we do not store the full path to
    every directory entry that is iterated over. The new filename_only
    option means we only store the filename in the directory entry, as that
    is all we need in order to use openat or unlinkat.

    Finally, rather than duplicating everything for the Filesystem TS, the
    std::experimental::filesystem::remove_all implementation now just calls
    std::filesystem::remove_all to do the work.

    libstdc++-v3/ChangeLog:

            PR libstdc++/104161
            * acinclude.m4 (GLIBCXX_CHECK_FILESYSTEM_DEPS): Check for dirfd
            and unlinkat.
            * config.h.in: Regenerate.
            * configure: Regenerate.
            * include/bits/fs_dir.h (recursive_directory_iterator): Declare
            remove_all overloads as friends.
            (recursive_directory_iterator::__erase): Declare new member
            function.
            * include/bits/fs_fwd.h (remove, remove_all): Declare.
            * src/c++17/fs_dir.cc (_Dir): Add filename_only parameter to
            constructor. Pass file descriptor argument to base constructor.
            (_Dir::dir_and_pathname, _Dir::open_subdir, _Dir::do_unlink)
            (_Dir::unlink, _Dir::rmdir): Define new member functions.
            (directory_iterator): Pass filename_only argument to _Dir
            constructor.
            (recursive_directory_iterator::_Dir_stack): Adjust constructor
            parameters to take a _Dir rvalue instead of creating one.
            (_Dir_stack::orig): Add data member for storing original path.
            (_Dir_stack::report_error): Define new member function.
            (__directory_iterator_nofollow): Move here from dir-common.h and
            fix value to be a power of two.
            (__directory_iterator_filename_only): Define new constant.
            (recursive_directory_iterator): Construct _Dir object and move
            into _M_dirs stack. Pass skip_permission_denied argument to first
            advance call.
            (recursive_directory_iterator::increment): Use _Dir::open_subdir.
            (recursive_directory_iterator::__erase): Define new member
            function.
            * src/c++17/fs_ops.cc (ErrorReporter, do_remove_all): Remove.
            (fs::remove_all): Use new recursive_directory_iterator::__erase
            member function.
            * src/filesystem/dir-common.h (_Dir_base): Add int parameter to
            constructor and use openat to implement nofollow semantics.
            (_Dir_base::fdcwd, _Dir_base::set_close_on_exec,
_Dir_base::openat):
            Define new member functions.
            (__directory_iterator_nofollow): Move to fs_dir.cc.
            * src/filesystem/dir.cc (_Dir): Pass file descriptor argument to
            base constructor.
            (_Dir::dir_and_pathname, _Dir::open_subdir): Define new member
            functions.
            (recursive_directory_iterator::_Dir_stack): Adjust constructor
            parameters to take a _Dir rvalue instead of creating one.
            (recursive_directory_iterator): Check for new nofollow option.
            Construct _Dir object and move into _M_dirs stack. Pass
            skip_permission_denied argument to first advance call.
            (recursive_directory_iterator::increment): Use _Dir::open_subdir.
            * src/filesystem/ops.cc (fs::remove_all): Use C++17 remove_all.

    (cherry picked from commit ebf6175464768983a2d8c82c2d47771ee89192b8)