Skip to content

Reject a Zip64 uncompressed size that does not fit a signed int64 - #2369

Merged
xuri merged 2 commits into
qax-os:masterfrom
arpitjain099:fix/zip64-negative-uncompressed-size
Aug 5, 2026
Merged

Reject a Zip64 uncompressed size that does not fit a signed int64#2369
xuri merged 2 commits into
qax-os:masterfrom
arpitjain099:fix/zip64-negative-uncompressed-size

Conversation

@arpitjain099

Copy link
Copy Markdown
Contributor

Follows up on the advisory discussion, where you asked whether I wanted to send a patch.

zip.File.FileInfo().Size() returns int64(UncompressedSize64), and UncompressedSize64 comes from the Zip64 extended information extra field in the central directory. Any declared value in [2^63, 2^64) turns negative on that conversion, and the guard in ReadZipReader is signed arithmetic, so:

  • unzipSize += fileSize makes the running total negative, and unzipSize > f.options.UnzipSizeLimit stays false, so the decompression cap never fires
  • the two UnzipXMLSizeLimit comparisons are false as well, so a large part is never routed to a temporary file
  • readFile reaches make([]byte, 0, negative), which panics with makeslice: cap out of range

A 159 byte archive is enough to reach it through OpenFile, OpenReader or ReadZipReader, and nothing in the library recovers, so the panic propagates to the caller.

The change

Compare the declared size against the limit as a uint64 before it is narrowed. An entry that cannot fit is rejected on its own, so the signed arithmetic below only ever sees a value already within the limit, and fileSize can no longer be negative. The UnzipSizeLimit < 0 clause keeps the conversion honest if someone configures a negative limit.

readFile also bounds its capacity hint, so the allocation is safe regardless of how that function is reached. It takes the limit as a parameter since it is package level and has no access to f.options; there is one caller.

Testing

TestZip64UncompressedSizeOverflow builds the archive in memory rather than committing a fixture. It asserts the declared size really is negative after conversion, then that OpenReader returns the unzip size limit error instead of panicking. It also runs a control at 2^63-1, one below the sign flip, which already returned the limit error before this change. That is what pins the bug to the conversion rather than to the size being large.

Verified fail before and pass after: on master the new test panics with makeslice: cap out of range, and with the fix it passes. Full suite is green locally on go1.26.5 darwin/arm64.

zip.File.FileInfo().Size() returns int64(UncompressedSize64), and
UncompressedSize64 is read from the Zip64 extended information extra
field in the central directory. A declared size of 2^63 or above turns
negative on that conversion, so the running unzipSize total went
negative too and stayed under UnzipSizeLimit, the UnzipXMLSizeLimit
comparisons that route large parts to a temporary file were never true,
and readFile reached make([]byte, 0, negative), which panics with
makeslice: cap out of range. A 159 byte archive was enough.

Compare the declared size against the limit as a uint64 before it is
narrowed, so an entry that cannot fit is rejected on its own and the
signed arithmetic below only ever sees a value within the limit. Bound
the capacity hint in readFile as well, so the allocation is safe
independently of how it is reached.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
@xuri xuri added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 3, 2026
@xuri xuri added this to v2.11.1 Aug 3, 2026
@xuri xuri moved this to Security in v2.11.1 Aug 3, 2026
@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.62%. Comparing base (ad2cb3e) to head (db7b243).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2369   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files          32       32           
  Lines       26979    26994   +15     
=======================================
+ Hits        26877    26892   +15     
  Misses         53       53           
  Partials       49       49           
Flag Coverage Δ
unittests 99.62% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread lib.go Outdated

@xuri xuri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. I've made some changes based on your branch.

@xuri xuri added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 5, 2026
@arpitjain099

Copy link
Copy Markdown
Contributor Author

Thakn you @xuri Yesterday and today have been busy, will look at your feedback items later in the evening.

@xuri

xuri commented Aug 5, 2026

Copy link
Copy Markdown
Member

Thakn you @xuri Yesterday and today have been busy, will look at your feedback items later in the evening.

No worries, I've made some changes based on your branch. Please take a look for my changes. If your have any additional suggestions, please let me know.

@xuri
xuri merged commit db93f8d into qax-os:master Aug 5, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

Status: Security

Development

Successfully merging this pull request may close these issues.

2 participants