Unlimit UNIX remove_dir_all() implementation (take 2)

[hkratz]

The current recursive implementation runs out of file descriptors traversing deep directory hierarchies. This implementation fixes this. Details:

• An on-heap stack of PathComponents containing name, dev, inode of ancestors from the deletion root to the currently deleted directory is used instead of recursing to avoid stack overflows.
• Up to 15 open ancestor file descriptors and associated readdirs (LazyReadDir) are cached in a VecDeque.
• If a file descriptor is shifted out of the cache, on the way back up it is reopened with openat(dirfd, "..", O_NOFOLLOW) and the associated (dev, inode) pair is compared to the expected (dev, inode) pair. The grandparent directory is opened and compared as well to prevent node reuse attacks. For a scenario see Unlimit UNIX remove_dir_all() implementation #93160 (comment).

The implementations is similar to the Gnulib (not glibc!) fts implementation in FTS_CWDFD mode except for the additional grandparent check going up. Gnulib fts is used in coreutils, e.g. for rm. BSD rm use chdir-based fts (also going up by opening ..), which is not suitable for multi-threading.

This is a much improved version of #93160 which has extensive discussion. It did not make sense to continue in the old PR since the code has been heavily restructured and moved into a dir_fd submodule as per @the8472's suggestion.

Supersedes #93473 and #93160.

cc @the8472 @cuviper

[rust-highfive]

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

[bors]

☔ The latest upstream changes (presumably #96253) made this pull request unmergeable. Please resolve the merge conflicts.

[hkratz]

@rustbot label +t-libs -t-compiler

[rustbot]

Error: Label t-libs can only be set by Rust team members

Please let @rust-lang/release know if you're having trouble with this bot.

[the8472]

Since the parent of the root is already opened as root_parent_component can we use that fd instead of AT_FDCWD?

[the8472]

This gets opened with O_RDONLY but unlinkat only needs write access to the directory so we should use some other permission flags here, e.g. O_PATH on linux. That's relevant in weird cases where one has -wx on the ancestor. Or even -w- if relative paths are involved and the ancestor sits above the current working directory. The UI test could test these edge-cases too.

Additionally we may want to allow failure here and just stat('..') instead to get the dev and ino of the parent without having to open it at all because there would be cases where the recursive version could remove an entire tree and only fail on the final unlink when the parent isn't accessible.

Another option is to keep the root fd open instead of its parent, that'll prevent its inode number from being recycled and we can dup the fd in that case instead of checking the grandparent.

[the8472]

I think the comment could be clearer that this is a security measure, otherwise it seems bizarre that something like this could happen.

[rbtcollins]

Its not clear to me how ascending to the grandparent is needed. Or perhaps its just a wording confusion problem.

#93160 (comment) describes causing a privileged process to delete the contents of a sticky directory by moving.

1. foo/bar/baz is being deleted
2. Alice moves bar aside
3. Alice deletes foo
4. Alice waits for the inode foo had to be recycled into a desirable context, for instance as a new sticky dir /somewhere/newtmp
5. Alice moves bar into that dir
6. the priviled process moves up from bar to /somewhere/newtmp

At this point the parent of /somewhere/newtmp needs to be verified as being legitimate, so its (dev,inode) are cross-checked; the grandparent is not relevant.

But as I say perhaps I'm just misunderstanding :/. It seems to me that a rule of 'only operate (readdir/unlink_at its children) on a directory that was either a) opened going down or b) was reopened going up and still has its own original parent (inode,dev) seems quite robust: the greatest attack possible is to operate on newly created directories that have the same inode as they had before and are in the same containing directory-by-inode that they had before. That property holds because downward traversal to discover a directory inode must occur before inode reuse can be attempted.

On the other hand, if reuse attacks are something worth guarding against, I don't think that that property is sufficient: the privileged attacker can be fooled into deleting content that was not in the original file tree, and if that acts as a gadget into some other attack, we now have a chained attack possible.

[hkratz]

At this point the parent of /somewhere/newtmp needs to be verified as being legitimate, so its (dev,inode) are cross-checked; the grandparent is not relevant.

That is what the code does. The comment is semantically located just before going up, current_readdir = parent_readdir happens further down. So at the point of the comment all children of current_dir have been processed but it (still) points to /somewhere/newtmp/bar, parent_dir points to /somewhere/newtmp and grandparent_readdir will point to /somewhere.

There is no "ascending to the grandparent". The invariant is that the current directory and its parent directory have the correct (dev, ino) and are kept open, while the children of the current directory are processed.

You are right that this does not guard against all reuse attacks but it does make reuse attacks much harder. GNU rm does not protect against any of those attacks IIRC.

[bors]

☔ The latest upstream changes (presumably #95897) made this pull request unmergeable. Please resolve the merge conflicts.

[JohnCSimon]

still waiting on review

[bors]

☔ The latest upstream changes (presumably #93668) made this pull request unmergeable. Please resolve the merge conflicts.

[pitaj]

Ping from triage. @hkratz please resolve the merge conflicts and @rustbot review when ready

@rustbot author

[Dylan-DPC]

Closing this pr as inactive. Feel free to reöpen or create a new pr if you or anyone plans on working on this in the future