llamafile_open_zip: validate the zip entry window against the actual file size

[professor-moody]

Fixes the crash reported in #1038.

llamafile_open_zip() trusted the offset and compressed size from the zip central directory without checking them against the real file, and assigned the int64_t results of get_zip_cfile_offset() and get_zip_cfile_compressed_size() into uint64_t and size_t. Those functions return -1 on failure, so a crafted archive could set file->size to SIZE_MAX, wrap the mapsize arithmetic, and end up with a one page mapping that llamafile_size() then reported as SIZE_MAX bytes.

Three changes, all in llamafile/llamafile.c:

1. Keep the real file length from the lseek that already happens at open time. file->size holds it briefly before being overwritten at :178 by the value the archive claims, so this just preserves it as a local to validate against.

2. Take both accessor results as int64_t and reject negatives before they reach unsigned types, then require the declared window to fit inside the file. The bound is written as "entry_off > zipsize - entry_size" so it cannot overflow.

3. Require a nonzero central directory size in both the zip64 and zip32 branches. Zero passed the existing "<= INT_MAX" test and led to malloc(0) followed by a 4 byte read.

Tested against a build of 9a716e7 on Fedora 43 x86_64 and on arm64 macOS. Before the change the crafted 162 byte archive gives SIGSEGV on Linux and SIGBUS on macOS, 10 runs out of 10. After it, both are rejected with "zip entry has an unreadable offset or size", 0 out of 10. A separate archive using a plain oversized size field rather than the zip64 sentinel is rejected with "zip entry extends past the end of the file". Control archives behave exactly as before.

Regression checked against a real model. I packed a Qwen2.5-0.5B-Instruct Q4_K_M gguf into a llamafile with zipalign -j0 and ran it, so the load goes through llamafile_open_zip with legitimate values. Before and after the change it loads and generates byte-identical output. The loose-file path (-m on a plain .gguf) is unchanged too.

• AI Usage:
  • AI was used in an assistive capacity.

[aittalam]

Thank you @professor-moody! I am trying to wrap up a release by EOD, so I will not be able to merge this immediately as I want to run the whole suite of integration tests on this. But I will test, review, and merge your PR afterwards, so we'll have time to make sure it satisfies all tests before the new release. I hope that's ok! 🙏

[professor-moody]

That timing works fine, thanks for letting me know.

Good thing you had not merged yet. I kept testing my own patch against a wider set of crafted archives while waiting, and found a gap in it. Pushed a fixup, so please review the branch rather than the original commit.

The original bound validated the offset and size taken from the central directory. The problem is that offset points at the local file header, not at the data. The existing off += ZIP_LFILE_HDRSIZE(lfile) then advances past namesize and extrasize, two attacker-controlled uint16 fields worth up to 131100 bytes together, and nothing revalidated the window after that. So an archive with a completely honest central directory could still push the mapping past the end of the file and reproduce the original fault.

Two 182 byte archives with byte-identical central directories, running the guard as it stood:

control:  guard passed -> off=40      -> reads the first weight byte, clean
crafted:  guard passed -> off=131100  -> AddressSanitizer: BUS on unknown address, READ

The fixup revalidates after the advance, written as a subtraction so it cannot overflow. It also tightens the central directory size check from > 0 to >= kZipCfileHdrMinSize, since a record is 46 bytes by construction and sizes of 1 to 3 otherwise reached malloc() followed by a 4 byte read.

I specifically checked that it does not over-reject, since that would be the worse failure for you: an archive with a legitimate 8000 byte local extra field still loads normally, as does a real llamafile. A large local extra field is legal and zipalign relies on it, so the fix bounds the resulting window rather than capping the header size.

Worth noting for your test run: the original commit does still fix the more serious half on its own, since file->size can no longer become SIZE_MAX and neuter llama.cpp's own nbytes_remain and tensor-extent checks. What it missed was the crash, which is what the fixup closes.

[tahazarif10]

The ZIP64 branch still only checks ZIP_CDIR64_SIZE(bufdata) > 0, while ZIP32 now requires >= kZipCfileHdrMinSize. That seems to leave the small-central-directory OOB read open for ZIP64 too. I'd mirror the same minimum-size check there.

[professor-moody]

@tahazarif10 Good catch, you're right — the two branches should agree.

Tracing it against the branch head, the exposure is narrower than it first looks, so worth being precise. cdirsize comes from ZIP_CDIR64_SIZE, cdirdata = gc(malloc(cdirsize)) at :152, and :158 then does ZIP_READ32(cdirdata) unconditionally. Sizes 1-3 reach that four-byte read with a shorter allocation. Sizes 4-45 are not exposed to it: the read is in bounds, and the entry loop short-circuits on entry_offset + kZipCfileHdrMinSize <= cdirsize at :171 before ZIP_CFILE_HDRSIZE is evaluated.

I swept the declared size on a build of this branch to confirm which path each one takes. No crash in any case, and I'm not claiming one — the over-read is small and I don't have a sanitised build to measure its extent, so that part is from reading the source rather than observing it.

Pushed a commit applying the minimum-size guard to the zip64 branch, so it now matches zip32.

[professor-moody]

@aittalam This is ready for the integration-test pass whenever it suits you. The branch now carries the original entry-window validation plus the zip64 minimum-size guard from @tahazarif10's review.