Skip to content

Commit

Permalink
encoding/xml: limit depth of nesting in unmarshal
Browse files Browse the repository at this point in the history
Prevent exhausting the stack limit when unmarshalling extremely deeply
nested structures into nested types.

Fixes golang#53611
Fixes CVE-2022-30633

Change-Id: Ic6c5d41674c93cfc9a316135a408db9156d39c59
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1421319
Reviewed-by: Damien Neil <[email protected]>
Reviewed-by: Julie Qiu <[email protected]>
Reviewed-on: https://go-review.googlesource.com/c/go/+/417061
Run-TryBot: Michael Knyszek <[email protected]>
Reviewed-by: Heschi Kreinick <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
  • Loading branch information
rolandshoemaker authored and mknyszek committed Jul 12, 2022
1 parent 913d051 commit c4c1993
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 8 deletions.
27 changes: 19 additions & 8 deletions src/encoding/xml/read.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ func (d *Decoder) DecodeElement(v any, start *StartElement) error {
if val.IsNil() {
return errors.New("nil pointer passed to Unmarshal")
}
return d.unmarshal(val.Elem(), start)
return d.unmarshal(val.Elem(), start, 0)
}

// An UnmarshalError represents an error in the unmarshaling process.
Expand Down Expand Up @@ -308,8 +308,15 @@ var (
textUnmarshalerType = reflect.TypeOf((*encoding.TextUnmarshaler)(nil)).Elem()
)

const maxUnmarshalDepth = 10000

var errExeceededMaxUnmarshalDepth = errors.New("exceeded max depth")

// Unmarshal a single XML element into val.
func (d *Decoder) unmarshal(val reflect.Value, start *StartElement) error {
func (d *Decoder) unmarshal(val reflect.Value, start *StartElement, depth int) error {
if depth >= maxUnmarshalDepth {
return errExeceededMaxUnmarshalDepth
}
// Find start element if we need it.
if start == nil {
for {
Expand Down Expand Up @@ -402,7 +409,7 @@ func (d *Decoder) unmarshal(val reflect.Value, start *StartElement) error {
v.Set(reflect.Append(val, reflect.Zero(v.Type().Elem())))

// Recur to read element into slice.
if err := d.unmarshal(v.Index(n), start); err != nil {
if err := d.unmarshal(v.Index(n), start, depth+1); err != nil {
v.SetLen(n)
return err
}
Expand Down Expand Up @@ -525,13 +532,15 @@ Loop:
case StartElement:
consumed := false
if sv.IsValid() {
consumed, err = d.unmarshalPath(tinfo, sv, nil, &t)
// unmarshalPath can call unmarshal, so we need to pass the depth through so that
// we can continue to enforce the maximum recusion limit.
consumed, err = d.unmarshalPath(tinfo, sv, nil, &t, depth)
if err != nil {
return err
}
if !consumed && saveAny.IsValid() {
consumed = true
if err := d.unmarshal(saveAny, &t); err != nil {
if err := d.unmarshal(saveAny, &t, depth+1); err != nil {
return err
}
}
Expand Down Expand Up @@ -676,7 +685,7 @@ func copyValue(dst reflect.Value, src []byte) (err error) {
// The consumed result tells whether XML elements have been consumed
// from the Decoder until start's matching end element, or if it's
// still untouched because start is uninteresting for sv's fields.
func (d *Decoder) unmarshalPath(tinfo *typeInfo, sv reflect.Value, parents []string, start *StartElement) (consumed bool, err error) {
func (d *Decoder) unmarshalPath(tinfo *typeInfo, sv reflect.Value, parents []string, start *StartElement, depth int) (consumed bool, err error) {
recurse := false
Loop:
for i := range tinfo.fields {
Expand All @@ -691,7 +700,7 @@ Loop:
}
if len(finfo.parents) == len(parents) && finfo.name == start.Name.Local {
// It's a perfect match, unmarshal the field.
return true, d.unmarshal(finfo.value(sv, initNilPointers), start)
return true, d.unmarshal(finfo.value(sv, initNilPointers), start, depth+1)
}
if len(finfo.parents) > len(parents) && finfo.parents[len(parents)] == start.Name.Local {
// It's a prefix for the field. Break and recurse
Expand Down Expand Up @@ -720,7 +729,9 @@ Loop:
}
switch t := tok.(type) {
case StartElement:
consumed2, err := d.unmarshalPath(tinfo, sv, parents, &t)
// the recursion depth of unmarshalPath is limited to the path length specified
// by the struct field tag, so we don't increment the depth here.
consumed2, err := d.unmarshalPath(tinfo, sv, parents, &t, depth)
if err != nil {
return true, err
}
Expand Down
15 changes: 15 additions & 0 deletions src/encoding/xml/read_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
package xml

import (
"bytes"
"errors"
"io"
"reflect"
"strings"
Expand Down Expand Up @@ -1094,3 +1096,16 @@ func TestUnmarshalIntoNil(t *testing.T) {
}

}

func TestCVE202228131(t *testing.T) {
type nested struct {
Parent *nested `xml:",any"`
}
var n nested
err := Unmarshal(bytes.Repeat([]byte("<a>"), maxUnmarshalDepth+1), &n)
if err == nil {
t.Fatal("Unmarshal did not fail")
} else if !errors.Is(err, errExeceededMaxUnmarshalDepth) {
t.Fatalf("Unmarshal unexpected error: got %q, want %q", err, errExeceededMaxUnmarshalDepth)
}
}

0 comments on commit c4c1993

Please sign in to comment.