From 4065f0501f97865f75ae459664c065e2b07b8828 Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Mon, 17 Jun 2024 17:44:34 -0700 Subject: [PATCH] fix: Always search for data start when opening an archive for append, and reject the header if data appears to start after central directory --- src/read.rs | 35 +++++++++++++------------ src/write.rs | 74 ++++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 88 insertions(+), 21 deletions(-) diff --git a/src/read.rs b/src/read.rs index 842bb449..e2ebf97b 100644 --- a/src/read.rs +++ b/src/read.rs @@ -284,7 +284,7 @@ fn find_data_start( // If the value was already set in the meantime, ensure it matches (this is probably // unnecessary). Err(_) => { - assert_eq!(*data.data_start.get().unwrap(), data_start); + debug_assert_eq!(*data.data_start.get().unwrap(), data_start); } } Ok(data_start) @@ -730,25 +730,21 @@ impl ZipArchive { } else { dir_info.number_of_files }; + if dir_info.disk_number != dir_info.disk_with_central_directory { + return unsupported_zip_error("Support for multi-disk files is not implemented"); + } let mut files = Vec::with_capacity(file_capacity); reader.seek(io::SeekFrom::Start(dir_info.directory_start))?; for _ in 0..dir_info.number_of_files { let file = central_header_to_zip_file(reader, dir_info.archive_offset)?; - let central_end = reader.stream_position()?; - find_data_start(&file, reader)?; - reader.seek(SeekFrom::Start(central_end))?; files.push(file); } - if dir_info.disk_number != dir_info.disk_with_central_directory { - unsupported_zip_error("Support for multi-disk files is not implemented") - } else { - Ok(SharedBuilder { - files, - offset: dir_info.archive_offset, - dir_start: dir_info.directory_start, - config, - }) - } + Ok(SharedBuilder { + files, + offset: dir_info.archive_offset, + dir_start: dir_info.directory_start, + config, + }) } fn sort_result( @@ -1128,7 +1124,15 @@ pub(crate) fn central_header_to_zip_file( // Parse central header let block = ZipCentralEntryBlock::parse(reader)?; - central_header_to_zip_file_inner(reader, archive_offset, central_header_start, block) + let file = central_header_to_zip_file_inner(reader, archive_offset, central_header_start, block)?; + let central_header_end = reader.stream_position()?; + let data_start = find_data_start(&file, reader)?; + if data_start > central_header_start { + return Err(InvalidArchive("A file can't start after its central-directory header")); + } + file.data_start.get_or_init(|| data_start); + reader.seek(SeekFrom::Start(central_header_end))?; + Ok(file) } #[inline] @@ -1173,7 +1177,6 @@ fn central_header_to_zip_file_inner( let file_name_raw = read_variable_length_byte_field(reader, file_name_length as usize)?; let extra_field = read_variable_length_byte_field(reader, extra_field_length as usize)?; let file_comment_raw = read_variable_length_byte_field(reader, file_comment_length as usize)?; - let file_name: Box = match is_utf8 { true => String::from_utf8_lossy(&file_name_raw).into(), false => file_name_raw.clone().from_cp437(), diff --git a/src/write.rs b/src/write.rs index e19cb64a..32d7ad52 100644 --- a/src/write.rs +++ b/src/write.rs @@ -3105,6 +3105,7 @@ mod test { fn test_fuzz_crash_2024_06_17a() -> ZipResult<()> { let mut writer = ZipWriter::new(Cursor::new(Vec::new())); writer.set_flush_on_finish_file(false); + const PATH_1: &'static str = "\0I\01\0P\0\0\u{2}\0\0\u{1a}\u{1a}\u{1a}\u{1a}\u{1b}\u{1a}UT\u{5}\0\0\u{1a}\u{1a}\u{1a}\u{1a}UT\u{5}\0\u{1}\0\u{1a}\u{1a}\u{1a}UT\t\0uc\u{5}\0\0\0\0\u{7f}\u{7f}\u{7f}\u{7f}PK\u{6}"; let sub_writer = { let mut writer = ZipWriter::new(Cursor::new(Vec::new())); writer.set_flush_on_finish_file(false); @@ -3120,18 +3121,81 @@ mod test { writer.merge_archive(sub_writer.finish_into_readable()?)?; writer = ZipWriter::new_append(writer.finish_into_readable()?.into_inner())?; let options = FileOptions { compression_method: Stored, compression_level: None, last_modified_time: DateTime::from_date_and_time(1980, 11, 14, 10, 46, 47)?, permissions: None, large_file: false, encrypt_with: None, extended_options: ExtendedFileOptions {extra_data: vec![].into(), central_extra_data: vec![].into()}, alignment: 0, ..Default::default() }; - writer.start_file_from_path("\0I\01\0P\0\0\u{2}\0\0\u{1a}\u{1a}\u{1a}\u{1a}\u{1b}\u{1a}UT\u{5}\0\0\u{1a}\u{1a}\u{1a}\u{1a}UT\u{5}\0\u{1}\0\u{1a}\u{1a}\u{1a}UT\t\0uc\u{5}\0\0\0\0\u{7f}\u{7f}\u{7f}\u{7f}PK\u{6}", options)?; - writer.deep_copy_file_from_path("\0I\01\0P\0\0\u{2}\0\0\u{1a}\u{1a}\u{1a}\u{1a}\u{1b}\u{1a}UT\u{5}\0\0\u{1a}\u{1a}\u{1a}\u{1a}UT\u{5}\0\u{1}\0\u{1a}\u{1a}\u{1a}UT\t\0uc\u{5}\0\0\0\0\u{7f}\u{7f}\u{7f}\u{7f}PK\u{6}", "eee\u{6}\0\0\0\0\0\0\0\0\0\0\0$\0\0\0\0\0\0\u{7f}\u{7f}PK\u{6}\u{6}K\u{6}\u{6}\u{6}\0\0\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}\0\0PK\u{1}\u{1e},\0\0\0\0\0\0\0\0\0\0\0\u{8}\0*\0\0\u{1}PK\u{6}\u{7}PK\u{6}\u{6}\0\0\0\0\0\0\0\0}K\u{2}\u{6}")?; + writer.start_file_from_path(PATH_1, options)?; + writer.deep_copy_file_from_path(PATH_1, "eee\u{6}\0\0\0\0\0\0\0\0\0\0\0$\0\0\0\0\0\0\u{7f}\u{7f}PK\u{6}\u{6}K\u{6}\u{6}\u{6}\0\0\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}\0\0PK\u{1}\u{1e},\0\0\0\0\0\0\0\0\0\0\0\u{8}\0*\0\0\u{1}PK\u{6}\u{7}PK\u{6}\u{6}\0\0\0\0\0\0\0\0}K\u{2}\u{6}")?; writer }; writer.merge_archive(sub_writer.finish_into_readable()?)?; writer = ZipWriter::new_append(writer.finish_into_readable()?.into_inner())?; - writer.deep_copy_file_from_path("", "copy")?; + writer.deep_copy_file_from_path(PATH_1, "")?; writer = ZipWriter::new_append(writer.finish_into_readable()?.into_inner())?; writer.shallow_copy_file_from_path("", "copy")?; writer = ZipWriter::new_append(writer.finish_into_readable()?.into_inner())?; - writer.deep_copy_file_from_path("", "copy")?; - writer = ZipWriter::new_append(writer.finish_into_readable()?.into_inner())?; + let _ = writer.finish_into_readable()?; + Ok(()) + } + + #[test] + #[cfg(feature = "bzip2")] + fn test_fuzz_crash_2024_06_17b() -> ZipResult<()> { + 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 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 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 = FileOptions { compression_method: Stored, compression_level: None, last_modified_time: DateTime::from_date_and_time(1981, 1, 1, 0, 0, 21)?, permissions: Some(16908288), large_file: false, encrypt_with: None, extended_options: ExtendedFileOptions {extra_data: vec![].into(), central_extra_data: vec![].into()}, alignment: 20555, ..Default::default() }; + writer.start_file_from_path("\0\u{7}\u{1}\0\0\0\0\0\0\0\0\u{1}\0\0PK\u{1}\u{2};\u{1a}\u{18}\u{1a}UT\t.........................\0u", options)?; + writer + }; + writer.merge_archive(sub_writer.finish_into_readable()?)?; + let options = FileOptions { compression_method: CompressionMethod::Bzip2, compression_level: Some(5), last_modified_time: DateTime::from_date_and_time(2055, 7, 7, 3, 6, 6)?, permissions: None, large_file: false, encrypt_with: None, extended_options: ExtendedFileOptions {extra_data: vec![].into(), central_extra_data: vec![].into()}, alignment: 0, ..Default::default() }; + writer.start_file_from_path("\0\0\0\0..\0\0\0\0\0\u{7f}\u{7f}PK\u{6}\u{6}K\u{6}\u{6}\u{6}\0\0\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}\0\0PK\u{1}\u{1e},\0\0\0\0\0\0\0\0\0\0\0\u{8}\0*\0\0\u{1}PK\u{6}\u{7}PK\u{6}\u{6}\0\0\0\0\0\0\0\0}K\u{2}\u{6}", options)?; + writer = ZipWriter::new_append(writer.finish_into_readable()?.into_inner())?; + writer + }; + writer.merge_archive(sub_writer.finish_into_readable()?)?; + writer = ZipWriter::new_append(writer.finish_into_readable()?.into_inner())?; + writer + }; + writer.merge_archive(sub_writer.finish_into_readable()?)?; + writer = ZipWriter::new_append(writer.finish_into_readable()?.into_inner())?; + writer + }; + writer.merge_archive(sub_writer.finish_into_readable()?)?; + 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()?)?; + writer + }; + writer.merge_archive(sub_writer.finish_into_readable()?)?; + writer + }; + writer.merge_archive(sub_writer.finish_into_readable()?)?; let _ = writer.finish_into_readable()?; Ok(()) }