From a8af76284d4dadc7720b69cc3e1882865ce509e5 Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Mon, 24 Apr 2023 09:31:06 -0700 Subject: [PATCH] archive/zip: reject overflowing directorySize & directoryOffset We added a check for incorrect baseOffset in CL 408734, but in doing so we introduced a panic when directoryOffset overflowed a int64. The zip spec uses uint64, but since io.SectionReader requires int64 we convert, and possibly introduce an overflow. If offset < 0 && size-offset < 0, SectionReader will panic when we attempt to read from it. Since it's extremely unlikely we're ever going to process a zip file larger than 1<<63-1 byte, just limit directory size and offset to the max int64. Change-Id: I1aaa755cf4da927a6e12ef59f97dfc83a3426d86 Reviewed-on: https://go-review.googlesource.com/c/go/+/488195 Reviewed-by: Ian Lance Taylor Run-TryBot: Roland Shoemaker TryBot-Result: Gopher Robot Auto-Submit: Roland Shoemaker --- src/archive/zip/reader.go | 5 +++++ src/archive/zip/reader_test.go | 41 ++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/archive/zip/reader.go b/src/archive/zip/reader.go index ae296e6fa7..c0e8d97e4e 100644 --- a/src/archive/zip/reader.go +++ b/src/archive/zip/reader.go @@ -615,6 +615,11 @@ func readDirectoryEnd(r io.ReaderAt, size int64) (dir *directoryEnd, baseOffset } } + maxInt64 := uint64(1<<63 - 1) + if d.directorySize > maxInt64 || d.directoryOffset > maxInt64 { + return nil, 0, ErrFormat + } + baseOffset = directoryEndOffset - int64(d.directorySize) - int64(d.directoryOffset) // Make sure directoryOffset points to somewhere in our file. diff --git a/src/archive/zip/reader_test.go b/src/archive/zip/reader_test.go index f793e01e2b..a67c33598d 100644 --- a/src/archive/zip/reader_test.go +++ b/src/archive/zip/reader_test.go @@ -1785,3 +1785,44 @@ func TestCompressedDirectory(t *testing.T) { } } } + +func TestBaseOffsetPlusOverflow(t *testing.T) { + // directoryOffset > maxInt64 && size-directoryOffset < 0 + data := []byte{ + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0xff, 0xff, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x20, 0x20, 0x20, 0x50, 0x4b, 0x06, 0x06, 0x20, + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x20, 0xff, 0xff, 0x20, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x20, 0x08, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x80, 0x50, 0x4b, 0x06, 0x07, 0x00, + 0x00, 0x00, 0x00, 0x6b, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x50, + 0x4b, 0x05, 0x06, 0x20, 0x20, 0x20, 0x20, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0x20, 0x00, + } + defer func() { + if r := recover(); r != nil { + t.Fatalf("NewReader panicked: %s", r) + } + }() + // Previously, this would trigger a panic as we attempt to read from + // a io.SectionReader which would access a slice at a negative offset + // as the section reader offset & size were < 0. + NewReader(bytes.NewReader(data), int64(len(data))+1875) +} -- 2.50.0