archive/zip: provide way to limit number of entries in an archive

[neild]

The archive/zip.Reader.File field contains an exported field containing a list of files in the archive. This field is populated by zip.NewReader. A ZIP archive can contain an arbitrarily large number of files, which means opening a ZIP archive can allocate an arbitrarily large amount of memory for the file list.

While this is arguably a vulnerability, it's one which is inherent in the zip.Reader API. We could put a limit on how many file entries we're willing to create, but it's not clear if there is any default limit we could choose that wouldn't be both too large in some situations (allows excessive memory allocation) and too small in others (rejects valid archives containing many files).

The number of file entries is bounded by the size of the ZIP archive itself, but there's no easy way for a user to distinguish between an archive with a small number of large files and one with a large number of small files.

We should provide a way to safely open a possibly-malicious archive.

A few possibilities might include:

• A GODEBUG=zipmaxnumfiles=N setting to set a global archive size limit. (Possibly with a default value? But as described above, it's hard to pick a good default here.)
• A function which reports how many files an archive contains, so the user can decide whether to open it or not.
• A new zip.NewReaderOpts function that takes options, including a limit on the number of files in the archive.
• A replacement for zip.NewReader that does not populate the Reader.File list, with some other way of accessing the files. (A method returning the number of files and another returning an iterator, maybe?)

This issue is related to #33036, but somewhat distinct: That issue is (I think) about limiting the amount of data read from an archive file, rather than the number of files within an archive.

Thanks to Jakub Ciolek for reporting this issue.

[gabyhelp]

Related Issues

• archive/zip: provide API for resource limits #33036
• archive/zip: malformed archive may cause panic or memory exhaustion #46242 (closed)
• archive/zip: cap out of range #10956 (closed)
• archive/zip: file might overflowed while decompressing #49791 (closed)
• proposal: archive/tar, archive/zip: add NewReaderOptions with directory traversal defenses #57850
• archive/zip: sanitize the FileHeader.Name to remove path traversal ("../../") from zip files? #25849 (closed)
• archive/zip: EOCDR comment length handling is inconsistent with other ZIP implementations #66869 (closed)
• archive/zip: OpenReader does not implement GODEBUG=zipinsecurepath=0 #58641 (closed)

Related Code Changes

• archive/zip: detect and reject Better Zip Bomb
• archive/zip: only preallocate File slice if reasonably sized

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[ianlancetaylor]

A replacement for zip.NewReader that does not populate the Reader.File list, with some other way of accessing the files. (A method returning the number of files and another returning an iterator, maybe?)

I think we should do this, plus a GODEBUG that disables allocating the File slice in NewReader and OpenReader. Then after a couple of releases we can deprecate the File slice. Then after another couple of releases we can perhaps flip the GODEBUG default.

[mwriter]

A replacement for zip.NewReader that does not populate the Reader.File list, with some other way of accessing the files. (A method returning the number of files and another returning an iterator, maybe?)

Personally, I've been using a function like func FilesWith(path string, opts Options) iter.Seq2[*zip.File, error] for a long time.
But you can implement several options to choose from at once.

[mwriter]

and OpenReader

Yep, this is also an important point.

Then after a couple of releases we can deprecate the File slice. Then after another couple of releases we can perhaps flip the GODEBUG default.

Won't it have to be done at the same time? After all, after deleting the field, the setting in GODEBUG will no longer make sense.

[ianlancetaylor]

I suggested deprecating the File slice, not deleting it. Code that refers to File will continue to work. Tools like gopls will issue warnings about using File.

[neild]

A possible option is an archive/zip/v2, which would also give us the opportunity to make Reader reject unsafe filenames (absolute paths, relative paths that escape cwd, probably any relative path) by default.

We'd want to be pretty sure we weren't going to want a /v3 in short order, though.

[mwriter]

I suggested deprecating the File slice, not deleting it. Code that refers to File will continue to work

I've seen this before and I really didn't like it :)
But this isn't cryptography, and indeed it's beneficial for everyone, so any option will be gratefully accepted.

A possible option is an archive/zip/v2

In principle, this is also a proven approach, if you really care about real compatibility. But this way is more difficult to support.

reject unsafe filenames (absolute paths, relative paths that escape cwd, probably any relative path)

Yep, I had to write all this myself. It would be great if it became customizable for everyone.

[dmitshur]

CC @dsnet, @bradfitz.