fix(zip): guard extra-field header read before bounds are known

[shoemoney]

Description

Fixes a heap-buffer-overflow READ in llamafile/zip.c that professor-moody flagged in #1038 but that #1039 does not touch (#1039 only changes llamafile/llamafile.c, adding a bound check on the result of these functions — it does not fix the out-of-bounds read that happens inside them while walking the extra field). This PR closes that gap.

#1038 says, about this exact spot:

I also hit two smaller out-of-bounds reads in the same parser under ASan while looking at this. llamafile.c:129 accepts a central directory whose size field is 0 as long as the record count is nonzero... There is a 2 byte one in zip.c:38. Neither is likely to be noticed by a production allocator, but they share the root cause and the patch closes the first.

This PR is the fix for the zip.c:38 one, plus the two other call sites that share the identical loop and are vulnerable the same way.

The bug

get_zip_cfile_uncompressed_size() (zip.c:26), get_zip_cfile_compressed_size() (zip.c:38), and get_zip_cfile_offset() (zip.c:54) all walk the zip64 extra-field region with the same loop:

for (; p + ZIP_EXTRA_SIZE(p) <= pe; p += ZIP_EXTRA_SIZE(p))

ZIP_EXTRA_SIZE(P) expands to ZIP_EXTRA_CONTENTSIZE(P) + kZipExtraHdrSize, and ZIP_EXTRA_CONTENTSIZE(P) is ZIP_READ16((P)+2) (zip.h:256,258). So the loop condition reads p[2]/p[3] before anything has confirmed that 4 bytes exist at p. If the extra-field region (pe - p, i.e. ZIP_CFILE_EXTRASIZE(z)) is 1-3 bytes on the first iteration, that read runs past pe before the check that's supposed to stop it ever executes.

pe = ZIP_CFILE_EXTRA(z) + ZIP_CFILE_EXTRASIZE(z), and ZIP_CFILE_EXTRASIZE is a raw 16-bit field out of the central directory record — fully attacker controlled.

Reachability: llamafile.c:177-178 calls get_zip_cfile_offset() and get_zip_cfile_compressed_size() directly on cdirdata + entry_offset, where cdirdata is malloc(cdirsize) and cdirsize is itself attacker controlled. Entry enumeration checks that the record's declared extent fits inside cdirdata, but that says nothing about whether the sub-loop inside these functions stays inside the declared extra-field bytes before its first read. A crafted central directory entry with compressedsize (or offset) set to the zip64 sentinel 0xFFFFFFFF and extrasize = 1, 2, or 3 hits this. llamafile_open_zip() runs on every zip-format llamafile load.

The fix

