From efa95dde0467b64e0d2e5569cd79e7900e3ef9c0 Mon Sep 17 00:00:00 2001 From: Jordan Le Date: Wed, 10 Jun 2026 11:06:28 -0400 Subject: [PATCH 1/2] Cap recursion depth and input length in SCIM Path and FilterExpression parsers (FireWatch 6e560c31) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- .../Protocol/FilterExpression.cs | 43 ++++++++++++++++++- .../Protocol/Path.cs | 29 ++++++++++++- 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/Microsoft.SystemForCrossDomainIdentityManagement/Protocol/FilterExpression.cs b/Microsoft.SystemForCrossDomainIdentityManagement/Protocol/FilterExpression.cs index c01c685b..cf191245 100644 --- a/Microsoft.SystemForCrossDomainIdentityManagement/Protocol/FilterExpression.cs +++ b/Microsoft.SystemForCrossDomainIdentityManagement/Protocol/FilterExpression.cs @@ -35,6 +35,14 @@ namespace Microsoft.SCIM // and the second pair of bracketed clauses are in a third group. internal sealed class FilterExpression : IFilterExpression { + // Defense-in-depth caps for the recursive FilterExpression parser. Without these caps an + // attacker can submit a filter string with thousands of clauses chained by 'and'/'or' and + // trigger a StackOverflowException, which is non-catchable in .NET and terminates the + // process. Real SCIM filters are <2KB and have <20 clauses; these caps leave generous + // headroom while bounding worst-case recursion depth. + private const int MaxFilterLengthChars = 16_384; + private const int MaxRecursionDepth = 32; + private const char BracketClose = ')'; private const char Escape = '\\'; private const char Quote = '"'; @@ -99,6 +107,7 @@ internal sealed class FilterExpression : IFilterExpression private ComparisonOperator filterOperator; private int groupValue; private int levelValue; + private int depth; private LogicalOperatorValue logicalOperator; private FilterExpression next; private ComparisonValue value; @@ -126,17 +135,49 @@ private FilterExpression(FilterExpression other) } private FilterExpression(string text, int group, int level) + : this(text, group, level, depth: 0) + { + } + + private FilterExpression(string text, int group, int level, int depth) { if (string.IsNullOrWhiteSpace(text)) { throw new ArgumentNullException(nameof(text)); } + // Top-level only: reject overlong inputs before any parsing begins. + if (depth == 0 && text.Length > MaxFilterLengthChars) + { + throw new ArgumentException( + string.Format( + CultureInfo.InvariantCulture, + "Filter expression length {0} exceeds maximum allowed length {1}.", + text.Length, + MaxFilterLengthChars), + nameof(text)); + } + + // Per-recursion: reject if we've exceeded the recursion-depth budget. The parser + // recurses once per logical operator ('and'/'or'); this depth cap bounds worst-case + // recursion even if the input length cap is bypassed by future callers. + if (depth > MaxRecursionDepth) + { + throw new ArgumentException( + string.Format( + CultureInfo.InvariantCulture, + "Filter expression nesting exceeds maximum allowed depth {0}.", + MaxRecursionDepth), + nameof(text)); + } + this.Text = text.Trim(); this.Level = level; this.Group = group; + this.depth = depth; + MatchCollection matches = FilterExpression.Expression.Value.Matches(this.Text); foreach (Match match in matches) { @@ -627,7 +668,7 @@ private void Initialize(Group left, Group @operator, Group right) nextExpressionLevel = this.Level; nextExpressionGroup = this.Group; } - this.next = new FilterExpression(nextExpression, nextExpressionGroup, nextExpressionLevel); + this.next = new FilterExpression(nextExpression, nextExpressionGroup, nextExpressionLevel, this.depth + 1); this.next.Previous = this; } diff --git a/Microsoft.SystemForCrossDomainIdentityManagement/Protocol/Path.cs b/Microsoft.SystemForCrossDomainIdentityManagement/Protocol/Path.cs index b3278d9f..9a86787b 100644 --- a/Microsoft.SystemForCrossDomainIdentityManagement/Protocol/Path.cs +++ b/Microsoft.SystemForCrossDomainIdentityManagement/Protocol/Path.cs @@ -13,6 +13,14 @@ public sealed class Path : IPath { private const string ArgumentNamePathExpression = "pathExpression"; + // Defense-in-depth caps for the recursive Path.TryParse parser. Without these caps an + // attacker can submit a path with thousands of dot-separated segments and trigger a + // StackOverflowException, which is non-catchable in .NET and terminates the process. + // Real SCIM paths are <200 chars and have <5 dot-separators; these caps leave generous + // headroom while bounding worst-case recursion depth. + private const int MaxPathLengthChars = 16_384; + private const int MaxRecursionDepth = 32; + private const string ConstructNameSubAttributes = "subAttr"; private const string ConstructNameValuePath = "valuePath"; private const string PatternTemplate = @"(?<{0}>.*)\[(?<{1}>.*)\]"; @@ -139,6 +147,11 @@ private static bool TryExtractSchemaIdentifier(string pathExpression, out string } public static bool TryParse(string pathExpression, out IPath path) + { + return TryParse(pathExpression, depth: 0, out path); + } + + private static bool TryParse(string pathExpression, int depth, out IPath path) { path = null; @@ -147,6 +160,20 @@ public static bool TryParse(string pathExpression, out IPath path) throw new ArgumentNullException(Path.ArgumentNamePathExpression); } + // Top-level only: reject overlong inputs before any parsing begins. + if (depth == 0 && pathExpression.Length > MaxPathLengthChars) + { + return false; + } + + // Per-recursion: reject if we've exceeded the recursion-depth budget. The parser + // recurses once per '.' separator, and the worst-case depth is bounded by the input + // length cap; this depth cap is a redundant defense. + if (depth > MaxRecursionDepth) + { + return false; + } + Path buffer = new Path(pathExpression); string expression = pathExpression; @@ -164,7 +191,7 @@ public static bool TryParse(string pathExpression, out IPath path) expression = expression.Substring(0, seperatorIndex); - if (!Path.TryParse(valuePathExpression, out IPath valuePath)) + if (!Path.TryParse(valuePathExpression, depth + 1, out IPath valuePath)) { return false; } From a7f842d82e7e5e92cf7bfb7d24c01f48eb83c1db Mon Sep 17 00:00:00 2001 From: Jordan Le Date: Wed, 17 Jun 2026 16:41:44 -0400 Subject: [PATCH 2/2] Address review feedback: fix off-by-one, copy ctor, and comment - 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> --- .../Protocol/FilterExpression.cs | 3 ++- .../Protocol/Path.cs | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/Microsoft.SystemForCrossDomainIdentityManagement/Protocol/FilterExpression.cs b/Microsoft.SystemForCrossDomainIdentityManagement/Protocol/FilterExpression.cs index cf191245..efbcbf93 100644 --- a/Microsoft.SystemForCrossDomainIdentityManagement/Protocol/FilterExpression.cs +++ b/Microsoft.SystemForCrossDomainIdentityManagement/Protocol/FilterExpression.cs @@ -125,6 +125,7 @@ private FilterExpression(FilterExpression other) this.filterOperator = other.filterOperator; this.Group = other.Group; this.Level = other.Level; + this.depth = other.depth; this.logicalOperator = other.logicalOperator; this.value = other.value; if (other.next != null) @@ -161,7 +162,7 @@ private FilterExpression(string text, int group, int level, int depth) // Per-recursion: reject if we've exceeded the recursion-depth budget. The parser // recurses once per logical operator ('and'/'or'); this depth cap bounds worst-case // recursion even if the input length cap is bypassed by future callers. - if (depth > MaxRecursionDepth) + if (depth >= MaxRecursionDepth) { throw new ArgumentException( string.Format( diff --git a/Microsoft.SystemForCrossDomainIdentityManagement/Protocol/Path.cs b/Microsoft.SystemForCrossDomainIdentityManagement/Protocol/Path.cs index 9a86787b..cd16f717 100644 --- a/Microsoft.SystemForCrossDomainIdentityManagement/Protocol/Path.cs +++ b/Microsoft.SystemForCrossDomainIdentityManagement/Protocol/Path.cs @@ -167,9 +167,9 @@ private static bool TryParse(string pathExpression, int depth, out IPath path) } // Per-recursion: reject if we've exceeded the recursion-depth budget. The parser - // recurses once per '.' separator, and the worst-case depth is bounded by the input - // length cap; this depth cap is a redundant defense. - if (depth > MaxRecursionDepth) + // recurses once per bracket filter expression (valuePathExpression inside [...]); + // this depth cap bounds worst-case recursion even if the input length cap is bypassed. + if (depth >= MaxRecursionDepth) { return false; }