fix(install): follow symlink components in destination path with -D#11505
Conversation
9c7f91a to
4fc3ac8
Compare
|
GNU testsuite comparison: |
Merging this PR will not alter performance
Comparing Footnotes
|
ebc0ba6 to
cd070c4
Compare
|
GNU testsuite comparison: |
cd070c4 to
ed62194
Compare
|
GNU testsuite comparison: |
|
The test fails in the mkfifo test, anyway hope this PR can be merged, and the related issue not left to the next released uutils. |
agreed. seems like flakiness, since mkfifo is unrelated. |
|
GNU testsuite comparison: |
install -D was replacing pre-existing symlinks in the destination path with real directories instead of following them, breaking any workflow where part of the install prefix is a symlink (BOSH, Homebrew, Nix, stow, `make install` with a symlinked prefix).
93383c7 to
dbe8864
Compare
| /// Find the deepest existing real directory ancestor for a path. | ||
| /// | ||
| /// Returns the existing ancestor path and a list of components that need to be created. | ||
| /// Uses `symlink_metadata` to detect symlinks - symlinks are NOT followed and are |
There was a problem hiding this comment.
This comment is wrong after the change. Should be changed to something like:
/// Uses `metadata` (follows symlinks) so that symlinks to directories are treated
/// as existing ancestors rather than components to create.
| /// Open or create a subdirectory using fd-based operations only. | ||
| /// | ||
| /// This is a helper function for `create_dir_all_safe` that handles a single | ||
| /// path component. If a symlink exists where a directory should be, it is |
There was a problem hiding this comment.
this comment is wrong after the change and needs to be updated.
| /// This function prevents TOCTOU race conditions by: | ||
| /// 1. Finding the deepest existing ancestor directory (path-based, but safe since it exists) | ||
| /// 2. Opening that ancestor with a file descriptor | ||
| /// 3. Creating all new directories using fd-based operations (mkdirat, openat with O_NOFOLLOW) |
There was a problem hiding this comment.
after the change: existing symlinks are followed (with O_DIRECTORY as a safety net), while only newly created path components are protected with O_NOFOLLOW.
| fn test_install_d_symlink_race_condition() { | ||
| // Test for symlink race condition fix (issue #10013) | ||
| // Verifies that pre-existing symlinks in path are handled safely | ||
| // Test that pre-existing symlinks in the path are followed (GNU coreutils behavior). |
There was a problem hiding this comment.
the test name doesn't match what is being tested. it's not related to race-conditions any more
|
Thanks. I've merged the branch and will add the cosmetic changes in a follow-up PR. This should be merged ASAP since it is a critical fix. |
Summary
Fixes #11469
install -Dwas replacing pre-existing symlinks in the destination path with real directories instead of following them. This broke any workflow where part of the install prefix is a symlink; including BOSH deployments, Homebrew, Nix, stow, and anymake installtargeting a symlinked prefix.Reproduction (from the issue):
Root cause
PR #10140 introduced
create_dir_all_safe()insafe_traversal.rsto prevent TOCTOU symlink race conditions. The fix was correct in intent but too aggressive:open_or_create_subdir()unconditionally unlinked and recreated any symlink it encountered, including pre-existing legitimate ones.Changes
src/uucore/src/lib/features/safe_traversal.rsopen_or_create_subdir: whenstat_atreturnsS_IFLNK, callopen_subdir(Follow)instead ofunlink_at + mkdir_at. TheO_DIRECTORYflag already inopen_subdirmeans dangling or non-directory symlinks still return an error cleanly.find_existing_ancestor: switch fromfs::symlink_metadatatofs::metadataso that a symlink-to-directory is recognised as an existing ancestor rather than a component to recreate (this was already the stated intent in the function's doc comment).src/uu/install/src/install.rsdir_existscheck and theDirFd::opencall to also follow symlinks, consistent with the above.tests/by-util/test_install.rstest_install_d_follows_symlink_prefixas a direct regression test for the issue's reproduction case.TOCTOU / security note
The true TOCTOU race (a symlink injected during the operation into a not-yet-existing path component) is still blocked:
mkdiratfails withEEXISTif an attacker creates a symlink betweenstat_atreturningENOENTand ourmkdir_at. Newly-created directories are still opened withO_NOFOLLOW.What changes is that pre-existing symlinks are now followed — which is exactly what GNU coreutils 8.32 does. The previous behavior was stricter than GNU in this regard.