add zip test for blob with negative size#127990
Closed
wfurt wants to merge 3 commits into
Closed
Conversation
Contributor
|
Tagging subscribers to this area: @karelz, @dotnet/area-system-io-compression |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a regression/security-hardening test to the System.IO.Compression ZipArchive test suite to exercise parsing of a ZIP64 extra field that encodes an invalid (negative) uncompressed size, ensuring consumption remains safe.
Changes:
- Added a new test covering a ZIP64 extra field with
UncompressedSize = -1. - Added a byte-level ZIP builder helper to generate the malformed archive payload used by the test.
Comment on lines
+2423
to
+2447
| // Attempting to actually read the data must fail safely — either by throwing, | ||
| // or by returning zero bytes (matching the actual stored data). It must NOT | ||
| // crash, hang, allocate based on the negative size, or read past the end of | ||
| // the underlying stream. Defense-in-depth in CrcValidatingReadStream rejects | ||
| // a negative expected length on Open(), so an ArgumentOutOfRangeException or | ||
| // InvalidDataException is the expected outcome here. | ||
| try | ||
| { | ||
| using Stream s = entry.Open(); | ||
| byte[] buffer = new byte[1024]; | ||
| int totalRead = 0; | ||
| int read; | ||
| while ((read = s.Read(buffer, 0, buffer.Length)) > 0) | ||
| { | ||
| totalRead += read; | ||
| // Guard against the negative size being misinterpreted as a huge unsigned | ||
| // length that would let the read loop run forever. | ||
| Assert.True(totalRead <= 1024 * 1024, "Read returned more data than the archive contains."); | ||
| } | ||
| Assert.Equal(0, totalRead); | ||
| } | ||
| catch (Exception ex) when (ex is InvalidDataException or ArgumentOutOfRangeException) | ||
| { | ||
| // Acceptable: downstream code rejects the malformed entry on Open/Read. | ||
| } |
Comment on lines
+2406
to
+2411
| // Validation for IO-021: Zip64 extra field with negative UncompressedSize (-1). | ||
| // The minimal 16-byte Zip64 extra field bypasses the negative-value check in | ||
| // Zip64ExtraField.TryGetZip64BlockFromGenericExtraField (the check only runs after | ||
| // all four fields would have been read). This test verifies that the bypassed | ||
| // negative value does NOT lead to memory corruption, buffer over-read, infinite | ||
| // loop, or other harmful behavior — downstream code handles it safely. |
Comment on lines
+2506
to
+2507
| long negativeUncompressedSize = -1; // 0xFFFFFFFFFFFFFFFF in two's complement | ||
| long negativeCompressedSize = 0; |
Comment on lines
+2533
to
+2536
| // No file data | ||
| long dataOffset = ms.Position; | ||
|
|
||
| // Central Directory File Header |
rzikm
approved these changes
May 11, 2026
Member
|
@wfurt Do you want to continue with this PR? it is slowly getting stale |
- Reject negative compressed/uncompressed sizes in IsOpenableInitialVerifications so a malformed Zip64 entry surfaces as InvalidDataException instead of ArgumentOutOfRangeException. - Tighten test catch to InvalidDataException only. - Simplify test comments to describe externally observable behavior. - Rename zip64 size locals and centralDirectoryOffset for clarity.
3 tasks
rzikm
reviewed
May 27, 2026
Comment on lines
+2451
to
+2453
| // The negative value propagates as-is to the entry's Length property. | ||
| // This is observable but not by itself harmful. | ||
| Assert.Equal(-1L, entry.Length); |
Member
There was a problem hiding this comment.
I don't think this is the behavior we want, we should not return garbage data just so that we would throw later in Open(). We should validate the length ASAP in the process, which likely means during initial central directory parsing
Member
There was a problem hiding this comment.
I agree, I believe the checks can be made inside the TryGetZip64BlockFromGenericExtraField(Async)
Member
|
Closed this because of follow-up here: #129121 |
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.
verifies we throw on malformed zip entry.