archive/zip: Reader.Open panics on empty string

[colin-sitehost]

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

$ go version
go version go1.17 linux/amd64

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/user/go/pkg/mod"
GOOS="linux"
GOPATH="/home/colin/go"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2958766942=/tmp/go-build -gno-record-gcc-switches"

What did you do?

r, _ := zip.NewReader(bytes.NewReader(raw), int64(len(raw)))
r.Open("")

https://play.golang.org/p/7x5dvzJSoCY

What did you expect to see?

What did you see instead?

panic: runtime error: index out of range [-1]

goroutine 1 [running]:
archive/zip.split(...)
	/usr/local/go-faketime/src/archive/zip/reader.go:800
archive/zip.(*Reader).openLookup(0x0, {0x0, 0xffffffffffffffff})
	/usr/local/go-faketime/src/archive/zip/reader.go:821 +0x265
archive/zip.(*Reader).Open(0x4e1360, {0x0, 0x16})
	/usr/local/go-faketime/src/archive/zip/reader.go:785 +0x45
main.main()
	/tmp/sandbox521722687/prog.go:11 +0xb2

Program exited: status 2.

Anything else?

This was discovered via io/fs.WalkDir, but the root cause is Reader.Open: https://play.golang.org/p/WWHxZRKu2gY:

panic: runtime error: index out of range [-1]

goroutine 1 [running]:
archive/zip.split(...)
	/usr/local/go-faketime/src/archive/zip/reader.go:800
archive/zip.(*Reader).openLookup(0x40c6a7, {0x0, 0x4bb740})
	/usr/local/go-faketime/src/archive/zip/reader.go:821 +0x265
archive/zip.(*Reader).Open(0x40c354, {0x0, 0xc000134000})
	/usr/local/go-faketime/src/archive/zip/reader.go:785 +0x45
io/fs.Stat({0x4e4060, 0xc000134000}, {0x0, 0x0})
	/usr/local/go-faketime/src/io/fs/stat.go:25 +0x9c
io/fs.WalkDir({0x4e4060, 0xc000134000}, {0x0, 0x0}, 0x0)
	/usr/local/go-faketime/src/io/fs/walk.go:108 +0x3c
main.main()
	/tmp/sandbox3566573894/prog.go:12 +0xbe

Program exited: status 2.

I was not expecting this to work, since most other fs.FS implementations (say embed.FS) return a not found error when root == "", but a panic seemed like a bug.

[mengzhuo]

go/src/archive/zip/reader.go

Lines 800 to 804 in d384ebd

	if name[len(name)-1] == '/' {
	isDir = true
	name = name[:len(name)-1]
	}
	i := len(name) - 1

It seems to be a bug, I'll try to fix it.

[gopherbot]

Change https://golang.org/cl/346349 mentions this issue: archive/zip: do path valid check first in Open

[colin-sitehost]

does this qualify for backporting to the current releases? (happy to assist with that effort)

[mengzhuo]

cc @dsnet @rolandshoemaker

[Strum355]

Also having the same issue/symptoms, code and zip (actually JAR) file attached. Of note when printing the file names from reader.Files (as commented out below) is that there are absolute paths including a /. See picture at end

Details

package main

import (
	"archive/zip"
	"io"
	"os"
	"path"
)

const destination = "./extracted"

func main() {
	reader, err := zip.OpenReader("liquibase-core-4.4.3-sources.zip")
	if err != nil {
		panic(err)
	}
	defer reader.Close()

	// names := []string{}

	// for _, file := range reader.File {
	// 	names = append(names, "'"+file.Name+"'")
	// }

	// fmt.Println(strings.Join(names, "\n"))

	for _, file := range reader.File {
		outputPath := path.Join(destination, file.Name)
		err := copyZipFileEntry(reader, file, outputPath)
		if err != nil {
			panic(err)
		}
	}
}

