public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug c++/104161] New: Potential Security Vulnerability: remove_all and symbolic link
@ 2022-01-21 11:47 adrien.devresse at metamorphe dot engineering
  2022-01-21 13:02 ` [Bug libstdc++/104161] " redi at gcc dot gnu.org
                   ` (11 more replies)
  0 siblings, 12 replies; 13+ messages in thread
From: adrien.devresse at metamorphe dot engineering @ 2022-01-21 11:47 UTC (permalink / raw)
  To: gcc-bugs

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

            Bug ID: 104161
           Summary: Potential Security Vulnerability: remove_all and
                    symbolic link
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: adrien.devresse at metamorphe dot engineering
  Target Milestone: ---

Dear all,

The Rust compiler recently reported a CVE about a vulnerability about the
recursive deletion on the filesystem and the use of symlink.

https://blog.rust-lang.org/2022/01/20/cve-2022-21658.html


Ater a quick check, I do believe the implementation of
std::filesystem::remove_all presents the same vulnerability

https://en.cppreference.com/w/cpp/filesystem/remove

The logic rely on directory_iterator NOT opening symbolic link liked mandated
by the standard

https://en.cppreference.com/w/cpp/filesystem/directory_iterator


https://github.com/playbar/cpplearn/blob/master/gcc/libstdc%2B%2B-v3/src/c%2B%2B17/fs_ops.cc#L1300

However, it seems GCC implementation of directory_iterator does open and follow
symbolic links when used on it

https://godbolt.org/#z:OYLghAFBqd5QCxAYwPYBMCmBRdBLAF1QCcAaPECAMzwBtMA7AQwFtMQByARg9KtQYEAysib0QXACx8BBAKoBnTAAUAHpwAMvAFYTStJg1DIApACYAQuYukl9ZATwDKjdAGFUtAK4sGIM9KuADJ4DJgAcj4ARpjE/tIADqgKhE4MHt6%2B8aRJKY4CIWGRLDFxAbaY9vkMQgRMxAQZPn7ldpgOabX1BIUR0bHZCnUNTVmtwz2hfSUDAQCUtqhexMjsHOYAzKHI3lgA1CYbbk5DxJish9gmGgCCm9u7mAdHNPQKAJ5DmCyX13dmWwYOy8%2B0Obi8DDwQ3QADoEL9bn9QgQ9iwmKEIMi9vVgMhSHtkAh6gAqYnY4jAABucxMAHYrLc9kyCQIhgSicQ9mSFLRQgBrZ4AEQOZjMAHoCCwEhLMENzGZDhYDoiVTdmXsxWKCWcmAQnh8WLyGHy/uq8FQIHsDUa%2BRB5WLMARTKL8Tz%2BXMmWAwIdhRoafTTermdCQChYpywWCRWY9lR0bRtedHEYre9DfyDgBWNwMeWKwNBvaYVSECBcGkbBlq5l0wV/euM5mavZ6tmEU3XACcIZA4ZIAH00Pt2oqayrO0wvEQ9oQhVaCOhQ69ZZ89SxQ/gzg4SO9%2B4RYrqSBA3cb8e0K1WC/PFyglijI0dozF%2BGc9ryhqFgCZs7nRfnxy%2BECTtO5gAGyxnsIAzgQ/qXhonZdj2aBTs8bhRlQqFRvK0aYY%2BeY/nmlYNghtJ1o2TJIXeuFuNGTBUHqnLvsmX4EX%2BRGIqRfwcAstCcJmvB%2BBwWikKgnBoZY1hWksKxPJsPCkAQmjcQsfIgBsGwwupWnaTpoH6JwkgCUpImcLwCggBoClKQscCwEgaBSnQsTkJQDkJE5cTAFwXBmHwdAMeZEBRMZUShPU7ycPJDlsIIADyDC0BFQm8FgaJGOIyWkJu7SOJSsrGcW7RTmswnIpUxm8lExDhR4WDGQQxB4Ou3DcXwBjAAoABqeCYAA7rFCSMJFvD8IIIhiOwUgyIIigqOomW6Fw%2BiGMY1jWPoeBROZkALKgCTVOZHAALTQj6zpWJYXC0mZlQ5WkLgMO4njNHowRTMUpR6LkqQCKMfhLd91S9B9AxLW0HQCF0IzPVkYO3RDNQTMD/RxGDEx/XoQzdMjMyowsCjSasegNZgaw8DxfFGZlokcKoAAcoFHaBkh7LiyB7N5MIxhA4kXTYey4IQJAihsS17B4jn0BGALlrwinJXMKlqRpOmq1pem8RwhmkIJwk02ZFlWQrpC2YgIB3gkU4uRAbkea9%2BBEKj03CKI4hTaN8hKGo9VnGTvC9dVCTDRTHD8Trxk07FU6WyiqAYfTjPM6zyDs5z3MS%2B5Usi7LRtaIrpCqepmlq6r%2Bla1TeumbYhvy3nIdmBXvD67nymkHlxApM4khAA


