diff --git a/src/uucore/src/lib/features/safe_traversal.rs b/src/uucore/src/lib/features/safe_traversal.rs index 075bce7b67..6807449a74 100644 --- a/src/uucore/src/lib/features/safe_traversal.rs +++ b/src/uucore/src/lib/features/safe_traversal.rs @@ -465,11 +465,11 @@ impl DirFd { } } -/// Find the deepest existing real directory ancestor for a path. +/// Find the deepest existing 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 -/// treated as components that need to be created/replaced. +/// Uses `metadata` (follows symlinks) so that symlinks to directories are treated as +/// existing ancestors rather than components to create. fn find_existing_ancestor(path: &Path) -> io::Result<(PathBuf, Vec)> { let mut current = path.to_path_buf(); let mut components: Vec = Vec::new(); @@ -537,8 +537,8 @@ fn find_existing_ancestor(path: &Path) -> io::Result<(PathBuf, Vec)> { /// 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 -/// removed and replaced with a real directory. +/// path component. If a symlink to a directory exists, it is followed (GNU +/// coreutils behavior). Dangling symlinks and non-directory entries are errors. /// /// # Arguments /// * `parent_fd` - The parent directory file descriptor @@ -580,8 +580,8 @@ fn open_or_create_subdir(parent_fd: &DirFd, name: &OsStr, mode: u32) -> io::Resu /// This prevents symlink race conditions by anchoring all operations to directory fds. /// /// # Security -/// This function prevents TOCTOU race conditions by: -/// 1. Finding the deepest existing ancestor directory (path-based, but safe since it exists) +/// This function prevents TOCTOU race conditions for newly created directories by: +/// 1. Finding the deepest existing ancestor directory (path-based, following symlinks) /// 2. Opening that ancestor with a file descriptor /// 3. Creating all new directories using fd-based operations (mkdirat, openat with O_NOFOLLOW) /// @@ -589,8 +589,10 @@ fn open_or_create_subdir(parent_fd: &DirFd, name: &OsStr, mode: u32) -> io::Resu /// as the anchor. If an attacker replaces a newly-created directory with a symlink, /// our openat with O_NOFOLLOW will fail, preventing the attack. /// -/// Existing symlinks in the path (like /var -> /private/var on macOS) are followed -/// when finding the ancestor, which is safe since they already exist. +/// Pre-existing symlinks to directories in the path are followed (GNU coreutils behavior). +/// `O_DIRECTORY` is used when opening them, so dangling or non-directory symlinks error out. +/// Note that a residual TOCTOU window exists between stat and open for such symlinks, +/// which is the same trade-off made by GNU coreutils. /// /// # Arguments /// * `path` - The path to create directories for diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index 411565de8b..ebac9f5127 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -2582,7 +2582,7 @@ fn test_install_normal_file_replaces_symlink() { #[test] #[cfg(unix)] -fn test_install_d_symlink_race_condition() { +fn test_install_d_symlink_in_path_is_followed() { // 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; @@ -2621,45 +2621,6 @@ fn test_install_d_symlink_race_condition() { ); } -#[test] -#[cfg(unix)] -fn test_install_d_symlink_race_condition_concurrent() { - // 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; - - at.mkdir("target2"); - at.write("source_file2", "test content 2"); - - at.mkdir_all("testdir2/a"); - symlink(at.plus("target2"), at.plus("testdir2/a/b")).unwrap(); - - scene - .ucmd() - .arg("-D") - .arg(at.plus("source_file2")) - .arg(at.plus("testdir2/a/b/c/file")) - .succeeds(); - - // 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" - ); - - // Symlink should be preserved - assert!( - 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() {