Merge commit from fork

Author: Its-Just-Nans

src/aes.rs

@@ -6,7 +6,7 @@
 
 use crate::aes_ctr::AesCipher;
 use crate::format::aes::AesMode;
-use crate::result::ZipResult;
+use crate::result::{ZipResult, invalid};
 use crate::{aes_ctr, result::ZipError};
 use constant_time_eq::constant_time_eq;
 use hmac::{KeyInit, Mac, SimpleHmacReset};
@@ -140,15 +140,18 @@ pub struct AesReader<R> {
 }
 
 impl<R: Read> AesReader<R> {
-    pub const fn new(reader: R, aes_mode: AesMode, compressed_size: u64) -> AesReader<R> {
-        let data_length = compressed_size
-            - (PWD_VERIFY_LENGTH + AUTH_CODE_LENGTH + aes_mode.salt_length()) as u64;
-
-        Self {
+    pub fn new(reader: R, aes_mode: AesMode, compressed_size: u64) -> ZipResult<AesReader<R>> {
+        let metadata_len = (PWD_VERIFY_LENGTH + AUTH_CODE_LENGTH + aes_mode.salt_length()) as u64;
+        // we accept 0 size data
+        if compressed_size < metadata_len {
+            return Err(invalid!("Compressed size of AES data is invalid"));
+        }
+        let data_length = compressed_size - metadata_len;
+        Ok(Self {
             reader,
             aes_mode,
             data_length,
-        }
+        })
     }
 
     /// Read the AES header bytes and validate the password.
@@ -192,6 +195,7 @@ impl<R: Read> AesReader<R> {
 
         Ok(AesReaderValid {
             reader: self.reader,
+            data_length: self.data_length,
             data_remaining: self.data_length,
             cipher,
             hmac,
@@ -227,6 +231,7 @@ impl<R: Read> AesReader<R> {
 #[derive(Debug)]
 pub struct AesReaderValid<R: Read> {
     reader: R,
+    data_length: u64,
     data_remaining: u64,
     cipher: Cipher,
     hmac: SimpleHmacReset<Sha1>,
@@ -245,6 +250,10 @@ impl<R: Read> Read for AesReaderValid<R> {
     /// practically unusable, since its position after the error is not known.
     fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
         if self.data_remaining == 0 {
+            // handle case were payload is empty
+            if self.data_length == 0 {
+                self.verify_hmac()?;
+            }
             return Ok(0);
         }
 
@@ -262,38 +271,48 @@ impl<R: Read> Read for AesReaderValid<R> {
 
         // if there is no data left to read, check the integrity of the data
         if self.data_remaining == 0 {
-            debug_assert!(
-                !self.finalized,
-                "Tried to use an already finalized HMAC. This is a bug!"
-            );
-            if self.finalized {
-                return Err(Error::new(
-                    ErrorKind::InvalidData,
-                    "Tried to use an already finalized HMAC",
-                ));
-            }
-            self.finalized = true;
-
-            // Zip uses HMAC-Sha1-80, which only uses the first half of the hash
-            // see https://www.winzip.com/win/en/aes_info.html#auth-faq
-            let mut read_auth_code = [0; AUTH_CODE_LENGTH];
-            self.reader.read_exact(&mut read_auth_code)?;
-            let computed_auth_code = &self.hmac.finalize_reset().into_bytes()[0..AUTH_CODE_LENGTH];
-
-            // use constant time comparison to mitigate timing attacks
-            if !constant_time_eq(computed_auth_code, &read_auth_code) {
-                return Err(Error::new(
-                    ErrorKind::InvalidData,
-                    "Invalid authentication code, this could be due to an invalid password or errors in the data",
-                ));
-            }
+            self.verify_hmac()?;
+        } else if read == 0 && self.data_remaining > 0 {
+            return Err(Error::new(
+                ErrorKind::UnexpectedEof,
+                "Read 0 bytes of AES data but still contains remaining data",
+            ));
         }
 
         Ok(read)
     }
 }
 
 impl<R: Read> AesReaderValid<R> {
+    fn verify_hmac(&mut self) -> io::Result<()> {
+        debug_assert!(
+            !self.finalized,
+            "Tried to use an already finalized HMAC. This is a bug!"
+        );
+        if self.finalized {
+            return Err(Error::new(
+                ErrorKind::InvalidData,
+                "Tried to use an already finalized HMAC",
+            ));
+        }
+        self.finalized = true;
+
+        // Zip uses HMAC-Sha1-80, which only uses the first half of the hash
+        // see https://www.winzip.com/win/en/aes_info.html#auth-faq
+        let mut read_auth_code = [0; AUTH_CODE_LENGTH];
+        self.reader.read_exact(&mut read_auth_code)?;
+        let computed_auth_code = &self.hmac.finalize_reset().into_bytes()[0..AUTH_CODE_LENGTH];
+
+        // use constant time comparison to mitigate timing attacks
+        if !constant_time_eq(computed_auth_code, &read_auth_code) {
+            return Err(Error::new(
+                ErrorKind::InvalidData,
+                "Invalid authentication code, this could be due to an invalid password or errors in the data",
+            ));
+        }
+        Ok(())
+    }
+
     /// Consumes this decoder, returning the underlying reader.
     pub fn into_inner(self) -> R {
         self.reader
@@ -452,7 +471,7 @@ mod tests {
         {
             let compressed_length = buf.get_ref().len() as u64;
             let mut reader =
-                AesReader::new(&mut buf, aes_mode, compressed_length).validate(password)?;
+                AesReader::new(&mut buf, aes_mode, compressed_length)?.validate(password)?;
             reader.read_to_end(&mut read_buffer)?;
         }
 

src/read/readers.rs

@@ -215,7 +215,7 @@ pub(crate) fn make_crypto_reader<'a, R: Read + ?Sized>(
         }
         #[cfg(feature = "aes-crypto")]
         (Some(password), Some((aes_mode, _))) => CryptoReader::Aes {
-            reader: crate::aes::AesReader::new(reader, aes_mode, data.compressed_size)
+            reader: crate::aes::AesReader::new(reader, aes_mode, data.compressed_size)?
                 .validate(password)?,
         },
         (Some(password), None) => {

src/read/zip_archive.rs

@@ -248,7 +248,7 @@ impl<R: Read + Seek> ZipArchive<R> {
             None => Ok(None),
             Some((aes_mode, _)) => {
                 let (verification_value, salt) =
-                    crate::aes::AesReader::new(limit_reader, aes_mode, data.compressed_size)
+                    crate::aes::AesReader::new(limit_reader, aes_mode, data.compressed_size)?
                         .get_verification_value_and_salt()?;
                 let aes_info = crate::aes::AesInfo {
                     aes_mode,

tests/aes_encryption.rs

@@ -390,3 +390,97 @@ fn test_update_aes_version_on_threshold() {
     assert_eq!(data[167], 3); // AES Mode
     assert_eq!(data[168..170], [0, 0]); // Compression Method
 }
+
+const GENUINE: &[u8] = b"balance: 100 USD\n";
+const FORGED: &[u8] = b"balance: 999 USD\n";
+const AES256_OVERHEAD: usize = 28; // 16-byte salt + 2-byte verifier + 10-byte tag
+
+fn genuine_archive(plaintext: &[u8]) -> Vec<u8> {
+    use std::io::{Cursor, Write};
+    use zip::write::SimpleFileOptions;
+    use zip::{AesMode, CompressionMethod, ZipWriter};
+    let mut w = ZipWriter::new(Cursor::new(Vec::new()));
+    let opts = SimpleFileOptions::default()
+        .compression_method(CompressionMethod::Stored)
+        .with_aes_encryption_bytes(AesMode::Aes256, PASSWORD);
+    w.start_file("secret.txt", opts).unwrap();
+    w.write_all(plaintext).unwrap();
+    w.finish().unwrap().into_inner()
+}
+
+/// What a victim does: open with the correct password and read the entry.
+fn victim_reads(archive: &[u8]) -> std::io::Result<Vec<u8>> {
+    use std::io::{Cursor, Read};
+    use zip::ZipArchive;
+    let mut a = ZipArchive::new(Cursor::new(archive.to_vec())).unwrap();
+    let mut f = a.by_index_decrypt(0, PASSWORD).unwrap();
+    let mut out = Vec::new();
+    f.read_to_end(&mut out)?;
+    Ok(out)
+}
+
+fn u16le(b: &[u8], off: usize) -> usize {
+    u16::from_le_bytes([b[off], b[off + 1]]) as usize
+}
+
+/// Payload offset of the single local file header at offset 0.
+fn payload_start(a: &[u8]) -> usize {
+    30 + u16le(a, 26) + u16le(a, 28)
+}
+
+/// Control: on a well-formed archive the HMAC does reject forged ciphertext.
+#[test]
+fn tampering_is_detected_when_the_declared_size_is_honest() {
+    let mut a = genuine_archive(GENUINE);
+    let ct = payload_start(&a) + 18;
+    for i in 0..GENUINE.len() {
+        a[ct + i] ^= GENUINE[i] ^ FORGED[i]; // AES-CTR is malleable
+    }
+    let err = victim_reads(&a).unwrap_err();
+    assert!(err.to_string().contains("Invalid authentication code"));
+}
+
+/// The bypass: chosen plaintext is delivered verbatim, without the password.
+#[test]
+fn hmac_is_bypassed_when_the_entry_ends_early() {
+    let genuine = genuine_archive(GENUINE);
+    let hdr_len = payload_start(&genuine);
+    let eocd = genuine.len() - 22; // no archive comment yet
+    let cd_offset = u32::from_le_bytes(genuine[eocd + 16..eocd + 20].try_into().unwrap()) as usize;
+
+    // Park a second copy of the entry after the end-of-central-directory record,
+    // inside the archive comment, so its data ends at true EOF. Header, salt and
+    // password verifier are copied verbatim — no password knowledge is required.
+    let mut evil = genuine.clone();
+    let new_hdr_offset = evil.len() as u32;
+    evil.extend_from_slice(&genuine[..hdr_len + 18]); // local header + salt + verifier
+    for i in 0..GENUINE.len() {
+        // XOR the keystream onto the chosen plaintext; the 10-byte tag is omitted.
+        evil.push(genuine[hdr_len + 18 + i] ^ GENUINE[i] ^ FORGED[i]);
+    }
+    let comment_len = (evil.len() - genuine.len()) as u16;
+    evil[eocd + 20..eocd + 22].copy_from_slice(&comment_len.to_le_bytes());
+
+    // Repoint the central directory at the parked copy and declare more data than
+    // is present, so `data_remaining` can never reach zero.
+    let declared = (AES256_OVERHEAD + GENUINE.len() + 64) as u32;
+    evil[cd_offset + 42..cd_offset + 46].copy_from_slice(&new_hdr_offset.to_le_bytes());
+    evil[cd_offset + 20..cd_offset + 24].copy_from_slice(&declared.to_le_bytes());
+    let hdr2 = new_hdr_offset as usize;
+    evil[hdr2 + 18..hdr2 + 22].copy_from_slice(&declared.to_le_bytes());
+
+    let read_result = victim_reads(&evil);
+    // unauthenticated content was detected
+    assert!(read_result.is_err());
+}
+
+/// Second path: an entry with no payload bytes is never authenticated at all.
+#[test]
+fn hmac_is_never_checked_for_a_zero_length_entry() {
+    let mut a = genuine_archive(b"");
+    let tag = payload_start(&a) + 18;
+    a[tag] ^= 0xff; // corrupt the 10-byte authentication code
+    let read_result = victim_reads(&a);
+    // corrupt authentication code was detected
+    assert!(read_result.is_err());
+}