Open joeyh opened 5 years ago
I think it's possible to use O_NOFOLLOW
internally for removePathRecursive
, but this cannot be done for listDirectory
as that would change its behavior on symlinks.
It would necessary to have fdopendir
implemented upstream in unix
though. Can you file a feature request on that?
Any such implementation would need to support Windows. I think it is possible, given that Cygwin implements it, but it involves touching some rather exotic APIs like NtQueryDirectoryFile
: https://github.com/Alexpux/Cygwin/blob/b39cd00f07e8df11c5446fb35da490ef85f12821/winsup/cygwin/fhandler_disk_file.cc#L2186
It's also important to define the behavior of dir_nofollow
on POSIXes that don't support O_NOFOLLOW
. For removeDirectoryRecursive
we should probably just fall back to a non-atomic solution.
If we directly expose the ability to read directories with an optional O_NOFOLLOW
flag, then I think the safest and simplest fallback would be to just raise an exception and let the user decide what non-atomic alternative they want to use.
Symlinks are much more common on unix than Windows, and this is a security hole, so IMHO it should be fixed ASAP on unixes that support O_NOFOLLOW
without being blocked on a fix being implemented for Windows.
Also, Rust recently fixed the same issue, and their patches might provide some useful pointers for eg Windows. https://blog.rust-lang.org/2022/01/20/cve-2022-21658.html https://github.com/rust-lang/wg-security-response/blob/master/patches/CVE-2022-21658/0001-Fix-CVE-2022-21658-for-Windows.patch The complexity of that Windows patch, oof.
Rescoping this issue to POSIX.
Windows is tracked in #110.
(Misread, please ignore.)
It would necessary to have
fdopendir
implemented upstream inunix
though. Can you file a feature request on that?
Yep, that is no longer a blocker.
removeDirectoryRecursive and similar are supposed to avoid following symlinks. However, they all appear to have a TOCTOU flaw that exposes them to a race condition that could lead to deleting symlinked directory trees.
Ie, in a call
removePathRecursive "foo"
, iffoo
is a directory at the stat call, and then gets replaced with a symlink before removeContentsRecursive callslistDirectory
, then it will get a list of the files in the symlinked directory, and proceed to delete them.This could be a security problem if deleting a directory that is writable by another user, or perhaps by a containerized, untrusted process that has been given write access to the directory. Security aside, this kind of race can lead to unexpected data loss, though the probability of the right sequence of events is of course lower.
I checked how rm -r (from coreutils) handles this, and it uses open with
O_NOFOLLOW
when opening directories, and so avoids listing the contents of directory symlinks.I think that a similar fix could work here, if
listDirectory
usedO_NOFOLLOW
it seems it would nicely solve the problem. (It would also speed it up by avoiding needing to stat every directory it traverses.)openDirStream does not currently have a way to pass that flag, so it would need to be patched first.