csharp: propagate recursion depth into JsonReplayTokenizer to honor Any nesting limit#26835
csharp: propagate recursion depth into JsonReplayTokenizer to honor Any nesting limit#26835MindflareX wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@googlebot I signed it! |
|
@google-cla I signed it! |
JsonParser tracks recursion depth on the JsonTokenizer instance, but
MergeAny() constructs a fresh JsonReplayTokenizer for every nested
google.protobuf.Any body. The replay tokenizer started at depth zero,
so the JsonParser.Settings.RecursionLimit check in Merge() never fired
across nested Any payloads. A document such as
{"@type":".../Any","value":{"@type":".../Any","value":{...}}}
could be nested arbitrarily deep, recursing through MergeAny ->
MergeWellKnownTypeAnyBody -> Merge -> MergeAny until the .NET stack
overflowed. StackOverflowException cannot be caught in .NET, so any
service parsing untrusted protobuf JSON with Any in its TypeRegistry
would crash.
This is the C# variant of the same bug previously fixed in:
- Java (mergeAnyMessage / currentDepth)
- Python (_ConvertAnyMessage)
The minimal fix is to copy the parent tokenizer's RecursionDepth into
the new JsonReplayTokenizer so that the existing limit check applies
across the replay boundary. The pre-existing
"// FIXME: Object depth not maintained..." comment is removed.
A regression test (MaliciousRecursionOfAnyInAny) has been added that
mirrors the existing MaliciousRecursionOfObjectsInValue /
MaliciousRecursionOfArraysInValue tests.
7b41978 to
8d5f579
Compare
|
@googlebot I signed it! |
|
Gentle ping — this PR fixes a stack-overflow DoS in the C# JsonParser.MergeAny path where JsonReplayTokenizer didn't inherit the parent's RecursionDepth, so RecursionLimit never fired for nested Any. Java and Python had the equivalent fix years ago; the C# case was overlooked. @esrauchg — since you're already looking at #26851 (sibling C# JsonParser fix), would you mind taking this one too? They're independent root causes but land in the same area. |
esrauchg
left a comment
There was a problem hiding this comment.
Thank you for this proposed fix!
…ny nesting limit (#26835) ## Summary `JsonParser.MergeAny()` records the JSON token stream of an Any body and replays it through a fresh `JsonReplayTokenizer` to parse the inner message. The replay tokenizer's `RecursionDepth` was never initialized from the parent tokenizer, so it defaulted to zero — and the recursion-depth check in `JsonParser.Merge()` reads the **tokenizer's** depth field. As a result, every nested `google.protobuf.Any` started at depth zero and the `Settings.RecursionLimit` check never fired. A pathological JSON document such as ```json {"@type":"type.googleapis.com/google.protobuf.Any","value": {"@type":"type.googleapis.com/google.protobuf.Any","value": {"@type":"type.googleapis.com/google.protobuf.Any","value": ... }}} ``` recurses through `MergeAny -> MergeWellKnownTypeAnyBody -> Merge -> MergeAny -> ...` until the .NET stack is exhausted, raising an uncatchable `StackOverflowException` and terminating the host process. The pre-existing `// FIXME: Object depth not maintained...` comment in `JsonReplayTokenizer` already acknowledged the gap. ## Prior art The same bug class has previously been fixed in the sibling language implementations of protobuf: - **Java** — `mergeAnyMessage` was missing the `currentDepth` increment. - **Python** — `_ConvertAnyMessage` reached via `methodcaller(...)` bypassed the depth tracking. The C# `MergeAny` path was overlooked at the time. A C# *array* recursion fix landed previously, but the Any case was not covered. ## Fix Inherit the parent tokenizer's `RecursionDepth` when constructing a `JsonReplayTokenizer`, so that `JsonParser.Merge()`'s existing limit check applies across the replay boundary. ```csharp internal JsonReplayTokenizer(IList<JsonToken> tokens, JsonTokenizer nextTokenizer) { this.tokens = tokens; this.nextTokenizer = nextTokenizer; this.RecursionDepth = nextTokenizer.RecursionDepth; } ``` ## Tests Added `JsonParserTest.MaliciousRecursionOfAnyInAny`, mirroring the existing `MaliciousRecursionOfObjectsInValue` / `MaliciousRecursionOfArraysInValue` regression tests: - Builds a 100-deep Any-of-Any document. - A parser with a generous limit parses it without exception. - A parser with an insufficient limit throws `InvalidProtocolBufferException` (rather than overflowing the stack). Without the fix, the second assertion either spins past the configured limit (because each replay tokenizer resets depth to zero) or terminates the test runner with `StackOverflowException`. Closes #26835 COPYBARA_INTEGRATE_REVIEW=#26835 from MindflareX:fix/csharp-any-recursion-depth 8d5f579 FUTURE_COPYBARA_INTEGRATE_REVIEW=#26835 from MindflareX:fix/csharp-any-recursion-depth 8d5f579 PiperOrigin-RevId: 903860227
|
Tracked internally by Google OSS VRP as https://issuetracker.google.com/issues/493487045. |
Summary
JsonParser.MergeAny()records the JSON token stream of an Any body and replays it through a freshJsonReplayTokenizerto parse the inner message. The replay tokenizer'sRecursionDepthwas never initialized from the parent tokenizer, so it defaulted to zero — and the recursion-depth check inJsonParser.Merge()reads the tokenizer's depth field. As a result, every nestedgoogle.protobuf.Anystarted at depth zero and theSettings.RecursionLimitcheck never fired.A pathological JSON document such as
{"@type":"type.googleapis.com/google.protobuf.Any","value": {"@type":"type.googleapis.com/google.protobuf.Any","value": {"@type":"type.googleapis.com/google.protobuf.Any","value": ... }}}recurses through
MergeAny -> MergeWellKnownTypeAnyBody -> Merge -> MergeAny -> ...until the .NET stack is exhausted, raising an uncatchableStackOverflowExceptionand terminating the host process.The pre-existing
// FIXME: Object depth not maintained...comment inJsonReplayTokenizeralready acknowledged the gap.Prior art
The same bug class has previously been fixed in the sibling language implementations of protobuf:
mergeAnyMessagewas missing thecurrentDepthincrement._ConvertAnyMessagereached viamethodcaller(...)bypassed the depth tracking.The C#
MergeAnypath was overlooked at the time. A C# array recursion fix landed previously, but the Any case was not covered.Fix
Inherit the parent tokenizer's
RecursionDepthwhen constructing aJsonReplayTokenizer, so thatJsonParser.Merge()'s existing limit check applies across the replay boundary.Tests
Added
JsonParserTest.MaliciousRecursionOfAnyInAny, mirroring the existingMaliciousRecursionOfObjectsInValue/MaliciousRecursionOfArraysInValueregression tests:InvalidProtocolBufferException(rather than overflowing the stack).Without the fix, the second assertion either spins past the configured limit (because each replay tokenizer resets depth to zero) or terminates the test runner with
StackOverflowException.