Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/uu/install/src/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -631,8 +631,7 @@ fn standard(mut paths: Vec<OsString>, b: &Behavior) -> UResult<()> {
};

let dir_exists = if to_create.exists() {
fs::symlink_metadata(to_create)
.is_ok_and(|m| m.is_dir() && !m.file_type().is_symlink())
metadata(to_create).is_ok_and(|m| m.is_dir())
} else {
false
};
Expand All @@ -643,7 +642,7 @@ fn standard(mut paths: Vec<OsString>, b: &Behavior) -> UResult<()> {
if b.target_dir.is_none()
&& sources.len() == 1
&& !is_potential_directory_path(&target)
&& let Ok(dir_fd) = DirFd::open(to_create, SymlinkBehavior::NoFollow)
&& let Ok(dir_fd) = DirFd::open(to_create, SymlinkBehavior::Follow)
&& let Some(filename) = target.file_name()
{
target_parent_fd = Some(dir_fd);
Expand Down
50 changes: 25 additions & 25 deletions src/uucore/src/lib/features/safe_traversal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -377,16 +377,16 @@ fn find_existing_ancestor(path: &Path) -> io::Result<(PathBuf, Vec<OsString>)> {
let mut components: Vec<OsString> = Vec::new();

loop {
// Use symlink_metadata to NOT follow symlinks
match fs::symlink_metadata(&current) {
// Use metadata (follow symlinks) so that symlinks to directories are
// treated as existing ancestors rather than components to create.
match fs::metadata(&current) {
Ok(meta) => {
if meta.is_dir() && !meta.file_type().is_symlink() {
// Found a real directory (not a symlink to a directory)
if meta.is_dir() {
// Found a directory (real or via symlink)
components.reverse();
return Ok((current, components));
}
// It's a symlink, file, or other non-directory - treat as needing creation
// This ensures symlinks get replaced by open_or_create_subdir
// It's a file or other non-directory - treat as needing creation
if let Some(file_name) = current.file_name() {
components.push(file_name.to_os_string());
}
Expand Down Expand Up @@ -456,9 +456,10 @@ fn open_or_create_subdir(parent_fd: &DirFd, name: &OsStr, mode: u32) -> io::Resu
match file_type {
libc::S_IFDIR => parent_fd.open_subdir(name, SymlinkBehavior::NoFollow),
libc::S_IFLNK => {
parent_fd.unlink_at(name, false)?;
parent_fd.mkdir_at(name, mode)?;
parent_fd.open_subdir(name, SymlinkBehavior::NoFollow)
// Follow symlinks to directories (GNU coreutils behavior).
// O_DIRECTORY in open_subdir ensures we only succeed if the
// symlink resolves to a directory; dangling or non-dir symlinks error out.
parent_fd.open_subdir(name, SymlinkBehavior::Follow)
}
_ => Err(io::Error::new(
io::ErrorKind::AlreadyExists,
Expand Down Expand Up @@ -1150,23 +1151,23 @@ mod tests {
}

#[test]
fn test_create_dir_all_safe_replaces_symlink() {
fn test_create_dir_all_safe_follows_symlink() {
let temp_dir = TempDir::new().unwrap();
let target_dir = temp_dir.path().join("target");
fs::create_dir(&target_dir).unwrap();

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

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

// Verify the symlink was replaced with a real directory
assert!(symlink_path.is_dir());
assert!(!symlink_path.is_symlink());
// Verify the symlink is preserved (not replaced with a real directory)
assert!(symlink_path.is_symlink());
assert!(symlink_path.is_dir()); // still resolves to a directory via the symlink
}

#[test]
Expand All @@ -1183,8 +1184,8 @@ mod tests {
fn test_create_dir_all_safe_nested_symlink_in_path() {
let temp_dir = TempDir::new().unwrap();

// Create: parent/symlink -> target
// Then try to create: parent/symlink/subdir
// Create: parent/link -> target
// Then create: parent/link/subdir
let parent = temp_dir.path().join("parent");
let target = temp_dir.path().join("target");
fs::create_dir(&parent).unwrap();
Expand All @@ -1193,18 +1194,17 @@ mod tests {
let symlink_in_path = parent.join("link");
symlink(&target, &symlink_in_path).unwrap();

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

// The symlink should have been replaced with a real directory
assert!(!symlink_in_path.is_symlink());
assert!(symlink_in_path.is_dir());
assert!(nested_path.is_dir());
// The symlink should be preserved, not replaced
assert!(symlink_in_path.is_symlink());
assert!(symlink_in_path.is_dir()); // resolves via symlink

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

#[test]
Expand Down
142 changes: 103 additions & 39 deletions tests/by-util/test_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2573,90 +2573,154 @@ fn test_install_normal_file_replaces_symlink() {
#[test]
#[cfg(unix)]
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).
// install -D should traverse symlink components rather than replacing them.
use std::os::unix::fs::symlink;

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

// Create test directories
at.mkdir("target");

// Create source file
at.write("source_file", "test content");

// Set up a pre-existing symlink attack scenario
at.mkdir_all("testdir/a");
let intermediate_dir = at.plus("testdir/a/b");
symlink(at.plus("target"), &intermediate_dir).unwrap();
symlink(at.plus("target"), at.plus("testdir/a/b")).unwrap();

// Run install -D which should detect and handle the symlink
let result = scene
// install -D should follow the symlink and write into the symlink target
scene
.ucmd()
.arg("-D")
.arg(at.plus("source_file"))
.arg(at.plus("testdir/a/b/c/file"))
.run();

let wrong_location = at.plus("target/c/file");
.succeeds();

// The critical assertion: file must NOT be in symlink target (race prevented)
// File must be written through the symlink, i.e. inside the real target dir
assert!(
!wrong_location.exists(),
"RACE CONDITION NOT PREVENTED: File was created in symlink target"
at.plus("target/c/file").exists(),
"File should be written through the symlink into the real target directory"
);
assert_eq!(
fs::read_to_string(at.plus("target/c/file")).unwrap(),
"test content"
);

// If the command succeeded, verify the file is in the correct location
if result.succeeded() {
assert!(at.file_exists("testdir/a/b/c/file"));
assert_eq!(at.read("testdir/a/b/c/file"), "test content");
// The symlink should have been replaced with a real directory
assert!(
at.plus("testdir/a/b").is_dir() && !at.plus("testdir/a/b").is_symlink(),
"Intermediate path should be a real directory, not a symlink"
);
}
// The symlink must not have been replaced with a real directory
assert!(
at.plus("testdir/a/b").is_symlink(),
"Intermediate symlink should be preserved, not replaced with a real directory"
);
}

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

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

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

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

// Run install -D
scene
.ucmd()
.arg("-D")
.arg(at.plus("source_file2"))
.arg(at.plus("testdir2/a/b/c/file"))
.succeeds();

// Verify file was created at the intended destination
assert!(at.file_exists("testdir2/a/b/c/file"));
assert_eq!(at.read("testdir2/a/b/c/file"), "test content 2");
// File should be in the real target directory (symlink was followed)
assert!(
at.plus("target2/c/file").exists(),
"File should be written through the symlink into the real target directory"
);
assert_eq!(
fs::read_to_string(at.plus("target2/c/file")).unwrap(),
"test content 2"
);

// Verify file was NOT created in symlink target
// Symlink should be preserved
assert!(
!at.plus("target2/c/file").exists(),
"File should NOT be in symlink target location"
at.plus("testdir2/a/b").is_symlink(),
"Intermediate symlink should be preserved, not replaced with a real directory"
);
}

#[test]
#[cfg(unix)]
fn test_install_d_follows_symlink_prefix() {
// Regression test for: install -D replaces symlink components instead of following them.
// Reproduces the exact scenario from the bug report: a symlinked install prefix
// (common in BOSH, Homebrew, Nix, stow) must be followed, not destroyed.
use std::os::unix::fs::symlink;

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

// Simulate: ln -s /tmp/target /tmp/link
at.mkdir("target");
symlink(at.plus("target"), at.plus("link")).unwrap();

at.write("file.txt", "hello");

// Verify intermediate path is now a real directory
// install -D -m 644 file.txt link/subdir/file.txt
scene
.ucmd()
.args(&["-D", "-m", "644"])
.arg(at.plus("file.txt"))
.arg(at.plus("link/subdir/file.txt"))
.succeeds();

// GNU expected: /tmp/link remains a symlink, file written to /tmp/target/subdir/file.txt
assert!(
at.plus("link").is_symlink(),
"The symlinked prefix must remain a symlink"
);
assert!(
at.plus("target/subdir/file.txt").exists(),
"File must be written into the real target directory via the symlink"
);
assert_eq!(
fs::read_to_string(at.plus("target/subdir/file.txt")).unwrap(),
"hello"
);
}

#[test]
#[cfg(unix)]
fn test_install_d_dangling_symlink_in_path_errors() {
// A dangling symlink as a path component must not be silently replaced with a
// real directory. GNU coreutils errors out in this case; we should too.
use std::os::unix::fs::symlink;

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

// Create a symlink pointing to a nonexistent target (dangling)
symlink(at.plus("nonexistent"), at.plus("dangling")).unwrap();
assert!(at.plus("dangling").is_symlink());

at.write("file.txt", "hello");

// install -D file.txt dangling/subdir/file.txt should fail
scene
.ucmd()
.args(&["-D", "-m", "644"])
.arg(at.plus("file.txt"))
.arg(at.plus("dangling/subdir/file.txt"))
.fails();

// The dangling symlink must not have been replaced with a real directory
assert!(
at.plus("dangling").is_symlink(),
"Dangling symlink must not be replaced with a real directory"
);
assert!(
at.plus("testdir2/a/b").is_dir() && !at.plus("testdir2/a/b").is_symlink(),
"Intermediate directory should be a real directory, not a symlink"
!at.plus("nonexistent").exists(),
"The symlink target must not have been created"
);
}
Loading