diff --git a/src/uu/rm/Cargo.toml b/src/uu/rm/Cargo.toml index a7d959977..b8d0955f5 100644 --- a/src/uu/rm/Cargo.toml +++ b/src/uu/rm/Cargo.toml @@ -20,7 +20,7 @@ path = "src/rm.rs" [dependencies] thiserror = { workspace = true } clap = { workspace = true } -uucore = { workspace = true, features = ["fs", "parser"] } +uucore = { workspace = true, features = ["fs", "parser", "safe-traversal"] } fluent = { workspace = true } [target.'cfg(unix)'.dependencies] diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index ac5a818a6..763590f79 100644 --- a/src/uu/rm/src/rm.rs +++ b/src/uu/rm/src/rm.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (path) eacces inacc rm-r4 +// spell-checker:ignore (path) eacces inacc rm-r4 unlinkat fstatat use clap::builder::{PossibleValue, ValueParser}; use clap::{Arg, ArgAction, Command, parser::ValueSource}; @@ -21,6 +21,8 @@ use thiserror::Error; use uucore::display::Quotable; use uucore::error::{FromIo, UError, UResult}; use uucore::parser::shortcut_value_parser::ShortcutValueParser; +#[cfg(target_os = "linux")] +use uucore::safe_traversal::DirFd; use uucore::translate; use uucore::{format_usage, os_str_as_bytes, prompt_yes, show_error}; @@ -428,6 +430,140 @@ fn is_writable(_path: &Path) -> bool { true } +#[cfg(target_os = "linux")] +fn safe_remove_dir_recursive(path: &Path, options: &Options) -> bool { + // Try to open the directory using DirFd for secure traversal + let dir_fd = match DirFd::open(path) { + Ok(fd) => fd, + Err(e) => { + show_error!( + "{}", + e.map_err_context(|| translate!("rm-error-cannot-remove", "file" => path.quote())) + ); + return true; + } + }; + + let error = safe_remove_dir_recursive_impl(path, &dir_fd, options); + + // After processing all children, remove the directory itself + if error { + error + } else { + // Ask user permission if needed + if options.interactive == InteractiveMode::Always && !prompt_dir(path, options) { + return false; + } + + // Use regular fs::remove_dir for the root since we can't unlinkat ourselves + match fs::remove_dir(path) { + Ok(_) => false, + Err(e) => { + let e = e.map_err_context( + || translate!("rm-error-cannot-remove", "file" => path.quote()), + ); + show_error!("{e}"); + true + } + } + } +} + +#[cfg(target_os = "linux")] +fn safe_remove_dir_recursive_impl(path: &Path, dir_fd: &DirFd, options: &Options) -> bool { + // Check if we should descend into this directory + if options.interactive == InteractiveMode::Always + && !is_dir_empty(path) + && !prompt_descend(path) + { + return false; + } + + // Read directory entries using safe traversal + let entries = match dir_fd.read_dir() { + Ok(entries) => entries, + Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { + // This is not considered an error - just like the original + return false; + } + Err(e) => { + show_error!( + "{}", + e.map_err_context(|| translate!("rm-error-cannot-remove", "file" => path.quote())) + ); + return true; + } + }; + + let mut error = false; + + // Process each entry + for entry_name in entries { + let entry_path = path.join(&entry_name); + + // Get metadata for the entry using fstatat + let entry_stat = match dir_fd.stat_at(&entry_name, false) { + Ok(stat) => stat, + Err(e) => { + let e = e.map_err_context( + || translate!("rm-error-cannot-remove", "file" => entry_path.quote()), + ); + show_error!("{e}"); + error = true; + continue; + } + }; + + // Check if it's a directory + let is_dir = (entry_stat.st_mode & libc::S_IFMT) == libc::S_IFDIR; + + if is_dir { + // Recursively remove directory + let subdir_fd = match dir_fd.open_subdir(&entry_name) { + Ok(fd) => fd, + Err(e) => { + let e = e.map_err_context( + || translate!("rm-error-cannot-remove", "file" => entry_path.quote()), + ); + show_error!("{e}"); + error = true; + continue; + } + }; + + let child_error = safe_remove_dir_recursive_impl(&entry_path, &subdir_fd, options); + error = error || child_error; + + // Try to remove the directory (even if there were some child errors) + // Ask user permission if needed + if options.interactive == InteractiveMode::Always && !prompt_dir(&entry_path, options) { + continue; + } + + if let Err(e) = dir_fd.unlink_at(&entry_name, true) { + let e = e.map_err_context( + || translate!("rm-error-cannot-remove", "file" => entry_path.quote()), + ); + show_error!("{e}"); + error = true; + } + } else { + // Remove file - check if user wants to remove it first + if prompt_file(&entry_path, options) { + if let Err(e) = dir_fd.unlink_at(&entry_name, false) { + let e = e.map_err_context( + || translate!("rm-error-cannot-remove", "file" => entry_path.quote()), + ); + show_error!("{e}"); + error = true; + } + } + } + } + + error +} + /// Recursively remove the directory tree rooted at the given path. /// /// If `path` is a file or a symbolic link, just remove it. If it is a @@ -454,25 +590,30 @@ fn remove_dir_recursive(path: &Path, options: &Options) -> bool { return false; } - // Special case: if we cannot access the metadata because the - // filename is too long, fall back to try - // `fs::remove_dir_all()`. - // - // TODO This is a temporary bandage; we shouldn't need to do this - // at all. Instead of using the full path like "x/y/z", which - // causes a `InvalidFilename` error when trying to access the file - // metadata, we should be able to use just the last part of the - // path, "z", and know that it is relative to the parent, "x/y". - if let Some(s) = path.to_str() { - if s.len() > 1000 { - match fs::remove_dir_all(path) { - Ok(_) => return false, - Err(e) => { - let e = e.map_err_context( - || translate!("rm-error-cannot-remove", "file" => path.quote()), - ); - show_error!("{e}"); - return true; + // Use secure traversal on Linux for long paths + #[cfg(target_os = "linux")] + { + if let Some(s) = path.to_str() { + if s.len() > 1000 { + return safe_remove_dir_recursive(path, options); + } + } + } + + // Fallback for non-Linux or shorter paths + #[cfg(not(target_os = "linux"))] + { + if let Some(s) = path.to_str() { + if s.len() > 1000 { + match fs::remove_dir_all(path) { + Ok(_) => return false, + Err(e) => { + let e = e.map_err_context( + || translate!("rm-error-cannot-remove", "file" => path.quote()), + ); + show_error!("{e}"); + return true; + } } } } diff --git a/src/uucore/src/lib/features/safe_traversal.rs b/src/uucore/src/lib/features/safe_traversal.rs index 177689b8c..f4817ce41 100644 --- a/src/uucore/src/lib/features/safe_traversal.rs +++ b/src/uucore/src/lib/features/safe_traversal.rs @@ -255,8 +255,35 @@ impl DirFd { }) } - pub fn as_raw_fd(&self) -> RawFd { - self.fd + /// Remove a file or empty directory relative to this directory + pub fn unlink_at(&self, name: &OsStr, is_dir: bool) -> io::Result<()> { + let name_str = name.to_string_lossy(); + let name_cstr = + CString::new(name.as_bytes()).map_err(|_| SafeTraversalError::PathContainsNull)?; + let flags = if is_dir { libc::AT_REMOVEDIR } else { 0 }; + + let ret = unsafe { libc::unlinkat(self.fd, name_cstr.as_ptr(), flags) }; + + if ret < 0 { + Err(SafeTraversalError::UnlinkFailed { + path: name_str.to_string(), + source: io::Error::last_os_error(), + } + .into()) + } else { + Ok(()) + } + } + + /// Create a DirFd from an existing file descriptor (does not take ownership) + pub fn from_raw_fd(fd: RawFd) -> io::Result { + if fd < 0 { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "invalid file descriptor", + )); + } + Ok(DirFd { fd, owned: false }) } } diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index 69ef09691..db31ab876 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -1054,3 +1054,27 @@ fn test_non_utf8_paths() { assert!(!at.dir_exists(non_utf8_dir_name)); } + +#[test] +#[cfg(target_os = "linux")] +fn test_rm_recursive_long_path_safe_traversal() { + let ts = TestScenario::new(util_name!()); + let at = &ts.fixtures; + + let mut deep_path = String::from("rm_deep"); + at.mkdir(&deep_path); + + for i in 0..12 { + let long_dir_name = format!("{}{}", "z".repeat(80), i); + deep_path = format!("{deep_path}/{long_dir_name}"); + at.mkdir_all(&deep_path); + } + + at.write("rm_deep/test1.txt", "content1"); + at.write(&format!("{deep_path}/test2.txt"), "content2"); + + ts.ucmd().arg("-rf").arg("rm_deep").succeeds(); + + // Verify the directory is completely removed + assert!(!at.dir_exists("rm_deep")); +}