archive/tar, archive/zip: return ErrInsecurePath for unsafe paths

Return a distinguishable error when reading an archive file
with a path that is:

	- absolute
	- escapes the current directory (../a)
	- on Windows, a reserved name such as NUL

Users may ignore this error and proceed if they do not need name
sanitization or intend to perform it themselves.

Fixes #25849
Fixes #55356

Change-Id: Ieefa163f00384bc285ab329ea21a6561d39d8096
Reviewed-on: https://go-review.googlesource.com/c/go/+/449937
Reviewed-by: Joseph Tsai <joetsai@digital-static.net>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>

api/next/55356.txt

@@ -0,0 +1,2 @@
+pkg archive/tar, var ErrInsecurePath error #55356
+pkg archive/zip, var ErrInsecurePath error #55356

doc/go1.20.html

@@ -281,8 +281,33 @@ proxyHandler := &httputil.ReverseProxy{
   TODO: complete this section
 </p>
 
+<dl id="archive/tar"><dt><a href="/pkg/archive/tar/">archive/tar</a></dt>
+  <dd>
+    <p><!-- https://go.dev/issue/55356 -->
+      <code>(*Reader).Next</code> will now return the error <code>ErrInsecurePath</code>
+      when opening an archive which contains file names that are absolute,
+      refer to a location outside the current directory, contain invalid
+      characters, or (on Windows) are reserved names such as <code>NUL</code>.
+    </p>
+    <p>
+      Programs that want to operate on archives containing insecure file names may
+      ignore this error.
+    </p>
+  </dd>
+</dl><!-- archive/tar -->
+
 <dl id="archive/zip"><dt><a href="/pkg/archive/zip/">archive/zip</a></dt>
   <dd>
+    <p><!-- https://go.dev/issue/55356 -->
+      <code>NewReader</code> will now return the error <code>ErrInsecurePath</code>
+      when opening an archive which contains file names that are absolute,
+      refer to a location outside the current directory, contain invalid
+      characters, or (on Windows) are reserved names such as <code>NUL</code>.
+    </p>
+    <p>
+      Programs that want to operate on archives containing insecure file names may
+      ignore this error.
+    </p>
     <p><!-- CL 449955 -->
       Reading from a directory file that contains file data will now return an error.
       The zip specification does not permit directory files to contain file data,

src/archive/tar/common.go

@@ -31,6 +31,7 @@ var (
 	ErrWriteTooLong    = errors.New("archive/tar: write too long")
 	ErrFieldTooLong    = errors.New("archive/tar: header field too long")
 	ErrWriteAfterClose = errors.New("archive/tar: write after close")
+	ErrInsecurePath    = errors.New("archive/tar: insecure file path")
 	errMissData        = errors.New("archive/tar: sparse file references non-existent data")
 	errUnrefData       = errors.New("archive/tar: sparse file contains unreferenced data")
 	errWriteHole       = errors.New("archive/tar: write non-NUL byte in sparse hole")

src/archive/tar/reader.go

@@ -7,6 +7,7 @@ package tar
 import (
 	"bytes"
 	"io"
+	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
@@ -44,12 +45,24 @@ func NewReader(r io.Reader) *Reader {
 // Any remaining data in the current file is automatically discarded.
 //
 // io.EOF is returned at the end of the input.
+//
+// ErrInsecurePath and a valid *Header are returned if the next file's name is:
+//
+//   - absolute;
+//   - a relative path escaping the current directory, such as "../a"; or
+//   - on Windows, a reserved file name such as "NUL".
+//
+// The caller may ignore the ErrInsecurePath error,
+// but is then responsible for sanitizing paths as appropriate.
 func (tr *Reader) Next() (*Header, error) {
 	if tr.err != nil {
 		return nil, tr.err
 	}
 	hdr, err := tr.next()
 	tr.err = err
+	if err == nil && !filepath.IsLocal(hdr.Name) {
+		err = ErrInsecurePath
+	}
 	return hdr, err
 }
 

src/archive/tar/reader_test.go

@@ -1615,3 +1615,40 @@ func TestFileReader(t *testing.T) {
 		}
 	}
 }
+
+func TestInsecurePaths(t *testing.T) {
+	for _, path := range []string{
+		"../foo",
+		"/foo",
+		"a/b/../../../c",
+	} {
+		var buf bytes.Buffer
+		tw := NewWriter(&buf)
+		tw.WriteHeader(&Header{
+			Name: path,
+		})
+		const securePath = "secure"
+		tw.WriteHeader(&Header{
+			Name: securePath,
+		})
+		tw.Close()
+
+		tr := NewReader(&buf)
+		h, err := tr.Next()
+		if err != ErrInsecurePath {
+			t.Errorf("tr.Next for file %q: got err %v, want ErrInsecurePath", path, err)
+			continue
+		}
+		if h.Name != path {
+			t.Errorf("tr.Next for file %q: got name %q, want %q", path, h.Name, path)
+		}
+		// Error should not be sticky.
+		h, err = tr.Next()
+		if err != nil {
+			t.Errorf("tr.Next for file %q: got err %v, want nil", securePath, err)
+		}
+		if h.Name != securePath {
+			t.Errorf("tr.Next for file %q: got name %q, want %q", securePath, h.Name, securePath)
+		}
+	}
+}

src/archive/tar/writer_test.go

@@ -780,7 +780,7 @@ func TestUSTARLongName(t *testing.T) {
 	// Test that we can get a long name back out of the archive.
 	reader := NewReader(&buf)
 	hdr, err = reader.Next()
-	if err != nil {
+	if err != nil && err != ErrInsecurePath {
 		t.Fatal(err)
 	}
 	if hdr.Name != longName {
@@ -995,7 +995,7 @@ func TestIssue12594(t *testing.T) {
 
 		tr := NewReader(&b)
 		hdr, err := tr.Next()
-		if err != nil {
+		if err != nil && err != ErrInsecurePath {
 			t.Errorf("test %d, unexpected Next error: %v", i, err)
 		}
 		if hdr.Name != name {

src/archive/zip/reader.go

@@ -14,6 +14,7 @@ import (
 	"io/fs"
 	"os"
 	"path"
+	"path/filepath"
 	"sort"
 	"strings"
 	"sync"
@@ -21,9 +22,10 @@ import (
 )
 
 var (
-	ErrFormat    = errors.New("zip: not a valid zip file")
-	ErrAlgorithm = errors.New("zip: unsupported compression algorithm")
-	ErrChecksum  = errors.New("zip: checksum error")
+	ErrFormat       = errors.New("zip: not a valid zip file")
+	ErrAlgorithm    = errors.New("zip: unsupported compression algorithm")
+	ErrChecksum     = errors.New("zip: checksum error")
+	ErrInsecurePath = errors.New("zip: insecure file path")
 )
 
 // A Reader serves content from a ZIP archive.
@@ -82,6 +84,17 @@ func OpenReader(name string) (*ReadCloser, error) {
 
 // NewReader returns a new Reader reading from r, which is assumed to
 // have the given size in bytes.
+//
+// ErrInsecurePath and a valid *Reader are returned if the names of any
+// files in the archive:
+//
+//   - are absolute;
+//   - are a relative path escaping the current directory, such as "../a";
+//   - contain a backslash (\) character; or
+//   - on Windows, are a reserved file name such as "NUL".
+//
+// The caller may ignore the ErrInsecurePath error,
+// but is then responsible for sanitizing paths as appropriate.
 func NewReader(r io.ReaderAt, size int64) (*Reader, error) {
 	if size < 0 {
 		return nil, errors.New("zip: size cannot be negative")
@@ -90,6 +103,17 @@ func NewReader(r io.ReaderAt, size int64) (*Reader, error) {
 	if err := zr.init(r, size); err != nil {
 		return nil, err
 	}
+	for _, f := range zr.File {
+		if f.Name == "" {
+			// Zip permits an empty file name field.
+			continue
+		}
+		// The zip specification states that names must use forward slashes,
+		// so consider any backslashes in the name insecure.
+		if !filepath.IsLocal(f.Name) || strings.Contains(f.Name, `\`) {
+			return zr, ErrInsecurePath
+		}
+	}
 	return zr, nil
 }
 

src/archive/zip/reader_test.go

@@ -1315,7 +1315,7 @@ func TestCVE202127919(t *testing.T) {
 		0x00, 0x00, 0x59, 0x00, 0x00, 0x00, 0x00, 0x00,
 	}
 	r, err := NewReader(bytes.NewReader([]byte(data)), int64(len(data)))
-	if err != nil {
+	if err != ErrInsecurePath {
 		t.Fatalf("Error reading the archive: %v", err)
 	}
 	_, err = r.Open("test.txt")
@@ -1484,7 +1484,7 @@ func TestCVE202141772(t *testing.T) {
 		0x00, 0x90, 0x00, 0x00, 0x00, 0x00, 0x00,
 	}
 	r, err := NewReader(bytes.NewReader([]byte(data)), int64(len(data)))
-	if err != nil {
+	if err != ErrInsecurePath {
 		t.Fatalf("Error reading the archive: %v", err)
 	}
 	entryNames := []string{`/`, `//`, `\`, `/test.txt`}
@@ -1584,3 +1584,35 @@ func TestIssue54801(t *testing.T) {
 		}
 	}
 }
+
+func TestInsecurePaths(t *testing.T) {
+	for _, path := range []string{
+		"../foo",
+		"/foo",
+		"a/b/../../../c",
+		`a\b`,
+	} {
+		var buf bytes.Buffer
+		zw := NewWriter(&buf)
+		_, err := zw.Create(path)
+		if err != nil {
+			t.Errorf("zw.Create(%q) = %v", path, err)
+			continue
+		}
+		zw.Close()
+
+		zr, err := NewReader(bytes.NewReader(buf.Bytes()), int64(buf.Len()))
+		if err != ErrInsecurePath {
+			t.Errorf("NewReader for archive with file %q: got err %v, want ErrInsecurePath", path, err)
+			continue
+		}
+		var gotPaths []string
+		for _, f := range zr.File {
+			gotPaths = append(gotPaths, f.Name)
+		}
+		if !reflect.DeepEqual(gotPaths, []string{path}) {
+			t.Errorf("NewReader for archive with file %q: got files %q", path, gotPaths)
+			continue
+		}
+	}
+}

src/archive/zip/struct.go

@@ -85,13 +85,6 @@ type FileHeader struct {
 	// It must be a relative path, not start with a drive letter (such as "C:"),
 	// and must use forward slashes instead of back slashes. A trailing slash
 	// indicates that this file is a directory and should have no data.
-	//
-	// When reading zip files, the Name field is populated from
-	// the zip file directly and is not validated for correctness.
-	// It is the caller's responsibility to sanitize it as
-	// appropriate, including canonicalizing slash directions,
-	// validating that paths are relative, and preventing path
-	// traversal through filenames ("../../../").
 	Name string
 
 	// Comment is any arbitrary user-defined string shorter than 64KiB.