From c92adf420a3d9a5510f9aea382d826f0c9216a10 Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Tue, 11 May 2021 11:31:31 -0700 Subject: [release-branch.go1.15] archive/zip: only preallocate File slice if reasonably sized Since the number of files in the EOCD record isn't validated, it isn't safe to preallocate Reader.Files using that field. A malformed archive can indicate it contains up to 1 << 128 - 1 files. We can still safely preallocate the slice by checking if the specified number of files in the archive is reasonable, given the size of the archive. Thanks to the OSS-Fuzz project for discovering this issue and to Emmanuel Odeke for reporting it. Updates #46242 Fixes #46396 Fixes CVE-2021-33196 Change-Id: I3c76d8eec178468b380d87fdb4a3f2cb06f0ee76 Reviewed-on: https://go-review.googlesource.com/c/go/+/318909 Trust: Roland Shoemaker Trust: Katie Hockman Trust: Joe Tsai Run-TryBot: Roland Shoemaker TryBot-Result: Go Bot Reviewed-by: Katie Hockman Reviewed-by: Joe Tsai (cherry picked from commit 74242baa4136c7a9132a8ccd9881354442788c8c) Reviewed-on: https://go-review.googlesource.com/c/go/+/322949 Reviewed-by: Filippo Valsorda --- src/archive/zip/reader.go | 10 ++++++- src/archive/zip/reader_test.go | 59 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/src/archive/zip/reader.go b/src/archive/zip/reader.go index 13ff9ddcf42..2d5151af3da 100644 --- a/src/archive/zip/reader.go +++ b/src/archive/zip/reader.go @@ -84,7 +84,15 @@ func (z *Reader) init(r io.ReaderAt, size int64) error { return err } z.r = r - z.File = make([]*File, 0, end.directoryRecords) + // Since the number of directory records is not validated, it is not + // safe to preallocate z.File without first checking that the specified + // number of files is reasonable, since a malformed archive may + // indicate it contains up to 1 << 128 - 1 files. Since each file has a + // header which will be _at least_ 30 bytes we can safely preallocate + // if (data size / 30) >= end.directoryRecords. + if (uint64(size)-end.directorySize)/30 >= end.directoryRecords { + z.File = make([]*File, 0, end.directoryRecords) + } z.Comment = end.comment rs := io.NewSectionReader(r, 0, size) if _, err = rs.Seek(int64(end.directoryOffset), io.SeekStart); err != nil { diff --git a/src/archive/zip/reader_test.go b/src/archive/zip/reader_test.go index adca87a8b38..6f67d2e4a94 100644 --- a/src/archive/zip/reader_test.go +++ b/src/archive/zip/reader_test.go @@ -1070,3 +1070,62 @@ func TestIssue12449(t *testing.T) { t.Errorf("Error reading the archive: %v", err) } } + +func TestCVE202133196(t *testing.T) { + // Archive that indicates it has 1 << 128 -1 files, + // this would previously cause a panic due to attempting + // to allocate a slice with 1 << 128 -1 elements. + data := []byte{ + 0x50, 0x4b, 0x03, 0x04, 0x14, 0x00, 0x08, 0x08, + 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x01, 0x02, + 0x03, 0x62, 0x61, 0x65, 0x03, 0x04, 0x00, 0x00, + 0xff, 0xff, 0x50, 0x4b, 0x07, 0x08, 0xbe, 0x20, + 0x5c, 0x6c, 0x09, 0x00, 0x00, 0x00, 0x03, 0x00, + 0x00, 0x00, 0x50, 0x4b, 0x01, 0x02, 0x14, 0x00, + 0x14, 0x00, 0x08, 0x08, 0x08, 0x00, 0x00, 0x00, + 0x00, 0x00, 0xbe, 0x20, 0x5c, 0x6c, 0x09, 0x00, + 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x03, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x01, 0x02, 0x03, 0x50, 0x4b, 0x06, 0x06, 0x2c, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x2d, + 0x00, 0x2d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0x31, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x3a, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x50, 0x4b, 0x06, 0x07, 0x00, + 0x00, 0x00, 0x00, 0x6b, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x50, + 0x4b, 0x05, 0x06, 0x00, 0x00, 0x00, 0x00, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0x00, 0x00, + } + _, err := NewReader(bytes.NewReader(data), int64(len(data))) + if err != ErrFormat { + t.Fatalf("unexpected error, got: %v, want: %v", err, ErrFormat) + } + + // Also check that an archive containing a handful of empty + // files doesn't cause an issue + b := bytes.NewBuffer(nil) + w := NewWriter(b) + for i := 0; i < 5; i++ { + _, err := w.Create("") + if err != nil { + t.Fatalf("Writer.Create failed: %s", err) + } + } + if err := w.Close(); err != nil { + t.Fatalf("Writer.Close failed: %s", err) + } + r, err := NewReader(bytes.NewReader(b.Bytes()), int64(b.Len())) + if err != nil { + t.Fatalf("NewReader failed: %s", err) + } + if len(r.File) != 5 { + t.Errorf("Archive has unexpected number of files, got %d, want 5", len(r.File)) + } +} -- cgit v1.2.3-54-g00ecf