Skip to content

Commit

Permalink
encoding/gob: cover missed cases when checking ignore depth
Browse files Browse the repository at this point in the history
This change makes sure that we are properly checking the ignored field
recursion depth in decIgnoreOpFor consistently. This prevents stack
exhaustion when attempting to decode a message that contains an
extremely deeply nested struct which is ignored.

Thanks to Md Sakib Anwar of The Ohio State University ([email protected])
for reporting this issue.

Updates #69139
Fixes #69144
Fixes CVE-2024-34156

Change-Id: Iacce06be95a5892b3064f1c40fcba2e2567862d6
Reviewed-on: https://go-internal-review.googlesource.com/c/go/+/1440
Reviewed-by: Russ Cox <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
(cherry picked from commit f0a11f9b3aaa362cb1d05e095e3c8d421d4f087f)
Reviewed-on: https://go-internal-review.googlesource.com/c/go/+/1580
Reviewed-by: Tatiana Bradley <[email protected]>
Reviewed-on: https://go-review.googlesource.com/c/go/+/611182
TryBot-Bypass: Dmitri Shuralyov <[email protected]>
Reviewed-by: Michael Pratt <[email protected]>
Auto-Submit: Dmitri Shuralyov <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>

Backported by the golang-fips authors.
  • Loading branch information
rolandshoemaker authored and dbenoit17 committed Sep 13, 2024
1 parent f6b7854 commit 7787391
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 8 deletions.
19 changes: 11 additions & 8 deletions src/encoding/gob/decode.go
Original file line number Diff line number Diff line change
Expand Up @@ -874,8 +874,11 @@ func (dec *Decoder) decOpFor(wireId typeId, rt reflect.Type, name string, inProg
var maxIgnoreNestingDepth = 10000

// decIgnoreOpFor returns the decoding op for a field that has no destination.
func (dec *Decoder) decIgnoreOpFor(wireId typeId, inProgress map[typeId]*decOp, depth int) *decOp {
if depth > maxIgnoreNestingDepth {
func (dec *Decoder) decIgnoreOpFor(wireId typeId, inProgress map[typeId]*decOp) *decOp {
// Track how deep we've recursed trying to skip nested ignored fields.
dec.ignoreDepth++
defer func() { dec.ignoreDepth-- }()
if dec.ignoreDepth > maxIgnoreNestingDepth {
error_(errors.New("invalid nesting depth"))
}
// If this type is already in progress, it's a recursive type (e.g. map[string]*T).
Expand All @@ -901,23 +904,23 @@ func (dec *Decoder) decIgnoreOpFor(wireId typeId, inProgress map[typeId]*decOp,
errorf("bad data: undefined type %s", wireId.string())
case wire.ArrayT != nil:
elemId := wire.ArrayT.Elem
elemOp := dec.decIgnoreOpFor(elemId, inProgress, depth+1)
elemOp := dec.decIgnoreOpFor(elemId, inProgress)
op = func(i *decInstr, state *decoderState, value reflect.Value) {
state.dec.ignoreArray(state, *elemOp, wire.ArrayT.Len)
}

case wire.MapT != nil:
keyId := dec.wireType[wireId].MapT.Key
elemId := dec.wireType[wireId].MapT.Elem
keyOp := dec.decIgnoreOpFor(keyId, inProgress, depth+1)
elemOp := dec.decIgnoreOpFor(elemId, inProgress, depth+1)
keyOp := dec.decIgnoreOpFor(keyId, inProgress)
elemOp := dec.decIgnoreOpFor(elemId, inProgress)
op = func(i *decInstr, state *decoderState, value reflect.Value) {
state.dec.ignoreMap(state, *keyOp, *elemOp)
}

case wire.SliceT != nil:
elemId := wire.SliceT.Elem
elemOp := dec.decIgnoreOpFor(elemId, inProgress, depth+1)
elemOp := dec.decIgnoreOpFor(elemId, inProgress)
op = func(i *decInstr, state *decoderState, value reflect.Value) {
state.dec.ignoreSlice(state, *elemOp)
}
Expand Down Expand Up @@ -1078,7 +1081,7 @@ func (dec *Decoder) compileSingle(remoteId typeId, ut *userTypeInfo) (engine *de
func (dec *Decoder) compileIgnoreSingle(remoteId typeId) *decEngine {
engine := new(decEngine)
engine.instr = make([]decInstr, 1) // one item
op := dec.decIgnoreOpFor(remoteId, make(map[typeId]*decOp), 0)
op := dec.decIgnoreOpFor(remoteId, make(map[typeId]*decOp))
ovfl := overflow(dec.typeString(remoteId))
engine.instr[0] = decInstr{*op, 0, nil, ovfl}
engine.numInstr = 1
Expand Down Expand Up @@ -1123,7 +1126,7 @@ func (dec *Decoder) compileDec(remoteId typeId, ut *userTypeInfo) (engine *decEn
localField, present := srt.FieldByName(wireField.Name)
// TODO(r): anonymous names
if !present || !isExported(wireField.Name) {
op := dec.decIgnoreOpFor(wireField.Id, make(map[typeId]*decOp), 0)
op := dec.decIgnoreOpFor(wireField.Id, make(map[typeId]*decOp))
engine.instr[fieldnum] = decInstr{*op, fieldnum, nil, ovfl}
continue
}
Expand Down
2 changes: 2 additions & 0 deletions src/encoding/gob/decoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ type Decoder struct {
freeList *decoderState // list of free decoderStates; avoids reallocation
countBuf []byte // used for decoding integers while parsing messages
err error
// ignoreDepth tracks the depth of recursively parsed ignored fields
ignoreDepth int
}

// NewDecoder returns a new decoder that reads from the io.Reader.
Expand Down
14 changes: 14 additions & 0 deletions src/encoding/gob/gobencdec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -806,6 +806,8 @@ func TestIngoreDepthLimit(t *testing.T) {
defer func() { maxIgnoreNestingDepth = oldNestingDepth }()
b := new(bytes.Buffer)
enc := NewEncoder(b)

// Nested slice
typ := reflect.TypeOf(int(0))
nested := reflect.ArrayOf(1, typ)
for i := 0; i < 100; i++ {
Expand All @@ -819,4 +821,16 @@ func TestIngoreDepthLimit(t *testing.T) {
if err := dec.Decode(&output); err == nil || err.Error() != expectedErr {
t.Errorf("Decode didn't fail with depth limit of 100: want %q, got %q", expectedErr, err)
}

// Nested struct
nested = reflect.StructOf([]reflect.StructField{{Name: "F", Type: typ}})
for i := 0; i < 100; i++ {
nested = reflect.StructOf([]reflect.StructField{{Name: "F", Type: nested}})
}
badStruct = reflect.New(reflect.StructOf([]reflect.StructField{{Name: "F", Type: nested}}))
enc.Encode(badStruct.Interface())
dec = NewDecoder(b)
if err := dec.Decode(&output); err == nil || err.Error() != expectedErr {
t.Errorf("Decode didn't fail with depth limit of 100: want %q, got %q", expectedErr, err)
}
}

0 comments on commit 7787391

Please sign in to comment.