Consequently, GCC seems logicially vulnerable to this attack.

Regards,

A.D

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Bug libstdc++/104161] Potential Security Vulnerability: remove_all and symbolic link
  2022-01-21 11:47 [Bug c++/104161] New: Potential Security Vulnerability: remove_all and symbolic link adrien.devresse at metamorphe dot engineering
@ 2022-01-21 13:02 ` redi at gcc dot gnu.org
  2022-01-21 14:27 ` adrien.devresse at metamorphe dot engineering
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: redi at gcc dot gnu.org @ 2022-01-21 13:02 UTC (permalink / raw)
  To: gcc-bugs

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|c++                         |libstdc++
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2022-01-21
     Ever confirmed|0                           |1

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I already have a patch.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Bug libstdc++/104161] Potential Security Vulnerability: remove_all and symbolic link
  2022-01-21 11:47 [Bug c++/104161] New: Potential Security Vulnerability: remove_all and symbolic link adrien.devresse at metamorphe dot engineering
  2022-01-21 13:02 ` [Bug libstdc++/104161] " redi at gcc dot gnu.org
@ 2022-01-21 14:27 ` adrien.devresse at metamorphe dot engineering
  2022-01-25 21:09 ` cvs-commit at gcc dot gnu.org
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: adrien.devresse at metamorphe dot engineering @ 2022-01-21 14:27 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #2 from Adev <adrien.devresse at metamorphe dot engineering> ---
> I already have a patch.

Thanks Jonathan.

Simple curiosity: Was that already reported before ?

If you do need any testing, do not hesitate.


A.D

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Bug libstdc++/104161] Potential Security Vulnerability: remove_all and symbolic link
  2022-01-21 11:47 [Bug c++/104161] New: Potential Security Vulnerability: remove_all and symbolic link adrien.devresse at metamorphe dot engineering
  2022-01-21 13:02 ` [Bug libstdc++/104161] " redi at gcc dot gnu.org
  2022-01-21 14:27 ` adrien.devresse at metamorphe dot engineering
@ 2022-01-25 21:09 ` cvs-commit at gcc dot gnu.org
  2022-01-26  0:30 ` redi at gcc dot gnu.org
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2022-01-25 21:09 UTC (permalink / raw)
  To: gcc-bugs

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

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

https://gcc.gnu.org/g:c8bd4dc8212e43b2f9af08b80df97f90cdb0df4f

commit r12-6866-gc8bd4dc8212e43b2f9af08b80df97f90cdb0df4f
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Sun Jan 23 21:45:16 2022 +0000

    libstdc++: Avoid symlink race in filesystem::remove_all [PR104161]

    This adds a new internal flag to the filesystem::directory_iterator
    constructor that makes it fail if the path is a symlink that resolves to
    a directory. This prevents filesystem::remove_all from following a
    symlink to a directory, rather than deleting the symlink itself.

    We can also use that new flag in recursive_directory_iterator to ensure
    that we don't follow symlinks if the follow_directory_symlink option is
    not set.

    This also moves an error check in filesystem::remove_all after the while
    loop, so that errors from the directory_iterator constructor are
    reproted, instead of continuing to the filesystem::remove call below.

    libstdc++-v3/ChangeLog:

            PR libstdc++/104161
            * acinclude.m4 (GLIBCXX_CHECK_FILESYSTEM_DEPS): Check for
            fdopendir.
            * config.h.in: Regenerate.
            * configure: Regenerate.
            * src/c++17/fs_dir.cc (_Dir): Add nofollow flag to constructor
            and pass it to base class constructor.
            (directory_iterator): Pass nofollow flag to _Dir constructor.
            (fs::recursive_directory_iterator::increment): Likewise.
            * src/c++17/fs_ops.cc (do_remove_all): Use nofollow option for
            directory_iterator constructor. Move error check outside loop.
            * src/filesystem/dir-common.h (_Dir_base): Add nofollow flag to
            constructor and when it's set use ::open with O_NOFOLLOW and
            O_DIRECTORY.
            * src/filesystem/dir.cc (_Dir): Add nofollow flag to constructor
            and pass it to base class constructor.
            (directory_iterator): Pass nofollow flag to _Dir constructor.
            (fs::recursive_directory_iterator::increment): Likewise.
            * src/filesystem/ops.cc (remove_all): Use nofollow option for
            directory_iterator constructor. Move error check outside loop.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Bug libstdc++/104161] Potential Security Vulnerability: remove_all and symbolic link
  2022-01-21 11:47 [Bug c++/104161] New: Potential Security Vulnerability: remove_all and symbolic link adrien.devresse at metamorphe dot engineering
                   ` (2 preceding siblings ...)
  2022-01-25 21:09 ` cvs-commit at gcc dot gnu.org
@ 2022-01-26  0:30 ` redi at gcc dot gnu.org
  2022-01-27  1:51 ` jistone at redhat dot com
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: redi at gcc dot gnu.org @ 2022-01-26  0:30 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This reduces the scope of the problem, but if the directory is replaced with a
symlink *after* constructing the directory_iterator then filesystem::remove can
still remove the wrong things.

We really need to unlinkat, using a file descriptor obtained from dirfd on the
iterator's DIR*.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Bug libstdc++/104161] Potential Security Vulnerability: remove_all and symbolic link
  2022-01-21 11:47 [Bug c++/104161] New: Potential Security Vulnerability: remove_all and symbolic link adrien.devresse at metamorphe dot engineering
                   ` (3 preceding siblings ...)
  2022-01-26  0:30 ` redi at gcc dot gnu.org
@ 2022-01-27  1:51 ` jistone at redhat dot com
  2022-01-27  8:29 ` redi at gcc dot gnu.org
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: jistone at redhat dot com @ 2022-01-27  1:51 UTC (permalink / raw)
  To: gcc-bugs

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

Josh Stone <jistone at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jistone at redhat dot com

--- Comment #5 from Josh Stone <jistone at redhat dot com> ---
You also need to use openat, because O_NOFOLLOW only affects the *trailing*
component of the path. That is, if we're removing "/base" and you're down to
"/base/foo/bar", I could change "/base/foo" to a symlink and then you'll open
"/other/bar".

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Bug libstdc++/104161] Potential Security Vulnerability: remove_all and symbolic link
  2022-01-21 11:47 [Bug c++/104161] New: Potential Security Vulnerability: remove_all and symbolic link adrien.devresse at metamorphe dot engineering
                   ` (4 preceding siblings ...)
  2022-01-27  1:51 ` jistone at redhat dot com
@ 2022-01-27  8:29 ` redi at gcc dot gnu.org
  2022-02-04 23:50 ` cvs-commit at gcc dot gnu.org
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: redi at gcc dot gnu.org @ 2022-01-27  8:29 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Yes, I have a new ctor for directory_iterator that takes the FD and does all
operations relative to that.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Bug libstdc++/104161] Potential Security Vulnerability: remove_all and symbolic link
  2022-01-21 11:47 [Bug c++/104161] New: Potential Security Vulnerability: remove_all and symbolic link adrien.devresse at metamorphe dot engineering
                   ` (5 preceding siblings ...)
  2022-01-27  8:29 ` redi at gcc dot gnu.org
@ 2022-02-04 23:50 ` cvs-commit at gcc dot gnu.org
  2022-02-08 13:40 ` cvs-commit at gcc dot gnu.org
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2022-02-04 23:50 UTC (permalink / raw)
  To: gcc-bugs

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

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

https://gcc.gnu.org/g:ebf6175464768983a2d8c82c2d47771ee89192b8

commit r12-7062-gebf6175464768983a2d8c82c2d47771ee89192b8
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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Bug libstdc++/104161] Potential Security Vulnerability: remove_all and symbolic link
  2022-01-21 11:47 [Bug c++/104161] New: Potential Security Vulnerability: remove_all and symbolic link adrien.devresse at metamorphe dot engineering
                   ` (6 preceding siblings ...)
  2022-02-04 23:50 ` cvs-commit at gcc dot gnu.org
@ 2022-02-08 13:40 ` cvs-commit at gcc dot gnu.org
  2023-10-04 11:28 ` cvs-commit at gcc dot gnu.org
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2022-02-08 13:40 UTC (permalink / raw)
  To: gcc-bugs

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

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

https://gcc.gnu.org/g:5750952bec1e632d1f804f4a1bed2f74c0f3b189

commit r12-7099-g5750952bec1e632d1f804f4a1bed2f74c0f3b189
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Mon Feb 7 23:36:47 2022 +0000

    libstdc++: Fix filesystem::remove_all for Windows [PR104161]

    The recursive_directory_iterator::__erase member was failing for
    Windows, because the entry._M_type value is always file_type::none
    (because _Dir_base::advance doesn't populate it for Windows) and
    top.unlink uses fs::remove which sets an error using the
    system_category. That meant that ec.value() was a Windows error code and
    not an errno value, so the comparisons to EPERM and EISDIR failed.
    Instead of depending on a specific Windows error code for attempting to
    remove a directory, just use directory_entry::refresh() to query the
    type first. This doesn't avoid the TOCTTOU races with directory
    symlinks, but we can't avoid them on Windows without openat and
    unlinkat, and creating symlinks requires admin privs on Windows anyway.

    This also fixes the fs::remove_all(const path&) overload, which was
    supposed to use the same logic as the other overload, but I forgot to
    change it before my previous commit.

    libstdc++-v3/ChangeLog:

            PR libstdc++/104161
            * src/c++17/fs_dir.cc (fs::recursive_directory_iterator::__erase):
            [i_GLIBCXX_FILESYSTEM_IS_WINDOWS]: Refresh entry._M_type member,
            instead of checking for errno values indicating a directory.
            * src/c++17/fs_ops.cc (fs::remove_all(const path&)): Use similar
            logic to non-throwing overload.
            (fs::remove_all(const path&, error_code&)): Add comments.
            * src/filesystem/ops-common.h: Likewise.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Bug libstdc++/104161] Potential Security Vulnerability: remove_all and symbolic link
  2022-01-21 11:47 [Bug c++/104161] New: Potential Security Vulnerability: remove_all and symbolic link adrien.devresse at metamorphe dot engineering
                   ` (7 preceding siblings ...)
  2022-02-08 13:40 ` cvs-commit at gcc dot gnu.org
@ 2023-10-04 11:28 ` cvs-commit at gcc dot gnu.org
  2023-10-04 11:28 ` cvs-commit at gcc dot gnu.org
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2023-10-04 11:28 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #9 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:e20b1c711f70d8d251d45694c50c02e66a4d9f7b

commit r11-11036-ge20b1c711f70d8d251d45694c50c02e66a4d9f7b
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Sun Jan 23 21:45:16 2022 +0000

    libstdc++: Avoid symlink race in filesystem::remove_all [PR104161]

    This adds a new internal flag to the filesystem::directory_iterator
    constructor that makes it fail if the path is a symlink that resolves to
    a directory. This prevents filesystem::remove_all from following a
    symlink to a directory, rather than deleting the symlink itself.

    We can also use that new flag in recursive_directory_iterator to ensure
    that we don't follow symlinks if the follow_directory_symlink option is
    not set.

    This also moves an error check in filesystem::remove_all after the while
    loop, so that errors from the directory_iterator constructor are
    reproted, instead of continuing to the filesystem::remove call below.

    libstdc++-v3/ChangeLog:

            PR libstdc++/104161
            * acinclude.m4 (GLIBCXX_CHECK_FILESYSTEM_DEPS): Check for
            fdopendir.
            * config.h.in: Regenerate.
            * configure: Regenerate.
            * src/c++17/fs_dir.cc (_Dir): Add nofollow flag to constructor
            and pass it to base class constructor.
            (directory_iterator): Pass nofollow flag to _Dir constructor.
            (fs::recursive_directory_iterator::increment): Likewise.
            * src/c++17/fs_ops.cc (do_remove_all): Use nofollow option for
            directory_iterator constructor. Move error check outside loop.
            * src/filesystem/dir-common.h (_Dir_base): Add nofollow flag to
            constructor and when it's set use ::open with O_NOFOLLOW and
            O_DIRECTORY.
            * src/filesystem/dir.cc (_Dir): Add nofollow flag to constructor
            and pass it to base class constructor.
            (directory_iterator): Pass nofollow flag to _Dir constructor.
            (fs::recursive_directory_iterator::increment): Likewise.
            * src/filesystem/ops.cc (remove_all): Use nofollow option for
            directory_iterator constructor. Move error check outside loop.

    (cherry picked from commit c8bd4dc8212e43b2f9af08b80df97f90cdb0df4f)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Bug libstdc++/104161] Potential Security Vulnerability: remove_all and symbolic link
  2022-01-21 11:47 [Bug c++/104161] New: Potential Security Vulnerability: remove_all and symbolic link adrien.devresse at metamorphe dot engineering
                   ` (8 preceding siblings ...)
  2023-10-04 11:28 ` cvs-commit at gcc dot gnu.org
@ 2023-10-04 11:28 ` cvs-commit at gcc dot gnu.org
  2023-10-04 11:28 ` cvs-commit at gcc dot gnu.org
  2023-10-04 11:33 ` redi at gcc dot gnu.org
  11 siblings, 0 replies; 13+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2023-10-04 11:28 UTC (permalink / raw)
  To: gcc-bugs

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)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Bug libstdc++/104161] Potential Security Vulnerability: remove_all and symbolic link
  2022-01-21 11:47 [Bug c++/104161] New: Potential Security Vulnerability: remove_all and symbolic link adrien.devresse at metamorphe dot engineering
                   ` (9 preceding siblings ...)
  2023-10-04 11:28 ` cvs-commit at gcc dot gnu.org
