dynamic: panic in decodeVarint

[bradleyjkemp]

Hi, this is a bug I found while fuzzing the library so it's probably pretty unlikely anyone will see this in normal usage.

I've been using the unknown field support in dynamic.Message to write a utility to enrich a MessageDescriptor so that it pretends it knows about all the fields present in a message (i.e. so that unknown fields can be marshalled to JSON (and unmarshalled again)).

While fuzzing my implementation I found this index out of range panic in github.com/jhump/protoreflect/dynamic.(*codedBuffer).decodeVarint:

panic: runtime error: index out of range

goroutine 1 [running]:
github.com/jhump/protoreflect/dynamic.(*codedBuffer).decodeVarint(0xc0000fdd90, 0x2, 0x0, 0x0)
	$GOPATH/github.com/jhump/protoreflect/dynamic/codec.go:79 +0x6ad
github.com/jhump/protoreflect/dynamic.(*codedBuffer).decodeTagAndWireType(0xc0000fdd90, 0xbd7fbdebfdef5fef, 0x0, 0x0)
	$GOPATH/github.com/jhump/protoreflect/dynamic/codec.go:172 +0x47
github.com/jhump/protoreflect/dynamic.skipGroup(0xc0000fdd90, 0x0, 0x0, 0x0, 0x0)
	$GOPATH/github.com/jhump/protoreflect/dynamic/binary.go:667 +0x1ba
github.com/jhump/protoreflect/dynamic.(*Message).unmarshalUnknownField(0xc00014b7d0, 0x30000000c, 0xc0000fdd90, 0x0, 0x0)
	$GOPATH/github.com/jhump/protoreflect/dynamic/binary.go:639 +0x3fb
github.com/jhump/protoreflect/dynamic.(*Message).unmarshal(0xc00014b7d0, 0xc0000fdd90, 0x0, 0x0, 0x0)
	$GOPATH/github.com/jhump/protoreflect/dynamic/binary.go:422 +0x203
github.com/jhump/protoreflect/dynamic.(*Message).UnmarshalMerge(0xc00014b7d0, 0x4d70000, 0xc, 0x200000, 0x1, 0x4a50100)
	$GOPATH/github.com/jhump/protoreflect/dynamic/binary.go:403 +0xa7
github.com/jhump/protoreflect/dynamic.(*Message).Unmarshal(0xc00014b7d0, 0x4d70000, 0xc, 0x200000, 0xc00014b7d0, 0xc0000fde01)
	$GOPATH/github.com/jhump/protoreflect/dynamic/binary.go:392 +0x73
github.com/golang/protobuf/proto.Unmarshal(0x4d70000, 0xc, 0x200000, 0x173c440, 0xc00014b7d0, 0x5d2f3018, 0x397abca8)
	$GOPATH/github.com/golang/protobuf/proto/decode.go:340 +0x141
github.com/bradleyjkemp/grpc-tools/internal/proto_decoder.Fuzz(0x4d70000, 0xc, 0x200000, 0x3)
	$GOPATH/github.com/bradleyjkemp/grpc-tools/internal/proto_decoder/fuzz.go:28 +0x265
go-fuzz-dep.Main(0xc0000fdf80, 0x1, 0x1)
	/var/folders/6m/r713l1xj6_dcykbx21svs09h0000gp/T/go-fuzz-build861821362/goroot/src/go-fuzz-dep/main.go:36 +0x1b6
main.main()
	/var/folders/6m/r713l1xj6_dcykbx21svs09h0000gp/T/go-fuzz-build861821362/gopath/src/github.com/bradleyjkemp/grpc-tools/internal/proto_decoder/go.fuzz.main/main.go:15 +0x52
exit status 2

You can replicate this using this fuzzing harness with data = "c\x02���O":

package proto_decoder

import (
	"github.com/golang/protobuf/proto"
	"github.com/golang/protobuf/ptypes/empty"
	"github.com/jhump/protoreflect/desc"
	"github.com/jhump/protoreflect/desc/builder"
	"github.com/jhump/protoreflect/dynamic"
)

func Fuzz(data []byte) int {
	d, err := desc.LoadMessageDescriptorForMessage(&empty.Empty{})
	if err != nil {
		panic(err)
	}
	mb, err := builder.FromMessage(d)
	if err != nil {
		panic(err)
	}

	msg, err := mb.Build()
	if err != nil {
		panic(err)
	}

	decoded := dynamic.NewMessage(msg)
	err = proto.Unmarshal(data, decoded)
	if err != nil {
		return 0
	}

	return 1
}

[jhump]

@bradleyjkemp, thanks for posting this! But it doesn't look like GitHub liked the unicode data you added in the data value. It appears to be showing three characters using the "invalid code point" glyph.

Do you mind pasting the value as a literal []byte? That would be more helpful for me to debug with the right data.

You might need to post a similar bug against the main project (github.org/golang/protobuf) since the code is copied/forked from their *Buffer type in the proto package therein. So it almost certainly has the same problem.

Strangely, there is a bounds check right above. But it's not checking for negative values. So perhaps, somehow, this puts the buffer into a bad state where it thinks it's current index in the byte slice is a negative value.

[jhump]

Never mind about an alternate data value. Looks like even w/ those strange codepoints, I can repro (I guess maybe those are valid codepoints, but GitHub's web font doesn't support them?).

Anyhow, looking into it now.

[bradleyjkemp]

Yeah I was a bit skeptical of the unicode too but did seem to still work for me when copy-pasted.

Here's a different crashing input encoded as base64: AwPSMO+/ve+/ve+/vQ8=

[jhump]

It looks like this is not an issue in the main protobuf repo. I've added my own methods to this forked code for skipping over chunks of data (length-delimited fields), but wasn't checking for numeric overflow in that code!

Fix coming momentarily.

[jhump]

#235 has a fix that works with both inputs.

Instead of panicking, they both return an error:

• The former example returns "unexpected EOF" -- the length doesn't overflow a signed int, but it is (much) bigger than the rest of the buffer.
• The latter example returns "bad byte length -4791902657223630865" since it does overflow a signed int.

[bradleyjkemp]

Thanks! Those error messages seem fine for an issue you'll only encounter if something is going very wrong with your data 🙂