security improvements

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
 
 - Decompress: exit with a non-zero code when a file conflict can't be resolved on non-interactive stdin, instead of silently skipping and reporting success (https://github.com/ouch-org/ouch/pull/1011)
@@ -49,9 +60,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

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 {

src/archive/sevenz.rs

@@ -1,8 +1,7 @@
 //! SevenZip archive format compress function
 
 use std::{
-    env,
-    io::{self, BufWriter, Read, Seek, Write},
+    io::{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, 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,
 };
@@ -33,7 +33,20 @@ 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 = 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));
@@ -47,7 +60,7 @@ where
 
                 let file = fs::File::create(path)?;
                 let mut writer = BufWriter::new(file);
-                io::copy(reader, &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
@@ -136,6 +149,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 +186,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,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<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)?;
+                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<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 +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()?)

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, 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,
 };
@@ -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, 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, Path::new(&target))?;
                     #[cfg(unix)]
                     std::os::unix::fs::symlink(&target, &file_path)?;
                     #[cfg(windows)]
@@ -81,12 +89,27 @@ where
                     let mut target = String::new();
                     file.read_to_string(&mut target)?;
 
+                    validate_symlink_target(&relpath, Path::new(&target))?;
                     info!("linking {} -> \"{}\"", PathFmt(file_path), 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)?;
+                    {
+                        copy_limited_decompression(&mut file, &mut output_file)?;
+                    }
                     set_last_modified_time(&file, file_path)?;
                     #[cfg(unix)]
                     unix_set_permissions(file_path, &file)?;
@@ -182,6 +205,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 +279,6 @@ where
                 }
             }
         }
-
-        env::set_current_dir(previous_location)?;
     }
 
     let bytes = writer.finish()?;
@@ -315,7 +337,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