Repository navigation
fix(security): eliminate polynomial ReDoS in sanitizer, renderer and router - #3
Merged
Merged
Conversation
…router
Every tag pattern in HtmlSanitizer ended in `[^>]*`. On input containing many
'<' and no '>', the engine rescanned to end of input from each '<' and
backtracked, making sanitizing quadratic in input size.
MAX_INPUT_SIZE did not mitigate this - 1MB is inside the cap, and quadratic
growth at that size extrapolates to ~19 minutes of blocked event loop. Node is
single-threaded, so one request stalled the whole process; on Lambda it is a
billed timeout. Measured: 20KB 421ms, 40KB 1.6s, 80KB 6.6s, a clean 4x per
doubling. After the fix 500KB completes in under 60ms.
Tag patterns now match unquoted characters and fully quoted attribute values
separately, via a shared TAG_ATTRS pattern. The alternatives are disjoint on
their first character, so matching is deterministic and linear. A plain
`[^<>]*` would have been linear but wrong: a quoted attribute value may legally
contain '<', and ending the tag there let attribute content escape into the
document as structure - `<a title="a<b">t</a>` emitted a live `<b>`.
CodeQL flagged 2 of these sites; the same defect was present at 3 more it did
not report. All five are fixed, plus:
- Router path normalization used `replace(/\/+$/, '')`, whose anchored `+` made
the engine retry from every index in a run of slashes. Paths are
attacker-controlled though bounded by HTTP header limits, so the cost was
~100-260ms of CPU per request. Now an index scan.
- HtmlRenderer matched `{{#if}}` conditions with `\s+([^}]+)`, where both parts
match a space, so the engine tried every split point. Exploiting it needs an
attacker-controlled template - data goes through the slot table and is never
re-parsed - but HtmlRenderer has no input cap, so it is fixed regardless.
Behaviour is unchanged apart from one case: `clean('<<b>>')` now returns '' where
it returned '>'. Verified by diffing every sanitizer and renderer output
against 10.3.0 across a payload corpus - 1 difference in 130 comparisons, in the
direction of removing more.
Adds ReDoS regression tests asserting an absolute time budget on pathological
input: 4-55 seconds each against the unfixed code, milliseconds against the fix.
They assert a ceiling rather than a growth ratio between input sizes, because at
millisecond scale the ratio is dominated by JIT warmup and was flaky.
Also adds sanitizer bypass tests pinning the multi-character-sanitization
mitigation that CodeQL cannot see (strip-until-stable, escape residual '<',
allowlist builder), so the two false-positive dismissals fail loudly if that
defense is ever removed.
CodeQL's PR scan found the first HtmlRenderer fix incomplete. Requiring the
condition to start with a non-space fixed one long whitespace run, but not the
general case: with `{{#if` repeated, every occurrence is a start position whose
`[^}]*` condition scan still ran to end of input, so the work stayed quadratic
on input like `{{{{#if !` repeated.
A condition can never contain a brace, so excluding `{` and `}` bounds each scan
at the next `{` instead of at end of input. Measured over 2k/8k/32k repetitions
of four different opening shapes, all now linear - 32,000 repetitions (~576KB of
template) completes in under 30ms.
Adds those four vectors as regression tests, including the `{{#each}}` opening,
which shares the shape.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the 5 live
js/polynomial-redosCodeQL alerts, and pins the mitigation behind the 2 that are false positives.The serious one
Every tag pattern in
HtmlSanitizerended in[^>]*. On input with many<and no>, the engine rescanned to end of input from each<and backtracked — quadratic in input size.MAX_INPUT_SIZEdoes not mitigate this. 1MB is inside the cap:clean()beforeA clean 4x per doubling. Node is single-threaded, so one request stalls the whole process; on Lambda it's a billed timeout. Sanitizer input is attacker-controlled by definition.
The fix
Tag patterns now match unquoted characters and fully quoted attribute values separately, via a shared
TAG_ATTRS. The three alternatives are disjoint on their first character, so matching is deterministic and linear.A plain
[^<>]*would also have been linear but wrong — a quoted attribute value may legally contain<, and ending the tag there let attribute content escape as document structure. I caught this mid-fix:<a title="a<b">t</a>was emitting a live<b>. The quote-aware pattern returns<a>t</a>, matching 10.3.0.CodeQL flagged 2 of these sites; the same defect was at 3 more it did not report. All five fixed.
Also fixed:
Routerpath normalization (/\/+$/→ index scan; ~100–260 ms CPU per request, bounded by HTTP header limits) andHtmlRenderer{{#if}}conditions (\s+([^}]+)overlap; needs an attacker-controlled template, so low real-world risk, but there's no input cap).Behaviour change: one case
clean('<<b>>')returns''where it returned'>'. Verified by diffing every sanitizer and renderer output against published 10.3.0 across a payload corpus — 1 difference in 130 comparisons, in the direction of removing more.Tests
ReDoS regression tests — absolute time budgets on pathological input. Against the unfixed code these take 4–55 seconds each and fail; against the fix, milliseconds. I first wrote them as growth-ratio assertions and the pre-commit hook caught them being flaky (JIT warmup dominates at ms scale), so they assert a ceiling instead. Verified stable across 5 consecutive runs.
Sanitizer bypass tests — 15 payloads covering nested reassembly (
<scr<script>ipt>), triple nesting, comment splitting, entity/hex encoding, tab-brokenjavascript:, andon*handlers smuggled through attribute values.These pin the mitigation CodeQL can't see behind alerts #3 and #16. It reports
js/incomplete-multi-character-sanitizationon the individual.replace()calls, but the sanitizer strips until stable, escapes residual<, and re-emits only allowlisted tags. Those payload tests pass against the unfixed code too, which is independent confirmation the alerts were already mitigated. Dismissing them is safe precisely because these tests will fail if the defense is removed.852 tests passing, 0 lint errors, clean under TS 5.9.3 and 7.0.2.