UPSTREAM PR #26916: Fix text format parser recursion limit bypass#145
Open
UPSTREAM PR #26916: Fix text format parser recursion limit bypass#145
Conversation
Two security issues in the text format parser's recursion handling: 1. The Parser constructor defaults recursion_limit_ to INT_MAX instead of 100, effectively disabling stack depth protection. The binary format parser (coded_stream.h) correctly defaults to 100. A deeply nested text-format message can cause stack overflow / crash. 2. ConsumeAnyValue calls ConsumeMessage without decrementing recursion_limit_, so the ConsumeField -> ConsumeAnyValue -> ConsumeMessage -> ConsumeField call chain bypasses the recursion limit entirely. Add the same decrement/increment guard used by ConsumeFieldMessage and SkipFieldMessage.
- Revert default recursion_limit_ back to INT_MAX since TextProto is not a wire format and many users have trusted inputs deeper than 100. - Refactor ConsumeAnyValue recursion guard to use a single balanced decrement/increment: --recursion_limit_ at entry, ++recursion_limit_ only on the success path. Error paths use DO() or bare return false, which fail the entire parse, so restoring the limit is unnecessary. This matches the pattern in ConsumeFieldMessage and SkipFieldMessage.
|
The analysis encountered an error. Please review the Processing Details for more information. |
loci-dev
force-pushed
the
main
branch
24 times, most recently
from
63b262e to
463aed9
Compare
loci-dev
force-pushed
the
main
branch
25 times, most recently
from
d1d44f5 to
f292971
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Source pull request: protocolbuffers/protobuf#26916
Summary
Two security vulnerabilities in the text format parser's recursion depth handling allow unbounded stack growth, leading to stack overflow / crash when parsing untrusted input:
Default recursion limit is INT_MAX. The
Parserconstructor initializesrecursion_limit_tostd::numeric_limits<int>::max(), effectively disabling depth protection. The binary format parser (CodedInputStream) correctly defaults to 100. This patch changes the text format default to 100 to match.ConsumeAnyValuebypasses the recursion limit. The call chainConsumeField->ConsumeAnyValue->ConsumeMessage->ConsumeFieldnever decrementsrecursion_limit_, so even a finite limit can be bypassed via nestedAnymessages. This patch adds the same decrement/increment guard already used byConsumeFieldMessageandSkipFieldMessage.Test plan
Anyvalues in text format are also rejected.