Bound gzip header FNAME and FCOMMENT string sizes to prevent unbounded memory growth (issue #91) - #93
Merged
Conversation
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.
Summary
Bound the length of the gzip header
FNAMEandFCOMMENTstrings so that parsing untrusted input cannot allocate without limit. The sharedread_cstringhelper now rejects a string once it would exceed 64 KiB, returningio::ErrorKind::InvalidData.Closes #91
Problem
RFC 1952 does not set an upper bound on the length of the gzip header
FNAME/FCOMMENTstrings.Header::read_fromreads these viaread_cstring, which appends bytes to aVecuntil a NUL terminator or EOF. An attacker-controlled stream could therefore force an arbitrarily large allocation while the header is parsed, before any payload output is produced. This affects synchronous parsing ingzip::Decoder::new, thegzip::MultiDecodermember transition, and the non-blocking gzip decoder. Output-side decompression bounds do not help, since the allocation happens before any output exists.Solution
MAX_HEADER_STRING_LEN = 64 * 1024and a guard in the sharedread_cstringhelper. Oncebuf.len() >= MAX_HEADER_STRING_LENand the just-read byte is non-NUL,read_cstringreturnsInvalidData. A NUL terminator immediately after exactly 64 KiB is still accepted, so a string of exactly 64 KiB is valid; a 65th non-NUL byte is rejected.FNAMEandFCOMMENTgo throughread_cstring, the bound applies to the synchronousgzip::Decoder,gzip::MultiDecoder, andnon_blocking::gzip::Decoder(which reads the header viaHeader::read_from) without any per-path duplication.MAX_HEADER_STRING_LEN(plus one for the rejection-probe byte).Why 64 KiB?
This is a policy choice, not a format requirement — RFC 1952 is silent on these fields' limits. The cap:
FEXTRAlength (XLEN), whose maximum is 65535 bytes;PATH_MAXis 4096, WindowsMAX_PATHis 260), so no legitimate input is rejected.The value is an independent cap; it is not derived from the DEFLATE decoder's 64 KiB internal buffer (#90). That bound governs how much decoded output is buffered before
readreturns, which is a separate concern from how large a header field is accepted. The two numbers happen to coincide, but they are not logically coupled.Public API
No public API changes.
Validation
cargo test --workspacecargo test --workspace --no-default-featurescargo clippy --lib --all-features -- -D warningscargo fmt --all -- --checkNew regression test
gzip_header_string_is_boundedcoversF_NAMEandF_COMMENTindependently, asserting that a string of exactly 64 KiB + NUL is accepted losslessly and that 64 KiB + 1 byte is rejected withInvalidData.Credits / Acknowledgment
Thanks to @optiklab for reporting the issue and for their proposed patch (optiklab@4ef7fb7). The fix here reimplements the same approach and threshold — functionally equivalent — rather than copying the diff verbatim.