From 52ac8a9f3d043ec846ffc7156a219b6ce6051c78 Mon Sep 17 00:00:00 2001 From: curious-rabbit Date: Wed, 20 May 2026 23:11:12 +0200 Subject: [PATCH 1/6] security improvements --- src/archive/rar.rs | 20 ++- src/archive/sevenz.rs | 32 ++++- src/archive/tar.rs | 35 +++-- src/archive/zip.rs | 53 +++++-- src/cli/args.rs | 2 +- src/commands/decompress.rs | 26 +++- src/commands/list.rs | 28 +++- src/list.rs | 19 +-- src/utils/formatting.rs | 43 +++++- src/utils/fs.rs | 129 ++++++++++++++++++ src/utils/io.rs | 1 + src/utils/logger.rs | 5 +- .../ui__ui_test_usage_help_flag-2.snap | 3 +- .../ui__ui_test_usage_help_flag.snap | 2 + 14 files changed, 337 insertions(+), 61 deletions(-) diff --git a/src/archive/rar.rs b/src/archive/rar.rs index acc55575b..d8e2c1c3a 100644 --- a/src/archive/rar.rs +++ b/src/archive/rar.rs @@ -11,7 +11,8 @@ use crate::{ error::{Error, FinalError, Result}, info, list::{FileInArchive, ListFileType}, - utils::{BytesFmt, PathFmt}, + utils::{BytesFmt, PathFmt, validate_entry_path}, + warning, }; /// Unpacks the archive given by `archive_path` into the folder given by `output_folder`. @@ -26,8 +27,18 @@ pub fn unpack_archive(archive_path: &Path, output_folder: &Path, password: Optio let mut files_unpacked: u64 = 0; let mut first_err: Option<(PathBuf, i32)> = None; + let mut unsafe_path: Option<(PathBuf, String)> = None; let cb_result = archive.extract_all_with_callback(output_folder, |event| match event { + ExtractEvent::Start { filename, .. } => { + if let Err(e) = validate_entry_path(&filename) { + warning!("refusing unsafe rar entry {}: {}", PathFmt(&filename), e); + unsafe_path = Some((filename, e.to_string())); + false + } else { + true + } + } ExtractEvent::Ok { filename, size } => { info!("extracted ({}) {}", BytesFmt(size), PathFmt(&filename)); files_unpacked += 1; @@ -55,6 +66,13 @@ pub fn unpack_archive(archive_path: &Path, output_folder: &Path, password: Optio _ => true, }); + if let Some((path, reason)) = unsafe_path { + return Err(Error::Custom { + reason: FinalError::with_title(format!("refusing to extract unsafe rar entry {}", PathFmt(&path))) + .detail(reason), + }); + } + if let Some((path, code)) = first_err { let inner = UnrarError::from(Code::from(code), When::Process).to_string(); return Err(Error::Custom { diff --git a/src/archive/sevenz.rs b/src/archive/sevenz.rs index a45248e61..593d60b95 100644 --- a/src/archive/sevenz.rs +++ b/src/archive/sevenz.rs @@ -1,7 +1,6 @@ //! SevenZip archive format compress function use std::{ - env, io::{self, BufWriter, Read, Seek, Write}, path::{Path, PathBuf}, }; @@ -18,7 +17,9 @@ use crate::{ info, list::{FileInArchive, ListFileType}, utils::{ - BytesFmt, FileVisibilityPolicy, PathFmt, cd_into_same_dir_as, ensure_parent_dir_exists, is_same_file_as_output, + BytesFmt, FileVisibilityPolicy, LimitedReader, PathFmt, SanitizedStr, cd_into_same_dir_as, + ensure_parent_dir_exists, is_same_file_as_output, max_decompressed_bytes, validate_dest_inside_root, + validate_entry_path, }, warning, }; @@ -33,10 +34,27 @@ where |entry: &ArchiveEntry, reader: &mut dyn Read, path: &PathBuf| -> Result { // Manually handle writing all files from 7z archive (the library defaults ignore empty files) - let file_path = output_path.join(entry.name()); + let name_as_path = std::path::Path::new(entry.name()); + let safe_relpath = match validate_entry_path(name_as_path) { + Ok(p) => p, + Err(e) => { + warning!("skipping unsafe 7z entry {}: {}", PathFmt(name_as_path), e); + return Ok(true); + } + }; + let file_path = output_path.join(&safe_relpath); + + if let Err(e) = validate_dest_inside_root(output_path, &file_path) { + warning!("skipping 7z entry {}: {}", PathFmt(&file_path), e); + return Ok(true); + } if entry.is_directory() { - info!("File {} extracted to {}", entry.name(), PathFmt(&file_path)); + info!( + "File {} extracted to {}", + SanitizedStr(entry.name()), + PathFmt(&file_path) + ); if !path.fs_err_try_exists()? { fs::create_dir_all(path)?; } @@ -47,7 +65,8 @@ where let file = fs::File::create(path)?; let mut writer = BufWriter::new(file); - io::copy(reader, &mut writer)?; + let mut limited = LimitedReader::new(reader, max_decompressed_bytes()); + io::copy(&mut limited, &mut writer)?; use filetime_creation as ft; // Surface mtime-set failures as warnings so users know timestamps weren't preserved @@ -136,6 +155,7 @@ where for filename in files { let previous_location = cd_into_same_dir_as(filename)?; + let _cwd_guard = crate::utils::CwdGuard::new(previous_location); // Unwrap safety: // paths should be canonicalized by now, and the root directory rejected. @@ -172,8 +192,6 @@ where writer.push_archive_entry::(entry, entry_data)?; } - - env::set_current_dir(previous_location)?; } let bytes = writer.finish()?; diff --git a/src/archive/tar.rs b/src/archive/tar.rs index 843cd3cf7..fbd8dfa4e 100644 --- a/src/archive/tar.rs +++ b/src/archive/tar.rs @@ -4,7 +4,6 @@ use std::os::unix::fs::MetadataExt; use std::{ collections::HashMap, - env, io::{self, prelude::*}, ops::Not, path::{Path, PathBuf}, @@ -20,7 +19,8 @@ use crate::{ list::{FileInArchive, ListFileType}, utils::{ self, BytesFmt, FileType, FileVisibilityPolicy, PathFmt, canonicalize, create_symlink, is_same_file_as_output, - read_file_type, set_permission_mode, + read_file_type, sanitize_archive_mode, set_permission_mode, validate_dest_inside_root, validate_entry_path, + validate_symlink_target, }, warning, }; @@ -38,23 +38,31 @@ pub fn unpack_archive(reader: impl Read, output_folder: &Path) -> Result { match entry.header().entry_type() { tar::EntryType::Symlink => { - let relative_path = entry.path()?; - let full_path = output_folder.join(&relative_path); + let raw_path = entry.path()?.into_owned(); + let safe_relpath = validate_entry_path(&raw_path)?; + let full_path = output_folder.join(&safe_relpath); let target = entry .link_name()? .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidData, "Missing symlink target"))?; + validate_symlink_target(&safe_relpath, &target)?; + validate_dest_inside_root(output_folder, &full_path)?; create_symlink(&target, &full_path)?; } tar::EntryType::Link => { - let link_path = entry.path()?; - let target = entry + let raw_link = entry.path()?.into_owned(); + let safe_link_path = validate_entry_path(&raw_link)?; + let raw_target = entry .link_name()? - .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidData, "Missing hardlink target"))?; + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidData, "Missing hardlink target"))? + .into_owned(); + let safe_target = validate_entry_path(&raw_target)?; - let full_link_path = output_folder.join(&link_path); - let full_target_path = output_folder.join(&target); + let full_link_path = output_folder.join(&safe_link_path); + let full_target_path = output_folder.join(&safe_target); + validate_dest_inside_root(output_folder, &full_link_path)?; + validate_dest_inside_root(output_folder, &full_target_path)?; fs::hard_link(&full_target_path, &full_link_path)?; } tar::EntryType::Regular | tar::EntryType::GNUSparse => { @@ -71,10 +79,11 @@ pub fn unpack_archive(reader: impl Read, output_folder: &Path) -> Result { // We unpacked a read-only directory, make it writeable so that we can // create the files inside of it, by the end, restore the original mode let original_path = entry.path()?.to_path_buf(); - let unpacked = output_folder.join(&original_path); - set_permission_mode(&unpacked, original_mode | 0o200)?; + let safe_relpath = validate_entry_path(&original_path)?; + let unpacked = output_folder.join(&safe_relpath); + set_permission_mode(&unpacked, sanitize_archive_mode(original_mode) | 0o200)?; - read_only_dirs_and_modes.push((original_path, original_mode)); + read_only_dirs_and_modes.push((original_path, sanitize_archive_mode(original_mode))); } } _ => continue, @@ -146,6 +155,7 @@ where for explicit_path in explicit_paths { let previous_location = utils::cd_into_same_dir_as(explicit_path)?; + let _cwd_guard = utils::CwdGuard::new(previous_location); // Unwrap expectation: // paths should be canonicalized by now, and the root directory rejected. @@ -231,7 +241,6 @@ where } } } - env::set_current_dir(previous_location)?; } Ok(builder.into_inner()?) diff --git a/src/archive/zip.rs b/src/archive/zip.rs index eae9d327a..ca91f3f00 100644 --- a/src/archive/zip.rs +++ b/src/archive/zip.rs @@ -3,7 +3,6 @@ #[cfg(unix)] use std::os::unix::fs::PermissionsExt; use std::{ - env, io::{self, prelude::*}, path::{Path, PathBuf}, }; @@ -15,15 +14,18 @@ use same_file::Handle; use time::{OffsetDateTime, PrimitiveDateTime}; use zip::{self, DateTime, ZipArchive, read::ZipFile}; +#[cfg(unix)] +use crate::utils::sanitize_archive_mode; use crate::{ Result, error::FinalError, info, info_accessible, list::{FileInArchive, ListFileType}, utils::{ - BytesFmt, FileType, FileVisibilityPolicy, PathFmt, canonicalize, cd_into_same_dir_as, create_symlink, - ensure_parent_dir_exists, get_invalid_utf8_paths, is_same_file_as_output, pretty_format_list_of_paths, - read_file_type, strip_cur_dir, + BytesFmt, FileType, FileVisibilityPolicy, LimitedReader, PathFmt, SanitizedStr, canonicalize, + cd_into_same_dir_as, create_symlink, ensure_parent_dir_exists, get_invalid_utf8_paths, is_same_file_as_output, + max_decompressed_bytes, pretty_format_list_of_paths, read_file_type, strip_cur_dir, validate_dest_inside_root, + validate_symlink_target, }, warning, }; @@ -42,12 +44,17 @@ where Some(password) => archive.by_index_decrypt(idx, password)?, None => archive.by_index(idx)?, }; - let file_path = match file.enclosed_name() { + let relpath = match file.enclosed_name() { Some(path) => path.to_owned(), - None => continue, + None => { + warning!("skipping entry {} with unsafe name: {}", idx, SanitizedStr(file.name())); + continue; + } }; - let file_path = output_folder.join(file_path); + let file_path = output_folder.join(&relpath); + + validate_dest_inside_root(output_folder, &file_path)?; display_zip_comment_if_exists(&file); @@ -62,6 +69,7 @@ where let mut target = String::new(); file.read_to_string(&mut target)?; + validate_symlink_target(&relpath, std::path::Path::new(&target))?; #[cfg(unix)] std::os::unix::fs::symlink(&target, &file_path)?; #[cfg(windows)] @@ -81,12 +89,28 @@ where let mut target = String::new(); file.read_to_string(&mut target)?; - info!("linking {} -> \"{}\"", PathFmt(file_path), target); + validate_symlink_target(&relpath, std::path::Path::new(&target))?; + info!("linking {} -> \"{}\"", PathFmt(file_path), SanitizedStr(&target)); create_symlink(Path::new(&target), file_path)?; } else { + #[cfg(unix)] + let mut output_file = { + use fs_err::os::unix::fs::OpenOptionsExt; + let mode = file.unix_mode().map(sanitize_archive_mode).unwrap_or(0o644); + fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(mode) + .open(file_path)? + }; + #[cfg(not(unix))] let mut output_file = fs::File::create(file_path)?; - io::copy(&mut file, &mut output_file)?; + { + let mut limited = LimitedReader::new(&mut file, max_decompressed_bytes()); + io::copy(&mut limited, &mut output_file)?; + } set_last_modified_time(&file, file_path)?; #[cfg(unix)] unix_set_permissions(file_path, &file)?; @@ -182,6 +206,7 @@ where for explicit_path in input_filenames { let previous_location = cd_into_same_dir_as(explicit_path)?; + let _cwd_guard = crate::utils::CwdGuard::new(previous_location); // Unwrap safety: // paths should be canonicalized by now, and the root directory rejected. @@ -255,8 +280,6 @@ where } } } - - env::set_current_dir(previous_location)?; } let bytes = writer.finish()?; @@ -276,7 +299,11 @@ fn display_zip_comment_if_exists(file: &ZipFile<'_, R>) { // the future, maybe asking the user if he wants to display the comment // (informing him of its size) would be sensible for both normal and // accessibility mode.. - info_accessible!("Found comment in {}: {}", file.name(), comment); + info_accessible!( + "Found comment in {}: {}", + SanitizedStr(file.name()), + SanitizedStr(comment) + ); } } @@ -315,7 +342,7 @@ fn unix_set_permissions(file_path: &Path, file: &ZipFile<'_, R>) -> Res use std::fs::Permissions; if let Some(mode) = file.unix_mode() { - fs::set_permissions(file_path, Permissions::from_mode(mode))?; + fs::set_permissions(file_path, Permissions::from_mode(sanitize_archive_mode(mode)))?; } Ok(()) diff --git a/src/cli/args.rs b/src/cli/args.rs index 0da80e4f8..5d9973472 100644 --- a/src/cli/args.rs +++ b/src/cli/args.rs @@ -43,7 +43,7 @@ pub struct CliArgs { pub format: Option, /// Decompress or list with password - #[arg(short, long = "password", aliases = ["pass", "pw"], global = true)] + #[arg(short, long = "password", aliases = ["pass", "pw"], env = "OUCH_PASSWORD", global = true)] pub password: Option, /// Concurrent working threads diff --git a/src/commands/decompress.rs b/src/commands/decompress.rs index a2ba2eba2..f17aaa6c7 100644 --- a/src/commands/decompress.rs +++ b/src/commands/decompress.rs @@ -16,9 +16,9 @@ use crate::{ info, info_accessible, non_archive::lz4::MultiFrameLz4Decoder, utils::{ - self, BytesFmt, PathFmt, file_size, + self, BytesFmt, LZMA_MEMLIMIT_BYTES, LimitedReader, PathFmt, file_size, io::{ReadSeek, lock_and_flush_output_stdio}, - is_path_stdin, resolve_path_conflict, user_wants_to_continue, + is_path_stdin, max_decompressed_bytes, resolve_path_conflict, user_wants_to_continue, }, }; @@ -65,7 +65,11 @@ pub fn decompress_file(options: DecompressOptions) -> Result<()> { Box::new(bzip3::read::Bz3Decoder::new(decoder)?) } Lz4 => Box::new(MultiFrameLz4Decoder::new(decoder)), - Lzma => Box::new(lzma_rust2::LzmaReader::new_mem_limit(decoder, u32::MAX, None)?), + Lzma => Box::new(lzma_rust2::LzmaReader::new_mem_limit( + decoder, + LZMA_MEMLIMIT_BYTES, + None, + )?), Xz => Box::new(lzma_rust2::XzReader::new(decoder, true)), Lzip => Box::new(lzma_rust2::LzipReader::new(decoder)), Snappy => Box::new(snap::read::FrameDecoder::new(decoder)), @@ -108,7 +112,9 @@ pub fn decompress_file(options: DecompressOptions) -> Result<()> { let control_flow = match first_extension { Gzip | Bzip | Bzip3 | Lz4 | Lzma | Xz | Lzip | Snappy | Zstd | Brotli => { let reader = create_decoder_up_to_first_extension()?; - let mut reader = chain_reader_decoder(&first_extension, reader)?; + let reader = chain_reader_decoder(&first_extension, reader)?; + // Bomb cap: abort if decompressed output exceeds OUCH_MAX_DECOMPRESSED_BYTES + let mut reader = LimitedReader::new(reader, max_decompressed_bytes()); let (mut writer, final_output_path) = match utils::create_file_or_prompt_on_conflict( &options.output_file_path, @@ -125,7 +131,10 @@ pub fn decompress_file(options: DecompressOptions) -> Result<()> { }) } Tar => unpack_archive( - |output_dir| crate::archive::tar::unpack_archive(create_decoder_up_to_first_extension()?, output_dir), + |output_dir| { + let reader = LimitedReader::new(create_decoder_up_to_first_extension()?, max_decompressed_bytes()); + crate::archive::tar::unpack_archive(reader, output_dir) + }, archive_output_dir, options.question_policy, )?, @@ -160,7 +169,9 @@ pub fn decompress_file(options: DecompressOptions) -> Result<()> { drop(locks); let mut vec = vec![]; - io::copy(&mut create_decoder_up_to_first_extension()?, &mut vec)?; + // Bomb cap: abort if the in-memory decompressed image exceeds the limit + let mut reader = LimitedReader::new(create_decoder_up_to_first_extension()?, max_decompressed_bytes()); + io::copy(&mut reader, &mut vec)?; Box::new(io::Cursor::new(vec)) } else { Box::new(BufReader::with_capacity( @@ -179,7 +190,8 @@ pub fn decompress_file(options: DecompressOptions) -> Result<()> { Rar => { let unpack_fn: Box Result> = if options.formats.len() > 1 || input_is_stdin { let mut temp_file = tempfile::NamedTempFile::new()?; - io::copy(&mut create_decoder_up_to_first_extension()?, &mut temp_file)?; + let mut limited = LimitedReader::new(create_decoder_up_to_first_extension()?, max_decompressed_bytes()); + io::copy(&mut limited, &mut temp_file)?; Box::new(move |output_dir| { crate::archive::rar::unpack_archive(temp_file.path(), output_dir, options.password) }) diff --git a/src/commands/list.rs b/src/commands/list.rs index 78c9a79fe..f61643e45 100644 --- a/src/commands/list.rs +++ b/src/commands/list.rs @@ -11,7 +11,10 @@ use crate::{ extension::CompressionFormat::{self, *}, list::{self, FileInArchive, ListOptions}, non_archive::lz4::MultiFrameLz4Decoder, - utils::{io::lock_and_flush_output_stdio, user_wants_to_continue}, + utils::{ + LZMA_MEMLIMIT_BYTES, LimitedReader, io::lock_and_flush_output_stdio, max_decompressed_bytes, + user_wants_to_continue, + }, }; /// File at archive_path is opened for reading, example: "archive.tar.gz" @@ -57,7 +60,11 @@ pub fn list_archive_contents( Box::new(bzip3::read::Bz3Decoder::new(decoder)?) } Lz4 => Box::new(MultiFrameLz4Decoder::new(decoder)), - Lzma => Box::new(lzma_rust2::LzmaReader::new_mem_limit(decoder, u32::MAX, None)?), + Lzma => Box::new(lzma_rust2::LzmaReader::new_mem_limit( + decoder, + LZMA_MEMLIMIT_BYTES, + None, + )?), Xz => Box::new(lzma_rust2::XzReader::new(decoder, true)), Lzip => Box::new(lzma_rust2::LzipReader::new(decoder)), Snappy => Box::new(snap::read::FrameDecoder::new(decoder)), @@ -79,7 +86,10 @@ pub fn list_archive_contents( let archive_format = misplaced_archive_format.unwrap_or(formats[0]); let files: Box>> = match archive_format { - Tar => Box::new(crate::archive::tar::list_archive(tar::Archive::new(reader))?), + Tar => { + let limited = LimitedReader::new(reader, max_decompressed_bytes()); + Box::new(crate::archive::tar::list_archive(tar::Archive::new(limited))?) + } Zip => { if formats.len() > 1 { // Make thread own locks to keep output messages adjacent @@ -92,7 +102,9 @@ pub fn list_archive_contents( } let mut vec = vec![]; - io::copy(&mut reader, &mut vec)?; + // Bomb cap: abort if the in-memory decompressed image exceeds the limit + let mut limited = LimitedReader::new(&mut reader, max_decompressed_bytes()); + io::copy(&mut limited, &mut vec)?; let zip_archive = zip::ZipArchive::new(io::Cursor::new(vec))?; Box::new(crate::archive::zip::list_archive(zip_archive, password)) @@ -101,7 +113,9 @@ pub fn list_archive_contents( Rar => { if formats.len() > 1 { let mut temp_file = tempfile::NamedTempFile::new()?; - io::copy(&mut reader, &mut temp_file)?; + // Bomb cap: abort if decompressed output to tempfile exceeds the limit + let mut limited = LimitedReader::new(&mut reader, max_decompressed_bytes()); + io::copy(&mut limited, &mut temp_file)?; Box::new(crate::archive::rar::list_archive(temp_file.path(), password)?) } else { Box::new(crate::archive::rar::list_archive(archive_path, password)?) @@ -122,7 +136,9 @@ pub fn list_archive_contents( drop(locks); let mut vec = vec![]; - io::copy(&mut reader, &mut vec)?; + // Bomb cap: abort if the in-memory decompressed image exceeds the limit + let mut limited = LimitedReader::new(&mut reader, max_decompressed_bytes()); + io::copy(&mut limited, &mut vec)?; Box::new(archive::sevenz::list_archive(io::Cursor::new(vec), password)?) } else { diff --git a/src/list.rs b/src/list.rs index fb7fe5ada..c2376cae0 100644 --- a/src/list.rs +++ b/src/list.rs @@ -7,7 +7,11 @@ use std::{ }; use self::tree::Tree; -use crate::{Result, accessible::is_running_in_accessible_mode, utils::PathFmt}; +use crate::{ + Result, + accessible::is_running_in_accessible_mode, + utils::{NoQuotePathFmt, PathFmt}, +}; /// Options controlling how archive contents should be listed #[derive(Debug, Clone, Copy)] @@ -56,7 +60,7 @@ pub fn list_files( } else { for file in files { let FileInArchive { path, file_type } = file?; - print_entry(&mut out, path.display(), &file_type, list_options.quiet); + print_entry(&mut out, NoQuotePathFmt(&path), &file_type, list_options.quiet); } } Ok(()) @@ -86,14 +90,14 @@ fn print_entry(out: &mut impl Write, name: impl fmt::Display, file_type: &ListFi }; if is_running_in_accessible_mode() { - let _ = writeln!(out, "{name} -> {}{suffix}", target.display()); + let _ = writeln!(out, "{name} -> {}{suffix}", NoQuotePathFmt(target)); } else { let _ = writeln!( out, "{c}{name}{r} {c}-> {c}{target}{suffix}{r}", c = *CYAN, r = *ALL_RESET, - target = target.display() + target = NoQuotePathFmt(target) ); } } @@ -125,11 +129,10 @@ mod tree { path, }; - use bstr::{ByteSlice, ByteVec}; use indexmap::IndexMap; use super::{FileInArchive, ListFileType}; - use crate::{utils::NoQuotePathFmt, warning}; + use crate::warning; /// Directory tree #[derive(Debug, Default)] @@ -165,7 +168,7 @@ mod tree { Some(file) => { warning!( "multiple files with the same name in a single directory ({})", - NoQuotePathFmt(&file.path), + super::NoQuotePathFmt(&file.path), ); } } @@ -195,7 +198,7 @@ mod tree { }; super::print_entry( out, - as ByteVec>::from_os_str_lossy(name).as_bstr(), + super::NoQuotePathFmt(path::Path::new(name)), &file_type, false, // Always show targets in tree view, regardless of --quiet flag ); diff --git a/src/utils/formatting.rs b/src/utils/formatting.rs index 19461e7cb..c65a98af2 100644 --- a/src/utils/formatting.rs +++ b/src/utils/formatting.rs @@ -99,6 +99,30 @@ pub fn append_ascii_suffix_to_os_str(os_str: &OsStr, ascii_suffix: &str) -> OsSt pub struct PathFmt<'a>(pub &'a Path); pub struct NoQuotePathFmt<'a>(pub &'a Path); +/// Returns true for bytes/chars that must be stripped before hitting a terminal +/// Covers C0 controls, DEL, C1 controls (U+0080..U+009F), line/paragraph separators, +/// BiDi formatting characters (Trojan Source, CVE-2021-42574), and zero-width +/// / invisible characters used for homoglyph or hidden-content attacks +fn is_unsafe_display_char(ch: char) -> bool { + match ch { + c if c.is_control() => true, + '\u{2028}' | '\u{2029}' => true, + '\u{202A}'..='\u{202E}' => true, + '\u{2066}'..='\u{2069}' => true, + '\u{200B}' | '\u{200C}' | '\u{200D}' | '\u{FEFF}' | '\u{00AD}' | '\u{180E}' | '\u{034F}' | '\u{061C}' => true, + _ => false, + } +} + +/// Write a char to a formatter, replacing unsafe chars with U+FFFD +fn write_sanitized_char(ch: char, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if is_unsafe_display_char(ch) { + f.write_char('\u{FFFD}') + } else { + f.write_char(ch) + } +} + /// Same as NoQuotePathFmt, but surrounded by "". impl<'a> fmt::Display for PathFmt<'a> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -106,7 +130,21 @@ impl<'a> fmt::Display for PathFmt<'a> { } } +/// Wrapper that writes a string to a formatter, replacing unsafe chars +/// Use for any archive-derived string that doesn't flow through PathFmt +pub struct SanitizedStr<'a>(pub &'a str); + +impl<'a> fmt::Display for SanitizedStr<'a> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + for ch in self.0.chars() { + write_sanitized_char(ch, f)?; + } + Ok(()) + } +} + /// Same as `path.display()` but try to strip some common noise prefixes +/// Also strips control bytes and BiDi/zero-width chars to prevent terminal injection impl<'a> fmt::Display for NoQuotePathFmt<'a> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let path = self.0; @@ -122,7 +160,10 @@ impl<'a> fmt::Display for NoQuotePathFmt<'a> { path }; - write!(f, "{}", path.display()) + for ch in path.display().to_string().chars() { + write_sanitized_char(ch, f)?; + } + Ok(()) } } diff --git a/src/utils/fs.rs b/src/utils/fs.rs index b9df5e219..b057ce5af 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -118,6 +118,135 @@ pub fn create_dir_if_non_existent(path: &Path) -> Result<()> { } /// Ensures the parent directory of a file path exists, creating it if necessary. +/// Lexically normalize a relative path; returns None on absolute or escape via `..`. +pub fn normalize_safe_path(path: &Path) -> Option { + let mut out = PathBuf::new(); + for comp in path.components() { + match comp { + std::path::Component::Normal(c) => out.push(c), + std::path::Component::CurDir => {} + std::path::Component::ParentDir => { + if !out.pop() { + return None; + } + } + std::path::Component::Prefix(_) | std::path::Component::RootDir => return None, + } + } + Some(out) +} + +/// Reject ZipSlip-style entry paths; returns the lexically-normalized safe form. +pub fn validate_entry_path(path: &Path) -> Result { + normalize_safe_path(path).ok_or_else(|| { + FinalError::with_title("refusing to extract archive entry with unsafe path") + .detail(format!("entry: {}", PathFmt(path))) + .into() + }) +} + +/// Reject symlink targets whose relative path would escape the extraction root. +pub fn validate_symlink_target(link_relpath: &Path, target: &Path) -> Result<()> { + if target.is_absolute() { + return Ok(()); + } + let parent = link_relpath.parent().unwrap_or(Path::new("")); + if normalize_safe_path(&parent.join(target)).is_none() { + return Err( + FinalError::with_title("refusing to create symlink escaping extraction root") + .detail(format!("link: {} target: {}", PathFmt(link_relpath), PathFmt(target))) + .into(), + ); + } + Ok(()) +} + +/// Refuse to write through an on-disk symlink created by an earlier entry. +/// Walks every existing prefix between root and dest and errors if any component is a symlink. +pub fn validate_dest_inside_root(root: &Path, dest: &Path) -> Result<()> { + let rel = dest.strip_prefix(root).map_err(|_| { + FinalError::with_title("refusing to write outside extraction root").detail(format!("dest: {}", PathFmt(dest))) + })?; + let mut probe = root.to_path_buf(); + for comp in rel.components() { + probe.push(comp); + match fs::symlink_metadata(&probe) { + Ok(md) if md.file_type().is_symlink() => { + return Err( + FinalError::with_title("refusing to traverse on-disk symlink during extraction") + .detail(format!("path: {}", PathFmt(&probe))) + .into(), + ); + } + _ => {} + } + } + Ok(()) +} + +pub const DEFAULT_MAX_DECOMPRESSED_BYTES: u64 = 32 * 1024 * 1024 * 1024; + +/// LZMA/XZ dictionary memory cap (256 MiB). Bounds malformed-stream allocations +pub const LZMA_MEMLIMIT_BYTES: u32 = 256 * 1024 * 1024; + +pub fn max_decompressed_bytes() -> u64 { + env::var("OUCH_MAX_DECOMPRESSED_BYTES") + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(DEFAULT_MAX_DECOMPRESSED_BYTES) +} + +pub struct LimitedReader { + inner: R, + remaining: u64, +} + +impl LimitedReader { + pub fn new(inner: R, limit: u64) -> Self { + Self { + inner, + remaining: limit, + } + } +} + +impl Read for LimitedReader { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + if self.remaining == 0 { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "decompression output exceeded configured limit (see OUCH_MAX_DECOMPRESSED_BYTES)", + )); + } + let cap = usize::try_from(self.remaining).unwrap_or(usize::MAX).min(buf.len()); + let n = self.inner.read(&mut buf[..cap])?; + self.remaining = self.remaining.saturating_sub(n as u64); + Ok(n) + } +} + +/// Strip setuid/setgid/sticky bits from an archive-supplied mode. +pub fn sanitize_archive_mode(mode: u32) -> u32 { + mode & 0o0777 +} + +/// RAII guard that restores the process CWD on drop. +pub struct CwdGuard(Option); + +impl CwdGuard { + pub fn new(previous: PathBuf) -> Self { + Self(Some(previous)) + } +} + +impl Drop for CwdGuard { + fn drop(&mut self) { + if let Some(p) = self.0.take() { + let _ = env::set_current_dir(&p); + } + } +} + pub fn ensure_parent_dir_exists(file_path: &Path) -> io::Result<()> { if let Some(parent) = file_path.parent() && !parent.fs_err_try_exists()? diff --git a/src/utils/io.rs b/src/utils/io.rs index 73028aff7..59fb20a94 100644 --- a/src/utils/io.rs +++ b/src/utils/io.rs @@ -1,5 +1,6 @@ use std::io::{self, StderrLock, StdoutLock, Write, stderr, stdout}; +#[cfg(unix)] use fs_err as fs; use crate::utils::logger; diff --git a/src/utils/logger.rs b/src/utils/logger.rs index dc5aec643..5d4d095f3 100644 --- a/src/utils/logger.rs +++ b/src/utils/logger.rs @@ -279,9 +279,8 @@ mod logger_thread { fn flush_logs_to_stderr(buffer: &mut Vec) { if !buffer.is_empty() { - if let Err(err) = stderr().write_all(buffer) { - panic!("Failed to write to STDERR: {err}"); - } + // Ignore stderr write failures (broken pipe, closed terminal) instead of panicking + let _ = stderr().write_all(buffer); buffer.clear(); } } diff --git a/tests/snapshots/ui__ui_test_usage_help_flag-2.snap b/tests/snapshots/ui__ui_test_usage_help_flag-2.snap index af5c50c0b..200e4da01 100644 --- a/tests/snapshots/ui__ui_test_usage_help_flag-2.snap +++ b/tests/snapshots/ui__ui_test_usage_help_flag-2.snap @@ -1,5 +1,6 @@ --- source: tests/ui.rs +assertion_line: 192 expression: "output_to_string(ouch!(\"-h\"))" --- A command-line utility for easily compressing and decompressing files and directories. @@ -20,7 +21,7 @@ Options: -q, --quiet Silence output -g, --gitignore Ignore files matched by git's ignore files -f, --format Specify the format of the archive - -p, --password Decompress or list with password + -p, --password Decompress or list with password [env: OUCH_PASSWORD=] -c, --threads Concurrent working threads -h, --help Print help (see more with '--help') -V, --version Print version diff --git a/tests/snapshots/ui__ui_test_usage_help_flag.snap b/tests/snapshots/ui__ui_test_usage_help_flag.snap index 7b0572a13..78b62fcf7 100644 --- a/tests/snapshots/ui__ui_test_usage_help_flag.snap +++ b/tests/snapshots/ui__ui_test_usage_help_flag.snap @@ -42,6 +42,8 @@ Options: -p, --password Decompress or list with password + + [env: OUCH_PASSWORD=] -c, --threads Concurrent working threads From e88c6c9b0ec609877786e8ad1baef7af5c43fb9a Mon Sep 17 00:00:00 2001 From: curious-rabbit Date: Wed, 20 May 2026 23:52:56 +0200 Subject: [PATCH 2/6] remove limiter --- src/utils/fs.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/utils/fs.rs b/src/utils/fs.rs index b057ce5af..b970ca9e5 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -184,7 +184,13 @@ pub fn validate_dest_inside_root(root: &Path, dest: &Path) -> Result<()> { Ok(()) } -pub const DEFAULT_MAX_DECOMPRESSED_BYTES: u64 = 32 * 1024 * 1024 * 1024; +// TODO: pick a sensible default cap. No mainstream extractor (GNU tar, +// bsdtar, unzip, 7z, gzip, xz, zstd) imposes one; 32 GiB broke legitimate +// large archives (VM images, database dumps, ML datasets, backups). Until +// we have a value that's bomb-resistant without breaking real use, the +// cap is opt-in via OUCH_MAX_DECOMPRESSED_BYTES. +// pub const DEFAULT_MAX_DECOMPRESSED_BYTES: u64 = 32 * 1024 * 1024 * 1024; +pub const DEFAULT_MAX_DECOMPRESSED_BYTES: u64 = u64::MAX; /// LZMA/XZ dictionary memory cap (256 MiB). Bounds malformed-stream allocations pub const LZMA_MEMLIMIT_BYTES: u32 = 256 * 1024 * 1024; From fa77efd845f87531c962f262b09b968a6437c32b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Marcos?= Date: Tue, 26 May 2026 20:15:18 -0300 Subject: [PATCH 3/6] style tweaks --- src/utils/fs.rs | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/utils/fs.rs b/src/utils/fs.rs index b970ca9e5..e0b91d1b1 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -184,14 +184,6 @@ pub fn validate_dest_inside_root(root: &Path, dest: &Path) -> Result<()> { Ok(()) } -// TODO: pick a sensible default cap. No mainstream extractor (GNU tar, -// bsdtar, unzip, 7z, gzip, xz, zstd) imposes one; 32 GiB broke legitimate -// large archives (VM images, database dumps, ML datasets, backups). Until -// we have a value that's bomb-resistant without breaking real use, the -// cap is opt-in via OUCH_MAX_DECOMPRESSED_BYTES. -// pub const DEFAULT_MAX_DECOMPRESSED_BYTES: u64 = 32 * 1024 * 1024 * 1024; -pub const DEFAULT_MAX_DECOMPRESSED_BYTES: u64 = u64::MAX; - /// LZMA/XZ dictionary memory cap (256 MiB). Bounds malformed-stream allocations pub const LZMA_MEMLIMIT_BYTES: u32 = 256 * 1024 * 1024; @@ -199,7 +191,7 @@ pub fn max_decompressed_bytes() -> u64 { env::var("OUCH_MAX_DECOMPRESSED_BYTES") .ok() .and_then(|v| v.parse().ok()) - .unwrap_or(DEFAULT_MAX_DECOMPRESSED_BYTES) + .unwrap_or(u64::MAX) } pub struct LimitedReader { @@ -218,16 +210,22 @@ impl LimitedReader { impl Read for LimitedReader { fn read(&mut self, buf: &mut [u8]) -> io::Result { + if buf.is_empty() { + return Ok(0); + } + if self.remaining == 0 { return Err(io::Error::new( io::ErrorKind::InvalidData, "decompression output exceeded configured limit (see OUCH_MAX_DECOMPRESSED_BYTES)", )); } - let cap = usize::try_from(self.remaining).unwrap_or(usize::MAX).min(buf.len()); - let n = self.inner.read(&mut buf[..cap])?; - self.remaining = self.remaining.saturating_sub(n as u64); - Ok(n) + + let bytes_to_ready = usize::try_from(self.remaining).unwrap_or(usize::MAX).min(buf.len()); + let bytes_read = self.inner.read(&mut buf[..bytes_to_ready])?; + + self.remaining = self.remaining.saturating_sub(bytes_read as u64); + Ok(bytes_read) } } From 26b20c840396b83dfa828a08cb094b5707c7ce00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Marcos?= Date: Tue, 26 May 2026 20:26:31 -0300 Subject: [PATCH 4/6] remove SanitizedStr, move checks to logger thread --- src/archive/sevenz.rs | 13 ++++--------- src/archive/zip.rs | 18 +++++++---------- src/error.rs | 32 +++++++++++++++++++++++++----- src/list.rs | 2 +- src/utils/formatting.rs | 43 ++++++++++++++++++----------------------- src/utils/logger.rs | 20 +++++++++++++++++-- tests/integration.rs | 4 ++-- 7 files changed, 78 insertions(+), 54 deletions(-) diff --git a/src/archive/sevenz.rs b/src/archive/sevenz.rs index 593d60b95..f25c61d90 100644 --- a/src/archive/sevenz.rs +++ b/src/archive/sevenz.rs @@ -17,9 +17,8 @@ use crate::{ info, list::{FileInArchive, ListFileType}, utils::{ - BytesFmt, FileVisibilityPolicy, LimitedReader, PathFmt, SanitizedStr, cd_into_same_dir_as, - ensure_parent_dir_exists, is_same_file_as_output, max_decompressed_bytes, validate_dest_inside_root, - validate_entry_path, + BytesFmt, FileVisibilityPolicy, LimitedReader, PathFmt, cd_into_same_dir_as, ensure_parent_dir_exists, + is_same_file_as_output, max_decompressed_bytes, validate_dest_inside_root, validate_entry_path, }, warning, }; @@ -34,7 +33,7 @@ where |entry: &ArchiveEntry, reader: &mut dyn Read, path: &PathBuf| -> Result { // Manually handle writing all files from 7z archive (the library defaults ignore empty files) - let name_as_path = std::path::Path::new(entry.name()); + let name_as_path = Path::new(entry.name()); let safe_relpath = match validate_entry_path(name_as_path) { Ok(p) => p, Err(e) => { @@ -50,11 +49,7 @@ where } if entry.is_directory() { - info!( - "File {} extracted to {}", - SanitizedStr(entry.name()), - PathFmt(&file_path) - ); + info!("File {} extracted to {}", entry.name(), PathFmt(&file_path)); if !path.fs_err_try_exists()? { fs::create_dir_all(path)?; } diff --git a/src/archive/zip.rs b/src/archive/zip.rs index ca91f3f00..d8f09686e 100644 --- a/src/archive/zip.rs +++ b/src/archive/zip.rs @@ -22,8 +22,8 @@ use crate::{ info, info_accessible, list::{FileInArchive, ListFileType}, utils::{ - BytesFmt, FileType, FileVisibilityPolicy, LimitedReader, PathFmt, SanitizedStr, canonicalize, - cd_into_same_dir_as, create_symlink, ensure_parent_dir_exists, get_invalid_utf8_paths, is_same_file_as_output, + BytesFmt, FileType, FileVisibilityPolicy, LimitedReader, PathFmt, canonicalize, cd_into_same_dir_as, + create_symlink, ensure_parent_dir_exists, get_invalid_utf8_paths, is_same_file_as_output, max_decompressed_bytes, pretty_format_list_of_paths, read_file_type, strip_cur_dir, validate_dest_inside_root, validate_symlink_target, }, @@ -47,7 +47,7 @@ where let relpath = match file.enclosed_name() { Some(path) => path.to_owned(), None => { - warning!("skipping entry {} with unsafe name: {}", idx, SanitizedStr(file.name())); + warning!("skipping entry {} with unsafe name: {}", idx, file.name()); continue; } }; @@ -69,7 +69,7 @@ where let mut target = String::new(); file.read_to_string(&mut target)?; - validate_symlink_target(&relpath, std::path::Path::new(&target))?; + validate_symlink_target(&relpath, Path::new(&target))?; #[cfg(unix)] std::os::unix::fs::symlink(&target, &file_path)?; #[cfg(windows)] @@ -89,8 +89,8 @@ where let mut target = String::new(); file.read_to_string(&mut target)?; - validate_symlink_target(&relpath, std::path::Path::new(&target))?; - info!("linking {} -> \"{}\"", PathFmt(file_path), SanitizedStr(&target)); + validate_symlink_target(&relpath, Path::new(&target))?; + info!("linking {} -> \"{}\"", PathFmt(file_path), target); create_symlink(Path::new(&target), file_path)?; } else { @@ -299,11 +299,7 @@ fn display_zip_comment_if_exists(file: &ZipFile<'_, R>) { // the future, maybe asking the user if he wants to display the comment // (informing him of its size) would be sensible for both normal and // accessibility mode.. - info_accessible!( - "Found comment in {}: {}", - SanitizedStr(file.name()), - SanitizedStr(comment) - ); + info_accessible!("Found comment in {}: {}", file.name(), comment); } } diff --git a/src/error.rs b/src/error.rs index 29e96dd9a..c1757dbc2 100644 --- a/src/error.rs +++ b/src/error.rs @@ -11,6 +11,7 @@ use std::{ use crate::{ accessible::is_running_in_accessible_mode, extension::{PRETTY_SUPPORTED_ALIASES, PRETTY_SUPPORTED_EXTENSIONS}, + utils::sanitize_for_display, }; /// All errors that can be generated by `ouch` @@ -88,14 +89,14 @@ impl Display for FinalError { // // When in ACCESSIBLE mode, the square brackets are suppressed if is_running_in_accessible_mode() { - write!(f, "{}ERROR{}: {}", *RED, *RESET, self.title)?; + write!(f, "{}ERROR{}: {}", *RED, *RESET, sanitize_for_display(&self.title))?; } else { - write!(f, "{}[ERROR]{} {}", *RED, *RESET, self.title)?; + write!(f, "{}[ERROR]{} {}", *RED, *RESET, sanitize_for_display(&self.title))?; } // Details for detail in &self.details { - write!(f, "\n - {}{}{}", *YELLOW, detail, *RESET)?; + write!(f, "\n - {}{}{}", *YELLOW, sanitize_for_display(detail), *RESET)?; } // Hints @@ -107,11 +108,11 @@ impl Display for FinalError { if is_running_in_accessible_mode() { write!(f, "\n{}hints:{}", *GREEN, *RESET)?; for hint in &self.hints { - write!(f, "\n{hint}")?; + write!(f, "\n{}", sanitize_for_display(hint))?; } } else { for hint in &self.hints { - write!(f, "\n{}hint:{} {}", *GREEN, *RESET, hint)?; + write!(f, "\n{}hint:{} {}", *GREEN, *RESET, sanitize_for_display(hint))?; } } } @@ -275,3 +276,24 @@ impl From for Error { Self::Custom { reason: err } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn final_error_display_sanitizes_untrusted_text() { + let err = FinalError::with_title("bad\u{1b}]title") + .detail("bad\u{202e}detail") + .hint("bad\u{200b}hint"); + + let rendered = err.to_string(); + + assert!(!rendered.contains("\u{1b}]")); + assert!(!rendered.contains('\u{202e}')); + assert!(!rendered.contains('\u{200b}')); + assert!(rendered.contains("bad�]title")); + assert!(rendered.contains("bad�detail")); + assert!(rendered.contains("bad�hint")); + } +} diff --git a/src/list.rs b/src/list.rs index c2376cae0..b42c215e6 100644 --- a/src/list.rs +++ b/src/list.rs @@ -198,7 +198,7 @@ mod tree { }; super::print_entry( out, - super::NoQuotePathFmt(path::Path::new(name)), + super::NoQuotePathFmt(name.as_ref()), &file_type, false, // Always show targets in tree view, regardless of --quiet flag ); diff --git a/src/utils/formatting.rs b/src/utils/formatting.rs index c65a98af2..5fed55069 100644 --- a/src/utils/formatting.rs +++ b/src/utils/formatting.rs @@ -103,7 +103,7 @@ pub struct NoQuotePathFmt<'a>(pub &'a Path); /// Covers C0 controls, DEL, C1 controls (U+0080..U+009F), line/paragraph separators, /// BiDi formatting characters (Trojan Source, CVE-2021-42574), and zero-width /// / invisible characters used for homoglyph or hidden-content attacks -fn is_unsafe_display_char(ch: char) -> bool { +pub fn is_unsafe_display_char(ch: char) -> bool { match ch { c if c.is_control() => true, '\u{2028}' | '\u{2029}' => true, @@ -114,13 +114,24 @@ fn is_unsafe_display_char(ch: char) -> bool { } } -/// Write a char to a formatter, replacing unsafe chars with U+FFFD -fn write_sanitized_char(ch: char, f: &mut fmt::Formatter<'_>) -> fmt::Result { - if is_unsafe_display_char(ch) { - f.write_char('\u{FFFD}') - } else { - f.write_char(ch) +pub fn contains_unsafe_display_char(text: &str) -> bool { + text.chars().any(is_unsafe_display_char) +} + +pub fn sanitize_for_display(text: &str) -> Cow<'_, str> { + if !contains_unsafe_display_char(text) { + return Cow::Borrowed(text); + } + + let mut sanitized = String::with_capacity(text.len()); + for ch in text.chars() { + if is_unsafe_display_char(ch) { + sanitized.push('\u{FFFD}'); + } else { + sanitized.push(ch); + } } + Cow::Owned(sanitized) } /// Same as NoQuotePathFmt, but surrounded by "". @@ -130,19 +141,6 @@ impl<'a> fmt::Display for PathFmt<'a> { } } -/// Wrapper that writes a string to a formatter, replacing unsafe chars -/// Use for any archive-derived string that doesn't flow through PathFmt -pub struct SanitizedStr<'a>(pub &'a str); - -impl<'a> fmt::Display for SanitizedStr<'a> { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - for ch in self.0.chars() { - write_sanitized_char(ch, f)?; - } - Ok(()) - } -} - /// Same as `path.display()` but try to strip some common noise prefixes /// Also strips control bytes and BiDi/zero-width chars to prevent terminal injection impl<'a> fmt::Display for NoQuotePathFmt<'a> { @@ -160,10 +158,7 @@ impl<'a> fmt::Display for NoQuotePathFmt<'a> { path }; - for ch in path.display().to_string().chars() { - write_sanitized_char(ch, f)?; - } - Ok(()) + f.write_str(&sanitize_for_display(&path.display().to_string())) } } diff --git a/src/utils/logger.rs b/src/utils/logger.rs index 5d4d095f3..ac2ae37f6 100644 --- a/src/utils/logger.rs +++ b/src/utils/logger.rs @@ -6,7 +6,10 @@ use std::{ pub use logger_thread::spawn_logger_thread; -use super::colors::{GREEN, ORANGE, RESET}; +use super::{ + colors::{GREEN, ORANGE, RESET}, + formatting::{contains_unsafe_display_char, sanitize_for_display}, +}; use crate::accessible::is_running_in_accessible_mode; #[macro_export] @@ -261,7 +264,7 @@ mod logger_thread { LoggerCommand::Print(msg) => { // Append message to buffer if msg.should_display() { - writeln!(buffer, "{msg}").unwrap(); + write_message_to_buffer(&mut buffer, &msg); } } LoggerCommand::Flush { finished_barrier } => { @@ -277,6 +280,19 @@ mod logger_thread { } } + fn write_message_to_buffer(buffer: &mut Vec, msg: &PrintMessage) { + if !contains_unsafe_display_char(&msg.contents) { + writeln!(buffer, "{msg}").unwrap(); + } else { + let msg = PrintMessage { + contents: sanitize_for_display(&msg.contents).into_owned(), + accessible: msg.accessible, + level: msg.level, + }; + writeln!(buffer, "{msg}").unwrap(); + } + } + fn flush_logs_to_stderr(buffer: &mut Vec) { if !buffer.is_empty() { // Ignore stderr write failures (broken pipe, closed terminal) instead of panicking diff --git a/tests/integration.rs b/tests/integration.rs index 4c30533b7..406e3918a 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -377,7 +377,7 @@ fn multiple_files_with_conflict_and_choice_to_rename_with_already_a_renamed( #[cfg(feature = "unrar")] #[test] fn unpack_rar() -> Result<(), Box> { - fn test_unpack_rar_single(input: &std::path::Path) -> Result<(), Box> { + fn test_unpack_rar_single(input: &Path) -> Result<(), Box> { let (_tempdir, dirpath) = testdir()?; let unpacked_path = &dirpath.join("testfile.txt"); ouch!("-A", "d", input, "-d", dirpath); @@ -399,7 +399,7 @@ fn unpack_rar() -> Result<(), Box> { #[cfg(feature = "unrar")] #[test] fn unpack_rar_stdin() -> Result<(), Box> { - fn test_unpack_rar_single(input: &std::path::Path, format: &str) -> Result<(), Box> { + fn test_unpack_rar_single(input: &Path, format: &str) -> Result<(), Box> { let (_tempdir, dirpath) = testdir()?; let unpacked_path = &dirpath.join("testfile.txt"); crate::utils::cargo_bin() From 8113f3befdabb5e1c5c9b0b29094fceb2e9d3e3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Marcos?= Date: Tue, 26 May 2026 21:27:12 -0300 Subject: [PATCH 5/6] update changelog --- CHANGELOG.md | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dad2816b1..a4bb884a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ Categories Used: - Tweaks - anything that doesn't fit into other categories, small typo fixes, most CI stuff, meta changes (e.g. README updates), etc. - Removals - removal of a feature (and most likely a breaking change) +- Safety - Changes focused on preventing malicious attacks **Bullet points in chronological order by PR** @@ -21,6 +22,16 @@ Categories Used: ### Improvements +- Add `OUCH_PASSWORD` env var as alternative to `--password` (https://github.com/ouch-org/ouch/pull/951). +- Add `OUCH_MAX_DECOMPRESSED_BYTES` to optionally cap decompressed output size and listing (https://github.com/ouch-org/ouch/pull/951). + +### Safety + +- Limit LZMA/XZ decoder size limit to 256 MiB to prevent OOM attacks from malicious inputs (https://github.com/ouch-org/ouch/pull/951). +- Prevent path traversal and symlink/hardlink escape attacks in tar, zip, 7z, and rar entries (https://github.com/ouch-org/ouch/pull/951). +- Sanitize archive-controlled filenames, comments, symlink targets, and error text before printing to the terminal (https://github.com/ouch-org/ouch/pull/951). +- Strip special permission bits from archive-supplied file modes and create zip outputs with final sanitized permissions immediately (https://github.com/ouch-org/ouch/pull/951). + ### Bug Fixes ### Tweaks @@ -47,9 +58,6 @@ Categories Used: - Fix various panics not handled gracefully (https://github.com/ouch-org/ouch/pull/950) - Handle GNUSparse archive entries during tar decompression (https://github.com/ouch-org/ouch/pull/975) -### Tweaks - - ## [0.7.1](https://github.com/ouch-org/ouch/releases/tag/0.7.1) ### Bug Fixes From 13c5f20f76632632f716e88abfe6b7468fd96f87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Marcos?= Date: Sat, 20 Jun 2026 23:32:48 -0300 Subject: [PATCH 6/6] reduce boilerplate for LimitedReader --- src/archive/sevenz.rs | 9 ++++----- src/archive/zip.rs | 9 ++++----- src/commands/decompress.rs | 14 ++++++-------- src/commands/list.rs | 13 +++++-------- src/utils/fs.rs | 11 ++++++++--- 5 files changed, 27 insertions(+), 29 deletions(-) diff --git a/src/archive/sevenz.rs b/src/archive/sevenz.rs index f25c61d90..5e1b33b40 100644 --- a/src/archive/sevenz.rs +++ b/src/archive/sevenz.rs @@ -1,7 +1,7 @@ //! SevenZip archive format compress function use std::{ - io::{self, BufWriter, Read, Seek, Write}, + io::{BufWriter, Read, Seek, Write}, path::{Path, PathBuf}, }; @@ -17,8 +17,8 @@ use crate::{ info, list::{FileInArchive, ListFileType}, utils::{ - BytesFmt, FileVisibilityPolicy, LimitedReader, PathFmt, cd_into_same_dir_as, ensure_parent_dir_exists, - is_same_file_as_output, max_decompressed_bytes, validate_dest_inside_root, validate_entry_path, + BytesFmt, FileVisibilityPolicy, PathFmt, cd_into_same_dir_as, copy_limited_decompression, + ensure_parent_dir_exists, is_same_file_as_output, validate_dest_inside_root, validate_entry_path, }, warning, }; @@ -60,8 +60,7 @@ where let file = fs::File::create(path)?; let mut writer = BufWriter::new(file); - let mut limited = LimitedReader::new(reader, max_decompressed_bytes()); - io::copy(&mut limited, &mut writer)?; + copy_limited_decompression(reader, &mut writer)?; use filetime_creation as ft; // Surface mtime-set failures as warnings so users know timestamps weren't preserved diff --git a/src/archive/zip.rs b/src/archive/zip.rs index d8f09686e..d8ab5e0e2 100644 --- a/src/archive/zip.rs +++ b/src/archive/zip.rs @@ -22,9 +22,9 @@ use crate::{ info, info_accessible, list::{FileInArchive, ListFileType}, utils::{ - BytesFmt, FileType, FileVisibilityPolicy, LimitedReader, PathFmt, canonicalize, cd_into_same_dir_as, - create_symlink, ensure_parent_dir_exists, get_invalid_utf8_paths, is_same_file_as_output, - max_decompressed_bytes, pretty_format_list_of_paths, read_file_type, strip_cur_dir, validate_dest_inside_root, + BytesFmt, FileType, FileVisibilityPolicy, PathFmt, canonicalize, cd_into_same_dir_as, + copy_limited_decompression, create_symlink, ensure_parent_dir_exists, get_invalid_utf8_paths, + is_same_file_as_output, pretty_format_list_of_paths, read_file_type, strip_cur_dir, validate_dest_inside_root, validate_symlink_target, }, warning, @@ -108,8 +108,7 @@ where #[cfg(not(unix))] let mut output_file = fs::File::create(file_path)?; { - let mut limited = LimitedReader::new(&mut file, max_decompressed_bytes()); - io::copy(&mut limited, &mut output_file)?; + copy_limited_decompression(&mut file, &mut output_file)?; } set_last_modified_time(&file, file_path)?; #[cfg(unix)] diff --git a/src/commands/decompress.rs b/src/commands/decompress.rs index 449d7c384..dae6396fc 100644 --- a/src/commands/decompress.rs +++ b/src/commands/decompress.rs @@ -16,9 +16,9 @@ use crate::{ info, info_accessible, non_archive::lz4::MultiFrameLz4Decoder, utils::{ - self, BytesFmt, LZMA_MEMLIMIT_BYTES, LimitedReader, PathFmt, file_size, + self, BytesFmt, LZMA_MEMLIMIT_BYTES, LimitedReader, PathFmt, copy_limited_decompression, file_size, io::{ReadSeek, lock_and_flush_output_stdio}, - is_path_stdin, max_decompressed_bytes, resolve_path_conflict, user_wants_to_continue, + is_path_stdin, resolve_path_conflict, user_wants_to_continue, }, }; @@ -114,7 +114,7 @@ pub fn decompress_file(options: DecompressOptions) -> Result<()> { let reader = create_decoder_up_to_first_extension()?; let reader = chain_reader_decoder(&first_extension, reader)?; // Bomb cap: abort if decompressed output exceeds OUCH_MAX_DECOMPRESSED_BYTES - let mut reader = LimitedReader::new(reader, max_decompressed_bytes()); + let mut reader = LimitedReader::new(reader); let (mut writer, final_output_path) = match utils::create_file_or_prompt_on_conflict( &options.output_file_path, @@ -132,7 +132,7 @@ pub fn decompress_file(options: DecompressOptions) -> Result<()> { } Tar => unpack_archive( |output_dir| { - let reader = LimitedReader::new(create_decoder_up_to_first_extension()?, max_decompressed_bytes()); + let reader = LimitedReader::new(create_decoder_up_to_first_extension()?); crate::archive::tar::unpack_archive(reader, output_dir) }, archive_output_dir, @@ -170,8 +170,7 @@ pub fn decompress_file(options: DecompressOptions) -> Result<()> { let mut vec = vec![]; // Bomb cap: abort if the in-memory decompressed image exceeds the limit - let mut reader = LimitedReader::new(create_decoder_up_to_first_extension()?, max_decompressed_bytes()); - io::copy(&mut reader, &mut vec)?; + copy_limited_decompression(create_decoder_up_to_first_extension()?, &mut vec)?; Box::new(io::Cursor::new(vec)) } else { Box::new(BufReader::with_capacity( @@ -190,8 +189,7 @@ pub fn decompress_file(options: DecompressOptions) -> Result<()> { Rar => { let unpack_fn: Box Result> = if options.formats.len() > 1 || input_is_stdin { let mut temp_file = tempfile::NamedTempFile::new()?; - let mut limited = LimitedReader::new(create_decoder_up_to_first_extension()?, max_decompressed_bytes()); - io::copy(&mut limited, &mut temp_file)?; + copy_limited_decompression(create_decoder_up_to_first_extension()?, &mut temp_file)?; Box::new(move |output_dir| { crate::archive::rar::unpack_archive(temp_file.path(), output_dir, options.password) }) diff --git a/src/commands/list.rs b/src/commands/list.rs index f61643e45..7afc49932 100644 --- a/src/commands/list.rs +++ b/src/commands/list.rs @@ -12,7 +12,7 @@ use crate::{ list::{self, FileInArchive, ListOptions}, non_archive::lz4::MultiFrameLz4Decoder, utils::{ - LZMA_MEMLIMIT_BYTES, LimitedReader, io::lock_and_flush_output_stdio, max_decompressed_bytes, + LZMA_MEMLIMIT_BYTES, LimitedReader, copy_limited_decompression, io::lock_and_flush_output_stdio, user_wants_to_continue, }, }; @@ -87,7 +87,7 @@ pub fn list_archive_contents( let archive_format = misplaced_archive_format.unwrap_or(formats[0]); let files: Box>> = match archive_format { Tar => { - let limited = LimitedReader::new(reader, max_decompressed_bytes()); + let limited = LimitedReader::new(reader); Box::new(crate::archive::tar::list_archive(tar::Archive::new(limited))?) } Zip => { @@ -103,8 +103,7 @@ pub fn list_archive_contents( let mut vec = vec![]; // Bomb cap: abort if the in-memory decompressed image exceeds the limit - let mut limited = LimitedReader::new(&mut reader, max_decompressed_bytes()); - io::copy(&mut limited, &mut vec)?; + copy_limited_decompression(&mut reader, &mut vec)?; let zip_archive = zip::ZipArchive::new(io::Cursor::new(vec))?; Box::new(crate::archive::zip::list_archive(zip_archive, password)) @@ -114,8 +113,7 @@ pub fn list_archive_contents( if formats.len() > 1 { let mut temp_file = tempfile::NamedTempFile::new()?; // Bomb cap: abort if decompressed output to tempfile exceeds the limit - let mut limited = LimitedReader::new(&mut reader, max_decompressed_bytes()); - io::copy(&mut limited, &mut temp_file)?; + copy_limited_decompression(&mut reader, &mut temp_file)?; Box::new(crate::archive::rar::list_archive(temp_file.path(), password)?) } else { Box::new(crate::archive::rar::list_archive(archive_path, password)?) @@ -137,8 +135,7 @@ pub fn list_archive_contents( let mut vec = vec![]; // Bomb cap: abort if the in-memory decompressed image exceeds the limit - let mut limited = LimitedReader::new(&mut reader, max_decompressed_bytes()); - io::copy(&mut limited, &mut vec)?; + copy_limited_decompression(&mut reader, &mut vec)?; Box::new(archive::sevenz::list_archive(io::Cursor::new(vec), password)?) } else { diff --git a/src/utils/fs.rs b/src/utils/fs.rs index e0b91d1b1..aaa882710 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -3,7 +3,7 @@ use std::{ borrow::Cow, env, - io::{self, Read}, + io::{self, Read, Write}, path::{Path, PathBuf}, }; @@ -200,14 +200,19 @@ pub struct LimitedReader { } impl LimitedReader { - pub fn new(inner: R, limit: u64) -> Self { + pub fn new(inner: R) -> Self { Self { inner, - remaining: limit, + remaining: max_decompressed_bytes(), } } } +pub fn copy_limited_decompression(reader: R, writer: &mut W) -> io::Result { + let mut limited = LimitedReader::new(reader); + io::copy(&mut limited, writer) +} + impl Read for LimitedReader { fn read(&mut self, buf: &mut [u8]) -> io::Result { if buf.is_empty() {