fix: May have to consider multiple CDEs before filtering for validity

2024-06-18 19:58:16 -07:00 · 2024-06-18 19:58:16 -07:00 · 9bf914d7d4
commit 9bf914d7d4
parent 45472486f1
3 changed files with 129 additions and 57 deletions
--- a/src/read.rs
+++ b/src/read.rs
@ -8,7 +8,7 @@ use crate::crc32::Crc32Reader;
 use crate::extra_fields::{ExtendedTimestamp, ExtraField};
 use crate::read::zip_archive::{Shared, SharedBuilder};
 use crate::result::{ZipError, ZipResult};
-use crate::spec::{self, FixedSizeBlock};
+use crate::spec::{self, FixedSizeBlock, Zip32CentralDirectoryEnd};
 use crate::types::{
    AesMode, AesVendorVersion, DateTime, System, ZipCentralEntryBlock, ZipFileData,
    ZipLocalEntryBlock,
@ -23,6 +23,7 @@ use std::mem;
 use std::mem::size_of;
 use std::ops::Deref;
 use std::path::{Path, PathBuf};
+use std::rc::Rc;
 use std::sync::{Arc, OnceLock};

 #[cfg(feature = "deflate-flate2")]
@ -671,51 +672,55 @@ impl<R: Read + Seek> ZipArchive<R> {
    pub(crate) fn get_metadata(
        config: Config,
        reader: &mut R,
-        footer: &spec::Zip32CentralDirectoryEnd,
-        cde_start_pos: u64,
-    ) -> ZipResult<Shared> {
-        // Check if file has a zip64 footer
-        let results = Self::get_directory_info_zip64(&config, reader, footer, cde_start_pos)
-            .unwrap_or_else(|e| vec![Err(e)]);
+    ) -> ZipResult<(Zip32CentralDirectoryEnd, Shared)> {
        let mut invalid_errors = Vec::new();
        let mut unsupported_errors = Vec::new();
        let mut ok_results = Vec::new();
-        results
-            .into_iter()
-            .map(|result| {
-                result.and_then(|dir_info| Self::read_central_header(dir_info, config, reader))
-            })
-            .for_each(|result| {
-                Self::sort_result(
-                    result,
-                    &mut invalid_errors,
-                    &mut unsupported_errors,
-                    &mut ok_results,
-                )
-            });
-        if ok_results.is_empty() {
+        let cde_locations = spec::Zip32CentralDirectoryEnd::find_and_parse(reader)?;
+        IntoIterator::into_iter(cde_locations).for_each(|(footer, cde_start_pos)| {
            let zip32_result =
-                Self::get_directory_info_zip32(&config, reader, footer, cde_start_pos);
+                Self::get_directory_info_zip32(&config, reader, &footer, cde_start_pos);
            Self::sort_result(
                zip32_result.and_then(|result| Self::read_central_header(result, config, reader)),
                &mut invalid_errors,
                &mut unsupported_errors,
                &mut ok_results,
+                &footer,
            );
-        }
+            // Check if file has a zip64 footer
+            if let Ok(zip64_footers) =
+                Self::get_directory_info_zip64(&config, reader, &footer, cde_start_pos)
+            {
+                zip64_footers
+                    .into_iter()
+                    .map(|result| {
+                        result.and_then(|dir_info| {
+                            Self::read_central_header(dir_info, config, reader)
+                        })
+                    })
+                    .for_each(|result| {
+                        Self::sort_result(
+                            result,
+                            &mut invalid_errors,
+                            &mut unsupported_errors,
+                            &mut ok_results,
+                            &footer,
+                        )
+                    });
+            }
+        });
        if ok_results.is_empty() {
            return Err(unsupported_errors
                .into_iter()
                .next()
                .unwrap_or_else(|| invalid_errors.into_iter().next().unwrap()));
        }
-        let shared = ok_results
+        let (footer, shared) = ok_results
            .into_iter()
-            .max_by_key(|shared| (shared.dir_start - shared.offset, shared.dir_start))
-            .unwrap()
-            .build();
+            .max_by_key(|(_, shared)| (shared.dir_start - shared.offset, shared.dir_start))
+            .unwrap();
        reader.seek(io::SeekFrom::Start(shared.dir_start))?;
-        Ok(shared)
+        Ok((Rc::try_unwrap(footer).unwrap(), shared.build()))
    }

    fn read_central_header(
@ -751,14 +756,15 @@ impl<R: Read + Seek> ZipArchive<R> {
        result: Result<SharedBuilder, ZipError>,
        invalid_errors: &mut Vec<ZipError>,
        unsupported_errors: &mut Vec<ZipError>,
-        ok_results: &mut Vec<SharedBuilder>,
+        ok_results: &mut Vec<(Rc<Zip32CentralDirectoryEnd>, SharedBuilder)>,
+        footer: &Rc<Zip32CentralDirectoryEnd>,
    ) {
        match result {
            Err(ZipError::UnsupportedArchive(e)) => {
                unsupported_errors.push(ZipError::UnsupportedArchive(e))
            }
            Err(e) => invalid_errors.push(e),
-            Ok(o) => ok_results.push(o),
+            Ok(o) => ok_results.push((footer.clone(), o)),
        }
    }

@ -810,15 +816,12 @@ impl<R: Read + Seek> ZipArchive<R> {
    ///
    /// This uses the central directory record of the ZIP file, and ignores local file headers.
    pub fn with_config(config: Config, mut reader: R) -> ZipResult<ZipArchive<R>> {
-        let mut results = spec::Zip32CentralDirectoryEnd::find_and_parse(&mut reader)?;
-        for (footer, cde_start_pos) in results.iter_mut() {
-            if let Ok(shared) = Self::get_metadata(config, &mut reader, footer, *cde_start_pos) {
-                return Ok(ZipArchive {
-                    reader,
-                    shared: shared.into(),
-                    comment: Arc::from(mem::replace(&mut footer.zip_file_comment, Box::new([]))),
-                });
-            }
+        if let Ok((footer, shared)) = Self::get_metadata(config, &mut reader) {
+            return Ok(ZipArchive {
+                reader,
+                shared: shared.into(),
+                comment: footer.zip_file_comment.into(),
+            });
        }
        Err(InvalidArchive("No valid central directory found"))
    }
--- a/src/spec.rs
+++ b/src/spec.rs
@ -5,6 +5,7 @@ use memchr::memmem::FinderRev;
 use std::io;
 use std::io::prelude::*;
 use std::mem;
+use std::rc::Rc;

 /// "Magic" header values used in the zip spec to locate metadata records.
 ///
@ -298,7 +299,7 @@ impl Zip32CentralDirectoryEnd {

    pub fn find_and_parse<T: Read + Seek>(
        reader: &mut T,
-    ) -> ZipResult<Box<[(Zip32CentralDirectoryEnd, u64)]>> {
+    ) -> ZipResult<Box<[(Rc<Zip32CentralDirectoryEnd>, u64)]>> {
        let mut results = vec![];
        let file_length = reader.seek(io::SeekFrom::End(0))?;

@ -338,7 +339,7 @@ impl Zip32CentralDirectoryEnd {
                reader.seek(io::SeekFrom::Start(cde_start_pos))?;
                /* Drop any headers that don't parse. */
                if let Ok(cde) = Self::parse(reader) {
-                    results.push((cde, cde_start_pos));
+                    results.push((Rc::new(cde), cde_start_pos));
                }
            }

--- a/src/write.rs
+++ b/src/write.rs
@ -625,23 +625,19 @@ impl<A: Read + Write + Seek> ZipWriter<A> {
    ///
    /// This uses the given read configuration to initially read the archive.
    pub fn new_append_with_config(config: Config, mut readwriter: A) -> ZipResult<ZipWriter<A>> {
-        let mut results = spec::Zip32CentralDirectoryEnd::find_and_parse(&mut readwriter)?;
-        for (footer, cde_start_pos) in results.iter_mut() {
-            if let Ok(metadata) =
-                ZipArchive::get_metadata(config, &mut readwriter, footer, *cde_start_pos)
-            {
-                return Ok(ZipWriter {
-                    inner: Storer(MaybeEncrypted::Unencrypted(readwriter)),
-                    files: metadata.files,
-                    stats: Default::default(),
-                    writing_to_file: false,
-                    comment: mem::replace(&mut footer.zip_file_comment, Box::new([])),
-                    writing_raw: true, // avoid recomputing the last file's header
-                    flush_on_finish_file: false,
-                });
-            }
+        if let Ok((footer, shared)) = ZipArchive::get_metadata(config, &mut readwriter) {
+            Ok(ZipWriter {
+                inner: Storer(MaybeEncrypted::Unencrypted(readwriter)),
+                files: shared.files,
+                stats: Default::default(),
+                writing_to_file: false,
+                comment: footer.zip_file_comment,
+                writing_raw: true, // avoid recomputing the last file's header
+                flush_on_finish_file: false,
+            })
+        } else {
+            Err(InvalidArchive("No central-directory end header found"))
        }
-        Err(InvalidArchive("No central-directory end header found"))
    }

    /// `flush_on_finish_file` is designed to support a streaming `inner` that may unload flushed
@ -3300,4 +3296,76 @@ mod test {
        let _ = writer.finish_into_readable()?;
        Ok(())
    }
+
+    #[test]
+    fn test_fuzz_crash_2024_06_18a() -> ZipResult<()> {
+        let mut writer = ZipWriter::new(Cursor::new(Vec::new()));
+        writer.set_flush_on_finish_file(false);
+        writer.set_raw_comment(Box::<[u8]>::from([]));
+        let sub_writer = {
+            let mut writer = ZipWriter::new(Cursor::new(Vec::new()));
+            writer.set_flush_on_finish_file(false);
+            let sub_writer = {
+                let mut writer = ZipWriter::new(Cursor::new(Vec::new()));
+                writer.set_flush_on_finish_file(false);
+                let sub_writer = {
+                    let mut writer = ZipWriter::new(Cursor::new(Vec::new()));
+                    writer.set_flush_on_finish_file(false);
+                    let options = FullFileOptions {
+                        compression_method: Stored,
+                        compression_level: None,
+                        last_modified_time: DateTime::from_date_and_time(2107, 4, 8, 14, 0, 19)?,
+                        permissions: None,
+                        large_file: false,
+                        encrypt_with: None,
+                        extended_options: ExtendedFileOptions {
+                            extra_data: vec![
+                                182, 180, 1, 0, 180, 182, 74, 0, 0, 200, 0, 0, 0, 2, 0, 0, 0,
+                            ]
+                            .into(),
+                            central_extra_data: vec![].into(),
+                        },
+                        alignment: 1542,
+                        ..Default::default()
+                    };
+                    writer.start_file_from_path("\0\0PK\u{6}\u{6}K\u{6}PK\u{3}\u{4}\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\u{1}\0\0\0\0\0\0\0\0\u{1}\u{1}\0PK\u{1}\u{2},\0\0\0\0\0\0\0\0\0\0\0P\u{7}\u{4}/.\0KP\0\0;\0\0\0\u{1e}\0\0\0\0\0\0\0\0\0\0\0\0\0", options)?;
+                    let finished = writer.finish_into_readable()?;
+                    assert_eq!(1, finished.file_names().count());
+                    writer = ZipWriter::new_append(finished.into_inner())?;
+                    let options = FullFileOptions {
+                        compression_method: Stored,
+                        compression_level: Some(5),
+                        last_modified_time: DateTime::from_date_and_time(2107, 4, 1, 0, 0, 0)?,
+                        permissions: None,
+                        large_file: false,
+                        encrypt_with: Some(ZipCrypto(
+                            ZipCryptoKeys::of(0x0, 0x62e4b50, 0x100),
+                            PhantomData,
+                        )),
+                        ..Default::default()
+                    };
+                    writer.add_symlink_from_path(
+                        "\0K\u{6}\0PK\u{6}\u{7}PK\u{6}\u{6}\0\0\0\0\0\0\0\0PK\u{2}\u{6}",
+                        "\u{8}\0\0\0\0/\0",
+                        options,
+                    )?;
+                    let finished = writer.finish_into_readable()?;
+                    assert_eq!(2, finished.file_names().count());
+                    writer = ZipWriter::new_append(finished.into_inner())?;
+                    assert_eq!(2, writer.files.len());
+                    writer
+                };
+                let finished = sub_writer.finish_into_readable()?;
+                assert_eq!(2, finished.file_names().count());
+                writer.merge_archive(finished)?;
+                writer = ZipWriter::new_append(writer.finish_into_readable()?.into_inner())?;
+                writer
+            };
+            writer.merge_archive(sub_writer.finish_into_readable()?)?;
+            writer
+        };
+        writer.merge_archive(sub_writer.finish_into_readable()?)?;
+        let _ = writer.finish_into_readable()?;
+        Ok(())
+    }
 }