proposal: archive/tar, archive/zip: add NewReaderOptions with directory traversal defenses

[neild]

This is another attempt at hardening the archive/tar and archive/zip packages against file traversal attacks, fixing #25849.

Naive handling of archives containing filenames such as ../../../etc/passwd leads to security vulnerabilities. (See #55356 for a bunch of reference CVEs.) In #55356, I proposed changing archive/tar and archive/zip to return an ErrInsecurePath error when encountering an unsafe filename. Unfortunately, we subsequently discovered that Docker images frequently include filenames with absolute paths, making this change infeasible.

I propose adding new safe-by-default functions to archive/tar and archive/zip.

package tar

type ReaderOptions struct {
	AllowAbsolutePaths          bool
	AllowRelativePathComponents bool
}

// NewReaderWithOptions creates a new reader reading from r.
//
// The Reader's Next method will return ErrInsecurePath and a valid
// *Header if the next file's name is:
//
//   - absolute and opts.AllowAbsolutePaths is not set;
//   - contains a ".." path component and opts.AllowRelativePathComponents is not set; or
//   - on Windows, a reserved file name such as "NUL".
func NewReaderWithOptions(r io.Reader, opts ReaderOptions) *Reader

package zip

type ReaderOptions struct {
	AllowAbsolutePaths          bool
	AllowRelativePathComponents bool
}

// NewReaderWithOptions returns a new Reader reading from r,
// which is assumed to have the given size in bytes.
//
// 
// NewReaderWithOptions will return ErrInsecurePath and a valid
// *Reader if the archive contains file names that are:
//
//   - absolute and opts.AllowAbsolutePaths is not set;
//   - contains a ".." path component and opts.AllowRelativePathComponents is not set;
//   - contain a backslash (\) character; or
//   - on Windows, a reserved file name such as "NUL".
func NewReaderWithOptions(r io.ReaderAt, size int64, opts ReaderOptions) (*Reader, error)

The NewReaderWithOptions functions will provide the same functionality as proposed in #55356, but in a new API to avoid breaking existing users. In addition, these functions provide an easy way for users who want to accept some forms of unsafe path to do so while still defending against unexpected cases: For example, permitting absolute filenames while rejecting ones containing unexpected backslash characters.

I further propose that we update the NewReader documentation to encourage all users to migrate to NewReaderWithOptions. Once all supported Go versions include NewReaderWithOptions, we can go further and deprecate NewReader. We might also consider changing NewReader to be safe by default at some point, although that decision is out of scope for this proposal.

[ianlancetaylor]

Bikeshed: instead of NewReaderWithOptions, call the function Open. This corresponds to the function os.Open which opens a file for read-only access.

[rsc]

What happened with the errors being introduced in Go 1.20? I thought the plan was to turn them on by default in some newer release?

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[neild]

We stated that we might turn on name security checks, but the pervasiveness of Docker tar files with absolute paths makes me dubious that we will ever be able to do so without causing widespread breakage.

Adding a new safe-by-default function could give us a path to making that change: First we add the safe function under a different name, then we deprecate the unsafe functions, and when a sufficient portion of the ecosystem has moved off of the unsafe functions we might be able to change them to be safe by default and even undeprecate them.

Open is an interesting choice of name. Unfortunately, archive/zip has an OpenReader function which opens a file rather than an io.ReaderAt; an Open function would be confusing there. I overlooked OpenReader when writing this proposal; we should also have a safe variant of it as well, such as OpenReaderWithOptions.

[rsc]

This still feels like it's getting very complicated. One way it can be hard to write secure code is when there are too many ways to do it, and it feels like we're headed down that road. Can we simplify this?

How many different programs or libraries read "Docker tar files"? If those update to recognize the new errors, that problem is fixed, right?

[dsnet]

I would expect NewReader and NewReaderWithOptions(ReaderOptions{}) to be the same, but that's not the case since the former is insecure, while the latter is secure.

Adding a new safe-by-default function...

Safe-by-default, but also one that's just much harder to call than NewReader. Even if we rename NewReaderWithOptions to just Open, it's still harder to call without #12854. That's quite unfortunate. I'm personally in favor of changing NewReader and directing callers depending on absolute paths to ignore ErrInsecurePath.

Also, I personally find Open a confusing name for this since it doesn't automatically open a file from the FS.

[neild]

This still feels like it's getting very complicated. One way it can be hard to write secure code is when there are too many ways to do it, and it feels like we're headed down that road. Can we simplify this?

How many different programs or libraries read "Docker tar files"? If those update to recognize the new errors, that problem is fixed, right?

We could simplify this by adding a safe-by-default API with no options, or with fewer options. For example:

package tar
func SafeReader(r io.Reader) *Reader

The alternative is to change the existing readers as proposed in #55356. I don't know how many programs would need to be updated to handle this change. Docker is pretty popular, though. I'll try running Google-internal tests (which include a fair bit of third-party package coverage) with GODEBUG=tarinsecurepath=0,zipinsecurepath=0 and report back on how many things break. (We did this in the runup to 1.20, but I don't have the failure list to hand any more.)

[loveyandex]

hi hi

[rsc]

We could add NewSafeReader, but I don't think we've exhausted trying #55356 (default on) yet. We should probably do that first or else understand why we can't ever do that. The Google test failures looked like they had a small number of root causes. Docker may be popular but I don't think direct reading of Docker tar files is nearly as popular.

[csmith]

This is a noble goal but it feels as though the proposal is trying to fix the issue in the wrong place. A path in an archive isn't inherently secure or insecure, so why is the reader concerning itself with making that decision and returning an error? It feels a little like trying to prevent SQL injection by making http.Request.PostFormValue() aware of 'dangerous' SQL characters.

Would it make more sense to add API to handle extracting an archive to a filesystem directly? That would remove a lot of the "boilerplate" you need to write at present, and provide a sensible place for the "secure or not" determination to live where it can make OS-specific decisions.