Cap recursion depth and input length in SCIM Path and FilterExpression parsers (FireWatch 6e560c31)#138
Merged
himanshusainig merged 2 commits intoJun 17, 2026
Conversation
…n parsers (FireWatch 6e560c31)
Both Path.TryParse and the FilterExpression(string,int,int) chain are hand-rolled
recursive-descent parsers that process attacker-supplied strings. With no depth,
length, or clause-count guard, an input with ~8000+ dot-separators (Path) or
~8000+ logical clauses (FilterExpression) triggers a StackOverflowException,
which is non-catchable in .NET and terminates the process — a DoS that crashes
any SCIM endpoint validating /Users?filter= or PATCH path parameters.
FireWatch finding 6e560c31, IMPORTANT, PARTIALLY_EXPLOITABLE.
Adds two defensive caps to each parser:
* MaxPathLengthChars / MaxFilterLengthChars (16384) - input cap, top-level only
* MaxRecursionDepth (32) - per-recursion cap, defense in depth
Real SCIM paths are <200 chars with <5 segments; real filters are <2KB with <20
clauses; the caps leave 80x+ headroom for legitimate use while bounding the
worst-case recursion depth to a trivial fraction of the .NET stack.
Implementation:
* Path.TryParse now has a public (depth=0) and private (depth+1 recursive)
overload. Returns false at top-level on overlong input or when depth exceeds
the recursion cap (consistent with existing TryParse return-value semantics).
* FilterExpression gains a (text, group, level, depth) private constructor;
the existing (text, group, level) ctor delegates with depth=0. Length/depth
cap violations throw ArgumentException, which Filter.TryParse already
catches and returns false.
No tests in this repo (SCIMReferenceCode has no test project per its
sample-only charter); comprehensive tests for the same fix are provided in the
downstream port to internal SyncFabric-SCIM-Tools.
FireWatch finding: https://firewatch-pilot-fd-atb3ceg5egfxc9gg.b02.azurefd.net/Services/fbfae0f8-2ef1-47f4-94e5-029d635c2081/scimreferencecode/6e560c31-8f32-4d36-aa0c-92a5e1d621f4
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
@microsoft-github-policy-service agree [company="Microsoft"] |
Contributor
Author
|
@microsoft-github-policy-service agree company="Microsoft" |
- Fix off-by-one: depth > MaxRecursionDepth -> depth >= MaxRecursionDepth (both Path.cs and FilterExpression.cs) - Copy depth field in FilterExpression copy constructor to prevent bypass - Fix misleading comment in Path.cs: recursion is for bracket filter expressions, not dot-separated segments Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
FireWatch finding:
6e560c31· IMPORTANT · CWE-674: Uncontrolled RecursionSummary
Both
Path.TryParseandFilterExpression(string,int,int)are recursive-descent parsers that process attacker-supplied SCIM query strings. With no depth or length guard, ~8000+ dot-separated segments (Path) or logical clauses (FilterExpression) trigger a non-catchableStackOverflowExceptionthat terminates the process — a DoS that crashes any SCIM endpoint.Fix
Adds two defensive caps to each parser:
MaxPathLengthChars/MaxFilterLengthCharsMaxRecursionDepthReal SCIM paths are <200 chars with <5 segments; real filters are <2KB with <20 clauses. Caps leave 80x+ headroom.
Files Changed
Protocol/Path.cs— public TryParse delegates to private depth-tracking overloadProtocol/FilterExpression.cs— existing ctor delegates to new depth-tracking ctorComprehensive tests for this fix live in the downstream SyncFabric-SCIM-Tools port (PR 16038663).