Skip to content

Commit b212ba6

Browse files
Jason7602FiloSottile
authored and
katiehockman
committed
[release-branch.go1.17] archive/zip: don't panic on (*Reader).Open
Previously, opening a zip with (*Reader).Open could result in a panic if the zip contained a file whose name was exclusively made up of slash characters or ".." path elements. Open could also panic if passed the empty string directly as an argument. Now, any files in the zip whose name could not be made valid for fs.FS.Open will be skipped, and no longer added to the fs.FS file list, although they are still accessible through (*Reader).File. Note that it was already the case that a file could be accessible from (*Reader).Open with a name different from the one in (*Reader).File, as the former is the cleaned name, while the latter is the original one. Finally, made the actual panic site robust as a defense-in-depth measure. Fixes CVE-2021-41772 Fixes #48252 Updates #48085 Co-authored-by: Filippo Valsorda <[email protected]> Change-Id: I6271a3f2892e7746f52e213b8eba9a1bba974678 Reviewed-on: https://go-review.googlesource.com/c/go/+/349770 Run-TryBot: Filippo Valsorda <[email protected]> Reviewed-by: Katie Hockman <[email protected]> Reviewed-by: Filippo Valsorda <[email protected]> Trust: Katie Hockman <[email protected]> Trust: Julie Qiu <[email protected]> (cherry picked from commit b246873) Reviewed-on: https://go-review.googlesource.com/c/go/+/360859 Trust: Dmitri Shuralyov <[email protected]> Run-TryBot: Dmitri Shuralyov <[email protected]> TryBot-Result: Go Bot <[email protected]>
1 parent f9cb33c commit b212ba6

File tree

2 files changed

+136
-2
lines changed

2 files changed

+136
-2
lines changed

‎src/archive/zip/reader.go

