Neutralize CSV formulas and escape control characters in logged strings (#931)

* Neutralize CSV formulas and escape control characters in logged strings

CSV fields starting with =, +, -, @, tab, or CR get a leading apostrophe
(OWASP CSV Injection, CWE-1236). Sender-controlled subject/From and a few
aggregate-parser values are passed through the new sanitize_for_log()
before they reach error messages and logs.

Closes #929

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Sanitize more sender-controlled log values; fix sanitize_for_log docs

Escape the SMTP TLS policy-type in its error and org_name/report_id in the
duplicate-report debug lines, correct the sanitize_for_log docstring, and
drop the unreachable return in parse_report_email().

Closes #929

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Test the From-header escaping with a raw control character

mail-parser 4.8.0, which CI resolves, no longer decodes an RFC 2047
encoded word in the From header (4.6.2 did), so the previous test saw the
encoded word rather than the CR LF it was meant to observe. A raw ESC in
the display name survives both versions.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

CHANGELOG.md

@@ -1,5 +1,12 @@
 # Changelog
 
+## Unreleased
+
+### Changes
+
+- **CSV output now neutralizes spreadsheet formulas** (CWE-1236, [OWASP CSV Injection](https://owasp.org/www-community/attacks/CSV_Injection)). Every text field in the aggregate, failure, and SMTP TLS CSV output comes from a report that a third party sent, and a cell beginning with `=`, `+`, `-`, or `@` is evaluated as a formula by Excel, LibreOffice, and Google Sheets, so a reporter could set an `org_name` such as `=HYPERLINK("http://evil","x")` and have it run when an analyst opens the CSV. `_csv_safe()` now prefixes a string field that starts with `=`, `+`, `-`, `@`, a tab, or a carriage return with a single `'`, the OWASP-recommended fix, after the existing NUL strip; non-string values such as ASNs and counts are untouched. Compatibility: only cells starting with one of those six characters change, and real report fields almost never do. The JSON output and the data sent to the other output integrations are unchanged. (Closes #929)
+- **Control characters in sender-controlled strings are escaped before they reach logs and error messages.** An RFC 2047 encoded word in a `Subject` or `From` header can decode to a raw CR LF (`=?utf-8?q?line1=0D=0AINJECTED?=`), and `parse_report_email()` interpolated the decoded subject into its error messages and the `From` header into its `Parsing mail from` info line, so a sender could forge extra log lines in the mailbox watch loop's warnings. The new `parsedmarc.utils.sanitize_for_log()` replaces every non-printable character with its `unicode_escape` form (`\r\n`, `\x1b`, `\u2028`) rather than stripping it, so the log still shows that the characters were there; the ASCII space, letters in any script, and most emoji pass through, while non-ASCII spaces such as NBSP and U+3000 and format characters such as the zero-width joiner are escaped. It is applied to the subject and `From` header in `parse_report_email()`, the SMTP TLS `policy-type` in its "Invalid policy type" error, the RFC 9990 `np`, `testing`, and `discovery_method` values and the per-record parse error that the aggregate parser logs, and the `org_name` and `report_id` in the "Skipping duplicate aggregate report" debug lines in `__init__.py` and `cli.py`. (Closes #929)
+
 ## 11.0.5
 
 ### Bug fixes

docs/source/usage.md

@@ -136,6 +136,10 @@ The full set of configuration options is:
       results to STDOUT (Default: `True`)
   - `output` - str: Directory to place JSON and CSV files in. The JSON
       and CSV filename options below only take effect when this is set.
+      CSV cells that begin with `=`, `+`, `-`, `@`, a tab, or a
+      carriage return are written with a leading apostrophe (`'`), so
+      spreadsheet applications treat them as text rather than formulas
+      ([CSV injection](https://owasp.org/www-community/attacks/CSV_Injection)).
   - `archive_directory` - str: Optional. When set, successfully
       processed report files given as local file/directory path
       arguments are moved into

parsedmarc/__init__.py

@@ -77,6 +77,7 @@ from parsedmarc.utils import (
     human_timestamp_to_datetime,
     is_outlook_msg,
     parse_email,
+    sanitize_for_log,
     timestamp_to_human,
 )
 
@@ -732,7 +733,9 @@ def _parse_smtp_tls_report_policy(policy: dict[str, Any]):
         policy_type = policy["policy"]["policy-type"]
         failure_details = []
         if policy_type not in policy_types:
-            raise InvalidSMTPTLSReport(f"Invalid policy type {policy_type}")
+            raise InvalidSMTPTLSReport(
+                f"Invalid policy type {sanitize_for_log(str(policy_type))}"
+            )
         new_policy: dict[str, Any] = {
             "policy_domain": policy_domain,
             "policy_type": policy_type,
@@ -807,17 +810,35 @@ def parse_smtp_tls_report_json(report: str | bytes) -> SMTPTLSReport:
         raise InvalidSMTPTLSReport(str(e) + _exc_origin(e)) from e
 
 
-def _csv_safe(value: Any) -> Any:
-    """Strips NUL characters from a string CSV field.
+# Characters that make a spreadsheet application read a cell as a formula.
+_CSV_FORMULA_PREFIXES = ("=", "+", "-", "@", "\t", "\r")
 
-    Python 3.10's ``csv`` writer raises ``_csv.Error: need to escape, but
-    no escapechar set`` on any field containing NUL (CPython issue 97503,
-    fixed in 3.11). Report text fields come from untrusted mail, so the
-    character is dropped here, matching ``get_filename_safe_string``.
-    Non-string values are returned unchanged.
+
+def _csv_safe(value: Any) -> Any:
+    """Makes a string CSV field safe to write and to open in a spreadsheet.
+
+    Two things happen to a string, in this order:
+
+    1. NUL characters are stripped. Python 3.10's ``csv`` writer raises
+       ``_csv.Error: need to escape, but no escapechar set`` on any field
+       containing NUL (CPython issue 97503, fixed in 3.11), and the
+       character is dropped to match ``get_filename_safe_string``.
+    2. A string starting with ``=``, ``+``, ``-``, ``@``, tab, or carriage
+       return gets a leading ``'``. Spreadsheet applications evaluate such
+       a cell as a formula, and report text fields come from untrusted
+       mail, so a reporter could otherwise plant a formula like
+       ``=HYPERLINK(...)`` in the output (CWE-1236). The apostrophe is the
+       OWASP CSV Injection recommendation, and makes the application
+       treat the cell as text:
+       https://owasp.org/www-community/attacks/CSV_Injection
+
+    Non-string values (ASN integers, counts) are returned unchanged.
     """
     if isinstance(value, str):
-        return value.replace("\x00", "")
+        value = value.replace("\x00", "")
+        if value.startswith(_CSV_FORMULA_PREFIXES):
+            return "'" + value
+        return value
     return value
 
 
@@ -1135,14 +1156,16 @@ def parse_aggregate_report_xml(
             if policy_published["np"] is not None:
                 np_ = policy_published["np"]
                 if np_ not in ("none", "quarantine", "reject"):
-                    logger.warning(f"Invalid np value: {np_}")
+                    logger.warning(f"Invalid np value: {sanitize_for_log(str(np_))}")
         new_policy_published["np"] = np_
         testing = None
         if "testing" in policy_published:
             if policy_published["testing"] is not None:
                 testing = policy_published["testing"]
                 if testing not in ("n", "y"):
-                    logger.warning(f"Invalid testing value: {testing}")
+                    logger.warning(
+                        f"Invalid testing value: {sanitize_for_log(str(testing))}"
+                    )
         new_policy_published["testing"] = testing
         discovery_method = None
         if "discovery_method" in policy_published:
@@ -1150,7 +1173,8 @@ def parse_aggregate_report_xml(
                 discovery_method = policy_published["discovery_method"]
                 if discovery_method not in ("psl", "treewalk"):
                     logger.warning(
-                        f"Invalid discovery_method value: {discovery_method}"
+                        f"Invalid discovery_method value: "
+                        f"{sanitize_for_log(str(discovery_method))}"
                     )
         new_policy_published["discovery_method"] = discovery_method
         new_report["policy_published"] = new_policy_published
@@ -1175,7 +1199,9 @@ def parse_aggregate_report_xml(
                         normalize=normalize_timespan,
                     )
                 except Exception as e:
-                    logger.warning(f"Could not parse record: {e}")
+                    logger.warning(
+                        f"Could not parse record: {sanitize_for_log(str(e))}"
+                    )
 
         else:
             report_record = _parse_report_record(
@@ -2142,15 +2168,21 @@ def parse_report_email(
 
     except Exception as e:
         raise ParserError(e.__str__() + _exc_origin(e)) from e
-    subject = None
+    subject_text = "None"
     feedback_report = None
     smtp_tls_report = None
     sample = None
     is_feedback_report: bool = False
     if "From" in msg_headers:
-        logger.info("Parsing mail from {} on {}".format(msg_headers["From"], date))
+        logger.info(
+            "Parsing mail from {} on {}".format(
+                sanitize_for_log(str(msg_headers["From"])), date
+            )
+        )
     if "Subject" in msg_headers:
-        subject = msg_headers["Subject"]
+        # RFC 2047 decoding can yield raw CR/LF, so escape control
+        # characters before the subject reaches an error message or log.
+        subject_text = sanitize_for_log(str(msg_headers["Subject"]))
     for part in msg.walk():
         content_type = part.get_content_type().lower()
         payload_obj = part.get_payload()
@@ -2199,7 +2231,7 @@ def parse_report_email(
                         fields["received-date"], fields["sender-ip-address"]
                     )
                 except Exception as e:
-                    error = f'Unable to parse message with subject "{subject}": {e}{_exc_origin(e)}'
+                    error = f'Unable to parse message with subject "{subject_text}": {e}{_exc_origin(e)}'
                     raise InvalidDMARCReport(error) from e
 
                 sample = parts[1].lstrip()
@@ -2236,13 +2268,11 @@ def parse_report_email(
                 pass
 
             except InvalidDMARCReport as e:
-                error = (
-                    f'Message with subject "{subject}" is not a valid DMARC report: {e}'
-                )
+                error = f'Message with subject "{subject_text}" is not a valid DMARC report: {e}'
                 raise ParserError(error) from e
 
             except Exception as e:
-                error = f'Unable to parse message with subject "{subject}": {e}{_exc_origin(e)}'
+                error = f'Unable to parse message with subject "{subject_text}": {e}{_exc_origin(e)}'
                 raise ParserError(error) from e
 
     if feedback_report and sample:
@@ -2255,7 +2285,7 @@ def parse_report_email(
             )
         except InvalidFailureReport as e:
             error = (
-                f'Message with subject "{subject}" '
+                f'Message with subject "{subject_text}" '
                 "is not a valid "
                 f"failure DMARC report: {e}"
             )
@@ -2264,11 +2294,10 @@ def parse_report_email(
         result = {"report_type": "failure", "report": failure_report}
         return result
 
-    if result is None:
-        error = f'Message with subject "{subject}" is not a valid report'
-        raise InvalidDMARCReport(error)
-
-    return result
+    # Every branch that sets ``result`` returns immediately, so reaching this
+    # point always means no report was found.
+    error = f'Message with subject "{subject_text}" is not a valid report'
+    raise InvalidDMARCReport(error)
 
 
 # An RFC 5322 header field name (printable ASCII excluding the colon) followed
@@ -2494,8 +2523,9 @@ def _classify_parsed_email(
             aggregate_reports.append(parsed_email["report"])
         else:
             logger.debug(
-                f"Skipping duplicate aggregate report from {report_org} "
-                f"with ID: {report_id}"
+                "Skipping duplicate aggregate report from "
+                f"{sanitize_for_log(str(report_org))} "
+                f"with ID: {sanitize_for_log(str(report_id))}"
             )
     elif parsed_email["report_type"] == "failure":
         failure_reports.append(parsed_email["report"])

parsedmarc/cli.py

@@ -133,6 +133,7 @@ from parsedmarc.utils import (
     load_ip_db,
     load_psl_overrides,
     load_reverse_dns_map,
+    sanitize_for_log,
 )
 
 # Increase the max header limit for very large emails. `_MAXHEADERS` is a
@@ -3251,7 +3252,8 @@ def _main():
                 else:
                     logger.debug(
                         "Skipping duplicate aggregate report "
-                        f"from {report_org} with ID: {report_id}"
+                        f"from {sanitize_for_log(str(report_org))} "
+                        f"with ID: {sanitize_for_log(str(report_id))}"
                     )
             elif result["report_type"] == "failure":
                 failure_reports.append(result["report"])

parsedmarc/utils.py

@@ -1266,6 +1266,33 @@ def parse_email_address(original_address: str) -> dict[str, str | None]:
     }
 
 
+def sanitize_for_log(value: str) -> str:
+    """Escapes the non-printable characters of a sender-controlled string
+    so it is safe to put in a log line or an error message.
+
+    Every character for which ``str.isprintable()`` is false (CR, LF, tab,
+    ESC, U+2028, NUL, and so on) is replaced by its ``unicode_escape``
+    form, e.g. ``\\r\\n``. A sender can plant such characters in a
+    header through an RFC 2047 encoded-word (``=?utf-8?q?a=0D=0Ab?=``
+    decodes to a raw CR LF), and an unescaped CR/LF would let it forge
+    extra log lines. The characters are escaped rather than stripped so
+    the log still shows that they were present. The ASCII space, letters
+    in any script, and most emoji are printable and pass through
+    unchanged; non-ASCII spaces (NBSP, U+3000) and format characters such
+    as the zero-width joiner are not, and are escaped.
+
+    Args:
+        value (str): The string to escape
+
+    Returns:
+        str: ``value`` with non-printable characters escaped
+    """
+    return "".join(
+        ch if ch.isprintable() else ch.encode("unicode_escape").decode("ascii")
+        for ch in value
+    )
+
+
 def get_filename_safe_string(string: str | None) -> str:
     """
     Converts a string to a string that is safe to use as a filename

tests/test_init.py

@@ -1007,6 +1007,94 @@ class Test(unittest.TestCase):
         row = next(reader)
         self.assertEqual(row["organization_name"], "ExampleInc.")
 
+    def testCsvSafeNeutralizesFormulaPrefixes(self):
+        """_csv_safe prefixes a string starting with any spreadsheet
+        formula trigger with ``'`` (CWE-1236; OWASP CSV Injection,
+        https://owasp.org/www-community/attacks/CSV_Injection) and leaves
+        everything else alone, including non-strings."""
+        cases = [
+            ("=1+1", "'=1+1"),
+            ("+1", "'+1"),
+            ("-1", "'-1"),
+            ("@SUM(A1)", "'@SUM(A1)"),
+            ("\tcmd", "'\tcmd"),
+            ("\rcmd", "'\rcmd"),
+            ("\x00=1+1", "'=1+1"),
+            ("Example Inc.", "Example Inc."),
+            ("a-b=c+d@e", "a-b=c+d@e"),
+            ("", ""),
+            (15169, 15169),
+            (None, None),
+        ]
+        for value, expected in cases:
+            with self.subTest(value=value):
+                self.assertEqual(parsedmarc._csv_safe(value), expected)
+
+    def testAggregateCsvNeutralizesFormulaInOrgName(self):
+        """A reporter-supplied ``=HYPERLINK(...)`` org_name is written to
+        the aggregate CSV with a leading apostrophe, while ordinary
+        values and numeric columns are unchanged."""
+        payload = '=HYPERLINK("http://evil","x")'
+        escaped = payload.replace('"', """)
+        xml = (
+            "<feedback><report_metadata>"
+            f"<org_name>{escaped}</org_name>"
+            "<email>a@example.com</email><report_id>r-1</report_id>"
+            "<date_range><begin>1529366400</begin><end>1529452799</end>"
+            "</date_range></report_metadata><policy_published>"
+            "<domain>example.com</domain><p>none</p></policy_published>"
+            "<record><row><source_ip>192.0.2.1</source_ip><count>3</count>"
+            "<policy_evaluated><disposition>none</disposition>"
+            "<dkim>fail</dkim><spf>fail</spf></policy_evaluated></row>"
+            "<identifiers><header_from>example.com</header_from>"
+            "</identifiers><auth_results></auth_results></record></feedback>"
+        )
+        report = parsedmarc.parse_aggregate_report_xml(xml, offline=True)
+        self.assertEqual(report["report_metadata"]["org_name"], payload)
+
+        csv_text = parsedmarc.parsed_aggregate_reports_to_csv(report)
+
+        row = next(csv.DictReader(StringIO(csv_text)))
+        self.assertEqual(row["org_name"], "'" + payload)
+        self.assertEqual(row["domain"], "example.com")
+        self.assertEqual(row["count"], "3")
+
+        report["report_metadata"]["org_name"] = "Example-Org Inc."
+        row = next(
+            csv.DictReader(StringIO(parsedmarc.parsed_aggregate_reports_to_csv(report)))
+        )
+        self.assertEqual(row["org_name"], "Example-Org Inc.")
+
+    def testFailureReportCsvNeutralizesFormulaInSubject(self):
+        """A failure report sample subject starting with ``@`` is written
+        with a leading apostrophe."""
+        parsed_report = cast(
+            FailureReport,
+            parsedmarc.parse_report_file(
+                "samples/failure/dmarc_ruf_report_linkedin.eml", offline=True
+            )["report"],
+        )
+        parsed_report["parsed_sample"]["subject"] = "@evil"
+
+        csv_text = parsedmarc.parsed_failure_reports_to_csv(parsed_report)
+
+        row = next(csv.DictReader(StringIO(csv_text)))
+        self.assertEqual(row["subject"], "'@evil")
+
+    def testSmtpTlsCsvNeutralizesFormulaInOrganizationName(self):
+        """An SMTP TLS report string field starting with ``+`` is written
+        with a leading apostrophe."""
+        result = parsedmarc.parse_report_file(
+            "samples/smtp_tls/smtp_tls.json", offline=True
+        )
+        report = cast(SMTPTLSReport, result["report"])
+        report["organization_name"] = "+1-555-0100"
+
+        csv_text = parsedmarc.parsed_smtp_tls_reports_to_csv(report)
+
+        row = next(csv.DictReader(StringIO(csv_text)))
+        self.assertEqual(row["organization_name"], "'+1-555-0100")
+
     def testAggregateCsvExposesASNColumns(self):
         """The aggregate CSV output should include source_asn, source_as_name,
         and source_as_domain columns."""
@@ -3094,6 +3182,77 @@ This is not a DMARC report."""
         with self.assertRaises(parsedmarc.InvalidDMARCReport):
             parsedmarc.parse_report_email(email_str, offline=True)
 
+    def testSubjectControlCharactersEscapedInErrorMessage(self):
+        """An RFC 2047 subject that decodes to CR LF is escaped in the
+        error message, so a sender cannot forge extra log lines (the
+        mailbox loops log this message at WARNING)."""
+        email_str = (
+            "From: test@example.com\n"
+            "Subject: =?utf-8?q?line1=0D=0AINJECTED=20fake=20log?=\n"
+            "Content-Type: text/plain\n\nNot a report."
+        )
+        with self.assertRaises(parsedmarc.InvalidDMARCReport) as ctx:
+            parsedmarc.parse_report_email(email_str, offline=True)
+        message = str(ctx.exception)
+        self.assertIn("line1\\r\\nINJECTED fake log", message)
+        self.assertNotIn("\r", message)
+        self.assertNotIn("\n", message)
+
+    def testSmtpTlsPolicyTypeControlCharactersEscapedInError(self):
+        """A sender-supplied ``policy-type`` containing CR LF is escaped in
+        the ``Invalid policy type`` error, which the mailbox loops log."""
+        report = json.dumps(
+            {
+                "organization-name": "Example",
+                "date-range": {
+                    "start-datetime": "2016-04-01T00:00:00Z",
+                    "end-datetime": "2016-04-01T23:59:59Z",
+                },
+                "contact-info": "tls@example.com",
+                "report-id": "r-1",
+                "policies": [
+                    {
+                        "policy": {
+                            "policy-type": "sts\r\nWARNING:cli.py:1:INJECTED",
+                            "policy-domain": "example.com",
+                        },
+                        "summary": {
+                            "total-successful-session-count": 1,
+                            "total-failure-session-count": 0,
+                        },
+                    }
+                ],
+            }
+        )
+        with self.assertRaises(parsedmarc.InvalidSMTPTLSReport) as ctx:
+            parsedmarc.parse_smtp_tls_report_json(report)
+        message = str(ctx.exception)
+        self.assertIn("sts\\r\\nWARNING:cli.py:1:INJECTED", message)
+        self.assertNotIn("\r", message)
+        self.assertNotIn("\n", message)
+
+    def testFromHeaderControlCharactersEscapedInInfoLog(self):
+        """The ``Parsing mail from`` info line escapes control characters
+        in the From header.
+
+        The control character is a raw ESC in the display name rather than
+        an RFC 2047 encoded word: mail-parser 4.6.2 decodes encoded words
+        in ``From`` but 4.8.0 (what CI resolves) leaves them encoded, while
+        both keep a raw control character, so the test observes the
+        escaping regardless of which mail-parser is installed."""
+        email_str = (
+            'From: "a\x1bFORGED" <test@example.com>\n'
+            "Subject: Hello\n"
+            "Content-Type: text/plain\n\nNot a report."
+        )
+        with self.assertLogs("parsedmarc.log", level="INFO") as logs:
+            with self.assertRaises(parsedmarc.InvalidDMARCReport):
+                parsedmarc.parse_report_email(email_str, offline=True)
+        line = next(m for m in logs.output if "Parsing mail from" in m)
+        self.assertIn("\\x1bFORGED", line)
+        self.assertNotIn("\x1b", line)
+        self.assertEqual(len(line.splitlines()), 1)
+
     def testUnparseableDateRaisesParserError(self):
         """An unparseable Date header trips the initial mail-parse catch-all"""
         # human_timestamp_to_datetime() raises on a junk Date, which the

tests/test_utils.py

@@ -471,6 +471,31 @@ class Test(unittest.TestCase):
         self.assertTrue(parsedmarc.utils.is_outlook_msg(magic))
 
 
+class TestSanitizeForLog(unittest.TestCase):
+    """Tests for parsedmarc.utils.sanitize_for_log()"""
+
+    def testEscapesNonPrintableAndKeepsPrintable(self):
+        cases = [
+            ("plain ASCII", "plain ASCII"),
+            ("", ""),
+            ("line1\r\nINJECTED fake log", "line1\\r\\nINJECTED fake log"),
+            ("a\tb", "a\\tb"),
+            ("\x1b[31mred", "\\x1b[31mred"),
+            ("a\u2028b", "a\\u2028b"),
+            ("nul\x00byte", "nul\\x00byte"),
+            ("\u65e5\u672c\u8a9e report", "\u65e5\u672c\u8a9e report"),
+            ("ok \U0001f600", "ok \U0001f600"),
+        ]
+        for value, expected in cases:
+            with self.subTest(value=value):
+                self.assertEqual(parsedmarc.utils.sanitize_for_log(value), expected)
+
+    def testResultHasNoRawControlCharacters(self):
+        result = parsedmarc.utils.sanitize_for_log("a\r\nb\nc\rd")
+        self.assertNotIn("\r", result)
+        self.assertNotIn("\n", result)
+
+
 class TestLoadPSLOverrides(unittest.TestCase):
     """Covers `parsedmarc.utils.load_psl_overrides`."""