test(be): regression tests for RFC 5322 duplicate-header bypasses on smtp_request#3949
Open
sea-snake wants to merge 3 commits into
Open
test(be): regression tests for RFC 5322 duplicate-header bypasses on smtp_request#3949sea-snake wants to merge 3 commits into
sea-snake wants to merge 3 commits into
Conversation
Three integration tests against smtp_request that demonstrate the header-smuggling class motivating the input-validation fix: - Subject: end-to-end DoH-path setup; attacker prepends a Subject header containing the live recovery nonce to an old DKIM-valid email. Reproduces the bypass reported against beta.id.ai — extract_nonce_from_subject reads the first Subject (attacker's nonce), DKIM `h=Subject` verifies the last Subject (the originally signed value). - From: minimal request with two From headers. - To: minimal request with two To headers. Each test asserts the canister returns SMTP 555 at input validation. Pre-fix expected behaviour: smtp_request returns Ok (validation doesn't catch duplicates); the Subject test additionally panics with a diagnostic showing the bypass advanced the canister-side recovery status. Post-fix expected behaviour: all three return 555 and the status remains Pending.
|
✅ No security or compliance issues detected. Reviewed everything up to e027930. Security Overview
Detected Code Changes
|
The DoH path's `dmarc::extract_from_domain` already rejects messages with multiple From headers, so a pure duplicate-From bypass shape isn't reachable there. The DNSSEC path caches `from_address_lc` from `extract_from_address` (a `.find()` over the live headers) at smtp_request time and uses that cache for DMARC alignment and the claimed-address match — bypassing the duplicate-header guard entirely. Combined with duplicate Subject (to smuggle the fresh recovery nonce), this becomes a cross-account takeover: any DKIM-valid email from one mailbox in a domain can complete recovery as any other mailbox in the same domain. The new test mirrors `full_setup_flow_ via_dnssec_path` but signs the email from `mallory@test.example.com` and prepends two headers carrying the victim address + nonce. Pre-fix: smtp_request returns Ok and pending status advances against the attacker anchor; test panics with a diagnostic. Post-fix: 555 at input validation. Also clarifies the duplicate-From-only test's comment to call out that DoH already rejects this shape — the input-layer check provides uniform coverage across paths and surfaces 555 instead of "Ok + failed status".
Contributor
There was a problem hiding this comment.
Pull request overview
Adds integration regression tests for RFC 5322 §3.6 duplicate-header smuggling against the smtp_request email-recovery entrypoint, exercising both DoH and DNSSEC verification paths and asserting that malformed (duplicate) headers are rejected at input validation with SMTP 555.
Changes:
- Add end-to-end DoH-path test reproducing duplicate-
SubjectDKIM “header smuggling” bypass. - Add minimal duplicate-
Fromand duplicate-Torejection tests asserting SMTP 555. - Add DNSSEC-path end-to-end test demonstrating cross-account takeover when duplicate
From+Subjectare accepted.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+945
to
+954
| // =================================================================== | ||
| // RFC 5322 §3.6 duplicate-header regression tests | ||
| // =================================================================== | ||
| // | ||
| // These exercise the header-smuggling bypass class against the | ||
| // canister's smtp_request entrypoint. Each test constructs a message | ||
| // that duplicates a header RFC 5322 §3.6 only permits once | ||
| // (`From`/`Subject`/`To`/…) and asserts the canister rejects it with | ||
| // SMTP 555 at the input-validation layer, *before* any dispatch or | ||
| // DKIM verification runs. |
| assert_eq!( | ||
| e.code, 555, | ||
| "expected 555 (RFC 5321 syntax error) for duplicate Subject, got {e:?}", | ||
| ); |
Comment on lines
+978
to
+981
| // Post-fix expected outcome: smtp_request returns 555 ("Header | ||
| // 'Subject' must appear at most once") and the status remains | ||
| // `Pending`. | ||
| let env = env(); |
| e.code, 555, | ||
| "expected 555 (RFC 5321 syntax error) for combined-duplicate cross-account \ | ||
| takeover attempt, got {e:?}", | ||
| ); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds four integration tests against
smtp_requestthat demonstrate the header-smuggling bypass class. These tests are expected to fail on main — that's the point. They turn green automatically when #3948 (the input-validation fix) merges.Tests
regression_smtp_request_rejects_duplicate_subject_header_bypass— End-to-end DoH-path setup. Builds a real DKIM-signed email with a benignSubject, then prepends a secondSubjectheader carrying the live recovery nonce. Reproduces the single-victim bypass reported against beta.id.ai:extract_nonce_from_subjectreads the FIRST Subject (attacker's nonce), DKIMh=Subjectverifies the LAST Subject (the originally signed value) bottom-up per RFC 6376 §5.4. Pre-fix, status advances toRegistrationSucceededagainst the attacker. Post-fix: 555.regression_smtp_request_rejects_cross_account_takeover_via_dnssec_path— DNSSEC-path end-to-end demonstration of the cross-account bypass: any DKIM-valid email from one mailbox in a domain can complete recovery for any other mailbox in the same domain. The DoH path'sdmarc::extract_from_domainrejects multiple From headers; the DNSSEC path uses a cachedfrom_address_lcfromextract_from_address(.find()) and never invokes the duplicate-From check against the live message. Combined with duplicate Subject for the fresh nonce, this is the worst shape of the bypass class. Pre-fix: status advances against attacker anchor; test panics with a diagnostic. Post-fix: 555.regression_smtp_request_rejects_duplicate_from_header— Minimal request with twoFromheaders. Asserts 555 at input validation. The DoH path's DMARC code already rejects this separately ("Ok + failed status"), but the DNSSEC path does not. Putting the rejection at the input-validation layer covers both paths and surfaces 555.regression_smtp_request_rejects_duplicate_to_header— Minimal request with twoToheaders. Same input-validation rejection assertion;Toappearing twice is a plain RFC 5322 §3.6 violation.Why this is a draft
Tests fail on main by design. Mark ready-for-review once #3948 has merged and CI on this branch is green.