* [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