8000 Reject oversized zip entries instead of silently truncating them by m86hussein · Pull Request #4027 · quran/quran_android · GitHub
[go: up one dir, main page]
Skip to content

Reject oversized zip entries instead of silently truncating them - #4027

Open
m86hussein wants to merge 1 commit into
quran:mainfrom
m86hussein:fix/zip-silent-truncation-over-size-limit
Open

Reject oversized zip entries instead of silently truncating them#4027
m86hussein wants to merge 1 commit into
quran:mainfrom
m86hussein:fix/zip-silent-truncation-over-size-limit

Conversation

@m86hussein
@m86hussein m86hussein commented Nov 9, 2026
Copy link
Copy Markdown
Contributor

Summary

ZipUtils.unzipFile protects against zip bombs (CERT IDS04-J) by rejecting archives whose total unzipped size exceeds MAX_UNZIPPED_SIZE. However, the read loop could silently truncate an oversized entry instead of rejecting the archive.

The bug

The inner read loop was:

while (total + BUFFER_SIZE <= MAX_UNZIPPED_SIZE && (size = is.read(buf)) > 0) {
  ostream.write(buf, 0, size);
  total += size;
}
...
if (processedFiles >= MAX_FILES || total >= MAX_UNZIPPED_SIZE) {
  throw new IllegalStateException("Invalid zip file.");
}

The loop stops reading an entry as soon as total gets within one BUFFER_SIZE of the limit, which leaves total strictly below MAX_UNZIPPED_SIZE. The post-loop guard total >= MAX_UNZIPPED_SIZE therefore never fires, and the oversized entry is written out in truncated form while extraction continues as if nothing went wrong.

The existing testFileWithSmallMaxUnzipSize only passes by coincidence: its 5MB limit is a multiple of the 512-byte buffer, so the 512-byte reads land total exactly on the limit and trip the post-check. With a limit that isn't buffer-aligned, the archive is silently truncated.

The fix

Move the size check inside the loop so the archive is rejected the moment the limit is crossed:

while ((size = is.read(buf)) > 0) {
  total += size;
  if (total >= MAX_UNZIPPED_SIZE) {
    throw new IllegalStateException("Invalid zip file.");
  }
  ostream.write(buf, 0, size);
}

The now-unreachable total >= MAX_UNZIPPED_SIZE post-check is removed.

Testing

Added testEntryExceedingLimitIsRejectedNotTruncated, which builds a zip with a single 10KB entry and a 1KB (non-buffer-aligned) limit. It fails against the old code (the entry is truncated, no exception) and passes with the fix. All existing ZipUtilsTest tests still pass.

@m86hussein
Reject oversized zip entries instead of silently truncating them
8c234d8 
ZipUtils.unzipFile guards against zip bombs (CERT IDS04-J) by capping the total unzipped size. The read loop condition was `total + BUFFER_SIZE <= MAX_UNZIPPED_SIZE`, which stops reading an entry once `total` gets within one buffer of the limit, leaving `total` below MAX_UNZIPPED_SIZE. The post-loop guard `total >= MAX_UNZIPPED_SIZE` then never fired, so an oversized entry was silently written out truncated rather than rejected.

Move the size check inside the loop so the archive is rejected as soon as the limit is crossed. Added a test with a 10KB entry against a 1KB (non-buffer-aligned) limit that reproduces the silent truncation before the fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@m86hussein
0