From 14aa287dc135a90482a0bee451115bf3c889c14d Mon Sep 17 00:00:00 2001 From: Sean Whalen <44679+seanthegeek@users.noreply.github.com> Date: Sun, 18 Oct 2026 12:01:32 -0400 Subject: [PATCH] Fix ReDoS in failure-report, schema-tag, and timestamp regexes (11.0.4) (#921) Three regular expressions that run on attacker-controlled input backtracked super-linearly (CWE-1333): GHSA-7grx-mj6g-qxmc (affects 8.2.0 through 11.0.3, reported by mjatin2007): text_report_regex, used by the Exim/cPanel plain-text failure-report fallback, was \s*([a-zA-Z\s]+):\s(.+). The leading \s* and the \s inside the class overlap and the pattern is unanchored, so a whitespace run is retried from every offset and every split: cubic, about 8x per doubling (400 spaces 0.06 s, 1600 spaces 3.3 s on CPython 3.12). A ~100 KB message would stall the watch loop for days and be re-parsed after every restart. The pattern is now anchored to a line start with disjoint classes. GHSA-4h9r-c435-j834 (affects 4.1.7 through 11.0.3, found while auditing every regex in the package): xml_schema_regex was , whose greedy .*> rescans to end of line for every ]*>. This also fixes the greedy match deleting a minified single-line report from the schema tag to the last ">" on the line. human_timestamp_to_datetime() stripped RFC 5322 comments with \s*\(.*\)\s*, quadratic on whitespace (200,000 spaces 18.6 s) and reached through every Date header; filed as a bug fix because MTAs cap header size. Comments are now removed by a depth-counting scan, which is linear and handles the nested comments and quoted-pairs RFC 5322 section 3.2.2 allows (a character-class regex would have regressed them). Adds regression tests sized so the old patterns fail the time bound in minutes rather than hanging CI, a "Regular expressions on untrusted input" section in AGENTS.md, and the 11.0.4 changelog and version bump. Co-authored-by: Claude Fable 5.1 --- AGENTS.md | 11 +++++ CHANGELOG.md | 8 +++- parsedmarc/__init__.py | 13 +++++- parsedmarc/constants.py | 2 +- parsedmarc/utils.py | 33 ++++++++++++-- tests/test_init.py | 95 +++++++++++++++++++++++++++++++++++++++++ tests/test_utils.py | 46 ++++++++++++++++++++ 7 files changed, 200 insertions(+), 8 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9e38ecbe..e6675c47 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -124,6 +124,17 @@ IP address info cached for 4 hours, seen aggregate report IDs cached for 1 hour - Token file writes must create parent directories before opening for write. - Store natively numeric values as numbers, not pre-formatted strings. Example: ASN is stored as `int 15169`, not `"AS15169"`; Elasticsearch / OpenSearch mappings for such fields use `Integer()` so consumers can do range queries and numeric sorts. Display layers format with a prefix at render time. +### Regular expressions on untrusted input + +Assume every regex in the package runs on content a remote sender controls (message bodies, headers, attachments inflated to the 100 MiB decompression cap), so a pattern that backtracks super-linearly is a remote CPU-exhaustion bug, not a style nit (GHSA-7grx-mj6g-qxmc: a cubic pattern on whitespace; GHSA-4h9r-c435-j834: a quadratic `.*>` that a 194 KB gzip attachment could turn into days of CPU). Before adding or editing a pattern: + +- **Adjacent quantifiers must not overlap.** `\s*([a-zA-Z\s]+)` lets both parts consume the same whitespace, so every split is retried. Make neighbouring classes disjoint (`[ \t]*` next to `[A-Za-z]`). +- **A repeated class must exclude the delimiter it stops on.** Write `\([^()]*\)` and `]*>`, never `\(.*\)` or ``: a greedy `.*` scans to end of line and backtracks for every occurrence, which is quadratic whenever the closer is missing, and also matches past the *first* closer (the old schema-tag regex deleted a whole minified report that way). +- **Anchor when the match can only start at a line start** (`^` with `re.MULTILINE`); an unanchored `findall` retries the whole pattern from every offset. +- **Prefer `str` methods** (`split`, `partition`, `startswith`) when the format is fixed and a regex adds nothing, and a plain loop when the grammar nests: balanced parentheses cannot be matched by a regex at all, which is why `human_timestamp_to_datetime()` strips RFC 5322 comments with a depth-counting scan. +- **Time the pattern on an adversarial input before shipping it**, not just on the samples: a 1 MB string of the repeated opener with no closer, and a 200,000-character whitespace run. A linear pattern finishes in milliseconds; anything that takes seconds is a bug. Add that input as a time-bound regression test with a generous bound (seconds) and cite the measured old timing in the docstring, as the regression tests in `tests/test_init.py` and `tests/test_utils.py` do. +- Python 3.10 is the floor, so possessive quantifiers and atomic groups (3.11+) are not available; fix the pattern's structure instead. + ## Testing standards These rules govern *every* test added to `tests/`. They exist because the project has been burned by tests that looked like coverage but caught nothing, and by bug claims that turned out to be wrong about the spec. Both failure modes erode trust faster than missing coverage does. diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ef46a88..59d2145f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,9 +1,15 @@ # Changelog -## Unreleased +## 11.0.4 + +### Security + +- **Fixed catastrophic regex backtracking in the plain-text failure report fallback** ([GHSA-7grx-mj6g-qxmc](https://github.com/domainaware/parsedmarc/security/advisories/GHSA-7grx-mj6g-qxmc), affects 8.2.0 through 11.0.3). A `text/plain` message containing `A message claiming to be from you has failed` is parsed as an Exim/cPanel failure notice by `text_report_regex`, `\s*([a-zA-Z\s]+):\s(.+)`. Its leading `\s*` and the `\s` inside the field-name class can each consume the same whitespace, and the pattern is unanchored, so a run of whitespace is retried from every starting position and every way of splitting it: the cost grows cubically, about 8x per doubling — on CPython 3.12 (x86-64), 400 spaces took 0.06 s and 1600 spaces took 3.3 s. Anyone who can send mail to a monitored mailbox controls the body, so a message of about 100 KB would stall the mailbox watch loop for days, and since the message is never marked processed it would be re-parsed after every restart. The pattern is now anchored to the start of a line and its classes no longer overlap, so it runs in linear time. The pattern now requires a field name to start a line, with its words separated by single spaces, which is what Exim emits; notices with a tab, doubled space, or line break inside a field name were already rejected, because such names never produced the keys the parser looks up. Reported by mjatin2007. +- **Fixed quadratic regex backtracking when stripping XML schema tags from aggregate reports** ([GHSA-4h9r-c435-j834](https://github.com/domainaware/parsedmarc/security/advisories/GHSA-4h9r-c435-j834), affects 4.1.7 through 11.0.3). `parse_aggregate_report_xml()` removed `` tags with ``, whose unanchored greedy `.*>` scanned to the end of the line for every `` (`]*>`) and runs in linear time. This also fixes a correctness bug: on a minified report with the whole document on one line, the greedy `.*>` deleted everything from the schema tag to the last `>` on the line, discarding the report. ### Bug fixes +- **Parsing a timestamp no longer takes quadratic time on whitespace.** `human_timestamp_to_datetime()` stripped parenthesized comments with the regex `\s*\(.*\)\s*`, whose leading `\s*` retried every whitespace run from each starting position. The function runs on every message's `Date` header, so a long folded header was enough: 200,000 spaces took 18.6 s. This is the same weakness as the two entries above, but a lesser one: the input is a header that mail servers cap in size (Postfix's default is 100 KB, about 5 s) rather than an attachment inflated to the 100 MiB decompression cap, so it is filed as a bug fix without an advisory. Comments are now removed by a single depth-counting pass, which is linear and also handles the nested comments and quoted-pairs that RFC 5322 section 3.2.2 allows and that the old regex only stripped by accident. - **The Grafana dashboard's "Message sources by name and type" table no longer comes up empty when a message source has a name but no type.** Whenever a source's reverse DNS base domain is not in `base_reverse_dns_map.csv`, `get_service_from_reverse_dns_base_domain()` names the source after that base domain and leaves its type `None`; a source with no PTR record whose AS domain is not in the map is likewise named after its AS name with no type. For every such source the saved aggregate record has a `source_name` but no `source_type`. The panel nests a `terms` aggregation on `source_type.keyword` under one on `source_name.keyword`, without a `missing` value, so such a source produced a `source_name` bucket with no `source_type` sub-bucket, and Grafana's Elasticsearch data source then failed to build the table from the response. With Grafana 13.2.4 (bundled Elasticsearch data source 12.9.2) and the reports in `samples/aggregate/`, 12 sources had a name but no type and the table showed "No data"; with `missing` set, it lists all 27 name/type rows. The `source_type.keyword` aggregation now sets `missing` to `-`, the placeholder most of the dashboard's other tables already use for absent values, so those sources are listed with `-` as their type. ## 11.0.3 diff --git a/parsedmarc/__init__.py b/parsedmarc/__init__.py index 2e710c32..cfba4be9 100644 --- a/parsedmarc/__init__.py +++ b/parsedmarc/__init__.py @@ -88,8 +88,17 @@ logger.debug(f"parsedmarc v{__version__}") # than relying on ``.`` — otherwise every field value keeps a trailing CR. feedback_report_regex = re.compile(r"^([\w\-]+): ([^\r\n]+)\r?$", re.MULTILINE) xml_header_regex = re.compile(r"^<\?xml .*?>", re.MULTILINE) -xml_schema_regex = re.compile(r"", re.MULTILINE) -text_report_regex = re.compile(r"\s*([a-zA-Z\s]+):\s(.+)", re.MULTILINE) +# Every pattern here runs on attacker-controlled input; the next two were +# rewritten to match in linear time (GHSA-4h9r-c435-j834, GHSA-7grx-mj6g-qxmc): +# a repeated character class excludes the delimiter it stops on, adjacent +# quantifiers never overlap, and text_report_regex is anchored to a line start, +# so each candidate match is scanned once instead of being rescanned from every +# offset on every backtrack. See "Regular expressions on untrusted input" in +# AGENTS.md. +xml_schema_regex = re.compile(r"]*>") +text_report_regex = re.compile( + r"^[ \t]*([A-Za-z]+(?: [A-Za-z]+)*):[ \t]+([^\r\n]+)", re.MULTILINE +) # Captures the value of any xmlns (default or prefixed) declaration so the # RFC 9990 namespace can be detected before xmltodict drops it. xml_namespace_regex = re.compile( diff --git a/parsedmarc/constants.py b/parsedmarc/constants.py index 581d74b2..587e636b 100644 --- a/parsedmarc/constants.py +++ b/parsedmarc/constants.py @@ -1,4 +1,4 @@ -__version__ = "11.0.3" +__version__ = "11.0.4" USER_AGENT = f"parsedmarc/{__version__}" diff --git a/parsedmarc/utils.py b/parsedmarc/utils.py index 6303045a..b061c811 100644 --- a/parsedmarc/utils.py +++ b/parsedmarc/utils.py @@ -12,7 +12,6 @@ import json import logging import mailbox import os -import re import shutil import subprocess import tempfile @@ -61,8 +60,6 @@ _RETRYABLE_DNS_ERRORS = ( _DOH_SESSION: httpx.Client | None = None _DOH_SESSION_PID: int | None = None -parenthesis_regex = re.compile(r"\s*\(.*\)\s*") - null_file = subprocess.DEVNULL mailparser_logger = logging.getLogger("mailparser") mailparser_logger.setLevel(logging.CRITICAL) @@ -536,6 +533,34 @@ def timestamp_to_human(timestamp: int) -> str: return timestamp_to_datetime(timestamp).strftime("%Y-%m-%d %H:%M:%S") +def _strip_comments(value: str) -> str: + """Removes RFC 5322 comments from a header value. + + A comment is parenthesized text that may nest and may contain quoted-pairs + such as ``\\)`` (RFC 5322 section 3.2.2), so it cannot be matched by a + regular expression at all. This single pass tracks the nesting depth + instead; an unclosed comment runs to the end of the value. + """ + out: list[str] = [] + depth = 0 + escaped = False + for char in value: + if depth: + if escaped: + escaped = False + elif char == "\\": + escaped = True + elif char == "(": + depth += 1 + elif char == ")": + depth -= 1 + elif char == "(": + depth = 1 + else: + out.append(char) + return "".join(out) + + def human_timestamp_to_datetime( human_timestamp: str, *, to_utc: bool = False, assume_utc: bool = False ) -> datetime: @@ -556,7 +581,7 @@ def human_timestamp_to_datetime( """ human_timestamp = human_timestamp.replace("-0000", "") - human_timestamp = parenthesis_regex.sub("", human_timestamp) + human_timestamp = _strip_comments(human_timestamp).strip() dt = parse_date(human_timestamp) if assume_utc and dt.tzinfo is None: diff --git a/tests/test_init.py b/tests/test_init.py index c63bb161..8b20d800 100644 --- a/tests/test_init.py +++ b/tests/test_init.py @@ -16,6 +16,7 @@ import logging import mailbox import os import quopri +import time import unittest from collections.abc import Callable from datetime import datetime, timedelta, timezone @@ -2625,6 +2626,53 @@ class TestMalformedXmlRecovery(unittest.TestCase): self.assertIn("missing required section", str(ctx.exception)) self.assertIsInstance(ctx.exception.__cause__, AttributeError) + def testXmlSchemaRegexRejectsRepeatedTagsQuickly(self): + """A report made of repeated ````, whose unanchored greedy ``.*>`` rescanned to the + end of the line for every ```` tag. + + The old greedy ```` (GHSA-4h9r-c435-j834) deleted + from the schema tag to the last ``>`` on the line, which on a + single-line document discarded the whole report.""" + path = "samples/aggregate/ikea.com!example.de!1538690400!1538776800.xml" + with open(path, encoding="utf-8") as f: + original = f.read() + minified = "".join(line.strip() for line in original.splitlines()) + self.assertIn("