From d1e93020d83af4b29f8180bd6a66b9baa7d13143 Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Wed, 25 Oct 2023 13:01:11 +0200 Subject: [PATCH] give delivering to mx targets with underscores in name a chance of succeeding the underscores aren't valid, but have been seen in the wild, so we have a workaround for them. there are limitations, it won't work with idna domains. and if the domain has other policies, like mta-sts, the mx host won't pass either. after report from richard g about delivery issue, thanks! --- dns/dns.go | 48 +++++++++++++++++++++++++++++++++++++++++--- dns/dns_test.go | 27 +++++++++++++++++-------- smtpclient/gather.go | 3 ++- spf/spf.go | 3 ++- 4 files changed, 68 insertions(+), 13 deletions(-) diff --git a/dns/dns.go b/dns/dns.go index 3e1fdb6..3247ba3 100644 --- a/dns/dns.go +++ b/dns/dns.go @@ -10,9 +10,15 @@ import ( "golang.org/x/net/idna" "github.com/mjl-/adns" + + "github.com/mjl-/mox/moxvar" ) -var errTrailingDot = errors.New("dns name has trailing dot") +var ( + errTrailingDot = errors.New("dns name has trailing dot") + errUnderscore = errors.New("domain name with underscore") + errIDNA = errors.New("idna") +) // Domain is a domain name, with one or more labels, with at least an ASCII // representation, and for IDNA non-ASCII domains a unicode representation. @@ -83,13 +89,14 @@ func ParseDomain(s string) (Domain, error) { if strings.HasSuffix(s, ".") { return Domain{}, errTrailingDot } + ascii, err := idna.Lookup.ToASCII(s) if err != nil { - return Domain{}, fmt.Errorf("to ascii: %w", err) + return Domain{}, fmt.Errorf("%w: to ascii: %v", errIDNA, err) } unicode, err := idna.Lookup.ToUnicode(s) if err != nil { - return Domain{}, fmt.Errorf("to unicode: %w", err) + return Domain{}, fmt.Errorf("%w: to unicode: %w", errIDNA, err) } // todo: should we cause errors for unicode domains that were not in // canonical form? we are now accepting all kinds of obscure spellings @@ -101,6 +108,41 @@ func ParseDomain(s string) (Domain, error) { return Domain{ascii, unicode}, nil } +// ParseDomainLax parses a domain like ParseDomain, but allows labels with +// underscores if the entire domain name is ASCII-only non-IDNA and Pedantic mode +// is not enabled. Used for interoperability, e.g. domains may specify MX +// targets with underscores. +func ParseDomainLax(s string) (Domain, error) { + if moxvar.Pedantic || !strings.Contains(s, "_") { + return ParseDomain(s) + } + + // If there is any non-ASCII, this is certainly not an A-label-only domain. + s = strings.ToLower(s) + for _, c := range s { + if c >= 0x80 { + return Domain{}, fmt.Errorf("%w: underscore and non-ascii not allowed", errUnderscore) + } + } + + // Try parsing with underscores replaced with allowed ASCII character. + // If that's not valid, the version with underscore isn't either. + repl := strings.ReplaceAll(s, "_", "a") + d, err := ParseDomain(repl) + if err != nil { + return Domain{}, fmt.Errorf("%w: %v", errUnderscore, err) + } + // If we found an IDNA domain, we're not going to allow it. + if d.Unicode != "" { + return Domain{}, fmt.Errorf("%w: idna domain with underscores not allowed", errUnderscore) + } + // Just to be safe, ensure no unexpected conversions happened. + if d.ASCII != repl { + return Domain{}, fmt.Errorf("%w: underscores and non-canonical names not allowed", errUnderscore) + } + return Domain{ASCII: s}, nil +} + // IsNotFound returns whether an error is an adns.DNSError with IsNotFound set. // IsNotFound means the requested type does not exist for the given domain (a // nodata or nxdomain response). It doesn't not necessarily mean no other types for diff --git a/dns/dns_test.go b/dns/dns_test.go index 9ad84df..3c42a2d 100644 --- a/dns/dns_test.go +++ b/dns/dns_test.go @@ -6,9 +6,15 @@ import ( ) func TestParseDomain(t *testing.T) { - test := func(s string, exp Domain, expErr error) { + test := func(lax bool, s string, exp Domain, expErr error) { t.Helper() - dom, err := ParseDomain(s) + var dom Domain + var err error + if lax { + dom, err = ParseDomainLax(s) + } else { + dom, err = ParseDomain(s) + } if (err == nil) != (expErr == nil) || expErr != nil && !errors.Is(err, expErr) { t.Fatalf("parse domain %q: err %v, expected %v", s, err, expErr) } @@ -18,10 +24,15 @@ func TestParseDomain(t *testing.T) { } // We rely on normalization of names throughout the code base. - test("xmox.nl", Domain{"xmox.nl", ""}, nil) - test("XMOX.NL", Domain{"xmox.nl", ""}, nil) - test("TEST☺.XMOX.NL", Domain{"xn--test-3o3b.xmox.nl", "test☺.xmox.nl"}, nil) - test("TEST☺.XMOX.NL", Domain{"xn--test-3o3b.xmox.nl", "test☺.xmox.nl"}, nil) - test("ℂᵤⓇℒ。𝐒🄴", Domain{"curl.se", ""}, nil) // https://daniel.haxx.se/blog/2022/12/14/idn-is-crazy/ - test("xmox.nl.", Domain{}, errTrailingDot) + test(false, "xmox.nl", Domain{"xmox.nl", ""}, nil) + test(false, "XMOX.NL", Domain{"xmox.nl", ""}, nil) + test(false, "TEST☺.XMOX.NL", Domain{"xn--test-3o3b.xmox.nl", "test☺.xmox.nl"}, nil) + test(false, "TEST☺.XMOX.NL", Domain{"xn--test-3o3b.xmox.nl", "test☺.xmox.nl"}, nil) + test(false, "ℂᵤⓇℒ。𝐒🄴", Domain{"curl.se", ""}, nil) // https://daniel.haxx.se/blog/2022/12/14/idn-is-crazy/ + test(false, "xmox.nl.", Domain{}, errTrailingDot) + + test(false, "_underscore.xmox.nl", Domain{}, errIDNA) + test(true, "_underscore.xmox.NL", Domain{ASCII: "_underscore.xmox.nl"}, nil) + test(true, "_underscore.☺.xmox.nl", Domain{}, errUnderscore) + test(true, "_underscore.xn--test-3o3b.xmox.nl", Domain{}, errUnderscore) } diff --git a/smtpclient/gather.go b/smtpclient/gather.go index 55f468a..7d1ba7a 100644 --- a/smtpclient/gather.go +++ b/smtpclient/gather.go @@ -148,7 +148,8 @@ func GatherDestinations(ctx context.Context, log *mlog.Log, resolver dns.Resolve // The Go resolver already sorts by preference, randomizing records of same // preference. ../rfc/5321:3885 for _, mx := range mxl { - host, err := dns.ParseDomain(strings.TrimSuffix(mx.Host, ".")) + // Parsing lax (unless pedantic mode) for MX targets with underscores as seen in the wild. + host, err := dns.ParseDomainLax(strings.TrimSuffix(mx.Host, ".")) if err != nil { // note: should not happen because Go resolver already filters these out. err = fmt.Errorf("%w: invalid host name in mx record %q: %v", errDNS, mx.Host, err) diff --git a/spf/spf.go b/spf/spf.go index 2a4ca78..3619edf 100644 --- a/spf/spf.go +++ b/spf/spf.go @@ -444,7 +444,8 @@ func evaluate(ctx context.Context, record *Record, resolver dns.Resolver, args A if i >= 10 { return StatusPermerror, d.MechanismString(), "", rauthentic, ErrTooManyDNSRequests } - mxd, err := dns.ParseDomain(strings.TrimSuffix(mx.Host, ".")) + // Parsing lax (unless in pedantic mode) for MX targets with underscores as seen in the wild. + mxd, err := dns.ParseDomainLax(strings.TrimSuffix(mx.Host, ".")) if err != nil { return StatusPermerror, d.MechanismString(), "", rauthentic, err }