Skip to content

csharp: propagate recursion depth into JsonReplayTokenizer to honor Any nesting limit#26835

Closed
MindflareX wants to merge 1 commit into
protocolbuffers:mainfrom
MindflareX:fix/csharp-any-recursion-depth
Closed

csharp: propagate recursion depth into JsonReplayTokenizer to honor Any nesting limit#26835
MindflareX wants to merge 1 commit into
protocolbuffers:mainfrom
MindflareX:fix/csharp-any-recursion-depth

Conversation

@MindflareX

Copy link
Copy Markdown
Contributor

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

{"@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:

  • JavamergeAnyMessage 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.

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.

@MindflareX
MindflareX requested a review from a team as a code owner April 11, 2026 04:49
@MindflareX
MindflareX requested review from jskeet and removed request for a team April 11, 2026 04:49
@google-cla

google-cla Bot commented Apr 11, 2026

Copy link
Copy Markdown

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.

@MindflareX

Copy link
Copy Markdown
Contributor Author

@googlebot I signed it!

@MindflareX

Copy link
Copy Markdown
Contributor Author

@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.
@MindflareX
MindflareX force-pushed the fix/csharp-any-recursion-depth branch from 7b41978 to 8d5f579 Compare April 11, 2026 05:00
@MindflareX

Copy link
Copy Markdown
Contributor Author

@googlebot I signed it!

@MindflareX

Copy link
Copy Markdown
Contributor Author

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 esrauchg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this proposed fix!

@esrauchg esrauchg added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Apr 21, 2026
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Apr 21, 2026
copybara-service Bot pushed a commit that referenced this pull request Apr 23, 2026
…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
@MindflareX

Copy link
Copy Markdown
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants