From d86e53e896eca907ad67300c0bb495e3dd925358 Mon Sep 17 00:00:00 2001 From: Katie Hockman Date: Mon, 1 Mar 2021 09:54:00 -0500 Subject: [release-branch.go1.16-security] encoding/xml: prevent infinite loop while decoding This change properly handles a TokenReader which returns an EOF in the middle of an open XML element. Thanks to Sam Whited for reporting this. Fixes CVE-2021-27918 Change-Id: Id02a3f3def4a1b415fa2d9a8e3b373eb6cb0f433 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1004594 Reviewed-by: Russ Cox Reviewed-by: Roland Shoemaker Reviewed-by: Filippo Valsorda (cherry picked from commit e7ce1f6746223ec7b4caa3b1ece25d9be3864710) Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1014235 --- src/encoding/xml/xml.go | 19 ++++---- src/encoding/xml/xml_test.go | 104 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 92 insertions(+), 31 deletions(-) diff --git a/src/encoding/xml/xml.go b/src/encoding/xml/xml.go index adaf4daf19..6f9594d7ba 100644 --- a/src/encoding/xml/xml.go +++ b/src/encoding/xml/xml.go @@ -271,7 +271,7 @@ func NewTokenDecoder(t TokenReader) *Decoder { // it will return an error. // // Token implements XML name spaces as described by -// https://www.w3.org/TR/REC-xml-names/. Each of the +// https://www.w3.org/TR/REC-xml-names/. Each of the // Name structures contained in the Token has the Space // set to the URL identifying its name space when known. // If Token encounters an unrecognized name space prefix, @@ -285,16 +285,17 @@ func (d *Decoder) Token() (Token, error) { if d.nextToken != nil { t = d.nextToken d.nextToken = nil - } else if t, err = d.rawToken(); err != nil { - switch { - case err == io.EOF && d.t != nil: - err = nil - case err == io.EOF && d.stk != nil && d.stk.kind != stkEOF: - err = d.syntaxError("unexpected EOF") + } else { + if t, err = d.rawToken(); t == nil && err != nil { + if err == io.EOF && d.stk != nil && d.stk.kind != stkEOF { + err = d.syntaxError("unexpected EOF") + } + return nil, err } - return t, err + // We still have a token to process, so clear any + // errors (e.g. EOF) and proceed. + err = nil } - if !d.Strict { if t1, ok := d.autoClose(t); ok { d.nextToken = t diff --git a/src/encoding/xml/xml_test.go b/src/encoding/xml/xml_test.go index efddca43e9..5672ebb375 100644 --- a/src/encoding/xml/xml_test.go +++ b/src/encoding/xml/xml_test.go @@ -33,30 +33,90 @@ func (t *toks) Token() (Token, error) { func TestDecodeEOF(t *testing.T) { start := StartElement{Name: Name{Local: "test"}} - t.Run("EarlyEOF", func(t *testing.T) { - d := NewTokenDecoder(&toks{earlyEOF: true, t: []Token{ - start, - start.End(), - }}) - err := d.Decode(&struct { - XMLName Name `xml:"test"` - }{}) - if err != nil { - t.Error(err) + tests := []struct { + name string + tokens []Token + ok bool + }{ + { + name: "OK", + tokens: []Token{ + start, + start.End(), + }, + ok: true, + }, + { + name: "Malformed", + tokens: []Token{ + start, + StartElement{Name: Name{Local: "bad"}}, + start.End(), + }, + ok: false, + }, + } + for _, tc := range tests { + for _, eof := range []bool{true, false} { + name := fmt.Sprintf("%s/earlyEOF=%v", tc.name, eof) + t.Run(name, func(t *testing.T) { + d := NewTokenDecoder(&toks{ + earlyEOF: eof, + t: tc.tokens, + }) + err := d.Decode(&struct { + XMLName Name `xml:"test"` + }{}) + if tc.ok && err != nil { + t.Fatalf("d.Decode: expected nil error, got %v", err) + } + if _, ok := err.(*SyntaxError); !tc.ok && !ok { + t.Errorf("d.Decode: expected syntax error, got %v", err) + } + }) } - }) - t.Run("LateEOF", func(t *testing.T) { - d := NewTokenDecoder(&toks{t: []Token{ - start, - start.End(), - }}) - err := d.Decode(&struct { - XMLName Name `xml:"test"` - }{}) - if err != nil { - t.Error(err) + } +} + +type toksNil struct { + returnEOF bool + t []Token +} + +func (t *toksNil) Token() (Token, error) { + if len(t.t) == 0 { + if !t.returnEOF { + // Return nil, nil before returning an EOF. It's legal, but + // discouraged. + t.returnEOF = true + return nil, nil } - }) + return nil, io.EOF + } + var tok Token + tok, t.t = t.t[0], t.t[1:] + return tok, nil +} + +func TestDecodeNilToken(t *testing.T) { + for _, strict := range []bool{true, false} { + name := fmt.Sprintf("Strict=%v", strict) + t.Run(name, func(t *testing.T) { + start := StartElement{Name: Name{Local: "test"}} + bad := StartElement{Name: Name{Local: "bad"}} + d := NewTokenDecoder(&toksNil{ + // Malformed + t: []Token{start, bad, start.End()}, + }) + d.Strict = strict + err := d.Decode(&struct { + XMLName Name `xml:"test"` + }{}) + if _, ok := err.(*SyntaxError); !ok { + t.Errorf("d.Decode: expected syntax error, got %v", err) + } + }) + } } const testInput = ` -- cgit v1.2.3-54-g00ecf From 634d28d78ccbeb6e86f8bfeba030ea8be518f8fa Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Tue, 2 Mar 2021 10:00:53 -0800 Subject: [release-branch.go1.16-security] archive/zip: fix panic in Reader.Open 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 Change-Id: Ic755d8126cb0897e2cbbdacf572439c38dde7b35 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1004761 Reviewed-by: Filippo Valsorda Reviewed-by: Russ Cox Reviewed-by: Katie Hockman (cherry picked from commit ce22003b26eaf8e4a690757f699aae7062d41472) Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1013753 Reviewed-by: Roland Shoemaker --- src/archive/zip/reader.go | 2 +- src/archive/zip/reader_test.go | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/archive/zip/reader.go b/src/archive/zip/reader.go index 8b4e77875f..c288ad965b 100644 --- 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 index 34e96f7da4..5faf1f49b5 100644 --- 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) + } +} -- cgit v1.2.3-54-g00ecf From e9e0473681e581040b4adcd64b53967e1572fe8d Mon Sep 17 00:00:00 2001 From: Alexander Rakoczy Date: Tue, 9 Mar 2021 16:22:44 -0500 Subject: [release-branch.go1.16-security] go1.16.1 Change-Id: I4999d72caf9462554a2a6f1d761244cafec34718 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1014541 Reviewed-by: Katie Hockman --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 166e7aec71..81026a006a 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -go1.16 \ No newline at end of file +go1.16.1 \ No newline at end of file -- cgit v1.2.3-54-g00ecf