func copyZipFileEntry(reader *zip.ReadCloser, entry *zip.File, outputPath string) (err error) {
	inputFile, err := reader.Open(entry.Name)
	if err != nil {
		return err
	}
	defer func() {
		err1 := inputFile.Close()
		if err == nil {
			err = err1
		}
	}()

	if err = os.MkdirAll(path.Dir(outputPath), 0700); err != nil {
		return err
	}
	outputFile, err := os.OpenFile(outputPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600)
	if err != nil {
		return err
	}
	defer func() {
		err1 := outputFile.Close()
		if err == nil {
			err = err1
		}
	}()

	_, err = io.Copy(outputFile, inputFile)
	return err
}

liquibase-core-4.4.3-sources.zip

[Strum355]

Our stacktrace is slightly different, but ultimately still hits the panic in archive/zip.split. The change listed above would not address this for us, as we are trying to open a legitimate file META-INF/MANIFEST.MF and hitting the panic on the first (and only) call to initFileList

panic: runtime error: index out of range [-1]

goroutine 1 [running]:
archive/zip.split(...)
        /usr/lib/go/src/archive/zip/reader.go:800
archive/zip.fileEntryLess({0xc000168900, 0x4bacc0}, {0x569758, 0xc00021e000})
        /usr/lib/go/src/archive/zip/reader.go:774 +0x1e5
archive/zip.(*Reader).initFileList.func1.1(0xc00007ca88, 0x493)
        /usr/lib/go/src/archive/zip/reader.go:768 +0x4f
sort.doPivot_func({0xc00007ca88, 0xc0001a92f0}, 0x0, 0x4e0)
        /usr/lib/go/src/sort/zfuncversion.go:87 +0x1d9
sort.quickSort_func({0xc00007ca88, 0xc0001a92f0}, 0xc00000c060, 0x0, 0xc00007cac8)
        /usr/lib/go/src/sort/zfuncversion.go:143 +0x85
sort.Slice({0x4afa00, 0xc00000c060}, 0x500)
        /usr/lib/go/src/sort/slice.go:20 +0x9f
archive/zip.(*Reader).initFileList.func1()
        /usr/lib/go/src/archive/zip/reader.go:768 +0x42a
sync.(*Once).doSlow(0x0, 0x0)
        /usr/lib/go/src/sync/once.go:68 +0xd2
sync.(*Once).Do(...)
        /usr/lib/go/src/sync/once.go:59
archive/zip.(*Reader).initFileList(0x0)
        /usr/lib/go/src/archive/zip/reader.go:738 +0x45
archive/zip.(*Reader).Open(0x462b3e, {0xc000020228, 0xc00007cdc8})
        /usr/lib/go/src/archive/zip/reader.go:783 +0x2c
main.copyZipFileEntry(0x4e7e00, 0xc00000e018, {0xc0001b0dc0, 0x1e})
        /home/noah/Sourcegraph/coursiertest/main.go:40 +0x65
main.main()
        /home/noah/Sourcegraph/coursiertest/main.go:32 +0x377
exit status 2

[Strum355]

Screenshot showing debugger state when calling initFileList. Note the filename / being transformed to empty string. Simply doing a check after toValidName for empty string and doing continue results in panic: open /: file does not exist later in program execution, which while could be ignored, doesn't seem like a proper solution imo but also not sure what the behaviour should be in that case.

[colin-sitehost]

per the spec, what you provided is an invalid zip file:

   4.4.17.1 The name of the file, with optional relative path.
  The path stored MUST NOT contain a drive or
  device letter, or a leading slash.  All slashes
  MUST be forward slashes '/' as opposed to
  backwards slashes '\' for compatibility with Amiga
  and UNIX file systems etc.  If input came from standard
  input, there is no file name field.  

As such, I think it is totally valid for go to reject archives with input path names that are absolute (either /path/to/ or C:/path/to); that being said, for compatibility, we could chose to translate them.

---

