Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dmarc: fix handling if multiple records #73

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

dmarc: fix handling if multiple records #73

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
552330f
dmarc: fix handling if multiple records
mimi89999 Jan 15, 2025
748ade0
Do not use a separate function
mimi89999 Feb 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 44 additions & 3 deletions dmarc/lookup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,54 @@ func LookupWithOptions(domain string, options *LookupOptions) (*Record, error) {
}
return nil, errors.New("dmarc: failed to lookup TXT record: " + err.Error())
}

// net.LookupTXT will concatenate strings contained in a single TXT record.
// In other words, net.LookupTXT returns one entry per TXT record, even if
// a record contains multiple strings.
if len(txts) == 0 {
return nil, ErrNoPolicy
}

// Long keys are split in multiple parts
txt := strings.Join(txts, "")
return Parse(txt)
// RFC 6376:
// Records that do not start with a "v=" tag that identifies the
// current version of DMARC are discarded.
dmarcRecords := make([]string, 0, len(txts))
for _, record := range txts {
if IsDmarcRecord(record) {
dmarcRecords = append(dmarcRecords, record)
}
}

if len(dmarcRecords) != 1 {
return nil, ErrNoPolicy
}

return Parse(dmarcRecords[0])
}

func IsDmarcRecord(txt string) bool {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to section 6.6.3, checking for the v= prefix should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it states: that identifies the current version of DMARC
There is also:

   v: Version (plain-text; REQUIRED).  Identifies the record retrieved
      as a DMARC record.  It MUST have the value of "DMARC1".  The value
      of this tag MUST match precisely; if it does not or it is absent,
      the entire retrieved record MUST be ignored.  It MUST be the first
      tag in the list.

I believe that the current version is part of the statement as if there was a conjunction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, matching only the v= would match SPF and DKIM records. That wouldn't make much sense.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that identifies the current version of DMARC

Hm, you're right. I'm not a fan of duplicating the parsing logic though. I'd suggest either matching on the v=DMARC1 prefix, or parsing all records for key-values.

Also, matching only the v= would match SPF and DKIM records. That wouldn't make much sense.

DMARC records are stored in a separate, special "_dmarc" subdomain, so that shouldn't be the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, you're right. I'm not a fan of duplicating the parsing logic though. I'd suggest either matching on the v=DMARC1 prefix, or parsing all records for key-values.

That would also match things like v=DMARC1.0 or v=DMARC10 that shouldn't be matched. We also can't simple match v=DMARC1; as leading and trailing spaces in the tag value should be ignored. I believe that the current logic is quite robust and makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emersion I'm not quite sure how I should change the pull request. Could you please propose something more specific?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current logic duplicates parsing, and I'd like to avoid this. parseParams() can be called instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the duplication now.

// RFC 6376:
// DMARC records follow the extensible "tag-value" syntax for DNS-based
// key records defined in DKIM.
firstTagSpec, _, _ := strings.Cut(txt, ";")

tagName, tagValue, found := strings.Cut(firstTagSpec, "=")
if !found {
return false
}

// RFC 6376:
// Note that WSP is allowed anywhere around tags. In particular, any
// WSP after the "=" and any WSP before the terminating ";" is not part
// of the value; however, WSP inside the value is significant.
if strings.TrimSpace(tagName) != "v" {
return false
}
if strings.TrimSpace(tagValue) != "DMARC1" {
return false
}

return true
}

func Parse(txt string) (*Record, error) {
Expand Down