Repository navigation
Conversation
Once the decompression layer reached the end of the compressed stream, its stat replaced the size from the archive with the number of bytes it had produced. The CRC layer then compared the data length with itself, so a size in the central directory that didn't match the data went unnoticed. Keep the size from the lower layer if there is one, as already happened for zstd, whose decompressor never reports the end of the stream. Reading such an entry now fails with ZIP_ER_INCONS (ZIP_ER_DETAIL_INVALID_FILE_LENGTH). Recompressing it in zip_close fails with ZIP_ER_DATA_LENGTH instead of writing the actual size. Assisted-by: Claude Code (Opus 5.5)
Member
|
I don't like making the decompression source worse to make validation work. The crc source should get the data to compare against when it is created. We'll restructure the code accordingly. |
Author
|
Thanks, that makes sense. Passing the expected size to the CRC source when it's created would also cover the AE-2 case in #596, since that check uses the same layer. Would you like me to rework both PRs that way, or would you prefer to do the restructuring yourselves? The regression tests should apply either way. |
This branch has not been deployed
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.
For deflate, bzip2, LZMA and XZ entries, the uncompressed size from the central directory was never compared with the length of the decompressed data. Once the decompression layer reached the end of the stream, its stat replaced the size from the archive with the number of bytes it had produced, so the CRC layer compared the data length with itself. Stored and zstd entries were already checked.
The fix keeps the size from the lower layer if there is one (one condition in
zip_source_compress.c).Reading such an entry now fails at the end of the data with
ZIP_ER_INCONS/ZIP_ER_DETAIL_INVALID_FILE_LENGTH, as stored and zstd entries already do.Recompressing such an entry in
zip_close()now fails withZIP_ER_DATA_LENGTHinstead of writing the actual size. Entries thatzip_close()copies without recompressing are not affected.New tests
read_incons_deflated_longerandread_incons_deflated_shorterread deflated entries whose CRC matches the data but whose uncompressed size is too small or too large.Under LeakSanitizer the two new tests only pass with #591 applied, because
ziptool cathits thezip_source_free()leak fixed there.Assisted-by: Claude Code (Opus 5.5)