Skip to content

Commit 59e59b1

Browse files
Pr0metheangithub-code-quality[bot]amazon-q-developer[bot]
authored
refactor: Use FixedSizeBlock trait to serialize AesExtraField (#640)
* Apply suggested fix to src/write.rs from Copilot Autofix Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> * Apply suggested fix to src/write.rs from Copilot Autofix Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> * Apply suggested fix to src/write.rs from Copilot Autofix Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> * Apply suggested fix to src/write.rs from Copilot Autofix Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> * Apply suggested fix to src/write.rs from Copilot Autofix Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> * Update src/write.rs Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com> Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> * Update AesMode reference in AesExtraField struct Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> * Refactor: use FixedSizeBlock trait to serialize AesExtraField * cargo fmt --all * [skip ci] fix: Add repr(packed, C) to AesExtraField to fix test failures The failing tests `write::test::fuzz_crash_2024_07_19a` and `write::test::test_fuzz_failure_2024_05_08` were encountering "InvalidArchive("Invalid AES encryption strength")" errors because the `AesExtraField` struct was missing the `#[repr(packed, C)]` attribute. Without this attribute, the struct had implicit padding between the `aes_mode` (u8) field and the `compression_method` (u16) field, causing the binary serialization to not match the expected ZIP AES extra field format. This led to incorrect byte offsets when reading the aes_mode value, resulting in invalid AES encryption strength values. This fix ensures the struct is properly packed without padding, matching the binary format expected by the ZIP specification. * Fix: #[repr(packed, C)] applies to struct, not member * Fix: needs to understand AesExtraField even without aes-crypto feature enabled --------- Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
1 parent 3935182 commit 59e59b1

3 files changed

Lines changed: 130 additions & 34 deletions

File tree

‎src/spec.rs‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -225,9 +225,10 @@ pub(crate) unsafe trait Pod: Copy + 'static {
225225
}
226226

