From bba5ad91110be0265109ffa9b0da677d73dc3328 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 11 Oct 2026 18:37:58 +0200 Subject: [PATCH] security: cap :not()/:has()/:host() recursion depth and :is() output count Continuing the same audit as the calc() fix in this branch: two more text-level CSS parsers recurse on attacker-controlled nesting depth with no guard. parseSimple/parseSimpleSelectorList/parseHasSelectorList (the :not()/:has()/:host() argument parsers) form one mutually recursive cycle with no depth check -- the same shape as calc()'s, but each level's own string-scanning cost compounds, making it closer to quadratic than linear: a 500,000-deep :not(:not(...)) chain used to time out past 60s and now completes in ~2.5s. Fixed with a depth parameter threaded through all three, capped at maxSelectorNestingDepth (256, matching calc()'s own cap). expandFunctionalPseudos (:is()/:where()/:matches()) is a different, more severe failure mode: it doubles its OUTPUT COUNT at every level a comma-containing wrapper nests inside another one of itself -- n levels of self-nesting give 2^n alternatives from a selector whose own TEXT depth is just n. 20 such levels (elaborate-looking, not obviously malicious) already produces 1,048,576 alternatives and takes several seconds. A depth cap alone would not have been enough, since the depth itself stays small while the output explodes -- fixed instead with a shared output budget (expandFunctionalPseudosBudget, maxSelectorExpansions = 4096) threaded through the whole expansion, truncating the result once the TOTAL alternatives produced hits the cap. Co-Authored-By: Claude Sonnet 5 --- FIDELITY.md | 8 ++++- css/edge_test.go | 4 +-- css/position_test.go | 2 +- css/selector.go | 82 +++++++++++++++++++++++++++++++++++++------- css/selector_test.go | 69 +++++++++++++++++++++++++++++++++++-- 5 files changed, 146 insertions(+), 19 deletions(-) diff --git a/FIDELITY.md b/FIDELITY.md index 09dd6ce..7d3d829 100644 --- a/FIDELITY.md +++ b/FIDELITY.md @@ -18,7 +18,7 @@ The committed PNGs under `testdata/renders/` back every claim here. Reproduce them with the commands at the bottom. The measured-vs-Chrome numbers live in [`bench/REPORT.md`](bench/REPORT.md). -## 2026-10-11 (round 170) — a pathologically deep `calc()` nesting in a single `style="..."` attribute crashed the process with an unrecoverable stack overflow, the same class of bug round 166 fixed for DOM trees (security audit, patch) +## 2026-10-11 (round 170) — three text-level CSS parsers (`calc()`, `:not()`, `:is()`) recursed on attacker-controlled nesting depth with no guard, the same class of bug round 166 fixed for DOM trees — one of them exponential, not merely deep recursion (security audit, patch) Continuing the "audit de sécurité" pass after round 166's DOM-depth fix: checked whether any OTHER recursive-descent parser in this codebase has the same shape — unbounded recursion keyed to attacker-controlled input depth, with no equivalent to the HTML parser's own 512-node guard. `css/calc.go`'s `calcParser` does: its grammar is `expr := term (('+'|'-') term)*`, `term := factor (('*'|'/') factor)*`, `factor := number[unit] | '(' expr ')'` — a textbook mutually-recursive expression grammar, and `factor`'s own paren branch recurses straight back into `expr` with no depth check at all. @@ -27,6 +27,12 @@ Continuing the "audit de sécurité" pass after round 166's DOM-depth fix: check - **Test**: `TestResolveCalcDoesNotCrashOnPathologicallyDeepParens` — a 10,000-deep synthetic value (comfortably past the 256 cap, cheap to build and run) asserts `resolveCalc` reports failure instead of crashing. - **Coverage**: `css` package coverage unaffected (99.5% before and after, confirmed via `git stash`); `factor`'s own coverage actually rises slightly (92.0%→95.0%) since the new depth-cap branch is exercised by the new test — the two statements it remains short of (a `ParseFloat` failure path, a numeric-sign-prefix path) are pre-existing gaps this round did not touch. +Continuing the same check into `css/selector.go` found two MORE instances of the same general class, one of them more severe than `calc()`'s: + +- **`:not(:not(:not(...)))` nesting**: `parseSimple` (compound parsing), `parseSimpleSelectorList` (`:not`/`:host` arguments) and `parseHasSelectorList` (`:has` arguments) form one mutually recursive cycle with no depth check, the exact same shape as `calc()`'s — and WORSE than a plain stack overflow at smaller scale: each level's own string-scanning cost compounds, so the cost is closer to quadratic in nesting depth than linear. Confirmed live: a 500,000-deep synthetic `:not(:not(...))` chain that used to time out past 60 seconds unbounded now completes in ~2.5s. Fixed the same way as `calc()`: a `depth` parameter threaded through all three functions, capped at `maxSelectorNestingDepth` (256, matching `maxCalcDepth` — both guard the identical shape of bug). Test: `TestParseSimpleDoesNotCrashOnPathologicallyDeepNotNesting`. +- **`:is(:is(...),:is(...))` nesting — a DIFFERENT, more severe failure mode: exponential output, not merely deep recursion.** `expandFunctionalPseudos` (the `:is()`/`:where()`/`:matches()` expander) doubles its OUTPUT COUNT at every level a comma-containing wrapper is nested inside another one of itself: `n` levels of self-nesting give `2^n` alternatives from a selector whose own TEXT nesting depth is just `n`, not `2^n` — so even a depth cap alone would not have been enough; a selector that LOOKS only moderately nested can already be computationally enormous. Confirmed live: 20 levels of this shape — elaborate-looking but not obviously malicious — already produces 1,048,576 expanded alternatives and takes several seconds; one size class further and this becomes a real denial of service from a selector well within this engine's existing page-size limits. Fixed with a shared output BUDGET (not a depth counter) threaded through the whole recursive expansion (`expandFunctionalPseudosBudget`, `maxSelectorExpansions = 4096`): once the total number of alternatives produced across the ENTIRE expansion hits the cap, expansion stops and the selector list is truncated — same "reduce, don't drop" degradation this package already uses elsewhere — rather than continuing to multiply. Test: `TestExpandFunctionalPseudosCapsCombinatorialBlowup` (plus a direct white-box test for the budget-already-exhausted entry guard, a real but awkward-to-drive-naturally path). +- Coverage: both new functions' depth/budget-cap branches land at 100%; `css` package stays at its 99.5% baseline. + ## 2026-10-11 (round 169) — go.mod's own declared Go version had drifted behind CI's, making `govulncheck` report 8 already-patched stdlib vulnerabilities as live; `golang.org/x/net` bumped too (security audit, patch) A concurrent session's PR #270 moved CI's `GO_VERSION` from 1.27.1 to 1.27.2 (a security release, 13 stdlib advisories) but — correctly, since a `go` directive is a floor, not a pin — left `go.mod`'s own `go 1.27.1` line untouched. Re-reading this session's own notes per the standing "relis tes notes" instruction, then re-running `govulncheck` to confirm the new CI pin actually closed something, surfaced a real gap: `govulncheck ./...` reported 8 reachable stdlib vulnerabilities (GO-2026-6605, -6603, -6607, -6608, and others) "Found in: net/http@go1.27.1, Fixed in: go1.27.2" — even though the installed toolchain is 1.27.2. `govulncheck -version` showed why: it had resolved its own subprocess `go` to literally **1.27.1**, not the 1.27.2 my interactive shell's `go version` reported — a live, concrete instance of the exact trap an earlier round's own memory note already names ("Go's `stable` tag can silently point at a different minor than you think — measure with the CI's own toolchain, not an assumption"), except here the drift was go.mod's own stale `go` directive feeding govulncheck's stdlib-vulnerability baseline, not a `stable`-tag alias. diff --git a/css/edge_test.go b/css/edge_test.go index 9fe0083..cabfe78 100644 --- a/css/edge_test.go +++ b/css/edge_test.go @@ -110,10 +110,10 @@ func TestMarginShorthandFiveValues(t *testing.T) { } func TestParseSimpleAllEmpty(t *testing.T) { - if _, ok := parseSimple("."); ok { + if _, ok := parseSimple(".", 0); ok { t.Error("lone . should fail (no name)") } - if _, ok := parseSimple(":only"); ok { + if _, ok := parseSimple(":only", 0); ok { t.Error("lone pseudo should fail") } } diff --git a/css/position_test.go b/css/position_test.go index 68581a8..c888b3f 100644 --- a/css/position_test.go +++ b/css/position_test.go @@ -189,7 +189,7 @@ func TestSelectorListKeepsNonDynamicMembers(t *testing.T) { func TestDynamicPseudoSpecificity(t *testing.T) { // A dynamic pseudo contributes class-level specificity, like :root. - c, ok := parseSimple(".btn:hover") + c, ok := parseSimple(".btn:hover", 0) if !ok || !c.Dynamic { t.Fatalf("parse .btn:hover => %+v ok=%v", c, ok) } diff --git a/css/selector.go b/css/selector.go index 1f5f3f2..2e00d8b 100644 --- a/css/selector.go +++ b/css/selector.go @@ -912,7 +912,7 @@ func hasMatchingDescendant(n *dom.Node, sel compound) bool { // multi-compound chain, an unmodelled/dynamic pseudo, ...) is skipped — // never treated as "no constraint" (see compound.Has's own doc comment for // why :has() specifically fails closed rather than open). -func parseHasSelectorList(arg string) []hasRelative { +func parseHasSelectorList(arg string, depth int) []hasRelative { var out []hasRelative for _, alt := range splitSelectorCommas(arg) { alt = strings.TrimSpace(alt) @@ -928,7 +928,7 @@ func parseHasSelectorList(arg string) []hasRelative { case '~': kind, alt = hasSubsequentSibling, strings.TrimSpace(alt[1:]) } - sel, ok := parseSimple(alt) + sel, ok := parseSimple(alt, depth) if !ok || sel.Dynamic { continue } @@ -990,12 +990,54 @@ func splitSelectorCommas(s string) []string { // is approximated as its argument's (a minor over-count that does not affect // these pages). Selectors with no such wrapper are returned unchanged. func expandFunctionalPseudos(sel string) []string { + budget := maxSelectorExpansions + return expandFunctionalPseudosBudget(sel, &budget) +} + +// maxSelectorExpansions bounds the TOTAL number of strings +// expandFunctionalPseudos may ever produce across its entire recursive +// expansion, not merely how deeply it nests. Nesting depth alone is not what +// makes this dangerous: a comma-containing wrapper doubles (or more) its +// OUTPUT COUNT at every level it is nested inside another one of itself — +// `:is(:is(a,b),:is(a,b))` repeated n levels deep produces 2^n alternatives +// from a selector whose own nesting depth is just n, not 2^n. Confirmed +// live: 20 such levels (a perfectly plausible-looking, if elaborate, +// selector — nothing about it screams "attack") already produces 1,048,576 +// alternatives and takes several seconds; one more size class up and the +// cost becomes a genuine denial of service from a selector well within this +// engine's existing page-size limits. 4096 is enormously generous against +// any real selector (which needs at most a handful of true alternatives, +// not thousands) while keeping the worst case trivial. +const maxSelectorExpansions = 4096 + +// maxSelectorNestingDepth bounds parseSimple/parseSimpleSelectorList/ +// parseHasSelectorList's own mutual recursion — see parseSimple's own doc +// comment for why. 256 matches css/calc.go's maxCalcDepth: both guard the +// same shape of bug (a text-level parser recursing with no depth check, +// bounded only by the document's own 32MB fetch cap) and both are +// enormously generous against any real selector while keeping the worst +// case trivial. +const maxSelectorNestingDepth = 256 + +// expandFunctionalPseudosBudget is expandFunctionalPseudos's own recursive +// implementation, threading a shared output budget through every level so +// the cap applies to the TOTAL expansion, not a fresh allowance per call. +// Hitting the cap truncates the result (a selector list with fewer +// alternatives than the source literally names) rather than failing the +// whole selector — the same "reduce, don't drop" degradation this package +// already applies to every other "too much to fully handle" case. +func expandFunctionalPseudosBudget(sel string, budget *int) []string { + if *budget <= 0 { + return nil + } open, argOpen := findFunctionalPseudo(sel) if open < 0 { + *budget-- return []string{sel} } close, ok := matchParenAt(sel, argOpen) if !ok { + *budget-- return []string{sel} // malformed; leave as-is (will likely fail to parse) } prefix := sel[:open] @@ -1003,6 +1045,9 @@ func expandFunctionalPseudos(sel string) []string { inner := sel[argOpen+1 : close] var out []string for _, alt := range splitSelectorCommas(inner) { + if *budget <= 0 { + break + } alt = strings.TrimSpace(alt) if alt == "" { continue @@ -1024,16 +1069,16 @@ func expandFunctionalPseudos(sel string) []string { // stripTrailingSelfCombinator. if prefix != "" { if head, comb, ok := stripTrailingSelfCombinator(alt); ok { - out = append(out, expandFunctionalPseudos(head+string(comb)+prefix+suffix)...) + out = append(out, expandFunctionalPseudosBudget(head+string(comb)+prefix+suffix, budget)...) continue } } - out = append(out, expandFunctionalPseudos(prefix+alt+suffix)...) + out = append(out, expandFunctionalPseudosBudget(prefix+alt+suffix, budget)...) } if len(out) == 0 { // An empty :is()/:where() matches nothing; drop the wrapper's effect by // keeping the surrounding selector. - return expandFunctionalPseudos(prefix + suffix) + return expandFunctionalPseudosBudget(prefix+suffix, budget) } return out } @@ -1159,7 +1204,7 @@ func parseComplex(s string) (Selector, bool) { pendingComb, haveComb = t.combKind, true continue } - c, ok := parseSimple(t.text) + c, ok := parseSimple(t.text, 0) if !ok { return Selector{}, false } @@ -1253,7 +1298,20 @@ func tokenizeSelector(s string) []selToken { // a class token like `.dark\:bg-wash-dark` (Tailwind's `dark:`, `sm:`, `lg:` // variant syntax) is parsed as the single class `dark:bg-wash-dark` rather than // being truncated at the colon. Without this, no Tailwind variant class matches. -func parseSimple(s string) (compound, bool) { +// +// depth counts nested :not()/:has()/:host() argument parsing — parseSimple, +// parseSimpleSelectorList and parseHasSelectorList form one mutually +// recursive cycle (the three pseudos' own arguments are themselves parsed +// via this same function), and depth bounds it the same way css/calc.go's +// own parser bounds paren nesting: `:not(:not(:not(...)))` has no parser- +// level guard the way deep HTML element nesting does, and a page's CSS text +// is bounded only by the main document's own 32MB cap (engine.go) — ample +// room for a pathological chain. A root call (parseComplex, ParseSelectorList's +// own callers) always passes 0. +func parseSimple(s string, depth int) (compound, bool) { + if depth > maxSelectorNestingDepth { + return compound{}, false + } var c compound // Separate the compound's tag/class/id/attribute base from its pseudo tokens // at the first top-level (not inside [] or ()) ':'. ":root" is honoured (it @@ -1320,13 +1378,13 @@ func parseSimple(s string) (compound, bool) { // to unknown pseudos. Attribute selectors inside ":not()" ARE modelled // (see below), so `:not([data-theme])` is now a real negation, not a // no-op. - c.Not = append(c.Not, parseSimpleSelectorList(arg)...) + c.Not = append(c.Not, parseSimpleSelectorList(arg, depth+1)...) case "has": // ":has()" — see compound.Has's own doc // comment for the scope (single-compound alternatives, four // relationship kinds) and why this fails closed rather than open. c.HasPresent = true - c.Has = parseHasSelectorList(arg) + c.Has = parseHasSelectorList(arg, depth+1) case "host": // ":host" / ":host()" — see compound.Host's doc // comment. ":host-context(...)" is a real, related pseudo (matches @@ -1347,7 +1405,7 @@ func parseSimple(s string) (compound, bool) { c.Host = true if arg != "" { c.HostHasArg = true - c.HostSelector = parseSimpleSelectorList(arg) + c.HostSelector = parseSimpleSelectorList(arg, depth+1) } case "part": // "::part()" — see compound.Part's own doc comment. An empty @@ -1502,14 +1560,14 @@ func pseudoNameArg(p string) (name, arg string) { // constraints (tag/class/id/:checked/nested :not/:host) are returned, so // `:not(:checked)` is precise while `:root:not([data-theme])` degrades to // `:root` rather than dropping the rule. -func parseSimpleSelectorList(arg string) []compound { +func parseSimpleSelectorList(arg string, depth int) []compound { var out []compound for _, alt := range splitSelectorCommas(arg) { alt = strings.TrimSpace(alt) if alt == "" { continue } - neg, ok := parseSimple(alt) + neg, ok := parseSimple(alt, depth) if !ok || neg.Dynamic { continue // unmodelled or always-true-statically → no constraint } diff --git a/css/selector_test.go b/css/selector_test.go index 7dd4395..d153d2a 100644 --- a/css/selector_test.go +++ b/css/selector_test.go @@ -4,6 +4,7 @@ package css import ( + "strings" "testing" "github.com/go-webengine/engine/dom" @@ -54,6 +55,68 @@ func TestParseSelectorList(t *testing.T) { } } +func TestExpandFunctionalPseudosCapsCombinatorialBlowup(t *testing.T) { + // A comma-containing wrapper doubles its OUTPUT COUNT at every level it + // is nested inside another one of itself: n levels of + // ":is(X,X)" (X = the previous level) gives 2^n alternatives from a + // selector whose own nesting depth is just n, not 2^n. Confirmed live + // (round 170): 20 such levels already produced 1,048,576 alternatives + // and took seconds — a quite plausible-looking, if elaborate, selector, + // not an obviously pathological one. maxSelectorExpansions should hold + // the total far below that regardless of how much deeper it nests. + build := func(depth int) string { + s := "a" + for i := 0; i < depth; i++ { + s = ":is(" + s + "," + s + ")" + } + return s + } + // The INPUT string's own length is exponential in depth too (build + // doubles it each level), so this stays well short of depths that would + // make the TEST itself slow to construct — the cap holding at 15 proves + // it holds at 20+ just as well, since nothing about the mechanism + // changes with depth once past it. + for _, depth := range []int{10, 13, 15} { + sel := "div" + build(depth) + got := expandFunctionalPseudos(sel) + if len(got) > maxSelectorExpansions { + t.Errorf("depth %d: got %d expansions, want <= %d (maxSelectorExpansions)", depth, len(got), maxSelectorExpansions) + } + } +} + +func TestExpandFunctionalPseudosBudgetEntryGuard(t *testing.T) { + // expandFunctionalPseudosBudget's own entry check (budget already + // exhausted) is reached only from its "empty :is()/:where() matches + // nothing" fallback call — a real but awkward-to-drive-naturally path + // (it needs the budget to hit zero mid-loop AND every alternative + // processed so far to have trimmed to empty). Exercised directly here + // rather than contorting a selector to hit it, the same "a defensive + // guard proven reachable deserves a direct test, not a convoluted one" + // judgment call this package already makes elsewhere. + budget := 0 + if got := expandFunctionalPseudosBudget("div:is(a)", &budget); got != nil { + t.Errorf("got %v, want nil with an already-exhausted budget", got) + } +} + +func TestParseSimpleDoesNotCrashOnPathologicallyDeepNotNesting(t *testing.T) { + // A style="..." attribute on the main document (capped only at 32MB — + // see engine.go) can trivially carry a selector-like value with + // millions of nested ":not(...)" wrappers. Before maxSelectorNestingDepth + // existed, parseSimple's own recursion through parseSimpleSelectorList + // back into parseSimple (once per ":not(") cost increasingly more work + // per level against an ever-larger remaining string, and crashed with an + // unrecoverable "fatal error: stack overflow" at a few million levels; + // this uses 10,000, comfortably past maxSelectorNestingDepth(256) while + // staying fast to build and run. + const depth = 10000 + sel := strings.Repeat(":not(", depth) + "a" + strings.Repeat(")", depth) + if _, ok := parseSimple(sel, 0); ok { + t.Error("want ok=false past the depth cap, got true") + } +} + func TestSpecificity(t *testing.T) { id, _ := parseComplex("#a") cls, _ := parseComplex(".a") @@ -1131,7 +1194,7 @@ func TestParseComplexEdgeCases(t *testing.T) { } else if sel.Matches(el("div", "", "")) { t.Error(":hover must never match statically") } - c, ok := parseSimple("p.") + c, ok := parseSimple("p.", 0) if !ok || c.Tag != "p" || len(c.Classes) != 0 { t.Errorf("p. = %+v %v", c, ok) } @@ -1140,13 +1203,13 @@ func TestParseComplexEdgeCases(t *testing.T) { len(attr.parts[0].Attrs) != 1 || attr.parts[0].Attrs[0] != (attrMatch{Name: "type", Op: attrEqual, Value: "text"}) { t.Errorf("attr selector = %+v %v", attr, ok) } - if _, ok := parseSimple(""); ok { + if _, ok := parseSimple("", 0); ok { t.Error("empty simple should fail") } // A bare attribute compound (no tag/class/id) is a real, modelled // constraint on its own — e.g. the "[hidden]" idiom — so it must parse, // not be dropped as "reduces to nothing". - if c, ok := parseSimple("[x]"); !ok || len(c.Attrs) != 1 || c.Attrs[0] != (attrMatch{Name: "x", Op: attrPresence}) { + if c, ok := parseSimple("[x]", 0); !ok || len(c.Attrs) != 1 || c.Attrs[0] != (attrMatch{Name: "x", Op: attrPresence}) { t.Errorf("bare attribute compound = %+v %v, want a presence check on \"x\"", c, ok) } }