encoding/xml: handle leading, trailing, or double colons in names

Before this change, <:name> would parse as <name>, which could cause
issues in applications that rely on the parse-encode cycle to
round-trip. Similarly, <x name:=""> would parse as expected but then
have the attribute dropped when serializing because its name was empty.
Finally, <a:b:c> would parse and get serialized incorrectly. All these
values are invalid XML, but to minimize the impact of this change, we
parse them whole into Name.Local.

This issue was reported by Juho Nurminen of Mattermost as it leads to
round-trip mismatches. See #43168. It's not being fixed in a security
release because round-trip stability is not a currently supported
security property of encoding/xml, and we don't believe these fixes
would be sufficient to reliably guarantee it in the future.

Fixes CVE-2020-29509
Fixes CVE-2020-29511
Updates #43168

Change-Id: I68321c4d867305046f664347192948a889af3c7f
Reviewed-on: https://go-review.googlesource.com/c/go/+/277892
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>

diff --git a/src/encoding/xml/xml.go b/src/encoding/xml/xml.go
index 384d6ad4b86b49dfa06a0d12de5a2d287a43eac1..c902f1295aafea07166699a14006e89ba54b473d 100644 (file)
--- a/src/encoding/xml/xml.go
+++ b/src/encoding/xml/xml.go
@@ -1156,8 +1156,9 @@ func (d *Decoder) nsname() (name Name, ok bool) {
        if !ok {
                return
        }
-       i := strings.Index(s, ":")
-       if i < 0 {
+       if strings.Count(s, ":") > 1 {
+               name.Local = s
+       } else if i := strings.Index(s, ":"); i < 1 || i > len(s)-2 {
                name.Local = s
        } else {
                name.Space = s[0:i]

diff --git a/src/encoding/xml/xml_test.go b/src/encoding/xml/xml_test.go
index 5a10f5309dce3f28afe260a22e20182a52e966a6..47d0c391670df15b25be19624a17108417320886 100644 (file)
--- a/src/encoding/xml/xml_test.go
+++ b/src/encoding/xml/xml_test.go
@@ -1003,3 +1003,59 @@ func TestTokenUnmarshaler(t *testing.T) {
        d := NewTokenDecoder(tokReader{})
        d.Decode(&Failure{})
 }
+
+func testRoundTrip(t *testing.T, input string) {
+       d := NewDecoder(strings.NewReader(input))
+       var tokens []Token
+       var buf bytes.Buffer
+       e := NewEncoder(&buf)
+       for {
+               tok, err := d.Token()
+               if err == io.EOF {
+                       break
+               }
+               if err != nil {
+                       t.Fatalf("invalid input: %v", err)
+               }
+               if err := e.EncodeToken(tok); err != nil {
+                       t.Fatalf("failed to re-encode input: %v", err)
+               }
+               tokens = append(tokens, CopyToken(tok))
+       }
+       if err := e.Flush(); err != nil {
+               t.Fatal(err)
+       }
+
+       d = NewDecoder(&buf)
+       for {
+               tok, err := d.Token()
+               if err == io.EOF {
+                       break
+               }
+               if err != nil {
+                       t.Fatalf("failed to decode output: %v", err)
+               }
+               if len(tokens) == 0 {
+                       t.Fatalf("unexpected token: %#v", tok)
+               }
+               a, b := tokens[0], tok
+               if !reflect.DeepEqual(a, b) {
+                       t.Fatalf("token mismatch: %#v vs %#v", a, b)
+               }
+               tokens = tokens[1:]
+       }
+       if len(tokens) > 0 {
+               t.Fatalf("lost tokens: %#v", tokens)
+       }
+}
+
+func TestRoundTrip(t *testing.T) {
+       tests := map[string]string{
+               "leading colon":  `<::Test ::foo="bar"><:::Hello></:::Hello><Hello></Hello></::Test>`,
+               "trailing colon": `<foo abc:="x"></foo>`,
+               "double colon":   `<x:y:foo></x:y:foo>`,
+       }
+       for name, input := range tests {
+               t.Run(name, func(t *testing.T) { testRoundTrip(t, input) })
+       }
+}