From 03613cb56e1441f3bbb5376e207e1ce3ba6b5fa8 Mon Sep 17 00:00:00 2001
From: Nick Babcock <nbabcock19@hotmail.com>
Date: Mon, 25 Apr 2022 07:04:19 -0500
Subject: [PATCH] Fix capacity overflow on invalid zips reads

Preemptively allocating structures with the number of reported files can
lead to trouble as an invalid zip can still have a valid central
directory end that is fed into a `with_capacity` causing it to overflow.

This commit introduces a heuristic that will use the reported number of
files as long as the number is less than the cde offset.

Benchmarks were unaffected by this change.
---
 src/read.rs                                   |  44 +++++++++++++++++-
 ...ber_of_files_allocation_greater_offset.zip | Bin 0 -> 124 bytes
 ...ber_of_files_allocation_smaller_offset.zip | Bin 0 -> 212 bytes
 3 files changed, 42 insertions(+), 2 deletions(-)
 create mode 100644 tests/data/invalid_cde_number_of_files_allocation_greater_offset.zip
 create mode 100644 tests/data/invalid_cde_number_of_files_allocation_smaller_offset.zip

diff --git a/src/read.rs b/src/read.rs
index 0c06db17..c0311947 100644
--- a/src/read.rs
+++ b/src/read.rs
@@ -408,8 +408,16 @@ impl<R: Read + io::Seek> ZipArchive<R> {
         let (archive_offset, directory_start, number_of_files) =
             Self::get_directory_counts(&mut reader, &footer, cde_start_pos)?;
 
-        let mut files = Vec::with_capacity(number_of_files);
-        let mut names_map = HashMap::with_capacity(number_of_files);
+        // If the parsed number of files is greater than the offset then
+        // something fishy is going on and we shouldn't trust number_of_files.
+        let file_capacity = if number_of_files > cde_start_pos as usize {
+            0
+        } else {
+            number_of_files
+        };
+
+        let mut files = Vec::with_capacity(file_capacity);
+        let mut names_map = HashMap::with_capacity(file_capacity);
 
         if reader.seek(io::SeekFrom::Start(directory_start)).is_err() {
             return Err(ZipError::InvalidArchive(
@@ -1267,4 +1275,36 @@ mod test {
             );
         }
     }
+
+    /// test case to ensure we don't preemptively over allocate based on the
+    /// declared number of files in the CDE of an invalid zip when the number of
+    /// files declared is more than the alleged offset in the CDE
+    #[test]
+    fn invalid_cde_number_of_files_allocation_smaller_offset() {
+        use super::ZipArchive;
+        use std::io;
+
+        let mut v = Vec::new();
+        v.extend_from_slice(include_bytes!(
+            "../tests/data/invalid_cde_number_of_files_allocation_smaller_offset.zip"
+        ));
+        let reader = ZipArchive::new(io::Cursor::new(v));
+        assert!(reader.is_err());
+    }
+
+    /// test case to ensure we don't preemptively over allocate based on the
+    /// declared number of files in the CDE of an invalid zip when the number of
+    /// files declared is less than the alleged offset in the CDE
+    #[test]
+    fn invalid_cde_number_of_files_allocation_greater_offset() {
+        use super::ZipArchive;
+        use std::io;
+
+        let mut v = Vec::new();
+        v.extend_from_slice(include_bytes!(
+            "../tests/data/invalid_cde_number_of_files_allocation_greater_offset.zip"
+        ));
+        let reader = ZipArchive::new(io::Cursor::new(v));
+        assert!(reader.is_err());
+    }
 }
diff --git a/tests/data/invalid_cde_number_of_files_allocation_greater_offset.zip b/tests/data/invalid_cde_number_of_files_allocation_greater_offset.zip
new file mode 100644
index 0000000000000000000000000000000000000000..a428ca7e3650cc3fe185b3eada5d19410b657281
GIT binary patch
literal 124
zcmZPy@MdHC>%+>%@E;ANGcYh{1H~8^Vu37d;3r%M`~Uy@VH}`=?C7ckynz;g0S}1u
J-Ub8=3;=~>F9QGo

literal 0
HcmV?d00001

diff --git a/tests/data/invalid_cde_number_of_files_allocation_smaller_offset.zip b/tests/data/invalid_cde_number_of_files_allocation_smaller_offset.zip
new file mode 100644
index 0000000000000000000000000000000000000000..2cc90073f6ec0bc3fa60e178af2c3927a79923bc
GIT binary patch
literal 212
zcmWIW4)A7U5AZGng8!rdkcJ`<0S9;gLcnSW#moc-K(pD{AR_-6umVO7n7weLkQllU
U_JlH!JJIzAc(byhi!m?&0OC4jTL1t6

literal 0
HcmV?d00001