Bug fix: don't allow writing files with certain ZIP64 magic strings in their names

src/read.rs

@@ -1123,6 +1123,10 @@ pub fn read_zipfile_from_stream<'a, R: Read>(reader: &'a mut R) -> ZipResult<Opt
 
 #[cfg(test)]
 mod test {
+    use crate::ZipArchive;
+    use io::Cursor;
+    use std::io;
+
     #[test]
     fn invalid_offset() {
         use super::ZipArchive;
@@ -1130,7 +1134,7 @@ mod test {
 
         let mut v = Vec::new();
         v.extend_from_slice(include_bytes!("../tests/data/invalid_offset.zip"));
-        let reader = ZipArchive::new(io::Cursor::new(v));
+        let reader = ZipArchive::new(Cursor::new(v));
         assert!(reader.is_err());
     }
 
@@ -1141,7 +1145,7 @@ mod test {
 
         let mut v = Vec::new();
         v.extend_from_slice(include_bytes!("../tests/data/invalid_offset2.zip"));
-        let reader = ZipArchive::new(io::Cursor::new(v));
+        let reader = ZipArchive::new(Cursor::new(v));
         assert!(reader.is_err());
     }
 
@@ -1152,7 +1156,7 @@ mod test {
 
         let mut v = Vec::new();
         v.extend_from_slice(include_bytes!("../tests/data/zip64_demo.zip"));
-        let reader = ZipArchive::new(io::Cursor::new(v)).unwrap();
+        let reader = ZipArchive::new(Cursor::new(v)).unwrap();
         assert_eq!(reader.len(), 1);
     }
 
@@ -1163,7 +1167,7 @@ mod test {
 
         let mut v = Vec::new();
         v.extend_from_slice(include_bytes!("../tests/data/mimetype.zip"));
-        let mut reader = ZipArchive::new(io::Cursor::new(v)).unwrap();
+        let mut reader = ZipArchive::new(Cursor::new(v)).unwrap();
         assert_eq!(reader.comment(), b"");
         assert_eq!(reader.by_index(0).unwrap().central_header_start(), 77);
     }
@@ -1175,7 +1179,7 @@ mod test {
 
         let mut v = Vec::new();
         v.extend_from_slice(include_bytes!("../tests/data/mimetype.zip"));
-        let mut reader = io::Cursor::new(v);
+        let mut reader = Cursor::new(v);
         loop {
             if read_zipfile_from_stream(&mut reader).unwrap().is_none() {
                 break;
@@ -1190,7 +1194,7 @@ mod test {
 
         let mut v = Vec::new();
         v.extend_from_slice(include_bytes!("../tests/data/mimetype.zip"));
-        let mut reader1 = ZipArchive::new(io::Cursor::new(v)).unwrap();
+        let mut reader1 = ZipArchive::new(Cursor::new(v)).unwrap();
         let mut reader2 = reader1.clone();
 
         let mut file1 = reader1.by_index(0).unwrap();
@@ -1231,7 +1235,7 @@ mod test {
 
         let mut v = Vec::new();
         v.extend_from_slice(include_bytes!("../tests/data/files_and_dirs.zip"));
-        let mut zip = ZipArchive::new(io::Cursor::new(v)).unwrap();
+        let mut zip = ZipArchive::new(Cursor::new(v)).unwrap();
 
         for i in 0..zip.len() {
             let zip_file = zip.by_index(i).unwrap();
@@ -1244,6 +1248,22 @@ mod test {
         }
     }
 
+    #[test]
+    fn zip64_magic_in_filenames() {
+        let files = vec![
+            include_bytes!("../tests/data/zip64_magic_in_filename_1.zip"),
+            include_bytes!("../tests/data/zip64_magic_in_filename_2.zip"),
+            include_bytes!("../tests/data/zip64_magic_in_filename_3.zip"),
+        ];
+        // Although we don't allow adding files whose names contain the ZIP64 CDB-end or
+        // CDB-end-locator signatures, we still read them when they aren't genuinely ambiguous.
+        for file in files {
+            let mut v = Vec::new();
+            v.extend_from_slice(file);
+            ZipArchive::new(Cursor::new(v)).unwrap();
+        }
+    }
+
     /// 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
@@ -1256,7 +1276,7 @@ mod test {
         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));
+        let reader = ZipArchive::new(Cursor::new(v));
         assert!(reader.is_err() || reader.unwrap().is_empty());
     }
 
@@ -1272,7 +1292,7 @@ mod test {
         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));
+        let reader = ZipArchive::new(Cursor::new(v));
         assert!(reader.is_err());
     }
 }

src/spec.rs

@@ -5,9 +5,9 @@ use std::io::prelude::*;
 
 pub const LOCAL_FILE_HEADER_SIGNATURE: u32 = 0x04034b50;
 pub const CENTRAL_DIRECTORY_HEADER_SIGNATURE: u32 = 0x02014b50;
-const CENTRAL_DIRECTORY_END_SIGNATURE: u32 = 0x06054b50;
+pub(crate) const CENTRAL_DIRECTORY_END_SIGNATURE: u32 = 0x06054b50;
 pub const ZIP64_CENTRAL_DIRECTORY_END_SIGNATURE: u32 = 0x06064b50;
-const ZIP64_CENTRAL_DIRECTORY_END_LOCATOR_SIGNATURE: u32 = 0x07064b50;
+pub(crate) const ZIP64_CENTRAL_DIRECTORY_END_LOCATOR_SIGNATURE: u32 = 0x07064b50;
 
 pub const ZIP64_BYTES_THR: u64 = u32::MAX as u64;
 pub const ZIP64_ENTRY_THR: usize = u16::MAX as usize;

src/write.rs

@@ -108,6 +108,7 @@ pub(crate) mod zip_writer {
         pub(super) comment: Vec<u8>,
     }
 }
+use crate::result::ZipError::InvalidArchive;
 use crate::write::GenericZipWriter::{Closed, Storer};
 pub use zip_writer::ZipWriter;
 
@@ -300,7 +301,7 @@ impl<A: Read + Write + Seek> ZipWriter<A> {
             ZipArchive::get_directory_counts(&mut readwriter, &footer, cde_start_pos)?;
 
         if readwriter.seek(SeekFrom::Start(directory_start)).is_err() {
-            return Err(ZipError::InvalidArchive(
+            return Err(InvalidArchive(
                 "Could not seek to start of central directory",
             ));
         }
@@ -438,6 +439,7 @@ impl<W: Write + Seek> ZipWriter<W> {
         {
             let header_start = self.inner.get_plain().stream_position()?;
             let name = name.into();
+            Self::validate_name(&name)?;
 
             let permissions = options.permissions.unwrap_or(0o100644);
             let file = ZipFileData {
@@ -491,7 +493,7 @@ impl<W: Write + Seek> ZipWriter<W> {
     fn insert_file_data(&mut self, file: ZipFileData) -> ZipResult<usize> {
         let name = &file.file_name;
         if self.files_by_name.contains_key(name) {
-            return Err(ZipError::InvalidArchive("Duplicate filename"));
+            return Err(InvalidArchive("Duplicate filename"));
         }
         let name = name.to_owned();
         self.files.push(file);
@@ -1030,6 +1032,27 @@ impl<W: Write + Seek> ZipWriter<W> {
         self.insert_file_data(dest_data)?;
         Ok(())
     }
+    fn validate_name(name: &String) -> ZipResult<()> {
+        for (index, _) in name.match_indices("PK") {
+            if name.len() >= index + 4 {
+                let magic_number = name[index..index + 4]
+                    .as_bytes()
+                    .read_u32::<LittleEndian>()?;
+                match magic_number {
+                    spec::ZIP64_CENTRAL_DIRECTORY_END_SIGNATURE => {
+                        return Err(InvalidArchive("Filename can't contain ZIP64 end signature"));
+                    }
+                    spec::ZIP64_CENTRAL_DIRECTORY_END_LOCATOR_SIGNATURE => {
+                        return Err(InvalidArchive(
+                            "Filename can't contain ZIP64 end-locator signature",
+                        ));
+                    }
+                    _ => {}
+                }
+            }
+        }
+        Ok(())
+    }
 }
 
 impl<W: Write + Seek> Drop for ZipWriter<W> {
@@ -1731,79 +1754,6 @@ mod test {
             .start_file("foo/bar/test", FileOptions::default())
             .expect_err("Expected duplicate filename not to be allowed");
     }
-
-    #[test]
-    fn test_filename_looks_like_zip64_locator() {
-        let mut writer = ZipWriter::new(io::Cursor::new(Vec::new()));
-        writer
-            .start_file(
-                "PK\u{6}\u{7}\0\0\0\u{11}\0\0\0\0\0\0\0\0\0\0\0\0",
-                FileOptions::default(),
-            )
-            .unwrap();
-        let zip = writer.finish().unwrap();
-        let _ = ZipArchive::new(zip).unwrap();
-    }
-
-    #[test]
-    fn test_filename_looks_like_zip64_locator_2() {
-        let mut writer = ZipWriter::new(io::Cursor::new(Vec::new()));
-        writer
-            .start_file(
-                "PK\u{6}\u{6}\0\0\0\0\0\0\0\0\0\0PK\u{6}\u{7}\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0",
-                FileOptions::default(),
-            )
-            .unwrap();
-        let zip = writer.finish().unwrap();
-        println!("{:02x?}", zip.get_ref());
-        let _ = ZipArchive::new(zip).unwrap();
-    }
-
-    #[test]
-    fn test_filename_looks_like_zip64_locator_2a() {
-        let mut writer = ZipWriter::new(io::Cursor::new(Vec::new()));
-        writer
-            .start_file(
-                "PK\u{6}\u{6}PK\u{6}\u{7}\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0",
-                FileOptions::default(),
-            )
-            .unwrap();
-        let zip = writer.finish().unwrap();
-        println!("{:02x?}", zip.get_ref());
-        let _ = ZipArchive::new(zip).unwrap();
-    }
-
-    #[test]
-    fn test_filename_looks_like_zip64_locator_3() {
-        let mut writer = ZipWriter::new(io::Cursor::new(Vec::new()));
-        writer
-            .start_file("\0PK\u{6}\u{6}", FileOptions::default())
-            .unwrap();
-        writer
-            .start_file(
-                "\0\u{4}\0\0PK\u{6}\u{7}\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\u{3}",
-                FileOptions::default(),
-            )
-            .unwrap();
-        let zip = writer.finish().unwrap();
-        println!("{:02x?}", zip.get_ref());
-        let _ = ZipArchive::new(zip).unwrap();
-    }
-
-    #[test]
-    fn path_to_string() {
-        let mut path = std::path::PathBuf::new();
-        #[cfg(windows)]
-        path.push(r"C:\");
-        #[cfg(unix)]
-        path.push("/");
-        path.push("windows");
-        path.push("..");
-        path.push(".");
-        path.push("system32");
-        let path_str = super::path_to_string(&path);
-        assert_eq!(path_str, "windows/system32");
-    }
 }
 
 #[cfg(not(feature = "unreserved"))]