diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index 6eb9add77e..ff8576a0d9 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -533,9 +533,12 @@ impl Chmoder { ) .and(r); - // Recurse into subdirectories using the existing directory fd + // Recurse into subdirectories using the existing directory fd. + // Open with NoFollow unless `-L` was requested: this prevents a + // TOCTOU where an attacker swaps the just-stat'd directory for a + // symlink before the open and redirects the descent off-tree. if meta.is_dir() { - match dir_fd.open_subdir(&entry_name, SymlinkBehavior::Follow) { + match dir_fd.open_subdir(&entry_name, should_follow_symlink.into()) { Ok(child_dir_fd) => { r = self.safe_traverse_dir(&child_dir_fd, &entry_path).and(r); } diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index d2bcf6e2f4..c7ef3f581d 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -380,8 +380,12 @@ pub fn safe_remove_dir_recursive_impl(path: &Path, dir_fd: &DirFd, options: &Opt continue; } - // Recursively remove subdirectory using safe traversal - let child_dir_fd = match dir_fd.open_subdir(&entry_name, SymlinkBehavior::Follow) { + // Recursively remove subdirectory using safe traversal. rm never + // follows symlinks during recursion, so open with NoFollow: if an + // attacker swaps this just-stat'd directory for a symlink before the + // open, O_NOFOLLOW makes openat fail instead of descending off-tree + // and deleting unrelated files. + let child_dir_fd = match dir_fd.open_subdir(&entry_name, SymlinkBehavior::NoFollow) { Ok(fd) => fd, Err(e) => { // If we can't open the subdirectory for safe traversal, diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 79a51b0cca..871f2c1e61 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -535,9 +535,13 @@ impl ChownExecutor { ); } - // Recurse into subdirectories + // Recurse into subdirectories. Open with the same symlink behavior + // used for the stat above: with NoFollow (the default, `-P`/`-H`) an + // attacker that swaps the just-stat'd directory for a symlink between + // the stat and this open cannot redirect the descent off-tree + // (O_NOFOLLOW makes openat fail). Only follow when `-L` was requested. if meta.is_dir() && (follow || !meta.file_type().is_symlink()) { - match dir_fd.open_subdir(&entry_name, SymlinkBehavior::Follow) { + match dir_fd.open_subdir(&entry_name, follow.into()) { Ok(subdir_fd) => { self.safe_traverse_dir(&subdir_fd, &entry_path, ret); } diff --git a/src/uucore/src/lib/features/safe_traversal.rs b/src/uucore/src/lib/features/safe_traversal.rs index 6807449a74..1dff1b4aa4 100644 --- a/src/uucore/src/lib/features/safe_traversal.rs +++ b/src/uucore/src/lib/features/safe_traversal.rs @@ -948,6 +948,35 @@ mod tests { assert!(subdir_fd.as_raw_fd() >= 0); } + #[test] + fn test_dirfd_open_subdir_nofollow_refuses_symlink() { + // A symlink to a directory must NOT be opened as a subdir when NoFollow + // is requested. Recursive chown/chgrp/chmod/rm rely on this to refuse a + // directory that was swapped for a symlink mid-traversal (TOCTOU), + // instead of following it off-tree. + let temp_dir = TempDir::new().unwrap(); + let real_dir = temp_dir.path().join("real"); + fs::create_dir(&real_dir).unwrap(); + symlink(&real_dir, temp_dir.path().join("link")).unwrap(); + + let parent_fd = DirFd::open(temp_dir.path(), SymlinkBehavior::Follow).unwrap(); + + // NoFollow must reject the symlink (ELOOP). + let nofollow = parent_fd.open_subdir(OsStr::new("link"), SymlinkBehavior::NoFollow); + assert!(nofollow.is_err()); + + // Follow still resolves it (the explicit `-L` opt-in). + let follow = parent_fd.open_subdir(OsStr::new("link"), SymlinkBehavior::Follow); + assert!(follow.is_ok()); + + // A real directory is opened fine either way. + assert!( + parent_fd + .open_subdir(OsStr::new("real"), SymlinkBehavior::NoFollow) + .is_ok() + ); + } + #[test] fn test_dirfd_open_nonexistent_subdir() { let temp_dir = TempDir::new().unwrap(); diff --git a/util/check-safe-traversal.sh b/util/check-safe-traversal.sh index 97869789d7..871abb0a9e 100755 --- a/util/check-safe-traversal.sh +++ b/util/check-safe-traversal.sh @@ -168,6 +168,24 @@ check_utility() { fi } +# Recursive descent must open every subdirectory relative to the parent dirfd +# WITH O_NOFOLLOW. Otherwise an attacker who swaps the just-stat'd directory for +# a symlink between the stat and the open redirects the descent off-tree (the +# chown/chgrp/chmod/rm recursive-descent TOCTOU). The top-level command-line +# argument is opened via AT_FDCWD and is allowed to follow, so it is excluded by +# matching only numeric-dirfd openat calls. +assert_descent_nofollow() { + local util="$1" + local log="$2" + local offenders + offenders=$(grep -E 'openat\([0-9]+, "[^"]*",[^)]*O_DIRECTORY' "$log" | grep -v 'O_NOFOLLOW' || true) + if [ -n "$offenders" ]; then + echo "$offenders" + fail_immediately "$util descends into subdirectories without O_NOFOLLOW (TOCTOU symlink-swap risk)" + fi + echo "✓ $util descent opens use O_NOFOLLOW" +} + # Get list of available utilities if [ "$USE_MULTICALL" -eq 1 ]; then AVAILABLE_UTILS=$($COREUTILS_BIN --list) @@ -192,6 +210,7 @@ if echo "$AVAILABLE_UTILS" | grep -q "rm"; then if grep -qE 'statx\(AT_FDCWD, "[^"]*/' strace_rm_recursive_remove.log; then fail_immediately "rm is using path-based statx (multi-component relative path); expected dirfd-relative newfstatat" fi + assert_descent_nofollow "rm" strace_rm_recursive_remove.log fi # Test chmod - should use openat, fchmodat, newfstatat @@ -203,6 +222,7 @@ if echo "$AVAILABLE_UTILS" | grep -q "chmod"; then if grep -q 'openat(AT_FDCWD, "test_chmod/' strace_chmod_recursive_chmod.log; then fail_immediately "chmod recursed using AT_FDCWD with a multi-component path; expected dirfd-relative openat" fi + assert_descent_nofollow "chmod" strace_chmod_recursive_chmod.log fi # Test chown - should use openat, fchownat, newfstatat @@ -211,12 +231,14 @@ if echo "$AVAILABLE_UTILS" | grep -q "chown"; then USER_ID=$(id -u) GROUP_ID=$(id -g) check_utility "chown" "openat,fchownat,newfstatat,chown,lchown" "openat fchownat" "-R $USER_ID:$GROUP_ID test_chown" "recursive_chown" + assert_descent_nofollow "chown" strace_chown_recursive_chown.log fi # Test chgrp - should use openat, fchownat, newfstatat if echo "$AVAILABLE_UTILS" | grep -q "chgrp"; then cp -r test_dir test_chgrp check_utility "chgrp" "openat,fchownat,newfstatat,chown,lchown" "openat fchownat" "-R $GROUP_ID test_chgrp" "recursive_chgrp" + assert_descent_nofollow "chgrp" strace_chgrp_recursive_chgrp.log fi # Test du - should use openat, newfstatat