From 330d5211c9aabbb805eb3e47932d4d5899dc4e51 Mon Sep 17 00:00:00 2001 From: "W.C.A. Wijngaards" Date: Fri, 10 Apr 2026 15:45:28 +0200 Subject: [PATCH] - Fix for EDNS client subnet so that it does not store SERVFAIL in the global cache after a failed lookup, such as timeouts. A failure entry is stored in the subnet cache, for the query name, for a couple of seconds. Queries can continue to use the subnet cache during that time. --- doc/Changelog | 7 + edns-subnet/subnetmod.c | 67 +++++- edns-subnet/subnetmod.h | 10 + iterator/iterator.c | 1 + services/mesh.c | 1 + testdata/subnet_cached_servfail_timeout.crpl | 239 +++++++++++++++++++ util/module.h | 4 + 7 files changed, 328 insertions(+), 1 deletion(-) create mode 100644 testdata/subnet_cached_servfail_timeout.crpl diff --git a/doc/Changelog b/doc/Changelog index b8cb16dc1..0cc46722d 100644 --- a/doc/Changelog +++ b/doc/Changelog @@ -1,3 +1,10 @@ +10 April 2026: Wouter + - Fix for EDNS client subnet so that it does not store SERVFAIL in + the global cache after a failed lookup, such as timeouts. A failure + entry is stored in the subnet cache, for the query name, for a + couple of seconds. Queries can continue to use the subnet cache + during that time. + 7 April 2026: Yorgos - Fix unused variable warning. diff --git a/edns-subnet/subnetmod.c b/edns-subnet/subnetmod.c index 8745d472a..2ed0ef931 100644 --- a/edns-subnet/subnetmod.c +++ b/edns-subnet/subnetmod.c @@ -70,6 +70,7 @@ subnet_data_delete(void *d, void *ATTR_UNUSED(arg)) r = (struct subnet_msg_cache_data*)d; addrtree_delete(r->tree4); addrtree_delete(r->tree6); + free(r->reason_fail); free(r); } @@ -84,6 +85,8 @@ msg_cache_sizefunc(void *k, void *d) + q->key.qname_len + lock_get_mem(&q->entry.lock); s += addrtree_size(r->tree4); s += addrtree_size(r->tree6); + if(r->reason_fail) + s += strlen(r->reason_fail)+1; return s; } @@ -200,12 +203,18 @@ int ecs_whitelist_check(struct query_info* qinfo, if(sq->ecs_server_out.subnet_source_mask == 0) { sq->subnet_sent_no_subnet = 1; sq->subnet_sent = 0; + /* The result should end up in subnet cache, + * not in global cache. */ + qstate->no_cache_store = 1; return 1; } subnet_ecs_opt_list_append(&sq->ecs_server_out, &qstate->edns_opts_back_out, qstate, region); } sq->subnet_sent = 1; + /* Do not store servfails in global cache, since the subnet + * option is sent out. */ + qstate->no_cache_store = 1; } else { /* Outgoing ECS option is set, but we don't want to sent it to @@ -427,6 +436,26 @@ update_cache(struct module_qstate *qstate, int id) } /* lru_entry->lock is locked regardless of how we got here, * either from the slabhash_lookup, or above in the new allocated */ + if(!qstate->return_msg && qstate->error_response_cache) { + struct subnet_msg_cache_data *data = + (struct subnet_msg_cache_data*)lru_entry->data; + data->ttl_servfail = *qstate->env->now + NORR_TTL; + data->ede_fail = errinf_to_reason_bogus(qstate); + diff_size = (data->reason_fail?strlen(data->reason_fail)+1:0); + if(qstate->errinf) { + char* str = errinf_to_str_misc(qstate); + free(data->reason_fail); + data->reason_fail = NULL; + if(str) + data->reason_fail = strdup(str); + } + diff_size = (data->reason_fail?strlen(data->reason_fail)+1:0) + - diff_size; + lock_rw_unlock(&lru_entry->lock); + slabhash_update_space_used(subnet_msg_cache, h, NULL, + diff_size); + return; + } /* Step 2, find the correct tree */ if (!(tree = get_tree(lru_entry->data, edns, sne, qstate->env->cfg))) { lock_rw_unlock(&lru_entry->lock); @@ -470,6 +499,21 @@ update_cache(struct module_qstate *qstate, int id) } } +/** See if there is a stored servfail, returns true if so, and sets reply. */ +static int +lookup_check_servfail(struct module_qstate *qstate, + struct subnet_msg_cache_data *data) +{ + struct module_env *env = qstate->env; + if(!data) + return 0; + if(!data->ttl_servfail || TTL_IS_EXPIRED(data->ttl_servfail, *env->now)) + return 0; + qstate->return_rcode = LDNS_RCODE_SERVFAIL; + errinf_ede(qstate, data->reason_fail, data->ede_fail); + return 1; +} + /** Lookup in cache and reply true iff reply is sent. */ static int lookup_and_reply(struct module_qstate *qstate, int id, struct subnet_qstate *sq, int prefetch) @@ -498,12 +542,20 @@ lookup_and_reply(struct module_qstate *qstate, int id, struct subnet_qstate *sq, tree = (ecs->subnet_addr_fam == EDNSSUBNET_ADDRFAM_IP4)? data->tree4 : data->tree6; if (!tree) { /* qinfo in cache but not for this family */ + if(lookup_check_servfail(qstate, data)) { + lock_rw_unlock(&e->lock); + return 1; + } lock_rw_unlock(&e->lock); return 0; } node = addrtree_find(tree, (addrkey_t*)ecs->subnet_addr, ecs->subnet_source_mask, *env->now); if (!node) { /* plain old cache miss */ + if(lookup_check_servfail(qstate, data)) { + lock_rw_unlock(&e->lock); + return 1; + } lock_rw_unlock(&e->lock); return 0; } @@ -512,11 +564,16 @@ lookup_and_reply(struct module_qstate *qstate, int id, struct subnet_qstate *sq, (struct reply_info *)node->elem, qstate->region, *env->now, 0, env->scratch); scope = (uint8_t)node->scope; - lock_rw_unlock(&e->lock); if (!qstate->return_msg) { /* Failed allocation or expired TTL */ + if(lookup_check_servfail(qstate, data)) { + lock_rw_unlock(&e->lock); + return 1; + } + lock_rw_unlock(&e->lock); return 0; } + lock_rw_unlock(&e->lock); if(qstate->return_msg->rep->security == sec_status_unchecked && must_validate) { /* The message has to be validated first. */ @@ -652,6 +709,12 @@ eval_response(struct module_qstate *qstate, int id, struct subnet_qstate *sq) /* already an answer and its not a message, but retain * the actual rcode, instead of module_error, so send * module_finished */ + if(qstate->error_response_cache) { + verbose(VERB_ALGO, "subnet: store error response"); + lock_rw_wrlock(&sne->biglock); + update_cache(qstate, id); + lock_rw_unlock(&sne->biglock); + } return module_finished; } @@ -901,9 +964,11 @@ ecs_edns_back_parsed(struct module_qstate* qstate, int id, sq->max_scope = sq->ecs_server_in.subnet_scope_mask; } else if(sq->subnet_sent_no_subnet) { /* The answer can be stored as scope 0, not in global cache. */ + /* This was already set in ecs_whitelist_check */ qstate->no_cache_store = 1; } else if(sq->subnet_sent) { /* Need another query to be able to store in global cache. */ + /* This was already set in ecs_whitelist_check */ qstate->no_cache_store = 1; } diff --git a/edns-subnet/subnetmod.h b/edns-subnet/subnetmod.h index d2d9e957f..5130f781b 100644 --- a/edns-subnet/subnetmod.h +++ b/edns-subnet/subnetmod.h @@ -69,8 +69,18 @@ struct subnet_env { }; struct subnet_msg_cache_data { + /** Tree for nodes with IPv4 subnets. */ struct addrtree* tree4; + /** Tree for nodes with IPv6 subnets. */ struct addrtree* tree6; + /** If servfail is stored, for how long. Abs time in seconds. + * This protects against too much recusion on the item when + * resolution fails, for a couple of seconds. */ + time_t ttl_servfail; + /** servfail ede */ + sldns_ede_code ede_fail; + /** servfail reason */ + char* reason_fail; }; struct subnet_qstate { diff --git a/iterator/iterator.c b/iterator/iterator.c index 5013c75ad..b8bfc4327 100644 --- a/iterator/iterator.c +++ b/iterator/iterator.c @@ -297,6 +297,7 @@ error_response_cache(struct module_qstate* qstate, int id, int rcode) struct reply_info err; struct msgreply_entry* msg; if(qstate->no_cache_store) { + qstate->error_response_cache = 1; return error_response(qstate, id, rcode); } if(qstate->prefetch_leeway > NORR_TTL) { diff --git a/services/mesh.c b/services/mesh.c index 3a1cc2a78..ad79acf41 100644 --- a/services/mesh.c +++ b/services/mesh.c @@ -1036,6 +1036,7 @@ mesh_state_create(struct module_env* env, struct query_info* qinfo, mstate->s.no_cache_store = 0; mstate->s.need_refetch = 0; mstate->s.was_ratelimited = 0; + mstate->s.error_response_cache = 0; mstate->s.qstarttime = *env->now; /* init modules */ diff --git a/testdata/subnet_cached_servfail_timeout.crpl b/testdata/subnet_cached_servfail_timeout.crpl new file mode 100644 index 000000000..baf523227 --- /dev/null +++ b/testdata/subnet_cached_servfail_timeout.crpl @@ -0,0 +1,239 @@ +; Check if an SERVFAIL answer is not stored in the global cache, and +; does not block ECS queries to reach the ECS cache. + +server: + trust-anchor-signaling: no + target-fetch-policy: "0 0 0 0 0" + ;send-client-subnet: 1.2.3.4 + client-subnet-zone: "example.com" + max-client-subnet-ipv4: 21 + module-config: "subnetcache iterator" + verbosity: 3 + access-control: 127.0.0.1 allow_snoop + qname-minimisation: no + minimal-responses: yes + prefetch: yes + outbound-msg-retry: 3 + ede: yes + log-servfail: yes + +stub-zone: + name: "example.com." + stub-addr: 1.2.3.4 +CONFIG_END + +SCENARIO_BEGIN Test that SERVFAIL after timeout does not block clients to reach the ECS cache +; And that withing the servfail time a couple of seconds have cached servfail +; for the subnet queries for that name. + +; ns.example.com. +RANGE_BEGIN 1 20 +ADDRESS 1.2.3.4 +ENTRY_BEGIN +MATCH opcode qtype qname +ADJUST copy_id +REPLY QR NOERROR +SECTION QUESTION +example.com. IN NS +SECTION ANSWER +example.com. IN NS ns.example.com. +SECTION ADDITIONAL +ns.example.com. IN A 1.2.3.4 +ENTRY_END + +; response to query of interest +ENTRY_BEGIN +MATCH opcode qtype qname ednsdata +ADJUST copy_id copy_ednsdata_assume_clientsubnet +REPLY QR NOERROR +SECTION QUESTION +www.example.com. IN A +SECTION ANSWER +www.example.com. 10 IN A 10.20.30.40 +SECTION AUTHORITY +SECTION ADDITIONAL +HEX_EDNSDATA_BEGIN + ; client is 127.0.0.1 + 00 08 ; OPC + 00 05 ; option length + 00 01 ; Family + 08 00 ; source mask, scopemask + 7f ; address +HEX_EDNSDATA_END +ENTRY_END +RANGE_END + +; ns.example.com. +RANGE_BEGIN 100 120 +ADDRESS 1.2.3.4 + +; response to query of interest +ENTRY_BEGIN +MATCH opcode qtype qname ednsdata +ADJUST copy_id copy_ednsdata_assume_clientsubnet +REPLY QR NOERROR +SECTION QUESTION +www.example.com. IN A +SECTION ANSWER +www.example.com. 10 IN A 10.20.30.41 +SECTION AUTHORITY +SECTION ADDITIONAL +HEX_EDNSDATA_BEGIN + ; client is 1.0.0.0 + 00 08 ; OPC + 00 05 ; option length + 00 01 ; Family + 08 00 ; source mask, scopemask + 01 ; address +HEX_EDNSDATA_END +ENTRY_END +RANGE_END + +; Put an item in subnet cache +STEP 10 QUERY +ENTRY_BEGIN +REPLY RD DO +SECTION QUESTION +www.example.com. IN A +SECTION ADDITIONAL +HEX_EDNSDATA_BEGIN + 00 08 00 05 ; OPC, optlen + 00 01 08 08 ; ip4, source 8, scope 8 + 7f ; 127.0.0.0/8 +HEX_EDNSDATA_END +ENTRY_END + +STEP 20 CHECK_ANSWER +ENTRY_BEGIN +MATCH all ttl +REPLY QR RD RA DO NOERROR +SECTION QUESTION +www.example.com. IN A +SECTION ANSWER +www.example.com. 10 IN A 10.20.30.40 +SECTION AUTHORITY +SECTION ADDITIONAL +HEX_EDNSDATA_BEGIN + 00 08 00 05 ; OPC, optlen + 00 01 08 08 ; ip4, source 8, scope 8 + 7f ; 127.0.0.0/8 +HEX_EDNSDATA_END +ENTRY_END + +; There is a valid subnet query in cache. +; this query timeouts. +STEP 30 QUERY +ENTRY_BEGIN +REPLY RD DO +SECTION QUESTION +www.example.com. IN A +SECTION ADDITIONAL +HEX_EDNSDATA_BEGIN + 00 08 00 05 ; OPC, optlen + 00 01 08 00 ; ip4, source 8, scope 0 + 01 ; 1.0.0.0/8 +HEX_EDNSDATA_END +ENTRY_END + +; This query faces timeouts during the resolution. +; The timeouted query is the 1.0.0.0/8 subnet lookup of www.example.com. A. +STEP 31 TIMEOUT +STEP 32 TIMEOUT +STEP 33 TIMEOUT + +STEP 40 CHECK_ANSWER +ENTRY_BEGIN +MATCH all +REPLY QR RD DO RA SERVFAIL +SECTION QUESTION +www.example.com. IN A +ENTRY_END + +; Check if subnet cache item can be accessed. +STEP 50 QUERY +ENTRY_BEGIN +REPLY RD DO +SECTION QUESTION +www.example.com. IN A +SECTION ADDITIONAL +HEX_EDNSDATA_BEGIN + 00 08 00 05 ; OPC, optlen + 00 01 08 00 ; ip4, source 8, scope 0 + 7f ; 127.0.0.0/8 +HEX_EDNSDATA_END +ENTRY_END + +STEP 60 CHECK_ANSWER +ENTRY_BEGIN +MATCH all ttl +REPLY QR RD RA DO NOERROR +SECTION QUESTION +www.example.com. IN A +SECTION ANSWER +www.example.com. 10 IN A 10.20.30.40 +SECTION AUTHORITY +SECTION ADDITIONAL +HEX_EDNSDATA_BEGIN + 00 08 00 05 ; OPC, optlen + 00 01 08 08 ; ip4, source 8, scope 8 + 7f ; 127.0.0.0/8 +HEX_EDNSDATA_END +ENTRY_END + +; the existing subnet cache item can be accessed. +; but another resolution, is now not cached at all? +STEP 70 QUERY +ENTRY_BEGIN +REPLY RD DO +SECTION QUESTION +www.example.com. IN A +SECTION ADDITIONAL +HEX_EDNSDATA_BEGIN + 00 08 00 05 ; OPC, optlen + 00 01 08 00 ; ip4, source 8, scope 0 + 01 ; 1.0.0.0/8 +HEX_EDNSDATA_END +ENTRY_END + +STEP 80 CHECK_ANSWER +ENTRY_BEGIN +MATCH all +REPLY QR RD DO RA SERVFAIL +SECTION QUESTION +www.example.com. IN A +ENTRY_END + +; after a couple of seconds, the servfail entry should have cleared. +STEP 90 TIME_PASSES ELAPSE 10 + +STEP 100 QUERY +ENTRY_BEGIN +REPLY RD DO +SECTION QUESTION +www.example.com. IN A +SECTION ADDITIONAL +HEX_EDNSDATA_BEGIN + 00 08 00 05 ; OPC, optlen + 00 01 08 00 ; ip4, source 8, scope 0 + 01 ; 1.0.0.0/8 +HEX_EDNSDATA_END +ENTRY_END + +STEP 110 CHECK_ANSWER +ENTRY_BEGIN +MATCH all ttl +REPLY QR RD RA DO NOERROR +SECTION QUESTION +www.example.com. IN A +SECTION ANSWER +www.example.com. 10 IN A 10.20.30.41 +SECTION AUTHORITY +SECTION ADDITIONAL +HEX_EDNSDATA_BEGIN + 00 08 00 05 ; OPC, optlen + 00 01 08 08 ; ip4, source 8, scope 8 + 01 ; 1.0.0.0/8 +HEX_EDNSDATA_END +ENTRY_END + +SCENARIO_END diff --git a/util/module.h b/util/module.h index 8ad8c48d1..a6fa6be90 100644 --- a/util/module.h +++ b/util/module.h @@ -698,6 +698,10 @@ struct module_qstate { time_t qstarttime; /** whether a message from cachedb will be used for the reply */ int is_cachedb_answer; + /** if the response as error is from error_response_cache, and is + * suitable for caching (briefly) the error response. Set by the + * iterator when no_cache_store is enabled, and there is an error. */ + int error_response_cache; /** * Attributes of clients that share the qstate that may affect IP-based -- 2.47.3