From 8a55b77854af4e97aa3d40575eba63262de55857 Mon Sep 17 00:00:00 2001 From: Jim Manico Date: Thu, 24 Sep 2026 18:34:32 -0700 Subject: [PATCH 1/4] Drop CSS declarations that nest functions deeper than sixteen CssGrammar.parsePropertyValue recurses once per function, with no bound on the depth, so a style attribute that nested functions without bound overflowed the thread's stack and the StackOverflowError escaped from PolicyFactory.sanitize(): about 60 KB of "rgb(" on a default-sized stack, about 5 KB on a 256 KB one, on JDK 11, 17, 21 and 25, with the prepackaged Sanitizers.STYLES. Before a declaration's value is parsed, scan it for the deepest function nesting; past MAX_FUNCTION_DEPTH, sixteen, skip to the end of the declaration without handing any of it to the handler. The scan stops exactly where the value parse would, at the first semicolon inside no function, so that no token is read twice: a scan that ran on to the matching bracket and seeked back was quadratic on "a:(;a:)" repeated. The lexer pairs every bracket, so a bit per depth is enough to tell a function's close from a bare bracket's. Declarations before and after the deep one are kept, one at the limit parses as it always has, and bare brackets, which are punctuation, do not count. Nothing in CssSchema has a use for anything near that depth. Regression tests at the limit, one past it, and at two hundred thousand levels, with and without the closing brackets, at the StylingPolicy level and through Sanitizers.STYLES.and(Sanitizers.BLOCKS), plus the rescan shape under a timeout. With the guard disabled, two of them die with StackOverflowError and two fail on output. Co-Authored-By: Claude Fable 5.1 --- change_log.md | 8 ++ .../main/java/org/owasp/html/CssGrammar.java | 84 +++++++++++++++ .../java/org/owasp/html/SanitizersTest.java | 32 ++++++ .../org/owasp/html/StylingPolicyTest.java | 101 +++++++++++++++++- 4 files changed, 224 insertions(+), 1 deletion(-) diff --git a/change_log.md b/change_log.md index 79f206be..2b52ff33 100644 --- a/change_log.md +++ b/change_log.md @@ -1,6 +1,14 @@ # 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. * Release 20260922.1 * CSS URLs in style attributes now percent-encode single quotes, backslashes, and control characters after rewriting so they stay diff --git a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/CssGrammar.java b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/CssGrammar.java index a3a34d94..3f7b4ba6 100644 --- a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/CssGrammar.java +++ b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/CssGrammar.java @@ -27,8 +27,24 @@ package org.owasp.html; +import java.util.BitSet; + final class CssGrammar { + /** + * The deepest a declaration's value may nest functions before the + * declaration is dropped without being parsed. + * + *

