From c3ddacbd2e67af0f7bbd463ebbe3db814bd5291b Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Thu, 9 Oct 2025 03:30:15 -0700 Subject: [PATCH] fix: apply recursion limits when parsing JSON well-known types with deep arrays PiperOrigin-RevId: 817105608 --- .../Google.Protobuf.Test/JsonParserTest.cs | 36 ++++++ csharp/src/Google.Protobuf/JsonParser.cs | 110 ++++++++++-------- csharp/src/Google.Protobuf/JsonTokenizer.cs | 7 ++ 3 files changed, 106 insertions(+), 47 deletions(-) diff --git a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs index 8dbf8360d5..7e41a02aca 100644 --- a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs @@ -15,6 +15,7 @@ using ProtobufTestMessages.Proto2; using ProtobufTestMessages.Proto3; using ProtobufUnittest; using System; +using System.Linq; using UnitTest.Issues.TestProtos; namespace Google.Protobuf @@ -906,6 +907,41 @@ namespace Google.Protobuf Assert.Throws(() => parser63.Parse(data64)); } + [Test] + public void MaliciousRecursionOfObjectsInValue() + { + int depth = 64; + string json = string.Join("", Enumerable.Repeat("{\"a\":", depth)) + + "{}" + + string.Join("", Enumerable.Repeat("}", depth)); + + // Each object level requires a Value and a Struct, so we can effectively only + // handle half as much depth as in a normal message. + var sufficientLimitParser = new JsonParser(new JsonParser.Settings(depth * 2 + 1)); + sufficientLimitParser.Parse(json); + + var insufficientLimitParser = new JsonParser(new JsonParser.Settings(depth * 2)); + Assert.Throws(() => insufficientLimitParser.Parse(json)); + } + + [Test] + public void MaliciousRecursionOfArraysInValue() + { + int depth = 64; + string json = new string('[', depth) + new string(']', depth); + + // Each array level requires a Value and a ListValue, so we can effectively only + // handle half as much depth as in a normal message. The limits here are slightly different than + // the limits for object recursion due to implementation details, but the inconsistency + // is preferred over the complexity of making arrays and objects match precisely in + // limit handling. + var sufficientLimitParser = new JsonParser(new JsonParser.Settings(depth * 2 - 1)); + sufficientLimitParser.Parse(json); + + var insufficientLimitParser = new JsonParser(new JsonParser.Settings(depth * 2 - 2)); + Assert.Throws(() => insufficientLimitParser.Parse(json)); + } + [Test] [TestCase("AQI")] [TestCase("_-==")] diff --git a/csharp/src/Google.Protobuf/JsonParser.cs b/csharp/src/Google.Protobuf/JsonParser.cs index 9bdfed6caa..eae0f927ff 100644 --- a/csharp/src/Google.Protobuf/JsonParser.cs +++ b/csharp/src/Google.Protobuf/JsonParser.cs @@ -124,71 +124,87 @@ namespace Google.Protobuf /// of tokens provided by the tokenizer. This token stream is assumed to be valid JSON, with the /// tokenizer performing that validation - but not every token stream is valid "protobuf JSON". /// + /// + /// This method maintains and checks the recursion depth, so *must* be called for any nested parsing. + /// private void Merge(IMessage message, JsonTokenizer tokenizer) { - if (tokenizer.ObjectDepth > settings.RecursionLimit) + if (tokenizer.RecursionDepth > settings.RecursionLimit) { throw InvalidProtocolBufferException.JsonRecursionLimitExceeded(); } - if (message.Descriptor.IsWellKnownType) + tokenizer.RecursionDepth++; + + // try/finally used in order to decrement the recursion depth regardless of outcome. + // If an exception is thrown, the recursion depth is irrelevant anyway - but as the method + // has multiple return statements, this is the simplest way of ensuring the recursion depth + // is always decremented. An alternative would be to use a local function. + try { - if (WellKnownTypeHandlers.TryGetValue(message.Descriptor.FullName, out Action handler)) + if (message.Descriptor.IsWellKnownType) { - handler(this, message, tokenizer); - return; - } - // Well-known types with no special handling continue in the normal way. - } - var token = tokenizer.Next(); - if (token.Type != JsonToken.TokenType.StartObject) - { - throw new InvalidProtocolBufferException("Expected an object"); - } - var descriptor = message.Descriptor; - var jsonFieldMap = descriptor.Fields.ByJsonName(); - // All the oneof fields we've already accounted for - we can only see each of them once. - // The set is created lazily to avoid the overhead of creating a set for every message - // we parsed, when oneofs are relatively rare. - HashSet seenOneofs = null; - while (true) - { - token = tokenizer.Next(); - if (token.Type == JsonToken.TokenType.EndObject) - { - return; - } - if (token.Type != JsonToken.TokenType.Name) - { - throw new InvalidOperationException("Unexpected token type " + token.Type); - } - string name = token.StringValue; - if (jsonFieldMap.TryGetValue(name, out FieldDescriptor field)) - { - if (field.ContainingOneof != null) + if (WellKnownTypeHandlers.TryGetValue(message.Descriptor.FullName, out Action handler)) { - if (seenOneofs == null) - { - seenOneofs = new HashSet(); - } - if (!seenOneofs.Add(field.ContainingOneof)) - { - throw new InvalidProtocolBufferException($"Multiple values specified for oneof {field.ContainingOneof.Name}"); - } + handler(this, message, tokenizer); + return; } - MergeField(message, field, tokenizer); + // Well-known types with no special handling continue in the normal way. } - else + var token = tokenizer.Next(); + if (token.Type != JsonToken.TokenType.StartObject) { - if (settings.IgnoreUnknownFields) + throw new InvalidProtocolBufferException("Expected an object"); + } + var descriptor = message.Descriptor; + var jsonFieldMap = descriptor.Fields.ByJsonName(); + // All the oneof fields we've already accounted for - we can only see each of them once. + // The set is created lazily to avoid the overhead of creating a set for every message + // we parsed, when oneofs are relatively rare. + HashSet seenOneofs = null; + while (true) + { + token = tokenizer.Next(); + if (token.Type == JsonToken.TokenType.EndObject) { - tokenizer.SkipValue(); + return; + } + if (token.Type != JsonToken.TokenType.Name) + { + throw new InvalidOperationException("Unexpected token type " + token.Type); + } + string name = token.StringValue; + if (jsonFieldMap.TryGetValue(name, out FieldDescriptor field)) + { + if (field.ContainingOneof != null) + { + if (seenOneofs == null) + { + seenOneofs = new HashSet(); + } + if (!seenOneofs.Add(field.ContainingOneof)) + { + throw new InvalidProtocolBufferException($"Multiple values specified for oneof {field.ContainingOneof.Name}"); + } + } + MergeField(message, field, tokenizer); } else { - throw new InvalidProtocolBufferException("Unknown field: " + name); + if (settings.IgnoreUnknownFields) + { + tokenizer.SkipValue(); + } + else + { + throw new InvalidProtocolBufferException("Unknown field: " + name); + } } } } + finally + { + tokenizer.RecursionDepth--; + } } private void MergeField(IMessage message, FieldDescriptor field, JsonTokenizer tokenizer) diff --git a/csharp/src/Google.Protobuf/JsonTokenizer.cs b/csharp/src/Google.Protobuf/JsonTokenizer.cs index d12d427d4f..5611beaf41 100644 --- a/csharp/src/Google.Protobuf/JsonTokenizer.cs +++ b/csharp/src/Google.Protobuf/JsonTokenizer.cs @@ -51,6 +51,13 @@ namespace Google.Protobuf return new JsonReplayTokenizer(tokens, continuation); } + /// + /// The depth of recursion within JsonParser. This is not directly computable within the tokenizer itself, + /// as arrays contribute to the recursion depth for ListValue parsing, but not for "normal" message types. + /// This is maintained (and checked) by . + /// + internal int RecursionDepth { get; set; } + /// /// Returns the depth of the stack, purely in objects (not collections). /// Informally, this is the number of remaining unclosed '{' characters we have.