security improvements

Author: curious-rabbit

src/archive/rar.rs

@@ -8,7 +8,7 @@ use crate::{
     error::{Error, Result},
     info,
     list::{FileInArchive, ListFileType},
-    utils::BytesFmt,
+    utils::{BytesFmt, PathFmt},
 };
 
 /// Unpacks the archive given by `archive_path` into the folder given by `output_folder`.
@@ -28,7 +28,7 @@ pub fn unpack_archive(archive_path: &Path, output_folder: &Path, password: Optio
             info!(
                 "extracted ({}) {}",
                 BytesFmt(entry.unpacked_size),
-                entry.filename.display(),
+                PathFmt(&entry.filename),
             );
             files_unpacked += 1;
             header.extract_with_base(output_folder)?

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,8 @@ 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, PathFmt, SanitizedStr, cd_into_same_dir_as, ensure_parent_dir_exists,
+        is_same_file_as_output, validate_entry_path,
     },
     warning,
 };
@@ -33,10 +33,22 @@ where
         |entry: &ArchiveEntry, reader: &mut dyn Read, path: &PathBuf| -> Result<bool, sevenz_rust2::Error> {
             // 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 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)?;
                 }
@@ -136,6 +148,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 +185,6 @@ where
 
             writer.push_archive_entry::<fs::File>(entry, entry_data)?;
         }
-
-        env::set_current_dir(previous_location)?;
     }
 
     let bytes = writer.finish()?;

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,7 @@ 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_entry_path, validate_symlink_target,
     },
     warning,
 };
@@ -38,22 +37,27 @@ pub fn unpack_archive(reader: impl Read, output_folder: &Path) -> Result<u64> {
 
         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)?;
                 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);
 
                 fs::hard_link(&full_target_path, &full_link_path)?;
             }
@@ -71,10 +75,11 @@ pub fn unpack_archive(reader: impl Read, output_folder: &Path) -> Result<u64> {
                     // 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 +151,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 +237,6 @@ where
                 }
             }
         }
-        env::set_current_dir(previous_location)?;
     }
 
     Ok(builder.into_inner()?)

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,17 @@ use same_file::Handle;
 use time::OffsetDateTime;
 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, PathFmt, SanitizedStr, 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, validate_symlink_target,
     },
     warning,
 };
@@ -42,12 +43,15 @@ 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);
 
         display_zip_comment_if_exists(&file);
 
@@ -62,6 +66,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,10 +86,23 @@ 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)?;
                     set_last_modified_time(&file, file_path)?;
@@ -182,6 +200,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 +274,6 @@ where
                 }
             }
         }
-
-        env::set_current_dir(previous_location)?;
     }
 
     let bytes = writer.finish()?;
@@ -276,7 +293,11 @@ fn display_zip_comment_if_exists<R: Read>(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)
+        );
     }
 }
 
@@ -311,7 +332,7 @@ fn unix_set_permissions<R: Read>(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(())

src/cli/args.rs

@@ -43,7 +43,7 @@ pub struct CliArgs {
     pub format: Option<String>,
 
     /// 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<OsString>,
 
     /// Concurrent working threads

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,
     },
 };
 
@@ -59,7 +59,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)),
@@ -90,7 +94,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,
@@ -142,7 +148,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(

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)),
@@ -92,7 +99,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 +110,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 +133,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 {