From 6b7c203126dc242dfe112b9b3b53155113c3d14a Mon Sep 17 00:00:00 2001 From: Sean Whalen <44679+seanthegeek@users.noreply.github.com> Date: Mon, 26 Oct 2026 22:05:36 -0400 Subject: [PATCH] Merge commit from fork * Fix three security advisories (DOCTYPE amplification, poison mailbox message, byte-length filenames) GHSA-m47j-wp78-fqjg: reject any DOCTYPE in aggregate report XML. An internal subset's default attributes are applied by expat to every matching element, so 64 KB of XML (K=1000, N=10000) took 4.0 s and 1.4 GB here. xmltodict now gets an expat shim whose parsers raise on StartDoctypeDecl; the lxml recovery path checks docinfo and serializes only the root element. GHSA-qxwv-2vwj-v828: parse_report_email() now raises only ParserError subclasses (HeaderWriteError / RecursionError from re-serializing a part used to escape). Content errors raised while fetching a message (email.errors.MessageError, RecursionError) are returned as ParserError and the message is disposed of as invalid; transport errors still propagate. A failed invalid-message move/delete is logged instead of aborting the batch, because mailsuite's Maildir move_message() re-serializes the message too. mbox reads use get_bytes() instead of the re-serializing get_string(). GHSA-j53g-34vj-g25q: get_filename_safe_string() bounds names to 100 UTF-8 bytes as well as 100 characters; filesystems limit a name to 255 bytes, so 84 CJK characters + ".eml" failed with ENAMETOOLONG. Reported by helixnebulatech. Co-Authored-By: Claude Opus 5.5 * Address review: recursion margins, mbox wording, DOCTYPE check order - Nest the RecursionError poison 400 levels deep (POISON_NESTING_LEVELS) instead of 250: the smallest failing depth is ~247 from a shallow stack, 245 in a fork worker and ~222 at +100 frames, so 250 left a margin of a few levels. The shallow case uses 50 levels and now asserts InvalidDMARCReport ("is not a valid report"); the deep case asserts "recursion" in the message. - Poison-loop tests skip a benign poison per message (continue) and assert at least one poison was exercised, so they cannot pass vacuously. - lxml recovery: check for a DOCTYPE before serializing the root, and comment why the str() around tostring(encoding="unicode") stays (pyright, no lxml stubs). - CHANGELOG: describe what the walk actually serializes, drop the depth-dependent 245 figure, and say plainly that an mbox run still stops at such a message, now with a ParserError naming it. Co-Authored-By: Claude Opus 5.5 * Make the Maildir poison-message test interpreter-independent Whether MaildirConnection.move_message() can store the control-character message depends on the interpreter: upstream CPython (3.12.9) runs the CVE-2024-6923 generated-header check only in the text Generator used by as_string(), so mailbox.Maildir.add()'s BytesGenerator stores it, while Ubuntu's patched 3.12.3 also checks in BytesGenerator._write_headers and the move fails. The deeply nested message fails the move everywhere. The test now decides per message, by trying the same Maildir round trip, whether the message lands in Archive/Invalid with no mailbox error logged or stays in INBOX with the move error logged; both branches assert the batch completed with the aggregate archived. CHANGELOG and the _dispose_invalid_message docstring now say which messages stay put. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 6 + parsedmarc/__init__.py | 349 +++++++++++++++-------- parsedmarc/parallel.py | 5 +- parsedmarc/utils.py | 27 +- tests/test_init.py | 617 ++++++++++++++++++++++++++++++++++++++++- tests/test_utils.py | 43 +++ 6 files changed, 923 insertions(+), 124 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4299c44c..38aa7280 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ ## Unreleased +### Security + +- **Aggregate reports containing a DOCTYPE declaration are now rejected, which fixes XML default-attribute amplification** ([GHSA-m47j-wp78-fqjg](https://github.com/domainaware/parsedmarc/security/advisories/GHSA-m47j-wp78-fqjg), affects 11.0.5 and earlier). Entity expansion was already disabled (xmltodict's `disable_entities=True`), but an internal DTD subset can also declare default attributes: `` followed by 10,000 `` elements makes expat report 1,000 attributes on each element, and xmltodict builds 10 million dict entries from 64 KB of XML. Locally that took 4.0 s and 1.4 GB of memory per report; the reporter measured 29 s and 1.4 GB from a 3.8 KB gzip email, and anyone who can send mail to a monitored mailbox can send it. Real aggregate reports never carry a DOCTYPE, so `parse_aggregate_report_xml()` now aborts with `InvalidAggregateReport` as soon as expat reaches one, before it reads the subset; the same document is rejected in under a millisecond. The lxml recovery path for malformed XML, which a document reaches when its first parse fails before the DOCTYPE (for example on a malformed XML declaration), applies the same check and now serializes only the root element, so no internal subset reaches the second parse. Reported by helixnebulatech. +- **A report email that cannot be re-serialized no longer aborts the whole mailbox batch** ([GHSA-qxwv-2vwj-v828](https://github.com/domainaware/parsedmarc/security/advisories/GHSA-qxwv-2vwj-v828), affects 11.0.5 and earlier). While walking the message, `parse_report_email()` serializes the first child of each part, which re-renders any `message/rfc822` sub-message, and the Maildir backend re-serializes each message while fetching it, so a `message/rfc822` part with a `\x1c` control character in a header (`email.errors.HeaderWriteError`) or multiparts nested about 250 levels deep (`RecursionError`; a shallower nesting is rejected as not a report) raised an exception that the mailbox loop's per-message `except ParserError` did not catch. The batch aborted before the message could be moved to the `Invalid` folder, so every later run, and every watch-mode check, aborted on the same message and no report behind it was ever processed. `parse_report_email()` now raises only `ParserError` subclasses for any input. A content error raised while fetching a message (`email.errors.MessageError` or `RecursionError`) is treated like a parse failure, so the message is moved to the `Invalid` folder (or deleted with `delete_invalid`); transport errors still propagate, so a dropped connection cannot get a legitimate report filed as invalid. A move or delete of an invalid message that the backend rejects is now logged and skipped, as it already was for archiving parsed reports, instead of aborting the batch: the Maildir backend moves a message by storing it again through the binary generator, which fails for a message nested that deeply on every Python and, on interpreters whose binary generator also verifies generated headers (e.g. some distribution builds; upstream CPython verifies them only when serializing to text), for the control-character one too. Such a message stays in the inbox, logged on each run, until it is removed by hand or `delete_invalid` is set. `get_dmarc_reports_from_mbox()` now reads each message's stored bytes instead of a re-serialized string, so these messages reach the parser and fail with a `ParserError` naming the message rather than an unrelated exception; an mbox run still stops at such a message, as it does at any message that raises a `ParserError` other than `InvalidDMARCReport`. Reported by helixnebulatech. +- **Failure report sample filenames are now limited by UTF-8 bytes as well as characters** ([GHSA-j53g-34vj-g25q](https://github.com/domainaware/parsedmarc/security/advisories/GHSA-j53g-34vj-g25q), affects 11.0.5 and earlier). `get_filename_safe_string()` truncated the sample's `Subject` to 100 characters, but filesystems limit a name to 255 bytes (eCryptfs: 143), so a subject of 84 CJK characters or 63 emoji plus `.eml` made a 256-byte name. `save_output()` then failed with `ENAMETOOLONG` after the batch's JSON and CSV rows had already been appended, so a one-shot run retried the batch indefinitely and watch mode moved it, legitimate reports included, to the `Unsaved` folder, duplicating rows on each attempt; `email_results()` failed the same way. Names are now cut to at most 100 UTF-8 bytes without splitting a character, which leaves room for a ` (N)` suffix and the extension under every common limit; ASCII names are unchanged. Reported by helixnebulatech. + ### Changes - **CSV output now neutralizes spreadsheet formulas** (CWE-1236, [OWASP CSV Injection](https://owasp.org/www-community/attacks/CSV_Injection)). Every text field in the aggregate, failure, and SMTP TLS CSV output comes from a report that a third party sent, and a cell beginning with `=`, `+`, `-`, or `@` is evaluated as a formula by Excel, LibreOffice, and Google Sheets, so a reporter could set an `org_name` such as `=HYPERLINK("http://evil","x")` and have it run when an analyst opens the CSV. `_csv_safe()` now prefixes a string field that starts with `=`, `+`, `-`, `@`, a tab, or a carriage return with a single `'`, the OWASP-recommended fix, after the existing NUL strip; non-string values such as ASNs and counts are untouched. Compatibility: only cells starting with one of those six characters change, and real report fields almost never do. The JSON output and the data sent to the other output integrations are unchanged. (Closes #929) diff --git a/parsedmarc/__init__.py b/parsedmarc/__init__.py index 11f966ed..bd60bfaf 100644 --- a/parsedmarc/__init__.py +++ b/parsedmarc/__init__.py @@ -200,6 +200,42 @@ class InvalidFailureReport(InvalidDMARCReport): InvalidForensicReport = InvalidFailureReport +_DOCTYPE_ERROR = "Aggregate reports must not contain a DOCTYPE declaration" + + +class _XMLDoctypeFound(Exception): + """Raised by :class:`_NoDoctypeExpat` parsers as soon as expat reports + the start of a `` None: + raise _XMLDoctypeFound(_DOCTYPE_ERROR) + + +class _NoDoctypeExpat: + """A minimal stand-in for the ``xml.parsers.expat`` module, passed to + ``xmltodict.parse(expat=...)``, whose parsers abort on any DOCTYPE. + + An internal DTD subset can declare default attributes + (````) that expat then reports on + every matching element, so K declared defaults on N empty elements make + xmltodict build K x N dict entries from a few kilobytes of input + (GHSA-m47j-wp78-fqjg). Entity expansion is already off + (xmltodict's ``disable_entities=True``) but does not cover this. + DMARC aggregate reports never carry a DOCTYPE, so the declaration is + rejected outright, before expat reads any of the subset. + + ``xmltodict.parse()`` only ever calls ``ParserCreate()`` on the module it + is given. + """ + + @staticmethod + def ParserCreate(*args: Any, **kwargs: Any) -> Any: + parser = expat.ParserCreate(*args, **kwargs) + parser.StartDoctypeDeclHandler = _raise_doctype_found + return parser + + def _resolve_config( config: ParserConfig | None, *, @@ -1002,18 +1038,41 @@ def parse_aggregate_report_xml( xml_namespace = namespace_match.group(1) try: - xmltodict.parse(xml)["feedback"] + xmltodict.parse(xml, expat=_NoDoctypeExpat)["feedback"] + except _XMLDoctypeFound as e: + raise InvalidAggregateReport(_DOCTYPE_ERROR) from e except Exception as e: errors.append(f"Invalid XML: {e.__str__()}") + has_doctype = False try: tree = etree.parse( BytesIO(xml.encode("utf-8")), etree.XMLParser(recover=True, resolve_entities=False), ) - s = etree.tostring(tree) - xml = "" if s is None else s.decode("utf-8") + root = tree.getroot() + if root is None: + # Nothing recoverable; the second parse reports it as + # invalid XML. + xml = "" + else: + # The first parse can fail before expat reaches a DOCTYPE + # (e.g. on a malformed XML declaration, which is only + # rewritten below), so the recovered tree gets the same + # DOCTYPE check, made before serializing. The rejection is + # raised after this try, which would otherwise swallow it. + has_doctype = bool(tree.docinfo.doctype) or ( + tree.docinfo.internalDTD is not None + ) + if not has_doctype: + # Serializing the root element rather than the tree + # leaves any internal DTD subset out of the XML handed to + # the second parse. str() is a no-op at runtime; lxml ships + # no type stubs, so pyright would widen xml to str | bytes. + xml = str(etree.tostring(root, encoding="unicode")) except Exception: xml = "" + if has_doctype: + raise InvalidAggregateReport(_DOCTYPE_ERROR) from e try: # Replace XML header (sometimes they are invalid) @@ -1022,7 +1081,7 @@ def parse_aggregate_report_xml( # Remove invalid schema tags xml = xml_schema_regex.sub("", xml) - report = xmltodict.parse(xml)["feedback"] + report = xmltodict.parse(xml, expat=_NoDoctypeExpat)["feedback"] report_metadata = report["report_metadata"] # is xs:string in both RFC 7489 and RFC 9990, but defensive # parsing in the wild: some reporters emit it with an xml:lang or @@ -2125,6 +2184,10 @@ def parse_report_email( dict: * ``report_type``: ``aggregate``, ``failure``, or ``smtp_tls`` * ``report``: The parsed report + + Raises: + ParserError: If the message is not a valid report. Whatever the + input, only ``ParserError`` subclasses are raised. """ cfg = _resolve_config( config, @@ -2183,97 +2246,113 @@ def parse_report_email( # RFC 2047 decoding can yield raw CR/LF, so escape control # characters before the subject reaches an error message or log. subject_text = sanitize_for_log(str(msg_headers["Subject"])) - for part in msg.walk(): - content_type = part.get_content_type().lower() - payload_obj = part.get_payload() - if not isinstance(payload_obj, list): - payload_obj = [payload_obj] - payload = str(payload_obj[0]) - if content_type.startswith("multipart/"): - continue - if content_type == "text/html": - continue - elif content_type == "message/feedback-report": - is_feedback_report = True - decoded_payload = _decode_mime_payload(part, payload) - try: - if "Feedback-Type" in decoded_payload: - feedback_report = decoded_payload - else: - feedback_report = b64decode(decoded_payload).__str__() - feedback_report = feedback_report.lstrip("b'").rstrip("'") - feedback_report = feedback_report.replace("\\r", "") - feedback_report = feedback_report.replace("\\n", "\n") - except (ValueError, TypeError, binascii.Error): - feedback_report = decoded_payload - elif is_feedback_report and content_type in EMAIL_SAMPLE_CONTENT_TYPES: - sample = _decode_mime_payload(part, payload) - elif content_type == "application/tlsrpt+json": - if not payload.strip().startswith("{"): - payload = b64decode(payload).decode("utf-8", errors="replace") - smtp_tls_report = parse_smtp_tls_report_json(payload) - return {"report_type": "smtp_tls", "report": smtp_tls_report} - elif content_type == "application/tlsrpt+gzip": - payload = extract_report(payload) - smtp_tls_report = parse_smtp_tls_report_json(payload) - return {"report_type": "smtp_tls", "report": smtp_tls_report} - elif content_type == "text/plain": - if "A message claiming to be from you has failed" in payload: + # Serializing a part (``str(payload_obj[0])``) re-renders any + # ``message/rfc822`` sub-message, which can raise e.g. + # ``email.errors.HeaderWriteError`` on a control character in a header or + # ``RecursionError`` on deeply nested multiparts. Callers rely on this + # function raising only ``ParserError`` subclasses so that one bad message + # is skipped (and moved to ``Invalid``) rather than aborting a whole + # mailbox batch (GHSA-qxwv-2vwj-v828). + try: + for part in msg.walk(): + content_type = part.get_content_type().lower() + payload_obj = part.get_payload() + if not isinstance(payload_obj, list): + payload_obj = [payload_obj] + payload = str(payload_obj[0]) + if content_type.startswith("multipart/"): + continue + if content_type == "text/html": + continue + elif content_type == "message/feedback-report": + is_feedback_report = True + decoded_payload = _decode_mime_payload(part, payload) try: - parts = payload.split("detected.", 1) - field_matches = text_report_regex.findall(parts[0]) - fields = dict() - for match in field_matches: - field_name = match[0].lower().replace(" ", "-") - fields[field_name] = match[1].strip() + if "Feedback-Type" in decoded_payload: + feedback_report = decoded_payload + else: + feedback_report = b64decode(decoded_payload).__str__() + feedback_report = feedback_report.lstrip("b'").rstrip("'") + feedback_report = feedback_report.replace("\\r", "") + feedback_report = feedback_report.replace("\\n", "\n") + except (ValueError, TypeError, binascii.Error): + feedback_report = decoded_payload + elif is_feedback_report and content_type in EMAIL_SAMPLE_CONTENT_TYPES: + sample = _decode_mime_payload(part, payload) + elif content_type == "application/tlsrpt+json": + if not payload.strip().startswith("{"): + payload = b64decode(payload).decode("utf-8", errors="replace") + smtp_tls_report = parse_smtp_tls_report_json(payload) + return {"report_type": "smtp_tls", "report": smtp_tls_report} + elif content_type == "application/tlsrpt+gzip": + payload = extract_report(payload) + smtp_tls_report = parse_smtp_tls_report_json(payload) + return {"report_type": "smtp_tls", "report": smtp_tls_report} + elif content_type == "text/plain": + if "A message claiming to be from you has failed" in payload: + try: + parts = payload.split("detected.", 1) + field_matches = text_report_regex.findall(parts[0]) + fields = dict() + for match in field_matches: + field_name = match[0].lower().replace(" ", "-") + fields[field_name] = match[1].strip() + + feedback_report = "Arrival-Date: {}\nSource-IP: {}".format( + fields["received-date"], fields["sender-ip-address"] + ) + except Exception as e: + error = f'Unable to parse message with subject "{subject_text}": {e}{_exc_origin(e)}' + raise InvalidDMARCReport(error) from e + + sample = parts[1].lstrip() + logger.debug(sample) + else: + try: + payload_bytes = b64decode(payload) + if payload_bytes.startswith(MAGIC_ZIP) or payload_bytes.startswith( + MAGIC_GZIP + ): + payload_text = extract_report(payload_bytes) + else: + payload_text = payload_bytes.decode("utf-8", errors="replace") + + if payload_text.strip().startswith("{"): + smtp_tls_report = parse_smtp_tls_report_json(payload_text) + result = {"report_type": "smtp_tls", "report": smtp_tls_report} + return result + elif payload_text.strip().startswith("<"): + aggregate_report = parse_aggregate_report_xml( + payload_text, + config=cfg, + keep_alive=keep_alive, + ) + result = { + "report_type": "aggregate", + "report": aggregate_report, + } + + return result + + except (TypeError, ValueError, binascii.Error): + # b64decode() rejected this MIME part's payload as not + # base64-decodable, so it isn't a report attachment; fall + # through and keep walking the message for a part (or the + # feedback-report/sample pair below) that is. + pass + + except InvalidDMARCReport as e: + error = f'Message with subject "{subject_text}" is not a valid DMARC report: {e}' + raise ParserError(error) from e - feedback_report = "Arrival-Date: {}\nSource-IP: {}".format( - fields["received-date"], fields["sender-ip-address"] - ) except Exception as e: error = f'Unable to parse message with subject "{subject_text}": {e}{_exc_origin(e)}' - raise InvalidDMARCReport(error) from e - - sample = parts[1].lstrip() - logger.debug(sample) - else: - try: - payload_bytes = b64decode(payload) - if payload_bytes.startswith(MAGIC_ZIP) or payload_bytes.startswith( - MAGIC_GZIP - ): - payload_text = extract_report(payload_bytes) - else: - payload_text = payload_bytes.decode("utf-8", errors="replace") - - if payload_text.strip().startswith("{"): - smtp_tls_report = parse_smtp_tls_report_json(payload_text) - result = {"report_type": "smtp_tls", "report": smtp_tls_report} - return result - elif payload_text.strip().startswith("<"): - aggregate_report = parse_aggregate_report_xml( - payload_text, - config=cfg, - keep_alive=keep_alive, - ) - result = {"report_type": "aggregate", "report": aggregate_report} - - return result - - except (TypeError, ValueError, binascii.Error): - # b64decode() rejected this MIME part's payload as not - # base64-decodable, so it isn't a report attachment; fall - # through and keep walking the message for a part (or the - # feedback-report/sample pair below) that is. - pass - - except InvalidDMARCReport as e: - error = f'Message with subject "{subject_text}" is not a valid DMARC report: {e}' - raise ParserError(error) from e - - except Exception as e: - error = f'Unable to parse message with subject "{subject_text}": {e}{_exc_origin(e)}' - raise ParserError(error) from e + raise ParserError(error) from e + except ParserError: + raise + except Exception as e: + error = f'Unable to parse message with subject "{subject_text}": {e}{_exc_origin(e)}' + raise ParserError(error) from e if feedback_report and sample: try: @@ -2536,7 +2615,7 @@ def _classify_parsed_email( def _fetch_mailbox_message( connection: MailboxConnection, msg_uid: Any, test: bool -) -> tuple[int | str, str]: +) -> tuple[int | str, str | ParserError]: """Fetch one message from ``connection`` by UID, casting the UID to the type each backend's ``fetch_message`` expects. @@ -2545,20 +2624,37 @@ def _fetch_mailbox_message( Returns ``(message_id, msg_content)``; ``message_id`` is the backend-appropriate id to use for later move/delete calls. + + Some backends re-serialize the stored message while fetching it (the + Maildir backend calls ``Message.as_string()``), so the message's own + content can make the fetch fail: ``email.errors.HeaderWriteError`` on a + control character in a header, ``RecursionError`` on deeply nested + multiparts. Those content errors are returned, not raised, as + ``msg_content`` -- a ``ParserError`` the caller treats like a parse + failure, disposing of the message as invalid instead of aborting the + batch on every run (GHSA-qxwv-2vwj-v828). + + The catch is deliberately narrow (``email.errors.MessageError`` and + ``RecursionError``, not ``Exception``): transport and service errors + (``OSError``, IMAP and Microsoft Graph errors, ``httpx`` errors) say + nothing about the message and still propagate, because classifying a + transient network failure as invalid content would move a legitimate + report to the invalid folder. """ message_id: int | str if isinstance(connection, IMAPConnection): message_id = int(msg_uid) - msg_content = connection.fetch_message(message_id) - elif isinstance(connection, MSGraphConnection): - message_id = str(msg_uid) - msg_content = connection.fetch_message(message_id, mark_read=not test) - elif isinstance(connection, MaildirConnection): - message_id = str(msg_uid) if not isinstance(msg_uid, str) else msg_uid - msg_content = connection.fetch_message(message_id, mark_read=not test) else: message_id = str(msg_uid) if not isinstance(msg_uid, str) else msg_uid - msg_content = connection.fetch_message(message_id) + try: + if isinstance(connection, (MSGraphConnection, MaildirConnection)): + msg_content = connection.fetch_message(message_id, mark_read=not test) + else: + msg_content = connection.fetch_message(message_id) + except (email.errors.MessageError, RecursionError) as e: + return message_id, ParserError( + f"Unable to fetch message UID {message_id}: {type(e).__name__}: {e}" + ) return message_id, msg_content @@ -2575,19 +2671,38 @@ def _dispose_invalid_message( Shared, unmodified, by the sequential and parallel branches of ``get_dmarc_reports_from_mailbox`` so both dispose of invalid messages identically. + + A delete or move the backend rejects is logged and the message is left + in place, as the archival disposal of parsed reports does, rather than + aborting the rest of the batch. Besides transient backend errors, this + covers messages the backend cannot rewrite: the Maildir backend moves a + message by storing it again with ``mailbox.Maildir.add()``, which + re-serializes it through ``email.generator.BytesGenerator``. That raises + ``RecursionError`` for deeply nested multiparts on every Python, and + ``email.errors.HeaderWriteError`` for a control character in a header + only where ``BytesGenerator`` also verifies generated headers (some + distribution builds; upstream CPython checks only the text generator + used by ``as_string()``) (GHSA-qxwv-2vwj-v828). """ - if delete: - logger.debug(f"Deleting message UID {message_id}") - if isinstance(connection, IMAPConnection): - connection.delete_message(int(message_id)) + try: + if delete: + logger.debug(f"Deleting message UID {message_id}") + if isinstance(connection, IMAPConnection): + connection.delete_message(int(message_id)) + else: + connection.delete_message(str(message_id)) else: - connection.delete_message(str(message_id)) - else: - logger.debug(f"Moving message UID {message_id} to {invalid_reports_folder}") - if isinstance(connection, IMAPConnection): - connection.move_message(int(message_id), invalid_reports_folder) - else: - connection.move_message(str(message_id), invalid_reports_folder) + logger.debug(f"Moving message UID {message_id} to {invalid_reports_folder}") + if isinstance(connection, IMAPConnection): + connection.move_message(int(message_id), invalid_reports_folder) + else: + connection.move_message(str(message_id), invalid_reports_folder) + except Exception as e: + action = "deleting" if delete else "moving" + logger.error( + f"Mailbox error: Error {action} invalid message UID {message_id}: " + f"{type(e).__name__}: {e}" + ) def get_dmarc_reports_from_mbox( @@ -2668,7 +2783,7 @@ def get_dmarc_reports_from_mbox( for i in range(total_messages): message_key = message_keys[i] logger.info(f"Processing message {i + 1} of {total_messages}") - yield mbox.get_string(message_key) + yield mbox.get_bytes(message_key) for result in tqdm( parallel_map(func, _jobs(), n_procs), @@ -2691,7 +2806,7 @@ def get_dmarc_reports_from_mbox( for i in tqdm(range(total_messages), disable=None): message_key = message_keys[i] logger.info(f"Processing message {i + 1} of {total_messages}") - msg_content = mbox.get_string(message_key) + msg_content = mbox.get_bytes(message_key) try: parsed_email = parse_report_email(msg_content, config=cfg) _classify_parsed_email( @@ -3074,6 +3189,12 @@ def get_dmarc_reports_from_mailbox( message_id, msg_content = _fetch_mailbox_message( connection, msg_uid, test ) + if isinstance(msg_content, ParserError): + # Never submitted, so never yielded and never queued in + # fetched_ids; the post-loop disposal handles it. + logger.warning(str(msg_content)) + invalid_msg_ids.append(message_id) + continue fetched_ids.append(message_id) yield msg_content @@ -3116,6 +3237,8 @@ def get_dmarc_reports_from_mailbox( ) message_id, msg_content = _fetch_mailbox_message(connection, msg_uid, test) try: + if isinstance(msg_content, ParserError): + raise msg_content parsed_email = parse_report_email( msg_content, config=cfg, diff --git a/parsedmarc/parallel.py b/parsedmarc/parallel.py index 02a7a14f..6df5f5b4 100644 --- a/parsedmarc/parallel.py +++ b/parsedmarc/parallel.py @@ -88,8 +88,9 @@ def _parse_report_email_job( Returns the parsed report on success. A ``ParserError`` raised by ``parse_report_email`` is caught and returned as a value (never re-raised) so that a single invalid message doesn't need special - handling on the submission side; any other exception propagates and - surfaces as the future's exception. + handling on the submission side. ``parse_report_email`` raises only + ``ParserError`` subclasses, whatever the input, so no other exception + is expected to propagate from here. """ from parsedmarc import ParserError, parse_report_email diff --git a/parsedmarc/utils.py b/parsedmarc/utils.py index 2e1b9a3f..e8fa9e3f 100644 --- a/parsedmarc/utils.py +++ b/parsedmarc/utils.py @@ -1338,12 +1338,17 @@ def get_filename_safe_string(string: str | None) -> str: space (Windows drops both when creating a file, which would make two distinct subjects collide on one name), and it is never ``.`` or ``..`` (both consist only of stripped characters and collapse to ``""``). It - is at most 100 characters long. It can be empty -- when the input is - empty, consists only of stripped characters, or is truncated to a run - of dots and spaces -- so callers that need a non-empty name must supply - their own fallback (e.g. ``get_filename_safe_string(subject) or - "sample"``). Windows reserved device names such as ``CON`` are not - rewritten. + is at most 100 characters *and* at most 100 bytes when UTF-8 encoded: + filesystems limit a name component to 255 bytes (143 on eCryptfs), not + characters, so 84 CJK characters or 63 emoji plus ``.eml`` already + exceed the limit, and 100 bytes leaves room for a ``" (N)"`` + de-duplication suffix and an extension under every real limit. A + multi-byte character is never split, and lone surrogates are dropped. + It can be empty -- when the input is empty, consists only of stripped + characters, or is truncated to a run of dots and spaces -- so callers + that need a non-empty name must supply their own fallback (e.g. + ``get_filename_safe_string(subject) or "sample"``). Windows reserved + device names such as ``CON`` are not rewritten. Args: string (str | None): A string to make safe for a filename. @@ -1359,8 +1364,14 @@ def get_filename_safe_string(string: str | None) -> str: string = string.replace(char, "") # Truncate before stripping trailing dots and spaces, so that a name cut - # off just after one cannot end in it. - string = (string[:100]) if len(string) > 100 else string + # off just after one cannot end in it. Bound both characters and UTF-8 + # bytes (GHSA-j53g-34vj-g25q); decoding with errors="ignore" drops a + # multi-byte sequence cut in half, and encoding with errors="ignore" + # drops lone surrogates, which cannot be encoded at all. + string = string[:100] + string = string.encode("utf-8", errors="ignore")[:100].decode( + "utf-8", errors="ignore" + ) string = string.rstrip(". ") return string diff --git a/tests/test_init.py b/tests/test_init.py index 007fe0ce..e3a73ffd 100644 --- a/tests/test_init.py +++ b/tests/test_init.py @@ -9,6 +9,7 @@ import base64 import binascii import csv import email +import email.errors import gzip import inspect import json @@ -35,7 +36,7 @@ from lxml import etree # type: ignore[import-untyped] import parsedmarc import parsedmarc.constants as constants -from parsedmarc.mail import MaildirConnection, MSGraphConnection +from parsedmarc.mail import IMAPConnection, MaildirConnection, MSGraphConnection from parsedmarc.types import ( AggregateReport, FailureReport, @@ -168,6 +169,56 @@ def compare_xml(xml1, xml2): return etree.tostring(tree1) == etree.tostring(tree2) +# GHSA-qxwv-2vwj-v828 fixtures. A failure report whose message/rfc822 sample +# has a header containing \x1c, which str.splitlines() treats as a line +# break, so re-serializing the sample raises email.errors.HeaderWriteError. +POISON_HEADER_EMAIL = ( + "From: a@example.com\nSubject: x\nDate: Mon, 1 Jan 2024 00:00:00 +0000\n" + "MIME-Version: 1.0\n" + "Content-Type: multipart/report; report-type=feedback-report; boundary=B\n\n" + "--B\nContent-Type: text/plain\n\nhi\n" + "--B\nContent-Type: message/feedback-report\n\n" + "Feedback-Type: auth-failure\nVersion: 1\nSource-IP: 192.0.2.1\n" + "Arrival-Date: Mon, 1 Jan 2024 00:00:00 +0000\nReported-Domain: example.com\n" + "Authentication-Results: x\n\n" + "--B\nContent-Type: message/rfc822\n\nFrom: a@b\nX-A: a\x1cb\n\nbody\n--B--\n" +).encode() + + +# Far past the recursion limit wherever the message is re-serialized: the +# email generator recurses a few frames per level, so the smallest failing +# depth is about 247 levels from a shallow main thread and drops with every +# frame already on the stack (about 222 at +100 frames), while mail-parser's +# own nesting guard only trips far above this. Still parses in milliseconds. +POISON_NESTING_LEVELS = 400 +# Comfortably below the limit even from a deep stack. +SHALLOW_NESTING_LEVELS = 50 + + +def _nested_multipart_email(levels: int) -> bytes: + """A message with ``levels`` nested ``multipart/mixed`` parts around one + text part; re-serializing ``POISON_NESTING_LEVELS`` levels exceeds the + recursion limit.""" + message = "Subject: s\n" + for i in range(levels): + message += f"Content-Type: multipart/mixed; boundary=b{i}\n\n--b{i}\n" + message += "Content-Type: text/plain\n\nx\n" + for i in range(levels - 1, -1, -1): + message += f"--b{i}--\n" + return message.encode() + + +def _reserializes(raw: bytes) -> bool: + """Whether this Python can parse and re-serialize ``raw``; a Python + predating the header-injection check (CVE-2024-6923) writes + POISON_HEADER_EMAIL without raising.""" + try: + email.message_from_bytes(raw).as_string() + except (email.errors.MessageError, RecursionError): + return False + return True + + class Test(unittest.TestCase): """Kitchen-sink tests redistributed from the original tests.py monolith. Future PRs should split these further @@ -2761,6 +2812,93 @@ class TestMalformedXmlRecovery(unittest.TestCase): self.assertEqual(len(report["records"]), len(expected["records"])) self.assertGreater(len(report["records"]), 0) + DOCTYPE_SAMPLE = "samples/aggregate/!example.com!1538204542!1538463818.xml" + + def _sample_with_doctype(self, doctype, declaration=None): + """The sample report with ``doctype`` inserted right after its XML + declaration, which is replaced by ``declaration`` when given""" + with open(self.DOCTYPE_SAMPLE, encoding="utf-8") as f: + original = f.read() + self.assertTrue(original.startswith("", 1) + return (declaration or header + "?>") + doctype + body + + def testDoctypeIsRejected(self): + """An aggregate report carrying any DOCTYPE is rejected. + + Regression test for GHSA-m47j-wp78-fqjg: an internal DTD subset can + declare default attributes that expat applies to every matching + element, so the parser used to accept, and expand, this report. + Real aggregate reports never carry a DOCTYPE.""" + # The sample parses without the DOCTYPE, so only it is rejected. + parsedmarc.parse_aggregate_report_xml( + self._sample_with_doctype(""), offline=OFFLINE_MODE + ) + for doctype in ( + ']>', + "", + ): + with self.subTest(doctype=doctype): + with self.assertRaises(parsedmarc.InvalidAggregateReport) as ctx: + parsedmarc.parse_aggregate_report_xml( + self._sample_with_doctype(doctype), offline=OFFLINE_MODE + ) + self.assertIn("DOCTYPE", str(ctx.exception)) + + def testDoctypeIsRejectedOnTheRecoveryPath(self): + """A DOCTYPE behind a malformed XML declaration is rejected too. + + The malformed declaration makes the first parse fail before expat + reaches the DOCTYPE, so the document goes through the lxml recovery + path, which must apply the same check (GHSA-m47j-wp78-fqjg).""" + declaration = '' + # Without a DOCTYPE, the recovery path parses the report. + report = parsedmarc.parse_aggregate_report_xml( + self._sample_with_doctype("", declaration=declaration), + offline=OFFLINE_MODE, + ) + self.assertTrue( + any( + e.startswith("Invalid XML") for e in report["report_metadata"]["errors"] + ) + ) + with self.assertRaises(parsedmarc.InvalidAggregateReport) as ctx: + parsedmarc.parse_aggregate_report_xml( + self._sample_with_doctype( + ']>', + declaration=declaration, + ), + offline=OFFLINE_MODE, + ) + self.assertIn("DOCTYPE", str(ctx.exception)) + + def testEmptyDocumentIsInvalid(self): + """A document lxml cannot recover anything from (it raises "Document + is empty") is still reported as an InvalidAggregateReport.""" + with self.assertRaises(parsedmarc.InvalidAggregateReport): + parsedmarc.parse_aggregate_report_xml("", offline=OFFLINE_MODE) + + def testAttlistDefaultAttributeAmplificationIsRejectedQuickly(self): + """The advisory's default-attribute amplification document is + rejected before expat expands it. + + Regression test for GHSA-m47j-wp78-fqjg: ```` declaring + 1,000 default attributes, followed by 10,000 ```` elements (64 KB + of XML), made xmltodict build 10 million dict entries. Measured + locally, the old code took 4.0 s and 1.4 GB RSS on this input; the + reporter measured 29 s and 1.4 GB from a 3.8 KB gzip email. The fixed + code rejects it in about 0.1 ms, so the 1 s bound is not flaky.""" + attributes = " ".join(f'a{i} CDATA "vvvvvvvvvv"' for i in range(1000)) + xml = ( + f"]>" + f"{'' * 10_000}" + ) + start = time.perf_counter() + with self.assertRaises(parsedmarc.InvalidAggregateReport) as ctx: + parsedmarc.parse_aggregate_report_xml(xml, offline=OFFLINE_MODE) + self.assertLess(time.perf_counter() - start, 1) + self.assertIn("DOCTYPE", str(ctx.exception)) + class TestPolicyPublishedEdgeCases(unittest.TestCase): """Tests for edge cases in policy_published parsing""" @@ -3373,6 +3511,98 @@ This is not a DMARC report.""" [("Received Date", "Mon, 01 Jan 2024 00:00:00 +0000")], ) + def testUnserializableSubMessageRaisesParserError(self): + """A ``message/rfc822`` part whose header holds a control character + raises ``ParserError``, not ``email.errors.HeaderWriteError``. + + Regression test for GHSA-qxwv-2vwj-v828: re-serializing the part + raised ``HeaderWriteError``, which escaped the mailbox loop's + ``except ParserError`` and aborted the whole batch on every run. + (A Python without the header-injection check instead parses the + message as an invalid failure report, which is also a + ``ParserError``.)""" + with self.assertRaises(parsedmarc.ParserError): + parsedmarc.parse_report_email(POISON_HEADER_EMAIL, offline=True) + + def testDeeplyNestedMultipartRaisesParserError(self): + """Multiparts nested past the recursion limit raise ``ParserError``, + not ``RecursionError`` (GHSA-qxwv-2vwj-v828); a shallow nesting is + walked without error and rejected as not a report, as before.""" + with self.assertRaises(parsedmarc.ParserError) as ctx: + parsedmarc.parse_report_email( + _nested_multipart_email(POISON_NESTING_LEVELS), offline=True + ) + self.assertIn("recursion", str(ctx.exception)) + + with self.assertRaises(parsedmarc.InvalidDMARCReport) as ctx: + parsedmarc.parse_report_email( + _nested_multipart_email(SHALLOW_NESTING_LEVELS), offline=True + ) + self.assertIn("is not a valid report", str(ctx.exception)) + + @staticmethod + def _email_with_attachment(payload: bytes, subtype: str) -> str: + from email.mime.application import MIMEApplication + + msg = MIMEMultipart() + msg["From"] = "reports@example.com" + msg["Subject"] = "Report" + msg["Date"] = "Mon, 01 Jan 2024 00:00:00 +0000" + # MIMEApplication base64-encodes the payload. + msg.attach(MIMEApplication(payload, subtype)) + return msg.as_string() + + def testBase64SmtpTlsJsonPart(self): + """An ``application/tlsrpt+json`` part sent base64-encoded is decoded + and parsed as an SMTP TLS report""" + with open("samples/smtp_tls/rfc8460.json", "rb") as f: + payload = f.read() + result = parsedmarc.parse_report_email( + self._email_with_attachment(payload, "tlsrpt+json"), offline=True + ) + self.assertEqual(result["report_type"], "smtp_tls") + + def testSmtpTlsJsonInGenericAttachment(self): + """A JSON report in an attachment of any other type is recognized by + its leading ``{`` and parsed as an SMTP TLS report""" + with open("samples/smtp_tls/rfc8460.json", "rb") as f: + payload = f.read() + result = parsedmarc.parse_report_email( + self._email_with_attachment(payload, "octet-stream"), offline=True + ) + self.assertEqual(result["report_type"], "smtp_tls") + + def testNonBase64AttachmentIsSkipped(self): + """An attachment whose body is not base64 is not a report attachment: + it is skipped rather than treated as a parse failure, so a message + with nothing else in it is simply not a valid report""" + eml = ( + "From: reports@example.com\n" + "Subject: Report\n" + "Date: Mon, 01 Jan 2024 00:00:00 +0000\n" + "Content-Type: application/octet-stream\n\n" + # Three base64 characters: b64decode() raises "Incorrect padding". + "abc\n" + ) + with self.assertRaises(parsedmarc.InvalidDMARCReport) as ctx: + parsedmarc.parse_report_email(eml, offline=True) + self.assertEqual( + str(ctx.exception), 'Message with subject "Report" is not a valid report' + ) + + def testInvalidAggregateAttachmentRaisesParserError(self): + """An XML attachment that is not a valid aggregate report raises a + ParserError naming the message""" + with self.assertRaises(parsedmarc.ParserError) as ctx: + parsedmarc.parse_report_email( + self._email_with_attachment(b"", "xml"), + offline=True, + ) + self.assertIn( + 'Message with subject "Report" is not a valid DMARC report', + str(ctx.exception), + ) + class TestFailureReportParsing(unittest.TestCase): """Tests for failure report field defaults and edge cases""" @@ -3691,6 +3921,57 @@ class TestGetDmarcReportsFromMbox(unittest.TestCase): finally: os.remove(path) + def testPoisonMessageRaisesParserError(self): + """A message that cannot be re-serialized reaches the parser and + surfaces as a ``ParserError``, sequentially and with n_procs=2. + + Regression test for GHSA-qxwv-2vwj-v828: mbox.get_string() + re-serializes each message (it is + ``email.message_from_bytes(get_bytes(key)).as_string()``), so the + raw email.errors.HeaderWriteError or RecursionError escaped before + the message reached the parser. get_bytes() returns the stored + message as-is, and parse_report_email() turns the content error into + a ``ParserError``. The mbox loop's policy is unchanged: it skips only + ``InvalidDMARCReport`` and re-raises any other ``ParserError``.""" + # The error text, not __cause__, identifies the content error: an + # exception returned from a worker process loses its __cause__. + exercised = 0 + for poison_name, poison, cause in ( + ("header", POISON_HEADER_EMAIL, "folded header contains newline"), + ( + "nested", + _nested_multipart_email(POISON_NESTING_LEVELS), + "maximum recursion depth", + ), + ): + if _reserializes(poison): + # Not a poison message on this Python (see _reserializes). + continue + exercised += 1 + with NamedTemporaryFile(suffix=".mbox", delete=False) as f: + path = f.name + self.addCleanup(os.remove, path) + box = mailbox.mbox(path) + box.lock() + try: + box.add(poison) + with open("samples/aggregate/twilight.eml", "rb") as source: + box.add(source.read()) + box.flush() + finally: + box.unlock() + box.close() + for n_procs in (1, 2): + with self.subTest(poison=poison_name, n_procs=n_procs): + with self.assertRaises(parsedmarc.ParserError) as ctx: + parsedmarc.get_dmarc_reports_from_mbox( + path, offline=True, n_procs=n_procs + ) + self.assertIn("Unable to parse message", str(ctx.exception)) + self.assertIn(cause, str(ctx.exception)) + # The nested message is a poison on every Python. + self.assertGreater(exercised, 0) + class TestGetDmarcReportsFromMboxParallel(unittest.TestCase): """n_procs=1 vs n_procs=2 parity for get_dmarc_reports_from_mbox, driven @@ -4193,6 +4474,161 @@ class _FailingDisposalMaildirConnection(MaildirConnection): super().move_message(message_id, folder_name) +class _InMemoryMailboxConnection(parsedmarc.MailboxConnection): + """A mailbox backend held in memory. Like IMAP or Microsoft Graph, it + returns each message's stored text as-is and moves a message without + rewriting it, unlike the Maildir backend, which re-serializes both + ways. ``fetch_errors`` maps a message id to the exception its fetch + raises.""" + + def __init__(self, messages, fetch_errors=None): + self.folders = {"INBOX": dict(messages)} + self.fetch_errors = fetch_errors or {} + + def create_folder(self, folder_name): + self.folders.setdefault(folder_name, {}) + + def folder_exists(self, folder_name): + return folder_name in self.folders + + def fetch_messages(self, reports_folder, **kwargs): + return list(self.folders[reports_folder]) + + def fetch_message(self, message_id, **kwargs): + if message_id in self.fetch_errors: + raise self.fetch_errors[message_id] + return self.folders["INBOX"][message_id] + + def delete_message(self, message_id): + del self.folders["INBOX"][message_id] + + def move_message(self, message_id, folder_name): + self.folders[folder_name][message_id] = self.folders["INBOX"].pop(message_id) + + def keepalive(self): + return None + + def watch(self, check_callback, check_timeout, config_reloading=None): + return None + + +class _InMemoryIMAPConnection(_InMemoryMailboxConnection, IMAPConnection): + """The in-memory backend, typed as an IMAP connection so parsedmarc + takes its IMAP code paths. Its message ids are ints, as IMAP UIDs are, + so a lookup with any other type fails.""" + + +class TestGetDmarcReportsFromMailboxContentErrors(unittest.TestCase): + """GHSA-qxwv-2vwj-v828 on backends that neither fetch nor move a message + by re-serializing it: a message whose content raises a non-ParserError + exception is moved to the invalid folder (or deleted), and the rest of + the batch is processed, sequentially and with n_procs=2.""" + + def setUp(self): + parsedmarc.SEEN_AGGREGATE_REPORT_IDS.clear() + with open("samples/aggregate/twilight.eml", encoding="utf-8") as f: + self.aggregate = f.read() + + def _run(self, conn, n_procs, **kwargs): + parsedmarc.SEEN_AGGREGATE_REPORT_IDS.clear() + with self.assertLogs("parsedmarc.log", level="WARNING") as cm: + result = parsedmarc.get_dmarc_reports_from_mailbox( + connection=conn, offline=True, n_procs=n_procs, **kwargs + ) + self.assertEqual(len(result["aggregate_reports"]), 1) + self.assertEqual(result["failure_reports"], []) + return cm.output + + def test_fetch_content_error_moves_message_to_invalid(self): + """A content error raised by the backend's fetch (as the Maildir + backend raises it) moves that message to the invalid folder, or + deletes it with delete_invalid; IMAP UIDs stay ints throughout.""" + errors = ( + email.errors.HeaderWriteError("folded header contains newline"), + RecursionError("maximum recursion depth exceeded"), + ) + for conn_cls, poison_id, report_id in ( + (_InMemoryMailboxConnection, "poison", "report"), + (_InMemoryIMAPConnection, 1, 2), + ): + for error in errors: + for n_procs in (1, 2): + with self.subTest( + backend=conn_cls.__name__, + error=type(error).__name__, + n_procs=n_procs, + ): + conn = conn_cls( + {poison_id: "unused", report_id: self.aggregate}, + fetch_errors={poison_id: error}, + ) + output = self._run(conn, n_procs) + self.assertEqual(conn.folders["INBOX"], {}) + self.assertEqual( + list(conn.folders["Archive/Invalid"]), [poison_id] + ) + self.assertEqual( + list(conn.folders["Archive/Aggregate"]), [report_id] + ) + self.assertTrue( + any( + f"Unable to fetch message UID {poison_id}: " + f"{type(error).__name__}" in line + for line in output + ), + output, + ) + + conn = conn_cls( + {poison_id: "unused", report_id: self.aggregate}, + fetch_errors={poison_id: error}, + ) + self._run(conn, n_procs, delete_invalid=True) + self.assertEqual(conn.folders["INBOX"], {}) + self.assertEqual(conn.folders["Archive/Invalid"], {}) + self.assertEqual( + list(conn.folders["Archive/Aggregate"]), [report_id] + ) + + def test_unparseable_content_moves_message_to_invalid(self): + """A backend that returns the raw message, as IMAP does, hands the + poison content to parse_report_email(), whose ParserError now moves + the message to the invalid folder instead of a HeaderWriteError or + RecursionError aborting the batch.""" + exercised = 0 + for poison_name, poison in ( + ("header", POISON_HEADER_EMAIL), + ("nested", _nested_multipart_email(POISON_NESTING_LEVELS)), + ): + if _reserializes(poison): + # Not a poison message on this Python (see _reserializes). + continue + exercised += 1 + for n_procs in (1, 2): + with self.subTest(poison=poison_name, n_procs=n_procs): + conn = _InMemoryIMAPConnection( + {1: poison.decode(), 2: self.aggregate} + ) + output = self._run(conn, n_procs) + self.assertEqual(conn.folders["INBOX"], {}) + self.assertEqual(list(conn.folders["Archive/Invalid"]), [1]) + self.assertEqual(list(conn.folders["Archive/Aggregate"]), [2]) + self.assertTrue( + any("Unable to parse message" in line for line in output), + output, + ) + # The nested message is a poison on every Python. + self.assertGreater(exercised, 0) + + +class _FailingFetchMaildirConnection(MaildirConnection): + """A MaildirConnection whose every fetch fails like a dropped network + connection would.""" + + def fetch_message(self, message_id, **kwargs): + raise OSError("connection reset") + + class TestGetDmarcReportsFromMailboxMaildir(unittest.TestCase): """parsedmarc's real mailbox processing loop, end to end on an on-disk Maildir (mailsuite MaildirConnection, no mocks, offline parsing): fetch @@ -4519,6 +4955,168 @@ class TestGetDmarcReportsFromMailboxMaildir(unittest.TestCase): self.assertEqual(conn.fetch_messages("Archive/Aggregate"), []) self.assertEqual(len(conn.fetch_messages("Archive/SMTP-TLS")), 1) + def _poison_maildir(self, name, poison): + """A fresh Maildir holding ``poison`` (delivered byte-for-byte, so the + stdlib never has to re-serialize it) and the aggregate sample. + + Returns ``(path, poison_key)``.""" + path = os.path.join(self._tmp, name) + box = mailbox.Maildir(path, create=True) + poison_key = box.add(poison) + with open(self.AGGREGATE, "rb") as f: + box.add(f.read()) + box.flush() + return path, poison_key + + def _skip_unless_poison(self, poison): + if _reserializes(poison): + self.skipTest( + "this Python re-serializes the message without error, so it " + "is not a poison message here" + ) + + def _maildir_stores_parsed(self, poison): + """Whether this Python's ``mailbox.Maildir.add()`` can store + ``poison`` once it has been read back as a message object, which is + what MaildirConnection.move_message() does.""" + scratch = mkdtemp(dir=self._tmp) + source = mailbox.Maildir(os.path.join(scratch, "source"), create=True) + destination = mailbox.Maildir(os.path.join(scratch, "dest"), create=True) + try: + destination.add(source[source.add(poison)]) + except (email.errors.MessageError, RecursionError): + return False + finally: + rmtree(scratch, ignore_errors=True) + return True + + def test_poison_message_does_not_abort_the_batch(self): + """A message the Maildir backend cannot re-serialize while fetching + it no longer aborts the batch: the other report is still parsed and + archived, sequentially and with n_procs=2. + + Regression test for GHSA-qxwv-2vwj-v828: MaildirConnection's + fetch_message() calls Message.as_string(), so the + email.errors.HeaderWriteError (a control character in a sub-message + header) or RecursionError (deeply nested multiparts) escaped from the + fetch, outside the per-message ``except ParserError``, and every + run aborted on the same message. + + The message is then disposed of as invalid. move_message() stores + it with ``mailbox.Maildir.add()``, which re-serializes it through + ``email.generator.BytesGenerator``. The nested message overflows the + stack there on every Python, so its move fails, is logged, and the + message stays in the INBOX instead of aborting the batch a second + way. Whether the control-character message can be stored depends on + the interpreter: upstream CPython (e.g. 3.12.9) applies the + CVE-2024-6923 header check only in the text ``Generator`` that + ``as_string()`` uses, so the move succeeds, while some distribution + builds (e.g. Ubuntu's 3.12.3) also apply it in ``BytesGenerator``, + so the move fails like the nested one. The expected outcome is + decided per message, at runtime.""" + kept_in_inbox = 0 + for poison_name, poison in ( + ("header", POISON_HEADER_EMAIL), + ("nested", _nested_multipart_email(POISON_NESTING_LEVELS)), + ): + if _reserializes(poison): + # Not a poison message on this Python (see _reserializes). + continue + stored = self._maildir_stores_parsed(poison) + if not stored: + kept_in_inbox += 1 + for n_procs in (1, 2): + with self.subTest(poison=poison_name, n_procs=n_procs): + parsedmarc.SEEN_AGGREGATE_REPORT_IDS.clear() + path, poison_key = self._poison_maildir( + f"{poison_name}-{n_procs}", poison + ) + conn = MaildirConnection(path, maildir_create=True) + with self.assertLogs("parsedmarc.log", level="WARNING") as cm: + result = parsedmarc.get_dmarc_reports_from_mailbox( + connection=conn, + reports_folder="INBOX", + archive_folder="Archive", + offline=True, + test=False, + n_procs=n_procs, + ) + + # The batch completed and the real report was archived. + self.assertEqual(len(result["aggregate_reports"]), 1) + self.assertEqual(result["failure_reports"], []) + self.assertEqual(len(conn.fetch_messages("Archive/Aggregate")), 1) + self.assertTrue( + any( + f"Unable to fetch message UID {poison_key}" in line + for line in cm.output + ), + cm.output, + ) + move_errors = [ + line + for line in cm.output + if "Mailbox error: Error moving invalid message UID " + f"{poison_key}" in line + ] + if stored: + self.assertEqual(conn.fetch_messages("INBOX"), []) + self.assertEqual(len(conn.fetch_messages("Archive/Invalid")), 1) + self.assertFalse( + [line for line in cm.output if "Mailbox error" in line], + cm.output, + ) + else: + self.assertEqual(conn.fetch_messages("INBOX"), [poison_key]) + self.assertEqual(conn.fetch_messages("Archive/Invalid"), []) + self.assertTrue(move_errors, cm.output) + # The nested message cannot be stored on any Python, so the + # kept-in-INBOX branch always runs; the stored branch runs for the + # control-character message on upstream CPython. + self.assertGreater(kept_in_inbox, 0) + + def test_poison_message_is_deleted_with_delete_invalid(self): + """With delete_invalid, a message that failed to fetch is deleted + like any other invalid message (deleting does not re-serialize it), + sequentially and with n_procs=2 (GHSA-qxwv-2vwj-v828).""" + self._skip_unless_poison(POISON_HEADER_EMAIL) + for n_procs in (1, 2): + with self.subTest(n_procs=n_procs): + parsedmarc.SEEN_AGGREGATE_REPORT_IDS.clear() + path, _ = self._poison_maildir(f"delete-{n_procs}", POISON_HEADER_EMAIL) + conn = MaildirConnection(path, maildir_create=True) + with self.assertLogs("parsedmarc.log", level="WARNING"): + result = parsedmarc.get_dmarc_reports_from_mailbox( + connection=conn, + offline=True, + delete_invalid=True, + n_procs=n_procs, + ) + + self.assertEqual(len(result["aggregate_reports"]), 1) + self.assertEqual(conn.fetch_messages("INBOX"), []) + self.assertEqual(len(conn.fetch_messages("Archive/Aggregate")), 1) + self.assertEqual(conn.fetch_messages("Archive/Invalid"), []) + + def test_transport_error_on_fetch_still_propagates(self): + """Only content errors from the fetch mark a message invalid: a + transport error such as OSError still propagates, sequentially and + with n_procs=2, and the message is neither moved nor deleted, since + a transient failure says nothing about the message itself.""" + for n_procs in (1, 2): + with self.subTest(n_procs=n_procs): + path, _ = self._poison_maildir(f"oserror-{n_procs}", self.JUNK) + conn = _FailingFetchMaildirConnection(path, maildir_create=True) + with self.assertRaises(OSError): + parsedmarc.get_dmarc_reports_from_mailbox( + connection=conn, + offline=True, + delete_invalid=True, + n_procs=n_procs, + ) + self.assertEqual(len(conn.fetch_messages("INBOX")), 2) + self.assertEqual(conn.fetch_messages("Archive/Invalid"), []) + class _UncreatableFolderMaildirConnection(MaildirConnection): """A MaildirConnection whose folder_exists() always fails. @@ -5709,6 +6307,23 @@ class TestSaveOutput(unittest.TestCase): [os.path.join(samples, "report.eml")], ) + def testMultiByteSubjectWithinFilesystemNameLimit(self): + """A subject of 84 CJK characters is truncated to fit the + filesystem's 255-byte name limit, and the sample is written. + + Regression test for GHSA-j53g-34vj-g25q: the name was truncated to + 100 characters, so 84 three-byte characters plus ``.eml`` made a + 256-byte name and save_output() failed with ENAMETOOLONG after the + batch's JSON and CSV rows had already been appended.""" + subject = "\u4e2d" * 84 + self._save(self._failure_report(subject)) + + samples = os.path.join("parsedmarc", "output", "samples") + self.assertEqual( + [f for f in self._written_files() if f.endswith(".eml")], + [os.path.join(samples, "\u4e2d" * 33 + ".eml")], + ) + def testOrdinarySubjectNamesTheFileAndSampleIsWrittenVerbatim(self): """An ordinary subject is unchanged by sanitizing, and the file holds the message sample""" diff --git a/tests/test_utils.py b/tests/test_utils.py index 14a0a5cc..8d8c25a8 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -450,6 +450,49 @@ class Test(unittest.TestCase): result = parsedmarc.utils.get_filename_safe_string("a" * 200) self.assertEqual(len(result), 100) + def testGetFilenameSafeStringBoundsUtf8Bytes(self): + """Multi-byte names are truncated to at most 100 UTF-8 bytes, never + splitting a character, so the name plus an extension can be created. + + Regression test for GHSA-j53g-34vj-g25q: the name was truncated to + 100 *characters*, but filesystems limit a name component to 255 + *bytes*, so 84 three-byte CJK characters or 63 four-byte emoji plus + ``.eml`` (256 bytes) made ``save_output()`` fail with + ``ENAMETOOLONG``.""" + tmpdir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, tmpdir, ignore_errors=True) + for subject, expected in ( + # 33 whole characters (99 bytes); the 34th would straddle byte 100. + ("\u4e2d" * 84, "\u4e2d" * 33), + # Exactly 25 four-byte characters fit in 100 bytes. + ("\U0001f600" * 63, "\U0001f600" * 25), + ): + with self.subTest(subject=subject[:1]): + result = parsedmarc.utils.get_filename_safe_string(subject) + self.assertEqual(result, expected) + encoded = result.encode("utf-8") + self.assertLessEqual(len(encoded), 100) + # A strict round trip proves no partial character survived. + self.assertEqual(encoded.decode("utf-8"), result) + path = os.path.join(tmpdir, result + ".eml") + with open(path, "w") as f: + f.write("sample") + self.assertTrue(os.path.isfile(path)) + + def testGetFilenameSafeStringByteCutAfterDotIsStripped(self): + """When the byte bound cuts the name just after a dot, the dot is + still stripped: 33 CJK characters (99 bytes) and a dot fill exactly + 100 bytes, and the name stays under the 100-character bound.""" + subject = "\u4e2d" * 33 + "." + "\u4e2d" * 10 + result = parsedmarc.utils.get_filename_safe_string(subject) + self.assertEqual(result, "\u4e2d" * 33) + + def testGetFilenameSafeStringDropsLoneSurrogates(self): + """A lone surrogate cannot be encoded as UTF-8 (or used in a file + name), so it is dropped rather than raising.""" + result = parsedmarc.utils.get_filename_safe_string("re\ud800port") + self.assertEqual(result, "report") + def testGetFilenameSafeStringTrailingDot(self): """get_filename_safe_string strips trailing dots""" result = parsedmarc.utils.get_filename_safe_string("filename...")