git » spf » commit 6cf72fe

Add DNS "void lookup" count checks

author James Lamb
2022-01-18 08:59:44 UTC
committer Alberto Bertogli
2022-03-19 10:59:14 UTC
parent d9db5f70c02968c3f05c5c516ce520a4d1bc4397

Add DNS "void lookup" count checks

RFC7204 section 4.6.4 states:

> SPF implementations SHOULD limit "void lookups" to two. An
> implementation MAY choose to make such a limit configurable. In this
> case, a default of two is RECOMMENDED.  Exceeding the limit produces a
> "permerror" result.

A DNS "void lookup" is one that returns either an empty answer, or
NXDOMAIN.

Limiting the number of void lookups when doing SPF checks helps
reduce/prevent DNS amplification attacks (RFC7204 section
11.1, 4th bullet).

This patch introduces a void lookup check, with a default limit of 2 as
per the RFC recommendation, and an option to override it.

For reference, such a check already exists in pyspf, and it seems to
also be enforced by some large providers. An exploration of the top-1000
sites on the Alexa ranking showed all domains to be okay except for 3,
which were misconfigured for other reasons and would have issues
regardless.

See https://github.com/albertito/spf/pull/2 for more background and
discussion.

Amended-by: Alberto Bertogli <albertito@blitiri.com.ar>
  Edited commit message; minor comment indentation changes; added
  tracing; removed "skip" from a standard test, now that this limit is
  enforced; unified individual commits into a single one for history
  readability.

spf.go +80 -26
spf_test.go +40 -0
testdata/rfc7208-tests.yml +0 -1

