fix(go/grpc): avoid panic on short FlatBuffers input #8684
Conversation
The gRPC codec read the root UOffsetT without checking input size. On buffers shorter than SizeUOffsetT, GetUint32 touched data[3] and the process panics. Add a simple length check and validate the root offset stays within the buffer. Return clear errors (insufficient data / invalid root offset) instead of panicking. Signed-off-by: Ville Vesilehto <[email protected]>
| // The root UOffsetT must be within the data buffer | ||
| if int(off) > len(data)-SizeUOffsetT { | ||
| return ErrInvalidRootOffset |
There was a problem hiding this comment.
@aardappel looks good from my end, but I wonder if we should cast the offset into an int here. would it cause problems down the line?
There was a problem hiding this comment.
Thanks @mustiikhalil - IMO avoiding the signedness pitfalls is good practice. I changed the code to do the comparison in unsigned domain altogether.
Keep the bounds check in the unsigned domain (UOffsetT) to avoid signedness pitfalls. Signed-off-by: Ville Vesilehto <[email protected]>
go/grpc.go
Outdated
| // Unmarshal parses the wire format into v. | ||
| func (FlatbuffersCodec) Unmarshal(data []byte, v interface{}) error { | ||
| v.(flatbuffersInit).Init(data, GetUOffsetT(data)) | ||
| // Need at least 4 bytes to read the root UOffsetT |
There was a problem hiding this comment.
technically the minimum size is 20 i think, see: https://github.com/google/flatbuffers/blob/master/include/flatbuffers/base.h#L345-L350
There was a problem hiding this comment.
12 bytes, i cannot math tonight
There was a problem hiding this comment.
Thanks, I'll clarify the comment. The 4 bytes are only for uoffset_t as we're not validating a complete FlatBuffer structure, just preventing a panic while reading the header
A full FlatBuffer structure would be: - uoffset_t: root table offset (4 bytes) - soffset_t: vtable offset in root table (4 bytes) - uint16_t: vtable size (2 bytes) - uint16_t: table size (2 bytes) In total 12 bytes. We are only validating the data length before trying to read the uoffset_t, not the full structure. Signed-off-by: Ville Vesilehto <[email protected]>
Motivation
The FlatBuffers Go gRPC codec
go/grpc.gounconditionally reads the root offset (UOffsetT) from incoming message bytes. For inputs shorter than 4 bytes,GetUint32()indexesdata[3], causing a panic. This is applicable to servers usingFlatbuffersCodecwith gRPC. It is basically an unauthenticated DoS vector.See panic from the Go greeter grpc/examples/go/greeter below. Tested with the latest versions:
github.com/google/[email protected]+incompatiblegoogle.golang.org/[email protected]Panic output
panic: runtime error: index out of range [3] with length 0goroutine 5 [running]:
github.com/google/flatbuffers/go.GetUint32(...)
/git/flatbuffers/grpc/examples/go/greeter/server/vendor/github.com/google/flatbuffers/go/encode.go:47
github.com/google/flatbuffers/go.GetUOffsetT(...)
/git/flatbuffers/grpc/examples/go/greeter/server/vendor/github.com/google/flatbuffers/go/encode.go:121
github.com/google/flatbuffers/go.FlatbuffersCodec.Unmarshal({}, {0x0?, 0x0?, 0x0?}, {0x1051fc500?, 0x1400009a000?})
/git/flatbuffers/grpc/examples/go/greeter/server/vendor/github.com/google/flatbuffers/go/grpc.go:18 +0xe0
google.golang.org/grpc.codecV0Bridge.Unmarshal({{0x12c3d3e00?, 0x105631ba0?}}, {0x0?, 0x0?, 0x14000187748?}, {0x1051fc500, 0x1400009a000})
/git/flatbuffers/grpc/examples/go/greeter/server/vendor/google.golang.org/grpc/codec.go:78 +0x78
google.golang.org/grpc.(*Server).processUnaryRPC.func3({0x1051fc500, 0x1400009a000})
/git/flatbuffers/grpc/examples/go/greeter/server/vendor/google.golang.org/grpc/server.go:1404 +0xb4
github.com/google/flatbuffers/grpc/examples/go/greeter/models._Greeter_SayHello_Handler({0x1051fb900, 0x105631ba0}, {0x105287200, 0x14000090150}, 0x14000094100, 0x0)
/git/flatbuffers/grpc/examples/go/greeter/server/vendor/github.com/google/flatbuffers/grpc/examples/go/greeter/models/Greeter_grpc.go:105 +0x60
google.golang.org/grpc.(*Server).processUnaryRPC(0x1400021a200, {0x105287200, 0x140000900c0}, 0x14000102240, 0x140002282d0, 0x1055f1bf0, 0x0)
/git/flatbuffers/grpc/examples/go/greeter/server/vendor/google.golang.org/grpc/server.go:1431 +0xbd0
google.golang.org/grpc.(*Server).handleStream(0x1400021a200, {0x1052874b8, 0x140001aad00}, 0x14000102240)
/git/flatbuffers/grpc/examples/go/greeter/server/vendor/google.golang.org/grpc/server.go:1842 +0x858
google.golang.org/grpc.(*Server).serveStreams.func2.1()
/git/flatbuffers/grpc/examples/go/greeter/server/vendor/google.golang.org/grpc/server.go:1061 +0x74
created by google.golang.org/grpc.(*Server).serveStreams.func2 in goroutine 24
/git/flatbuffers/grpc/examples/go/greeter/server/vendor/google.golang.org/grpc/server.go:1072 +0x120
Changes
Add a simple length check and validate the root offset stays within the buffer. Return clear errors (insufficient data / invalid root offset) instead of panicking. Errors are exported from the module.
Alternatives considered
I considered adding the checks to
GetUint32(). I think performing explicit bounds checks is more targeted in the codec, and probably better w.r.t performance. Open to feedback.Other notes
Originally reported to Google OSS VRP. Happy to provide a private PoC & help with CVE if maintainers prefer.