Skip to content

StringSubstitutor recursive expansion has a cycle check but no fan-out, depth, or size bound - #772

Open
kornys wants to merge 1 commit into
apache:masterfrom
kornys:security/stringsubstitutor-expansion-limits
Open

kornys wants to merge 1 commit into
apache:masterfrom
kornys:security/stringsubstitutor-expansion-limits

Conversation

@kornys

@kornys kornys commented Sep 18, 2026

Copy link
Copy Markdown

This ports the recursion bound applied to the deprecated commons-lang3 StrSubstitutor (apache/commons-lang c8dc3121) over to StringSubstitutor, as suggested in the security-report thread ("Then please provide a PR on GitHub").

Problem

StringSubstitutor's checkCyclicSubstitution() only rejects a variable already on the current substitution stack. It does not bound:

  • acyclic fan-out — each of N references expanding to N more (a "billion-laughs" shape). No variable ever repeats on the stack, so the cycle check never fires.
  • deep nesting — ${v0} → ${v1} → … recurses once per level toward StackOverflowError (also reachable via nested variable names when setEnableSubstitutionInVariables(true) is set).

A crafted variable map (where the substitution values are attacker-controlled) can therefore drive interpolation into exponential output growth (CPU/memory exhaustion) or a stack overflow.

Fix

Two budgets per top-level substitution, matching the lang3 fix:

  • MAX_SUBSTITUTION_DEPTH = 256 — nesting-depth cap
  • MAX_SUBSTITUTION_LENGTH = 16 MiB — total emitted-characters cap

Both raise IllegalStateException when exceeded. substitute(...) is split into a small budget-enforcing wrapper plus the existing recursive body (substituteRecursive), so the counters cover value recursion and nested-name resolution alike and reset per top-level call — a budget-exceeded failure does not poison the next replace().

No public API change; behavior is unchanged for any input under the budgets.

Tests

Two regression tests added to StringSubstitutorTest (deep nesting → depth budget; exponential fan-out → size budget). The full StringSubstitutorTest suite passes (81 tests).

Before you push a pull request, review this list:

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?

Yes. I used Claude (Anthropic). It is a port of the maintainer's own commons-lang3 fix (c8dc3121) to StringSubstitutor; Claude wrote the two unit tests, and drafted this description under my direction. I reviewed the result, confirmed the tests fail without the runtime change, and I take responsibility for the contribution.

  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

…t, depth, or size bound

checkCyclicSubstitution() only rejects a variable already on the current
substitution stack. It does not bound acyclic fan-out (each of N references
expanding to N more) or deep nesting, so a crafted variable map can drive
interpolation into exponential output growth or a StackOverflowError without
any variable repeating on the stack.

Bound substitute() with a maximum interpolation depth (256) and a maximum total
output size (16 MiB) per top-level substitution, both raising
IllegalStateException. Mirrors the fix for the deprecated commons-lang3
StrSubstitutor (c8dc3121).
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.

1 participant