diff --git a/.claude/agents/security-reviewer.md b/.claude/agents/security-reviewer.md index 6aebc41..d0983bb 100644 --- a/.claude/agents/security-reviewer.md +++ b/.claude/agents/security-reviewer.md @@ -32,6 +32,12 @@ A security finding you cannot demonstrate is a guess. Before you report: pattern can be quadratic in isolation yet unreachable because an earlier step normalizes the input. Guard each probe with `signal.setitimer` so a true blowup fails fast instead of hanging. + **Complexity is not only regexes.** The dominant blowups in this codebase are + plain Python: a loop over attacker-chosen keys where each iteration rescans + the whole collection (`for k in keys: message.get_all(k)`) is O(keys × total) + with no regex involved. Vary the two axes *independently* — many distinct + header names vs. many repeats of one name — because a probe that only grows + the total byte count keeps the ratio flat and hides the quadratic term. - **Injection / traversal / write-primitive** — construct the malicious input and show the resulting command, path, or file write. A `../` that `os.path.basename` strips is not a finding. @@ -62,6 +68,69 @@ A security finding you cannot demonstrate is a guess. Before you report: | Outlook `.msg` path (`extract_msg`, `msgconvert`) | Untrusted `.msg` flows into third-party OLE/CFB + Perl parsers with their own CVE history — flag the transitive attack surface and the temp/subprocess handling around it. | | Logging (`log.debug` of headers/filenames) | Raw headers and attachment names are logged verbatim (PII / indicators). Data-handling note, not code-exec — report as Info. | | `eval`/`exec`/`pickle`/`yaml.load`/`__import__` | Should be absent. Any occurrence is a finding until proven inert. | +| `getattr(self, X)` where `X` is a header/part name | Reflective dispatch on an attacker-chosen name. See "Reflective dispatch" below — collision, recursion, and non-serializable leakage are all reachable from a 12-byte email. | +| `json.dumps` over a dynamically built dict | Values arriving from reflection are not guaranteed JSON-safe. A bound method or `Message` object in the dict is an uncaught `TypeError` on a public property. | +| `find()` / `in` / `split()` locating a security token | Naive substring search for a delimiter (`"by"`, a trust string) that an attacker also controls the *neighbouring* text of. See "Trust-boundary string parsing" below. | + +## Reflective dispatch on attacker-controlled names + +`MailParser.__getattr__` exposes every header as an attribute, and `_make_mail` +/ `headers` iterate `message.keys()` calling `getattr(self, name)`. The header +name is attacker-chosen, so **the attacker picks which Python attribute is +read**. Python resolves real class attributes *before* `__getattr__`, so any +name in `dir(cls)` shadows the header path. Always run this probe: + +```python +for a in [x for x in dir(MailParser) if not x.startswith("_")]: + try: + mailparser.parse_from_string(f"{a}: x\r\n\r\n").mail_json + except Exception as e: + print(a, type(e).__name__, e) +``` + +Three distinct bug classes fall out, and you must check for all three — finding +one does not rule out the others: + +- **Method/property collision** — the dict gets a bound method or a live object + instead of a string. Downstream `json.dumps` raises `TypeError`. Crash-DoS on + a public API from a minimal message. +- **Recursion cycle** — a property that itself iterates `message.keys()` and + calls `getattr` can re-enter itself when a header is named after that property + or one of its `_json` / `_raw` aliases. Check the cycle guard covers **every** + alias and is **case-insensitive**: `set(message.keys()) - {"headers"}` does not + exclude `Headers_json`, and `message.keys()` preserves the sender's casing. + A recursion cycle nested inside a per-key rescan multiplies the two costs — + measure it, it is usually the worst finding on the page. +- **Side-effecting property** — reflection can *invoke* a property the caller + never asked for. Confirm no property in the collision set writes files, spawns + a subprocess, or mutates state. + +The fix to argue for is structural, not a denylist: header values must resolve +through a lookup that never touches Python attributes. Reject patches that just +add another name to an exclusion set — that is how the `headers_json` cycle +survived the `headers` fix. + +## Trust-boundary string parsing + +`get_server_ipaddress(trust)` decides *which IP the mail came from* — its output +is used for attribution and blocklisting, so a wrong answer is a security +failure, not a cosmetic bug. Anywhere a security decision depends on locating a +delimiter, check the search is anchored: + +- `header.find("by")` matches inside `derby.example.com` or `nearby.example.org`. + Hostnames come from the sender's HELO (no DNS control needed) and land in the + trusted MTA's own `Received` header. +- Truncating the clause makes extraction fail on the *genuine* top hop, and the + loop then falls through to older, fully attacker-forged `Received` headers — + so the failure mode is not "returns nothing", it is "returns the attacker's + value". Always test the fall-through, not just the single-header case. +- Use the existing anchored `const._CLAUSE_SPLITTER` rather than a bare `\bby\b` + (`\b` still matches inside `host.by.example`, since `.` is a non-word char). + +Prove these with a control matrix, not a single PoC: benign hostname, malicious +hostname alone, forged header alone, and both together. If a benign hostname +also misattributes, say so — it makes the finding a correctness bug too and +raises the priority. ## Invariants that must stay true (verify, don't assume) diff --git a/CLAUDE.md b/CLAUDE.md index c90cef9..09ba441 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -61,6 +61,25 @@ Three access modes are supported for any attribute `X`: Address headers listed in `const.ADDRESSES_HEADERS` (`from`, `to`, `cc`, `bcc`, `reply-to`, `delivered-to`) return `list[tuple[str, str]]` (display_name, email) instead of plain strings. +**Never resolve a header name with `getattr`, and never rewrite it**: `__getattr__` serves names +the *caller* types, so it may fold `_`→`-`, honour the `_json`/`_raw` suffixes, and use `getattr` +to reach computed parts (`attachments_json`). Names taken from a parsed message must go through +`MailParser._header_value()`, which looks the name up literally in the header index and never +touches a Python attribute. Both halves matter: + +- Python looks up real class attributes before `__getattr__`, so a sender naming a header `Parse` + or `Headers_json` otherwise picks which attribute is read — a bound method lands in the parsed + mail, or a property re-enters itself. Excluding individual names from a key set is not a fix; + that is what let `Headers_json` through after `headers` was excluded. +- Suffix handling on a wire name is an amplifier and an evasion: each `_json` re-serializes the + previous result (five input bytes per doubling of the output), and `Subject_json:` reports the + value of `Subject` while silently dropping its own. + +**Header index**: `_build_header_index()` maps lowercased name → list of raw values once per +parse. `Message.get_all()` is a linear scan, so the one call per distinct name previously made by +`_make_mail()` cost O(distinct × total) on attacker-chosen names. Look up via the index inside any +loop over header names. + **RFC non-compliance fallback in `get_addresses()`**: Python's `email.utils.getaddresses(strict=True)` (hardened against CVE-2023-27043) rejects headers where the display name contains `@`. Since this is a forensics tool, `utils.py` applies @@ -71,6 +90,28 @@ actually in the header. (`from`, `by`, `via`, `with`, `id`, `for`, `envelope-from`) using `const._CLAUSE_SPLITTER`. Output list is ordered first-hop first. Unparseable headers fall back to `{"raw": ...}`. +**Sender-IP attribution fails closed**: `get_server_ipaddress()` walks trust-matching `Received` +headers only while a hop names a *private* IP (an internal relay); a hop naming **no** IP ends the +search with `None`. Never resume the walk on that case — older `Received` headers are written by +the sender, so "extraction failed" would become "returns the sender's chosen IP". + +Candidate addresses come only from the `from` clause (`utils.get_from_clause()`, which ends at the +next RFC 5321 keyword). Inside it, `_sender_ip_candidates()` applies one positional rule, because +**text cannot be classified by what it looks like** — a closed `[...]` pair the sender wrote is +byte-identical to one the MTA wrote, and `EHLO [8.8.8.8]` is a form RFC 5321 §4.1.3 requires: + +- the **first token** is the HELO name, whatever its shape, and is never a candidate; +- an explicit **HELO marker inside a comment group** (`(helo=x)`, `(account a@b HELO x)`) is + sender text and is excluded, located with `const._HELO_RE`; +- a candidate must sit **inside a `(`/`[` group** (`utils.group_spans()`), which is what makes a + clause truncated by a multi-word HELO fail closed — what it leaves behind is bare; +- IPv4 and IPv6 matches are merged **positionally**, never family-first: choosing IPv4 first let + one private literal at EHLO suppress the IPv6 scan and hide the real sender. + +Only concession: `from [ip] (helo=x)` (Exim/CommuniGate), accepted when there is no other +candidate and the marker is in a group. Do not add lookbehind guards to `_HELO_RE` to patch new +cases — three rounds of that each reopened a hole the previous one closed. + **Defect detection**: During `parse()`, every MIME part is walked and `_append_defects()` records RFC violations. `EPILOGUE_DEFECTS` triggers special epilogue extraction to recover hidden payloads in malformed boundaries. @@ -87,6 +128,10 @@ Accessible as `parser.mail` / `parser.mail_partial`. To make a header always appear in partial output, add its lowercase name to `OTHERS_PARTS` in `const.py`. Address-type headers (returning parsed name/email tuples) go in `ADDRESSES_HEADERS`. +If the new part is computed by a `MailParser` property rather than read off the wire, add it to +`COMPUTED_PARTS` as well — that set is exactly what `_make_mail()` is allowed to resolve through +attribute access. + ## Workflow After every change: diff --git a/README.md b/README.md index dcf4332..7eab039 100644 --- a/README.md +++ b/README.md @@ -179,9 +179,16 @@ pipelines. - `body` - Complete message body - `text_html` - HTML body parts (list) - `text_plain` - Plain text body parts (list) -- `headers` - All headers as a structured object +- `headers` - All headers as a structured object. Keys are the header names exactly as they + appear in the message, and every header is reported — including names that match a + `MailParser` method or property (`Parse`, `Message`, `Headers_json`) and names containing + underscores (`X_Spam_Flag`). Such names are always resolved as headers, never as attributes, + and are never rewritten. - `attachments` - Complete attachment metadata and payloads -- `get_server_ipaddress()` - Reliable sender IP extraction with trust levels +- `get_server_ipaddress()` - Reliable sender IP extraction with trust levels. Only the `from` + clause of the first trusted `Received` header is searched, with the sender-supplied HELO name + removed, and the result is `None` when that hop names no public IP. Attribution never falls + back to older `Received` headers, which the sender is free to forge. - `to_domains` - Extracted recipient domains for analysis - `timezone` - Detected timezone information - `defects` - RFC compliance issues for security analysis @@ -209,6 +216,12 @@ access the `X-MSMail-Priority` header: mail.X_MSMail_Priority ``` +This underscore-for-hyphen convenience, and the `_json` / `_raw` suffixes, apply only to +attribute access written by you. Names read out of a message — the keys of `mail` and `headers` — +are looked up literally, so a header genuinely named `X_Spam_Flag` or `Subject_json` keeps its own +name and its own value. Attribute names beginning with an underscore are not headers and raise +`AttributeError`. + The `received` header is intelligently parsed into individual hops, revealing the complete email routing path. Each hop contains structured fields: diff --git a/src/mailparser/const.py b/src/mailparser/const.py index 200dddf..d3c9266 100644 --- a/src/mailparser/const.py +++ b/src/mailparser/const.py @@ -25,16 +25,21 @@ ) # IPv6 pattern - matches standard and common compressed forms per RFC 5952 +# Alternation order matters: Python's ``re`` takes the first alternative +# that matches, not the longest. The "trailing ::" branch therefore has to +# come *after* every branch that continues past the ``::`` — with it listed +# early, ``2a00:1450:4864:20::32`` matched only as ``2a00:1450:4864:20::``, +# reporting a different, valid, routable address as the sender. REGXIP6 = re.compile( r"(?:(?:[0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}" # full form - r"|(?:[0-9a-fA-F]{1,4}:){1,7}:" # trailing :: - r"|(?:[0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}" # :: with 1 group after - r"|(?:[0-9a-fA-F]{1,4}:){1,5}(?::[0-9a-fA-F]{1,4}){1,2}" - r"|(?:[0-9a-fA-F]{1,4}:){1,4}(?::[0-9a-fA-F]{1,4}){1,3}" - r"|(?:[0-9a-fA-F]{1,4}:){1,3}(?::[0-9a-fA-F]{1,4}){1,4}" + r"|[0-9a-fA-F]{1,4}:(?::[0-9a-fA-F]{1,4}){1,6}" # 6 groups after :: r"|(?:[0-9a-fA-F]{1,4}:){1,2}(?::[0-9a-fA-F]{1,4}){1,5}" - r"|[0-9a-fA-F]{1,4}:(?::[0-9a-fA-F]{1,4}){1,6}" + r"|(?:[0-9a-fA-F]{1,4}:){1,3}(?::[0-9a-fA-F]{1,4}){1,4}" + r"|(?:[0-9a-fA-F]{1,4}:){1,4}(?::[0-9a-fA-F]{1,4}){1,3}" + r"|(?:[0-9a-fA-F]{1,4}:){1,5}(?::[0-9a-fA-F]{1,4}){1,2}" + r"|(?:[0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}" # 1 group after :: r"|:(?::[0-9a-fA-F]{1,4}){1,7}" # ::x:x... + r"|(?:[0-9a-fA-F]{1,4}:){1,7}:" # trailing :: r"|::)" # just :: ) @@ -75,6 +80,35 @@ # not collapse spaces, so this guard must live at the point of use. _WS_RUN_RE = re.compile(r"\s+") +# Matches the HELO/EHLO name an MTA records inside the ``from`` clause, +# in the two shapes seen in the wild: +# from evil.example ([203.0.113.9]:45321 helo=[8.8.8.8]) by ... (Exim) +# from [203.0.113.9] (account x@y HELO 8.8.8.8) by ... (CommuniGate) +# The HELO argument is chosen by the sender and may be an RFC 5321 address +# literal, so it must be removed before scanning the clause for the sender +# IP — otherwise the sender simply appends the IP they want reported. +# Used to locate the span, never to delete it: the trailing \S+ would +# otherwise swallow whatever follows, and a sender whose rDNS or HELO is +# literally ``helo`` can place that word right before the MTA-written IP. +# Two guards keep the span off MTA-written text. The lookbehind: the +# clause value *starts* with the HELO name, so at offset 0 the word is the +# name itself, not a label introducing one. The ``(?![\[(])`` on the +# space-separated form: an MTA writes its own address inside brackets or +# parentheses, so a following group is never part of the HELO argument. +# Exim's ``helo=[8.8.8.8]`` is a genuine bracketed HELO argument and keeps +# no such guard. +_HELO_RE = re.compile( + r"(?<=[(\s])e?helo\s*=\S*" # Exim "helo=value": after "(" or space + r"|(?<=\s)e?helo\s+\S+", # space form: only after whitespace + re.I, +) + +# RFC 5321 §4.1.3 tags an IPv6 literal as ``[IPv6:2a00:...]``. REGXIP6 +# happily starts matching at the ``6`` of the tag and yields +# ``6:2a00:1450:4864:20::`` — a different, valid, routable address that an +# analyst would then act on. Blank the tag before scanning. +_IPV6_TAG_RE = re.compile(r"IPv6:", re.I) + # Extracts envelope-from email: envelope-from _ENVELOPE_FROM_RE = re.compile(r"<([^>]+)>") @@ -108,3 +142,26 @@ "x-original-to", ] ) + +# Subset of OTHERS_PARTS that MailParser computes itself: each maps to a +# property, not to a header read off the wire. ``_make_mail()`` resolves +# only these names through attribute access; every other key it handles is +# a header name chosen by the sender and goes through +# ``MailParser._header_value()``, which never touches an attribute. Keep +# this set in sync with OTHERS_PARTS and with the properties in core.py. +# +# These names shadow a header of the same name in ``mail`` / ``mail_json``: +# a message carrying a literal ``Body:`` header reports the computed body +# there, not the header value. The wire value is never lost — it is in +# ``headers`` / ``headers_json`` under its own name — but a consumer +# reading only ``mail_json`` will not see it. +COMPUTED_PARTS = set( + [ + "attachments", + "body", + "date", + "received", + "timezone", + "to_domains", + ] +) diff --git a/src/mailparser/core.py b/src/mailparser/core.py index 5e9514b..5d02b09 100644 --- a/src/mailparser/core.py +++ b/src/mailparser/core.py @@ -23,19 +23,30 @@ import json import logging -from mailparser.const import ADDRESSES_HEADERS, EPILOGUE_DEFECTS, REGXIP, REGXIP6 +from mailparser.const import ( + _HELO_RE, + _IPV6_TAG_RE, + ADDRESSES_HEADERS, + COMPUTED_PARTS, + EPILOGUE_DEFECTS, + REGXIP, + REGXIP6, +) from mailparser.exceptions import MailParserRecursionError from mailparser.utils import ( _safe_attachment_filename, _safe_remove, convert_mail_date, decode_header_part, + decode_headers, extract_msg_convert, find_between, get_addresses, - get_header, + get_from_clause, get_mail_keys, get_to_domains, + group_spans, + in_spans, msgconvert, ported_open, ported_string, @@ -46,6 +57,12 @@ log = logging.getLogger(__name__) +# Keys ``_make_mail()`` sets itself after walking the headers. A sender can +# name a header after any of them, so they are skipped while collecting +# headers: otherwise ``mail["defects"]`` would hold a string from the wire +# instead of the list of parsed defects. +_RESERVED_MAIL_KEYS = frozenset({"defects", "defects_categories", "has_defects"}) + def parse_from_file_obj(fp): """ @@ -127,6 +144,10 @@ def __init__(self, message=None): Init a new object from a message object structure. """ self._message = message + # Set before parse() so that __getattr__ is never reached for this + # attribute — it resolves unknown names as headers instead of + # raising AttributeError, which would recurse. + self._header_index = {} if message is not None: log.debug("All headers of emails: {}".format(", ".join(message.keys()))) self.parse() @@ -296,6 +317,96 @@ def _reset(self): self._defects = [] self._defects_categories = set() self._has_defects = False + self._mail = {} + self._mail_partial = {} + self._header_index = self._build_header_index() + + def _build_header_index(self): + """ + Build a lowercased name -> list of raw values map of every header. + + ``Message.get_all()`` is a linear scan of the header list, so the + one call per distinct header name made by ``_make_mail()`` and + ``headers`` costs O(distinct x total). Header names are chosen by + the sender and are free to generate, which turns that quadratic + term into a denial of service (CWE-407): before this index, 16,000 + distinct names cost ~5.8 s of CPU against ~0.06 s for 32,000 + repeats of a single name. Indexing once makes every lookup O(1). + + Returns: + dict mapping a lowercased header name to its list of raw values + """ + index = {} + if self.message: + for name, value in self.message.items(): + index.setdefault(name.lower(), []).append(value) + return index + + def _header_value(self, name): + """ + Return the value of a header named exactly ``name``. + + This is the resolver for names that come off the wire. + ``_make_mail()`` and ``headers`` iterate the sender's header names, + so it does two things that ``__getattr__`` must not do here. + + It performs **no attribute lookup**. ``getattr(self, name)`` would + let the sender choose which attribute is read, because Python + resolves real class attributes before ``__getattr__``: a ``Parse:`` + header stored a bound method in the parsed mail and ``mail_json`` + raised ``TypeError`` (CWE-407), and a ``Headers_json:`` header + re-entered the ``headers`` property until the stack was exhausted. + + It also performs **no name rewriting**: the name is looked up + literally, with no ``_``/``-`` folding and no ``_json`` / ``_raw`` + suffix handling. Those conveniences exist for the caller's benefit + and are actively harmful on a sender-chosen name — ``Subject_json:`` + would report the value of ``Subject`` and drop its own (CWE-436), + and each ``_json`` suffix re-serialized the previous result, so + ``X_json_json...`` bought one doubling of the output per five input + bytes: a 148-byte header produced a 536 MB string (CWE-405). + + Args: + name (string): header name exactly as it appears in the message + + Returns: + list of (name, address) tuples for the address headers, + otherwise the decoded header value (str, or list when the + header repeats), or an empty str when the header is absent + """ + name_header = name.lower() + + # object headers + if name_header in ADDRESSES_HEADERS: + values = self._header_index.get(name_header) + # ``Message.get()`` semantics: only the first occurrence + raw_header = values[0] if values else "" + # Parse addresses. RFC 5322 §3.4 does not allow unquoted "@" in + # display names, so a strict parser correctly rejects headers like + # From: alice@example.com + # and returns ('', ''). mail-parser is a security/forensics tool, + # not an MTA: hiding addresses from analysts is worse than accepting + # non-conforming input. get_addresses() applies a regex fallback + # when strict parsing yields only empty results — see its docstring + # in utils.py for the full rationale. + parsed_addresses = get_addresses(raw_header) + + # decoded addresses — skip entries with no address (absent header) + return [ + ( + ( + "" + if (decoded_name := decode_header_part(name)) == email_addr + else decoded_name + ), + email_addr, + ) + for name, email_addr in parsed_addresses + if email_addr + ] + + # others headers + return decode_headers(self._header_index.get(name_header)) def _append_defects(self, part, part_content_type): """ @@ -342,7 +453,15 @@ def _make_mail(self, complete=True): for i in keys: log.debug(f"Getting header or part {i!r}") - value = getattr(self, i) + if i in _RESERVED_MAIL_KEYS: + # A header of this name would shadow the defect metadata + # below with a value of a different type. Still reachable + # by the caller as ``parser._raw``. + continue + # Only the computed parts are our own names and may be reached + # through attribute access; every other key is a header name + # chosen by the sender — see _header_value(). + value = getattr(self, i) if i in COMPUTED_PARTS else self._header_value(i) if value: mail[i] = value @@ -362,11 +481,18 @@ def parse(self): Instance of MailParser with raw email parsed """ - if not self.message: + # Reset first, so every attribute exists even when there is nothing + # to parse. Otherwise a property reading an unset attribute raises + # AttributeError, which __getattr__ silently answers as an absent + # header, and the caller sees "" instead of a failure. + self._reset() + + # ``Message.__len__`` is the header count, so a message with no + # headers at all is falsy: test against None, or a body-only + # message parses to nothing and reports no defect. + if self.message is None: return self - # reset and start parsing - self._reset() parts = [] # Normal parts plus defects # walk all mail parts to search defects @@ -554,11 +680,19 @@ def get_server_ipaddress(self, trust): In our case we trust only our mail server with the trust string. + The walk continues only while a trusted hop names a *private* IP, + which marks an internal relay. A trusted hop naming no IP at all + ends the search with ``None``: extraction failing there used to + drop the loop into older Received headers, which the sender wrote, + so every trick that defeats extraction on the genuine hop returned + an attacker-chosen IP instead of nothing (CWE-345). + Args: trust (string): String that identify our mail server Returns: - string with the ip address + string with the ip address, or None when no trusted hop names + a public sender IP """ log.debug(f"Trust string is {trust!r}") @@ -572,35 +706,115 @@ def get_server_ipaddress(self, trust): for i in received: i = ported_string(i) - if trust in i: - log.debug(f"Trust string {trust!r} is in {i!r}") - ip_str = self._extract_ip(i) - if ip_str: - return ip_str + if trust not in i: + continue + + log.debug(f"Trust string {trust!r} is in {i!r}") + candidates = self._sender_ip_candidates(i) + if not candidates: + # Nothing to read on an authoritative hop: stop, never fall + # through to a header the sender controls. + log.debug("Trusted hop names no sender IP, stopping") + return None - def _extract_ip(self, received_header): - """ - Extract the IP address from the received header if it is not private. - Supports both IPv4 (RFC 791) and IPv6 (RFC 5952) addresses. + ip_str = self._public_ip(candidates) + if ip_str: + return ip_str + # Private IP: an internal relay, keep following the chain. + + def _sender_ip_candidates(self, received_header): + """ + Return the IP addresses named in the ``from`` clause of a header. + + Only the ``from`` clause is searched: the ``by`` clause holds the + receiving server, whose IP must never be reported as the sender's. + The clause is isolated with ``get_from_clause()``, which anchors on + the RFC 5321 keywords — see its docstring for why a substring + search for ``"by"`` is spoofable (CWE-345). + + Within the clause exactly two regions are sender-written, and both + are excluded rather than trusted to look harmless: + + * **the first token** — the HELO name. It may be an address + literal (``EHLO [8.8.8.8]``, the form RFC 5321 §4.1.3 requires of + a client with no FQDN), so it is never a candidate. + * **an explicit HELO argument** inside a comment — Exim's + ``(helo=x)``, CommuniGate's ``(account a@b HELO x)``. + + Everything else was written by the receiving MTA, and a candidate + must additionally sit inside a ``(``/``[`` group, which is where an + MTA puts its own findings. That last rule is what makes a + truncated clause fail closed: a multi-word HELO can end the clause + early, but whatever it leaves behind is bare and so is not a + candidate. + + The one concession is the Exim/CommuniGate layout ``from [ip] + (helo=x)``, where the MTA writes the address as the first token + precisely because it recorded the HELO name separately. It is + accepted only when no other candidate exists and the HELO marker + is itself inside a group — the residual is a sender who writes + comment syntax into their own HELO name *and* whose MTA records it + verbatim, on a hop naming no other address. Args: received_header (string): The received header string Returns: - string with the ip address or None + list of IP address strings, in the order they appear + """ + # Blank the "IPv6:" tag with a non-space filler of the same width: + # offsets stay aligned, and no new token boundary or _HELO_RE + # lookbehind position is created inside the sender's own token. + from_part = _IPV6_TAG_RE.sub("_____", get_from_clause(received_header)) + if not from_part: + return [] + + groups = group_spans(from_part) + helo_spans = [ + m.span() + for m in _HELO_RE.finditer(from_part) + if in_spans(m.start(), groups) + ] + first_token_end = len(from_part.split(" ", 1)[0]) + + # Merge both families positionally. Choosing IPv4 over IPv6 by + # family let one private literal at EHLO suppress the IPv6 scan and + # hide the real sender. + matches = sorted( + list(REGXIP.finditer(from_part)) + list(REGXIP6.finditer(from_part)), + key=lambda m: m.start(), + ) + matches = [m for m in matches if not in_spans(m.start(), helo_spans)] + + candidates = [ + m.group() + for m in matches + if m.start() >= first_token_end and in_spans(m.start(), groups) + ] + if candidates: + return candidates + + if helo_spans: + # Exim/CommuniGate: the address is the first token because the + # HELO name was recorded in the comment instead. + return [m.group() for m in matches if m.start() < first_token_end] + + return [] + + def _public_ip(self, check): """ - by_idx = received_header.find("by") - from_part = received_header[:by_idx] if by_idx != -1 else received_header + Return the last candidate address, if it parses and is public. - # Try IPv4 first, then IPv6 - check = REGXIP.findall(from_part) - if not check: - check = REGXIP6.findall(from_part) + Args: + check (list): IP address strings from a ``from`` clause + Returns: + string with the ip address or None + """ if check: try: ip_str = str(check[-1]) - log.debug(f"Found sender IP {ip_str!r} in {received_header!r}") + log.debug(f"Found sender IP {ip_str!r}") ip = ipaddress.ip_address(ip_str) except ValueError: return None @@ -610,6 +824,19 @@ def _extract_ip(self, received_header): return ip_str return None + def _extract_ip(self, received_header): + """ + Extract the sender IP from a received header if it is not private. + Supports both IPv4 (RFC 791) and IPv6 (RFC 5952) addresses. + + Args: + received_header (string): The received header string + + Returns: + string with the ip address or None + """ + return self._public_ip(self._sender_ip_candidates(received_header)) + def write_attachments(self, base_path): """This method writes the attachments of mail on disk @@ -619,50 +846,39 @@ def write_attachments(self, base_path): write_attachments(attachments=self.attachments, base_path=base_path) def __getattr__(self, name): - name = name.strip("_").lower() - name_header = name.replace("_", "-") + """ + Expose any header dynamically as ``X``, ``X_json`` or ``X_raw``. - # json headers - if name.endswith("_json"): - name = name[:-5] - return json.dumps(getattr(self, name), ensure_ascii=False) + Underscores in ``name`` stand for dashes, so ``mail.X_MSMail_Priority`` + reads the ``X-MSMail-Priority`` header. Only reached for names the + *caller* asks for, so ``X_json`` may resolve through ``getattr`` and + reach the computed parts — ``attachments_json`` and friends. Names + taken from a parsed message must never come through here; + ``_header_value()`` serves those, and its docstring explains why. - # raw headers - elif name.endswith("_raw"): - name = name[:-4] - raw = self.message.get_all(name) if self.message else None - return json.dumps(raw, ensure_ascii=False) + Raises: + AttributeError: for private names. Without this, an unset + internal attribute is answered as an absent header and a + bug inside a property is silently reported as "". + """ + if name.startswith("_"): + raise AttributeError( + f"{type(self).__name__!r} object has no attribute {name!r}" + ) - # object headers - elif name_header in ADDRESSES_HEADERS: - raw_header = self.message.get(name_header, "") if self.message else "" - # Parse addresses. RFC 5322 §3.4 does not allow unquoted "@" in - # display names, so a strict parser correctly rejects headers like - # From: alice@example.com - # and returns ('', ''). mail-parser is a security/forensics tool, - # not an MTA: hiding addresses from analysts is worse than accepting - # non-conforming input. get_addresses() applies a regex fallback - # when strict parsing yields only empty results — see its docstring - # in utils.py for the full rationale. - parsed_addresses = get_addresses(raw_header) + name = name.rstrip("_").lower() - # decoded addresses — skip entries with no address (absent header) - return [ - ( - ( - "" - if (decoded_name := decode_header_part(name)) == email_addr - else decoded_name - ), - email_addr, - ) - for name, email_addr in parsed_addresses - if email_addr - ] + if name.endswith("_json"): + return json.dumps(getattr(self, name[:-5]), ensure_ascii=False) - # others headers - else: - return get_header(self.message, name_header) + if name.endswith("_raw"): + raw = self._header_index.get(name[:-4].replace("_", "-")) or [] + # Values are ``email.header.Header`` when the header carries + # 8-bit bytes (the from_bytes path); dumping one raises + # TypeError and kills the parse, so coerce to str first. + return json.dumps([ported_string(i) for i in raw], ensure_ascii=False) + + return self._header_value(name.replace("_", "-")) @property def attachments(self): @@ -714,10 +930,25 @@ def body(self): @property def headers(self) -> dict: """ - Return only the headers as Python object - """ - all_headers = set(self.message.keys() if self.message else []) - {"headers"} - return {i: getattr(self, i) for i in all_headers} + Return only the headers as Python object. + + Values resolve through ``_header_value()``, never ``getattr``: the + keys come straight from the sender, so attribute lookup on them + returns bound methods (``Parse:``) or re-enters this very property + (``Headers_json:``). Excluding single names from the key set is + not a fix — it is what let ``Headers_json`` through after + ``Headers`` was excluded. + """ + # Dedupe case-insensitively, keeping the first spelling seen as the + # key. Header names are compared case-insensitively, so _header_value + # returns every occurrence for each of them: deduping on the exact + # spelling instead would emit one full value list per casing, and a + # sender repeating one 20-char name in n casings would get an n x n + # blowup (CWE-407) — 271 KB of headers cost 7.6 s and 540 MB of JSON. + names = {} + for i in self.message.keys() if self.message else []: + names.setdefault(i.lower(), i) + return {i: self._header_value(i) for i in names.values()} @property def headers_json(self): diff --git a/src/mailparser/utils.py b/src/mailparser/utils.py index 7157c99..de4c773 100644 --- a/src/mailparser/utils.py +++ b/src/mailparser/utils.py @@ -547,6 +547,104 @@ def msgconvert(email): return temp, stdoutdata.decode("utf-8", errors="replace").strip() +def get_from_clause(received): + """ + Return the value of the ``from`` clause of a Received header. + + The sender address lives in the ``from`` clause; the ``by`` clause + names the *receiving* server and must never be mistaken for it. + Clause boundaries are located with the anchored RFC 5321 tokenizer + ``_CLAUSE_SPLITTER`` rather than a substring search, because a plain + ``received.find("by")`` also matches inside a hostname — ``derby``, + ``nearby`` — and the hostname comes from the sender's HELO. A false + match truncates the clause, extraction fails on the genuine hop, and + the caller falls through to older attacker-forged Received headers + (CWE-345). A word-boundary ``\\bby\\b`` is not enough either: ``.`` + is a non-word character, so it still matches in ``host.by.example``. + + Args: + received (string): raw Received header value + + Returns: + string with the ``from`` clause value, or an empty string when the + header has no ``from`` clause + """ + # Collapse whitespace runs before splitting, so the splitter stays + # linear — see the note on _WS_RUN_RE in const.py. + header = _WS_RUN_RE.sub(" ", received) + + # split() yields [preamble, keyword, value, keyword, value, ...] + parts = _CLAUSE_SPLITTER.split(header) + keywords = [(i, parts[i].lower()) for i in range(1, len(parts) - 1, 2)] + + start = next((i for i, kw in keywords if kw == "from"), None) + if start is None: + # No ``from`` clause: attribution fails closed. Returning the whole + # header instead would surface an IP taken from the ``by``, ``for``, + # ``with`` or ``id`` clause as the sender's — and the ``for`` clause + # holds the envelope recipient, which the sender picks at RCPT TO + # (``victim+8.8.8.8@example.com``). + return str() + + # The clause ends at the next keyword, full stop. Extending it to a + # later ``by`` to survive a multi-word HELO drags the ``by``, ``for`` + # and ``envelope-from`` values into the result, and the last two are + # sender-chosen: a quoted local part supplies the whitespace, so + # ``MAIL FROM:<"x 8.8.8.8 by q"@evil.example>`` puts an attacker IP + # into the sender-attribution scan (CWE-345). A multi-word HELO does + # truncate this clause, but that only costs the candidates — the caller + # then fails closed rather than reporting an attacker-chosen address. + return parts[start + 1].strip() + + +def group_spans(text): + """ + Return the spans of ``text`` enclosed in a ``(`` or ``[`` group. + + A single left-to-right pass, so the caller can classify many positions + without re-scanning the prefix for each one. Nesting is tracked with a + depth counter and an unclosed group runs to the end of the string. + + Args: + text (string): text to scan + + Returns: + list of (start, end) tuples, in order, excluding the delimiters + """ + spans = [] + depth = 0 + start = 0 + + for i, char in enumerate(text): + if char in "([": + if depth == 0: + start = i + 1 + depth += 1 + elif char in ")]" and depth: + depth -= 1 + if depth == 0: + spans.append((start, i)) + + if depth: + spans.append((start, len(text))) + + return spans + + +def in_spans(position, spans): + """ + Return True when ``position`` falls inside one of ``spans``. + + Args: + position (int): offset to test + spans (list): (start, end) tuples + + Returns: + bool + """ + return any(start <= position < end for start, end in spans) + + def parse_received(received): """ Parse a single received header by tokenizing on RFC 5321 §4.4 keywords. @@ -803,6 +901,30 @@ def get_to_domains(to=[], reply_to=[]): return list(domains) +def decode_headers(headers): + """ + Decode the raw values of a single header name with the correct charset. + + Args: + headers (list): raw values of one header name, or None if absent + + Returns: + str if there is one value + list if there are more than one + empty str if the header is absent + """ + + if not headers: + return str() + + decoded = [decode_header_part(i) for i in headers] + if len(decoded) == 1: + # in this case return a string + return decoded[0].strip() + # in this case return a list + return decoded + + def get_header(message, name): """ Gets an email.message.Message and a header name and returns @@ -819,14 +941,7 @@ def get_header(message, name): headers = message.get_all(name) log.debug(f"Getting header {name!r}: {headers!r}") - if headers: - headers = [decode_header_part(i) for i in headers] - if len(headers) == 1: - # in this case return a string - return headers[0].strip() - # in this case return a list - return headers - return str() + return decode_headers(headers) def get_mail_keys(message, complete=True): diff --git a/tests/test_mail_parser.py b/tests/test_mail_parser.py index cbfec0f..5f8248e 100644 --- a/tests/test_mail_parser.py +++ b/tests/test_mail_parser.py @@ -18,26 +18,32 @@ import datetime import hashlib +import json import logging import os import shutil import sys import tempfile +import time import unittest from unittest.mock import patch import pytest import mailparser +from mailparser.const import REGXIP6 from mailparser.exceptions import MailParserOSError, MailParserRecursionError from mailparser.utils import ( convert_mail_date, extract_msg_convert, fingerprints, get_addresses, + get_from_clause, get_header, get_mail_keys, get_to_domains, + group_spans, + in_spans, parse_received, ported_open, ported_string, @@ -787,6 +793,11 @@ def test_write_uuencode_attachment(self): self.assertEqual(md5.hexdigest(), "4f2cf891e7cfb349fca812091f184ecc") def test_issue_139(self): + # mail_test_16 carries a literal "headers: hello-world" header. It + # used to re-enter the headers property, and the first fix dropped + # the name from the key set. Header values no longer resolve + # through attribute lookup, so the recursion is impossible and the + # header is reported like any other instead of being hidden. mail = mailparser.parse_from_file(mail_test_16) assert mail.headers == { "MIME-Version": "1.0", @@ -798,6 +809,7 @@ def test_issue_139(self): "Message-ID": "", "Subject": "Test spam mail (GTUBE)", "To": [("Recipient", "recipient@example.net")], + "headers": "hello-world", } def test_issue_136(self): @@ -1123,29 +1135,27 @@ def test_sender_ip_no_message(self): self.assertIsNone(result) def test_extract_ip_ipv6_fallback(self): - """Test core.py:531 — _extract_ip uses IPv6 when IPv4 not found""" + """Test _extract_ip uses IPv6 when IPv4 not found""" + # 2001:db8::/32 is the documentation range, which Python reports as + # private, so it cannot stand in for a routable sender here. This + # test used to use it and passed only because REGXIP6 matched inside + # the "IPv6:" tag and returned the unrelated "6:2001:db8::". raw_mail = ( - "Received: from sender.example.com (IPv6:2001:db8::1)\n" + "Received: from sender.example.com (IPv6:2a00:1450:4864:20::32)\n" " by mail.trusted.net; Mon, 01 Jan 2024 12:00:00 +0000\n" "From: test@example.com\n" "Subject: IPv6 test\n\nBody" ) mail = mailparser.parse_from_string(raw_mail) - # 2001:db8:: is documentation range — it is not private result = mail.get_server_ipaddress("trusted.net") - # Should find the IPv6 address (it is globally routable) - self.assertIsNotNone(result) + # the whole address, and not a fragment of the tag before it + self.assertEqual(result, "2a00:1450:4864:20::32") - def test_extract_ip_invalid_ip_returns_none(self): - """Test core.py:538-539 — _extract_ip returns None for unparsable IP string""" + def test_public_ip_returns_none_for_unparsable_candidate(self): + """_public_ip returns None when the candidate is not an IP address""" parser = mailparser.parse_from_string("From: t@example.com\nSubject: x\n\nBody") - # Patch REGXIP to return a value that ipaddress.ip_address() cannot parse - with patch("mailparser.core.REGXIP") as mock_regxip: - with patch("mailparser.core.REGXIP6") as mock_regxip6: - mock_regxip.findall.return_value = [] - mock_regxip6.findall.return_value = ["not_a_valid_ip"] - result = parser._extract_ip("from invalid by host") - self.assertIsNone(result) + self.assertIsNone(parser._public_ip(["not_a_valid_ip"])) + self.assertIsNone(parser._public_ip([])) def test_extract_ip_private_ip_returns_none(self): """Test core.py:544 — _extract_ip returns None when IP is private""" @@ -1558,3 +1568,581 @@ def test_body_bytes_payload_decoded_with_charset(): """ m = mailparser.MailParser(_text_message_returning(b"caf\xe9")) assert m.text_plain == ["café"] + + +# --------------------------------------------------------------------- # +# Regression tests for the header-name attack surface. +# +# Every header name in a message is chosen by the sender, and MailParser +# exposes headers as attributes. Resolving a name off the wire through +# getattr() therefore let the sender pick which Python attribute was read. +# These tests pin the four resulting defects shut. +# --------------------------------------------------------------------- # + + +def _timed_parse(raw): + """Parse ``raw`` and return the elapsed wall-clock seconds.""" + start = time.perf_counter() + mailparser.parse_from_string(raw) + return time.perf_counter() - start + + +def test_distinct_header_names_scale_linearly(): + """ + Parsing cost must stay linear in the number of *distinct* header names. + + Every distinct name used to trigger its own full rescan of the header + list (Message.get_all is O(total)), so cost grew as O(distinct x total): + 16,000 names cost ~5.8 s against ~0.06 s for 32,000 repeats of one name. + Header names are attacker-chosen and cheap to generate (CWE-407). + """ + + def build(n): + headers = "".join(f"X{i:05d}: v\r\n" for i in range(n)) + return f"From: a@b.c\r\n{headers}\r\nbody\r\n" + + small = min(_timed_parse(build(2000)) for _ in range(3)) + large = min(_timed_parse(build(8000)) for _ in range(3)) + + # 4x the names must not cost anywhere near 16x the time. The bound is + # loose so the test does not flake on a loaded machine; the quadratic + # behaviour it guards against was ~14x here. + assert large < small * 8, f"{small=} {large=} — scaling is not linear" + + +def test_header_named_after_a_method_does_not_break_mail_json(): + """ + A header named after a MailParser method must not put that method into + the parsed mail. Python resolves real attributes before __getattr__, so + "Parse: x" used to store a bound method and mail_json raised + "TypeError: Object of type method is not JSON serializable" — a crash + DoS from a 12-byte message (CWE-407). + """ + colliding = [ + name for name in dir(mailparser.MailParser) if not name.startswith("_") + ] + assert "parse" in colliding, "sanity: the sweep must cover real methods" + + for name in colliding: + mail = mailparser.parse_from_string(f"{name}: x\r\n\r\n") + # must not raise + json.loads(mail.mail_json) + json.loads(mail.mail_partial_json) + + +def test_header_named_parse_is_reported_as_a_header(): + """The colliding name resolves to the header value, not to the method.""" + mail = mailparser.parse_from_string("Parse: x\r\n\r\n") + assert mail.mail["parse"] == "x" + + +def test_header_named_headers_json_does_not_recurse(): + """ + "Headers_json: x" used to drive unbounded recursion: the headers property + resolved the name through getattr, reaching the headers_json property, + which re-serialized headers again. Each cycle rebuilt the whole header + dict, so the cost compounded with the per-name rescan — a 32 KB message + burned ~48 s of CPU before the recursion guard aborted the parse. + + Excluding the name from the key set is not the fix and is not tested for: + that is what the earlier "headers" exclusion did, and "headers_json" + walked straight past it. + """ + filler = "".join(f"Z{i:04d}: v\r\n" for i in range(3200)) + raw = f"Headers_json: x\r\n{filler}\r\nbody\r\n" + + elapsed = _timed_parse(raw) + assert elapsed < 2, f"parse took {elapsed:.1f}s — recursion is back" + + +def test_header_named_headers_json_variants_do_not_recurse(): + """The alias and its double-suffixed form must be inert too.""" + for name in ("headers", "Headers", "Headers_json", "Headers_json_json"): + mail = mailparser.parse_from_string(f"{name}: x\r\n\r\nbody\r\n") + json.loads(mail.headers_json) + + +def test_reserved_defect_keys_are_not_shadowed_by_headers(): + """ + A header named after a defect output key must not replace the parsed + value with a string from the wire. + """ + raw = "Defects: x\r\nDefects_categories: y\r\nHas_defects: z\r\n\r\nbody\r\n" + mail = mailparser.parse_from_string(raw) + assert mail.mail["has_defects"] is False + assert "defects" not in mail.mail + + +# --------------------------------------------------------------------- # +# Regression tests for sender-IP attribution. +# --------------------------------------------------------------------- # + +_TRUST = "mx.victim.com" + +# Genuine top hop written by the trusted MTA. The hostname comes from the +# sender's HELO, so the sender controls it without controlling any DNS. +_GENUINE = ( + "Received: from {host} ({host} [{ip}])\r\n" + "\tby mx.victim.com (Postfix) with ESMTP id ABC\r\n" + "\tfor ; Mon, 1 Jan 2024 00:00:00 +0000\r\n" +) + +# Older hop, entirely attacker-authored, that also carries the trust string. +_FORGED = ( + "Received: from fake.example (fake.example [6.6.6.6])\r\n" + "\tby mx.victim.com (Postfix) with ESMTP id XYZ;" + " Mon, 1 Jan 2024 00:00:00 +0000\r\n" +) + + +def _sender_ip(*headers): + raw = "".join(headers) + "From: a@b.c\r\n\r\nbody\r\n" + return mailparser.parse_from_string(raw).get_server_ipaddress(_TRUST) + + +def test_sender_ip_not_spoofable_by_hostname_containing_by(): + """ + A HELO hostname containing "by" must not defeat sender attribution. + + get_server_ipaddress located the "by" clause with a substring search, so + "derby.attacker.com" truncated the from clause at the "by" inside the + hostname. Extraction then failed on the genuine hop and the loop fell + through to the older forged header — returning the attacker's chosen IP + rather than nothing (CWE-345). + """ + genuine = _GENUINE.format(host="derby.attacker.com", ip="1.2.3.4") + assert _sender_ip(genuine, _FORGED) == "1.2.3.4" + + +def test_sender_ip_control_matrix(): + """ + Both conditions were needed, and the same flaw misattributes honest + senders — so this is a correctness bug as well as an attack primitive. + """ + benign = _GENUINE.format(host="real.attacker.com", ip="1.2.3.4") + evil = _GENUINE.format(host="derby.attacker.com", ip="1.2.3.4") + + # forged header alone is not reached + assert _sender_ip(benign, _FORGED) == "1.2.3.4" + # malicious hostname alone must still resolve correctly + assert _sender_ip(evil) == "1.2.3.4" + # an ordinary hostname that merely contains "by" used to return None + assert _sender_ip(_GENUINE.format(host="nearby.example.org", ip="9.9.9.9")) == ( + "9.9.9.9" + ) + + +def test_sender_ip_ignores_the_receiving_server(): + """ + The IP of the "by" server must never be returned as the sender's: only + the from clause is searched. + """ + received = ( + "Received: from helo.example (helo.example)\r\n" + "\tby mx.victim.com (Postfix [8.8.8.8]) with ESMTP id ABC;" + " Mon, 1 Jan 2024 00:00:00 +0000\r\n" + ) + assert _sender_ip(received) is None + + +def test_get_from_clause_is_anchored(): + """get_from_clause splits on RFC 5321 keywords, not on substrings.""" + assert ( + get_from_clause("from derby.example.com (derby.example.com [1.2.3.4]) by mx") + == "derby.example.com (derby.example.com [1.2.3.4])" + ) + # no from clause: attribution fails closed rather than widening the + # search to the by / for / with / id clauses + assert get_from_clause("by mx.victim.com\r\n\twith ESMTP") == "" + + +def test_json_suffixed_header_name_does_not_amplify(): + """ + A header named ``X_json_json_...`` must not build a tower of JSON. + + Applying the caller-facing ``_json`` suffix to a wire name made each + level re-serialize the previous one, and json.dumps escapes every quote + and backslash: the value doubled per five input bytes. A 148-byte + header produced a 536 MB string, and 40 such headers reached tens of GB + — an out-of-memory kill of the worker (CWE-405). + """ + name = "X" + "_json" * 40 + raw = "".join(f"{name}{i}: x\r\n" for i in range(40)) + "\r\nbody\r\n" + + elapsed = _timed_parse(raw) + assert elapsed < 2, f"parse took {elapsed:.1f}s — the suffix cycle is back" + + mail = mailparser.parse_from_string(f"{name}: x\r\n\r\n") + # the wire value, not a JSON tower built out of it + assert mail.mail[name.lower()] == "x" + + +def test_wire_header_names_are_not_rewritten(): + """ + A header name is reported and looked up literally. + + Folding ``_`` to ``-`` and honouring the ``_json`` / ``_raw`` suffixes + on a sender-chosen name silently replaced one header's value with + another's and dropped the original — indicators vanished from every + output surface with no defect recorded (CWE-436). + """ + raw = ( + "From: Real Sender \r\n" + "Subject: innocuous\r\n" + "Subject_json: MALWARE-C2 http://evil.example/payload.exe\r\n" + "X_Spam_Flag: YES\r\n" + "\r\nbody\r\n" + ) + mail = mailparser.parse_from_string(raw) + + assert mail.mail["subject"] == "innocuous" + assert mail.mail["subject_json"] == "MALWARE-C2 http://evil.example/payload.exe" + assert mail.headers["X_Spam_Flag"] == "YES" + assert "MALWARE-C2" in mail.mail_json + assert "MALWARE-C2" in mail.headers_json + + +def test_raw_suffix_survives_eight_bit_headers(): + """ + ``X_raw`` must not crash on a header carrying 8-bit bytes. + + compat32 hands back an ``email.header.Header`` rather than a str for + those, and json.dumps raised "Object of type Header is not JSON + serializable" — an exception outside the MailParser hierarchy, so + _parse_guarded did not catch it and the worker died (CWE-248). + """ + mail = mailparser.parse_from_bytes(b"Subject: caf\xe9\r\n\r\nbody\r\n") + subject_raw = mail.subject_raw + assert isinstance(subject_raw, str) + # the undecodable byte survives as U+FFFD rather than killing the parse + assert json.loads(subject_raw) == ["caf�"] + + # the same value reached through a crafted wire name must not crash + crafted = mailparser.parse_from_bytes( + b"Subject: caf\xe9\r\nSubject_raw: x\r\n\r\nbody\r\n" + ) + assert crafted.mail["subject_raw"] == "x" + json.loads(crafted.mail_json) + + +def test_private_attributes_raise_instead_of_resolving_as_headers(): + """ + An unset internal attribute must fail loudly. + + __getattr__ answered any missing name as an absent header, so a bug + inside a property surfaced as "" instead of an error. + """ + mail = mailparser.parse_from_string("Subject: x\r\n\r\nbody\r\n") + with pytest.raises(AttributeError): + mail._not_a_real_attribute + # the documented trailing-underscore form still works + assert mail.from_ == [] + + +def test_message_without_headers_is_parsed(): + """ + A body-only message must still parse. + + ``Message.__len__`` is the header count, so a message with no headers + is falsy: ``if not self.message`` returned before _reset(), leaving + _mail and _text_plain unset, and __getattr__ then answered them as + absent headers. Every property returned "" — a pipeline grepping the + body for indicators saw nothing and no error (CWE-754). + """ + mail = mailparser.parse_from_string("Click http://evil.example/pay.exe now\r\n") + assert "evil.example" in mail.body + assert isinstance(mail.mail, dict) + assert isinstance(mail.attachments, list) + assert mail.has_defects is True + + +def test_sender_ip_ignores_the_helo_name(): + """ + The HELO/EHLO name is sender-supplied and may be an address literal, + so it must be stripped before scanning the from clause — the last IP + in the clause wins, and both Exim and CommuniGate record the HELO + after the genuine IP. + """ + exim = ( + "Received: from evil.example ([93.184.216.34]:45321 helo=[8.8.8.8])\r\n" + "\tby mx.victim.com with esmtps id ABC;" + " Mon, 1 Jan 2024 00:00:00 +0000\r\n" + ) + communigate = ( + "Received: from [93.184.216.34] (account bounce@tin.it HELO 8.8.8.8)\r\n" + "\tby mx.victim.com (CommuniGate) with ESMTP id ABC;" + " Mon, 1 Jan 2024 00:00:00 +0000\r\n" + ) + assert _sender_ip(exim) == "93.184.216.34" + assert _sender_ip(communigate) == "93.184.216.34" + + +def test_sender_ip_fails_closed_on_the_trusted_hop(): + """ + When the trusted hop yields no public sender IP the answer is None. + + Continuing to older Received headers hands the result to whoever wrote + them — the sender — so any future trick that defeats extraction on the + genuine hop would again return an attacker-chosen IP instead of + nothing (CWE-345). + """ + # a trusted hop with no from clause at all names nothing readable, and + # its envelope recipient is chosen by the sender at RCPT TO + no_from = ( + "Received: by mx.victim.com (Postfix) id 9F2\r\n" + "\tfor ; Mon, 1 Jan 2024 00:00:00 +0000\r\n" + ) + assert _sender_ip(no_from) is None + + +def test_headers_dedupes_case_variants_of_one_name(): + """ + Repeating one header name in many casings must not blow up ``headers``. + + Header names compare case-insensitively, so every casing resolves to the + same full value list. Deduping on the exact spelling emitted one list + per casing — an n x n blowup on a name the sender chooses (CWE-407): + 271 KB of headers cost 7.6 s and produced 540 MB of JSON. + """ + name = "x-aaaaaaaaaaaaaaaaaaa" + variants = ( + "".join(c.upper() if (i >> j) & 1 else c for j, c in enumerate(name)) + for i in range(8000) + ) + raw = "".join(f"{v}: value\r\n" for v in variants) + "\r\nbody\r\n" + + mail = mailparser.parse_from_string(raw) + start = time.perf_counter() + headers = mail.headers + elapsed = time.perf_counter() - start + + assert len(headers) == 1 + assert elapsed < 2, f"headers took {elapsed:.1f}s — the blowup is back" + + # honest duplicates keep every value under one key + assert mailparser.parse_from_string("Subject: a\r\nSUBJECT: b\r\n\r\n").headers == { + "Subject": ["a", "b"] + } + + +def test_multi_token_helo_never_steers_the_result(): + """ + A HELO of several words lets the sender write an RFC 5321 clause keyword + into the trusted MTA's own Received header, truncating the from clause. + + The answer must never be the sender's chosen address. Extending the + clause to a later ``by`` to survive this is worse than the truncation: + it pulls in the ``for`` and ``envelope-from`` values, which the sender + also picks — so truncation is left to fail closed (CWE-345). + """ + assert _sender_ip(_GENUINE.format(host="evil.example", ip="1.2.3.4")) == "1.2.3.4" + + for helo in ( + "evil 6.6.6.6 by z", + "evil 6.6.6.6 with z", + "evil 6.6.6.6 for z", + "a by b by c 6.6.6.6 by d", + ): + assert _sender_ip(_GENUINE.format(host=helo, ip="1.2.3.4"), _FORGED) is None, ( + f"HELO {helo!r} steered the result" + ) + + +def test_sender_ip_ignores_addresses_the_sender_wrote(): + """ + Only an address the receiving MTA wrote — inside a bracket or paren + group — can be the sender's. The ``for`` and ``envelope-from`` values + are chosen by the sender at RCPT TO / MAIL FROM, and a quoted local part + supplies the whitespace needed to look like a clause (CWE-345). + """ + exim = ( + "Received: from [45.33.32.156] (helo=evil.example)\r\n" + "\tby mx.victim.com with esmtp (Exim 4.94)\r\n" + '\t(envelope-from <"x 6.6.6.6 by q"@evil.example>)\r\n' + "\tid 1abc for v@victim.com; Mon, 1 Jan 2024 00:00:00 +0000\r\n" + ) + assert _sender_ip(exim, _FORGED) == "45.33.32.156" + + # the same trick aimed at the receiving server's own address + receiving = ( + "Received: from helo.example (helo.example [45.33.32.156])\r\n" + "\tby mx.victim.com (Postfix [6.6.6.6]) with ESMTP id ABC\r\n" + '\tfor <"v by q"@victim.com>; Mon, 1 Jan 2024 00:00:00 +0000\r\n' + ) + assert _sender_ip(receiving) == "45.33.32.156" + + +def test_ipv4_helo_literal_does_not_hide_an_ipv6_sender(): + """ + An address literal announced at EHLO is one bare token, never inside a + group, so it is not a candidate. Scanning IPv4 first and stopping on + any hit meant ``EHLO 10.0.0.1`` on an IPv6 connection produced a single + private candidate: attribution then resumed the walk into the sender's + own forged headers. + """ + genuine = ( + "Received: from 10.0.0.1 (evil.example [IPv6:2a00:1450:4864:20::32])\r\n" + "\tby mx.victim.com (Postfix) with ESMTPS id ABC" + " for ; Mon, 1 Jan 2024 00:00:00 +0000\r\n" + ) + assert _sender_ip(genuine, _FORGED) == "2a00:1450:4864:20::32" + + +def test_sender_ip_reads_real_world_clause_layouts(): + """The MTA layouts the candidate filter has to keep working on.""" + cases = { + # sendmail, unverified rDNS + "from 8.8.8.8 (helo [45.33.32.156] (may be forged))": "45.33.32.156", + # a sender whose HELO name is literally the word "helo" + "from helo ([45.33.32.156])": "45.33.32.156", + # Exim / CommuniGate: the MTA writes the address as the first token + # because it recorded the HELO name in the comment instead + "from [45.33.32.156] (helo=evil.example)": "45.33.32.156", + "from [45.33.32.156] (account a@b HELO evil.example)": "45.33.32.156", + "from evil ([45.33.32.156]:45321 helo=[8.8.8.8])": "45.33.32.156", + # a bare address with no HELO marker anywhere is indistinguishable + # from "EHLO 139.88.66.159", so it fails closed + "from 139.88.66.159": None, + } + for clause, expected in cases.items(): + received = ( + f"Received: {clause}\r\n" + "\tby mx.victim.com (Postfix) id ABC;" + " Mon, 1 Jan 2024 00:00:00 +0000\r\n" + ) + assert _sender_ip(received) == expected, clause + + +def test_sender_ip_ignores_an_address_literal_announced_at_ehlo(): + """ + ``EHLO [8.8.8.8]`` is the form RFC 5321 §4.1.3 requires of a client with + no FQDN, so the first token of a from clause may be a bracketed address + the sender chose. It is never a candidate: a closed bracket pair the + sender wrote is byte-identical to one the MTA wrote, so the rule is + positional — the first token is the HELO name, whatever it looks like. + """ + v6 = "2a00:1450:4864:20::32" + for helo in ("[8.8.8.8]", "[10.0.0.1]", "[203.0.113.77]"): + genuine = ( + f"Received: from {helo} (evil.example [IPv6:{v6}])\r\n" + "\tby mx.victim.com (Postfix) with ESMTPS id ABC;" + " Mon, 1 Jan 2024 00:00:00 +0000\r\n" + ) + assert _sender_ip(genuine, _FORGED) == v6, helo + + # with nothing else in the clause the answer is None, never the literal + alone = ( + "Received: from [8.8.8.8] (unknown)\r\n" + "\tby mx.victim.com (Postfix) id ABC; Mon, 1 Jan 2024 00:00:00 +0000\r\n" + ) + assert _sender_ip(alone, _FORGED) is None + + +def test_sender_ip_rejects_a_helo_marker_the_sender_injected(): + """ + The HELO-marker concession applies only to a marker inside a comment + group, where the MTA puts it. A marker the sender types into their own + HELO name sits at the top level and must not promote their address. + """ + injected = ( + "Received: from 8.8.8.8 helo=x\r\n" + "\tby mx.victim.com (Postfix) id ABC; Mon, 1 Jan 2024 00:00:00 +0000\r\n" + ) + assert _sender_ip(injected, _FORGED) is None + + +def test_regxip6_matches_the_whole_address(): + """ + Python's ``re`` takes the first matching alternative, not the longest, + so the compressed branches are ordered by descending trailing-group + count. A truncated address still parses and is still public, so it + would be reported silently — a different host attributed and blocked. + """ + for address in ( + "2a00:1450:4864:20::32", + "2001:470:1f0b:16c0::2:1", + "2a02:26f0:12d::1:2", + "2600:1f18:63bf::a:b:c", + "fe80::1234:5678", + "2001:4860:4860::8888", + ): + match = REGXIP6.search(address) + assert match is not None and match.group() == address, address + + +def test_helo_strip_keeps_a_sender_whose_helo_is_the_word_helo(): + """ + The HELO *argument* never sits at offset 0 of the from clause — that + position holds the HELO *name*. Without the lookbehind, a sender + announcing ``EHLO helo`` matched there and the pattern ate the + MTA-written IP, suppressing attribution entirely. + """ + for helo in ("vps.ovh.net", "helo", "HELO", "ehelo"): + received = ( + f"Received: from {helo} ([45.33.32.156])\r\n" + "\tby mx.victim.com (sendmail) with ESMTP id ABC;" + " Mon, 1 Jan 2024 00:00:00 +0000\r\n" + ) + assert _sender_ip(received) == "45.33.32.156", f"HELO {helo!r} lost the IP" + + +def test_sender_ip_walks_past_a_private_internal_relay(): + """ + A trusted hop naming a private IP is an internal relay, so the documented + walk continues down the chain. Only a hop naming *no* IP ends the + search — that is the case where falling through would hand the answer to + whoever wrote the older headers. + """ + internal = ( + "Received: from relay.internal (relay.internal [10.0.0.5])\r\n" + "\tby mx.victim.com (Postfix) id A; Mon, 1 Jan 2024 00:00:00 +0000\r\n" + ) + public = ( + "Received: from real.sender (real.sender [45.33.32.156])\r\n" + "\tby mx.victim.com (Postfix) id B; Mon, 1 Jan 2024 00:00:00 +0000\r\n" + ) + assert _sender_ip(internal, public) == "45.33.32.156" + + +def test_group_spans_tracks_nesting_and_unclosed_groups(): + """ + Group membership decides which addresses the MTA wrote, so an unclosed + delimiter must not silently drop the rest of the clause: a sender who + opens a bracket would otherwise hide every later address from the scan. + """ + # nesting collapses to the outermost group + assert group_spans("a (b [c] d) e") == [(3, 10)] + # an unclosed group runs to the end of the string + assert group_spans("a (b [c") == [(3, 7)] + assert group_spans("no groups here") == [] + # a stray closer is not a group + assert group_spans("a) b") == [] + + spans = group_spans("x (y)") + assert in_spans(3, spans) is True + assert in_spans(0, spans) is False + + +def test_raw_suffix_folds_underscores_to_dashes(): + """ + ``X_raw`` resolves the same header name as ``X``. + + The ``_raw`` branch used to look the name up with its underscores + intact while every other branch folded them to dashes, so + ``mail.x_mailer_raw`` reported "null" for a header ``mail.x_mailer`` + returned fine. + """ + mail = mailparser.parse_from_string( + "X-Mailer: Foo 1.0\r\nX-MSMail-Priority: High\r\nSubject: s\r\n\r\nbody\r\n" + ) + + def raw(name): + """Read a ``_raw`` attribute, which is always a JSON string.""" + value = getattr(mail, name) + assert isinstance(value, str), name + return json.loads(value) + + assert raw("x_mailer_raw") == ["Foo 1.0"] + assert raw("X_MSMail_Priority_raw") == ["High"] + # absent headers give an empty list, never "null" + assert raw("x_nonexistent_raw") == []