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. |
uutils install 0.8.0 as used in Ubuntu 26.04 has a bug in the install applet, breaking a number of packages: uutils/coreutils#11505 https://bugs.launchpad.net/ubuntu/+source/rust-coreutils/+bug/2151454 The fix has been merged upstream but not yet released or packaged in Ubuntu, so detect and reject the buggy version and explain how to change to the coreutils version. Once fixed the version output will hopefully change. For simplicity, only check for the exact 0.8.0 version string. Hopefully when it is fixed in Ubuntu, they also update the version string. Note that earlier versions of uutils have the same issue of course, but those versions were never the default "install" on Ubuntu (or anywhere else). Signed-off-by: Peter Korsgaard <peter@korsgaard.com> Signed-off-by: Arnout Vandecappelle <arnout@rnout.be>
uutils install 0.8.0 as used in Ubuntu 26.04 has a bug in the install applet, breaking a number of packages: uutils/coreutils#11505 https://bugs.launchpad.net/ubuntu/+source/rust-coreutils/+bug/2151454 The fix has been merged upstream but not yet released or packaged in Ubuntu, so detect and reject the buggy version and explain how to change to the coreutils version. Once fixed the version output will hopefully change. For simplicity, only check for the exact 0.8.0 version string. Hopefully when it is fixed in Ubuntu, they also update the version string. Note that earlier versions of uutils have the same issue of course, but those versions were never the default "install" on Ubuntu (or anywhere else). Signed-off-by: Peter Korsgaard <peter@korsgaard.com> Signed-off-by: Arnout Vandecappelle <arnout@rnout.be> (cherry picked from commit 64cb69f) Signed-off-by: Thomas Perale <thomas.perale@mind.be>
uutils install 0.8.0 as used in Ubuntu 26.04 has a bug in the install applet, breaking a number of packages: uutils/coreutils#11505 https://bugs.launchpad.net/ubuntu/+source/rust-coreutils/+bug/2151454 The fix has been merged upstream but not yet released or packaged in Ubuntu, so detect and reject the buggy version and explain how to change to the coreutils version. Once fixed the version output will hopefully change. For simplicity, only check for the exact 0.8.0 version string. Hopefully when it is fixed in Ubuntu, they also update the version string. Note that earlier versions of uutils have the same issue of course, but those versions were never the default "install" on Ubuntu (or anywhere else). Signed-off-by: Peter Korsgaard <peter@korsgaard.com> Signed-off-by: Arnout Vandecappelle <arnout@rnout.be> (cherry picked from commit 64cb69f) Signed-off-by: Thomas Perale <thomas.perale@mind.be>
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.