compress/gzip: Reader unable to parse headers with long comments

[dsnet]

Using go1.6

RFC 1952 for gzip does not specify a length for the comment and name fields.

If FCOMMENT is set, a zero-terminated file comment is
present. This comment is not interpreted; it is only
intended for human consumption. The comment must consist of
ISO 8859-1 (LATIN-1) characters. Line breaks should be
denoted by a single line feed character (10 decimal).

The current implementation is inconsistent:

• gzip.Writer permits the writing of any length comment/name string in gzip.Header.
• gzip.Reader fails to read any comment/name string in gzip.Header longer than 511 bytes.

Playground example: https://play.golang.org/p/Zvjf8Q7jXe
Change 512 to 511 in the example, and it works again.

This causes issues reading gzip files produced by GZinga, which produces syntactically valid gzip files, but abuses the comment field to store meta data.

Update: I have no intention of fixing this unless there is someone who needs this functionality. Whatever fix we do will need to be careful that we don't introduce a potential vector for DOS attacks since gzip is a common Transfer-Encoding in HTTP. We don't want an infinitely long comment to cause a server to allocate an infinite amount of memory.

[dsnet]

Bumping up milestone since #20083 provides good evidence that this is a real issue.

[gopherbot]

Change https://golang.org/cl/53637 mentions this issue: compress/gzip: permit parsing of GZIP files with long header fields

[dsnet]

Pushing to Go1.11 milestone... but I think a more general API for resource limitation might be the better approach. See #20169 for some discussion.

[philippfrank]

@dsnet This is still an issue. Did it go under?

[dsnet]

Running the snippet on Go1.15 clearly shows it's still an issue.

Huh, apparently I had a fix for this that I never submitted: https://golang.org/cl/53637

[philippfrank]

@dsnet Okay, any chance you can push that forward? I am using gzip.NewWriter() to compress arbitrary text files (up to 1GB in size) while being unable to uncompress those using the same gzip.NewReader().

For anyone else having that issue: a pragmatic solution might be to copy gunzip.go and adjust the maximum buffer size manually. It seems several other gzip modules out there have that exact same issue (because they copied the stdlib code).

[dsnet]

Having reviewed my CL, the reason I didn't push it through is because of concerns with this feature being an trivial avenue for denial-of-service attack where a malicious GZIP file could cause a server to allocate arbitrary amounts of memory.

We have several options:

1. We could immediately permit parsing of small headers (I proposed up to 32KiB to match the window size of the DEFLATE algorithm itself). However, this won't help use-cases that abuse the comment field to store machine-parsable information where it can easily exceed 32KiB.
2. Add a new Reader.MaxFieldSize method so that the user can opt-in to parsing larger headers. Use of this option would still be a bit odd, since you would have to do something like:

var zr gzip.Reader
zr.MaxFieldSize(math.MaxInt32)
if err := zr.Reset(r); err != nil {
    ... // handle err
}
... // make use of zr.Header.Comment

Since this would probably require an API change, I'm adding the NeedDecision label.
\cc @FiloSottile since he's probably interested in any possible DOS-related issues.