security improvements

Author: curious-rabbit

src/archive/rar.rs

@@ -8,7 +8,8 @@ use crate::{
     error::{Error, Result},
     info,
     list::{FileInArchive, ListFileType},
-    utils::BytesFmt,
+    utils::{BytesFmt, PathFmt, validate_entry_path},
+    warning,
 };
 
 /// Unpacks the archive given by `archive_path` into the folder given by `output_folder`.
@@ -25,13 +26,18 @@ pub fn unpack_archive(archive_path: &Path, output_folder: &Path, password: Optio
     while let Some(header) = archive.read_header()? {
         let entry = header.entry();
         archive = if entry.is_file() {
-            info!(
-                "extracted ({}) {}",
-                BytesFmt(entry.unpacked_size),
-                entry.filename.display(),
-            );
-            files_unpacked += 1;
-            header.extract_with_base(output_folder)?
+            if let Err(e) = validate_entry_path(&entry.filename) {
+                warning!("skipping unsafe rar entry {}: {}", PathFmt(&entry.filename), e);
+                header.skip()?
+            } else {
+                info!(
+                    "extracted ({}) {}",
+                    BytesFmt(entry.unpacked_size),
+                    PathFmt(&entry.filename),
+                );
+                files_unpacked += 1;
+                header.extract_with_base(output_folder)?
+            }
         } else {
             header.skip()?
         };

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<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 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::<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, 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<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)
+        );
     }
 }
 
@@ -315,7 +342,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