From: Vsevolod Stakhov Date: Sat, 25 Jul 2026 13:05:06 +0000 (+0100) Subject: [Fix] spf: enforce the include/redirect nesting limit X-Git-Tag: 4.1.3~9 X-Git-Url: http://git.ipfire.org/gitweb/index.cgi?a=commitdiff_plain;h=b80fd076db96367f0b5ab64ea885ccf2bd7697e6;p=thirdparty%2Frspamd.git [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 f89fc86762..67976d1e5c 100644 --- 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 87aa507254..4ad8845d13 100644 --- 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 7cfe58650a..451502c1b3 100644 --- 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 b219215154..789f4eef07 100644 --- 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 3a2fdde2c5..670aea3c20 100644 --- 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";