Skip to content

Commit 8068ab4

Browse files
ByGh00stPr0methean
andauthored
Merge commit from fork
* Add opt-in header consistency validation to ZipStreamReader (GHSA-m665-v7pm-7grj) * Enhance header consistency validation with offset sorting and metadata verification * Harden header consistency: account for byte spans, validate physical offsets, block UPath and SUID/SGID * Prevent files from overlapping with the central directory in has_overlap * Address review comments: factor metadata, align error messages, and add adversarial test vectors * Optimize metadata with Box<[u8]>, remove partial extraction rollback, and align Zombie ZIP test with real evasion * Disable filesystem tests on Miri, and skip zombie-ZIP test sooner when no compression methods are enabled Removed redundant check for default compression method before skipping the test. * feat: add header consistency validation to ZipArchive (GHSA-m665-v7pm-7grj follow-up) * Apply standard rustfmt formatting * Address review: decouple data descriptor validation, use borrowed metadata, and enforce strict ZIP64 size parity * refactor: add is_pending_data_descriptor to HeaderDataDescriptorMetadata * fix: strictly enforce raw filename equality and resolve local AES compression method * Address review: consolidate descriptor checks, use AsRef<[u8]> generics, update docstring, and enforce AES metadata parity * fix: enforce encryption flag, timestamp, and outer AES method consistency * Update comment for aes_extra_field_start field Clarified comment for aes_extra_field_start to indicate it is ignored by PartialEq. * Fix comment typo in AexEncryption struct * Deduplicate consistency metadata and harden against UnicodePath, duplicate-name, and overlap bypasses * Extract upath and upath_crc32 in a single scan * Remove unused imports and dead code warnings * test: disable header_consistency when only write-only deflate-zopfli is enabled * chore: trigger PR sync --------- Co-authored-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com>
1 parent 0b51128 commit 8068ab4

9 files changed

Lines changed: 1355 additions & 25 deletions

File tree

‎src/extra_fields/aex_encryption.rs‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,18 +11,26 @@ use crate::result::{ZipError, ZipResult, invalid, invalid_archive_const};
1111
use crate::unstable::LittleEndianReadExt;
1212

1313
/// AeX Encryption extra field
14-
#[derive(Copy, Clone, Debug, PartialEq)]
14+
#[derive(Copy, Clone, Debug, Eq)]
1515
pub struct AexEncryption {
1616
/// aes vendor
1717
pub(crate) aes_vendor_version: AesVendorVersion,
1818
/// aes mode
1919
pub(crate) aes_mode: AesMode,
2020
/// inner compression
2121
pub(crate) compression_method: CompressionMethod,
22-
/// aes extra field start index
22+
/// aes extra field start index (not written to the archive, therefore ignored by PartialEq)
2323
pub(crate) aes_extra_field_start: Option<usize>,
2424
}
2525

