diff --git a/README.md b/README.md index e584faa..22f746f 100644 --- a/README.md +++ b/README.md @@ -47,7 +47,7 @@ Amazon APM work order emails (from Hexagon EAM / HxGN SmartCloud) are received a 2. Gmail filter forwards APM emails to `apm@int.seahaven.com` (SES). 3. SES drops the raw MIME into `s3://workorder-ingest-emails-{AccountId}/inbound/`. 4. S3 triggers the `workorder-email-processor` Lambda. -5. Fail-closed sender authentication (INFRA-107): the SES-stamped `Authentication-Results` header must show `dkim=pass` for `seahaven.com` (the Google Groups forward re-signs the mail; see [Sender authentication](#sender-authentication-infra-107)); otherwise the email is logged and dropped. +5. Fail-closed sender authentication (INFRA-107): the SES-stamped `Authentication-Results` header must show `dkim=pass` for the domain that re-signs the forward (currently allowlisted as `seahaven.com` — see the validation caveat under [Sender authentication](#sender-authentication-infra-107)); otherwise the email is logged and dropped. 6. Claude extracts structured JSON (work order ID, site code, severity, priority, dates, assigned technician). 7. Work order upserted to `WorkOrders`, event/comment appended to `WorkOrderComments`. @@ -85,10 +85,12 @@ The allowlist is the `ALLOWED_DKIM_DOMAINS` Lambda environment variable (comma-s | Pipeline | `ALLOWED_DKIM_DOMAINS` | Why | |---|---|---| -| Work orders | `seahaven.com` | APM mail arrives via the `apm@` Google Groups forward, which re-signs as `seahaven.com` (the original `hxgnsmartcloud.com` signature does not survive the forward) | +| Work orders | `seahaven.com` | APM mail reaches `apm@int.seahaven.com` via a forward off `amazon@seahavenind.com`; the allowlist trusts the domain that **re-signs** DKIM on that forward (the original `hxgnsmartcloud.com` signature does not survive it). **Must be validated against a live SES-stamped header** — a plain Gmail auto-forward re-signs under the *sending Workspace* domain (`seahavenind.com` / a `*.gappssmtp.com` key), **not** `seahaven.com`; only a Google Group (or Workspace routing) with "sign as `seahaven.com`" produces `dkim=pass header.i=@seahaven.com`. If the observed re-signing domain differs, update this value (do **not** widen it to a shared key like `*.gappssmtp.com`, which any Google customer's mail would pass). The `workorder-email-processor-sender-auth-rejected` alarm pages if this assumption is wrong instead of silently dropping every work order. | | Purchase orders | `amazon.coupahost.com` | Coupa signs as `amazon.coupahost.com`. `amazonses.com` also passes but is deliberately not allowlisted — every SES customer's mail passes for it | -On any failure (env var unset, header missing/unparseable, verdict fail, unaligned domain) the processor logs a structured `sender_auth_rejected` warning with the reason and S3 key, skips the email, and returns normally — rejected mail never triggers Lambda retries or DLQ messages. Unit tests live in `tests/test_ses_auth.py`. +> **Clause-injection hardening:** SES echoes attacker-controlled SMTP-session tokens (`envelope-from`, `helo`, `header.from`) into its own `Authentication-Results` value, and an RFC 5321 quoted-local-part MAIL FROM may legally contain `;` and spaces. The parser therefore tokenises comment- and quoted-string-aware (RFC 8601 / RFC 5322): CFWS comments `(...)` are stripped and clauses are split only on semicolons **outside** a quoted string, so a `;` inside a quoted `envelope-from=` value can never be torn into a forged `dkim=pass` clause. + +On any failure (env var unset, header missing/unparseable, verdict fail, unaligned domain) the processor logs a structured `sender_auth_rejected` warning with the reason and S3 key, skips the email, and returns normally — rejected mail never triggers Lambda retries or DLQ messages, but the `-sender-auth-rejected` CloudWatch alarm (see [CloudWatch alarms](#cloudwatch-alarms)) pages on a rejection spike so a drift-induced outage is not silent. Unit tests live in `tests/test_ses_auth.py`. **Failure handling (INFRA-41):** Each email-processor is async-invoked (S3 → Lambda). Both have a CDK-managed SQS dead-letter queue (`dead_letter_queue=`, 14-day retention, SSL-enforced) so a failed parse is captured rather than silently dropped after Lambda's retries. @@ -103,6 +105,9 @@ Every alarm is **ALARM-only** (no OK action), sends to the shared `site-alerts` | `-errors` | `po-email-processor`, `po-ingest-site-extractor`, `workorder-email-processor` | `Errors` Sum, 5 min, `> 0`, eval 1 | | `-throttles` | `po-email-processor`, `po-ingest-site-extractor`, `po-web-ui`, `workorder-email-processor` | `Throttles` Sum, 5 min, `> 0`, eval 1 | | `-duration` | `po-email-processor`, `po-ingest-site-extractor`, `po-web-ui` (p99); `workorder-email-processor` (p95) | `Duration` percentile, 5 min, `>= 45000` ms (75% of the 60s timeout), eval 3 / datapoints 2 | +| `-sender-auth-rejected` | `po-email-processor`, `workorder-email-processor` | Log-metric-filter count (namespace `Seahaven/ProcurementIngest`, `default_value=0`) on `sender_auth_rejected` warnings, `Sum` 15 min, `>= 3`, eval 1 | + +The `-sender-auth-rejected` alarm closes the silent-drop gap in INFRA-107: a rejected email returns normally (no error, no retry, no DLQ message), so without a log-metric filter a signing-domain drift or a wrong allowlist would discard 100% of legitimate mail while every other alarm stayed green. `>= 3` rejections in 15 minutes tolerates the occasional spoofed probe to the internal ingest address but pages quickly on a genuine false-reject storm; the threshold is easy to tune in the CDK helper. The `-duration` and `-throttles` alarms for `po-email-processor` and `workorder-email-processor` supersede the orphaned, CLI-created `Lambda-Duration-*` / `Lambda-Throttles-*` alarms (deleted post-deploy). diff --git a/cdk/po_stack.py b/cdk/po_stack.py index 42a1a30..2840967 100644 --- a/cdk/po_stack.py +++ b/cdk/po_stack.py @@ -77,6 +77,67 @@ def _add_ddb_alarms(scope, id_prefix, table, alarm_name_prefix, alarm_topic): ).add_alarm_action(cw_actions.SnsAction(alarm_topic)) +# CloudWatch namespace for the log-derived sender-authentication metrics. +_SENDER_AUTH_METRIC_NAMESPACE = "Seahaven/ProcurementIngest" + + +def _add_sender_auth_rejected_alarm(scope, id_prefix, function_name, alarm_topic): + """Metric-filter + alarm on ``sender_auth_rejected`` warnings (INFRA-107). + + A rejected inbound email is skipped without erroring the invocation, so it + is invisible to the Errors/Throttles/DLQ alarms. This turns the structured + warning log into a CloudWatch metric and pages when rejections spike -- + catching a silent false-reject storm (allowlist wrong, signing-domain + drift, SES header-format change) that would otherwise discard legitimate + mail while the pipeline reports healthy. + + ALARM-only SnsAction to site-alerts; no OK action. The metric filter reads + the function's own log group (imported by the deterministic + ``/aws/lambda/`` name, created by the function's log_retention). A plain + substring pattern is used because Lambda prefixes each line with its own + level/timestamp/request-id, so the JSON payload is not a standalone JSON + log event a `{$.event=...}` pattern could match. + """ + metric_name = f"{function_name}-sender-auth-rejected" + logs.MetricFilter( + scope, + f"{id_prefix}SenderAuthRejectedFilter", + log_group=logs.LogGroup.from_log_group_name( + scope, + f"{id_prefix}LogGroup", + f"/aws/lambda/{function_name}", + ), + filter_pattern=logs.FilterPattern.literal('"sender_auth_rejected"'), + metric_namespace=_SENDER_AUTH_METRIC_NAMESPACE, + metric_name=metric_name, + metric_value="1", + default_value=0, + ) + + # >=3 rejections in a 15-min window pages: a lone spoofed probe to the + # (obscure, internal) ingest address is tolerated, but a genuine + # false-reject storm -- where legitimate senders are being dropped -- trips + # quickly. Threshold is intentionally conservative and easy to tune. + cloudwatch.Metric( + namespace=_SENDER_AUTH_METRIC_NAMESPACE, + metric_name=metric_name, + period=Duration.minutes(15), + statistic="Sum", + ).create_alarm( + scope, + f"{id_prefix}SenderAuthRejectedAlarm", + alarm_name=f"{function_name}-sender-auth-rejected", + alarm_description=( + f"{function_name} rejected inbound mail on sender authentication " + "(possible allowlist/DKIM-domain drift silently dropping real mail)" + ), + threshold=3, + evaluation_periods=1, + comparison_operator=cloudwatch.ComparisonOperator.GREATER_THAN_OR_EQUAL_TO_THRESHOLD, + treat_missing_data=cloudwatch.TreatMissingData.NOT_BREACHING, + ).add_alarm_action(cw_actions.SnsAction(alarm_topic)) + + class PoIngestStack(Stack): def __init__(self, scope: Construct, construct_id: str, **kwargs): super().__init__(scope, construct_id, **kwargs) @@ -218,6 +279,20 @@ class PoIngestStack(Stack): treat_missing_data=cloudwatch.TreatMissingData.NOT_BREACHING, ).add_alarm_action(cw_actions.SnsAction(alarm_topic)) + # --- Sender-auth rejection alarm (INFRA-107) --- + # A rejected email (bad/unaligned DKIM verdict) returns normally, so it + # produces NO Lambda error, NO DLQ message and NO retry -- only a + # `sender_auth_rejected` warning log. Without this metric filter + alarm a + # domain drift (Coupa rotates its signing subdomain, SES changes its + # Authentication-Results format, the allowlist is wrong) would silently + # discard 100% of legitimate PO mail while every other alarm stays green. + # A CloudWatch Logs metric filter turns those warnings into a metric so a + # false-reject storm pages instead of vanishing. default_value=0 keeps the + # series populated (alarm stays OK, never INSUFFICIENT_DATA) between events. + _add_sender_auth_rejected_alarm( + self, "EmailProcessor", "po-email-processor", alarm_topic + ) + # --- Throttles alarm: po-email-processor --- # Any throttled invocation (concurrency cap hit) in a 5-min window pages. # ALARM-only to site-alerts; no OK action; NOT_BREACHING when no data. diff --git a/cdk/wo_stack.py b/cdk/wo_stack.py index c70a9e2..c0aa477 100644 --- a/cdk/wo_stack.py +++ b/cdk/wo_stack.py @@ -76,6 +76,70 @@ def _add_ddb_alarms(scope, id_prefix, table, alarm_name_prefix, alarm_topic): ).add_alarm_action(cw_actions.SnsAction(alarm_topic)) +# CloudWatch namespace for the log-derived sender-authentication metrics. +_SENDER_AUTH_METRIC_NAMESPACE = "Seahaven/ProcurementIngest" + + +def _add_sender_auth_rejected_alarm(scope, id_prefix, function_name, alarm_topic): + """Metric-filter + alarm on ``sender_auth_rejected`` warnings (INFRA-107). + + A rejected inbound email is skipped without erroring the invocation, so it + is invisible to the Errors/Throttles/DLQ alarms. This turns the structured + warning log into a CloudWatch metric and pages when rejections spike -- + catching a silent false-reject storm (allowlist wrong, signing-domain + drift, SES header-format change) that would otherwise discard legitimate + mail while the pipeline reports healthy. This is the safety net for the WO + allowlist domain assumption (seahaven.com) -- if the real Gmail-forward + re-signing domain differs, this alarm surfaces it instead of a silent + work-order outage. + + ALARM-only SnsAction to site-alerts; no OK action. The metric filter reads + the function's own log group (imported by the deterministic + ``/aws/lambda/`` name, created by the function's log_retention). A plain + substring pattern is used because Lambda prefixes each line with its own + level/timestamp/request-id, so the JSON payload is not a standalone JSON + log event a `{$.event=...}` pattern could match. + """ + metric_name = f"{function_name}-sender-auth-rejected" + logs.MetricFilter( + scope, + f"{id_prefix}SenderAuthRejectedFilter", + log_group=logs.LogGroup.from_log_group_name( + scope, + f"{id_prefix}LogGroup", + f"/aws/lambda/{function_name}", + ), + filter_pattern=logs.FilterPattern.literal('"sender_auth_rejected"'), + metric_namespace=_SENDER_AUTH_METRIC_NAMESPACE, + metric_name=metric_name, + metric_value="1", + default_value=0, + ) + + # >=3 rejections in a 15-min window pages: a lone spoofed probe to the + # (obscure, internal) ingest address is tolerated, but a genuine + # false-reject storm -- where legitimate senders are being dropped -- trips + # quickly. Threshold is intentionally conservative and easy to tune. + cloudwatch.Metric( + namespace=_SENDER_AUTH_METRIC_NAMESPACE, + metric_name=metric_name, + period=Duration.minutes(15), + statistic="Sum", + ).create_alarm( + scope, + f"{id_prefix}SenderAuthRejectedAlarm", + alarm_name=f"{function_name}-sender-auth-rejected", + alarm_description=( + f"{function_name} rejected inbound mail on sender authentication " + "(possible allowlist/DKIM-domain drift silently dropping real mail)" + ), + threshold=3, + evaluation_periods=1, + comparison_operator=cloudwatch.ComparisonOperator.GREATER_THAN_OR_EQUAL_TO_THRESHOLD, + treat_missing_data=cloudwatch.TreatMissingData.NOT_BREACHING, + ).add_alarm_action(cw_actions.SnsAction(alarm_topic)) + + class WorkorderIngestStack(Stack): def __init__(self, scope: Construct, construct_id: str, **kwargs): super().__init__(scope, construct_id, **kwargs) @@ -236,6 +300,19 @@ class WorkorderIngestStack(Stack): treat_missing_data=cloudwatch.TreatMissingData.NOT_BREACHING, ).add_alarm_action(cw_actions.SnsAction(alarm_topic)) + # --- Sender-auth rejection alarm (INFRA-107) --- + # A rejected email (bad/unaligned DKIM verdict) returns normally, so it + # produces NO Lambda error, NO DLQ message and NO retry -- only a + # `sender_auth_rejected` warning log. The WO allowlist trusts dkim=pass + # for seahaven.com on the assumption the apm@ forward re-signs there; if + # that assumption is wrong (e.g. a Gmail auto-forward re-signs under a + # different domain), 100% of legitimate work-order mail is silently + # dropped. This metric filter + alarm turns those warnings into a paging + # signal so a false-reject storm surfaces instead of a silent outage. + _add_sender_auth_rejected_alarm( + self, "EmailProcessor", "workorder-email-processor", alarm_topic + ) + # --- Throttles alarm: workorder-email-processor --- # Any throttled invocation (concurrency cap hit) in a 5-min window pages. # ALARM-only to site-alerts; no OK action; NOT_BREACHING when no data. diff --git a/lambdas/po/email_processor/ses_auth.py b/lambdas/po/email_processor/ses_auth.py index cf2b43e..4bad288 100644 --- a/lambdas/po/email_processor/ses_auth.py +++ b/lambdas/po/email_processor/ses_auth.py @@ -27,6 +27,25 @@ Observed SES format (2026-07-15, both ingest buckets):: Note SES reports the passing DKIM identity as ``header.i=@`` (RFC 6376 AUID), not ``header.d=``; the parser accepts both. + +**Clause injection defence (INFRA-107 hardening).** SES echoes several +attacker-controlled SMTP-session tokens into its own Authentication-Results +value as their own semicolon-delimited property clauses -- notably +``envelope-from=``, ``helo=`` and ``header.from=``. An RFC 5321 quoted-local-part MAIL FROM may legally contain +spaces and semicolons, e.g.:: + + MAIL FROM:<"x; dkim=pass header.i=@amazon.coupahost.com"@attacker.com> + +which SES renders verbatim as ``envelope-from="x; dkim=pass +header.i=@amazon.coupahost.com"@attacker.com;``. A naive ``split(";")`` +would tear the quoted string apart and manufacture a synthetic +``dkim=pass header.i=@amazon.coupahost.com`` clause out of attacker input. +The parser therefore tokenises per RFC 8601 / RFC 5322 structure: CFWS +comments ``(...)`` are stripped first, and the value is split into clauses +only on semicolons that sit *outside* a quoted-string. A ``;`` inside a +quoted ``pvalue`` stays part of that one property clause and can never be +read as the start of a ``dkim=`` methodspec. """ import email.parser @@ -44,9 +63,11 @@ SES_AUTHSERV_ID = "amazonses.com" # One resinfo clause of an Authentication-Results value, e.g. # "dkim=pass header.i=@seahaven.com". The clause must START with the # method=result pair; header.d= / header.i= may appear anywhere after it. -# The result token must be terminated by end-of-clause, whitespace, or a -# comment so "dkim=pass-anything" can never be read as "pass". -_DKIM_RESULT_RE = re.compile(r"^dkim\s*=\s*([a-z0-9]+)(?=$|[\s(])", re.IGNORECASE) +# The result token must be terminated by end-of-clause or whitespace so +# "dkim=pass-anything" can never be read as "pass". (Comments are stripped +# before matching, so the pre-strip "dkim=pass(comment)" form arrives here +# as "dkim=pass ..." and still terminates on whitespace.) +_DKIM_RESULT_RE = re.compile(r"^dkim\s*=\s*([a-z0-9]+)(?=$|\s)", re.IGNORECASE) _HEADER_D_RE = re.compile(r"header\.d\s*=\s*\"?([^\s\";]+)", re.IGNORECASE) _HEADER_I_RE = re.compile(r"header\.i\s*=\s*\"?([^\s\";]+)", re.IGNORECASE) @@ -64,15 +85,120 @@ def _unfold(value: str) -> str: return re.sub(r"[\r\n\t ]+", " ", value).strip() +def _strip_comments(value: str) -> tuple: + """Remove RFC 5322 CFWS comments ``(...)`` from an unfolded header value. + + Comments may nest and may contain quoted pairs (``\\)``). A ``(`` that + appears *inside* a quoted-string is literal text, not a comment start, + so quoted-strings (which carry attacker-controlled ``pvalue`` content + such as a quoted MAIL FROM local part) are passed through untouched. + Dropping comments first means comment-embedded ``header.i=`` / ``;`` + fakes can never influence clause splitting or domain extraction. + + Returns ``(stripped_text, well_formed)``. ``well_formed`` is False when + the value ends inside an unterminated comment or quoted-string, i.e. the + parens/quotes are unbalanced. Callers reject on ``not well_formed`` so a + malformed header (which could otherwise be mis-tokenised) fails closed + rather than being partially parsed. + """ + out = [] + depth = 0 # comment nesting depth + in_quote = False # inside a quoted-string (only tracked at depth 0) + i = 0 + n = len(value) + while i < n: + c = value[i] + if depth > 0: + # Inside a comment: only quoted-pairs and nested parens matter. + if c == "\\": + i += 2 + continue + if c == "(": + depth += 1 + elif c == ")": + depth -= 1 + i += 1 + continue + if in_quote: + out.append(c) + if c == "\\" and i + 1 < n: + out.append(value[i + 1]) + i += 2 + continue + if c == '"': + in_quote = False + i += 1 + continue + # Normal context (outside any comment or quoted-string). + if c == "(": + depth += 1 + i += 1 + continue + if c == '"': + in_quote = True + out.append(c) + i += 1 + well_formed = depth == 0 and not in_quote + return "".join(out), well_formed + + +def _split_clauses(value: str) -> list: + """Split a comment-free header value into clauses on top-level ``;``. + + A semicolon inside a quoted-string is preserved as part of the clause, + so an attacker-controlled quoted ``pvalue`` (e.g. a quoted MAIL FROM + echoed into ``envelope-from=``) cannot smuggle in a fake ``dkim=pass`` + clause. Callers must run :func:`_strip_comments` first. + """ + clauses = [] + buf = [] + in_quote = False + i = 0 + n = len(value) + while i < n: + c = value[i] + if in_quote: + buf.append(c) + if c == "\\" and i + 1 < n: + buf.append(value[i + 1]) + i += 2 + continue + if c == '"': + in_quote = False + i += 1 + continue + if c == '"': + in_quote = True + buf.append(c) + i += 1 + continue + if c == ";": + clauses.append("".join(buf)) + buf = [] + i += 1 + continue + buf.append(c) + i += 1 + clauses.append("".join(buf)) + return clauses + + def parse_authentication_results(value: str) -> tuple: """Parse one Authentication-Results header value. Returns ``(authserv_id, passing_dkim_domains)`` where the domains are the lowercased d=/i= domains of every ``dkim=pass`` clause. Malformed input yields ``("", frozenset())``, which callers treat as a rejection. + + Tokenisation is comment- and quoted-string-aware (RFC 8601 / RFC 5322) + so attacker-controlled tokens SES echoes into its header (envelope-from, + helo, header.from) cannot be split into a forged ``dkim=pass`` clause. """ - text = _unfold(value) - clauses = [c.strip() for c in text.split(";")] + text, well_formed = _strip_comments(_unfold(value)) + if not well_formed: + # Unbalanced quotes/comments: refuse to guess how to tokenise it. + return "", frozenset() + clauses = [c.strip() for c in _split_clauses(text)] if not clauses or not clauses[0]: return "", frozenset() diff --git a/lambdas/wo/email_processor/ses_auth.py b/lambdas/wo/email_processor/ses_auth.py index cf2b43e..4bad288 100644 --- a/lambdas/wo/email_processor/ses_auth.py +++ b/lambdas/wo/email_processor/ses_auth.py @@ -27,6 +27,25 @@ Observed SES format (2026-07-15, both ingest buckets):: Note SES reports the passing DKIM identity as ``header.i=@`` (RFC 6376 AUID), not ``header.d=``; the parser accepts both. + +**Clause injection defence (INFRA-107 hardening).** SES echoes several +attacker-controlled SMTP-session tokens into its own Authentication-Results +value as their own semicolon-delimited property clauses -- notably +``envelope-from=``, ``helo=`` and ``header.from=``. An RFC 5321 quoted-local-part MAIL FROM may legally contain +spaces and semicolons, e.g.:: + + MAIL FROM:<"x; dkim=pass header.i=@amazon.coupahost.com"@attacker.com> + +which SES renders verbatim as ``envelope-from="x; dkim=pass +header.i=@amazon.coupahost.com"@attacker.com;``. A naive ``split(";")`` +would tear the quoted string apart and manufacture a synthetic +``dkim=pass header.i=@amazon.coupahost.com`` clause out of attacker input. +The parser therefore tokenises per RFC 8601 / RFC 5322 structure: CFWS +comments ``(...)`` are stripped first, and the value is split into clauses +only on semicolons that sit *outside* a quoted-string. A ``;`` inside a +quoted ``pvalue`` stays part of that one property clause and can never be +read as the start of a ``dkim=`` methodspec. """ import email.parser @@ -44,9 +63,11 @@ SES_AUTHSERV_ID = "amazonses.com" # One resinfo clause of an Authentication-Results value, e.g. # "dkim=pass header.i=@seahaven.com". The clause must START with the # method=result pair; header.d= / header.i= may appear anywhere after it. -# The result token must be terminated by end-of-clause, whitespace, or a -# comment so "dkim=pass-anything" can never be read as "pass". -_DKIM_RESULT_RE = re.compile(r"^dkim\s*=\s*([a-z0-9]+)(?=$|[\s(])", re.IGNORECASE) +# The result token must be terminated by end-of-clause or whitespace so +# "dkim=pass-anything" can never be read as "pass". (Comments are stripped +# before matching, so the pre-strip "dkim=pass(comment)" form arrives here +# as "dkim=pass ..." and still terminates on whitespace.) +_DKIM_RESULT_RE = re.compile(r"^dkim\s*=\s*([a-z0-9]+)(?=$|\s)", re.IGNORECASE) _HEADER_D_RE = re.compile(r"header\.d\s*=\s*\"?([^\s\";]+)", re.IGNORECASE) _HEADER_I_RE = re.compile(r"header\.i\s*=\s*\"?([^\s\";]+)", re.IGNORECASE) @@ -64,15 +85,120 @@ def _unfold(value: str) -> str: return re.sub(r"[\r\n\t ]+", " ", value).strip() +def _strip_comments(value: str) -> tuple: + """Remove RFC 5322 CFWS comments ``(...)`` from an unfolded header value. + + Comments may nest and may contain quoted pairs (``\\)``). A ``(`` that + appears *inside* a quoted-string is literal text, not a comment start, + so quoted-strings (which carry attacker-controlled ``pvalue`` content + such as a quoted MAIL FROM local part) are passed through untouched. + Dropping comments first means comment-embedded ``header.i=`` / ``;`` + fakes can never influence clause splitting or domain extraction. + + Returns ``(stripped_text, well_formed)``. ``well_formed`` is False when + the value ends inside an unterminated comment or quoted-string, i.e. the + parens/quotes are unbalanced. Callers reject on ``not well_formed`` so a + malformed header (which could otherwise be mis-tokenised) fails closed + rather than being partially parsed. + """ + out = [] + depth = 0 # comment nesting depth + in_quote = False # inside a quoted-string (only tracked at depth 0) + i = 0 + n = len(value) + while i < n: + c = value[i] + if depth > 0: + # Inside a comment: only quoted-pairs and nested parens matter. + if c == "\\": + i += 2 + continue + if c == "(": + depth += 1 + elif c == ")": + depth -= 1 + i += 1 + continue + if in_quote: + out.append(c) + if c == "\\" and i + 1 < n: + out.append(value[i + 1]) + i += 2 + continue + if c == '"': + in_quote = False + i += 1 + continue + # Normal context (outside any comment or quoted-string). + if c == "(": + depth += 1 + i += 1 + continue + if c == '"': + in_quote = True + out.append(c) + i += 1 + well_formed = depth == 0 and not in_quote + return "".join(out), well_formed + + +def _split_clauses(value: str) -> list: + """Split a comment-free header value into clauses on top-level ``;``. + + A semicolon inside a quoted-string is preserved as part of the clause, + so an attacker-controlled quoted ``pvalue`` (e.g. a quoted MAIL FROM + echoed into ``envelope-from=``) cannot smuggle in a fake ``dkim=pass`` + clause. Callers must run :func:`_strip_comments` first. + """ + clauses = [] + buf = [] + in_quote = False + i = 0 + n = len(value) + while i < n: + c = value[i] + if in_quote: + buf.append(c) + if c == "\\" and i + 1 < n: + buf.append(value[i + 1]) + i += 2 + continue + if c == '"': + in_quote = False + i += 1 + continue + if c == '"': + in_quote = True + buf.append(c) + i += 1 + continue + if c == ";": + clauses.append("".join(buf)) + buf = [] + i += 1 + continue + buf.append(c) + i += 1 + clauses.append("".join(buf)) + return clauses + + def parse_authentication_results(value: str) -> tuple: """Parse one Authentication-Results header value. Returns ``(authserv_id, passing_dkim_domains)`` where the domains are the lowercased d=/i= domains of every ``dkim=pass`` clause. Malformed input yields ``("", frozenset())``, which callers treat as a rejection. + + Tokenisation is comment- and quoted-string-aware (RFC 8601 / RFC 5322) + so attacker-controlled tokens SES echoes into its header (envelope-from, + helo, header.from) cannot be split into a forged ``dkim=pass`` clause. """ - text = _unfold(value) - clauses = [c.strip() for c in text.split(";")] + text, well_formed = _strip_comments(_unfold(value)) + if not well_formed: + # Unbalanced quotes/comments: refuse to guess how to tokenise it. + return "", frozenset() + clauses = [c.strip() for c in _split_clauses(text)] if not clauses or not clauses[0]: return "", frozenset() diff --git a/tests/test_ses_auth.py b/tests/test_ses_auth.py index 86ad49d..b26676c 100644 --- a/tests/test_ses_auth.py +++ b/tests/test_ses_auth.py @@ -108,6 +108,73 @@ class TestParseAuthenticationResults: ) assert passing == frozenset({"seahaven.com"}) + def test_quoted_semicolon_in_envelope_from_is_not_split(self, ses_auth): + # RFC 5321 quoted-local-part MAIL FROM can carry ';' and spaces; SES + # echoes it into envelope-from=. The ';' inside the quotes must stay + # part of the one envelope-from clause, never a synthetic dkim clause. + value = ( + "amazonses.com; spf=pass client-ip=1.2.3.4;" + ' envelope-from="x; dkim=pass header.i=@amazon.coupahost.com"@attacker.com;' + " helo=mail.attacker.com; dkim=fail header.i=@attacker.com;" + ) + _, passing = ses_auth.parse_authentication_results(value) + assert passing == frozenset() + + def test_quoted_pair_in_envelope_from_is_not_split(self, ses_auth): + # A quoted-pair (\") inside the quoted local part must not prematurely + # end the quoted-string and re-expose the smuggled tokens. + value = ( + 'amazonses.com; envelope-from="a\\"; dkim=pass header.d=seahaven.com"@evil.com;' + " dkim=fail header.i=@evil.com;" + ) + _, passing = ses_auth.parse_authentication_results(value) + assert passing == frozenset() + + def test_helo_injection_is_not_split(self, ses_auth): + # helo= is attacker-influenced too; a crafted value quoting a fake + # methodspec must not manufacture a passing clause. + value = ( + 'amazonses.com; helo="h; dkim=pass header.i=@seahaven.com";' + " dkim=fail header.i=@attacker.com;" + ) + _, passing = ses_auth.parse_authentication_results(value) + assert passing == frozenset() + + def test_comment_embedded_header_i_is_ignored(self, ses_auth): + # A CFWS comment carrying a fake header.i must be stripped before + # domain extraction, so only the real (failing) result is considered. + _, passing = ses_auth.parse_authentication_results( + "amazonses.com; dkim=fail (header.i=@seahaven.com) header.i=@attacker.com" + ) + assert passing == frozenset() + + def test_comment_hiding_semicolon_does_not_split(self, ses_auth): + # A ';' inside a comment is not a clause separator either. + _, passing = ses_auth.parse_authentication_results( + "amazonses.com; spf=pass (note: a; b) client-ip=1.2.3.4;" + " dkim=pass header.i=@seahaven.com" + ) + assert passing == frozenset({"seahaven.com"}) + + def test_unbalanced_quote_fails_closed(self, ses_auth): + # An unterminated quoted-string is malformed; refuse to parse it so a + # dkim=pass clause "swallowed" by the runaway quote can't be salvaged + # (and, conversely, a runaway quote can't be used to mis-tokenise). + authserv_id, passing = ses_auth.parse_authentication_results( + 'amazonses.com; envelope-from="oops@attacker.com;' + " dkim=pass header.i=@seahaven.com" + ) + assert authserv_id == "" + assert passing == frozenset() + + def test_unbalanced_comment_fails_closed(self, ses_auth): + # An unterminated comment is malformed and must fail closed. + authserv_id, passing = ses_auth.parse_authentication_results( + "amazonses.com; dkim=pass header.i=@seahaven.com (runaway comment" + ) + assert authserv_id == "" + assert passing == frozenset() + class TestEvaluateSenderAuthentication: def test_ses_stamped_pass_accepted(self, ses_auth): @@ -206,6 +273,26 @@ class TestEvaluateSenderAuthentication: assert not accepted assert reason == "allowlist_not_configured" + def test_envelope_from_clause_injection_rejected(self, ses_auth): + # Full forged email: attacker's quoted MAIL FROM is echoed by SES into + # its own (topmost, genuinely SES-stamped) Authentication-Results as + # envelope-from=, trying to smuggle a dkim=pass for an allowlisted + # domain. The real DKIM verdict is fail. Must fail closed. + injected = ( + "Authentication-Results: amazonses.com;\r\n" + " spf=pass (spfCheck: domain of attacker.com designates 1.2.3.4 as" + " permitted sender) client-ip=1.2.3.4;\r\n" + ' envelope-from="x; dkim=pass header.i=@amazon.coupahost.com"@attacker.com;' + " helo=mail.attacker.com;\r\n" + " dkim=fail header.i=@attacker.com;\r\n" + " dmarc=fail header.from=attacker.com;\r\n" + ) + accepted, reason, _ = ses_auth.evaluate_sender_authentication( + raw(injected), {"amazon.coupahost.com", "seahaven.com"} + ) + assert not accepted + assert reason == "no_passing_dkim_signature" + class TestAuthenticateInboundEmail: S3_KEY = "s3://bucket/inbound/abc123"