diff --git a/spf.go b/spf.go
index 8cc061f..6a06443 100644
--- a/spf.go
+++ b/spf.go
@@ -92,10 +92,11 @@ var (
 
 	// Errors related to DNS lookups.
 	// Note that the library functions may also return net.DNSError.
-	ErrNoResult           = errors.New("no DNS record found")
-	ErrLookupLimitReached = errors.New("lookup limit reached")
-	ErrTooManyMXRecords   = errors.New("too many MX records")
-	ErrMultipleRecords    = errors.New("multiple matching DNS records")
+	ErrNoResult               = errors.New("no DNS record found")
+	ErrLookupLimitReached     = errors.New("lookup limit reached")
+	ErrVoidLookupLimitReached = errors.New("void lookup limit reached")
+	ErrTooManyMXRecords       = errors.New("too many MX records")
+	ErrMultipleRecords        = errors.New("multiple matching DNS records")
 
 	// Errors returned on a successful match.
 	ErrMatchedAll    = errors.New("matched all")
@@ -106,10 +107,18 @@ var (
 	ErrMatchedExists = errors.New("matched exists")
 )
 
-// Default value for the maximum number of DNS lookups while resolving SPF.
-// RFC is quite clear 10 must be the maximum allowed.
-// https://tools.ietf.org/html/rfc7208#section-4.6.4
-const defaultMaxLookups = 10
+const (
+	// Default value for the maximum number of DNS lookups while resolving SPF.
+	// RFC is quite clear 10 must be the maximum allowed.
+	// https://tools.ietf.org/html/rfc7208#section-4.6.4
+	defaultMaxLookups = 10
+
+	// Default value for the maximum number of DNS void lookups while
+	// resolving SPF.  RFC suggests that implementations SHOULD limit these
+	// with a configurable default of 2.
+	// https://tools.ietf.org/html/rfc7208#section-4.6.4
+	defaultMaxVoidLookups = 2
+)
 
 // TraceFunc is the type of tracing functions.
 type TraceFunc func(f string, a ...interface{})
@@ -138,13 +147,14 @@ type Option func(*resolution)
 // Deprecated: use CheckHostWithSender instead.
 func CheckHost(ip net.IP, domain string) (Result, error) {
 	r := &resolution{
-		ip:       ip,
-		maxcount: defaultMaxLookups,
-		helo:     domain,
-		sender:   "@" + domain,
-		ctx:      context.TODO(),
-		resolver: defaultResolver,
-		trace:    defaultTrace,
+		ip:           ip,
+		maxcount:     defaultMaxLookups,
+		maxvoidcount: defaultMaxVoidLookups,
+		helo:         domain,
+		sender:       "@" + domain,
+		ctx:          context.TODO(),
+		resolver:     defaultResolver,
+		trace:        defaultTrace,
 	}
 	return r.Check(domain)
 }
@@ -168,13 +178,14 @@ func CheckHostWithSender(ip net.IP, helo, sender string, opts ...Option) (Result
 	}
 
 	r := &resolution{
-		ip:       ip,
-		maxcount: defaultMaxLookups,
-		helo:     helo,
-		sender:   sender,
-		ctx:      context.TODO(),
-		resolver: defaultResolver,
-		trace:    defaultTrace,
+		ip:           ip,
+		maxcount:     defaultMaxLookups,
+		maxvoidcount: defaultMaxVoidLookups,
+		helo:         helo,
+		sender:       sender,
+		ctx:          context.TODO(),
+		resolver:     defaultResolver,
+		trace:        defaultTrace,
 	}
 
 	for _, opt := range opts {
@@ -196,6 +207,18 @@ func OverrideLookupLimit(limit uint) Option {
 	}
 }
 
+// OverrideVoidLookupLimit overrides the maximum number of void DNS lookups allowed
+// during SPF evaluation. A void DNS lookup is one that returns an empty
+// answer, or a NXDOMAIN.  Note that as per RFC, the default value of 2 SHOULD
+// be used. Please use with care.
+//
+// This is EXPERIMENTAL for now, and the API is subject to change.
+func OverrideVoidLookupLimit(limit uint) Option {
+	return func(r *resolution) {
+		r.maxvoidcount = limit
+	}
+}
+
 // WithContext is an option to set the context for this operation, which will
 // be passed along to the resolver functions and other external calls if
 // needed.
@@ -255,9 +278,11 @@ func split(addr string) (string, string) {
 }
 
 type resolution struct {
-	ip       net.IP
-	count    uint
-	maxcount uint
+	ip           net.IP
+	count        uint
+	maxcount     uint
+	voidcount    uint
+	maxvoidcount uint
 
 	helo   string
 	sender string
@@ -281,7 +306,7 @@ var ptrField = regexp.MustCompile(`^(ptr$|ptr:)`)
 
 func (r *resolution) Check(domain string) (Result, error) {
 	r.count++
-	r.trace("check %q %d", domain, r.count)
+	r.trace("check %q %d %d", domain, r.count, r.voidcount)
 	txt, err := r.getDNSRecord(domain)
 	if err != nil {
 		if isTemporary(err) {
@@ -343,6 +368,11 @@ func (r *resolution) Check(domain string) (Result, error) {
 			return PermError, ErrLookupLimitReached
 		}
 
+		if r.voidcount > r.maxvoidcount {
+			r.trace("void lookup limit reached")
+			return PermError, ErrVoidLookupLimitReached
+		}
+
 		// See if we have a qualifier, defaulting to + (pass).
 		// https://tools.ietf.org/html/rfc7208#section-4.6.2
 		result, ok := qualToResult[field[0]]
@@ -454,6 +484,26 @@ func isTemporary(err error) bool {
 	return ok && derr.Temporary()
 }
 
+// Check if the given DNS error is a "void lookup" (0 answers, or nxdomain),
+// and if so increment the void lookup counter.
+func (r *resolution) checkVoidLookup(nanswers int, err error) {
+	if err == nil && nanswers == 0 {
+		r.voidcount++
+		r.trace("void lookup: no answers")
+		return
+	}
+
+	derr, ok := err.(*net.DNSError)
+	if !ok {
+		return
+	}
+
+	if derr.IsNotFound {
+		r.voidcount++
+		r.trace("void lookup: nxdomain")
+	}
+}
+
 // ipField processes an "ip" field.
 func (r *resolution) ipField(res Result, field string) (bool, Result, error) {
 	fip := field[4:]
@@ -499,6 +549,7 @@ func (r *resolution) ptrField(res Result, field, domain string) (bool, Result, e
 		r.ipNames = []string{}
 		r.count++
 		ns, err := r.resolver.LookupAddr(r.ctx, r.ip.String())
+		r.checkVoidLookup(len(ns), err)
 		if err != nil {
 			// https://tools.ietf.org/html/rfc7208#section-5
 			if isTemporary(err) {
@@ -555,6 +606,7 @@ func (r *resolution) existsField(res Result, field, domain string) (bool, Result
 
 	r.count++
 	ips, err := r.resolver.LookupIPAddr(r.ctx, eDomain)
+	r.checkVoidLookup(len(ips), err)
 	if err != nil {
 		// https://tools.ietf.org/html/rfc7208#section-5
 		if isTemporary(err) {
@@ -670,6 +722,7 @@ func (r *resolution) aField(res Result, field, domain string) (bool, Result, err
 
 	r.count++
 	ips, err := r.resolver.LookupIPAddr(r.ctx, aDomain)
+	r.checkVoidLookup(len(ips), err)
 	if err != nil {
 		// https://tools.ietf.org/html/rfc7208#section-5
 		if isTemporary(err) {
@@ -702,6 +755,7 @@ func (r *resolution) mxField(res Result, field, domain string) (bool, Result, er
 
 	r.count++
 	mxs, err := r.resolver.LookupMX(r.ctx, mxDomain)
+	r.checkVoidLookup(len(mxs), err)
 	if err != nil {
 		// https://tools.ietf.org/html/rfc7208#section-5
 		if isTemporary(err) {
diff --git a/spf_test.go b/spf_test.go
index 93c6fba..58ee9ee 100644
--- a/spf_test.go
+++ b/spf_test.go
@@ -522,6 +522,46 @@ func TestOverrideLookupLimit(t *testing.T) {
 	}
 }
 
+func TestOverrideVoidLookupLimit(t *testing.T) {
+	dns := NewDefaultResolver()
+	defaultTrace = t.Logf
+
+	nxDomainErr := &net.DNSError{
+		Err:         "no such domain",
+		IsTemporary: false,
+		IsNotFound:  true,
+	}
+
+	dns.Txt["domain1"] = []string{"v=spf1 exists:%{i}.one include:domain2"}
+	dns.Txt["domain2"] = []string{"v=spf1 exists:%{i}.two include:domain3"}
+	dns.Txt["domain3"] = []string{"v=spf1 exists:%{i}.three include:domain4"}
+	dns.Txt["domain4"] = []string{"v=spf1 +all"}
+	dns.Errors["1.1.1.1.one"] = nxDomainErr
+	dns.Errors["1.1.1.1.two"] = nxDomainErr
+	dns.Errors["1.1.1.1.three"] = nxDomainErr
+
+	// The default of 2
+	res, err := CheckHostWithSender(ip1111, "helo", "user@domain1")
+	if res != PermError {
+		t.Errorf("expected permerror, got %q / %q", res, err)
+	}
+
+	// Set the limit to 10, which is excessive.
+	res, err = CheckHostWithSender(ip1111, "helo", "user@domain1",
+		OverrideVoidLookupLimit(10))
+	if res != Pass {
+		t.Errorf("expected pass, got %q / %q", res, err)
+	}
+
+	// Set the limit to 1, which is not enough.
+	res, err = CheckHostWithSender(ip1111, "helo", "user@domain1",
+		OverrideVoidLookupLimit(1))
+	if res != PermError || err != ErrVoidLookupLimitReached {
+		t.Errorf("expected permerror/void lookup limit reached, got %q / %q",
+			res, err)
+	}
+}
+
 func TestWithContext(t *testing.T) {
 	dns := NewDefaultResolver()
 	defaultTrace = t.Logf
diff --git a/testdata/rfc7208-tests.yml b/testdata/rfc7208-tests.yml
index 9191fa2..b22dab9 100644
--- a/testdata/rfc7208-tests.yml
+++ b/testdata/rfc7208-tests.yml
@@ -2553,7 +2553,6 @@ tests:
     host: 1.2.3.4
     mailfrom: foo@e11.example.com
     result: permerror
-    skip: We don't limit this separately from the total 10 limit.
 zonedata:
   mail.example.com:
     - A: 1.2.3.4