26+
impl PartialEq for AexEncryption {
27+
fn eq(&self, other: &Self) -> bool {
28+
self.aes_vendor_version == other.aes_vendor_version
29+
&& self.aes_mode == other.aes_mode
30+
&& self.compression_method == other.compression_method
31+
}
32+
}
33+
2634
impl AexEncryption {
2735
/// Field Header ID
2836
pub(crate) const EXTRA_FIELD_ID: u16 = UsedExtraField::AeXEncryption.as_u16();

‎src/extra_fields/extra_field.rs‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,11 @@ impl ExtraField {
239239

240240
impl ZipFileData {
241241
pub(crate) fn apply_extra_fields(&mut self, file_name_raw: &mut Vec<u8>) -> ZipResult<()> {
242+
#[cfg(feature = "aes-crypto")]
243+
let mut has_aes = false;
244+
let mut orig_name_crc = None;
245+
let mut orig_comment_crc = None;
246+
242247
for one_extra_field in &self.extra_fields.inner {
243248
match one_extra_field {
244249
// Zip64 extended information extra field
@@ -260,6 +265,12 @@ impl ZipFileData {
260265
ExtraField::AeXEncryption(AexEncryption {
261266
compression_method, ..
262267
}) => {
268+
if has_aes || self.compression_method != crate::CompressionMethod::Aes {
269+
return Err(invalid!(
270+
"AES extra field present without AES compression method (99)"
271+
));
272+
}
273+
has_aes = true;
263274
self.compression_method = *compression_method;
264275
}
265276
ExtraField::UnicodeComment(unicode) => {
@@ -268,7 +279,9 @@ impl ZipFileData {
268279
// If the CRC check fails, this Unicode Comment extra field SHOULD be ignored and
269280
// the File Comment field in the header SHOULD be used instead.
270281
// Check if the comment is UTF-8
271-
if unicode.is_crc32_valid(self.file_comment.as_bytes())
282+
let crc = *orig_comment_crc
283+
.get_or_insert_with(|| crc32fast::hash(self.file_comment.as_bytes()));
284+
if unicode.crc32 == crc
272285
&& let Ok(comment) = String::from_utf8(unicode.content.to_vec())
273286
{
274287
self.file_comment = comment.into_boxed_str();
@@ -280,9 +293,8 @@ impl ZipFileData {
280293
// APPNOTE 4.6.9 and https://libzip.org/specifications/extrafld.txt
281294
// If the CRC check fails, this UTF-8 Path Extra Field SHOULD be ignored and
282295
// the File Name field in the header SHOULD be used instead.
283-
if unicode.is_crc32_valid(file_name_raw)
284-
&& std::str::from_utf8(&unicode.content).is_ok()
285-
{
296+
let crc = *orig_name_crc.get_or_insert_with(|| crc32fast::hash(file_name_raw));
297+
if unicode.crc32 == crc && std::str::from_utf8(&unicode.content).is_ok() {
286298
*file_name_raw = unicode.content.to_vec();
287299
self.flags |= ZipFlags::LanguageEncoding.as_u16();
288300
}

‎src/extra_fields/zipinfo_utf8.rs‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use std::io::{Read, Write};
77
/// specified in APPNOTE 4.6.8 and 4.6.9
88
#[derive(Clone, Debug, PartialEq)]
99
pub struct UnicodeExtraField {
10-
crc32: u32,
10+
pub(crate) crc32: u32,
1111
pub(crate) content: Box<[u8]>,
1212
}
1313

@@ -30,12 +30,6 @@ impl UnicodeExtraField {
3030
Ok(self.content)
3131
}
3232

33-
/// Check if the crc32 is valid
34-
pub(crate) fn is_crc32_valid(&self, ascii_field: &[u8]) -> bool {
35-
let computed_crc32 = crc32fast::hash(ascii_field);
36-
self.crc32 == computed_crc32
37-
}
38-
3933
pub(crate) fn try_from_reader<R: Read>(reader: &mut R, len: u16) -> ZipResult<Self> {
4034
// Read and discard version byte
4135
reader.read_exact(&mut [0u8])?;

‎src/read/mod.rs‎

Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,8 @@ pub struct ZipReadOptions<'a> {
266266
/// The uncompressed size is not known (streamed entry with a data descriptor), so it
267267
/// can't bound the output.
268268
pub(crate) size_unknown: bool,
269+
/// Enforce consistency between the local file header and central directory header.
270+
pub(crate) enforce_header_consistency: bool,
269271
}
270272

271273
impl<'a> ZipReadOptions<'a> {
@@ -316,4 +318,179 @@ impl<'a> ZipReadOptions<'a> {
316318
self.force_crc = Some(crc);
317319
self
318320
}
321+
322+
/// Enforces that the local file header (LFH) matches the central directory header (CDFH).
323+
#[must_use]
324+
pub fn enforce_header_consistency(mut self, enforce: bool) -> Self {
325+
self.enforce_header_consistency = enforce;
326+
self
327+
}
328+
}
329+
330+
/// Helper to extract Info-ZIP UnicodePath (0x7075) extra field content and CRC32.
331+
pub(crate) fn get_upath(extra_fields: &ExtraFields) -> (Option<&[u8]>, Option<u32>) {
332+
extra_fields
333+
.inner
334+
.iter()
335+
.rev()
336+
.find_map(|f| match f {
337+
ExtraField::UnicodePath(u) => Some((&*u.content, u.crc32)),
338+
_ => None,
339+
})
340+
.unzip()
341+
}
342+
343+
/// Data descriptor / payload metadata (CRC32 and sizes) that can be verified either
344+
/// immediately or after reading a data descriptor.
345+
#[derive(Debug, Copy, Clone, PartialEq, Eq, Default)]
346+
pub(crate) struct HeaderDataDescriptorMetadata {
347+
pub(crate) crc32: u32,
348+
pub(crate) compressed_size: u64,
349+
pub(crate) uncompressed_size: u64,
350+
pub(crate) is_pending_data_descriptor: bool,
351+
}
352+
353+
impl HeaderDataDescriptorMetadata {
354+
/// Validates data descriptor fields between local and central headers.
355+
pub(crate) fn validate_matches(&self, central: &Self) -> ZipResult<()> {
356+
if self.crc32 != central.crc32 && (!self.is_pending_data_descriptor || self.crc32 != 0) {
357+
return Err(invalid!(
358+
"header inconsistency detected: CRC32 checksum mismatch (does not match central directory)"
359+
));
360+
}
361+
362+
if self.compressed_size != central.compressed_size
363+
&& (!self.is_pending_data_descriptor || self.compressed_size != 0)
364+
{
365+
return Err(invalid!(
366+
"header inconsistency detected: compressed size mismatch (does not match central directory)"
367+
));
368+
}
369+
370+
if self.uncompressed_size != central.uncompressed_size
371+
&& (!self.is_pending_data_descriptor || self.uncompressed_size != 0)
372+
{
373+
return Err(invalid!(
374+
"header inconsistency detected: uncompressed size mismatch (does not match central directory)"
375+
));
376+
}
377+
378+
Ok(())
379+
}
380+
}
381+
382+
/// Unified header metadata used to validate consistency between Local and Central headers.
383+
#[derive(Debug, Clone, PartialEq)]
384+
pub(crate) struct HeaderConsistencyMetadata<T: AsRef<[u8]>, U: AsRef<[u8]> = T> {
385+
pub(crate) name: T,
386+
pub(crate) upath: Option<U>,
387+
pub(crate) upath_crc32: Option<u32>,
388+
pub(crate) compression_method: u16,
389+
pub(crate) is_encrypted: bool,
390+
pub(crate) is_using_data_descriptor: bool,
391+
pub(crate) last_modified_time: Option<DateTime>,
392+
pub(crate) data: HeaderDataDescriptorMetadata,
393+
#[cfg(feature = "aes-crypto")]
394+
pub(crate) aes: Option<crate::extra_fields::AexEncryption>,
395+
}
396+
397+
impl HeaderConsistencyMetadata<&[u8]> {
398+
pub(crate) fn into_owned(self) -> HeaderConsistencyMetadata<Box<[u8]>> {
399+
HeaderConsistencyMetadata {
400+
name: self.name.into(),
401+
upath: self.upath.map(Into::into),
402+
upath_crc32: self.upath_crc32,
403+
compression_method: self.compression_method,
404+
is_encrypted: self.is_encrypted,
405+
is_using_data_descriptor: self.is_using_data_descriptor,
406+
last_modified_time: self.last_modified_time,
407+
data: self.data,
408+
#[cfg(feature = "aes-crypto")]
409+
aes: self.aes,
410+
}
411+
}
412+
}
413+
414+
impl<T: AsRef<[u8]>, U: AsRef<[u8]>> HeaderConsistencyMetadata<T, U> {
415+
/// Validates header identity (filename, UnicodePath, compression method, and flags).
416+
pub(crate) fn validate_identity_matches<T2: AsRef<[u8]>, U2: AsRef<[u8]>>(
417+
&self,
418+
central: &HeaderConsistencyMetadata<T2, U2>,
419+
) -> ZipResult<()> {
420+
let local_effective = self
421+
.upath
422+
.as_ref()
423+
.map(AsRef::as_ref)
424+
.unwrap_or_else(|| self.name.as_ref());
425+
426+
if self.upath.as_ref().map(AsRef::as_ref) != central.upath.as_ref().map(AsRef::as_ref)
427+
|| self.upath_crc32 != central.upath_crc32
428+
{
429+
return Err(invalid!(
430+
"header inconsistency detected: UnicodePath (0x7075) extra field mismatch for '{}'",
431+
String::from_utf8_lossy(local_effective)
432+
));
433+
}
434+
435+
if self.name.as_ref() != central.name.as_ref() {
436+
return Err(invalid!(
437+
"header inconsistency detected: Local file header filename mismatch (does not match central directory)"
438+
));
439+
}
440+
441+
if self.compression_method != central.compression_method {
442+
return Err(invalid!(
443+
"header inconsistency detected: compression method mismatch (does not match central directory)"
444+
));
445+
}
446+
447+
if self.is_encrypted != central.is_encrypted {
448+
return Err(invalid!(
449+
"header inconsistency detected: encryption flag mismatch for '{}'",
450+
String::from_utf8_lossy(self.name.as_ref())
451+
));
452+
}
453+
454+
if self.is_using_data_descriptor != central.is_using_data_descriptor {
455+
return Err(invalid!(
456+
"header inconsistency detected: data descriptor flag mismatch for '{}'",
457+
String::from_utf8_lossy(self.name.as_ref())
458+
));
459+
}
460+
461+
if self.last_modified_time != central.last_modified_time {
462+
return Err(invalid!(
463+
"header inconsistency detected: last modified time mismatch for '{}'",
464+
String::from_utf8_lossy(self.name.as_ref())
465+
));
466+
}
467+
468+
#[cfg(feature = "aes-crypto")]
469+
if self.aes != central.aes {
470+
return Err(invalid!(
471+
"header inconsistency detected: AES encryption metadata mismatch for '{}'",
472+
String::from_utf8_lossy(self.name.as_ref())
473+
));
474+
}
475+
476+
Ok(())
477+
}
478+
479+
/// Validates data descriptor and payload consistency.
480+
pub(crate) fn validate_data_matches<T2: AsRef<[u8]>, U2: AsRef<[u8]>>(
481+
&self,
482+
central: &HeaderConsistencyMetadata<T2, U2>,
483+
) -> ZipResult<()> {
484+
self.data.validate_matches(&central.data)
485+
}
486+
487+
/// Validates that local header metadata matches central directory metadata.
488+
pub(crate) fn validate_matches<T2: AsRef<[u8]>, U2: AsRef<[u8]>>(
489+
&self,
490+
central: &HeaderConsistencyMetadata<T2, U2>,
491+
) -> ZipResult<()> {
492+
self.validate_identity_matches(central)?;
493+
self.validate_data_matches(central)?;
494+
Ok(())
495+
}
319496
}

0 commit comments

Comments
 (0)