jsonpb panics when using numbers for parsing timestamps

[djavorszky]

Steps you follow to reproduce the error:

1. Create a service that receives a google.protobuf.Timestamp in its request, e.g.

message TimeStampMessage {
    int32 id = 1;
    google.protobuf.Timestamp ts = 2;
}

2. Add grpc-gateway in such a way that allows embedding the timestamp in the GET parameters, e.g.

service YourService {
    rpc Echo(TimeStampMessage) returns (StringMessage) {
        option (google.api.http) = {
            get: "/v1/{id}/{ts}";
        };
    }
}

3. Generate the code, e.g.:

protoc -I/usr/local/include -I. \
  -I$GOPATH/src \
  -I$GOPATH/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis \
  --go_out=plugins=grpc:. \
  --grpc-gateway_out=logtostderr=true:. \
  bugtest.proto

4. Add the gateway, e.g.

ctx := context.Background()
ctx, cancel := context.WithCancel(ctx)
defer cancel()

mux := runtime.NewServeMux()
opts := []grpc.DialOption{grpc.WithInsecure()}
err := gw.RegisterYourServiceHandlerFromEndpoint(ctx, mux, *echoEndpoint, opts)
if err != nil {
    return err
}

return http.ListenAndServe(":8080", mux)

5. Start the server
6. Curl the endpoint, e.g:

curl localhost:8080/v1/1/123

Result: panic on server side, empty http response

2019/08/30 12:51:14 http: panic serving [::1]:58087: reflect: call of reflect.Value.Type on zero Value
goroutine 57 [running]:
net/http.(*conn).serve.func1(0xc00025a000)
	/usr/local/Cellar/go/1.12.9/libexec/src/net/http/server.go:1769 +0x139
panic(0x1510bc0, 0xc0000d4580)
	/usr/local/Cellar/go/1.12.9/libexec/src/runtime/panic.go:522 +0x1b5
reflect.Value.Type(0x0, 0x0, 0x0, 0x0, 0x0)
	/usr/local/Cellar/go/1.12.9/libexec/src/reflect/value.go:1813 +0x169
github.com/golang/protobuf/jsonpb.(*Unmarshaler).unmarshalValue(0xc0000d4520, 0x0, 0x0, 0x0, 0xc0000d83d0, 0x3, 0x8, 0x0, 0x100dfd8, 0x160)
	/Users/javdaniel/go/pkg/mod/github.com/golang/protobuf@v1.3.2/jsonpb/jsonpb.go:720 +0x5d
github.com/golang/protobuf/jsonpb.(*Unmarshaler).UnmarshalNext(0xc0000d4520, 0xc00011a9a0, 0x1675ba0, 0x0, 0xc0000de6c8, 0x153f0a0)
	/Users/javdaniel/go/pkg/mod/github.com/golang/protobuf@v1.3.2/jsonpb/jsonpb.go:682 +0x18f
github.com/golang/protobuf/jsonpb.(*Unmarshaler).Unmarshal(...)
	/Users/javdaniel/go/pkg/mod/github.com/golang/protobuf@v1.3.2/jsonpb/jsonpb.go:693
github.com/golang/protobuf/jsonpb.UnmarshalString(0xc0000dc14a, 0x3, 0x1675ba0, 0x0, 0xc0000de758, 0x1)
	/Users/javdaniel/go/pkg/mod/github.com/golang/protobuf@v1.3.2/jsonpb/jsonpb.go:714 +0xde
github.com/grpc-ecosystem/grpc-gateway/runtime.Timestamp(...)
	/Users/javdaniel/go/pkg/mod/github.com/grpc-ecosystem/grpc-gateway@v1.10.0/runtime/convert.go:210
