Add missing check for corrupted central directory - #131257
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR hardens System.IO.Compression ZIP reading for AES-encrypted entries by avoiding an unexpected ArgumentOutOfRangeException when a corrupted central directory points a local-header offset past the end of the archive. Instead of failing during central directory parsing (when salts are eagerly read), the error is deferred until the entry is opened, aligning AES behavior with non-encrypted entries.
Changes:
- Add an early “offset past end-of-stream” guard in AES salt pre-read logic (sync + async) to avoid
Seekthrowing on some stream types (e.g.,MemoryStream). - Add a regression test that corrupts a central directory record’s local header offset and verifies enumeration doesn’t throw unexpectedly, while
Open()still fails withInvalidDataException.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs |
Adds a regression test ensuring central directory parsing doesn’t throw unexpectedly for a corrupted AES entry offset and that failure is deferred to entry open. |
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs |
Adds a length-based guard in ReadEncryptionSaltIfNeeded to prevent Seek-triggered ArgumentOutOfRangeException during central directory parsing. |
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.Async.cs |
Mirrors the same guard for the async salt pre-read path. |
rzikm
left a comment
There was a problem hiding this comment.
LGTM, I assume you ran the fuzzer locally to confirm there aren't other bugs still lurking?
|
Yep, I ran it locally and it worked. Also ran it against the faulty input directly |
|
Thank you. |
|
Ran the fuzzer(s) successfully. Code coverage reports: |
|
/ba-g failures are unrelated to my change |
Fixes #131228