Skip to content

Commit 21fda8c

Browse files
authored
fix(install): follow symlink components in destination path with -D
## Summary Fixes #11469 `install -D` was 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 any `make install` targeting a symlinked prefix. **Reproduction (from the issue):** ```sh mkdir -p /tmp/target ln -s /tmp/target /tmp/link echo hello > /tmp/file.txt install -D -m 644 /tmp/file.txt /tmp/link/subdir/file.txt # GNU coreutils 8.32: /tmp/link stays a symlink, file lands in /tmp/target/subdir/file.txt # uutils 0.7.0: /tmp/link is replaced with a real directory — wrong ``` ## Root cause PR #10140 introduced `create_dir_all_safe()` in `safe_traversal.rs` to 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.rs`** - `open_or_create_subdir`: when `stat_at` returns `S_IFLNK`, call `open_subdir(Follow)` instead of `unlink_at + mkdir_at`. The `O_DIRECTORY` flag already in `open_subdir` means dangling or non-directory symlinks still return an error cleanly. - `find_existing_ancestor`: switch from `fs::symlink_metadata` to `fs::metadata` so 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.rs`** - Align the `dir_exists` check and the `DirFd::open` call to also follow symlinks, consistent with the above. **`tests/by-util/test_install.rs`** - Update the two tests added by #10140 — they were asserting the buggy behavior (symlink replaced). Flip the assertions to document the correct GNU behavior. - Add `test_install_d_follows_symlink_prefix` as 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: `mkdirat` fails with `EEXIST` if an attacker creates a symlink between `stat_at` returning `ENOENT` and our `mkdir_at`. Newly-created directories are still opened with `O_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.
1 parent 29fdaa6 commit 21fda8c

3 files changed

Lines changed: 130 additions & 67 deletions

File tree

src/uu/install/src/install.rs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -631,8 +631,7 @@ fn standard(mut paths: Vec<OsString>, b: &Behavior) -> UResult<()> {
631631
};
632632

