fix(dkim): N+1 oversign present headers to detect prepended duplicates - #751
Merged
Conversation
Listing each present header once in h= leaves header-duplication spoofing open. A verifier binds listed instances from the bottom up, so a hop that PREPENDS a second Subject still matches the original and reports pass - while the MUA displays the attacker's copy at the top. Confirmed both ways against the signer. With Subject listed once, a prepended spoof verifies clean; listed twice, it fails. The same holds for From, To, Reply-To and Message-ID. Present headers are now listed n+1 times, so the extra entry asserts "there is no further instance". Absent candidates are still omitted entirely, which is what keeps SES stamping its own Message-ID from breaking the signature - a dedicated test pins that this hardening does not resurrect the oversigning bug it builds on. Trade-off: a relay that legitimately adds a duplicate of a header we already set would now break the signature. No relay in this path does so, and the candidate list is narrow. That is the intended direction: the alternative is silently accepting a spoofed display value. Builds on the h= narrowing; should merge after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jiashuoz
force-pushed
the
fix/dkim-n-plus-one-oversigning
branch
from
July 28, 2026 05:49
453f4c8 to
dfc2e5e
Compare
This was referenced Jul 28, 2026
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.
Summary
Listing each present header once in
h=leaves header-duplication spoofing open. A DKIM verifier binds listed header instances from the bottom up, so a hop that prepends a secondSubjectstill matches the original and reports pass — while the recipient's MUA displays the attacker's copy at the top.Present headers are now listed n+1 times. The extra entry asserts "there is no further instance."
Raised during adversarial review of #746, which correctly noted the weakness predates that PR — the old static list also passed each candidate once.
Confirmed both ways against the signer
Not argued from the RFC — measured:
h=listingSubject: YOU HAVE WONFrom:Subject(before)From:From:Subject:Subject(after)Same result for
From,To,Reply-To, andMessage-ID.Mutation-verified in reverse too: reverting to single listing fails
TestSign_PrependedDuplicateHeaderIsDetectedwith "a prepended duplicate Subject went undetected" on every case.This must not undo #746
The two interact directly, so there's a dedicated test for it. Absent candidates are still omitted from
h=entirely — that's what keeps SES stamping its ownMessage-IDfrom breaking the signature. N+1 applies only to headers the message actually carries.TestSign_NPlusOneDoesNotResurrectOversigningasserts the composer-shaped message still verifies after SES addsMessage-ID, so this hardening cannot silently reintroduce the bug it builds on.Trade-off
A relay that legitimately adds a duplicate of a header we already set would now break the signature. Accepted: no relay in this path does so, and the candidate list is narrow. The alternative is silently accepting a spoofed display value, which is the worse failure — a signature that says "verified" over content the user never sees.
Client surface checklist
Not applicable — internal signing behavior. No API surface, schema, or migration;
make generateuntouched.Merge order
Builds on #746 (
fix/dkim-sign-present-headers) and is branched from it, since it modifies the function that PR introduces. Merge after #746.Test plan
go test ./internal/dkim/— greenmake test-unit— full unit suite, 0 failuresgofmt/go vetcleanSubject,From,To,Reply-To,Message-ID, with an unmutated control asserted firstsignedHeaderKeystable extended: present headers listed n+1, an already-duplicated header listed 3 times, absentFromstill listed exactly once per RFC 6376 § 5.4🤖 Generated with Claude Code