@ 2023-10-04 11:28 ` cvs-commit at gcc dot gnu.org
  2023-10-04 11:33 ` redi at gcc dot gnu.org
  11 siblings, 0 replies; 13+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2023-10-04 11:28 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #11 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:e742c6baa92403767b4ba8227f34fc7919db28e0

commit r11-11039-ge742c6baa92403767b4ba8227f34fc7919db28e0
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Mon Feb 7 23:36:47 2022 +0000

    libstdc++: Fix filesystem::remove_all for Windows [PR104161]

    The recursive_directory_iterator::__erase member was failing for
    Windows, because the entry._M_type value is always file_type::none
    (because _Dir_base::advance doesn't populate it for Windows) and
    top.unlink uses fs::remove which sets an error using the
    system_category. That meant that ec.value() was a Windows error code and
    not an errno value, so the comparisons to EPERM and EISDIR failed.
    Instead of depending on a specific Windows error code for attempting to
    remove a directory, just use directory_entry::refresh() to query the
    type first. This doesn't avoid the TOCTTOU races with directory
    symlinks, but we can't avoid them on Windows without openat and
    unlinkat, and creating symlinks requires admin privs on Windows anyway.

    This also fixes the fs::remove_all(const path&) overload, which was
    supposed to use the same logic as the other overload, but I forgot to
    change it before my previous commit.

    libstdc++-v3/ChangeLog:

            PR libstdc++/104161
            * src/c++17/fs_dir.cc (fs::recursive_directory_iterator::__erase):
            [i_GLIBCXX_FILESYSTEM_IS_WINDOWS]: Refresh entry._M_type member,
            instead of checking for errno values indicating a directory.
            * src/c++17/fs_ops.cc (fs::remove_all(const path&)): Use similar
            logic to non-throwing overload.
            (fs::remove_all(const path&, error_code&)): Add comments.
            * src/filesystem/ops-common.h: Likewise.

    (cherry picked from commit 5750952bec1e632d1f804f4a1bed2f74c0f3b189)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Bug libstdc++/104161] Potential Security Vulnerability: remove_all and symbolic link
  2022-01-21 11:47 [Bug c++/104161] New: Potential Security Vulnerability: remove_all and symbolic link adrien.devresse at metamorphe dot engineering
                   ` (10 preceding siblings ...)
  2023-10-04 11:28 ` cvs-commit at gcc dot gnu.org
