diff options
| -rw-r--r-- | abusectl/case.py | 4 | ||||
| -rw-r--r-- | abusectl/parse.py | 39 | ||||
| -rw-r--r-- | docs/specs/2026-09-09-report.md | 31 | ||||
| -rw-r--r-- | tests/test_case.py | 10 | ||||
| -rw-r--r-- | tests/test_cli.py | 7 | ||||
| -rw-r--r-- | tests/test_parse.py | 74 |
6 files changed, 160 insertions, 5 deletions
diff --git a/abusectl/case.py b/abusectl/case.py index 4c8c2db..233bc5f 100644 --- a/abusectl/case.py +++ b/abusectl/case.py @@ -87,6 +87,10 @@ def create(root: Path, raw: bytes) -> Case: "auth": [], "contacts": [], "destinations": [], + # Seeded like the rest so a created-but-unparsed case has the same + # shape as a parsed one: report indexes this block rather than + # guarding every access. Additive, so FORMAT_VERSION is unchanged. + "headers": [], } save(path, manifest) diff --git a/abusectl/parse.py b/abusectl/parse.py index 8773c85..2217583 100644 --- a/abusectl/parse.py +++ b/abusectl/parse.py @@ -63,9 +63,33 @@ _MAX_REDIRECT_DEPTH = 5 # The optional "for <addr>" clause of a Received header (RFC 5321 4.4). Our # own boundary relay writes the ENVELOPE RECIPIENT there, so the one Received # line a report publishes carries the victim's address verbatim unless this -# is removed. Matched up to the clause terminator rather than to end of line -# because "for" is not always last: a timestamp follows it after the ";". -_RECEIVED_FOR = re.compile(r"(?is)\bfor\s+<[^>]*>\s*(?=;|$)") +# is removed. +# +# Anchored on the ADDRESS, not on the clause terminator. Requiring "for" to +# be immediately followed by ";" or end of line is strictly stronger than +# the grammar: RFC 5321 4.4 puts For inside Opt-info, so With, ID, Via or a +# CFWS comment may legitimately follow it, and "for <a@b> (envelope-from +# <c@d>);" is routine Exim and Sendmail output. That terminator anchor +# stripped only the neatest shape and published the address in four +# ordinary ones. +# +# Angle brackets are optional because For is 1*( Path / Mailbox ) and +# Mailbox carries none. The address run stops at "<>;" and whitespace so the +# match cannot swallow the rest of the header, and a trailing ")" is trimmed +# so the clause inside a comment does not leave an orphan bracket. +_RECEIVED_FOR = re.compile(r"(?is)\bfor\s+<?[^\s<>;]+@[^\s<>;]+>?\)?") + +# Left behind once a clause is cut from the middle of a line: a doubled +# space, or a space now sitting against the ";" that ends the Opt-info. The +# line is published verbatim to a third party, so it should not read as the +# output of a broken tool. +_LEFTOVER_SPACE = re.compile(r"\s{2,}") +_ORPHAN_SEPARATOR = re.compile(r"\s+([;)])") + +# A comment that held nothing but the clause, e.g. "(for <a@b>)", is now an +# empty pair of brackets or a lone opening one. Cut rather than left as +# debris in a published line. +_EMPTY_COMMENT = re.compile(r"\s*\(\s*\)?\s*$") # The headers that may appear in a published report. A WHITELIST, never a # blacklist: a blacklist means every header this parser learns to read later @@ -193,8 +217,15 @@ def _strip_envelope_recipient(received_value: str) -> str: The clause is optional (RFC 5321 4.4) and carries nothing a desk needs: the report is about who SENT the message. Only the clause goes, so the hop's own evidence, the address and the receiving server, survives. + + Every shape of the clause is cut, not merely the tidy one: see + _RECEIVED_FOR, where anchoring on the terminator rather than on the + address let four ordinary shapes publish the address. """ - return _RECEIVED_FOR.sub("", received_value).rstrip() + cut = _RECEIVED_FOR.sub("", received_value) + cut = _LEFTOVER_SPACE.sub(" ", cut) + cut = _ORPHAN_SEPARATOR.sub(r"\1", cut) + return _EMPTY_COMMENT.sub("", cut).strip() def report_headers(raw: bytes, trusted: list[str]) -> list[tuple[str, str]]: diff --git a/docs/specs/2026-09-09-report.md b/docs/specs/2026-09-09-report.md index e5ca858..12d2157 100644 --- a/docs/specs/2026-09-09-report.md +++ b/docs/specs/2026-09-09-report.md @@ -207,6 +207,37 @@ The cost is real and is accepted: this is a change to `parse.py`, a new an address, and a whitelist written by hand is exactly the kind of thing a sweep catches being wrong. +### Two accepted disclosures, named so they are not mistaken for leaks + +The first property reads as an unqualified "recipient identifiers must never +reach a report". These are the deliberate exceptions the whitelist creates, +recorded here rather than left to be rediscovered in a test comment. + +**Our own receiving relay's hostname is published.** The boundary `Received` +line names it in its `by` clause and `Authentication-Results` names it as the +authserv-id, so `mx.example.org` travels with every report. That is the +user's mail host, not the user's identity, and an abuse desk learns it from +the report's own `From` regardless. It is accepted because removing it would +mean rewriting the inside of two headers whose value to a desk is precisely +that they are the receiving server's own verbatim words. The consequence is +that the manifest-wide "no bare `example.org`" assertion cannot hold over the +`headers` block; `tests/test_cli.py` narrows it there and asserts the +ADDRESS is still absent, which is the part that matters. + +**Attacker-controlled free text is published unfiltered.** `Subject` and the +`From` display name are kept deliberately, because they are what lets a desk +recognise a campaign. An attacker who writes the recipient's address into +one, plainly or obfuscated as `you%40example.org`, gets it published: the +whitelist governs WHICH headers travel, never what is inside one. This is +not fixed by filtering free text, which is the judgement-shaped problem that +`AGENTS.md` names as the origin of every leak this project has had. The sweep +over real mail is what covers this class, which is one more reason it is not +optional here. + +The envelope recipient is NOT in this list. Our own relay writes it into the +boundary `Received` line's optional `for` clause, and that clause is cut +before the line is stored, in every shape the grammar allows. + ## The reporting identity Three config keys, all under a `[reporter]` section: diff --git a/tests/test_case.py b/tests/test_case.py index c6964a9..ab7eeeb 100644 --- a/tests/test_case.py +++ b/tests/test_case.py @@ -45,6 +45,16 @@ class TestCaseCreation(unittest.TestCase): manifest = json.loads((created.path / "manifest.json").read_text()) self.assertEqual(manifest["format"], case.FORMAT_VERSION) + def test_every_block_is_seeded_present_and_empty(self): + # A created-but-unparsed case must have the same SHAPE as a parsed + # one, so a later reader indexes a block rather than guarding every + # access. report will read headers and would hit a KeyError. + created = case.create(self.root, b"x") + manifest = json.loads((created.path / "manifest.json").read_text()) + for block in ("iocs", "auth", "contacts", "destinations", "headers"): + self.assertIn(block, manifest) + self.assertEqual(manifest[block], []) + def test_two_cases_do_not_collide(self): a = case.create(self.root, b"one") b = case.create(self.root, b"two") diff --git a/tests/test_cli.py b/tests/test_cli.py index 425bce9..b8fffea 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -160,7 +160,12 @@ class TestParse(unittest.TestCase): headers = manifest.pop("headers") self.assertNotIn("example.org", json.dumps(manifest)) # And nothing shaped like an address survives in the exception. - self.assertNotIn("@example.org", json.dumps(headers)) + # Both spellings: you%40example.org is not a hypothetical, it is why + # leaky.eml exists, and docs/plans/2026-09-09-contacts.md records a + # From of phish@victim%40example.org.invalid. + blob = json.dumps(headers) + self.assertNotIn("@example.org", blob) + self.assertNotIn("you%40example.org", blob) names = [name for name, _ in headers] for name in ("To", "Cc", "Delivered-To", "X-Original-To"): self.assertNotIn(name, names) diff --git a/tests/test_parse.py b/tests/test_parse.py index fa3526a..6e3b7a4 100644 --- a/tests/test_parse.py +++ b/tests/test_parse.py @@ -336,6 +336,80 @@ class ReportHeaders(unittest.TestCase): # The rest of the hop survives; this is a cut, not a blanking. self.assertIn("203.0.113.42", received[0]) + def test_every_for_clause_shape_loses_the_address(self): + # RFC 5321 4.4 puts For inside Opt-info, so With, ID, Via or a CFWS + # comment may legitimately follow it, and its ABNF is + # 1*( Path / Mailbox ) where Mailbox carries no angle brackets. + # Anchoring on "for" being immediately followed by the clause + # terminator matched only the neatest shape and let four routine + # ones through, each publishing the victim's address. + hop = "from a.invalid (a.invalid [203.0.113.5]) by mx.example.org " + shapes = ( + "for <you@example.org> (envelope-from <b@c.invalid>); Mon, 07 Sep 2026 09:12:40 +0000", + "for you@example.org; Mon, 07 Sep 2026 09:12:40 +0000", + "for <you@example.org> with ESMTP; Mon, 07 Sep 2026 09:12:40 +0000", + "id qq; Mon, 07 Sep 2026 09:12:40 +0000 (for <you@example.org>)", + "for <you@example.org>; Mon, 07 Sep 2026 09:12:40 +0000", + ) + for tail in shapes: + with self.subTest(tail=tail): + stripped = parse._strip_envelope_recipient(hop + tail) + self.assertNotIn("you@example.org", stripped) + # The hop's own evidence survives: this is a cut, not a + # blanking, and a rule that ate the line would pass the + # assertion above while destroying the report. + self.assertIn("203.0.113.5", stripped) + self.assertIn("mx.example.org", stripped) + + def test_stripping_leaves_no_doubled_space_or_stray_separator(self): + # Cosmetic in isolation, but the result is published verbatim to a + # third party, so a mangled line reads as a broken tool. + hop = ("from a.invalid (a.invalid [203.0.113.5]) by mx.example.org" + " for <you@example.org>; Mon, 07 Sep 2026 09:12:40 +0000") + stripped = parse._strip_envelope_recipient(hop) + self.assertNotIn(" ", stripped) + self.assertNotIn(" ;", stripped) + self.assertIn("mx.example.org; Mon", stripped) + + def test_a_comment_holding_only_the_clause_leaves_no_debris(self): + hop = ("from a.invalid (a.invalid [203.0.113.5]) by mx.example.org" + " id qq; Mon, 07 Sep 2026 09:12:40 +0000 (for <you@example.org>)") + stripped = parse._strip_envelope_recipient(hop) + self.assertNotIn("you@example.org", stripped) + self.assertFalse(stripped.endswith("(")) + self.assertTrue(stripped.endswith("+0000")) + + def test_the_envelope_sender_comment_survives_the_cut(self): + # envelope-from is the SENDER, which is what the report is about, so + # cutting the recipient must not take it along. + hop = ("from a.invalid (a.invalid [203.0.113.5]) by mx.example.org" + " for <you@example.org> (envelope-from <bounce@sender.invalid>);" + " Mon, 07 Sep 2026 09:12:40 +0000") + stripped = parse._strip_envelope_recipient(hop) + self.assertNotIn("you@example.org", stripped) + self.assertIn("bounce@sender.invalid", stripped) + + def test_the_whitelist_does_not_filter_attacker_free_text(self): + # A DOCUMENTED LIMIT, not a guarantee. The spec keeps Subject and the + # From display name knowing both are attacker-controlled free text, + # because they are what lets a desk recognise a campaign. An attacker + # who writes the recipient's own address into one, obfuscated or not, + # gets it published: the whitelist governs WHICH headers travel, never + # what is inside one. + # + # This is asserted so the limit is visible and deliberate. Do not + # "fix" it by filtering free text, which is the judgement-shaped + # problem AGENTS.md names as the source of every leak here. The + # sweep over real mail is what covers this class, per AGENTS.md. + raw = (b"Received: from a.invalid (a.invalid [203.0.113.5])" + b" by mx.example.org with ESMTP id X;" + b" Mon, 07 Sep 2026 09:12:40 +0000\r\n" + b"From: <phish@sender.invalid>\r\n" + b"Subject: Verify you%40example.org\r\n\r\nbody\r\n") + headers = parse.report_headers(raw, trusted=["192.0.2.0/24"]) + subject = dict(headers)["Subject"] + self.assertIn("you%40example.org", subject) + def test_a_forged_chain_publishes_no_hop_below_the_boundary(self): # The same job test_a_forged_chain_stops_at_the_first_untrusted_hop # does for sending_ip(), asserted over what actually gets published: |