go: archive/zip: Reader.Open panics on empty string

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.

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

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

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

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 https://github.com/golang/go/issues/48085#issuecomment-912659635 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.

When the zip file exist absolute path entry, in function initFileList, the toValidName will remove / or ../ element in front of the path and change it to the relative path entry and add it to the reader.fileList, So even we do not use fs.ValidPathto check the path when via archive/zip.(*ReadCloser).Open that corresponds with an absolute path entry, it also will return fs.ErrNotExist. Actually the files are in the zip file, but Go return the result of file not exist which become more confused.

Actually the comment of archive/zip.(*ReadCloser).Open clearly tells developers not to open the filename leading with / or ../ elements.

// Open opens the named file in the ZIP archive,
// using the semantics of fs.FS.Open:
// paths are always slash separated, with no
// leading / or ../ elements.
func (r *Reader) Open(name string) (fs.File, error) {

So to better distinguish the two error cases, return the fs.PathError{fs.ErrInvalid} when open the filename leading with / or …/ is more accurate from my point.