fix(zip): guard zip64 extra-field parsing against malformed input - #3541
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A single malformed or malicious ZIP makes the central-directory parser in
@loaders.gl/zipthrow anuncaught
RangeError: Offset is outside the bounds of the DataView. It surfaces on public entrypoints that walk the central directory (
ZipFileSystem.readdir()/fetch(),makeZipCDHeaderIterator(), SLPK/i3s loading), so untrusted input can abort the caller.Root cause
findZip64DataInExtra()inmodules/zip/src/parse-zip/cd-file-header.tslocated the zip64 extrarecord with a byte-wise scan and then read from the result without validating it:
When a CD size/offset field holds the zip64 sentinel
0xffffffffbut no matching zip64 record ispresent,
findIndex()returns-1and the code callsgetBigUint64(3, true)— out of bounds for anempty or short extra field, hence the
RangeError. A truncated zip64 record hits the same path: therecord header matches, but the declared 16-byte payload is not there to read.
Two further defects came from the same scan:
getUint16(...bytes)helper decoded little-endian asbytes[0] + bytes[1] * 16insteadof
* 256, so adataSizefield of256(00 01) compared equal to16.01 00appearing inside apreceding extra record's payload was accepted as the zip64 header, and a short zip64 record could
read into the bytes of the following record.
Fix
findZip64DataInExtra()now walks the extra field as the spec describes it — a sequence of[headerId: uint16][dataSize: uint16][payload]records:headerId === 0x0001 && dataSize === zip64chunkSize, and each8-byte field read is bounds-checked against that record's payload end.
returned unchanged — no exception.
The broken
getUint16helper is removed;DataView.getUint16(offset, true)is used directly.Tests
modules/zip/test/zip-utils/cd-file-header.spec.tsgains four cases:createZip64Info()— 64-bit sizes still decoded.01 00 10 00— valuescome from the real record, not from the byte pattern inside the previous payload.
The first two cases throw
RangeErroronmaster; the last one silently decoded garbage there.Risk / compatibility
Behavior changes only for malformed input. Well-formed zip64 archives are parsed exactly as before —
the record they carry has
headerId 0x0001and adataSizeequal to the number of sentinel fields(8/16/24), which the new walk matches. The removed
* 16decode agreed with a correct little-endiandecode for those sizes, so no valid archive relied on it.
Verification needed
Dependency install and the test run could not be executed in the environment this patch was prepared
in, so the four new tests have been validated by byte-level tracing only. Please let CI run
modules/zip(yarn test-node) on this branch.