Skip to content

Commit

Permalink
encoding/protojson, internal/encoding/json: handle missing object values
Browse files Browse the repository at this point in the history
In internal/encoding/json, report an error when encountering a }
when we are expecting an object field value. For example, the input
`{"":}` now correctly results in an error at the closing } token.

In encoding/protojson, check for an unexpected EOF token in
skipJSONValue. This is redundant with the check in internal/encoding/json,
but adds a bit more defense against any other similar bugs that
might exist.

Fixes CVE-2024-24786

Change-Id: I03d52512acb5091c8549e31ca74541d57e56c99d
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/569356
TryBot-Bypass: Damien Neil <dneil@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Commit-Queue: Damien Neil <dneil@google.com>
  • Loading branch information
neild committed Mar 5, 2024
1 parent 235ef28 commit f01a588
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 1 deletion.
17 changes: 17 additions & 0 deletions encoding/protojson/decode_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2770,6 +2770,23 @@ func TestUnmarshal(t *testing.T) {
inputText: `{"foo":{"bar":[{"baz":[{}]]}}`,
umo: protojson.UnmarshalOptions{RecursionLimit: 5, DiscardUnknown: true},
wantErr: "exceeded max recursion depth",
}, {
desc: "Object missing value: no DiscardUnknown",
inputMessage: &testpb.TestAllTypes{},
inputText: `{"":}`,
umo: protojson.UnmarshalOptions{RecursionLimit: 5, DiscardUnknown: false},
wantErr: `(line 1:2): unknown field ""`,
}, {
desc: "Object missing value: DiscardUnknown",
inputMessage: &testpb.TestAllTypes{},
inputText: `{"":}`,
umo: protojson.UnmarshalOptions{RecursionLimit: 5, DiscardUnknown: true},
wantErr: `(line 1:5): unexpected token`,
}, {
desc: "Object missing value: Any",
inputMessage: &anypb.Any{},
inputText: `{"":}`,
wantErr: `(line 1:5): unexpected token`,
}}

for _, tt := range tests {
Expand Down
4 changes: 4 additions & 0 deletions encoding/protojson/well_known_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,10 @@ func (d decoder) skipJSONValue() error {
if open > d.opts.RecursionLimit {
return errors.New("exceeded max recursion depth")
}
case json.EOF:
// This can only happen if there's a bug in Decoder.Read.
// Avoid an infinite loop if this does happen.
return errors.New("unexpected EOF")
}
if open == 0 {
return nil
Expand Down
2 changes: 1 addition & 1 deletion internal/encoding/json/decode.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ func (d *Decoder) Read() (Token, error) {

case ObjectClose:
if len(d.openStack) == 0 ||
d.lastToken.kind == comma ||
d.lastToken.kind&(Name|comma) != 0 ||
d.openStack[len(d.openStack)-1] != ObjectOpen {
return Token{}, d.newSyntaxError(tok.pos, unexpectedFmt, tok.RawString())
}
Expand Down
15 changes: 15 additions & 0 deletions internal/encoding/json/decode_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1193,6 +1193,21 @@ func TestDecoder(t *testing.T) {
{E: errEOF},
},
},
{
in: `{""`,
want: []R{
{V: ObjectOpen},
{E: errEOF},
},
},
{
in: `{"":`,
want: []R{
{V: ObjectOpen},
{V: Name{""}},
{E: errEOF},
},
},
{
in: `{"34":"89",}`,
want: []R{
Expand Down

0 comments on commit f01a588

Please sign in to comment.