+ (*Reader).Next will now return the error ErrInsecurePath
+ 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 NUL.
+
+ Programs that want to operate on archives containing insecure file names may + ignore this error. +
+
+ NewReader will now return the error ErrInsecurePath
+ 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 NUL.
+
+ Programs that want to operate on archives containing insecure file names may + ignore this error. +
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, diff --git a/src/archive/tar/common.go b/src/archive/tar/common.go index f6d701d925c..be02a24542c 100644 --- a/src/archive/tar/common.go +++ b/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") diff --git a/src/archive/tar/reader.go b/src/archive/tar/reader.go index 45848304ed9..44166b4cdf9 100644 --- a/src/archive/tar/reader.go +++ b/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 } diff --git a/src/archive/tar/reader_test.go b/src/archive/tar/reader_test.go index 247030da57d..91dc1650e2d 100644 --- a/src/archive/tar/reader_test.go +++ b/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) + } + } +} diff --git a/src/archive/tar/writer_test.go b/src/archive/tar/writer_test.go index 32af16e20f6..f6d75c58032 100644 --- a/src/archive/tar/writer_test.go +++ b/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 { diff --git a/src/archive/zip/reader.go b/src/archive/zip/reader.go index db9ae3cf366..b64c61aab50 100644 --- a/src/archive/zip/reader.go +++ b/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 } diff --git a/src/archive/zip/reader_test.go b/src/archive/zip/reader_test.go index 3123892fb76..45cb5bfec3a 100644 --- a/src/archive/zip/reader_test.go +++ b/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 + } + } +} diff --git a/src/archive/zip/struct.go b/src/archive/zip/struct.go index 6f73fb8376a..08af88b245a 100644 --- a/src/archive/zip/struct.go +++ b/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.