227227
pub(crate) trait FixedSizeBlock: Pod {
228-
const MAGIC: Magic;
228+
type Magic: Copy + Eq;
229+
const MAGIC: Self::Magic;
229230

230-
fn magic(self) -> Magic;
231+
fn magic(self) -> Self::Magic;
231232

232233
const WRONG_MAGIC_ERROR: ZipError;
233234

@@ -252,7 +253,7 @@ pub(crate) trait FixedSizeBlock: Pod {
252253

253254
fn to_le(self) -> Self;
254255

255-
fn write<T: Write>(self, writer: &mut T) -> ZipResult<()> {
256+
fn write<T: Write + ?Sized>(self, writer: &mut T) -> ZipResult<()> {
256257
let block = self.to_le();
257258
writer.write_all(block.as_bytes())?;
258259
Ok(())
@@ -290,6 +291,7 @@ macro_rules! to_le {
290291
/* TODO: derive macro to generate these fields? */
291292
/// Implement `from_le()` and `to_le()`, providing the field specification to both macros
292293
/// and methods.
294+
#[macro_export]
293295
macro_rules! to_and_from_le {
294296
($($args:tt),+ $(,)?) => {
295297
#[inline(always)]
@@ -321,6 +323,7 @@ pub(crate) struct Zip32CDEBlock {
321323
unsafe impl Pod for Zip32CDEBlock {}
322324

323325
impl FixedSizeBlock for Zip32CDEBlock {
326+
type Magic = Magic;
324327
const MAGIC: Magic = Magic::CENTRAL_DIRECTORY_END_SIGNATURE;
325328

326329
#[inline(always)]
@@ -440,6 +443,7 @@ pub(crate) struct Zip64CDELocatorBlock {
440443
unsafe impl Pod for Zip64CDELocatorBlock {}
441444

442445
impl FixedSizeBlock for Zip64CDELocatorBlock {
446+
type Magic = Magic;
443447
const MAGIC: Magic = Magic::ZIP64_CENTRAL_DIRECTORY_END_LOCATOR_SIGNATURE;
444448

445449
#[inline(always)]
@@ -517,6 +521,7 @@ pub(crate) struct Zip64CDEBlock {
517521
unsafe impl Pod for Zip64CDEBlock {}
518522

519523
impl FixedSizeBlock for Zip64CDEBlock {
524+
type Magic = Magic;
520525
const MAGIC: Magic = Magic::ZIP64_CENTRAL_DIRECTORY_END_SIGNATURE;
521526

522527
fn magic(self) -> Magic {
@@ -897,6 +902,7 @@ mod test {
897902
unsafe impl Pod for TestBlock {}
898903

899904
impl FixedSizeBlock for TestBlock {
905+
type Magic = Magic;
900906
const MAGIC: Magic = Magic::literal(0x01111);
901907

902908
fn magic(self) -> Magic {

‎src/types.rs‎

Lines changed: 65 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
use crate::cfg_if_expr;
33
use crate::cp437::FromCp437;
44
use crate::result::{ZipError, ZipResult, invalid};
5-
use crate::spec::{self, FixedSizeBlock, Pod, ZipFlags};
5+
use crate::spec::{self, FixedSizeBlock, Magic, Pod, ZipFlags};
66
use crate::write::FileOptionExtension;
77
use crate::zipcrypto::EncryptWith;
88
use core::fmt::{self, Debug, Formatter};
@@ -18,7 +18,7 @@ pub(crate) mod ffi {
1818
pub const S_IFLNK: u32 = 0o0120000;
1919
}
2020

21-
use crate::extra_fields::ExtraField;
21+
use crate::extra_fields::{ExtraField, UsedExtraField};
2222
use crate::read::find_data_start;
2323
use crate::result::DateTimeRangeError;
2424
use crate::spec::is_dir;
@@ -1067,17 +1067,18 @@ pub(crate) struct ZipCentralEntryBlock {
10671067
unsafe impl Pod for ZipCentralEntryBlock {}
10681068

10691069
impl FixedSizeBlock for ZipCentralEntryBlock {
1070-
const MAGIC: spec::Magic = spec::Magic::CENTRAL_DIRECTORY_HEADER_SIGNATURE;
1070+
type Magic = Magic;
1071+
const MAGIC: Magic = Magic::CENTRAL_DIRECTORY_HEADER_SIGNATURE;
10711072

10721073
#[inline(always)]
1073-
fn magic(self) -> spec::Magic {
1074+
fn magic(self) -> Magic {
10741075
self.magic
10751076
}
10761077

10771078
const WRONG_MAGIC_ERROR: ZipError = invalid!("Invalid Central Directory header");
10781079

10791080
to_and_from_le![
1080-
(magic, spec::Magic),
1081+
(magic, Magic),
10811082
(version_made_by, u16),
10821083
(version_to_extract, u16),
10831084
(flags, u16),
@@ -1116,17 +1117,18 @@ pub(crate) struct ZipLocalEntryBlock {
11161117
unsafe impl Pod for ZipLocalEntryBlock {}
11171118

11181119
impl FixedSizeBlock for ZipLocalEntryBlock {
1119-
const MAGIC: spec::Magic = spec::Magic::LOCAL_FILE_HEADER_SIGNATURE;
1120+
type Magic = Magic;
1121+
const MAGIC: Magic = Magic::LOCAL_FILE_HEADER_SIGNATURE;
11201122

11211123
#[inline(always)]
1122-
fn magic(self) -> spec::Magic {
1124+
fn magic(self) -> Magic {
11231125
self.magic
11241126
}
11251127

11261128
const WRONG_MAGIC_ERROR: ZipError = invalid!("Invalid local file header");
11271129

11281130
to_and_from_le![
1129-
(magic, spec::Magic),
1131+
(magic, Magic),
11301132
(version_made_by, u16),
11311133
(flags, u16),
11321134
(compression_method, u16),
@@ -1239,17 +1241,18 @@ pub(crate) struct ZipDataDescriptorBlock {
12391241
unsafe impl Pod for ZipDataDescriptorBlock {}
12401242

12411243
impl FixedSizeBlock for ZipDataDescriptorBlock {
1242-
const MAGIC: spec::Magic = spec::Magic::DATA_DESCRIPTOR_SIGNATURE;
1244+
type Magic = Magic;
1245+
const MAGIC: Magic = Magic::DATA_DESCRIPTOR_SIGNATURE;
12431246

12441247
#[inline(always)]
1245-
fn magic(self) -> spec::Magic {
1248+
fn magic(self) -> Magic {
12461249
self.magic
12471250
}
12481251

12491252
const WRONG_MAGIC_ERROR: ZipError = invalid!("Invalid data descriptor header");
12501253

12511254
to_and_from_le![
1252-
(magic, spec::Magic),
1255+
(magic, Magic),
12531256
(crc32, u32),
12541257
(compressed_size, u32),
12551258
(uncompressed_size, u32),
@@ -1268,6 +1271,7 @@ pub(crate) struct Zip64DataDescriptorBlock {
12681271
unsafe impl Pod for Zip64DataDescriptorBlock {}
12691272

12701273
impl FixedSizeBlock for Zip64DataDescriptorBlock {
1274+
type Magic = Magic;
12711275
const MAGIC: spec::Magic = spec::Magic::DATA_DESCRIPTOR_SIGNATURE;
12721276

12731277
#[inline(always)]
@@ -1328,6 +1332,56 @@ impl AesMode {
13281332
}
13291333
}
13301334

1335+
#[derive(Copy, Clone)]
1336+
#[repr(packed, C)]
1337+
pub(crate) struct AesExtraField {
1338+
header_id: u16,
1339+
data_size: u16,
1340+
version: u16,
1341+
vendor_id: u16,
1342+
aes_mode: u8,
1343+
compression_method: u16,
1344+
}
1345+
1346+
unsafe impl Pod for AesExtraField {}
1347+
1348+
impl FixedSizeBlock for AesExtraField {
1349+
type Magic = u16;
1350+
const MAGIC: Self::Magic = UsedExtraField::AeXEncryption as u16;
1351+
1352+
fn magic(self) -> Self::Magic {
1353+
Self::MAGIC
1354+
}
1355+
1356+
const WRONG_MAGIC_ERROR: ZipError = invalid!("Wrong AES header ID");
1357+
1358+
to_and_from_le![
1359+
(header_id, u16),
1360+
(data_size, u16),
1361+
(version, u16),
1362+
(vendor_id, u16),
1363+
(aes_mode, u8),
1364+
(compression_method, u16)
1365+
];
1366+
}
1367+
1368+
impl AesExtraField {
1369+
pub(crate) fn new(
1370+
version: AesVendorVersion,
1371+
aes_mode: AesMode,
1372+
compression_method: CompressionMethod,
1373+
) -> Self {
1374+
Self {
1375+
header_id: UsedExtraField::AeXEncryption as u16,
1376+
data_size: 7,
1377+
version: version as u16,
1378+
vendor_id: u16::from_le_bytes(*b"AE"),
1379+
aes_mode: aes_mode as u8,
1380+
compression_method: compression_method.serialize_to_u16(),
1381+
}
1382+
}
1383+
}
1384+
13311385
#[cfg(test)]
13321386
mod test {
13331387
#[test]

‎src/write.rs‎

Lines changed: 56 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ use crate::result::{ZipError, ZipResult, invalid};
77
use crate::spec::{self, FixedSizeBlock, Zip32CDEBlock};
88
use crate::types::ffi::S_IFLNK;
99
use crate::types::{
10-
AesVendorVersion, DateTime, MIN_VERSION, Zip64ExtraFieldBlock, ZipFileData, ZipLocalEntryBlock,
11-
ZipRawValues, ffi,
10+
AesExtraField, AesVendorVersion, DateTime, MIN_VERSION, Zip64ExtraFieldBlock, ZipFileData,
11+
ZipLocalEntryBlock, ZipRawValues, ffi,
1212
};
1313
use core::default::Default;
1414
use core::fmt::{Debug, Formatter};
@@ -253,6 +253,28 @@ pub struct ExtendedFileOptions {
253253

254254
impl ExtendedFileOptions {
255255
/// Adds an extra data field, unless we detect that it's invalid.
256+
///
257+
/// # Parameters
258+
///
259+
/// * `header_id` – The 2‑byte identifier of the ZIP extra field to add.
260+
/// This value determines the type/format of `data` and should either be
261+
/// one of the standard ZIP extra field IDs defined by the ZIP
262+
/// specification or an application‑specific (vendor) ID.
263+
/// * `data` – The raw payload for the extra field, without the leading
264+
/// header ID or length; those are derived from `header_id` and
265+
/// `data.len()` and written automatically.
266+
/// * `central_only` – Controls where the extra field is stored:
267+
/// * When `true`, the field is appended only to the central directory
268+
/// extra data (`central_extra_data`), and the corresponding local file
269+
/// header is left unchanged.
270+
/// * When `false`, the field is appended to the local file header extra
271+
/// data (`extra_data`) and may also be reflected in the central
272+
/// directory, depending on how the ZIP is written.
273+
///
274+
/// The combined size of all extra data (local + central) must not exceed
275+
/// `u16::MAX`. If adding this field would exceed that limit or produce an
276+
/// invalid extra data structure, an error is returned and no data is
277+
/// added.
256278
pub fn add_extra_data<D: AsRef<[u8]>>(
257279
&mut self,
258280
header_id: u16,
@@ -276,6 +298,17 @@ impl ExtendedFileOptions {
276298
}
277299
}
278300

301+
/// Appends an extra data field to the given buffer without enforcing global size limits.
302+
///
303+
/// Unlike [`ExtendedFileOptions::add_extra_data`], this function:
304+
/// - Does **not** check that the combined size of local and central extra data fits into
305+
/// `u16::MAX` bytes as required by the ZIP format.
306+
/// - Does **not** re-validate the entire extra-data block after appending.
307+
///
308+
/// Callers must ensure that:
309+
/// - Using this function will not cause the overall extra-data length to exceed `u16::MAX`.
310+
/// - The resulting buffer remains a well-formed sequence of extra fields (or is validated
311+
/// later with [`Self::validate_extra_data`]).
279312
pub(crate) fn add_extra_data_unchecked(
280313
vec: &mut Vec<u8>,
281314
header_id: u16,
@@ -939,6 +972,21 @@ impl<W: Write + Seek> ZipWriter<W> {
939972
///
940973
/// # Safety
941974
///
975+
/// This function assumes that `length` and `crc32` accurately describe the data that has
976+
/// been (or will be) written for the currently open file entry.
977+
///
978+
/// The caller must ensure that:
979+
/// - A file entry is currently being written (that is, [`start_file`] or equivalent has
980+
/// been called successfully and `abort_file` has not been called since).
981+
/// - `length` is the exact uncompressed size, in bytes, of the file data written to the
982+
/// underlying [`Write`] implementation for this entry.
983+
/// - `crc32` is the correct CRC-32 checksum for that same data, in the format expected by
984+
/// the ZIP specification.
985+
///
986+
/// If these requirements are not met, the generated ZIP archive may contain inconsistent
987+
/// or corrupt metadata, which can cause readers to report errors, skip data, or accept
988+
/// data whose integrity cannot be verified.
989+
///
942990
/// This overwrites the internal crc32 calculation. It should only be used in case
943991
/// the underlying [Write] is written independently and you need to adjust the zip metadata.
944992
pub unsafe fn set_file_metadata(&mut self, length: u64, crc32: u32) -> ZipResult<()> {
@@ -1302,8 +1350,6 @@ impl<W: Write + Seek> ZipWriter<W> {
13021350
Ok(())
13031351
}
13041352

1305-
/* TODO: link to/use Self::finish_into_readable() from https://github.com/zip-rs/zip/pull/400 in
1306-
* this docstring. */
13071353
/// Copy over the entire contents of another archive verbatim.
13081354
///
13091355
/// This method extracts file metadata from the `source` archive, then simply performs a single
@@ -2166,22 +2212,11 @@ fn update_aes_extra_data<W: Write + Seek>(
21662212
extra_data_start + file.aes_extra_data_start,
21672213
))?;
21682214

2169-
let mut buf = Vec::new();
2170-
2171-
/* TODO: implement this using the Block trait! */
2172-
// Extra field header ID.
2173-
buf.write_u16_le(UsedExtraField::AeXEncryption as u16)?;
2174-
// Data size.
2175-
buf.write_u16_le(7)?;
2176-
// Integer version number.
2177-
buf.write_u16_le(*version as u16)?;
2178-
// Vendor ID.
2179-
buf.write_all(b"AE")?;
2180-
// AES encryption strength.
2181-
buf.write_all(&[*aes_mode as u8])?;
2182-
// Real compression method.
2183-
buf.write_u16_le(compression_method.serialize_to_u16())?;
2215+
let mut buf = [0u8; size_of::<AesExtraField>()];
2216+
2217+
let aes_extra_field = AesExtraField::new(*version, *aes_mode, *compression_method);
21842218

2219+
aes_extra_field.write(&mut buf.as_mut())?;
21852220
writer.write_all(&buf)?;
21862221

21872222
let aes_extra_data_start = file.aes_extra_data_start as usize;
@@ -2191,7 +2226,8 @@ fn update_aes_extra_data<W: Write + Seek>(
21912226
));
21922227
};
21932228
let extra_field = Arc::make_mut(extra_field);
2194-
extra_field[aes_extra_data_start..aes_extra_data_start + buf.len()].copy_from_slice(&buf);
2229+
extra_field[aes_extra_data_start..aes_extra_data_start + size_of::<AesExtraField>()]
2230+
.copy_from_slice(&buf);
21952231

21962232
Ok(())
21972233
}

0 commit comments

Comments
 (0)