Drop CSS declarations that nest functions deeper than sixteen

AGENTS.md

@@ -37,11 +37,12 @@ Hard rules. Never violate them, never refactor around them.
 5. **Encoding and rendering are part of the security boundary.** Changes to `Encoding`, `HtmlStreamRenderer`, `TagBalancingHtmlStreamEventReceiver`, `HtmlTextEscapingMode`, entity handling, or serialization are security changes and require adversarial tests.
 6. **The prepackaged policies in `Sanitizers` are published safety promises.** Never widen what they allow.
 7. **Round-trip safety:** sanitized output, re-parsed by a browser, must not mutate into a different, unsafe DOM (mXSS).
+8. **Any input terminates, in bounded stack, memory and time.** `sanitize()` must never throw, and never let a `StackOverflowError` or `OutOfMemoryError` out, for input of any size or shape. Recursion whose depth follows the input is forbidden: bound it with a small constant and drop what exceeds the bound, as `CssGrammar.MAX_FUNCTION_DEPTH` does, or walk with an explicit stack. Work that grows faster than linearly with the input, and output that grows without bound, are bugs of the same class; the tag balancer's quadratic paths and its formatting reconstruction were both found the hard way. A crash, a hang, or a blow-up on hostile input is a denial of service and is handled as a vulnerability.
 
 ## Change Rules
 
 - Small, single-purpose changes only. No drive-by refactors of parser, policy, or renderer code.
-- Any change touching parsing, policy enforcement, rendering, encoding, URL handling, or CSS handling requires new regression tests with hostile payloads, not just benign inputs.
+- Any change touching parsing, policy enforcement, rendering, encoding, URL handling, or CSS handling requires new regression tests with hostile payloads, not just benign inputs. Where the code walks the input recursively, keeps state per token, or re-scans what it has already seen, one of those tests runs at hostile size, hundreds of thousands of nested or repeated tokens, and shows no error and linear time.
 - Never weaken, delete, or loosen an existing test to make a change pass. A failing security test means the change is wrong.
 - No new dependencies. The published library has zero runtime dependencies. The only compile-time dependency is `spotbugs-annotations` (`provided` scope), which brings in JSR 305 (`com.google.code.findbugs:jsr305`) for the `javax.annotation` nullability and concurrency annotations. Both are annotations only and are not shipped. Test-scope dependencies (JUnit, commons-codec, validator.nu htmlparser) must stay test-scope. Keep it that way.
 - Follow the Contributing section of `README.md`: open an issue first to reach the maintainers, and include both positive and negative tests in any PR that changes behavior or adds functionality.
@@ -55,7 +56,7 @@ Hard rules. Never violate them, never refactor around them.
 - If you find a possible bypass while working, report it privately per `SECURITY.md` so a GitHub security advisory can coordinate the fix.
 - Regression tests for a vulnerability land publicly only after the advisory and fixed release are out.
 
-## Known Bypass Classes for Regression Testing
+## Known Bypass and Denial-of-Service Classes for Regression Testing
 
 Use these as targets when touching parser, policy, or renderer code:
 
@@ -65,4 +66,9 @@ Use these as targets when touching parser, policy, or renderer code:
 - SVG and MathML namespace and case tricks
 - `javascript:`, `data:`, and scheme-relative URLs in allowed attributes
 - Entity and encoding edge cases: malformed entities, entities without semicolons, null bytes, mixed case, nested and unbalanced tags
