From 11bf0f5d1280a4ad567afac13c8be018ed6db620 Mon Sep 17 00:00:00 2001 From: "seahaven-openswe[bot]" <296972425+seahaven-openswe[bot]@users.noreply.github.com> Date: Thu, 16 Jul 2026 19:00:40 +0000 Subject: [PATCH] fix(ses_auth): harden comment stripping and alarm evaluation window (#103) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SES-AR-01: treat ")" at depth 0 as an unmatched close, rejecting the value as not well-formed so a ")(...)" pair cannot manufacture a depth-0 gap where a smuggled dkim=pass clause gets parsed. SES-AR-02: emit a space when a comment is removed so comments act as CFWS folding whitespace (RFC 5322). Without this, "dk(z)im=pass" would become "dkim=pass" and an attacker comment could glue unrelated tokens. Alarm: widen evaluation_periods from 3→6 (30-min window) with datapoints_to_alarm still at 2, closing the sparse-outage residual where rejections >10-15 min apart fail to place breaching datapoints in 2 of 3 consecutive periods. Both ses_auth.py copies stay byte-identical. All 86 tests pass. Co-authored-by: amoussa1229 <166072409+amoussa1229@users.noreply.github.com> --- cdk/po_stack.py | 17 ++++---- cdk/wo_stack.py | 17 ++++---- lambdas/po/email_processor/ses_auth.py | 33 ++++++++++++--- lambdas/wo/email_processor/ses_auth.py | 33 ++++++++++++--- tests/test_ses_auth.py | 57 ++++++++++++++++++++++++++ 5 files changed, 133 insertions(+), 24 deletions(-) diff --git a/cdk/po_stack.py b/cdk/po_stack.py index 18b50c2..5311215 100644 --- a/cdk/po_stack.py +++ b/cdk/po_stack.py @@ -121,12 +121,15 @@ def _add_sender_auth_rejected_alarm(scope, id_prefix, function_name, alarm_topic # drift outage (allowlist wrong / signing-domain changed) may only produce # a trickle of rejects -- one every few minutes -- that never sums to 3 in # any window, so the outage never pages. Instead: >=1 reject per 5-min - # period, alarming when 2 of the last 3 periods breach (evaluation_periods=3 - # / datapoints_to_alarm=2, the same idiom as the duration alarm). A single - # stray spoof probe (one lone period) is tolerated and self-clears, but a - # sustained reject condition trips within ~10-15 min even at one reject per - # period. default_value=0 on the metric filter keeps the series continuous - # so NOT_BREACHING only applies before the first datapoint ever arrives. + # period, alarming when 2 of the last 6 periods breach (evaluation_periods=6 + # / datapoints_to_alarm=2). Six periods (30 min) with only 2 required + # datapoints closes the sparse-outage residual: even rejections >10-15 min + # apart can still place two breaching datapoints in a single 30-min + # evaluation window. A single stray spoof probe (one lone period) is + # tolerated and self-clears, but a sustained reject condition trips even + # at very low arrival rates. default_value=0 on the metric filter keeps the + # series continuous so NOT_BREACHING only applies before the first datapoint + # ever arrives. cloudwatch.Metric( namespace=_SENDER_AUTH_METRIC_NAMESPACE, metric_name=metric_name, @@ -141,7 +144,7 @@ def _add_sender_auth_rejected_alarm(scope, id_prefix, function_name, alarm_topic "(possible allowlist/DKIM-domain drift silently dropping real mail)" ), threshold=1, - evaluation_periods=3, + evaluation_periods=6, datapoints_to_alarm=2, comparison_operator=cloudwatch.ComparisonOperator.GREATER_THAN_OR_EQUAL_TO_THRESHOLD, treat_missing_data=cloudwatch.TreatMissingData.NOT_BREACHING, diff --git a/cdk/wo_stack.py b/cdk/wo_stack.py index 05d8ac2..0851cba 100644 --- a/cdk/wo_stack.py +++ b/cdk/wo_stack.py @@ -121,12 +121,15 @@ def _add_sender_auth_rejected_alarm(scope, id_prefix, function_name, alarm_topic # drift outage (allowlist wrong / signing-domain changed) may only produce # a trickle of rejects -- one every few minutes -- that never sums to 3 in # any window, so the outage never pages. Instead: >=1 reject per 5-min - # period, alarming when 2 of the last 3 periods breach (evaluation_periods=3 - # / datapoints_to_alarm=2, the same idiom as the duration alarm). A single - # stray spoof probe (one lone period) is tolerated and self-clears, but a - # sustained reject condition trips within ~10-15 min even at one reject per - # period. default_value=0 on the metric filter keeps the series continuous - # so NOT_BREACHING only applies before the first datapoint ever arrives. + # period, alarming when 2 of the last 6 periods breach (evaluation_periods=6 + # / datapoints_to_alarm=2). Six periods (30 min) with only 2 required + # datapoints closes the sparse-outage residual: even rejections >10-15 min + # apart can still place two breaching datapoints in a single 30-min + # evaluation window. A single stray spoof probe (one lone period) is + # tolerated and self-clears, but a sustained reject condition trips even + # at very low arrival rates. default_value=0 on the metric filter keeps the + # series continuous so NOT_BREACHING only applies before the first datapoint + # ever arrives. cloudwatch.Metric( namespace=_SENDER_AUTH_METRIC_NAMESPACE, metric_name=metric_name, @@ -141,7 +144,7 @@ def _add_sender_auth_rejected_alarm(scope, id_prefix, function_name, alarm_topic "(possible allowlist/DKIM-domain drift silently dropping real mail)" ), threshold=1, - evaluation_periods=3, + evaluation_periods=6, datapoints_to_alarm=2, comparison_operator=cloudwatch.ComparisonOperator.GREATER_THAN_OR_EQUAL_TO_THRESHOLD, treat_missing_data=cloudwatch.TreatMissingData.NOT_BREACHING, diff --git a/lambdas/po/email_processor/ses_auth.py b/lambdas/po/email_processor/ses_auth.py index db5445f..db58ce6 100644 --- a/lambdas/po/email_processor/ses_auth.py +++ b/lambdas/po/email_processor/ses_auth.py @@ -99,15 +99,28 @@ def _strip_comments(value: str) -> tuple: Dropping comments first means comment-embedded ``header.i=`` / ``;`` fakes can never influence clause splitting or domain extraction. + Every removed comment is replaced by a single space so comments act as + folding whitespace (RFC 5322 CFWS semantics). A ``)`` that appears + outside any comment (depth 0) is an unmatched close and makes the value + not well-formed, closing the ``)(`` clause-injection gap. + + CFWS also means a comment abutting a token *ends* that token: + ``header.d=seahaven.com(note)`` reads as domain ``seahaven.com``, where + the pre-CFWS gluing behaviour read ``seahaven.comnote`` (rejected). + This is deliberate -- ``header.d`` is written verbatim by SES from a + *verified* signature's ``d=`` tag, so a comment in that position is + never attacker-controlled. + 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. + the value ends inside an unterminated comment or quoted-string, or when + an unmatched ``)`` appears at the top level. 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) + extra_close = False # SES-AR-01: ")" at depth 0 (no matching open) i = 0 n = len(value) while i < n: @@ -121,6 +134,9 @@ def _strip_comments(value: str) -> tuple: depth += 1 elif c == ")": depth -= 1 + if depth == 0: + # SES-AR-02: comment acts as folding whitespace (RFC 5322). + out.append(" ") i += 1 continue if in_quote: @@ -138,11 +154,18 @@ def _strip_comments(value: str) -> tuple: depth += 1 i += 1 continue + if c == ")": + # SES-AR-01: ")" at depth 0 is an unmatched close -- refuse to + # tokenise rather than letting a ")(...)" pair manufacture a + # gap where attacker text leaks out at depth 0. + extra_close = True + i += 1 + continue if c == '"': in_quote = True out.append(c) i += 1 - well_formed = depth == 0 and not in_quote + well_formed = depth == 0 and not in_quote and not extra_close return "".join(out), well_formed diff --git a/lambdas/wo/email_processor/ses_auth.py b/lambdas/wo/email_processor/ses_auth.py index db5445f..db58ce6 100644 --- a/lambdas/wo/email_processor/ses_auth.py +++ b/lambdas/wo/email_processor/ses_auth.py @@ -99,15 +99,28 @@ def _strip_comments(value: str) -> tuple: Dropping comments first means comment-embedded ``header.i=`` / ``;`` fakes can never influence clause splitting or domain extraction. + Every removed comment is replaced by a single space so comments act as + folding whitespace (RFC 5322 CFWS semantics). A ``)`` that appears + outside any comment (depth 0) is an unmatched close and makes the value + not well-formed, closing the ``)(`` clause-injection gap. + + CFWS also means a comment abutting a token *ends* that token: + ``header.d=seahaven.com(note)`` reads as domain ``seahaven.com``, where + the pre-CFWS gluing behaviour read ``seahaven.comnote`` (rejected). + This is deliberate -- ``header.d`` is written verbatim by SES from a + *verified* signature's ``d=`` tag, so a comment in that position is + never attacker-controlled. + 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. + the value ends inside an unterminated comment or quoted-string, or when + an unmatched ``)`` appears at the top level. 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) + extra_close = False # SES-AR-01: ")" at depth 0 (no matching open) i = 0 n = len(value) while i < n: @@ -121,6 +134,9 @@ def _strip_comments(value: str) -> tuple: depth += 1 elif c == ")": depth -= 1 + if depth == 0: + # SES-AR-02: comment acts as folding whitespace (RFC 5322). + out.append(" ") i += 1 continue if in_quote: @@ -138,11 +154,18 @@ def _strip_comments(value: str) -> tuple: depth += 1 i += 1 continue + if c == ")": + # SES-AR-01: ")" at depth 0 is an unmatched close -- refuse to + # tokenise rather than letting a ")(...)" pair manufacture a + # gap where attacker text leaks out at depth 0. + extra_close = True + i += 1 + continue if c == '"': in_quote = True out.append(c) i += 1 - well_formed = depth == 0 and not in_quote + well_formed = depth == 0 and not in_quote and not extra_close return "".join(out), well_formed diff --git a/tests/test_ses_auth.py b/tests/test_ses_auth.py index 69bfa73..9537647 100644 --- a/tests/test_ses_auth.py +++ b/tests/test_ses_auth.py @@ -212,6 +212,63 @@ class TestParseAuthenticationResults: ) assert passing == frozenset({"seahaven.com"}) + # --- SES-AR-01: ")" at depth 0 (extra close) must fail closed --- + + def test_extra_close_paren_fails_closed(self, ses_auth): + # SES-AR-01: a ")" at depth 0 makes the value not well-formed, + # preventing a ")(...)" pair from manufacturing a depth-0 gap + # where a smuggled "dkim=pass" clause is parsed. + authserv_id, passing = ses_auth.parse_authentication_results( + "amazonses.com; spf=pass (legit)) ; dkim=pass header.d=seahaven.com (rest)" + ) + assert authserv_id == "" + assert passing == frozenset() + + def test_extra_close_paren_at_start_fails_closed(self, ses_auth): + authserv_id, passing = ses_auth.parse_authentication_results( + "amazonses.com;) dkim=pass header.d=seahaven.com" + ) + assert authserv_id == "" + assert passing == frozenset() + + # --- SES-AR-02: comment removal inserts space (CFWS semantics) --- + + def test_comment_adjacent_to_result_still_terminates_token(self, ses_auth): + # Regression: "dkim=pass(good signature)" must still read as + # "dkim=pass" after the comment is replaced by a space. + _, passing = ses_auth.parse_authentication_results( + "amazonses.com; dkim=pass(good signature) header.i=@seahaven.com" + ) + assert passing == frozenset({"seahaven.com"}) + + def test_comment_glue_does_not_create_dkim_clause(self, ses_auth): + # SES-AR-02: "dk(z)im=pass" must NOT become "dkim=pass" when the + # comment is stripped. The space inserted for the comment produces + # "dk zim=pass", which does not start with "dkim". + _, passing = ses_auth.parse_authentication_results( + "amazonses.com; dk(z)im=pass header.i=@seahaven.com" + ) + assert passing == frozenset() + + def test_comment_separates_adjacent_tokens(self, ses_auth): + # SES-AR-02: "sea(x)haven.com" must NOT become "seahaven.com". + # The space keeps the tokens separate. + _, passing = ses_auth.parse_authentication_results( + "amazonses.com; sea(x)haven.com dkim=pass header.i=@seahaven.com" + ) + assert passing == frozenset() + + def test_comment_after_header_d_ends_the_domain_token(self, ses_auth): + # CFWS: a comment abutting header.d ends the token, so the domain + # reads as "seahaven.com" (pre-CFWS gluing read "seahaven.comnote", + # rejected). Deliberate: header.d is written verbatim by SES from a + # *verified* signature's d= tag, so a comment in that position is + # never attacker-controlled. + _, passing = ses_auth.parse_authentication_results( + "amazonses.com; dkim=pass header.d=seahaven.com(note)" + ) + 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