From b56bd45a3c9dd670f35615801975f71578948902 Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Tue, 7 Jun 2022 13:00:43 -0700 Subject: [PATCH 1/3] [release-branch.go1.17] encoding/gob: add a depth limit for ignored fields Enforce a nesting limit of 10,000 for ignored fields during decoding of messages. This prevents the possibility of triggering stack exhaustion. Fixes #53709 Updates #53615 Fixes CVE-2022-30635 Change-Id: I05103d06dd5ca3945fcba3c1f5d3b5a645e8fb0f Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1484771 Reviewed-by: Damien Neil Reviewed-by: Tatiana Bradley (cherry picked from commit 55e8f938d22bfec29cc9dc9671044c5a41d1ea9c) Reviewed-on: https://go-review.googlesource.com/c/go/+/417074 Run-TryBot: Heschi Kreinick TryBot-Result: Gopher Robot Reviewed-by: Heschi Kreinick --- src/encoding/gob/decode.go | 19 ++++++++++++------- src/encoding/gob/gobencdec_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/src/encoding/gob/decode.go b/src/encoding/gob/decode.go index d2f6c749b1b..0e0ec75cccc 100644 --- a/src/encoding/gob/decode.go +++ b/src/encoding/gob/decode.go @@ -871,8 +871,13 @@ func (dec *Decoder) decOpFor(wireId typeId, rt reflect.Type, name string, inProg return &op } +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) *decOp { +func (dec *Decoder) decIgnoreOpFor(wireId typeId, inProgress map[typeId]*decOp, depth int) *decOp { + if depth > maxIgnoreNestingDepth { + error_(errors.New("invalid nesting depth")) + } // If this type is already in progress, it's a recursive type (e.g. map[string]*T). // Return the pointer to the op we're already building. if opPtr := inProgress[wireId]; opPtr != nil { @@ -896,7 +901,7 @@ 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) + elemOp := dec.decIgnoreOpFor(elemId, inProgress, depth+1) op = func(i *decInstr, state *decoderState, value reflect.Value) { state.dec.ignoreArray(state, *elemOp, wire.ArrayT.Len) } @@ -904,15 +909,15 @@ func (dec *Decoder) decIgnoreOpFor(wireId typeId, inProgress map[typeId]*decOp) case wire.MapT != nil: keyId := dec.wireType[wireId].MapT.Key elemId := dec.wireType[wireId].MapT.Elem - keyOp := dec.decIgnoreOpFor(keyId, inProgress) - elemOp := dec.decIgnoreOpFor(elemId, inProgress) + keyOp := dec.decIgnoreOpFor(keyId, inProgress, depth+1) + elemOp := dec.decIgnoreOpFor(elemId, inProgress, depth+1) 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) + elemOp := dec.decIgnoreOpFor(elemId, inProgress, depth+1) op = func(i *decInstr, state *decoderState, value reflect.Value) { state.dec.ignoreSlice(state, *elemOp) } @@ -1073,7 +1078,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)) + op := dec.decIgnoreOpFor(remoteId, make(map[typeId]*decOp), 0) ovfl := overflow(dec.typeString(remoteId)) engine.instr[0] = decInstr{*op, 0, nil, ovfl} engine.numInstr = 1 @@ -1118,7 +1123,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)) + op := dec.decIgnoreOpFor(wireField.Id, make(map[typeId]*decOp), 0) engine.instr[fieldnum] = decInstr{*op, fieldnum, nil, ovfl} continue } diff --git a/src/encoding/gob/gobencdec_test.go b/src/encoding/gob/gobencdec_test.go index 6d2c8db42d0..1b52ecc6c84 100644 --- a/src/encoding/gob/gobencdec_test.go +++ b/src/encoding/gob/gobencdec_test.go @@ -12,6 +12,7 @@ import ( "fmt" "io" "net" + "reflect" "strings" "testing" "time" @@ -796,3 +797,26 @@ func TestNetIP(t *testing.T) { t.Errorf("decoded to %v, want 1.2.3.4", ip.String()) } } + +func TestIngoreDepthLimit(t *testing.T) { + // We don't test the actual depth limit because it requires building an + // extremely large message, which takes quite a while. + oldNestingDepth := maxIgnoreNestingDepth + maxIgnoreNestingDepth = 100 + defer func() { maxIgnoreNestingDepth = oldNestingDepth }() + b := new(bytes.Buffer) + enc := NewEncoder(b) + typ := reflect.TypeOf(int(0)) + nested := reflect.ArrayOf(1, typ) + for i := 0; i < 100; i++ { + nested = reflect.ArrayOf(1, nested) + } + badStruct := reflect.New(reflect.StructOf([]reflect.StructField{{Name: "F", Type: nested}})) + enc.Encode(badStruct.Interface()) + dec := NewDecoder(b) + var output struct{ Hello int } + expectedErr := "invalid nesting depth" + 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) + } +} From d5bf5bff6e29f686da382cfa07c407337be00d9f Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Fri, 3 May 2024 09:21:39 -0400 Subject: [PATCH 2/3] encoding/gob: cover missed cases when checking ignore depth 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 (anwar.40@osu.edu) 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 Reviewed-by: Damien Neil (cherry picked from commit f0a11f9b3aaa362cb1d05e095e3c8d421d4f087f) Reviewed-on: https://go-internal-review.googlesource.com/c/go/+/1580 Reviewed-by: Tatiana Bradley Reviewed-on: https://go-review.googlesource.com/c/go/+/611182 TryBot-Bypass: Dmitri Shuralyov Reviewed-by: Michael Pratt Auto-Submit: Dmitri Shuralyov Reviewed-by: Dmitri Shuralyov Backported by the golang-fips authors. --- src/encoding/gob/decode.go | 19 +++++++++++-------- src/encoding/gob/decoder.go | 2 ++ src/encoding/gob/gobencdec_test.go | 14 ++++++++++++++ 3 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/encoding/gob/decode.go b/src/encoding/gob/decode.go index 0e0ec75cccc..92d64cb07f4 100644 --- a/src/encoding/gob/decode.go +++ b/src/encoding/gob/decode.go @@ -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). @@ -901,7 +904,7 @@ 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) } @@ -909,15 +912,15 @@ func (dec *Decoder) decIgnoreOpFor(wireId typeId, inProgress map[typeId]*decOp, 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) } @@ -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 @@ -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 } diff --git a/src/encoding/gob/decoder.go b/src/encoding/gob/decoder.go index b52aabe54b5..0d37b9dce6a 100644 --- a/src/encoding/gob/decoder.go +++ b/src/encoding/gob/decoder.go @@ -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. diff --git a/src/encoding/gob/gobencdec_test.go b/src/encoding/gob/gobencdec_test.go index 1b52ecc6c84..2b5f2a8e889 100644 --- a/src/encoding/gob/gobencdec_test.go +++ b/src/encoding/gob/gobencdec_test.go @@ -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++ { @@ -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) + } } From bebe92f1e75fd8087c2115dc86a23ed61c61b594 Mon Sep 17 00:00:00 2001 From: David Benoit Date: Fri, 13 Sep 2024 11:43:05 -0400 Subject: [PATCH 3/3] Skip overlong message test OpenSSL now returns a random string instead of an error to avoid timing-based attacks. --- src/crypto/rsa/pkcs1v15_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/crypto/rsa/pkcs1v15_test.go b/src/crypto/rsa/pkcs1v15_test.go index 3dd1ec99acf..abf7101c71f 100644 --- a/src/crypto/rsa/pkcs1v15_test.go +++ b/src/crypto/rsa/pkcs1v15_test.go @@ -238,6 +238,10 @@ func TestHashVerifyPKCS1v15(t *testing.T) { } func TestOverlongMessagePKCS1v15(t *testing.T) { + // OpenSSL now returns a random string instead of an error + if boring.Enabled() { + t.Skip("Not relevant in boring mode") + } ciphertext := decodeBase64("fjOVdirUzFoLlukv80dBllMLjXythIf22feqPrNo0YoIjzyzyoMFiLjAc/Y4krkeZ11XFThIrEvw\nkRiZcCq5ng==") _, err := DecryptPKCS1v15(nil, rsaPrivateKey, ciphertext) if err == nil {