diff options
| -rw-r--r-- | abusectl/report.py | 118 | ||||
| -rw-r--r-- | tests/test_report.py | 199 |
2 files changed, 308 insertions, 9 deletions
diff --git a/abusectl/report.py b/abusectl/report.py index 7410ae2..7cb4a9b 100644 --- a/abusectl/report.py +++ b/abusectl/report.py @@ -44,7 +44,57 @@ _WIDTH = 72 # resource that was never reported. Something must mark the seam, and this # marks it in the only direction that is safe: a fragment ANNOUNCES that it # is a fragment, rather than a whole value having to prove it is whole. +# +# Because the marker is a character a VALUE may also contain, every literal +# backslash in a value is DOUBLED before wrapping and halved on the way +# back. Without that, a value ending in "\" is indistinguishable from a wrap +# marker, and the values here are attacker-supplied: a trailing backslash is +# legal in a URL path, and an attacker reading this source could append one +# to make their indicator garble itself in the report a desk reads. +# +# Doubling EVERY backslash rather than only a trailing one is what keeps the +# encoding unambiguous. Escaping just the last character leaves "x\\" (two +# literal backslashes) encoding to the same text as "x\" followed by a wrap, +# which is the same bug one character further along. _CONTINUATION = "\\" +_ESCAPE = "\\" + + +def _escape(value: str) -> str: + """Double every backslash so none can be read as a continuation marker. + + The inverse is _unescape(). Applied to the value only, never to the + indent or the surrounding prose, so what a reader sees differs from the + literal value in exactly one way and unwrap() undoes exactly that. + """ + return value.replace(_ESCAPE, _ESCAPE + _ESCAPE) + + +def _unescape(value: str) -> str: + """Halve the doubled backslashes _escape() produced. + + Scans left to right, consuming a doubled pair as one character. On text + _escape() actually produced, every run is even and str.replace gives + the same answer, which a mutation test confirmed over every backslash + pattern up to length 11: this is NOT protecting the round trip, and + saying otherwise would overstate what the tests hold down. + + It is kept because unwrap() is public and may be handed a line a user + edited, where a run can be odd. The scan then consumes pairs strictly + left to right and leaves the odd one alone, which is the reading that + matches how the text was written; str.replace rescans its own output + and would fold a stray backslash into the pair beside it. + """ + out = [] + index = 0 + while index < len(value): + if value.startswith(_ESCAPE + _ESCAPE, index): + out.append(_ESCAPE) + index += 2 + else: + out.append(value[index]) + index += 1 + return "".join(out) def _wrap_value(value: str, indent: str) -> list[str]: @@ -59,16 +109,44 @@ def _wrap_value(value: str, indent: str) -> list[str]: An arbitrary break plus an explicit marker is legible precisely because the marker, not the position, carries the meaning. - The value is never altered, only divided; unwrap() is the exact inverse - and there is a test asserting the round trip on a 120-character URL. + The value is ESCAPED first, so its own backslashes cannot be mistaken + for the marker, and the wrap arithmetic then runs over the escaped text: + the column limit governs what is PRINTED, and the escaped form is what + is printed. Measuring the original instead would let a value full of + backslashes overflow the line. + + A break must NEVER land between the two halves of an escaped pair, and + the loop below backs off by one character to guarantee it. This is not + tidiness: unwrap() decides whether a trailing backslash is a marker or + content by the PARITY of the run it ends, and a break inside a pair + splits that run across two lines, so both halves are counted wrongly. + A genuine marker then reads as content, the continuation line is + orphaned, and the tail of the value is silently dropped. + + That defect survived a first fix and a passing test suite, because it + needs a backslash to land exactly on the break column: it appears in + random adversarial values roughly one time in eight and in none of the + hand-written cases. Backing off one character costs a column on a line + that has a backslash at its edge, and buys an invariant that holds for + every input rather than for the inputs someone thought of. + + The value is never altered, only divided and escaped; unwrap() is the + exact inverse, asserted over adversarial values including trailing and + doubled backslashes. """ + value = _escape(value) room = _WIDTH - len(indent) - len(_CONTINUATION) if len(indent) + len(value) <= _WIDTH: return [indent + value] lines = [] while len(value) > room: - lines.append(indent + value[:room] + _CONTINUATION) - value = value[room:] + cut = room + # An odd run of backslashes ending at the cut means the last one is + # the first half of a pair; move the break before it. + if (len(value[:cut]) - len(value[:cut].rstrip(_ESCAPE))) % 2 == 1: + cut -= 1 + lines.append(indent + value[:cut] + _CONTINUATION) + value = value[cut:] lines.append(indent + value) return lines @@ -87,14 +165,36 @@ def unwrap(text: str) -> str: content: no value this module emits begins with a space, because every one of them is an indicator, a header value or an identity, all of which are stripped before they arrive. + + A trailing backslash is a marker only when the run of backslashes it + ends is ODD, because _escape() doubled every literal one. An even run is + entirely escaped content and the line ends there. Testing endswith("\\") + alone was the defect this parity check replaces: it read a value's own + trailing backslash as a marker and swallowed the following line, which + for a Subject meant absorbing the Date header beneath it. + + Rejoining happens BEFORE unescaping, so a doubled pair split across a + break is whole again before it is decoded. """ - out = [] + joined = [] for line in text.splitlines(): - if out and out[-1].endswith(_CONTINUATION): - out[-1] = out[-1][:-len(_CONTINUATION)] + line.lstrip() + if joined and _ends_with_marker(joined[-1]): + joined[-1] = joined[-1][:-len(_CONTINUATION)] + line.lstrip() else: - out.append(line) - return "\n".join(out) + joined.append(line) + return "\n".join(_unescape(line) for line in joined) + + +def _ends_with_marker(line: str) -> bool: + """Whether this line ends in a continuation marker rather than content. + + The marker is one unescaped backslash, and _escape() doubled every + literal one, so the question is purely the PARITY of the trailing run: + odd means the last backslash has no partner and is the marker, even + means every one is half of an escaped pair and the line ends here. + """ + run = len(line) - len(line.rstrip(_ESCAPE)) + return run % 2 == 1 # What each parse.py origin means in a sentence a desk can act on. diff --git a/tests/test_report.py b/tests/test_report.py index 35c68ba..3b13d11 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -723,5 +723,204 @@ class TextPart(unittest.TestCase): self.assertNotIn("Authentication results", text) +class BackslashRoundTrip(unittest.TestCase): + """A value's own backslash must never be read as a wrap marker. + + The continuation marker is a trailing "\\", and a URL path may legally + end in one. Until this was fixed the two were indistinguishable, so an + attacker who read this source could append a backslash and make their + own indicator garble itself in the report an abuse desk reads. That is + an adversarial trigger on attacker-supplied text, not an edge case. + + The property asserted throughout is the only one that closes it: + unwrap(text_part(...)) contains the value EXACTLY, for every value, + wrapped or not. Asserting "the value appears" without unwrap, or + asserting only on long values, both leave the short case open, and the + short case is the one that needs no wrapping to corrupt. + """ + + def _render_ioc(self, value: str) -> str: + manifest = copy.deepcopy(MANIFEST) + manifest["iocs"] = [{"id": "ioc-1", "type": "url", "value": value, + "origin": "body"}] + manifest["contacts"] = [ + {"iocs": ["ioc-1"], "query": "example.invalid", + "abuse": ["abuse@host.invalid"], "source": "rdap"}, + ] + destination = report.email_destinations(manifest["contacts"])[0] + return report.text_part(manifest, destination, IDENTITY) + + def _assert_round_trips(self, value: str) -> None: + text = self._render_ioc(value) + for line in text.splitlines(): + self.assertLessEqual(len(line), 72, line) + self.assertIn(value, report.unwrap(text)) + + def test_a_short_value_ending_in_a_backslash_survives(self): + """The case that needs no wrapping at all to corrupt. + + Nothing is wrapped here, yet unwrap() used to eat the following + line, merging the indicator with its own origin annotation and + rendering "http://a.invalid/xseen in a link in the message body". + One corrupt line where there were two, and a wrong indicator. + """ + self._assert_round_trips("http://a.invalid/x\\") + + def test_a_long_value_ending_in_a_backslash_survives(self): + self._assert_round_trips("http://a.invalid/" + "b" * 90 + "\\") + + def test_a_value_with_an_interior_backslash_survives(self): + self._assert_round_trips("http://a.invalid/x\\y/z") + + def test_a_value_ending_in_two_backslashes_survives(self): + """Whatever escaping is chosen must not have its own off-by-one. + + Doubling every backslash makes a trailing pair into four, and a + decoder that consumes them greedily or in the wrong order gives + back one backslash or three. This is the test that catches that. + """ + self._assert_round_trips("http://a.invalid/x\\\\") + + def test_adversarial_values_round_trip_exactly(self): + """A handful of shapes chosen to sit on the seams. + + The two boundary values matter most: a value that exactly fills a + line and one a single character over it are where an off-by-one in + the wrap arithmetic lives, and a backslash landing exactly on the + break column is where escaping and wrapping interact. + """ + indent = 2 + room = 72 - indent + values = [ + "http://a.invalid/x\\", + "http://a.invalid/x\\y/z", + "http://a.invalid/x\\\\", + "\\" + "a" * 40, + "a" * 40 + "\\", + "http://a.invalid/" + "b" * 90 + "\\", + "a" * room, # exactly fills the line + "a" * (room + 1), # one character over + "a" * (room - 1) + "\\", # backslash at the break + "a" * room + "\\", + "\\\\" + "c" * 80 + "\\\\", + ] + for value in values: + with self.subTest(value=value): + self._assert_round_trips(value) + + def test_a_backslash_landing_on_the_break_column_survives(self): + """The case that a passing suite still missed. + + Escaping doubles each backslash, and a break falling BETWEEN the + two halves of a pair splits the run unwrap() counts the parity of. + Both halves are then misread, a real marker reads as content, the + continuation line is orphaned and the tail of the value is silently + dropped. It needs a backslash at exactly the break column, so no + hand-written case found it; a randomised sweep failed 454 of 3538. + + Walking the backslash across every position around the boundary is + what makes this deterministic rather than luck. + """ + room = 72 - 2 # indent is two spaces for an indicator line + for offset in range(-4, 5): + position = room + offset + if position < 1: + continue + value = "a" * position + "\\" + "b" * 30 + with self.subTest(offset=offset): + self._assert_round_trips(value) + + def test_a_run_of_backslashes_across_the_break_survives(self): + """A run is where an off-by-one in the back-off hides. + + Backing off one character is correct only if the character it lands + on is the first half of a pair; a run of three or four exercises + whether the parity test looks at the run rather than at one + character. + """ + room = 72 - 2 + for length in range(1, 6): + for offset in range(-3, 4): + position = room + offset + if position < 1: + continue + value = "a" * position + "\\" * length + "b" * 20 + with self.subTest(length=length, offset=offset): + self._assert_round_trips(value) + + def test_a_value_that_is_entirely_backslashes_survives(self): + """Escaping doubles the length, so this is the worst case for both + the wrap arithmetic and the parity test at once.""" + for length in (1, 2, 3, 34, 35, 36, 70, 71): + with self.subTest(length=length): + self._assert_round_trips("\\" * length) + + def test_an_attacker_subject_cannot_corrupt_the_header_block(self): + """Subject is attacker-controlled and sits beside headers it can eat. + + This is worse than the URL case: a trailing backslash on Subject + used to swallow the following line, rendering + "Subject: Verify nowDate: Mon, 07 Sep 2026 09:12:40 +0000". The + attacker's own text destroys a DIFFERENT field's value, so the + block misstates what the message declared, which is the one thing + that block exists to report faithfully. + """ + subject = "Verify now\\" + manifest = copy.deepcopy(MANIFEST) + manifest["headers"] = [ + ("Subject", subject), + ("Date", "Mon, 07 Sep 2026 09:12:40 +0000"), + ] + destination = report.email_destinations(manifest["contacts"])[0] + text = report.text_part(manifest, destination, IDENTITY) + + for line in text.splitlines(): + self.assertLessEqual(len(line), 72, line) + joined = report.unwrap(text) + self.assertIn(f"Subject: {subject}", joined) + # The Date must survive intact rather than being absorbed. + self.assertIn("Date: Mon, 07 Sep 2026 09:12:40 +0000", joined) + + def test_a_display_name_ending_in_a_backslash_survives(self): + """The From display name is attacker-controlled too, and a sweep + has already found a spoofed one.""" + value = '"Example Bank\\" <phish@sender.invalid>' + manifest = copy.deepcopy(MANIFEST) + manifest["headers"] = [("From", value), + ("Subject", "Your account requires check")] + destination = report.email_destinations(manifest["contacts"])[0] + text = report.text_part(manifest, destination, IDENTITY) + + joined = report.unwrap(text) + self.assertIn(f"From: {value}", joined) + self.assertIn("Subject: Your account requires check", joined) + + def test_unwrap_reads_a_marker_by_parity_not_by_a_trailing_backslash(self): + """unwrap() is PUBLIC, so its input is not only our own output. + + A case manifest is a file the user edits and a desk may script + against the text part, so unwrap() must decide correctly on a line + it did not generate. Inside generated text the wrap back-off means + a marker always follows an even run, so parity and a plain + endswith() agree and neither is distinguishable by a round-trip + test. They disagree here, on a line ending in an escaped pair and + nothing else: that is content, and the following line must NOT be + absorbed into it. + """ + # "a\\" escaped is a value ending in one literal backslash, whole + # on its line. endswith() reads the second half as a marker. + self.assertEqual(report.unwrap(" a\\\\\n next"), " a\\\n next") + # An odd run IS a marker: two escaped halves plus the marker. + self.assertEqual(report.unwrap(" a\\\\\\\n next"), " a\\next") + + def test_an_identity_containing_a_backslash_survives(self): + identity = {"name": "A Reporter\\", "org": "Example Consulting", + "email": "reporter@example.org"} + destination = report.email_destinations(MANIFEST["contacts"])[0] + text = report.text_part(MANIFEST, destination, identity) + self.assertIn("A Reporter\\", report.unwrap(text)) + self.assertIn("Generated by abusectl.", report.unwrap(text)) + + if __name__ == "__main__": unittest.main() |