+8-2
Original file line numberDiff line numberDiff line change
@@ -741,6 +741,9 @@ func (r *Reader) initFileList() {
741741
for _, file := range r.File {
742742
isDir := len(file.Name) > 0 && file.Name[len(file.Name)-1] == '/'
743743
name := toValidName(file.Name)
744+
if name == "" {
745+
continue
746+
}
744747
for dir := path.Dir(name); dir != "."; dir = path.Dir(dir) {
745748
dirs[dir] = true
746749
}
@@ -782,8 +785,11 @@ func fileEntryLess(x, y string) bool {
782785
func (r *Reader) Open(name string) (fs.File, error) {
783786
r.initFileList()
784787

788+
if !fs.ValidPath(name) {
789+
return nil, &fs.PathError{Op: "open", Path: name, Err: fs.ErrInvalid}
790+
}
785791
e := r.openLookup(name)
786-
if e == nil || !fs.ValidPath(name) {
792+
if e == nil {
787793
return nil, &fs.PathError{Op: "open", Path: name, Err: fs.ErrNotExist}
788794
}
789795
if e.isDir {
@@ -797,7 +803,7 @@ func (r *Reader) Open(name string) (fs.File, error) {
797803
}
798804

799805
func split(name string) (dir, elem string, isDir bool) {
800-
if name[len(name)-1] == '/' {
806+
if len(name) > 0 && name[len(name)-1] == '/' {
801807
isDir = true
802808
name = name[:len(name)-1]
803809
}

‎src/archive/zip/reader_test.go

+128
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"io/fs"
1414
"os"
1515
"path/filepath"
16+
"reflect"
1617
"regexp"
1718
"strings"
1819
"testing"
@@ -1202,6 +1203,15 @@ func TestCVE202127919(t *testing.T) {
12021203
if err != nil {
12031204
t.Errorf("Error reading file: %v", err)
12041205
}
1206+
if len(r.File) != 1 {
1207+
t.Fatalf("No entries in the file list")
1208+
}
1209+
if r.File[0].Name != "../test.txt" {
1210+
t.Errorf("Unexpected entry name: %s", r.File[0].Name)
1211+
}
1212+
if _, err := r.File[0].Open(); err != nil {
1213+
t.Errorf("Error opening file: %v", err)
1214+
}
12051215
}
12061216

12071217
func TestReadDataDescriptor(t *testing.T) {
@@ -1402,3 +1412,121 @@ func TestCVE202139293(t *testing.T) {
14021412
t.Fatalf("unexpected error, got: %v, want: %v", err, ErrFormat)
14031413
}
14041414
}
1415+
1416+
func TestCVE202141772(t *testing.T) {
1417+
// Archive contains a file whose name is exclusively made up of '/', '\'
1418+
// characters, or "../", "..\" paths, which would previously cause a panic.
1419+
//
1420+
// Length Method Size Cmpr Date Time CRC-32 Name
1421+
// -------- ------ ------- ---- ---------- ----- -------- ----
1422+
// 0 Stored 0 0% 08-05-2021 18:32 00000000 /
1423+
// 0 Stored 0 0% 09-14-2021 12:59 00000000 //
1424+
// 0 Stored 0 0% 09-14-2021 12:59 00000000 \
1425+
// 11 Stored 11 0% 09-14-2021 13:04 0d4a1185 /test.txt
1426+
// -------- ------- --- -------
1427+
// 11 11 0% 4 files
1428+
data := []byte{
1429+
0x50, 0x4b, 0x03, 0x04, 0x0a, 0x00, 0x00, 0x08,
1430+
0x00, 0x00, 0x06, 0x94, 0x05, 0x53, 0x00, 0x00,
1431+
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
1432+
0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x2f, 0x50,
1433+
0x4b, 0x03, 0x04, 0x0a, 0x00, 0x00, 0x00, 0x00,
1434+
0x00, 0x78, 0x67, 0x2e, 0x53, 0x00, 0x00, 0x00,
1435+
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
1436+
0x00, 0x02, 0x00, 0x00, 0x00, 0x2f, 0x2f, 0x50,
1437+
0x4b, 0x03, 0x04, 0x0a, 0x00, 0x00, 0x00, 0x00,
1438+
0x00, 0x78, 0x67, 0x2e, 0x53, 0x00, 0x00, 0x00,
1439+
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
1440+
0x00, 0x01, 0x00, 0x00, 0x00, 0x5c, 0x50, 0x4b,
1441+
0x03, 0x04, 0x0a, 0x00, 0x00, 0x00, 0x00, 0x00,
1442+
0x91, 0x68, 0x2e, 0x53, 0x85, 0x11, 0x4a, 0x0d,
1443+
0x0b, 0x00, 0x00, 0x00, 0x0b, 0x00, 0x00, 0x00,
1444+
0x09, 0x00, 0x00, 0x00, 0x2f, 0x74, 0x65, 0x73,
1445+
0x74, 0x2e, 0x74, 0x78, 0x74, 0x68, 0x65, 0x6c,
1446+
0x6c, 0x6f, 0x20, 0x77, 0x6f, 0x72, 0x6c, 0x64,
1447+
0x50, 0x4b, 0x01, 0x02, 0x14, 0x03, 0x0a, 0x00,
1448+
0x00, 0x08, 0x00, 0x00, 0x06, 0x94, 0x05, 0x53,
1449+
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
1450+
0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00,
1451+
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00,
1452+
0xed, 0x41, 0x00, 0x00, 0x00, 0x00, 0x2f, 0x50,
1453+
0x4b, 0x01, 0x02, 0x3f, 0x00, 0x0a, 0x00, 0x00,
1454+
0x00, 0x00, 0x00, 0x78, 0x67, 0x2e, 0x53, 0x00,
1455+
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
1456+
0x00, 0x00, 0x00, 0x02, 0x00, 0x24, 0x00, 0x00,
1457+
0x00, 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00,
1458+
0x00, 0x1f, 0x00, 0x00, 0x00, 0x2f, 0x2f, 0x0a,
1459+
0x00, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
1460+
0x00, 0x18, 0x00, 0x93, 0x98, 0x25, 0x57, 0x25,
1461+
0xa9, 0xd7, 0x01, 0x93, 0x98, 0x25, 0x57, 0x25,
1462+
0xa9, 0xd7, 0x01, 0x93, 0x98, 0x25, 0x57, 0x25,
1463+
0xa9, 0xd7, 0x01, 0x50, 0x4b, 0x01, 0x02, 0x3f,
1464+
0x00, 0x0a, 0x00, 0x00, 0x00, 0x00, 0x00, 0x78,
1465+
0x67, 0x2e, 0x53, 0x00, 0x00, 0x00, 0x00, 0x00,
1466+
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
1467+
0x00, 0x24, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
1468+
0x00, 0x20, 0x00, 0x00, 0x00, 0x3f, 0x00, 0x00,
1469+
0x00, 0x5c, 0x0a, 0x00, 0x20, 0x00, 0x00, 0x00,
1470+
0x00, 0x00, 0x01, 0x00, 0x18, 0x00, 0x93, 0x98,
1471+
0x25, 0x57, 0x25, 0xa9, 0xd7, 0x01, 0x93, 0x98,
1472+
0x25, 0x57, 0x25, 0xa9, 0xd7, 0x01, 0x93, 0x98,
1473+
0x25, 0x57, 0x25, 0xa9, 0xd7, 0x01, 0x50, 0x4b,
1474+
0x01, 0x02, 0x3f, 0x00, 0x0a, 0x00, 0x00, 0x00,
1475+
0x00, 0x00, 0x91, 0x68, 0x2e, 0x53, 0x85, 0x11,
1476+
0x4a, 0x0d, 0x0b, 0x00, 0x00, 0x00, 0x0b, 0x00,
1477+
0x00, 0x00, 0x09, 0x00, 0x24, 0x00, 0x00, 0x00,
1478+
0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, 0x00,
1479+
0x5e, 0x00, 0x00, 0x00, 0x2f, 0x74, 0x65, 0x73,
1480+
0x74, 0x2e, 0x74, 0x78, 0x74, 0x0a, 0x00, 0x20,
1481+
0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x18,
1482+
0x00, 0xa9, 0x80, 0x51, 0x01, 0x26, 0xa9, 0xd7,
1483+
0x01, 0x31, 0xd1, 0x57, 0x01, 0x26, 0xa9, 0xd7,
1484+
0x01, 0xdf, 0x48, 0x85, 0xf9, 0x25, 0xa9, 0xd7,
1485+
0x01, 0x50, 0x4b, 0x05, 0x06, 0x00, 0x00, 0x00,
1486+
0x00, 0x04, 0x00, 0x04, 0x00, 0x31, 0x01, 0x00,
1487+
0x00, 0x90, 0x00, 0x00, 0x00, 0x00, 0x00,
1488+
}
1489+
r, err := NewReader(bytes.NewReader([]byte(data)), int64(len(data)))
1490+
if err != nil {
1491+
t.Fatalf("Error reading the archive: %v", err)
1492+
}
1493+
entryNames := []string{`/`, `//`, `\`, `/test.txt`}
1494+
var names []string
1495+
for _, f := range r.File {
1496+
names = append(names, f.Name)
1497+
if _, err := f.Open(); err != nil {
1498+
t.Errorf("Error opening %q: %v", f.Name, err)
1499+
}
1500+
if _, err := r.Open(f.Name); err == nil {
1501+
t.Errorf("Opening %q with fs.FS API succeeded", f.Name)
1502+
}
1503+
}
1504+
if !reflect.DeepEqual(names, entryNames) {
1505+
t.Errorf("Unexpected file entries: %q", names)
1506+
}
1507+
if _, err := r.Open(""); err == nil {
1508+
t.Errorf("Opening %q with fs.FS API succeeded", "")
1509+
}
1510+
if _, err := r.Open("test.txt"); err != nil {
1511+
t.Errorf("Error opening %q with fs.FS API: %v", "test.txt", err)
1512+
}
1513+
dirEntries, err := fs.ReadDir(r, ".")
1514+
if err != nil {
1515+
t.Fatalf("Error reading the root directory: %v", err)
1516+
}
1517+
if len(dirEntries) != 1 || dirEntries[0].Name() != "test.txt" {
1518+
t.Errorf("Unexpected directory entries")
1519+
for _, dirEntry := range dirEntries {
1520+
_, err := r.Open(dirEntry.Name())
1521+
t.Logf("%q (Open error: %v)", dirEntry.Name(), err)
1522+
}
1523+
t.FailNow()
1524+
}
1525+
info, err := dirEntries[0].Info()
1526+
if err != nil {
1527+
t.Fatalf("Error reading info entry: %v", err)
1528+
}
1529+
if name := info.Name(); name != "test.txt" {
1530+
t.Errorf("Inconsistent name in info entry: %v", name)
1531+
}
1532+
}

0 commit comments

Comments
 (0)