grpc-go-bugtest/grpc/service.request_YourService_Echo_0(0x1678320, 0xc0000d6f00, 0x16799e0, 0x1a39d20, 0x166c160, 0xc0000ba028, 0xc00025e000, 0xc0000d6e70, 0x1, 0x1, ...)
	/Users/javdaniel/git/grpc-go-bugtest/grpc/service/bugtest.pb.gw.go:58 +0x23c
grpc-go-bugtest/grpc/service.RegisterYourServiceHandlerClient.func1(0x16773a0, 0xc000260000, 0xc00025e000, 0xc0000d6e70)
	/Users/javdaniel/git/grpc-go-bugtest/grpc/service/bugtest.pb.gw.go:182 +0x1c4
github.com/grpc-ecosystem/grpc-gateway/runtime.(*ServeMux).ServeHTTP(0xc0001a40e0, 0x16773a0, 0xc000260000, 0xc00025e000)
	/Users/javdaniel/go/pkg/mod/github.com/grpc-ecosystem/grpc-gateway@v1.10.0/runtime/mux.go:243 +0xbea
net/http.serverHandler.ServeHTTP(0xc0000e6680, 0x16773a0, 0xc000260000, 0xc00025e000)
	/usr/local/Cellar/go/1.12.9/libexec/src/net/http/server.go:2774 +0xa8
net/http.(*conn).serve(0xc00025a000, 0x1678260, 0xc0000c87c0)
	/usr/local/Cellar/go/1.12.9/libexec/src/net/http/server.go:1878 +0x851
created by net/http.(*Server).Serve
	/usr/local/Cellar/go/1.12.9/libexec/src/net/http/server.go:2884 +0x2f4

Example repository:

https://github.com/djavorszky/grpc-timestamp-bugtest

Steps here:

1. Start cmd (go run main.go)
2. Execute curl localhost:8080/v1/1/123

Notes: The implementation is not tied in, but as I can see the issue occurs before reaching the implementation, so that shouldn't be any issue. This has been tested and reproduced with a much more complicated service where the implementation works as intended

What did you expect to happen instead:

{
	"error": "invalid argument",
	"message": "invalid argument",
	"code": 3
}

What's your theory on why it isn't working:

Looking at the stacktrace and the underlying code, looks like reflection is used to try to parse the GET parameter as a Timestamp without making any assertions to see if it can be casted.

Either that, or not handling the failure gracefully.

Tested with gprc-gateway versions 1.8.3 and 1.10.0

[johanbrandhorst]

Hi, thanks for the brilliant bug report 😍. This is a good find, but I think the bug here is in github.com/golang/protobuf/jsonpb, the marshaler you are using (and this project uses by default). I've managed to reproduce this with the golang playground:

https://play.golang.org/p/2VS9bEb7sgD

The issue only seems to happen when using a pointer to the type before unmarshalling, as can be seen in this working example:

https://play.golang.org/p/kIAksR51kaz

I've raised an issue with https://github.com/golang/protobuf, we'll see if they think we're using it incorrectly, or if they are going to fix it. I would argue the unmarshaller shouldn't crash here, so I suspect it is a real bug.

Unfortunately, until that is resolved, I don't think there's much we can do here. I would consider this to be a DOS vulnerability in any servers using jsonpb in their public API, so hopefully the team working on the protobufs will ship a fix soon. I will leave this issue open until then, but I don't think we can do anything now.

A possible workaround is to use a string here and manually parse it in your gRPC server.

[johanbrandhorst]

Another option is to fork jsonpb and fix the bug. Would maybe be a nice first contribution?

[djavorszky]

Thanks @johanbrandhorst for looking into it and forwarding it to the relevant repository :-)

[johanbrandhorst]

Looks like this has been kicked back to us, and I'm pretty sure what the fix should be. We can change the runtime.Timestamp method to pass a concrete value into Unmarshal instead. I'd say at the same time we should look through the code base and see if we do this anywhere else.

Would you be interested in bringing this fix in?

[johanbrandhorst]

You snooze you lose 😁. I put together a fix in #1028.