From 3fbcb79a3082c281d503631beb41d1ede083c2ee Mon Sep 17 00:00:00 2001 From: Nikola Lukovic Date: Mon, 1 Jun 2026 15:13:58 +0200 Subject: [PATCH] chmod/chown: symlink cycles detection (#11805) --- src/uu/chmod/src/chmod.rs | 102 +++++++++++++++++++++++---- src/uucore/src/lib/features/fs.rs | 1 + src/uucore/src/lib/features/perms.rs | 33 ++++++++- tests/by-util/test_chmod.rs | 62 +++++++++++++++- tests/by-util/test_chown.rs | 86 ++++++++++++++++++++++ 5 files changed, 265 insertions(+), 19 deletions(-) diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index ff8576a0d..4c6b3e0f7 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -6,6 +6,7 @@ // spell-checker:ignore (ToDO) Chmoder cmode fmode fperm fref ugoa RFILE RFILE's use clap::{Arg, ArgAction, Command}; +use std::collections::HashSet; use std::ffi::OsString; use std::fs; use std::os::unix::fs::{MetadataExt, PermissionsExt}; @@ -13,7 +14,7 @@ use std::path::{Path, PathBuf}; use thiserror::Error; use uucore::display::Quotable; use uucore::error::{ExitCode, UError, UResult, USimpleError, UUsageError, set_exit_code}; -use uucore::fs::display_permissions_unix; +use uucore::fs::{FileInformation, display_permissions_unix}; use uucore::mode; use uucore::perms::{TraverseSymlinks, configure_symlink_and_recursion}; @@ -335,6 +336,7 @@ impl Chmoder { &self, path: &Path, is_command_line_arg: bool, + ancestors: &mut HashSet, ) -> UResult<()> { let should_follow_symlink = match self.traverse_symlinks { TraverseSymlinks::All => true, @@ -347,7 +349,7 @@ impl Chmoder { } match fs::metadata(path) { - Ok(meta) if meta.is_dir() => self.walk_dir_with_context(path, false), + Ok(meta) if meta.is_dir() => self.walk_dir_with_context(path, false, ancestors), Ok(_) => { // It's a file symlink, chmod it self.chmod_file(path) @@ -403,7 +405,10 @@ impl Chmoder { return Err(ChmodError::PreserveRoot("/".into()).into()); } if self.recursive { - r = self.walk_dir_with_context(file, true).and(r); + let mut ancestors = HashSet::new(); + r = self + .walk_dir_with_context(file, true, &mut ancestors) + .and(r); } else { r = self.chmod_file(file).and(r); } @@ -417,7 +422,12 @@ impl Chmoder { // Non-safe traversal implementation for platforms without safe_traversal support #[cfg(any(not(unix), target_os = "redox"))] - fn walk_dir_with_context(&self, file_path: &Path, is_command_line_arg: bool) -> UResult<()> { + fn walk_dir_with_context( + &self, + file_path: &Path, + is_command_line_arg: bool, + ancestors: &mut HashSet, + ) -> UResult<()> { let mut r = self.chmod_file(file_path); // Determine whether to traverse symlinks based on context and traversal mode @@ -429,6 +439,17 @@ impl Chmoder { // If the path is a directory (or we should follow symlinks), recurse into it if (!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir() { + // Cycle detection: identify this directory by its target's inode (resolved + // via dereference=true) once, and reuse it for the backtrack below so we + // don't stat the same directory twice. If it's already on the current path, + // it's a cycle. + let dir_info = FileInformation::from_path(file_path, true).ok(); + if let Some(info) = &dir_info { + if !ancestors.insert(info.clone()) { + return r; + } + } + // We buffer all paths in this dir to not keep too many fd's open during recursion let mut paths_in_this_dir = Vec::new(); @@ -445,22 +466,39 @@ impl Chmoder { #[cfg(not(unix))] { if path.is_symlink() { - r = self.handle_symlink_during_recursion(&path).and(r); + r = self + .handle_symlink_during_recursion(&path, ancestors) + .and(r); } else { - r = self.walk_dir_with_context(path.as_path(), false).and(r); + r = self + .walk_dir_with_context(path.as_path(), false, ancestors) + .and(r); } } #[cfg(target_os = "redox")] { - r = self.walk_dir_with_context(path.as_path(), false).and(r); + r = self + .walk_dir_with_context(path.as_path(), false, ancestors) + .and(r); } } + + // Backtrack: remove this directory from ancestors so sibling subtrees + // are not falsely treated as cycles. + if let Some(info) = dir_info { + ancestors.remove(&info); + } } r } #[cfg(all(unix, not(target_os = "redox")))] - fn walk_dir_with_context(&self, file_path: &Path, is_command_line_arg: bool) -> UResult<()> { + fn walk_dir_with_context( + &self, + file_path: &Path, + is_command_line_arg: bool, + ancestors: &mut HashSet, + ) -> UResult<()> { let mut r = self.chmod_file(file_path); // Determine whether to traverse symlinks based on context and traversal mode @@ -474,7 +512,7 @@ impl Chmoder { if (!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir() { match DirFd::open(file_path, SymlinkBehavior::Follow) { Ok(dir_fd) => { - r = self.safe_traverse_dir(&dir_fd, file_path).and(r); + r = self.safe_traverse_dir(&dir_fd, file_path, ancestors).and(r); } Err(err) => { // Handle permission denied errors with proper file path context @@ -490,9 +528,24 @@ impl Chmoder { } #[cfg(all(unix, not(target_os = "redox")))] - fn safe_traverse_dir(&self, dir_fd: &DirFd, dir_path: &Path) -> UResult<()> { + fn safe_traverse_dir( + &self, + dir_fd: &DirFd, + dir_path: &Path, + ancestors: &mut HashSet, + ) -> UResult<()> { let mut r = Ok(()); + // Cycle detection: identify this directory by (dev, ino) via the already-open + // fd. Using the fd is TOCTOU-safe (no path re-resolution through symlinks) and + // avoids a redundant path walk. If it's already on the current path, it's a cycle. + let dir_info = FileInformation::from_file(dir_fd).ok(); + if let Some(info) = &dir_info { + if !ancestors.insert(info.clone()) { + return r; // cycle: this directory is already an ancestor + } + } + let entries = dir_fd.read_dir()?; // Determine if we should follow symlinks (doesn't depend on entry_name) @@ -516,7 +569,12 @@ impl Chmoder { if entry_path.is_symlink() { r = self - .handle_symlink_during_safe_recursion(&entry_path, dir_fd, &entry_name) + .handle_symlink_during_safe_recursion( + &entry_path, + dir_fd, + &entry_name, + ancestors, + ) .and(r); } else { // For regular files and directories, chmod them. @@ -540,7 +598,9 @@ impl Chmoder { if meta.is_dir() { 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); + r = self + .safe_traverse_dir(&child_dir_fd, &entry_path, ancestors) + .and(r); } Err(err) => { let error = if err.kind() == std::io::ErrorKind::PermissionDenied { @@ -554,6 +614,13 @@ impl Chmoder { } } } + + // Backtrack so sibling subtrees that legitimately reach the same directory + // (e.g. two symlinks to one dir) are not mistaken for cycles. + if let Some(info) = dir_info { + ancestors.remove(&info); + } + r } @@ -563,6 +630,7 @@ impl Chmoder { path: &Path, dir_fd: &DirFd, entry_name: &std::ffi::OsStr, + ancestors: &mut HashSet, ) -> UResult<()> { // During recursion, determine behavior based on traversal mode match self.traverse_symlinks { @@ -574,7 +642,7 @@ impl Chmoder { // followed by Follow is the intended behavior and not a TOCTOU concern. // Check if the symlink target is a directory, but handle dangling symlinks gracefully match fs::metadata(path) { - Ok(meta) if meta.is_dir() => self.walk_dir_with_context(path, false), + Ok(meta) if meta.is_dir() => self.walk_dir_with_context(path, false, ancestors), Ok(meta) => { // It's a file symlink, chmod it using safe traversal self.safe_chmod_file( @@ -629,9 +697,13 @@ impl Chmoder { } #[cfg(not(unix))] - fn handle_symlink_during_recursion(&self, path: &Path) -> UResult<()> { + fn handle_symlink_during_recursion( + &self, + path: &Path, + ancestors: &mut HashSet, + ) -> UResult<()> { // Use the common symlink handling logic - self.handle_symlink_during_traversal(path, false) + self.handle_symlink_during_traversal(path, false, ancestors) } fn chmod_file(&self, file: &Path) -> UResult<()> { diff --git a/src/uucore/src/lib/features/fs.rs b/src/uucore/src/lib/features/fs.rs index 351a29f9d..28544d657 100644 --- a/src/uucore/src/lib/features/fs.rs +++ b/src/uucore/src/lib/features/fs.rs @@ -38,6 +38,7 @@ macro_rules! has { } /// Information to uniquely identify a file +#[derive(Clone)] pub struct FileInformation( #[cfg(unix)] rustix::fs::Stat, #[cfg(windows)] winapi_util::file::Information, diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 19bd94385..b94959c40 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -16,11 +16,15 @@ use clap::{Arg, ArgMatches, Command}; use libc::{gid_t, uid_t}; use options::traverse; +#[cfg(target_os = "linux")] +use std::collections::HashSet; use std::ffi::OsString; #[cfg(not(target_os = "linux"))] use walkdir::WalkDir; +#[cfg(target_os = "linux")] +use crate::features::fs::FileInformation; #[cfg(target_os = "linux")] use crate::features::safe_traversal::{DirFd, SymlinkBehavior}; @@ -450,13 +454,30 @@ impl ChownExecutor { return 1; }; + let mut ancestors = HashSet::new(); let mut ret = 0; - self.safe_traverse_dir(&dir_fd, root, &mut ret); + self.safe_traverse_dir(&dir_fd, root, &mut ret, &mut ancestors); ret } #[cfg(target_os = "linux")] - fn safe_traverse_dir(&self, dir_fd: &DirFd, dir_path: &Path, ret: &mut i32) { + fn safe_traverse_dir( + &self, + dir_fd: &DirFd, + dir_path: &Path, + ret: &mut i32, + ancestors: &mut HashSet, + ) { + // Cycle detection: identify this directory by (dev, ino) via the already-open + // fd. Using the fd is TOCTOU-safe (no path re-resolution through symlinks) and + // avoids a redundant path walk. If it's already on the current path, it's a cycle. + let dir_info = FileInformation::from_file(dir_fd).ok(); + if let Some(info) = &dir_info { + if !ancestors.insert(info.clone()) { + return; // cycle detected, stop silently + } + } + // Read directory entries let entries = match dir_fd.read_dir() { Ok(entries) => entries, @@ -544,7 +565,7 @@ impl ChownExecutor { if meta.is_dir() && (follow || !meta.file_type().is_symlink()) { match dir_fd.open_subdir(&entry_name, follow.into()) { Ok(subdir_fd) => { - self.safe_traverse_dir(&subdir_fd, &entry_path, ret); + self.safe_traverse_dir(&subdir_fd, &entry_path, ret, ancestors); } Err(e) => { *ret = 1; @@ -559,6 +580,12 @@ impl ChownExecutor { } } } + + // Backtrack so sibling subtrees that legitimately reach the same directory + // (e.g. two symlinks to one dir) are not mistaken for cycles. + if let Some(info) = dir_info { + ancestors.remove(&info); + } } #[cfg(not(target_os = "linux"))] diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index ac28c5ce5..2541a9f5f 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -2,7 +2,7 @@ // // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (words) dirfd subdirs openat FDCWD +// spell-checker:ignore (words) dirfd subdirs openat FDCWD rwxr use std::fs::{OpenOptions, Permissions, metadata, set_permissions}; use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; @@ -1440,3 +1440,63 @@ fn test_chmod_colored_output() { .stderr_contains("\x1b[31merreur\x1b[0m") // Red "erreur" in French .stderr_contains("\x1b[33m--invalid-option\x1b[0m"); // Yellow invalid option } + +#[test] +fn test_chmod_symlink_cycles() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.mkdir_all("a/b/c"); + at.symlink_dir("a", "a/b/c/d"); + + // Pin the directory modes so the assertions below don't depend on the ambient + // umask (a fresh mkdir under umask 077/027 would not be 0755). + at.set_mode("a", 0o755); + at.set_mode("a/b", 0o755); + at.set_mode("a/b/c", 0o755); + + scene + .ucmd() + .arg("-vRL") + .arg("+r") + .arg("a") + .run() + // cSpell:disable + .stdout_contains_line("mode of 'a' retained as 0755 (rwxr-xr-x)") + .stdout_contains_line("mode of 'a/b' retained as 0755 (rwxr-xr-x)") + .stdout_contains_line("mode of 'a/b/c' retained as 0755 (rwxr-xr-x)") + .stdout_contains_line("mode of 'a/b/c/d' retained as 0755 (rwxr-xr-x)") + .stdout_does_not_contain("mode of 'a/b/c/d/b' retained as 0755 (rwxr-xr-x)") + .stdout_does_not_contain("mode of 'a/b/c/d/b/c' retained as 0755 (rwxr-xr-x)") + .stdout_does_not_contain("mode of 'a/b/c/d/b/c/d' retained as 0755 (rwxr-xr-x)"); + // cSpell:enable +} + +#[test] +fn test_chmod_symlink_two_links_same_dir() { + // Two symlinks pointing at the same directory is NOT a cycle: neither link is + // an ancestor of the other, so the target's contents must be visited through + // *both* links (and through the real directory). This guards the backtracking + // in the cycle-detection logic against false positives. + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + // cSpell:disable + at.mkdir_all("base/realdir"); + at.touch("base/realdir/file"); + at.symlink_dir("base/realdir", "base/link1"); + at.symlink_dir("base/realdir", "base/link2"); + + // Assert on the path only (not the mode bits), so the test is robust to the + // file's umask-dependent permissions across platforms. + scene + .ucmd() + .arg("-vRL") + .arg("+r") + .arg("base") + .run() + .stdout_contains("mode of 'base/realdir/file'") + .stdout_contains("mode of 'base/link1/file'") + .stdout_contains("mode of 'base/link2/file'"); + // cSpell:enable +} diff --git a/tests/by-util/test_chown.rs b/tests/by-util/test_chown.rs index b5db811ac..c03682b4e 100644 --- a/tests/by-util/test_chown.rs +++ b/tests/by-util/test_chown.rs @@ -943,3 +943,89 @@ fn test_chown_no_dereference_symlink_to_dir() { "dir's ctime should not have changed" ); } + +#[test] +fn test_chown_symlink_cycles() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let result = scene.cmd("whoami").run(); + if skipping_test_is_okay(&result, "whoami: cannot find name for user ID") { + return; + } + let user_name = String::from(result.stdout_str().trim()); + assert!(!user_name.is_empty()); + + at.mkdir_all("a/b/c"); + at.symlink_dir("a", "a/b/c/d"); + + let result = scene.ucmd().arg("-vRL").arg(&user_name).arg("a").run(); + + if cfg!(target_os = "macos") || cfg!(target_os = "openbsd") || cfg!(target_os = "android") { + result + .stderr_contains(format!("ownership of 'a' retained as {user_name}")) + .stderr_contains(format!("ownership of 'a/b' retained as {user_name}")) + .stderr_contains(format!("ownership of 'a/b/c' retained as {user_name}")) + .stderr_does_not_contain(format!("ownership of 'a/b/c/d' retained as {user_name}")) + .stderr_does_not_contain(format!("ownership of 'a/b/c/d/b' retained as {user_name}")) + .stderr_does_not_contain(format!( + "ownership of 'a/b/c/d/b/c' retained as {user_name}" + )); + } else { + result + .success() + .stderr_contains(format!("ownership of 'a' retained as {user_name}")) + .stderr_contains(format!("ownership of 'a/b' retained as {user_name}")) + .stderr_contains(format!("ownership of 'a/b/c' retained as {user_name}")) + .stderr_contains(format!("ownership of 'a/b/c/d' retained as {user_name}")) + .stderr_does_not_contain(format!("ownership of 'a/b/c/d/b' retained as {user_name}")) + .stderr_does_not_contain(format!( + "ownership of 'a/b/c/d/b/c' retained as {user_name}" + )) + .stderr_does_not_contain(format!( + "ownership of 'a/b/c/d/b/c/d' retained as {user_name}" + )); + } +} + +#[test] +fn test_chown_symlink_two_links_same_dir() { + // Two symlinks pointing at the same directory is NOT a cycle: neither link is + // an ancestor of the other, so the target's contents must be visited through + // *both* links. This guards the backtracking in the linux safe-traversal + // cycle detection against false positives. + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let result = scene.cmd("whoami").run(); + if skipping_test_is_okay(&result, "whoami: cannot find name for user ID") { + return; + } + let user_name = String::from(result.stdout_str().trim()); + assert!(!user_name.is_empty()); + + // cSpell:disable + at.mkdir_all("base/realdir"); + at.touch("base/realdir/file"); + at.symlink_dir("base/realdir", "base/link1"); + at.symlink_dir("base/realdir", "base/link2"); + + let result = scene.ucmd().arg("-vRL").arg(&user_name).arg("base").run(); + + // Only linux uses the safe-traversal cycle detection that this test exercises; + // other platforms fall back to walkdir with its own loop handling. + if cfg!(target_os = "linux") { + result + .success() + .stderr_contains(format!( + "ownership of 'base/realdir/file' retained as {user_name}" + )) + .stderr_contains(format!( + "ownership of 'base/link1/file' retained as {user_name}" + )) + .stderr_contains(format!( + "ownership of 'base/link2/file' retained as {user_name}" + )); + } + // cSpell:enable +}