Limit metadata block size when reading TAR archives#127602
Limit metadata block size when reading TAR archives#127602iremyux wants to merge 1 commit intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-formats-tar |
There was a problem hiding this comment.
Pull request overview
This PR tightens TAR archive parsing by limiting the maximum size of metadata data sections (PAX extended attributes and GNU LongPath/LongLink payloads) to 1 MB, reducing risk of excessive allocations/DoS when reading malformed archives.
Changes:
- Introduced a
MaxMetadataBlockSizeconstant (1 MB) onTarHeader. - Updated the metadata
ValidateSize()check to enforce the new maximum when reading PAX/GNU metadata blocks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Read.cs |
Enforces the new 1 MB maximum via ValidateSize() before renting/reading metadata buffers. |
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.cs |
Adds the MaxMetadataBlockSize constant used by the reader. |
| private const string GnuMagic = "ustar "; | ||
| private const string GnuVersion = " \0"; | ||
|
|
||
| // // Maximum allowed size for metadata data sections (PAX extended attributes, GNU LongPath/LongLink). |
There was a problem hiding this comment.
The comment above MaxMetadataBlockSize has a duplicated comment marker (// //) and reads a bit oddly (“metadata data sections”). Consider fixing this to a single // and adjusting the wording to avoid repetition (e.g., “metadata sections” or “metadata data blocks”).
| // // Maximum allowed size for metadata data sections (PAX extended attributes, GNU LongPath/LongLink). | |
| // Maximum allowed size for metadata sections (PAX extended attributes, GNU LongPath/LongLink). |
| private void ValidateSize() | ||
| { | ||
| if ((uint)_size > (uint)Array.MaxLength) | ||
| if ((uint)_size > (uint)MaxMetadataBlockSize) | ||
| { | ||
| ThrowSizeFieldTooLarge(); | ||
| } |
There was a problem hiding this comment.
This introduces a new behavioral limit (1 MB) on PAX extended attribute blocks and GNU LongPath/LongLink metadata. Please add unit tests that exercise reading archives with metadata sections just over this limit and assert the expected exception is thrown (ideally for both sync and async paths, and at least one PAX and one GNU long-path case) so the limit doesn’t regress.
rzikm
left a comment
rzikm
left a comment
There was a problem hiding this comment.
LGTM in principle, are we sure 1MB is large enough to not break legitimate uses?
Limits the maximum allowed size of PAX extended attribute blocks and GNU LongPath/LongLink data sections to 1 MB when reading TAR archives by introducing a MaxMetadataBlockSize constant (1 MB).