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. diff --git a/change_log.md b/change_log.md index 79f206be..83860730 100644 --- a/change_log.md +++ b/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 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..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,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. + * + *

{@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; @@ -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. + * + *

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. 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) { + 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: 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/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..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 @@ -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,113 @@ 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"); + } + + /** + * 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. 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() { + final String rescan = repeat("a:(;a:)", 200000); + assertEquals( + "color:red", + 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))); + // 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) { StringBuilder sb = new StringBuilder(s.length() * n); for (int i = 0; i < n; ++i) { @@ -708,6 +817,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 +832,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); } }