@ 2023-10-04 11:33 ` redi at gcc dot gnu.org
  11 siblings, 0 replies; 13+ messages in thread
From: redi at gcc dot gnu.org @ 2023-10-04 11:33 UTC (permalink / raw)
  To: gcc-bugs

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|---                         |11.5

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed for 11.5 and later.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-10-04 11:33 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-21 11:47 [Bug c++/104161] New: Potential Security Vulnerability: remove_all and symbolic link adrien.devresse at metamorphe dot engineering
2022-01-21 13:02 ` [Bug libstdc++/104161] " redi at gcc dot gnu.org
2022-01-21 14:27 ` adrien.devresse at metamorphe dot engineering
2022-01-25 21:09 ` cvs-commit at gcc dot gnu.org
2022-01-26  0:30 ` redi at gcc dot gnu.org
2022-01-27  1:51 ` jistone at redhat dot com
2022-01-27  8:29 ` redi at gcc dot gnu.org
2022-02-04 23:50 ` cvs-commit at gcc dot gnu.org
2022-02-08 13:40 ` cvs-commit at gcc dot gnu.org
2023-10-04 11:28 ` cvs-commit at gcc dot gnu.org
2023-10-04 11:28 ` cvs-commit at gcc dot gnu.org
2023-10-04 11:28 ` cvs-commit at gcc dot gnu.org
2023-10-04 11:33 ` redi at gcc dot gnu.org

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).