Repository navigation
🐛 FIX: resolve regex vulnerabilities - #148
Conversation
KyleKing
changed the title
Hoist inline and string regexes to module-level compiled patterns (amsmath, tasklists, dollarmath, anchors) so the ReDoS guard discovers them. Compiling is also faster for these hot paths. Inline re.match(str, ...) still compiles once and caches, but pays a cache lookup on every call, so a compiled object runs about 2.2x faster per call (102 vs 235 ns on py3.14). The one-time compile (5 to 59 us) is paid once either way, and module globals sidestep re._cache's 512-entry eviction. Benched with timeit (best-of-5, 2M calls) comparing compiled.match(text) against a cache-warm re.match(pattern, text).
chrisjsewell
left a comment
chrisjsewell
left a comment
There was a problem hiding this comment.
Python already caches regex compilation, so it is not necessary to have any of these global variables
|
I know, but those are for the script in tests/test_redos.py to check them and compilation is on the µs scale compared to the overall overhead of Python + mdformat's architecture We could remove that general check file entirely if you don't want to have it fuzz test moving forward, but the alternative to use lazy compilation while also fuzzing is to walk the AST, which makes that script much more complicated TLDR: delete tests/test_redos.py and go back to lazy compilation? |
There was a problem hiding this comment.
Reviewed by checking the branch out and measuring the before/after behavior directly. The core fixes are correct and the suite is green (544 passed).
ReDoS fixes — confirmed real and resolved:
texmath```mathblock rules: the unpatched^{3}math\s+?([^]+?)\s+?{3}grows super-linearly on `` ```math `` + a whitespace run with no closing fence — I measured **0.14 s / 1.1 s / 7.5 s** at 400 / 800 / 1600 chars. The rewritten pattern stays flat (**<3 ms** at 200k chars). Same class of fix applied toadmon`.- The generic
tests/test_redos.pyguard discovers all 27 shipped patterns (including the ones nested in texmath'srulesdict) and would catch a reverted texmath fix — verified.
Precompile cleanups (amsmath RE_OPEN, anchors, dollarmath label normalizer, tasklists): behavior-preserving, verified parity on valid/invalid inputs. Good hygiene to bundle in.
Three notes (details inline):
tests/test_admon.py— the newtest_title_parsing_redosdoesn't reproduce the blow-up; its input passes on the unpatched regex, so it doesn't guard the fix. Easy to strengthen.texmath— the new capture trims surrounding whitespace / rejects whitespace-only content. Harmless, just flagging.- No
CHANGELOG.mdentry. Since this is the security-relevant fix (and #149 added one), it'd be good to record it here too, ideally noting which plugins/flavors were affected.
Nothing here blocks merging once the admon test is tightened. Nice work, and thanks for adding the categorical guard rather than just patching the two regexes.
|
Thanks @KyleKing — yeah, let's go back to lazy compilation. To be clear, it's not the perf: you're right that the compile cost is µs-scale and immaterial next to Python startup + everything downstream. My concern is structural — I don't want the library's module layout (a set of new top-level On
If either feels like too much for this PR, I'm also happy to drop |
|
@chrisjsewell, just following up, would you happen to have time to review? |
## What and why Two more HTML-injection points in the #143 family, both reachable with `html=False`: - **colon_fence** (default renderer): the block name was interpolated into `class="block-..."` unescaped, after entities in the info string had been unescaped. `:::a"onmouseover="alert(1)` rendered `class="block-a"onmouseover="alert(1)"`, and `:::a"><xss>` rendered a raw `<xss>` element. The name is now passed through `escapeHtml`. - **tasklists** with `label=True, label_after=True`: the label is built from the item's source text and emitted as an `html_inline` token, so `- [ ] <img src=x onerror=alert(1)>` rendered a raw `<img>`. The label text is now passed through `escapeHtml`. ## How it was tested - New regression tests, both with `html=False` and exact expected output: `tests/test_colon_fence.py::test_block_name_is_escaped` (raw-quote and entity-encoded payloads, plus a benign `&`) and `tests/test_tasklists.py::test_label_after_is_escaped` (raw HTML, `&`, plain text), with `test_label_after_entity_is_shown_literally` pinning how a source entity appears in the label (it asserts the label only, as earlier inline children of a `label_after` item are also rendered outside it, an existing quirk not changed here). - Every case except the plain-text one fails on master and passes with its fix; reverting either source file at HEAD makes its tests fail again. - `pytest -q`: 518 passed (511 on master plus the 7 new cases), markdown-it-py 4.2.0, Python 3.12. - `pre-commit run --all-files` (ruff, ruff-format, mypy): clean. - No fixture file changed; all existing colon_fence and tasklists fixtures render byte-identical. Part of #143 This does not close #143. What remains of it is in other open PRs: the texmath, dollarmath and myst_block escaping and the `attrs` default (#150), the ReDoS fixes (#148), and a test that guards every plugin renderer against XSS (#151). ## Notes for the reviewer - Output changes for benign input: a colon_fence block name containing `&` now emits `&` (`class="block-a&b"` instead of `class="block-a&b"`), which is the same attribute value; and in tasklists `label_after` mode, label text containing `&`, `<`, `>` or `"` is now escaped (`a & b` becomes `a & b`, which is correct HTML). The label is the item's raw source text (it already showed `*emphasis*` literally), so a source entity such as `&` now displays as the literal text `&`. This applies with `html=True` as well; on master, inline HTML in a `label_after` item already produced broken markup. No existing fixture uses `label_after`. - Adding these two plugins to `tests/test_xss.py` (which arrives with #151) is left for after those PRs land.
The ReDoS-safe pattern `\s+([^`\s]...)` also consumed the indentation of the body's first line, so "```math\n a = 1" rendered `<eqn>a = 1`. Match exactly one separator character and let the group start with `\s*[^`\s]`: `\s*` and `[^`\s]` are disjoint, so the pattern stays linear, and for every body that is not all whitespace it captures what the original pattern captured.
At the 15k-character run that `test_redos` uses, the old admon title pattern takes about 0.2 s, inside that test's 1 s budget, so reverting the admon fix left the suite green. `test_header_redos` renders runs of 25k and 100k characters and asserts the time grows less than 8x (about 4x when linear, about 16x when quadratic), so it does not depend on the speed of the machine.
test_header_redos stopped retrying at its first slow sample, so one scheduler or GC pause failed it (seen at 38x on a loaded machine). It now times the 25k and 100k runs in alternation with the GC paused, keeps the fastest of each, stops once the ratio holds, and stops retrying after 15s of renders so a quadratic regression fails on the assertion rather than the timeout. test_fenced_math_block_keeps_indent asserted on the rendered HTML, but every math block renders with the same template, so the eqno case could never pass. It now checks the token's type, content and info.
The title tail "(?P<title>.*)"\S*$ tried every quote in a run as the closing one, and scanned the rest of the run for each, so a header such as `!!! note "` followed by tens of thousands of quotes took seconds. Only the last quote can close the title, so "[^\s"]*$ matches exactly what it did, in linear time. test_header_redos now also times a quote run (10k and 40k), and retries up to 20 rounds rather than 5, so a heavily loaded machine does not fail it; the 15 s retry cap still bounds a quadratic parse.
The linear ```math patterns needed a non-whitespace character in the body, so a body that is only whitespace fell through to the code-fence rule. With an equation number the closer ``` (1) cannot close a code fence, so the fence then ran to the end of the document. The |\s branch captures the single whitespace character the previous pattern captured there, so the output is unchanged and matching stays linear.
Addressed: no module-level regex globals remain in the diff (the patterns are compiled where they were before).
Address confirm vulnerabilities from #143
Does not fully resolve the issue because there are unconfirmed/low risk vulnerabilities to consider and we don't know what was initially found that led to the ticket