[Fix] spf: return permerror when a DNS limit is hit

Exceeding `max_dns_requests` or `max_dns_nesting` only made the
offending element unparsed, so the record was still evaluated using
whatever fitted in the budget and usually ended up as a definitive fail
via its trailing `-all`. RFC 7208 4.6.4 requires permerror instead, and
rightly so: the record has not been evaluated to the end, hence its
verdict is unknown rather than negative.

Set `permfail` on both limits, as is already done for the address
lookups spawned by `mx` and `ptr`. Records carrying any flag are not put
into the LRU cache, so a permerror is not stored for the ttl of the
record.

Note that a strictly sequential evaluator could still return pass for a
record whose match precedes the term that exceeds the limit. Rspamd
resolves a record as a whole before matching, so it cannot tell that the
budget would have been enough, and reports permerror for the record.

diff --git a/src/libserver/spf.c b/src/libserver/spf.c
index f9e2843ee02247e30e04fb867785bac47898ba65..488e94a095501282578af5765ae9ed8a71c32d97 100644 (file)
--- a/src/libserver/spf.c
+++ b/src/libserver/spf.c
@@ -141,14 +141,21 @@ struct spf_dns_cb {
        unsigned expanded_names;    /* number of mx/ptr names expanded to addresses */
 };
 
+/*
+ * A record that hits a DNS limit cannot be evaluated to the end, so RFC 7208
+ * 4.6.4 requires permerror for it. Reporting a definitive fail instead would
+ * mean judging a record by the part of it that happened to fit in the budget
+ */
 static inline bool
