archive/zip: fix panic in Reader.Open

rolandshoemaker · FiloSottile · commit cd3b4ca9f20f · 2021-03-10T18:18:28.000Z
When operating on a Zip file that contains a file prefixed with "../", Open(...) would cause a panic in toValidName when attempting to strip the prefixed path components. Fixes CVE-2021-27919 Fixes golang#44916 Change-Id: Ic755d8126cb0897e2cbbdacf572439c38dde7b35 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1004761 Reviewed-by: Filippo Valsorda <valsorda@google.com> Reviewed-by: Russ Cox <rsc@google.com> Reviewed-by: Katie Hockman <katiehockman@google.com> Reviewed-on: https://go-review.googlesource.com/c/go/+/300489 Trust: Katie Hockman <katie@golang.org> Run-TryBot: Katie Hockman <katie@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Alexander Rakoczy <alex@golang.org> Reviewed-by: Filippo Valsorda <filippo@golang.org>
diff --git a/src/archive/zip/reader.go b/src/archive/zip/reader.go
@@ -664,7 +664,7 @@ func toValidName(name string) string {
 	if strings.HasPrefix(p, "/") {
 		p = p[len("/"):]
 	}
-	for strings.HasPrefix(name, "../") {
+	for strings.HasPrefix(p, "../") {
 		p = p[len("../"):]
 	}
 	return p
diff --git a/src/archive/zip/reader_test.go b/src/archive/zip/reader_test.go
@@ -1081,3 +1081,38 @@ func TestFS(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+func TestCVE202127919(t *testing.T) {
+	// Archive containing only the file "../test.txt"
+	data := []byte{
+		0x50, 0x4b, 0x03, 0x04, 0x14, 0x00, 0x08, 0x00,
+		0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x0b, 0x00, 0x00, 0x00, 0x2e, 0x2e,
+		0x2f, 0x74, 0x65, 0x73, 0x74, 0x2e, 0x74, 0x78,
+		0x74, 0x0a, 0xc9, 0xc8, 0x2c, 0x56, 0xc8, 0x2c,
+		0x56, 0x48, 0x54, 0x28, 0x49, 0x2d, 0x2e, 0x51,
+		0x28, 0x49, 0xad, 0x28, 0x51, 0x48, 0xcb, 0xcc,
+		0x49, 0xd5, 0xe3, 0x02, 0x04, 0x00, 0x00, 0xff,
+		0xff, 0x50, 0x4b, 0x07, 0x08, 0xc0, 0xd7, 0xed,
+		0xc3, 0x20, 0x00, 0x00, 0x00, 0x1a, 0x00, 0x00,
+		0x00, 0x50, 0x4b, 0x01, 0x02, 0x14, 0x00, 0x14,
+		0x00, 0x08, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0xc0, 0xd7, 0xed, 0xc3, 0x20, 0x00, 0x00,
+		0x00, 0x1a, 0x00, 0x00, 0x00, 0x0b, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x2e,
+		0x2e, 0x2f, 0x74, 0x65, 0x73, 0x74, 0x2e, 0x74,
+		0x78, 0x74, 0x50, 0x4b, 0x05, 0x06, 0x00, 0x00,
+		0x00, 0x00, 0x01, 0x00, 0x01, 0x00, 0x39, 0x00,
+		0x00, 0x00, 0x59, 0x00, 0x00, 0x00, 0x00, 0x00,
+	}
+	r, err := NewReader(bytes.NewReader([]byte(data)), int64(len(data)))
+	if err != nil {
+		t.Fatalf("Error reading the archive: %v", err)
+	}
+	_, err = r.Open("test.txt")
+	if err != nil {
+		t.Errorf("Error reading file: %v", err)
+	}
+}

Original file line number	Diff line number	Diff line change
`@@ -664,7 +664,7 @@ func toValidName(name string) string {`
`664`	`664`	`if strings.HasPrefix(p, "/") {`
`665`	`665`	`p = p[len("/"):]`
`666`	`666`	`}`
`667`		`- for strings.HasPrefix(name, "../") {`
	`667`	`+ for strings.HasPrefix(p, "../") {`
`668`	`668`	`p = p[len("../"):]`
`669`	`669`	`}`
`670`	`670`	`return p`