fix(install): follow symlink components in destination path with -D#11505
Open
abendrothj wants to merge 1 commit intouutils:mainfrom
Open
fix(install): follow symlink components in destination path with -D#11505abendrothj wants to merge 1 commit intouutils:mainfrom
abendrothj wants to merge 1 commit intouutils:mainfrom
Conversation
9c7f91a to
4fc3ac8
Compare
|
GNU testsuite comparison: |
Merging this PR will not alter performance
Comparing Footnotes
|
ebc0ba6 to
cd070c4
Compare
|
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).
cd070c4 to
ed62194
Compare
|
GNU testsuite comparison: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.