-spf_record_can_dns(const struct spf_record *rec)
+spf_record_can_dns(struct spf_record *rec)
 {
        if (spf_lib_ctx->max_dns_requests > 0 &&
                rec->dns_requests >= spf_lib_ctx->max_dns_requests) {
                msg_warn_spf("spf dns requests limit: %d >= %d is reached, domain: %s",
                                         rec->dns_requests, spf_lib_ctx->max_dns_requests,
                                         rec->sender_domain);
+               rec->permfail = true;
+
                return false;
        }
 
@@ -162,7 +169,7 @@ spf_record_can_dns(const struct spf_record *rec)
  * reflect the position in the include/redirect tree.
  */
 static inline bool
-spf_record_can_nest(const struct spf_record *rec,
+spf_record_can_nest(struct spf_record *rec,
                                        const struct spf_resolved_element *parent)
 {
        unsigned int nested = parent->nested + 1;
@@ -171,6 +178,8 @@ spf_record_can_nest(const struct spf_record *rec,
                msg_warn_spf("spf hard nesting limit: %ud > %ud is reached, domain: %s",
                                         nested, (unsigned int) SPF_MAX_NESTING_HARD,
                                         rec->sender_domain);
+               rec->permfail = true;
+
                return false;
        }
 
@@ -179,6 +188,8 @@ spf_record_can_nest(const struct spf_record *rec,
                msg_warn_spf("spf nesting limit: %ud > %ud is reached, domain: %s",
                                         nested, spf_lib_ctx->max_dns_nesting,
                                         rec->sender_domain);
+               rec->permfail = true;
+
                return false;
        }
 

diff --git a/test/functional/cases/001_merged/117_spf.robot b/test/functional/cases/001_merged/117_spf.robot
index e0a49bfafbb95cdec940b02de65356e6c3074444..a44897ba847e6302555132df332a8b55b77c7cc7 100644 (file)
--- a/test/functional/cases/001_merged/117_spf.robot
+++ b/test/functional/cases/001_merged/117_spf.robot
@@ -186,19 +186,24 @@ SPF PERMFAIL TOO MANY MX NAMES
   Expect Symbol  R_SPF_PERMFAIL
   Do Not Expect Symbol  R_SPF_FAIL
 
-SPF NESTING LIMIT
+SPF ALLOW NESTING AT LIMIT
   [Documentation]  An include chain is followed up to max_dns_nesting levels
-  ...  (10 by default) and no further: 8.8.8.8 is listed by the last allowed
-  ...  level, 8.8.4.4 by the level below it that must never be resolved
+  ...  (10 by default), 8.8.8.8 is listed by the last allowed one
   Scan File  ${RSPAMD_TESTDIR}/messages/dmarc/bad_dkim1.eml
-  ...  IP=8.8.8.8  From=x@nest.org.org.za
+  ...  IP=8.8.8.8  From=x@nestok.org.org.za
   ...  Settings=${SETTINGS_SPF}
   Expect Symbol  R_SPF_ALLOW
+
+SPF PERMFAIL NESTING BEYOND LIMIT
+  [Documentation]  The same chain entered one level deeper cannot be evaluated
+  ...  to the end, which is permerror per RFC 7208 4.6.4 rather than a fail
+  ...  based on the part of the record that fitted in the limit
   Scan File  ${RSPAMD_TESTDIR}/messages/dmarc/bad_dkim1.eml
-  ...  IP=8.8.4.4  From=x@nest.org.org.za
+  ...  IP=8.8.8.8  From=x@nestdeep.org.org.za
   ...  Settings=${SETTINGS_SPF}
-  Expect Symbol  R_SPF_FAIL
+  Expect Symbol  R_SPF_PERMFAIL
   Do Not Expect Symbol  R_SPF_ALLOW
+  Do Not Expect Symbol  R_SPF_FAIL
 
 SPF ALLOW EXISTS
   [Documentation]  RFC 7208 5.7: an exists element whose name resolves matches
@@ -218,16 +223,20 @@ SPF FAIL UNRESOLVEABLE EXISTS
   Expect Symbol  R_SPF_FAIL
   Do Not Expect Symbol  R_SPF_ALLOW
 
-SPF DNS REQUESTS LIMIT
+SPF ALLOW DNS REQUESTS AT LIMIT
   [Documentation]  Exactly max_dns_requests DNS elements (30 by default) are
-  ...  evaluated, the 31st one is not, so only the shorter record authorises
-  ...  8.8.8.8 by its last element
+  ...  evaluated, here the last of them authorises 8.8.8.8
   Scan File  ${RSPAMD_TESTDIR}/messages/dmarc/bad_dkim1.eml
   ...  IP=8.8.8.8  From=x@fewreq.org.org.za
   ...  Settings=${SETTINGS_SPF}
   Expect Symbol  R_SPF_ALLOW
+
+SPF PERMFAIL DNS REQUESTS BEYOND LIMIT
+  [Documentation]  RFC 7208 4.6.4: a record with one DNS element more than the
+  ...  limit allows cannot be evaluated to the end and yields permerror
   Scan File  ${RSPAMD_TESTDIR}/messages/dmarc/bad_dkim1.eml
   ...  IP=8.8.8.8  From=x@manyreq.org.org.za
   ...  Settings=${SETTINGS_SPF}
-  Expect Symbol  R_SPF_FAIL
+  Expect Symbol  R_SPF_PERMFAIL
   Do Not Expect Symbol  R_SPF_ALLOW
+  Do Not Expect Symbol  R_SPF_FAIL

diff --git a/test/functional/configs/merged-local.conf b/test/functional/configs/merged-local.conf
index 285232857d819131bd724dd9b4abb07b68764e38..f1685e949aeb7c63ed873b74386ec4babb88265c 100644 (file)
--- a/test/functional/configs/merged-local.conf
+++ b/test/functional/configs/merged-local.conf
@@ -362,33 +362,35 @@ options = {
         {name = "mx1.fewmx.org.org.za", type = "aaaa", rcode = "norec"},
         {name = "mx2.fewmx.org.org.za", type = "a", replies = ["192.0.2.20"]},
         {name = "mx2.fewmx.org.org.za", type = "aaaa", rcode = "norec"},
-        # 117_spf: an include chain that is exactly as deep as max_dns_nesting
-        # (10 by default) plus one more level that must not be followed:
-        # 8.8.8.8 comes from the last allowed level, 8.8.4.4 from beyond it
-        {name = "nest.org.org.za", type = "txt",
-          replies = ["v=spf1 include:l1.nest.org.org.za -all"]},
-        {name = "l1.nest.org.org.za", type = "txt",
-          replies = ["v=spf1 include:l2.nest.org.org.za -all"]},
-        {name = "l2.nest.org.org.za", type = "txt",
-          replies = ["v=spf1 include:l3.nest.org.org.za -all"]},
-        {name = "l3.nest.org.org.za", type = "txt",
-          replies = ["v=spf1 include:l4.nest.org.org.za -all"]},
-        {name = "l4.nest.org.org.za", type = "txt",
-          replies = ["v=spf1 include:l5.nest.org.org.za -all"]},
-        {name = "l5.nest.org.org.za", type = "txt",
-          replies = ["v=spf1 include:l6.nest.org.org.za -all"]},
-        {name = "l6.nest.org.org.za", type = "txt",
-          replies = ["v=spf1 include:l7.nest.org.org.za -all"]},
-        {name = "l7.nest.org.org.za", type = "txt",
-          replies = ["v=spf1 include:l8.nest.org.org.za -all"]},
-        {name = "l8.nest.org.org.za", type = "txt",
-          replies = ["v=spf1 include:l9.nest.org.org.za -all"]},
-        {name = "l9.nest.org.org.za", type = "txt",
-          replies = ["v=spf1 include:l10.nest.org.org.za -all"]},
-        {name = "l10.nest.org.org.za", type = "txt",
-          replies = ["v=spf1 ip4:8.8.8.8 include:l11.nest.org.org.za -all"]},
-        {name = "l11.nest.org.org.za", type = "txt",
-          replies = ["v=spf1 ip4:8.8.4.4 -all"]},
+        # 117_spf: two records sharing one include chain, entered one level
+        # apart, so that 8.8.8.8 sits exactly at max_dns_nesting (10 by
+        # default) for the first one and one level beyond it for the second
+        {name = "nestok.org.org.za", type = "txt",
+          replies = ["v=spf1 include:t1.nest.org.org.za -all"]},
+        {name = "nestdeep.org.org.za", type = "txt",
+          replies = ["v=spf1 include:t0.nest.org.org.za -all"]},
+        {name = "t0.nest.org.org.za", type = "txt",
+          replies = ["v=spf1 include:t1.nest.org.org.za -all"]},
+        {name = "t1.nest.org.org.za", type = "txt",
+          replies = ["v=spf1 include:t2.nest.org.org.za -all"]},
+        {name = "t2.nest.org.org.za", type = "txt",
+          replies = ["v=spf1 include:t3.nest.org.org.za -all"]},
+        {name = "t3.nest.org.org.za", type = "txt",
+          replies = ["v=spf1 include:t4.nest.org.org.za -all"]},
+        {name = "t4.nest.org.org.za", type = "txt",
+          replies = ["v=spf1 include:t5.nest.org.org.za -all"]},
+        {name = "t5.nest.org.org.za", type = "txt",
+          replies = ["v=spf1 include:t6.nest.org.org.za -all"]},
+        {name = "t6.nest.org.org.za", type = "txt",
+          replies = ["v=spf1 include:t7.nest.org.org.za -all"]},
+        {name = "t7.nest.org.org.za", type = "txt",
+          replies = ["v=spf1 include:t8.nest.org.org.za -all"]},
+        {name = "t8.nest.org.org.za", type = "txt",
+          replies = ["v=spf1 include:t9.nest.org.org.za -all"]},
+        {name = "t9.nest.org.org.za", type = "txt",
+          replies = ["v=spf1 include:hit.nest.org.org.za -all"]},
+        {name = "hit.nest.org.org.za", type = "txt",
+          replies = ["v=spf1 ip4:8.8.8.8 -all"]},
         # 117_spf: records with exactly max_dns_requests (30 by default) and one
         # more DNS element, the last element of each is the one matching 8.8.8.8
         {name = "manyreq.org.org.za", type = "txt",