+- Denial of service: unbounded recursion over nested CSS functions (GHSA-x6fc-6qh4-g3wr), quadratic tag-balancer paths and unbounded formatting reconstruction (release 20260921.1, #490)
 - Full history: `docs/vulnerabilities.md`
+
+## Keeping This File Current
+
+When a bug, a vulnerability, or a review finding shows that a rule was missing here, add the rule, judiciously: one that would have prevented the mistake, stated once, in the section it belongs to, in the voice of its neighbours. Do not log the incident; `change_log.md`, `docs/vulnerabilities.md` and the git history hold that. Do not add a rule for a one-off that no rule would have caught, and do not restate a rule that is already here.

CLAUDE.md

@@ -6,3 +6,6 @@ invariants, build and test commands, change rules, and vulnerability
 handling policy.
 
 Nothing in this file overrides `AGENTS.md`.
+
+When a mistake shows that a rule was missing, add the rule to `AGENTS.md`,
+judiciously, as its last section says, so the mistake is not made twice.

change_log.md

@@ -1,6 +1,18 @@
 # OWASP Java HTML Sanitizer Change Log
 
 Most recent at top.
+  * Next release
+    * A CSS declaration whose value nests functions more than sixteen deep
+      is dropped whole instead of parsed.  The CSS grammar recurses once per
+      function, so a style attribute that nested functions without bound
+      overflowed the thread's stack, about 60 KB of `rgb(` on a default-sized
+      stack and 5 KB on a 256 KB one, and the `StackOverflowError` escaped
+      from `sanitize()`.  Nothing in the schema has a use for anything like
+      that depth.  The declarations before and after the deep one are kept.
+    * The CSS lexer drops a close bracket that has no open partner without
+      walking the stack of open brackets, so a run of unmatched closes after
+      a run of opens no longer takes time quadratic in the input: 320 KB of
+      `(` then `]` took twenty seconds.
   * Release 20260922.1
     * CSS URLs in style attributes now percent-encode single quotes,
       backslashes, and control characters after rewriting so they stay

owasp-java-html-sanitizer/src/main/java/org/owasp/html/CssGrammar.java

@@ -27,8 +27,24 @@
 
 package org.owasp.html;
 
+import java.util.Arrays;
+
 final class CssGrammar {
 
+  /**
+   * The deepest a declaration's value may nest functions before the
+   * declaration is dropped without being parsed.
+   *
+   * <p>{@link #parsePropertyValue} recurses once per function, so a value
+   * that nests functions without bound would overflow the thread's stack:
+   * about 60&nbsp;KB of {@code rgb(} did on a default-sized stack, and about
+   * 5&nbsp;KB on a 256&nbsp;KB one, and the {@code StackOverflowError}
+   * escaped from {@code sanitize()}.  Nothing in {@link CssSchema} has a use
+   * for anything like this depth: a colour function inside a gradient, or a
+   * {@code calc()} inside a transform, nests two or three levels.
+   */
+  static final int MAX_FUNCTION_DEPTH = 16;
+
   private static void errorRecoveryUntilSemiOrCloseBracket(
       CssTokens.TokenIterator it) {
     int bracketDepth = 0;
@@ -81,12 +97,84 @@ static void parsePropertyGroup(String css, PropertyHandler handler) {
       }
       it.advance();
 
+      if (skipIfNestedTooDeeply(it)) {
+        continue;
+      }
+
       handler.startProperty(Strings.toLowerCase(name));
       parsePropertyValue(it, handler);
       handler.endProperty();
     }
   }
 
+  /**
+   * If the value at the iterator's position nests functions deeper than
+   * {@link #MAX_FUNCTION_DEPTH}, moves the iterator past the end of the
+   * declaration and returns true, so the handler never hears of it.
+   * Otherwise leaves the iterator where it was and returns false.
+   *
+   * <p>The declaration ends where {@link #parsePropertyValue} would stop:
+   * at the first semicolon that is inside no function, or at the end of the
+   * input.  A semicolon inside a function ends only that function's
+   * arguments, and one inside a bare bracket ends the declaration.
+   * Stopping exactly there matters: stopping later and seeking back would
+   * read the same tokens again for the next declaration, and again for the
+   * one after, which is quadratic.
+   *
+   * <p>The lexer pairs every bracket, closing what the input left open and
+   * dropping what it never opened, so a close bracket here always closes
+   * the innermost open one.  {@code functionAt} records, per depth, whether
+   * that open bracket is a function.  It is a plain array grown by
+   * doubling, so every open and close costs constant time however deep the
+   * value goes: a {@code BitSet} would not, since clearing a bit makes it
+   * look back for its highest set word, which on a value that opens deep,
+   * then sets and clears its top mark over and over, is quadratic.  A close
+   * bracket with no open partner in this value is left over from an
+   * earlier declaration that ended inside it, and is ignored, as the value
+   * parse ignores it.
+   *
+   * <p>Every function the value parse recurses into lies in this range and
+   * is counted here, so the recursion that follows goes no more than
+   * {@code MAX_FUNCTION_DEPTH} frames deep, however long the input.
+   */
+  private static boolean skipIfNestedTooDeeply(CssTokens.TokenIterator it) {
+    int start = it.tokenIndex();
+    boolean[] functionAt = new boolean[16];
+    int depth = 0;
+    int functionDepth = 0;
+    boolean tooDeep = false;
+    declaration:
+    while (it.hasNext()) {
+      CssTokens.TokenType type = it.type();
+      it.advance();
+      switch (type) {
+        case SEMICOLON:
+          if (functionDepth == 0) { break declaration; }
+          break;
+        case FUNCTION:
+        case LEFT_CURLY:
+        case LEFT_PAREN:
+        case LEFT_SQUARE:
+          if (depth == functionAt.length) {
+            functionAt = Arrays.copyOf(functionAt, depth * 2);
+          }
+          if (functionAt[depth++] = (type == CssTokens.TokenType.FUNCTION)) {
+            if (++functionDepth > MAX_FUNCTION_DEPTH) { tooDeep = true; }
+          }
+          break;
+        case RIGHT_CURLY:
+        case RIGHT_PAREN:
+        case RIGHT_SQUARE:
+          if (depth != 0 && functionAt[--depth]) { --functionDepth; }
+          break;
+        default:
+          break;
+      }
+    }
+    if (!tooDeep) { it.seek(start); }
+    return tooDeep;
+  }
+
   private static void parsePropertyValue(
       CssTokens.TokenIterator it, PropertyHandler handler) {
     propertyValueLoop:

owasp-java-html-sanitizer/src/main/java/org/owasp/html/CssTokens.java

@@ -326,6 +326,14 @@ private static final class Lexer {
      * {@code open[openLimit:]} is garbage space that the stack can grow into.
      */
     private int openLimit = 0;
+    /**
+     * For each close bracket character, how many brackets on {@link #open}
+     * it would close, so that a close bracket with no open partner is
+     * dropped without walking the stack.  Walking it made a run of
+     * unmatched closes after a run of opens take time quadratic in the
+     * input.  Indexed by the close bracket character.
+     */
+    private final int[] openCounts = new int['}' + 1];
 
     Lexer(String css) {
       this.css = css;
@@ -347,13 +355,19 @@ TokenType openBracket(char bracketChar) {
       open = expandIfNecessary(open, openLimit, 2);
       open[openLimit++] = bracketsLimit;
       open[openLimit++] = close;
+      ++openCounts[close];
       brackets[bracketsLimit++] = tokenBreaksLimit;
       brackets[bracketsLimit++] = -1;
       sb.append(bracketChar);
       return type;
     }
 
     void closeBracket(char bracketChar) {
+      if (openCounts[bracketChar] == 0) {
+        // Drop an orphaned close bracket.
+        breakOutput();
+        return;
+      }
       int openLimitAfterClose = openLimit;
       do {
         if (openLimitAfterClose == 0) {
@@ -376,6 +390,7 @@ private void closeBrackets(int openLimitAfterClose) {
         // Pop the stack.
         int closeBracket = open[--openLimit];
         int openBracketIndex = open[--openLimit];
+        --openCounts[closeBracket];
         int openTokenIndex = brackets[openBracketIndex];
         // Update open bracket to point to its partner.
         brackets[openBracketIndex + 1] = closeTokenIndex;

owasp-java-html-sanitizer/src/test/java/org/owasp/html/CssTokensTest.java

@@ -27,6 +27,7 @@
 
 package org.owasp.html;
 
+import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
@@ -37,6 +38,7 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertIterableEquals;
+import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively;
 import static org.owasp.html.CssTokens.TokenType.COLUMN;
 import static org.owasp.html.CssTokens.TokenType.IDENT;
 import static org.owasp.html.CssTokens.TokenType.LEFT_PAREN;
@@ -230,6 +232,33 @@ void testIdentReencoding() {
   @Test
   void testOrphanedCloseBrackets() {
     assertEquals("{foo bar}", lex("{foo]bar").normalizedCss);
+    // A close with no open partner is dropped; one whose partner is below
+    // other opens closes those first.
+    assertEquals("((( )))", lex("(((]]]").normalizedCss);
+    assertEquals("[()]", lex("[(])").normalizedCss);
+    assertEquals("a(b c)d", lex("a(b]c)d").normalizedCss);
+    assertEquals("rgb(1 2)", lex("rgb(1]2)").normalizedCss);
+    assertEquals("foo bar", lex("foo)bar").normalizedCss);
+    assertEquals("((()))", lex(")))(((").normalizedCss);
+    assertEquals("{[()]}", lex("{[(}])").normalizedCss);
+    assertEquals("x y z", lex("x]y)z}").normalizedCss);
+  }
+
+  /**
+   * Dropping a close bracket with no open partner used to walk the whole
+   * stack of open brackets, so a run of unmatched closes after a run of
+   * opens took time quadratic in the input: 320 KB took twenty seconds.
+   */
+  @Test
+  void testOrphanedCloseBracketsAreDroppedInLinearTime() {
+    int n = 400000;
+    StringBuilder sb = new StringBuilder(2 * n);
+    for (int i = 0; i < n; ++i) { sb.append('('); }
+    for (int i = 0; i < n; ++i) { sb.append(']'); }
+    final String css = sb.toString();
+    CssTokens tokens = assertTimeoutPreemptively(
+        Duration.ofSeconds(20), () -> CssTokens.lex(css));
+    assertEquals(2 * n + 1, tokens.normalizedCss.length());
   }
 
   @Test

owasp-java-html-sanitizer/src/test/java/org/owasp/html/SanitizersTest.java

@@ -454,6 +454,38 @@ void testSkipIfEmptyUnionsProperly() {
     assertEquals(want, policy.sanitize(input));
   }
 
+  /**
+   * Deeply nested CSS functions in a style attribute used to overflow the
+   * stack in the CSS grammar and let the {@code StackOverflowError} escape
+   * {@code sanitize()}.  The declaration is now dropped and the rest of the
+   * attribute kept.
+   */
+  @Test
+  void testDeeplyNestedCssFunctionsDoNotOverflowTheStack() {
+    PolicyFactory policy = Sanitizers.STYLES.and(Sanitizers.BLOCKS);
+    int depth = 200000;
+    StringBuilder opens = new StringBuilder(depth * 4);
+    StringBuilder closes = new StringBuilder(depth);
+    for (int i = 0; i < depth; ++i) {
+      opens.append("rgb(");
+      closes.append(')');
+    }
+    assertEquals(
+        "<div>x</div>",
+        policy.sanitize(
+            "<div style=\"color:" + opens + "1" + closes + "\">x</div>"));
+    assertEquals(
+        "<div style=\"color:red;background:blue\">x</div>",
+        policy.sanitize(
+            "<div style=\"color: red; width: " + opens + "1" + closes
+            + "; background: blue\">x</div>"));
+    // Unclosed, the lexer closes the functions at the end of the attribute.
+    assertEquals(
+        "<div style=\"color:red\">x</div>",
+        policy.sanitize(
+            "<div style=\"color: red; width: " + opens + "1\">x</div>"));
+  }
+
   @Test
   void testIssue30() {
     String test = "&nbsp;&gt;";