Integer overflow with header_start += archive_offset when reading file with malformed header

[5225225]

Found through fuzzing.

Reproduction program (sorry for the large blob of bytes, I don't really understand the zip format enough to cut it down):

fn main() {
    let f = [0, 80, 75, 1, 2, 127, 120, 0, 3, 3, 75, 80, 232, 3, 0, 0, 0, 0, 0, 0, 3, 0, 1, 0, 7,
    0, 0, 0, 0, 65, 0, 1, 0, 0, 0, 4, 0, 0, 224, 255, 0, 255, 255, 255, 255, 255, 255, 20, 39, 221,
    221, 221, 221, 221, 221, 205, 221, 221, 221, 42, 221, 221, 221, 221, 221, 221, 221, 221, 38,
    34, 34, 219, 80, 75, 5, 6, 0, 0, 0, 0, 5, 96, 0, 1, 71, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 234,
    236, 124, 221, 221, 37, 221, 221, 221, 221, 221, 129, 4, 0, 0, 221, 221, 80, 75, 1, 2, 127,
    120, 0, 4, 0, 0, 2, 127, 120, 0, 79, 75, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
    255, 0, 0, 234, 0, 0, 0, 3, 8, 4, 232, 3, 0, 0, 0, 255, 255, 255, 255, 1, 0, 0, 0, 0, 7, 0, 0,
    0, 0, 3, 0, 221, 209, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
    255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 58, 58, 42, 75,
    9, 2, 127, 120, 0, 99, 99, 99, 99, 99, 99, 94, 7, 0, 0, 0, 0, 0, 0, 213, 213, 213, 213, 213,
    213, 213, 213, 213, 7, 0, 0, 211, 211, 211, 211, 124, 236, 99, 99, 99, 94, 7, 0, 0, 0, 0, 0, 0,
    213, 213, 213, 213, 213, 213, 213, 213, 213, 7, 0, 0, 211, 211, 211, 211, 124, 236, 234, 0, 0,
    0, 3, 8, 0, 0, 0, 12, 0, 0, 0, 0, 0, 3, 0, 0, 0, 7, 0, 0, 0, 0, 0, 58, 58, 58, 42, 175, 221,
    253, 221, 221, 221, 221, 221, 80, 75, 9, 2, 127, 120, 0, 99, 99, 99, 99, 99, 99, 94, 7, 0, 0,
    0, 0, 0, 0, 213, 213, 213, 213, 213, 213, 213, 213, 213, 7, 0, 0, 211, 211, 211, 211, 124, 236,
    221, 221, 221, 221, 221, 80, 75, 9, 2, 127, 120, 0, 99, 99, 99, 99, 99, 99, 94, 7, 0, 0, 0, 0,
    0, 0, 213, 213, 213, 213, 213, 213, 213, 213, 213, 7, 0, 0, 211, 211, 211, 211, 124, 236];

    let reader = std::io::Cursor::new(&f);

    let mut archive = zip::ZipArchive::new(reader).expect("failed to make archive");

    for i in 0..archive.len() {
        let _file = archive.by_index(i);
    }
}

backtrace:

thread 'main' panicked at 'attempt to add with overflow', /home/jess/src/zip/src/read.rs:591:5
stack backtrace:
   0: rust_begin_unwind
             at /rustc/4ae0a8e413a67b9f6c38f09b24f6179e98c1ba25/library/std/src/panicking.rs:493:5
   1: core::panicking::panic_fmt
             at /rustc/4ae0a8e413a67b9f6c38f09b24f6179e98c1ba25/library/core/src/panicking.rs:92:14
   2: core::panicking::panic
             at /rustc/4ae0a8e413a67b9f6c38f09b24f6179e98c1ba25/library/core/src/panicking.rs:50:5
   3: zip::read::central_header_to_zip_file
             at /home/jess/src/zip/src/read.rs:591:5
   4: zip::read::ZipArchive<R>::new
             at /home/jess/src/zip/src/read.rs:334:24
   5: scratchGaVSRuLnQ::main
             at ./src/main.rs:22:23
   6: core::ops::function::FnOnce::call_once
             at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[Plecra]

The good news is that this issue could never affect any real archive 😄 The fuzzer's done a good job working through the library here. (It's also highlighted a few other minor bugs that I should fix, thankyou!)

This specific crash occurs when a central directory record contains an (implausibly) large zip64 local header offset.

The fix is simply adding header_start.checked_add(archive_offset).ok_or(Error::InvalidArchive)? to this line:

https://github.com/zip-rs/zip/blob/3fd44ffd5da55c8d4c60b3a27a3156ce872a3caf/src/read.rs#L610

I'll minimize the test and add the fix for this soon

[5225225]

I'll open a PR to add the fuzzer to the repo when I get off work, it does come in handy for finding issues.

It can't affect any legitimate archives, but it can cause panics in services that receive untrusted archives and then tries to extract them (which probably isn't all that uncommon), which would be a denial of service (if the panic isn't caught and ignored, or they use panic=abort).

[Plecra]

Yes absolutely~ Fixing this is a high priority, I'm just happy that it isn't (a symptom of) anything more severe.

I'm actually going to be following up on google/oss-fuzz#5400 in the next few days, which would have give much more resources to the fuzzing than I could otherwise. It'd be great to see the harness you used to catch this 😊

[5225225]

It was just

#![no_main]
use libfuzzer_sys::fuzz_target;

fuzz_target!(|data: &[u8]| {
    let reader = std::io::Cursor::new(data);
    let mut archive = if let Ok(x) = zip::ZipArchive::new(reader) {
        x
    } else {
        return;
    };

    for i in 0..archive.len() {
        use std::io::prelude::*;
        if let Ok(file) = archive.by_index(i) {
            for b in file.bytes() {
                if b.is_err() {
                    return;
                }
            }
        }
    }
});

Pretty basic, but also quite similar to the harness they showed.

[Plecra]

Ace, thankyou :)

[zamazan4ik]

@5225225 Could you please check that the issue is still reproducible with the latest zip-rs version from the master branch? Thanks!

[zamazan4ik]

Confirmed that the issue still exists in the library. Working on the fix.

[zamazan4ik]

Fix is here: #259

[zamazan4ik]

#259 is merged so I think the issue can be closed.