removeDirectoryRecursive symlink TOCTOU

[joeyh]

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", if foo is a directory at the stat call, and then gets replaced with a symlink before removeContentsRecursive calls listDirectory, 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 used O_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.

[Rufflewind]

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?

[Rufflewind]

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.

[joeyh]

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.

[Rufflewind]

Rescoping this issue to POSIX.

Windows is tracked in #110.

(Misread, please ignore.)

[hasufell]

It would necessary to have fdopendir implemented upstream in unix though. Can you file a feature request on that?

https://hackage.haskell.org/package/unix-2.8.5.1/docs/System-Posix-Directory-Fd.html#v:unsafeOpenDirStreamFd

[Rufflewind]

https://hackage.haskell.org/package/unix-2.8.5.1/docs/System-Posix-Directory-Fd.html#v:unsafeOpenDirStreamFd

Yep, that is no longer a blocker.