633633
let dir_exists = if to_create.exists() {
634-
fs::symlink_metadata(to_create)
635-
.is_ok_and(|m| m.is_dir() && !m.file_type().is_symlink())
634+
metadata(to_create).is_ok_and(|m| m.is_dir())
636635
} else {
637636
false
638637
};
@@ -643,7 +642,7 @@ fn standard(mut paths: Vec<OsString>, b: &Behavior) -> UResult<()> {
643642
if b.target_dir.is_none()
644643
&& sources.len() == 1
645644
&& !is_potential_directory_path(&target)
646-
&& let Ok(dir_fd) = DirFd::open(to_create, SymlinkBehavior::NoFollow)
645+
&& let Ok(dir_fd) = DirFd::open(to_create, SymlinkBehavior::Follow)
647646
&& let Some(filename) = target.file_name()
648647
{
649648
target_parent_fd = Some(dir_fd);

src/uucore/src/lib/features/safe_traversal.rs

Lines changed: 25 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -475,16 +475,16 @@ fn find_existing_ancestor(path: &Path) -> io::Result<(PathBuf, Vec<OsString>)> {
475475
let mut components: Vec<OsString> = Vec::new();
476476

477477
loop {
478-
// Use symlink_metadata to NOT follow symlinks
479-
match fs::symlink_metadata(&current) {
478+
// Use metadata (follow symlinks) so that symlinks to directories are
479+
// treated as existing ancestors rather than components to create.
480+
match fs::metadata(&current) {
480481
Ok(meta) => {
481-
if meta.is_dir() && !meta.file_type().is_symlink() {
482-
// Found a real directory (not a symlink to a directory)
482+
if meta.is_dir() {
483+
// Found a directory (real or via symlink)
483484
components.reverse();
484485
return Ok((current, components));
485486
}
486-
// It's a symlink, file, or other non-directory - treat as needing creation
487-
// This ensures symlinks get replaced by open_or_create_subdir
487+
// It's a file or other non-directory - treat as needing creation
488488
if let Some(file_name) = current.file_name() {
489489
components.push(file_name.to_os_string());
490490
}
@@ -554,9 +554,10 @@ fn open_or_create_subdir(parent_fd: &DirFd, name: &OsStr, mode: u32) -> io::Resu
554554
match file_type {
555555
libc::S_IFDIR => parent_fd.open_subdir(name, SymlinkBehavior::NoFollow),
556556
libc::S_IFLNK => {
557-
parent_fd.unlink_at(name, false)?;
558-
parent_fd.mkdir_at(name, mode)?;
559-
parent_fd.open_subdir(name, SymlinkBehavior::NoFollow)
557+
// Follow symlinks to directories (GNU coreutils behavior).
558+
// O_DIRECTORY in open_subdir ensures we only succeed if the
559+
// symlink resolves to a directory; dangling or non-dir symlinks error out.
560+
parent_fd.open_subdir(name, SymlinkBehavior::Follow)
560561
}
561562
_ => Err(io::Error::new(
562563
io::ErrorKind::AlreadyExists,
@@ -1251,23 +1252,23 @@ mod tests {
12511252
}
12521253

12531254
#[test]
1254-
fn test_create_dir_all_safe_replaces_symlink() {
1255+
fn test_create_dir_all_safe_follows_symlink() {
12551256
let temp_dir = TempDir::new().unwrap();
12561257
let target_dir = temp_dir.path().join("target");
12571258
fs::create_dir(&target_dir).unwrap();
12581259

1259-
// Create a symlink where we want to create a directory
1260-
let symlink_path = temp_dir.path().join("link_to_replace");
1260+
// Create a symlink pointing to an existing directory
1261+
let symlink_path = temp_dir.path().join("link");
12611262
symlink(&target_dir, &symlink_path).unwrap();
12621263
assert!(symlink_path.is_symlink());
12631264

1264-
// create_dir_all_safe should replace the symlink with a real directory
1265+
// create_dir_all_safe should follow the symlink (GNU coreutils behavior)
12651266
let dir_fd = create_dir_all_safe(&symlink_path, 0o755).unwrap();
12661267
assert!(dir_fd.as_raw_fd() >= 0);
12671268

1268-
// Verify the symlink was replaced with a real directory
1269-
assert!(symlink_path.is_dir());
1270-
assert!(!symlink_path.is_symlink());
1269+
// Verify the symlink is preserved (not replaced with a real directory)
1270+
assert!(symlink_path.is_symlink());
1271+
assert!(symlink_path.is_dir()); // still resolves to a directory via the symlink
12711272
}
12721273

12731274
#[test]
@@ -1284,8 +1285,8 @@ mod tests {
12841285
fn test_create_dir_all_safe_nested_symlink_in_path() {
12851286
let temp_dir = TempDir::new().unwrap();
12861287

1287-
// Create: parent/symlink -> target
1288-
// Then try to create: parent/symlink/subdir
1288+
// Create: parent/link -> target
1289+
// Then create: parent/link/subdir
12891290
let parent = temp_dir.path().join("parent");
12901291
let target = temp_dir.path().join("target");
12911292
fs::create_dir(&parent).unwrap();
@@ -1294,18 +1295,17 @@ mod tests {
12941295
let symlink_in_path = parent.join("link");
12951296
symlink(&target, &symlink_in_path).unwrap();
12961297

1297-
// Try to create parent/link/subdir - the symlink should be replaced
1298+
// Try to create parent/link/subdir - the symlink should be followed (GNU behavior)
12981299
let nested_path = symlink_in_path.join("subdir");
12991300
let dir_fd = create_dir_all_safe(&nested_path, 0o755).unwrap();
13001301
assert!(dir_fd.as_raw_fd() >= 0);
13011302

1302-
// The symlink should have been replaced with a real directory
1303-
assert!(!symlink_in_path.is_symlink());
1304-
assert!(symlink_in_path.is_dir());
1305-
assert!(nested_path.is_dir());
1303+
// The symlink should be preserved, not replaced
1304+
assert!(symlink_in_path.is_symlink());
1305+
assert!(symlink_in_path.is_dir()); // resolves via symlink
13061306

1307-
// Target directory should not contain subdir (race attack prevented)
1308-
assert!(!target.join("subdir").exists());
1307+
// subdir should have been created inside the real target directory
1308+
assert!(target.join("subdir").exists());
13091309
}
13101310

13111311
#[test]

tests/by-util/test_install.rs

Lines changed: 103 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -2583,90 +2583,154 @@ fn test_install_normal_file_replaces_symlink() {
25832583
#[test]
25842584
#[cfg(unix)]
25852585
fn test_install_d_symlink_race_condition() {
2586-
// Test for symlink race condition fix (issue #10013)
2587-
// Verifies that pre-existing symlinks in path are handled safely
2586+
// Test that pre-existing symlinks in the path are followed (GNU coreutils behavior).
2587+
// install -D should traverse symlink components rather than replacing them.
25882588
use std::os::unix::fs::symlink;
25892589

25902590
let scene = TestScenario::new(util_name!());
25912591
let at = &scene.fixtures;
25922592

2593-
// Create test directories
25942593
at.mkdir("target");
2595-
2596-
// Create source file
25972594
at.write("source_file", "test content");
25982595

2599-
// Set up a pre-existing symlink attack scenario
26002596
at.mkdir_all("testdir/a");
2601-
let intermediate_dir = at.plus("testdir/a/b");
2602-
symlink(at.plus("target"), &intermediate_dir).unwrap();
2597+
symlink(at.plus("target"), at.plus("testdir/a/b")).unwrap();
26032598

2604-
// Run install -D which should detect and handle the symlink
2605-
let result = scene
2599+
// install -D should follow the symlink and write into the symlink target
2600+
scene
26062601
.ucmd()
26072602
.arg("-D")
26082603
.arg(at.plus("source_file"))
26092604
.arg(at.plus("testdir/a/b/c/file"))
2610-
.run();
2611-
2612-
let wrong_location = at.plus("target/c/file");
2605+
.succeeds();
26132606

2614-
// The critical assertion: file must NOT be in symlink target (race prevented)
2607+
// File must be written through the symlink, i.e. inside the real target dir
26152608
assert!(
2616-
!wrong_location.exists(),
2617-
"RACE CONDITION NOT PREVENTED: File was created in symlink target"
2609+
at.plus("target/c/file").exists(),
2610+
"File should be written through the symlink into the real target directory"
2611+
);
2612+
assert_eq!(
2613+
fs::read_to_string(at.plus("target/c/file")).unwrap(),
2614+
"test content"
26182615
);
26192616

2620-
// If the command succeeded, verify the file is in the correct location
2621-
if result.succeeded() {
2622-
assert!(at.file_exists("testdir/a/b/c/file"));
2623-
assert_eq!(at.read("testdir/a/b/c/file"), "test content");
2624-
// The symlink should have been replaced with a real directory
2625-
assert!(
2626-
at.plus("testdir/a/b").is_dir() && !at.plus("testdir/a/b").is_symlink(),
2627-
"Intermediate path should be a real directory, not a symlink"
2628-
);
2629-
}
2617+
// The symlink must not have been replaced with a real directory
2618+
assert!(
2619+
at.plus("testdir/a/b").is_symlink(),
2620+
"Intermediate symlink should be preserved, not replaced with a real directory"
2621+
);
26302622
}
26312623

26322624
#[test]
26332625
#[cfg(unix)]
26342626
fn test_install_d_symlink_race_condition_concurrent() {
2635-
// Test pre-existing symlinks in intermediate paths are handled correctly
2627+
// Verify symlink-following behavior is consistent (companion to the test above).
26362628
use std::os::unix::fs::symlink;
26372629

26382630
let scene = TestScenario::new(util_name!());
26392631
let at = &scene.fixtures;
26402632

2641-
// Create test directories and source file using testing framework
26422633
at.mkdir("target2");
26432634
at.write("source_file2", "test content 2");
26442635

2645-
// Set up intermediate directory with symlink
26462636
at.mkdir_all("testdir2/a");
26472637
symlink(at.plus("target2"), at.plus("testdir2/a/b")).unwrap();
26482638

2649-
// Run install -D
26502639
scene
26512640
.ucmd()
26522641
.arg("-D")
26532642
.arg(at.plus("source_file2"))
26542643
.arg(at.plus("testdir2/a/b/c/file"))
26552644
.succeeds();
26562645

2657-
// Verify file was created at the intended destination
2658-
assert!(at.file_exists("testdir2/a/b/c/file"));
2659-
assert_eq!(at.read("testdir2/a/b/c/file"), "test content 2");
2646+
// File should be in the real target directory (symlink was followed)
2647+
assert!(
2648+
at.plus("target2/c/file").exists(),
2649+
"File should be written through the symlink into the real target directory"
2650+
);
2651+
assert_eq!(
2652+
fs::read_to_string(at.plus("target2/c/file")).unwrap(),
2653+
"test content 2"
2654+
);
26602655

2661-
// Verify file was NOT created in symlink target
2656+
// Symlink should be preserved
26622657
assert!(
2663-
!at.plus("target2/c/file").exists(),
2664-
"File should NOT be in symlink target location"
2658+
at.plus("testdir2/a/b").is_symlink(),
2659+
"Intermediate symlink should be preserved, not replaced with a real directory"
26652660
);
2661+
}
2662+
2663+
#[test]
2664+
#[cfg(unix)]
2665+
fn test_install_d_follows_symlink_prefix() {
2666+
// Regression test for: install -D replaces symlink components instead of following them.
2667+
// Reproduces the exact scenario from the bug report: a symlinked install prefix
2668+
// (common in BOSH, Homebrew, Nix, stow) must be followed, not destroyed.
2669+
use std::os::unix::fs::symlink;
2670+
2671+
let scene = TestScenario::new(util_name!());
2672+
let at = &scene.fixtures;
2673+
2674+
// Simulate: ln -s /tmp/target /tmp/link
2675+
at.mkdir("target");
2676+
symlink(at.plus("target"), at.plus("link")).unwrap();
2677+
2678+
at.write("file.txt", "hello");
26662679

2667-
// Verify intermediate path is now a real directory
2680+
// install -D -m 644 file.txt link/subdir/file.txt
2681+
scene
2682+
.ucmd()
2683+
.args(&["-D", "-m", "644"])
2684+
.arg(at.plus("file.txt"))
2685+
.arg(at.plus("link/subdir/file.txt"))
2686+
.succeeds();
2687+
2688+
// GNU expected: /tmp/link remains a symlink, file written to /tmp/target/subdir/file.txt
2689+
assert!(
2690+
at.plus("link").is_symlink(),
2691+
"The symlinked prefix must remain a symlink"
2692+
);
2693+
assert!(
2694+
at.plus("target/subdir/file.txt").exists(),
2695+
"File must be written into the real target directory via the symlink"
2696+
);
2697+
assert_eq!(
2698+
fs::read_to_string(at.plus("target/subdir/file.txt")).unwrap(),
2699+
"hello"
2700+
);
2701+
}
2702+
2703+
#[test]
2704+
#[cfg(unix)]
2705+
fn test_install_d_dangling_symlink_in_path_errors() {
2706+
// A dangling symlink as a path component must not be silently replaced with a
2707+
// real directory. GNU coreutils errors out in this case; we should too.
2708+
use std::os::unix::fs::symlink;
2709+
2710+
let scene = TestScenario::new(util_name!());
2711+
let at = &scene.fixtures;
2712+
2713+
// Create a symlink pointing to a nonexistent target (dangling)
2714+
symlink(at.plus("nonexistent"), at.plus("dangling")).unwrap();
2715+
assert!(at.plus("dangling").is_symlink());
2716+
2717+
at.write("file.txt", "hello");
2718+
2719+
// install -D file.txt dangling/subdir/file.txt should fail
2720+
scene
2721+
.ucmd()
2722+
.args(&["-D", "-m", "644"])
2723+
.arg(at.plus("file.txt"))
2724+
.arg(at.plus("dangling/subdir/file.txt"))
2725+
.fails();
2726+
2727+
// The dangling symlink must not have been replaced with a real directory
2728+
assert!(
2729+
at.plus("dangling").is_symlink(),
2730+
"Dangling symlink must not be replaced with a real directory"
2731+
);
26682732
assert!(
2669-
at.plus("testdir2/a/b").is_dir() && !at.plus("testdir2/a/b").is_symlink(),
2670-
"Intermediate directory should be a real directory, not a symlink"
2733+
!at.plus("nonexistent").exists(),
2734+
"The symlink target must not have been created"
26712735
);
26722736
}

0 commit comments

Comments
 (0)