Check that a full 4-byte extra-field header actually fits before evaluating ZIP_EXTRA_SIZE(p), in all three functions (leaving two of three known-identical instances unfixed didn't seem defensible):

for (; p + kZipExtraHdrSize <= pe && p + ZIP_EXTRA_SIZE(p) <= pe; p += ZIP_EXTRA_SIZE(p))

kZipExtraHdrSize is already defined as 4 in zip.h. No other logic changes.

Testing

This is C, so the proof is AddressSanitizer, not a unit test. I compiled the actual unmodified zip.c/zip.h from this repo with clang -fsanitize=address and drove all three functions with a 47-byte heap buffer: a minimal 46-byte cfile header + 1-byte extra field, with compressedsize/uncompressedsize/offset all set to the zip64 sentinel so each function walks into the extra field.

RED — before the fix, get_zip_cfile_compressed_size():

buffer: 47 bytes allocated (indices 0..46 valid)
ZIP_CFILE_EXTRA(z) is at index 46, ZIP_CFILE_EXTRASIZE(z) = 1
calling get_zip_cfile_compressed_size() -- this walks the extra
field with a buggy loop bound in zip.c...
=================================================================
==86217==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x604000000440 at pc 0x000104a1ed34 bp 0x00016bbdc660 sp 0x00016bbdbe10
READ of size 2 at 0x604000000440 thread T0
    #0 0x000104a1ed30 in __asan_memcpy+0x400 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x3ed30)
    #1 0x000104220b00 in get_zip_cfile_compressed_size zip.c:38
    #2 0x000104221598 in main poc_compressed.c:34

0x604000000440 is located 1 bytes after 47-byte region [0x604000000410,0x60400000043f)
allocated by thread T0 here:
    #0 0x000104a21214 in malloc+0x78 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x41214)
    #1 0x000104220f70 in main poc_compressed.c:16

SUMMARY: AddressSanitizer: heap-buffer-overflow zip.c:38 in get_zip_cfile_compressed_size
==86217==ABORTING

The same input drove the other two functions before the fix and both aborted identically:

SUMMARY: AddressSanitizer: heap-buffer-overflow zip.c:26 in get_zip_cfile_uncompressed_size
SUMMARY: AddressSanitizer: heap-buffer-overflow zip.c:54 in get_zip_cfile_offset

GREEN — after the fix, same crafted input, all three functions in one run:

[uncompressed_size] calling...
[uncompressed_size] returned -1 (no ASan trip)
[compressed_size] calling...
[compressed_size] returned -1 (no ASan trip)
[offset] calling...
[offset] returned -1 (no ASan trip)
ALL CALLS COMPLETED CLEANLY

All three now correctly report "not found" (-1) instead of reading out of bounds, exactly the behavior the code already has for a well-formed-but-absent zip64 record.

Regression — well-formed archive still parses identically. I built a second harness with a genuine 20-byte zip64 extra field (id 0x0001, size 24, holding real uncompressed/compressed/offset 64-bit values) and called all three functions before and after the patch:

uncompressed_size = 123456789012345 (expected 123456789012345)
compressed_size   = 98765432109876 (expected 98765432109876)
offset            = 555555555555 (expected 555555555555)
WELL-FORMED REGRESSION: PASS

Identical output before and after the fix. The added check only rejects a header the loop can't safely read; it never changes behavior for a record that actually has one.

Build: make setup && .cosmocc/4.0.2/bin/make -j8 completed cleanly against this branch with no new warnings from the changed file.

Recipe (if you want to reproduce this yourself)

size_t bufsz = 46 /* fixed cfile header */ + 0 /* namesize */ + 1 /* extrasize */;
uint8_t *z = malloc(bufsz);
memset(z, 0, bufsz);
z[20]=0xFF; z[21]=0xFF; z[22]=0xFF; z[23]=0xFF; /* compressedsize sentinel */
z[24]=0xFF; z[25]=0xFF; z[26]=0xFF; z[27]=0xFF; /* uncompressedsize sentinel */
z[28]=0; z[29]=0;   /* namesize = 0 */
z[30]=1; z[31]=0;   /* extrasize = 1 -- too small for even one 4-byte header */
get_zip_cfile_compressed_size(z); /* heap-buffer-overflow under ASan */

Checklist

• I understand the code I am submitting.
• I have run this code locally and verified the change.
• New and existing tests pass locally, or I have explained why tests were not run. (No existing unit tests cover zip.c; verification here is ASan RED/GREEN plus a well-formed-archive regression check, described above.)
• Documentation was updated where necessary. (Not applicable — no user-facing behavior change.)
• If I changed code in llama.cpp/, whisper.cpp/, or stable-diffusion.cpp/, I also updated the matching *.patches/ files. (Not applicable — this change is entirely within llamafile/zip.c, not a submodule.)
• I have read and followed the contribution guidelines.
• AI Usage:
  • No AI was used.
  • AI was used in an assistive capacity.
  • This PR includes substantial AI-generated content.

AI Usage Information

• AI Model used:
• AI Developer Tool used:
• Any other info you'd like to share:

When answering reviewer questions, please respond yourself rather than pasting reviewer comments into an AI system and posting the reply back unchanged.

• I am an AI Agent filling out this form (check box if true)