archive/zip: only preallocate File slice if reasonably sized

rolandshoemaker · neild · commit 74242baa4136 · 2021-05-25T23:41:42.000Z
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. Fixes #46242 Fixes CVE-2021-33196 Change-Id: I3c76d8eec178468b380d87fdb4a3f2cb06f0ee76 Reviewed-on: https://go-review.googlesource.com/c/go/+/318909 Trust: Roland Shoemaker <roland@golang.org> Trust: Katie Hockman <katie@golang.org> Trust: Joe Tsai <thebrokentoaster@gmail.com> Run-TryBot: Roland Shoemaker <roland@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Katie Hockman <katie@golang.org> Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
diff --git a/src/archive/zip/reader.go b/src/archive/zip/reader.go
@@ -96,7 +96,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
@@ -1325,3 +1325,62 @@ func TestReadDataDescriptor(t *testing.T) {
 		})
 	}
 }
+
+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))
+	}
+}
