From 9dfa1f79d68b82ce79dee05c4abaa7a7475c511c Mon Sep 17 00:00:00 2001 From: Jeff Bailey Date: Sat, 11 Jul 2026 11:06:29 +0100 Subject: [PATCH] tar: resolve symlink resilience and path normalization issues - Use symlink_metadata instead of exists/is_dir in create_archive, get_tree, and print_verbose_tree to avoid incorrectly following symlinks. - Disable following symlinks in tar Builder by default. - Implement clean_path to fix path normalization for paths containing parent directory components (..). - Add integration tests for symlinks, nested symlinks, and path normalization. --- src/uu/tar/src/operations/create.rs | 195 ++++++++++++----- src/uu/tar/src/operations/create_tests.rs | 18 ++ tests/by-util/test_tar.rs | 242 ++++++++++++++++++++++ 3 files changed, 403 insertions(+), 52 deletions(-) diff --git a/src/uu/tar/src/operations/create.rs b/src/uu/tar/src/operations/create.rs index 7714a43..3200615 100644 --- a/src/uu/tar/src/operations/create.rs +++ b/src/uu/tar/src/operations/create.rs @@ -14,14 +14,16 @@ use std::path::{self, Path, PathBuf}; use tar::Builder; use uucore::error::UResult; -/// Create a tar archive from the specified files +/// Create a tar archive from the specified files. /// /// # Arguments /// -/// * `output` - Destination where the tar archive should be written -/// * `files` - Slice of file paths to add to the archive -/// * `allow_absolute` - Allow absolute paths while creating archive -/// * `verbose` - Whether to print verbose output during creation +/// * `output` - Destination where the tar archive data should be written. +/// * `status_output` - Writer for verbose progress output (e.g. stderr/stdout). +/// * `files` - Slice of file paths to add to the archive. +/// * `allow_absolute` - Allow absolute paths while creating archive. +/// * `verbose` - Whether to print verbose output during creation. +/// * `compression` - The compression mode to apply to the output. /// /// # Errors /// @@ -44,16 +46,21 @@ pub fn create_archive( let writer = ArchiveWriter::new(output, compression)?; let mut builder = Builder::new(writer); builder.preserve_absolute(allow_absolute); + builder.follow_symlinks(false); // Add each file or directory to the archive for &path in files { - // Check if path exists - if !path.exists() { - return Err(TarError::FileNotFound { - path: path.to_path_buf(), + // Check if path exists (without following symlinks) + let metadata = match path.symlink_metadata() { + Ok(m) => m, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + return Err(TarError::FileNotFound { + path: path.to_path_buf(), + } + .into()); } - .into()); - } + Err(e) => return Err(TarError::Io(e).into()), + }; if verbose { print_verbose_tree(&mut status_output, path)?; @@ -62,7 +69,7 @@ pub fn create_archive( let normalized_name = get_normalized_path(path, allow_absolute)?; // If it's a directory, recursively add all contents - if path.is_dir() { + if metadata.is_dir() { builder.append_dir_all(normalized_name, path).map_err(|e| { TarError::CannotAddDirectory { path: path.to_path_buf(), @@ -70,7 +77,7 @@ pub fn create_archive( } })?; } else { - // For files, add them directly + // For files/symlinks, add them directly builder .append_path_with_name(path, normalized_name) .map_err(|e| TarError::CannotAddFile { @@ -90,36 +97,78 @@ pub fn create_archive( Ok(()) } +/// Prints the list of files in `path` recursively in a verbose format. +/// +/// Traverses the directory tree at `path` and writes each entry name to +/// `status_output`. Directory entries are printed with a trailing slash. +/// Does not follow symlinks. fn print_verbose_tree(status_output: &mut impl Write, path: &Path) -> UResult<()> { - let to_print = get_tree(path)? - .iter() - .map(|p| (p.is_dir(), p.display().to_string())) - .map(|(is_dir, path)| { - if is_dir { - format!("{}{}", path, path::MAIN_SEPARATOR) - } else { - path - } - }) - .collect::>() - .join("\n"); - writeln!(status_output, "{to_print}").map_err(TarError::Io)?; + for (p, is_dir) in get_tree(path)? { + if is_dir { + writeln!(status_output, "{}{}", p.display(), path::MAIN_SEPARATOR) + .map_err(TarError::Io)?; + } else { + writeln!(status_output, "{}", p.display()).map_err(TarError::Io)?; + } + } Ok(()) } +fn needs_cleaning(path: &Path, allow_absolute: bool) -> bool { + for component in path.components() { + match component { + Prefix(_) | RootDir => { + if !allow_absolute { + return true; + } + } + Component::CurDir | ParentDir => return true, + Component::Normal(_) => {} + } + } + false +} + +/// Normalizes the path for archiving and displays a warning if leading components are stripped. +/// +/// Lexically cleans the path first (resolving `.` and `..`). If `allow_absolute` +/// is false, it strips leading absolute prefixes (`RootDir`, `Prefix`) and +/// leading `ParentDir` (`..`) components. Prints a warning to stderr if any +/// prefix components were removed. fn get_normalized_path(path: &Path, allow_absolute: bool) -> Result { - if let Some(normalized) = normalize_path(path, allow_absolute) { - let original_components: Vec = path.components().collect(); - let normalized_components: Vec = normalized.components().collect(); - if original_components.len() > normalized_components.len() { - let removed: PathBuf = original_components - [..original_components.len() - normalized_components.len()] - .iter() - .collect(); + if !needs_cleaning(path, allow_absolute) { + return Ok(path.to_path_buf()); + } + let cleaned = clean_path(path); + let mut normalized = cleaned.clone(); + let mut prefix_removed = PathBuf::new(); + let mut changed = cleaned != path; + + if !allow_absolute { + let mut remaining = PathBuf::new(); + let mut stripped_any = false; + let mut in_leading = true; + for c in cleaned.components() { + if in_leading && matches!(c, RootDir | Prefix(_) | ParentDir) { + stripped_any = true; + prefix_removed.push(c.as_os_str()); + } else { + in_leading = false; + remaining.push(c.as_os_str()); + } + } + if stripped_any { + normalized = remaining; + changed = true; + } + } + + if changed { + if !prefix_removed.as_os_str().is_empty() { writeln!( std::io::stderr(), "tar: Removing leading `{}' from member names", - removed.display() + prefix_removed.display() )?; } Ok(normalized) @@ -128,33 +177,75 @@ fn get_normalized_path(path: &Path, allow_absolute: bool) -> Result Result, std::io::Error> { +/// Recursively gathers all paths within `path` without following symlinks. +/// +/// Returns a list of tuples containing the path and a boolean indicating +/// whether the path is a directory. Using `symlink_metadata` and +/// `DirEntry::file_type` prevents recursing into symlinked directories. +fn get_tree(path: &Path) -> Result, std::io::Error> { let mut paths = Vec::new(); let mut stack = VecDeque::new(); - stack.push_back(path.to_path_buf()); - - while let Some(current) = stack.pop_back() { - paths.push(current.clone()); - if current.is_dir() { - for entry in fs::read_dir(current)? { - let child = entry?.path(); - stack.push_back(child); + + let root_metadata = path.symlink_metadata()?; + let root_is_dir = root_metadata.is_dir(); + stack.push_back((path.to_path_buf(), root_is_dir)); + + while let Some((current, is_dir)) = stack.pop_back() { + if is_dir { + for entry in fs::read_dir(¤t)? { + let entry = entry?; + let child = entry.path(); + let file_type = entry.file_type()?; + let child_is_dir = file_type.is_dir(); + stack.push_back((child, child_is_dir)); } } + paths.push((current, is_dir)); } Ok(paths) } -fn normalize_path(path: &Path, allow_absolute: bool) -> Option { - if path.is_absolute() && !allow_absolute { - Some( - path.components() - .filter(|c| !matches!(c, RootDir | ParentDir | Prefix(_))) - .collect::(), - ) +/// Performs lexical cleaning of a path. +/// +/// Resolves `.` and `..` components where possible without accessing the +/// actual filesystem. This is used to normalize paths safely and prevent +/// equivalent path confusion. +fn clean_path(path: &Path) -> PathBuf { + let mut clean = PathBuf::new(); + for component in path.components() { + match component { + Prefix(_) | RootDir => { + clean.push(component.as_os_str()); + } + Component::CurDir => {} + ParentDir => { + if let Some(last) = clean.components().next_back() { + match last { + Component::Normal(_) => { + clean.pop(); + } + RootDir | Prefix(_) => { + // Cannot go above root/prefix + } + ParentDir => { + clean.push(component); + } + Component::CurDir => unreachable!(), + } + } else { + clean.push(component); + } + } + Component::Normal(c) => { + clean.push(c); + } + } + } + if clean.as_os_str().is_empty() { + PathBuf::from(".") } else { - None + clean } } diff --git a/src/uu/tar/src/operations/create_tests.rs b/src/uu/tar/src/operations/create_tests.rs index dbbc71c..1ed18af 100644 --- a/src/uu/tar/src/operations/create_tests.rs +++ b/src/uu/tar/src/operations/create_tests.rs @@ -98,3 +98,21 @@ fn test_create_archive_missing_file_fails() { .unwrap_err(); assert!(err.to_string().contains("missing.txt")); } + +#[test] +fn test_clean_path() { + // Empty/relative to current dir cases + assert_eq!(clean_path(Path::new(".")), PathBuf::from(".")); + assert_eq!(clean_path(Path::new("a/..")), PathBuf::from(".")); + assert_eq!(clean_path(Path::new("a/b/../..")), PathBuf::from(".")); + assert_eq!(clean_path(Path::new("")), PathBuf::from(".")); + + // Root/Prefix limit cases (cannot go above root) + assert_eq!(clean_path(Path::new("/..")), PathBuf::from("/")); + assert_eq!(clean_path(Path::new("/a/../../b")), PathBuf::from("/b")); + + // Parent dir preservation cases + assert_eq!(clean_path(Path::new("../..")), PathBuf::from("../..")); + assert_eq!(clean_path(Path::new("../../a")), PathBuf::from("../../a")); + assert_eq!(clean_path(Path::new("a/../../b")), PathBuf::from("../b")); +} diff --git a/tests/by-util/test_tar.rs b/tests/by-util/test_tar.rs index 23cefa3..8d4397b 100644 --- a/tests/by-util/test_tar.rs +++ b/tests/by-util/test_tar.rs @@ -1326,3 +1326,245 @@ fn test_extract_invalid_gzip_archive_fails() { ucmd.args(&["-xf", "invalid.tar.gz"]).fails().code_is(2); } + +#[test] +#[cfg(unix)] +fn test_create_with_symlinks() { + let (at, mut ucmd) = at_and_ucmd!(); + + at.write("file.txt", "target content"); + + let link_path = at.plus("link.txt"); + std::os::unix::fs::symlink("file.txt", &link_path).unwrap(); + + let broken_link_path = at.plus("broken_link.txt"); + std::os::unix::fs::symlink("nonexistent.txt", &broken_link_path).unwrap(); + + at.mkdir("dir"); + at.write("dir/file.txt", "dir file content"); + + let dir_link_path = at.plus("dir_link"); + std::os::unix::fs::symlink("dir", &dir_link_path).unwrap(); + + ucmd.args(&[ + "-cf", + "archive.tar", + "file.txt", + "link.txt", + "broken_link.txt", + "dir_link", + ]) + .succeeds() + .no_output(); + + assert!(at.file_exists("archive.tar")); + + at.mkdir("extract_dir"); + let archive_abs_path = at.plus("archive.tar"); + + new_ucmd!() + .arg("-xf") + .arg(&archive_abs_path) + .current_dir(at.plus("extract_dir")) + .succeeds() + .no_output(); + + let extracted_at = at.plus("extract_dir"); + + let link_meta = std::fs::symlink_metadata(extracted_at.join("link.txt")).unwrap(); + assert!(link_meta.file_type().is_symlink()); + assert_eq!( + std::fs::read_link(extracted_at.join("link.txt")).unwrap(), + std::path::Path::new("file.txt") + ); + + let broken_link_meta = std::fs::symlink_metadata(extracted_at.join("broken_link.txt")).unwrap(); + assert!(broken_link_meta.file_type().is_symlink()); + assert_eq!( + std::fs::read_link(extracted_at.join("broken_link.txt")).unwrap(), + std::path::Path::new("nonexistent.txt") + ); + + let dir_link_meta = std::fs::symlink_metadata(extracted_at.join("dir_link")).unwrap(); + assert!(dir_link_meta.file_type().is_symlink()); + assert_eq!( + std::fs::read_link(extracted_at.join("dir_link")).unwrap(), + std::path::Path::new("dir") + ); +} + +#[test] +#[cfg(unix)] +fn test_create_with_nested_symlinks() { + let (at, mut ucmd) = at_and_ucmd!(); + + at.mkdir("parent"); + at.write("parent/file.txt", "content"); + + let link_path = at.plus("parent/link.txt"); + std::os::unix::fs::symlink("file.txt", &link_path).unwrap(); + + let broken_link_path = at.plus("parent/broken_link.txt"); + std::os::unix::fs::symlink("nonexistent.txt", &broken_link_path).unwrap(); + + at.mkdir("parent/child"); + at.write("parent/child/file.txt", "child file"); + + let dir_link_path = at.plus("parent/dir_link"); + std::os::unix::fs::symlink("child", &dir_link_path).unwrap(); + + ucmd.args(&["-cf", "archive.tar", "parent"]) + .succeeds() + .no_output(); + + assert!(at.file_exists("archive.tar")); + + at.mkdir("extract_dir"); + let archive_abs_path = at.plus("archive.tar"); + + new_ucmd!() + .arg("-xf") + .arg(&archive_abs_path) + .current_dir(at.plus("extract_dir")) + .succeeds() + .no_output(); + + let extracted_parent = at.plus("extract_dir").join("parent"); + + assert_eq!( + std::fs::read_link(extracted_parent.join("link.txt")).unwrap(), + std::path::Path::new("file.txt") + ); + assert_eq!( + std::fs::read_link(extracted_parent.join("broken_link.txt")).unwrap(), + std::path::Path::new("nonexistent.txt") + ); + assert_eq!( + std::fs::read_link(extracted_parent.join("dir_link")).unwrap(), + std::path::Path::new("child") + ); +} + +#[test] +#[cfg(unix)] +fn test_create_with_symlinks_verbose() { + let (at, mut ucmd) = at_and_ucmd!(); + + at.write("file.txt", "content"); + + at.mkdir("dir"); + at.write("dir/file.txt", "content"); + + let dir_link_path = at.plus("dir_link"); + std::os::unix::fs::symlink("dir", &dir_link_path).unwrap(); + + let result = ucmd + .args(&["-cvf", "archive.tar", "file.txt", "dir_link"]) + .succeeds(); + + let stdout = result.stdout_str(); + + assert!(stdout.contains("file.txt")); + assert!(stdout.contains("dir_link")); + assert!(!stdout.contains("dir_link/")); + assert!(!stdout.contains("dir_link/file.txt")); +} + +#[test] +fn test_create_absolute_path_with_parent_dirs() { + let (at, mut ucmd) = at_and_ucmd!(); + + at.mkdir("a"); + at.mkdir("a/b"); + let mut file_abs_path = PathBuf::from(at.root_dir_resolved()); + file_abs_path.push("a"); + file_abs_path.push("b"); + file_abs_path.push(".."); + file_abs_path.push("c"); + + at.write("a/c", "content"); + + ucmd.args(&["-cf", "archive.tar", &file_abs_path.display().to_string()]) + .succeeds(); + + let expected_path = PathBuf::from(at.root_dir_resolved()) + .components() + .filter(|c| !matches!(c, path::Component::RootDir | path::Component::Prefix(_))) + .collect::() + .join("a") + .join("c"); + + let expected_path_str = expected_path.to_string_lossy().replace('\\', "/"); + new_ucmd!() + .args(&["-tf", "archive.tar"]) + .current_dir(at.as_string()) + .succeeds() + .stdout_contains(expected_path_str); +} + +#[test] +fn test_create_relative_path_with_leading_parent_dirs() { + let (at, mut ucmd) = at_and_ucmd!(); + + at.write("file.txt", "content"); + at.mkdir("subdir"); + + ucmd.args(&["-cf", "archive.tar", "../file.txt"]) + .current_dir(at.plus("subdir")) + .succeeds() + .stderr_contains("tar: Removing leading `..' from member names"); + + new_ucmd!() + .args(&["-tf", "subdir/archive.tar"]) + .current_dir(at.as_string()) + .succeeds() + .stdout_contains("file.txt"); +} + +#[test] +#[cfg(unix)] +fn test_create_input_permission_denied() { + if rustix::process::geteuid().is_root() { + eprintln!("skipping: running as root"); + return; + } + + let (at, mut ucmd) = at_and_ucmd!(); + + at.mkdir("private"); + at.write("private/file.txt", "content"); + // Make directory inaccessible + at.set_mode("private", 0o000); + + ucmd.args(&["-cf", "archive.tar", "private/file.txt"]) + .fails() + .code_is(2) + .stderr_contains("Permission denied"); + + // Cleanup + at.set_mode("private", 0o755); +} + +#[test] +#[cfg(unix)] +fn test_create_directory_unreadable_file() { + if rustix::process::geteuid().is_root() { + eprintln!("skipping: running as root"); + return; + } + + let (at, mut ucmd) = at_and_ucmd!(); + + at.mkdir("dir"); + at.write("dir/file.txt", "content"); + // Make file unreadable + at.set_mode("dir/file.txt", 0o000); + + ucmd.args(&["-cf", "archive.tar", "dir"]) + .fails() + .code_is(2) + .stderr_contains("Permission denied"); + + // Cleanup + at.set_mode("dir/file.txt", 0o644); +}