it looks like Info-ZIP helps out during unzip:

warning:  stripped absolute path spec from /var/jenkins/workspace/liquibasePro/4.4.x/53/liquibase/liquibase-dist/target/archive/licenses/epl-1.0.txt
  inflating: var/jenkins/workspace/liquibasePro/4.4.x/53/liquibase/liquibase-dist/target/archive/licenses/epl-1.0.txt
warning:  stripped absolute path spec from /var/jenkins/workspace/liquibasePro/4.4.x/53/liquibase/liquibase-dist/target/archive/licenses/isc.txt
  inflating: var/jenkins/workspace/liquibasePro/4.4.x/53/liquibase/liquibase-dist/target/archive/licenses/isc.txt
warning:  stripped absolute path spec from /var/jenkins/workspace/liquibasePro/4.4.x/53/liquibase/liquibase-dist/target/archive/licenses/lgpl-3.0.txt
  inflating: var/jenkins/workspace/liquibasePro/4.4.x/53/liquibase/liquibase-dist/target/archive/licenses/lgpl-3.0.txt

but it refuses to create one:

[user@localhost ~]$  zip info /path/to/file
[user@localhost ~]$ unzip -l info.zip
Archive:  info.zip
Length      Date    Time    Name
---------  ---------- -----   ----
   2075  2021-09-02 22:41   path/to/file
---------                     -------
   2075                     1 file

[Strum355]

As such, I think it is totally valid for go to panic on input path names that are absolute

I very strongly disagree with this. This was a cause of panics in production for us, and as a result, regardless of what the spec says, I strongly believe that archive/zip should not panic in such a scenario, as absolute paths in zips can and do exist in real-world files. If anything, I would consider this issue almost as serious as two previous panics in archive/zip, both of which have a CVE (first and second). Crafted/invalid input should not cause a panic, it should be handled appropriately and defensively.

[colin-sitehost]

I agree that this panic seems accidental, and we could probably return an error on NewReader, (I have reworded my previous comment to reflect this reality); but it is pretty common for stdlib to panic in absurd conditions (index out of bound, nil pointer, etc.), especially when it makes for a cleaner interface.

[Strum355]

but it is pretty common for stdlib to panic in absurd conditions (index out of bound, nil pointer, etc.), especially when it makes for a cleaner interface.

I don't believe that this case is comparable, nor that the scenario is particularly "absurd" when real world files exist with this characteristic. There is a precedent from the above linked CVEs (most particularly the second one) that the panic from absolute paths should be addressed, as well as a precedent from other utilities that handle this particular case.

I completely understand that the case wouldn't have been thought of while the code was written, I lay no blame on anyone for that, Im simply hoping for the scenario to be handled in such a way that it is still possible for me to consume the zip file and do with the erroneous paths as I wish (in this case, we would skip extracting them).

Of additional note, the issue is not triggered when I do *archive/zip.File.Open, but it is triggered when I do *archive/zip.Reader.Open, so theres a definitive inconsistency there that should be addressed. Also do observe that the panic is triggered not when I try to access an absolute path file, but when I was trying to access META-INF/MANIFEST.MF.

Regardless of our thoughts, I was strongly recommended to notify the security team of this issue, which I have 🙂 I appreciate your thoughts nonetheless

[FiloSottile]

Thank you for reporting this to security@golang.org. Invalid input should not cause programs to panic, if the input could be attacker controlled. If this required a call to Open("") to trigger, it could have been borderline, since it's hard for an attacker to control the argument to Open. However, the reproducer in #48085 (comment) triggers a panic with a real file name.

package main

import "archive/zip"

func main() {
	reader, err := zip.OpenReader("liquibase-core-4.4.3-sources.zip")
	if err != nil {
		panic(err)
	}

	reader.Open("META-INF/MANIFEST.MF")
}

We'll backport this as a security fix in the PUBLIC track. @gopherbot, please open backport issues.