diff options
| -rw-r--r-- | abusectl/report.py | 177 | ||||
| -rw-r--r-- | tests/test_report.py | 455 |
2 files changed, 632 insertions, 0 deletions
diff --git a/abusectl/report.py b/abusectl/report.py index 7cb4a9b..a3d017e 100644 --- a/abusectl/report.py +++ b/abusectl/report.py @@ -29,6 +29,9 @@ itself is a module that can disclose it in a code path nobody reviewed. import hashlib +VERSION = "0.1.0" + + # Hard wrap column, from the spec: "Plain text, hard-wrapped at 72 columns." # Abuse desks run ticketing systems that reflow or clip long lines, and a # clipped line is the defect this whole wrapping scheme exists to prevent. @@ -525,3 +528,177 @@ def text_part(manifest: dict, destination: dict, identity: dict) -> str: lines.append("Generated by abusectl.") return "\n".join(lines) + "\n" + + +# Every character that any reasonable reader of this part might treat as the +# end of a field, plus "%" itself. +# +# CR and LF are the ones that matter: a field value carrying one forges a +# field, and the forged field is read as something THIS TOOL asserted, on a +# document that carries the reporter's identity. That is header injection +# into mail we send, which the contacts spec already names as a hazard. +# +# The rest are here because "what counts as a line break" is not one answer. +# Python's own email module raises on U+2028 and U+2029 as readily as on LF, +# because it reaches for str.splitlines(), which also breaks on VT, FF, the +# three information separators and NEL. A value carrying one of those does +# not merely render oddly in the assembled document: it aborts the document. +# So the set is taken from splitlines() rather than from RFC 5322, on the +# principle that the defence must cover what the CONSUMERS break on, not what +# one specification says a break is. +# +# "%" is in the set for a different reason, and leaving it out is a second +# injection one step later: see _field_value(). +_UNSAFE = "".join(chr(c) for c in ( + 0x0A, 0x0B, 0x0C, 0x0D, 0x1C, 0x1D, 0x1E, 0x85, 0x2028, 0x2029, +)) + "%" + + +def _field_value(value) -> str: + """Make one attacker-supplied value safe to emit as a field value. + + ENCODES rather than rejects or strips, and the choice is the whole point + of this function. + + The value is reachable, not theoretical. redact.url_valued_parameters() + URL-DECODES a redirector's destination parameter in order to recover it + as an indicator in its own right, exactly as the second property + intends. So a message body carrying + + http://r.invalid/go?next=http%3A%2F%2Fa.invalid%2Fx%0AFeedback-Type... + + produces, through parse.iocs() on a real message, an IOC whose value + contains a literal newline followed by text shaped like a field. There + is no upstream filter between that and here. + + REJECTING the indicator loses a genuine redirect target, which is one of + the more actionable things a desk receives, over an attacker's choice of + byte. STRIPPING the character silently rewrites the indicator into a + different URL, and a desk that acts on the stripped form has acted on a + resource nobody reported: that is the truncation defect from the text + part wearing a different coat, and it is worse here because nothing in + the output says it happened. + + Percent-encoding is the URL's own native encoding, so it reads as + intended by the audience that receives it; it is EXACTLY REVERSIBLE, so + a desk or a later submit path recovers what the message declared; and it + is visible, so an altered value announces the alteration rather than + passing as a clean one. Same instinct as the continuation marker in the + text part: a fragment announces that it is a fragment. + + "%" must be encoded too, and this is not tidiness. A redacted URL + legitimately contains percent signs, so an encoder that leaves them + alone emits text whose reversal produces the very control character the + encoding existed to remove: an attacker writes the literal five + characters "%0A" into a path and unquote() hands the next reader a + newline. An encoding that is not a true inverse is not a defence. + + The escape runs over UTF-8 BYTES, not over code points, and that is not + a detail. Percent-encoding is defined on octets, so a character above + U+007F has more than one byte to spell: encoding U+2028 as "%2028" from + its ordinal produces text that unquote() reads back as "%20" followed by + the literal "28", which is a SPACE and the digits, not the character + that was there. The reversibility this function claims would then be + false for exactly the two characters that are here because Python's + email module breaks on them. Encoding each of its three UTF-8 bytes + gives "%E2%80%A8", and unquote() returns U+2028. + + Non-strings are coerced rather than raising. A manifest is a file the + user edits by hand and JSON has numbers; this module is the last step + before a reviewed case becomes a sent mail, and a report that raises + produces nothing at all. + """ + text = str(value) + if not any(ch in text for ch in _UNSAFE): + return text + out = [] + for ch in text: + if ch in _UNSAFE: + out.append("".join(f"%{b:02X}" for b in ch.encode("utf-8"))) + else: + out.append(ch) + return "".join(out) + + +def feedback_fields(manifest: dict, destination: dict) -> list[tuple[str, str]]: + """Build the machine-readable part: an RFC 5965 envelope carrying x-arf + fields inside it. + + RFC 5965 is an IETF standard and universally understood, but it was + designed for feedback loops, where a report is ABOUT A MESSAGE. These + reports are about INDICATORS, and 5965 has no field for "this specific + host is the thing being reported". x-arf's Source does. The envelope is + the standard's own extension point: 5965 requires an implementation to + ignore fields it does not support, so a standards parser reads what it + knows and x-arf tooling finds what it wants. + + Returned as PAIRS, not a dict, because Reported-Uri and Reported-Domain + repeat. Everything else does not, and that is enforced rather than + assumed: RFC 5965 gives Source-IP and Arrival-Date "once maximum". A + strict parser meeting a repeated single-occurrence field either rejects + the part or keeps the last occurrence, so a second Source-IP does not + add an address, it DISPLACES the primary one. Every address still + travels: the text part lists all of this destination's indicators, and + that is the part a human acts on. + + ARRIVAL-DATE IS DELIBERATELY ABSENT. 5965 defines it as when the + generating ADMD's own MTA received the message; the manifest's Date + header is when the SENDER CLAIMED to have sent it, which on a phishing + message is attacker-controlled free text. Copying one into the other + asserts an attacker's timestamp as our own observation, and a desk + correlating it against their logs finds nothing and discounts the + report. The honest source is the boundary Received hop's timestamp, + which parse.report_headers() already publishes; extracting it needs a + date parser, and 5965 makes the field optional, so it is omitted until + something actually needs it. An absent optional field misstates nothing. + + SOURCE IS OMITTED WHEN THERE IS NOTHING TO PUT IN IT, rather than + emitted empty. "Source:" with nothing after it tells a parser that the + thing being reported is the empty string; no field tells it nothing, + which is the truth. This happens when a desk's only indicators are a + sha256 or an observation, and both of those belong to the human part. + + sha256 and observation are NOT forced into the nearest-looking field. + Neither is a Source, a URI or a domain, and telling a desk that a file + hash is a URI is a false statement in the part meant to be machine-read. + + Every field NAME here is a literal in this module and none is derived + from data. The tempting generalisation, a table from an IOC's own type + string to a field name, would make a hand-edited manifest able to name + fields; the set is closed on purpose. Every field VALUE goes through + _field_value(), at the single point where the pairs are built, for the + reason the fourth property was learned three times over: validation + applied per branch gets forgotten on the next branch. + """ + by_id = {entry["id"]: entry for entry in manifest.get("iocs", [])} + mine = [by_id[i] for i in destination.get("iocs", []) if i in by_id] + + ips = [e.get("value") for e in mine if e.get("type") in ("ipv4", "ipv6")] + urls = [e.get("value") for e in mine if e.get("type") == "url"] + domains = [e.get("value") for e in mine if e.get("type") == "domain"] + + # Source is singular, so the primary indicator fills it and the rest + # travel in the repeatable fields and in the text part. An IP is the + # most actionable thing a hosting desk can act on, so it wins when + # present; a domain beats a URL because a desk suspending a name + # covers every URL under it. + primary = (ips or domains or urls or [None])[0] + + fields = [ + ("Feedback-Type", "abuse"), + ("User-Agent", f"abusectl/{VERSION}"), + ("Version", "1"), + ("Report-Type", "phishing"), + ] + + if primary is not None: + fields.append(("Source", primary)) + if ips: + # Once maximum, per RFC 5965. See the docstring. + fields.append(("Source-IP", ips[0])) + for domain in domains: + fields.append(("Reported-Domain", domain)) + for url in urls: + fields.append(("Reported-Uri", url)) + + return [(name, _field_value(value)) for name, value in fields] diff --git a/tests/test_report.py b/tests/test_report.py index 3b13d11..f414841 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -922,5 +922,460 @@ class BackslashRoundTrip(unittest.TestCase): self.assertIn("Generated by abusectl.", report.unwrap(text)) +class FeedbackPart(unittest.TestCase): + def setUp(self): + destination = report.email_destinations(MANIFEST["contacts"])[0] + self.fields = report.feedback_fields(MANIFEST, destination) + self.lookup = dict(self.fields) + + def test_the_three_rfc5965_required_fields_are_present(self): + self.assertEqual(self.lookup["Feedback-Type"], "abuse") + self.assertEqual(self.lookup["Version"], "1") + self.assertTrue(self.lookup["User-Agent"].startswith("abusectl/")) + + def test_the_xarf_report_type_is_phishing(self): + self.assertEqual(self.lookup["Report-Type"], "phishing") + + def test_source_is_the_primary_indicator(self): + self.assertEqual(self.lookup["Source"], "203.0.113.42") + self.assertEqual(self.lookup["Source-IP"], "203.0.113.42") + + def test_every_url_appears_as_a_reported_uri(self): + uris = [value for name, value in self.fields if name == "Reported-Uri"] + self.assertEqual( + uris, ["http://login.sender.invalid/verify?id=REDACTED"] + ) + + def test_a_destination_with_no_ip_omits_source_ip(self): + contacts = [{"iocs": ["ioc-2"], "query": "sender.invalid", + "abuse": ["abuse@host.invalid"], "source": "rdap"}] + destination = report.email_destinations(contacts)[0] + lookup = dict(report.feedback_fields(MANIFEST, destination)) + self.assertNotIn("Source-IP", lookup) + self.assertEqual( + lookup["Source"], "http://login.sender.invalid/verify?id=REDACTED" + ) + + # --- the parts the plan got wrong ------------------------------------- + + def _fields_for(self, iocs: list[dict]) -> list[tuple[str, str]]: + """Render the machine part for a hand-built IOC list. + + Every IOC reaches one destination, so the field list is exactly what + those indicators produce and nothing is filtered out behind the test. + """ + manifest = copy.deepcopy(MANIFEST) + manifest["iocs"] = iocs + manifest["contacts"] = [ + {"iocs": [entry["id"] for entry in iocs], "query": "x.invalid", + "abuse": ["abuse@host.invalid"], "source": "rdap"}, + ] + destination = report.email_destinations(manifest["contacts"])[0] + return report.feedback_fields(manifest, destination) + + def test_source_ip_is_emitted_at_most_once(self): + """RFC 5965 says Source-IP appears "once maximum". + + The plan emitted one per IP. A strict parser meeting a repeated + single-occurrence field either rejects the part or keeps whichever + occurrence it saw last, so the field a repeat was meant to add is + the field that displaces the primary one. Every IP still travels, + in the text part and in Reported-Uri's sibling below. + """ + fields = self._fields_for([ + {"id": "ioc-1", "type": "ipv4", "value": "203.0.113.42", + "origin": "received-chain", "confidence": "boundary-hop"}, + {"id": "ioc-2", "type": "ipv4", "value": "203.0.113.43", + "origin": "received-chain"}, + ]) + ips = [v for n, v in fields if n == "Source-IP"] + self.assertEqual(ips, ["203.0.113.42"]) + self.assertEqual(dict(fields)["Source"], "203.0.113.42") + + def test_no_field_appears_twice_unless_the_rfc_allows_it(self): + """The invariant behind the test above, stated once for every field. + + Reported-Uri and Reported-Domain are "any number of times"; every + other field this module emits is once-maximum. Asserting only on + Source-IP would let the next repeated field ship unnoticed. + """ + fields = self._fields_for([ + {"id": "ioc-1", "type": "ipv4", "value": "203.0.113.42", + "origin": "received-chain"}, + {"id": "ioc-2", "type": "ipv6", "value": "2001:db8::1", + "origin": "received-chain"}, + {"id": "ioc-3", "type": "url", "value": "http://a.invalid/x", + "origin": "body"}, + {"id": "ioc-4", "type": "url", "value": "http://b.invalid/y", + "origin": "body"}, + {"id": "ioc-5", "type": "domain", "value": "a.invalid", + "origin": "header-from"}, + {"id": "ioc-6", "type": "domain", "value": "b.invalid", + "origin": "header-reply_to"}, + ]) + seen: dict[str, int] = {} + for name, _ in fields: + seen[name] = seen.get(name, 0) + 1 + repeatable = {"Reported-Uri", "Reported-Domain"} + for name, count in seen.items(): + if name not in repeatable: + self.assertEqual(count, 1, f"{name} appeared {count} times") + self.assertEqual(seen["Reported-Uri"], 2) + self.assertEqual(seen["Reported-Domain"], 2) + + def test_an_ipv6_indicator_fills_source_ip_too(self): + """"ipv6" is a distinct type string from parse.iocs(). + + A branch testing only for "ipv4" drops every IPv6 sender, and the + given tests use IPv4 throughout so none of them would notice. + """ + fields = dict(self._fields_for([ + {"id": "ioc-1", "type": "ipv6", "value": "2001:db8::1", + "origin": "received-chain"}, + ])) + self.assertEqual(fields["Source"], "2001:db8::1") + self.assertEqual(fields["Source-IP"], "2001:db8::1") + + def test_a_destination_with_no_typed_indicator_omits_source(self): + """An empty Source is worse than an absent one. + + "Source:" with nothing after it asserts that the thing being + reported is the empty string. A 5965 parser reading a present-but- + empty field has been told a value; reading no field it has been + told nothing, which is the truth. Only sha256 and observation + indicators reach a desk here, and both belong in the text part. + """ + fields = self._fields_for([ + {"id": "ioc-1", "type": "observation", + "value": "display-name-carries-address", + "origin": "display-name-from"}, + ]) + lookup = dict(fields) + self.assertNotIn("Source", lookup) + self.assertNotIn("Source-IP", lookup) + # The envelope is still well formed: a desk gets a valid part. + self.assertEqual(lookup["Feedback-Type"], "abuse") + self.assertEqual(lookup["Version"], "1") + + def test_a_type_this_module_does_not_place_is_not_invented_into_one(self): + """sha256 and observation have no 5965 or x-arf field. + + Neither is a Source, a Reported-Uri or a Reported-Domain, and + forcing one into the nearest-looking field would tell a desk that a + file hash is a URI. They travel in the human part, which is where a + desk reads what an attachment was. + """ + fields = self._fields_for([ + {"id": "ioc-1", "type": "ipv4", "value": "203.0.113.42", + "origin": "received-chain"}, + {"id": "ioc-2", "type": "sha256", "value": "a" * 64, + "origin": "attachment", "filename": "invoice.zip"}, + {"id": "ioc-3", "type": "observation", + "value": "display-name-carries-address", + "origin": "display-name-from"}, + ]) + blob = repr(fields) + self.assertNotIn("a" * 64, blob) + self.assertNotIn("display-name-carries-address", blob) + self.assertEqual(dict(fields)["Source"], "203.0.113.42") + + def test_an_ioc_id_the_manifest_lacks_is_skipped_not_raised(self): + """A manifest is a file the user edits, so the two can disagree. + + text_part() already tolerates this; the machine part indexed with + destination["iocs"] straight into a dict would raise instead, and + the two parts of one document must not disagree about whether the + case can be reported at all. + """ + destination = {"id": "email-x", "kind": "email", + "target": "abuse@host.invalid", + "iocs": ["ioc-1", "ioc-404"], "body": None, + "status": "pending"} + lookup = dict(report.feedback_fields(MANIFEST, destination)) + self.assertEqual(lookup["Source"], "203.0.113.42") + self.assertNotIn("ioc-404", repr(lookup)) + + def test_a_destination_with_no_iocs_key_still_renders(self): + """destination.get("iocs"), not destination["iocs"].""" + destination = {"id": "email-x", "kind": "email", + "target": "abuse@host.invalid", "body": None, + "status": "pending"} + lookup = dict(report.feedback_fields(MANIFEST, destination)) + self.assertEqual(lookup["Feedback-Type"], "abuse") + + def test_arrival_date_is_not_taken_from_the_senders_date_header(self): + """RFC 5965: Arrival-Date is when the generating ADMD's MTA received + the message. The Date header is when the SENDER CLAIMS it was sent. + + The plan copied Date into Arrival-Date. On a phishing message that + header is attacker-controlled free text, so the report would assert + as our own observation a timestamp the attacker chose, and a desk + correlating it against their own logs would look in the wrong place + or find nothing and discount the report. + + The honest source is the boundary Received hop's own timestamp, + which parse.report_headers() already publishes. Parsing one is a + date parser this task does not need, so the field is OMITTED: 5965 + makes it optional, and an absent optional field misstates nothing. + """ + lookup = dict(self.fields) + self.assertNotIn("Arrival-Date", lookup) + self.assertNotIn("Mon, 07 Sep 2026 09:12:40 +0000", repr(lookup)) + + +class FeedbackInjection(unittest.TestCase): + """A field value carrying a line break forges a field in the report. + + This is not hypothetical and it is not stopped upstream. redact.py + URL-DECODES a redirector's destination parameter to recover it as an + indicator, so a message body carrying + + http://r.invalid/go?next=http%3A%2F%2Fa.invalid%2Fx%0AFeedback-Type... + + produces, through parse.iocs() on a real .eml, an IOC whose value is + "http://a.invalid/x\\nFeedback-Type: not-abuse". Emitted verbatim, the + abuse desk's parser reads a Feedback-Type this tool never asserted, on a + report that carries the reporter's identity. That is an attacker writing + fields into mail sent under our name. + + The answer here is to PERCENT-ENCODE the control characters rather than + to drop the indicator or strip them. Dropping loses a real redirect + target; stripping silently rewrites an indicator into a different one a + desk would then act on. Percent-encoding is the URL's own native + encoding, is exactly reversible, and leaves the value visibly altered + rather than quietly wrong. + """ + + def _value_out(self, value: str) -> str | None: + manifest = copy.deepcopy(MANIFEST) + manifest["iocs"] = [{"id": "ioc-1", "type": "url", "value": value, + "origin": "redirect-target"}] + manifest["contacts"] = [ + {"iocs": ["ioc-1"], "query": "x.invalid", + "abuse": ["abuse@host.invalid"], "source": "rdap"}, + ] + destination = report.email_destinations(manifest["contacts"])[0] + fields = report.feedback_fields(manifest, destination) + for name, out in fields: + if name == "Reported-Uri": + return out + return None + + def _assert_no_break(self, fields: list[tuple[str, str]]) -> None: + for name, value in fields: + for bad in ("\r", "\n", "
", "
", "\v", "\f", + "\x1c", "\x1d", "\x1e", "\x85"): + self.assertNotIn(bad, name) + self.assertNotIn(bad, value) + + def test_a_newline_in_a_value_cannot_forge_a_field(self): + out = self._value_out("http://a.invalid/x\nFeedback-Type: not-abuse") + self.assertNotIn("\n", out) + self.assertIn("%0A", out) + # The forged field name must not survive as a line of its own, but + # the text of the indicator is still legible and reversible. + self.assertEqual(out, + "http://a.invalid/x%0AFeedback-Type: not-abuse") + + def test_the_real_parse_output_that_makes_this_reachable(self): + """End to end from an .eml, not from a hand-written IOC. + + A test that only feeds feedback_fields() a crafted string proves the + encoder works; it does not prove the encoder is needed. This runs + the actual redirector through parse.iocs() so the fixture and the + defence cannot drift apart. + """ + from abusectl import parse + raw = ( + "Received: from evil.invalid ([203.0.113.9]) by mx.example.org; " + "Mon, 07 Sep 2026 09:12:40 +0000\r\n" + "From: <phish@sender.invalid>\r\n" + "Subject: verify\r\n" + "Date: Mon, 07 Sep 2026 09:12:40 +0000\r\n" + "Content-Type: text/plain\r\n\r\n" + "http://r.invalid/go?next=http%3A%2F%2Fa.invalid%2Fx%0A" + "Feedback-Type%3A%20not-abuse\r\n" + ).encode() + iocs = parse.iocs(raw, trusted=["192.0.2.0/24"]) + injected = [e for e in iocs if "\n" in e["value"]] + self.assertTrue(injected, "the injection vector itself has changed") + + manifest = {"format": 1, "iocs": iocs, "headers": [], "auth": {}} + destination = {"id": "email-x", "kind": "email", + "target": "abuse@host.invalid", + "iocs": [e["id"] for e in iocs], "body": None, + "status": "pending"} + fields = report.feedback_fields(manifest, destination) + self._assert_no_break(fields) + + def test_every_line_breaking_shape_is_neutralised(self): + """The adversarial sweep, not a handful of cases. + + U+2028 and U+2029 are in here because Python's own email module + raises on them: str.splitlines() treats them as breaks, so a value + carrying one would make the whole document fail to assemble in + Task 6 rather than merely render oddly. + """ + breaks = ["\n", "\r", "\r\n", "\n\r", "
", "
", + "\v", "\f", "\x1c", "\x1d", "\x1e", "\x85"] + shapes = [] + for brk in breaks: + shapes += [ + brk, + "http://a.invalid/x" + brk, + brk + "http://a.invalid/x", + "http://a.invalid/x" + brk + "Feedback-Type: not-abuse", + "http://a.invalid/" + brk * 3 + "Source: 192.0.2.1", + ] + for value in shapes: + with self.subTest(value=repr(value)): + out = self._value_out(value) + self.assertIsNotNone(out) + for bad in breaks: + if len(bad) == 1: + self.assertNotIn(bad, out) + + def test_a_value_that_is_only_a_newline_still_yields_a_field(self): + """It must not become an empty value or vanish silently.""" + out = self._value_out("\n") + self.assertEqual(out, "%0A") + + def test_encoding_is_reversible_so_the_indicator_is_not_misstated(self): + """The property that makes encoding honest rather than a strip. + + A desk, or a later submit path, must be able to recover exactly what + the message declared. Stripping the character would pass every + assertion above and hand the desk a DIFFERENT URL. + """ + from urllib.parse import unquote + for value in ("http://a.invalid/x\nFeedback-Type: not-abuse", + "http://a.invalid/\r\n\r\n", + "http://a.invalid/x
y"): + with self.subTest(value=repr(value)): + self.assertEqual(unquote(self._value_out(value)), value) + + def test_a_literal_percent_is_encoded_so_the_reversal_is_unambiguous(self): + """Without this, "%0A" typed by the attacker decodes to a newline. + + A redacted URL legitimately contains percent signs, and an encoder + that leaves them alone produces text that unquote() turns into the + very control character the encoder existed to remove. The reversal + must be a true inverse or it is a second injection one step later. + """ + from urllib.parse import unquote + value = "http://a.invalid/x?a=%0AFeedback-Type: not-abuse" + out = self._value_out(value) + self.assertNotIn("\n", out) + self.assertEqual(unquote(out), value) + + def test_an_injected_field_name_in_a_domain_is_neutralised_too(self): + """Reported-Domain and Source take the same path as Reported-Uri. + + The defence must not live in one branch. That is the exact shape of + the fourth property's three leaks: validation applied per branch + gets forgotten on the next branch. + """ + manifest = copy.deepcopy(MANIFEST) + manifest["iocs"] = [ + {"id": "ioc-1", "type": "domain", + "value": "a.invalid\nSource: 192.0.2.1", + "origin": "header-from"}, + {"id": "ioc-2", "type": "ipv4", + "value": "203.0.113.42\nVersion: 9", + "origin": "received-chain"}, + ] + manifest["contacts"] = [ + {"iocs": ["ioc-1", "ioc-2"], "query": "x.invalid", + "abuse": ["abuse@host.invalid"], "source": "rdap"}, + ] + destination = report.email_destinations(manifest["contacts"])[0] + fields = report.feedback_fields(manifest, destination) + self._assert_no_break(fields) + self.assertEqual(dict(fields)["Version"], "1") + lookup = dict(fields) + self.assertEqual(lookup["Source"], "203.0.113.42%0AVersion: 9") + self.assertEqual(lookup["Reported-Domain"], + "a.invalid%0ASource: 192.0.2.1") + + def test_the_rendered_part_survives_pythons_own_header_setter(self): + """The end the whole defence is for: Task 6 assembles with email. + + EmailMessage raises ValueError on a header value containing a break, + so an unencoded value does not merely render oddly, it aborts the + document. Asserting through the real setter is what makes this a + test of the outcome rather than of my own notion of a break. + """ + from email.message import EmailMessage + manifest = copy.deepcopy(MANIFEST) + manifest["iocs"] = [ + {"id": "ioc-1", "type": "url", + "value": "http://a.invalid/x\r\nFeedback-Type: not-abuse
z", + "origin": "redirect-target"}, + ] + manifest["contacts"] = [ + {"iocs": ["ioc-1"], "query": "x.invalid", + "abuse": ["abuse@host.invalid"], "source": "rdap"}, + ] + destination = report.email_destinations(manifest["contacts"])[0] + part = EmailMessage() + for name, value in report.feedback_fields(manifest, destination): + part[name] = value + rendered = part.as_string() + # The property is per LINE, not per substring: the attacker's text + # may legitimately appear INSIDE a value, and asserting it absent + # would forbid reporting a URL that merely contains the words. What + # must not exist is a line a parser reads as a field of its own. + names = [line.split(":", 1)[0] for line in rendered.splitlines() + if line and not line[0].isspace() and ":" in line] + self.assertEqual(names.count("Feedback-Type"), 1) + self.assertEqual( + [n for n in names if n == "Feedback-Type"], ["Feedback-Type"]) + for line in rendered.splitlines(): + self.assertNotEqual(line.strip(), "Feedback-Type: not-abuse") + + def test_a_field_name_is_never_taken_from_data(self): + """Names are literals in this module, so no input can invent one. + + Pinned because the obvious "generalise it" refactor is a table + mapping an IOC's own type string to a field name, and a manifest is + a file the user edits: a type of "x: y\\nFeedback-Type" would then + BE a field name. The set is closed on purpose. + """ + manifest = copy.deepcopy(MANIFEST) + manifest["iocs"] = [ + {"id": "ioc-1", "type": "url\nFeedback-Type", "value": "x", + "origin": "body"}, + ] + manifest["contacts"] = [ + {"iocs": ["ioc-1"], "query": "x.invalid", + "abuse": ["abuse@host.invalid"], "source": "rdap"}, + ] + destination = report.email_destinations(manifest["contacts"])[0] + fields = report.feedback_fields(manifest, destination) + self.assertEqual( + {name for name, _ in fields}, + {"Feedback-Type", "User-Agent", "Version", "Report-Type"}) + + def test_a_non_string_value_does_not_crash_the_report(self): + """A manifest is edited by hand and JSON has numbers. + + Not a security property, but a report that raises produces nothing + at all, and this is the one module standing between a reviewed case + and a sent mail. + """ + manifest = copy.deepcopy(MANIFEST) + manifest["iocs"] = [ + {"id": "ioc-1", "type": "ipv4", "value": 42, + "origin": "received-chain"}, + ] + manifest["contacts"] = [ + {"iocs": ["ioc-1"], "query": "x.invalid", + "abuse": ["abuse@host.invalid"], "source": "rdap"}, + ] + destination = report.email_destinations(manifest["contacts"])[0] + lookup = dict(report.feedback_fields(manifest, destination)) + self.assertEqual(lookup["Source"], "42") + + if __name__ == "__main__": unittest.main() |