[Fix] spf: enforce the include/redirect nesting limit

`rec->nested` was checked but never incremented, so `max_dns_nesting`
had no effect at all and an include chain was only ever bounded by the
DNS requests counter. That counter was compared with `>` while being
incremented after the check, hence 31 DNS elements were evaluated with
a limit of 30.

Store the depth per resolved element, as the resolution is asynchronous
and there is no call stack reflecting the position in the tree, and
check it when an include or a redirect is about to be followed. Compare
the DNS requests counter with `>=` so that exactly `max_dns_requests`
elements are evaluated.

Flattening a record recurses over the same chain, so it is also bounded
by `SPF_MAX_NESTING_HARD`, which applies even when the configurable
limits are relaxed or disabled by setting them to zero.

diff --git a/src/libserver/spf.c b/src/libserver/spf.c
index f89fc8676294a2bf35000e4592d1421b16afcce2..67976d1e5cc439cb3e2c10ff33a946c1aea1a73e 100644 (file)
--- a/src/libserver/spf.c
+++ b/src/libserver/spf.c
@@ -44,11 +44,11 @@
 struct spf_resolved_element {
        GPtrArray *elts;
        char *cur_domain;
+       unsigned int nested; /* Depth of this element in the include/redirect chain */
        gboolean redirected; /* Ignore level, it's redirected */
 };
 
 struct spf_record {
-       int nested;
        int dns_requests;
        unsigned int dns_expansions; /* Address requests spawned by mx/ptr expansion */
        int requests_inflight;
@@ -144,17 +144,40 @@ struct spf_dns_cb {
 static inline bool
 spf_record_can_dns(const struct spf_record *rec)
 {
-       if (spf_lib_ctx->max_dns_nesting > 0 &&
-               rec->nested > spf_lib_ctx->max_dns_nesting) {
-               msg_warn_spf("spf nesting limit: %d > %d is reached, domain: %s",
-                                        rec->nested, spf_lib_ctx->max_dns_nesting,
+       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);
                return false;
        }
-       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,
+
+       return true;
+}
+
+/*
+ * Check whether a new element can be nested under `parent`, that is whether an
+ * include or a redirect found in `parent` may be followed. Depth is stored per
+ * element as the resolution is asynchronous and there is no call stack to
+ * reflect the position in the include/redirect tree.
+ */
+static inline bool
+spf_record_can_nest(const struct spf_record *rec,
+                                       const struct spf_resolved_element *parent)
+{
+       unsigned int nested = parent->nested + 1;
+
+       if (nested > SPF_MAX_NESTING_HARD) {
+               msg_warn_spf("spf hard nesting limit: %ud > %ud is reached, domain: %s",
+                                        nested, (unsigned int) SPF_MAX_NESTING_HARD,
+                                        rec->sender_domain);
+               return false;
+       }
+
+       if (spf_lib_ctx->max_dns_nesting > 0 &&
+               nested > spf_lib_ctx->max_dns_nesting) {
+               msg_warn_spf("spf nesting limit: %ud > %ud is reached, domain: %s",
+                                        nested, spf_lib_ctx->max_dns_nesting,
                                         rec->sender_domain);
                return false;
        }
@@ -167,6 +190,11 @@ spf_record_can_dns(const struct spf_record *rec)
                if (!spf_record_can_dns(rec)) return FALSE; \
        } while (0)
 
+#define CHECK_NESTING(rec, parent)                           \
+       do {                                                     \
+               if (!spf_record_can_nest(rec, parent)) return FALSE; \
+       } while (0)
+
 RSPAMD_CONSTRUCTOR(rspamd_spf_lib_ctx_ctor)
 {
        spf_lib_ctx = g_malloc0(sizeof(*spf_lib_ctx));
@@ -412,12 +440,29 @@ rspamd_flatten_record_dtor(struct spf_resolved *r)
 
 static void
 rspamd_spf_process_reference(struct spf_resolved *target,
-                                                        struct spf_addr *addr, struct spf_record *rec, gboolean top)
+                                                        struct spf_addr *addr, struct spf_record *rec,
+                                                        gboolean top, unsigned int depth)
 {
        struct spf_resolved_element *elt, *relt;
        struct spf_addr *cur = NULL, taddr, *cur_addr;
        unsigned int i;
 
+       /*
+        * References always point to an element that has been added after the
+        * current one, so the graph cannot contain loops. Depth, however, follows
+        * the include/redirect chain, hence the same bound as for the resolution
+        * stage is applied here to keep this recursion shallow
+        */
+       if (depth > SPF_MAX_NESTING_HARD) {
+               msg_warn_spf("spf flattening depth limit: %ud > %ud is reached, "
+                                        "domain: %s",
+                                        depth, (unsigned int) SPF_MAX_NESTING_HARD,
+                                        rec->sender_domain);
+               target->flags |= RSPAMD_SPF_RESOLVED_PERM_FAILED;
+
+               return;
+       }
+
        if (addr) {
                g_assert(addr->m.idx < rec->resolved->len);
 
@@ -490,11 +535,11 @@ rspamd_spf_process_reference(struct spf_resolved *target,
                        /* Process reference */
                        if (cur->flags & RSPAMD_SPF_FLAG_REDIRECT) {
                                /* Stop on redirected domain */
-                               rspamd_spf_process_reference(target, cur, rec, top);
+                               rspamd_spf_process_reference(target, cur, rec, top, depth + 1);
                                break;
                        }
                        else {
-                               rspamd_spf_process_reference(target, cur, rec, FALSE);
+                               rspamd_spf_process_reference(target, cur, rec, FALSE, depth + 1);
                        }
                }
                else {
@@ -537,7 +582,7 @@ rspamd_spf_record_flatten(struct spf_record *rec)
                                                                          rec->resolved->len);
 
                if (rec->resolved->len > 0) {
-                       rspamd_spf_process_reference(res, NULL, rec, TRUE);
+                       rspamd_spf_process_reference(res, NULL, rec, TRUE, 0);
                }
        }
        else {
@@ -1727,13 +1772,15 @@ parse_spf_ip6(struct spf_record *rec, struct spf_addr *addr)
 
 
 static gboolean
-parse_spf_include(struct spf_record *rec, struct spf_addr *addr)
+parse_spf_include(struct spf_record *rec,
+                                 struct spf_resolved_element *resolved, struct spf_addr *addr)
 {
        struct spf_dns_cb *cb;
        const char *domain;
        struct rspamd_task *task = rec->task;
 
        CHECK_REC(rec);
+       CHECK_NESTING(rec, resolved);
        domain = strchr(addr->spf_string, ':');
 
        if (domain == NULL) {
@@ -1757,6 +1804,7 @@ parse_spf_include(struct spf_record *rec, struct spf_addr *addr)
        cb->initiated_by = SPF_RESOLVE_INCLUDE;
        addr->m.idx = rec->resolved->len;
        cb->resolved = rspamd_spf_new_addr_list(rec, domain);
+       cb->resolved->nested = resolved->nested + 1;
        cb->initiated_dns_name = domain;
        /* Set reference */
        addr->flags |= RSPAMD_SPF_FLAG_REFERENCE;
@@ -1793,6 +1841,7 @@ parse_spf_redirect(struct spf_record *rec,
        struct rspamd_task *task = rec->task;
 
        CHECK_REC(rec);
+       CHECK_NESTING(rec, resolved);
 
        domain = strchr(addr->spf_string, '=');
 
@@ -1821,6 +1870,7 @@ parse_spf_redirect(struct spf_record *rec,
        cb->addr = addr;
        cb->initiated_by = SPF_RESOLVE_REDIRECT;
        cb->resolved = rspamd_spf_new_addr_list(rec, domain);
+       cb->resolved->nested = resolved->nested + 1;
        cb->initiated_dns_name = domain;
        msg_debug_spf("resolve redirect %s", domain);
 
@@ -2442,7 +2492,7 @@ spf_process_element(struct spf_record *rec,
                        res = parse_spf_ip4(rec, addr);
                }
                else if (g_ascii_strncasecmp(begin, SPF_INCLUDE, sizeof(SPF_INCLUDE) - 1) == 0) {
-                       res = parse_spf_include(rec, addr);
+                       res = parse_spf_include(rec, resolved, addr);
                }
                else if (g_ascii_strncasecmp(begin, SPF_IP6, sizeof(SPF_IP6) - 1) == 0) {
                        res = parse_spf_ip6(rec, addr);

diff --git a/src/libserver/spf.h b/src/libserver/spf.h
index 87aa5072548b4dd94300005c91919719870222b5..4ad8845d132c6d73931705b2ee2a9644ca4968bf 100644 (file)
--- a/src/libserver/spf.h
+++ b/src/libserver/spf.h
@@ -83,6 +83,12 @@ typedef enum spf_action_e {
 /** Default SPF limits for avoiding abuse **/
 #define SPF_MAX_NESTING 10
 #define SPF_MAX_DNS_REQUESTS 30
+/*
+ * Absolute limit for the include/redirect chain depth. Flattening a record is
+ * recursive, hence this bound is applied even when the configurable limits are
+ * relaxed or disabled by setting them to zero
+ */
+#define SPF_MAX_NESTING_HARD 64
 /*
  * RFC 7208 4.6.4: evaluation of each `mx` or `ptr` record must not result in
  * querying more than 10 address records

diff --git a/src/plugins/lua/spf.lua b/src/plugins/lua/spf.lua
index 7cfe58650a4de22c5b3d2b215eac9385f6c254b3..451502c1b3d6b43b982d4cab5397788bfa054700 100644 (file)
--- a/src/plugins/lua/spf.lua
+++ b/src/plugins/lua/spf.lua
@@ -33,7 +33,8 @@ spf {
   spf_cache_expire = 1d;
   # Whitelist IPs from checks
   whitelist = "/path/to/some/file";
-  # Maximum number of recursive DNS subrequests (e.g. includes chanin length)
+  # Maximum number of recursive DNS subrequests (e.g. includes chanin length),
+  # 0 disables the limit but the chain is still capped at 64 levels
   max_dns_nesting = 10;
   # Maximum count of DNS requests per record
   max_dns_requests = 30;

diff --git a/test/functional/cases/001_merged/117_spf.robot b/test/functional/cases/001_merged/117_spf.robot
index b21921515408bf24b56704633a1b8fd197992161..789f4eef07bb0c8c0feab78dfb4e44ea03310fc3 100644 (file)
--- a/test/functional/cases/001_merged/117_spf.robot
+++ b/test/functional/cases/001_merged/117_spf.robot
@@ -185,3 +185,31 @@ SPF PERMFAIL TOO MANY MX NAMES
   ...  Settings=${SETTINGS_SPF}
   Expect Symbol  R_SPF_PERMFAIL
   Do Not Expect Symbol  R_SPF_FAIL
+
+SPF NESTING 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
+  Scan File  ${RSPAMD_TESTDIR}/messages/dmarc/bad_dkim1.eml
+  ...  IP=8.8.8.8  From=x@nest.org.org.za
+  ...  Settings=${SETTINGS_SPF}
+  Expect Symbol  R_SPF_ALLOW
+  Scan File  ${RSPAMD_TESTDIR}/messages/dmarc/bad_dkim1.eml
+  ...  IP=8.8.4.4  From=x@nest.org.org.za
+  ...  Settings=${SETTINGS_SPF}
+  Expect Symbol  R_SPF_FAIL
+  Do Not Expect Symbol  R_SPF_ALLOW
+
+SPF DNS REQUESTS 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
+  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
+  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
+  Do Not Expect Symbol  R_SPF_ALLOW

diff --git a/test/functional/configs/merged-local.conf b/test/functional/configs/merged-local.conf
index 3a2fdde2c5ab66bcb2698c553231a0a9561affcd..670aea3c20851deacaca51309aefec0211204870 100644 (file)
--- a/test/functional/configs/merged-local.conf
+++ b/test/functional/configs/merged-local.conf
@@ -362,6 +362,43 @@ 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: 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",
+          replies = ["v=spf1 a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:hit.req.org.org.za -all"]},
+        {name = "fewreq.org.org.za", type = "txt",
+          replies = ["v=spf1 a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:pad.req.org.org.za a:hit.req.org.org.za -all"]},
+        {name = "pad.req.org.org.za", type = "a", replies = ["192.0.2.30"]},
+        {name = "pad.req.org.org.za", type = "aaaa", rcode = "norec"},
+        {name = "hit.req.org.org.za", type = "a", replies = ["8.8.8.8"]},
+        {name = "hit.req.org.org.za", type = "aaaa", rcode = "norec"},
         {
           name = "fail7.org.org.za",
           type = "aaaa";