git » spf » commit 48ee700

Fix DNS lookup limit off-by-one error in the count

author Alberto Bertogli
2022-06-18 11:32:15 UTC
committer Alberto Bertogli
2022-06-18 12:00:14 UTC
parent d075e9e023b9ccfeb9f64019df4f3010bc27d46a

Fix DNS lookup limit off-by-one error in the count

As per RFC, we should not count the initial DNS lookup for the purposes
of the lookup limit, but the code currently does.

This can result in over-limiting by 1 lookup.

This patch fixes the problem by incrementing the counts on their
respective mechanism and modifier. That behaviour also matches the RFC
wording, so it is easier to follow.

https://datatracker.ietf.org/doc/html/rfc7208#section-4.6.4

Thanks to Anh Do <doducanh2710@gmail.com> for reporting this issue in
https://github.com/albertito/spf/issues/3.

spf.go +2 -1
spf_test.go +5 -5
testdata/blitirispf-tests.yml +47 -0

diff --git a/spf.go b/spf.go
index 2b748d1..8a75d71 100644
--- a/spf.go
+++ b/spf.go
@@ -305,7 +305,6 @@ var mxField = regexp.MustCompile(`^(mx$|mx:|mx/)`)
 var ptrField = regexp.MustCompile(`^(ptr$|ptr:)`)
 
 func (r *resolution) Check(domain string) (Result, error) {
-	r.count++
 	r.trace("check %q %d %d", domain, r.count, r.voidcount)
 	txt, err := r.getDNSRecord(domain)
 	if err != nil {
@@ -637,6 +636,7 @@ func (r *resolution) includeField(res Result, field, domain string) (bool, Resul
 	if err != nil {
 		return true, PermError, ErrInvalidMacro
 	}
+	r.count++
 	ir, err := r.Check(incdomain)
 	switch ir {
 	case Pass:
@@ -813,6 +813,7 @@ func (r *resolution) redirectField(field, domain string) (Result, error) {
 	}
 
 	// https://tools.ietf.org/html/rfc7208#section-6.1
+	r.count++
 	result, err := r.Check(rDomain)
 	if result == None {
 		result = PermError
diff --git a/spf_test.go b/spf_test.go
index 58ee9ee..bae1098 100644
--- a/spf_test.go
+++ b/spf_test.go
@@ -500,22 +500,22 @@ func TestOverrideLookupLimit(t *testing.T) {
 	dns.Txt["domain3"] = []string{"v=spf1 include:domain4"}
 	dns.Txt["domain4"] = []string{"v=spf1 +all"}
 
-	// The default of 10 should be enough.
+	// The default of 10 should be plenty enough.
 	res, err := CheckHostWithSender(ip1111, "helo", "user@domain1")
 	if res != Pass {
 		t.Errorf("expected pass, got %q / %q", res, err)
 	}
 
-	// Set the limit to 4, which is enough.
+	// Set the limit to 3, which is just enough.
 	res, err = CheckHostWithSender(ip1111, "helo", "user@domain1",
-		OverrideLookupLimit(4))
+		OverrideLookupLimit(3))
 	if res != Pass {
 		t.Errorf("expected pass, got %q / %q", res, err)
 	}
 
-	// Set the limit to 3, which is not enough.
+	// Set the limit to 2, which is not enough.
 	res, err = CheckHostWithSender(ip1111, "helo", "user@domain1",
-		OverrideLookupLimit(3))
+		OverrideLookupLimit(2))
 	if res != PermError || err != ErrLookupLimitReached {
 		t.Errorf("expected permerror/lookup limit reached, got %q / %q",
 			res, err)
diff --git a/testdata/blitirispf-tests.yml b/testdata/blitirispf-tests.yml
index e2246b9..cb6047e 100644
--- a/testdata/blitirispf-tests.yml
+++ b/testdata/blitirispf-tests.yml
@@ -178,3 +178,50 @@ zonedata:
     - A: 127.0.0.9
   dom10.com:
     - A: 127.0.0.10
+---
+description: Resolution limits
+tests:
+  resolution-with-10-lookups:
+    description: |
+      Check that a resolution with precisely 10 lookups (the default limit)
+      works fine.
+    mailfrom: "foo@okay.com"
+    host: 1.2.3.4
+    result: pass
+  resolution-with-11-lookups:
+    description: |
+      Check that a resolution with precisely 11 lookups (over default limit)
+      fails as expected.
+    mailfrom: "foo@bad.com"
+    host: 1.2.3.4
+    result: permerror
+zonedata:
+  okay.com:
+    - SPF: v=spf1 include:d11 include:d12 include:d13
+                  include:d14 include:d15 -all
+  bad.com:
+    - SPF: v=spf1 include:d00
+                  include:d11 include:d12 include:d13
+                  include:d14 include:d15 -all
+  d00:
+    - SPF: v=spf1 -all
+  d11:
+    - SPF: v=spf1 include:d21 ~all
+  d12:
+    - SPF: v=spf1 include:d22 ~all
+  d13:
+    - SPF: v=spf1 include:d23 ~all
+  d14:
+    - SPF: v=spf1 include:d24 ~all
+  d15:
+    - SPF: v=spf1 include:d25 ~all
+  d21:
+    - SPF: v=spf1 -all
+  d22:
+    - SPF: v=spf1 -all
+  d23:
+    - SPF: v=spf1 -all
+  d24:
+    - SPF: v=spf1 -all
+  d25:
+    - SPF: v=spf1 all