{@link #parsePropertyValue} recurses once per function, so a value + * that nests functions without bound would overflow the thread's stack: + * about 60 KB of {@code rgb(} did on a default-sized stack, and about + * 5 KB on a 256 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; @@ -64,6 +80,7 @@ static void parsePropertyGroup(String css, PropertyHandler handler) { CssTokens tokens = CssTokens.lex(css); CssTokens.TokenIterator it = tokens.iterator(); + BitSet functionAt = new BitSet(); propertyNameLoop: while (it.hasTokenAfterSpace()) { // Check that we have an identifier that might be a property name. @@ -81,12 +98,79 @@ static void parsePropertyGroup(String css, PropertyHandler handler) { } it.advance(); + if (skipIfNestedTooDeeply(it, functionAt)) { + 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. + * + *

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. + * + *

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. Each bit is written at the open before + * it is read at the matching close, so the set needs no clearing between + * declarations. 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. + * + *

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, BitSet functionAt) { + int start = it.tokenIndex(); + 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: + functionAt.set(depth++); + if (++functionDepth > MAX_FUNCTION_DEPTH) { tooDeep = true; } + break; + case LEFT_CURLY: + case LEFT_PAREN: + case LEFT_SQUARE: + functionAt.clear(depth++); + break; + case RIGHT_CURLY: + case RIGHT_PAREN: + case RIGHT_SQUARE: + if (depth != 0 && functionAt.get(--depth)) { --functionDepth; } + break; + default: + break; + } + } + if (!tooDeep) { it.seek(start); } + return tooDeep; + } + private static void parsePropertyValue( CssTokens.TokenIterator it, PropertyHandler handler) { propertyValueLoop: diff --git a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/SanitizersTest.java b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/SanitizersTest.java index 99be75a5..cd49aa75 100644 --- a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/SanitizersTest.java +++ b/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( + "

x
", + policy.sanitize( + "
x
")); + assertEquals( + "
x
", + policy.sanitize( + "
x
")); + // Unclosed, the lexer closes the functions at the end of the attribute. + assertEquals( + "
x
", + policy.sanitize( + "
x
")); + } + @Test void testIssue30() { String test = " >"; diff --git a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/StylingPolicyTest.java b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/StylingPolicyTest.java index f61051b2..b18b6920 100644 --- a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/StylingPolicyTest.java +++ b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/StylingPolicyTest.java @@ -27,12 +27,14 @@ package org.owasp.html; +import java.time.Duration; import java.util.Arrays; import javax.annotation.Nullable; import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; class StylingPolicyTest { @Test @@ -693,6 +695,95 @@ void testLongCssUrlsAreKeptUpToTheLengthCap() { assertSanitizedCss(null, "background-image: url(" + over + ")"); } + /** + * The grammar recurses once per function, so a declaration may nest + * functions only so deep; one at the limit still parses as it always has. + */ + @Test + void testNestingAtTheLimitIsKept() { + int depth = CssGrammar.MAX_FUNCTION_DEPTH; + // As before: the schema lets no colour function nest, so the inner + // ones go and the outer one stays. + assertSanitizedCss( + "color:rgb( )", + "color: " + repeat("rgb(", depth) + "1" + repeat(")", depth)); + // Bare brackets are punctuation, not recursion, and do not count. + assertSanitizedCss( + "width:calc(" + repeat(" (", 2 * depth) + " 1px" + + repeat(" )", 2 * depth + 1), + "width: calc(" + repeat("(", 2 * depth) + "1px" + + repeat(")", 2 * depth + 1)); + } + + /** + * One level past the limit drops the declaration whole, and only that + * declaration: the ones before and after it are untouched. + */ + @Test + void testNestingBeyondTheLimitDropsTheDeclaration() { + int depth = CssGrammar.MAX_FUNCTION_DEPTH + 1; + assertSanitizedCss( + null, + "color: " + repeat("rgb(", depth) + "1" + repeat(")", depth)); + assertSanitizedCss( + "color:red;background:blue", + "color: red; width: " + repeat("rgb(", depth) + "1" + + repeat(")", depth) + "; background: blue"); + // A semicolon inside a function ends only that function's arguments, + // so the declaration runs on to the one after the closes, and nothing + // inside the dropped value comes back as a declaration of its own. + assertSanitizedCss( + "color:red;background:blue", + "color: red; width: " + repeat("rgb(", depth) + "; color: blue" + + repeat(")", depth) + "; background: blue"); + // A semicolon inside a bare bracket ends the declaration, as it always + // has, so what follows is parsed on its own and is not too deep. + assertSanitizedCss( + "color:red;background:rgb( )", + "width: (; color: red; background: " + repeat("rgb(", depth - 1) + + "1" + repeat(")", depth - 1)); + } + + /** + * A value that nests functions without bound used to overflow the stack + * and let a {@code StackOverflowError} escape {@code sanitize()}: about + * 60 KB of {@code rgb(} on a default-sized thread stack, 5 KB on a small + * one. Now it is dropped, in linear time, with or without its closes. + */ + @Test + void testUnboundedNestingDoesNotOverflowTheStack() { + int depth = 200000; + String opens = repeat("rgb(", depth); + assertSanitizedCss(null, "color: " + opens + "1" + repeat(")", depth)); + assertSanitizedCss(null, "color: " + opens + "1"); + assertSanitizedCss( + "color:red;background:blue", + "color: red; width: " + opens + "1" + repeat(")", depth) + + "; background: blue"); + // Bare brackets are punctuation, and the semicolon inside them ends the + // declaration, as it always has. + assertSanitizedCss( + "color:red;background:blue", + "color: red; width: " + repeat("(", depth) + "; background: blue"); + } + + /** + * A shape that would make the depth scan quadratic. A semicolon inside + * a bare bracket ends a declaration, so a scan for the end of the + * declaration that ran on to the matching close would read the rest of + * the input once per declaration: seconds at a few hundred kilobytes, + * and well under a second here. + */ + @Test + void testHostileBracketShapesAreLinear() { + final String rescan = repeat("a:(;a:)", 200000); + assertEquals( + "color:red", + assertTimeoutPreemptively( + Duration.ofSeconds(20), + () -> sanitizeCss("color: red;" + rescan))); + } + private static String repeat(String s, int n) { StringBuilder sb = new StringBuilder(s.length() * n); for (int i = 0; i < n; ++i) { @@ -708,6 +799,14 @@ private static void assertSanitizedCss( private static void assertSanitizedCss( CssSchema cssSchema, @Nullable String expectedCss, String css) { + assertEquals(expectedCss, sanitizeCss(cssSchema, css)); + } + + private static @Nullable String sanitizeCss(String css) { + return sanitizeCss(CssSchema.DEFAULT, css); + } + + private static @Nullable String sanitizeCss(CssSchema cssSchema, String css) { StylingPolicy stylingPolicy = new StylingPolicy( cssSchema, url -> { @@ -715,6 +814,6 @@ private static void assertSanitizedCss( StandardUrlAttributePolicy.INSTANCE.apply("img", "src", url); return safeUrl != null ? safeUrl + "#sanitized" : null; }); - assertEquals(expectedCss, stylingPolicy.sanitizeCssProperties(css)); + return stylingPolicy.sanitizeCssProperties(css); } } From d0b06bddbcdc241deb86eb7ca9b5070b7f200e73 Mon Sep 17 00:00:00 2001 From: Jim Manico Date: Thu, 24 Sep 2026 18:34:38 -0700 Subject: [PATCH 2/4] Drop an unmatched CSS close bracket in constant time For every close bracket, CssTokens.Lexer.closeBracket walked the stack of open brackets looking for the partner, and walked all of it when there was none: a run of unmatched closes after a run of opens took time quadratic in the input, twenty seconds for 320 KB of "(" then "]", through sanitize() with Sanitizers.STYLES. Found by the review of the nesting fix, and of the same class. Keep, per close bracket character, a count of the open brackets it would close, and drop a close with no partner without the walk. A close that has a partner still pops everything above it, but each open is popped once, so that is linear. The normalized output is unchanged. Tests pin the normalization of orphaned and mismatched closes, and put four hundred thousand of each shape under a timeout, in CssTokensTest and through StylingPolicy. Co-Authored-By: Claude Fable 5.1 --- change_log.md | 4 +++ .../main/java/org/owasp/html/CssTokens.java | 15 ++++++++++ .../java/org/owasp/html/CssTokensTest.java | 29 +++++++++++++++++++ .../org/owasp/html/StylingPolicyTest.java | 18 ++++++++---- 4 files changed, 61 insertions(+), 5 deletions(-) diff --git a/change_log.md b/change_log.md index 2b52ff33..83860730 100644 --- a/change_log.md +++ b/change_log.md @@ -9,6 +9,10 @@ Most recent at top. 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 diff --git a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/CssTokens.java b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/CssTokens.java index 8f80cf52..ada7ff0e 100644 --- a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/CssTokens.java +++ b/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,6 +355,7 @@ 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); @@ -354,6 +363,11 @@ TokenType openBracket(char bracketChar) { } 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; diff --git a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/CssTokensTest.java b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/CssTokensTest.java index 195f9b1d..17750118 100644 --- a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/CssTokensTest.java +++ b/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 diff --git a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/StylingPolicyTest.java b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/StylingPolicyTest.java index b18b6920..233a6caa 100644 --- a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/StylingPolicyTest.java +++ b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/StylingPolicyTest.java @@ -768,11 +768,12 @@ void testUnboundedNestingDoesNotOverflowTheStack() { } /** - * A shape that would make the depth scan quadratic. A semicolon inside - * a bare bracket ends a declaration, so a scan for the end of the - * declaration that ran on to the matching close would read the rest of - * the input once per declaration: seconds at a few hundred kilobytes, - * and well under a second here. + * Shapes that made the CSS path quadratic. A semicolon inside a bare + * bracket ends a declaration, so a scan for the end of the declaration + * that ran on to the matching close read the rest of the input once per + * declaration. And the lexer walked the whole stack of open brackets + * for every close bracket that had no partner. Each takes seconds at a + * few hundred kilobytes when quadratic and well under a second here. */ @Test void testHostileBracketShapesAreLinear() { @@ -782,6 +783,13 @@ void testHostileBracketShapesAreLinear() { assertTimeoutPreemptively( Duration.ofSeconds(20), () -> sanitizeCss("color: red;" + rescan))); + final String orphans = + "color: " + repeat("(", 400000) + repeat("]", 400000); + assertEquals( + "color:red", + assertTimeoutPreemptively( + Duration.ofSeconds(20), + () -> sanitizeCss("color: red;" + orphans))); } private static String repeat(String s, int n) { From 82e9b108772d8a30e4fe4738e6c0997a9be7d9b1 Mon Sep 17 00:00:00 2001 From: Jim Manico Date: Thu, 24 Sep 2026 18:34:38 -0700 Subject: [PATCH 3/4] Add the denial-of-service invariant to AGENTS.md The CSS grammar's unbounded recursion was a class of bug the guidance did not name: it spoke of bypasses, not of input that crashes or hangs the sanitizer. Name it as an invariant, ask for a hostile-size test wherever code walks the input recursively or keeps state per token, list it with the regression targets, and say how this file is to grow when the next mistake shows a rule was missing. Co-Authored-By: Claude Fable 5.1 --- AGENTS.md | 10 ++++++++-- CLAUDE.md | 3 +++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 22011c8a..e645d517 100644 --- a/AGENTS.md +++ b/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. diff --git a/CLAUDE.md b/CLAUDE.md index b7b09676..0b1326b9 100644 --- a/CLAUDE.md +++ b/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. From 1231831b2d717e176262739204622cec475cc210 Mon Sep 17 00:00:00 2001 From: Jim Manico Date: Thu, 24 Sep 2026 18:46:30 -0700 Subject: [PATCH 4/4] Take the review: mark function depths in a plain array, not a BitSet The depth scan recorded, per bracket depth, whether the open bracket is a function, in a BitSet. BitSet.clear(int) looks back from the highest word in use for the next non-zero word, and the marks are indexed by total bracket depth, which the function limit does not bound. A value that opens deep with bare brackets, then sets and clears its top mark over and over ("(" x d then "rgb()()" x k) paid d/64 per repeat: 162, 274, 850 and 3,165 ms at 1, 2, 4 and 8 MB. Found by Astra's review. Use a boolean array grown by doubling and allocated per declaration, so every open and close costs constant time and no state crosses from one declaration to the next. The same shape now takes 116, 94, 140 and 304 ms; the other fourteen hostile shapes are unchanged, and 500,000 random inputs give the same output as before. The timed test gains this shape at 4 MB. Its 20 s budget guards only a gross regression: the quadratic constant here is small enough that the old code passed it in under a second, and the scaling above is the evidence. Co-Authored-By: Claude Fable 5.1 --- .../main/java/org/owasp/html/CssGrammar.java | 34 +++++++++++-------- .../org/owasp/html/StylingPolicyTest.java | 16 +++++++-- 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/CssGrammar.java b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/CssGrammar.java index 3f7b4ba6..8254f250 100644 --- a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/CssGrammar.java +++ b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/CssGrammar.java @@ -27,7 +27,7 @@ package org.owasp.html; -import java.util.BitSet; +import java.util.Arrays; final class CssGrammar { @@ -80,7 +80,6 @@ static void parsePropertyGroup(String css, PropertyHandler handler) { CssTokens tokens = CssTokens.lex(css); CssTokens.TokenIterator it = tokens.iterator(); - BitSet functionAt = new BitSet(); propertyNameLoop: while (it.hasTokenAfterSpace()) { // Check that we have an identifier that might be a property name. @@ -98,7 +97,7 @@ static void parsePropertyGroup(String css, PropertyHandler handler) { } it.advance(); - if (skipIfNestedTooDeeply(it, functionAt)) { + if (skipIfNestedTooDeeply(it)) { continue; } @@ -125,19 +124,22 @@ static void parsePropertyGroup(String css, PropertyHandler handler) { *

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. Each bit is written at the open before - * it is read at the matching close, so the set needs no clearing between - * declarations. 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. + * 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. * *

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, BitSet functionAt) { + 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; @@ -150,18 +152,20 @@ private static boolean skipIfNestedTooDeeply( if (functionDepth == 0) { break declaration; } break; case FUNCTION: - functionAt.set(depth++); - if (++functionDepth > MAX_FUNCTION_DEPTH) { tooDeep = true; } - break; case LEFT_CURLY: case LEFT_PAREN: case LEFT_SQUARE: - functionAt.clear(depth++); + 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.get(--depth)) { --functionDepth; } + if (depth != 0 && functionAt[--depth]) { --functionDepth; } break; default: break; diff --git a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/StylingPolicyTest.java b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/StylingPolicyTest.java index 233a6caa..9b26366b 100644 --- a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/StylingPolicyTest.java +++ b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/StylingPolicyTest.java @@ -771,9 +771,10 @@ void testUnboundedNestingDoesNotOverflowTheStack() { * Shapes that made the CSS path quadratic. A semicolon inside a bare * bracket ends a declaration, so a scan for the end of the declaration * that ran on to the matching close read the rest of the input once per - * declaration. And the lexer walked the whole stack of open brackets - * for every close bracket that had no partner. Each takes seconds at a - * few hundred kilobytes when quadratic and well under a second here. + * declaration. The lexer walked the whole stack of open brackets for + * every close bracket that had no partner. And the depth scan's marks + * lived in a {@code BitSet}, whose clear looks back for the highest set + * word. Each takes seconds when quadratic and well under a second here. */ @Test void testHostileBracketShapesAreLinear() { @@ -790,6 +791,15 @@ void testHostileBracketShapesAreLinear() { assertTimeoutPreemptively( Duration.ofSeconds(20), () -> sanitizeCss("color: red;" + orphans))); + // Opens deep, then sets and clears the depth scan's top mark over and + // over. Bookkeeping that looks back from the top on each clear, as a + // BitSet does, makes each repeat cost the depth. + final String churn = + "color: red; width: " + repeat("(", 500000) + repeat("rgb()()", 500000); + assertEquals( + "color:red", + assertTimeoutPreemptively( + Duration.ofSeconds(20), () -> sanitizeCss(churn))); } private static String repeat(String s, int n) {