From 9d7b1d3127f7245251adcd01a871d15d267064a3 Mon Sep 17 00:00:00 2001 From: Philip Homburg Date: Wed, 22 Mar 2023 10:51:56 +0100 Subject: [PATCH 1/4] Fix issue #860: Bad interaction with 0 TTL records and serve-expired --- services/cache/dns.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/services/cache/dns.c b/services/cache/dns.c index 6fc9919ef..1a4fbdd2f 100644 --- a/services/cache/dns.c +++ b/services/cache/dns.c @@ -134,24 +134,19 @@ msg_cache_remove(struct module_env* env, uint8_t* qname, size_t qnamelen, /** remove servfail msg cache entry */ static void -msg_del_servfail(struct module_env* env, struct query_info* qinfo, +msg_del_for_0ttl(struct module_env* env, struct query_info* qinfo, uint32_t flags) { struct msgreply_entry* e; - /* see if the entry is servfail, and then remove it, so that + /* see if there is an existing entry, and then remove it, so that * lookups move from the cacheresponse stage to the recursionresponse * stage */ e = msg_cache_lookup(env, qinfo->qname, qinfo->qname_len, qinfo->qtype, qinfo->qclass, flags, 0, 0); if(!e) return; - /* we don't check for the ttl here, also expired servfail entries + /* we don't check for the ttl here, also expired entries * are removed. If the user uses serve-expired, they would still be * used to answer from cache */ - if(FLAGS_GET_RCODE(((struct reply_info*)e->entry.data)->flags) - != LDNS_RCODE_SERVFAIL) { - lock_rw_unlock(&e->entry.lock); - return; - } lock_rw_unlock(&e->entry.lock); msg_cache_remove(env, qinfo->qname, qinfo->qname_len, qinfo->qtype, qinfo->qclass, flags); @@ -183,12 +178,18 @@ dns_cache_store_msg(struct module_env* env, struct query_info* qinfo, * which could be useful for delegation information */ verbose(VERB_ALGO, "TTL 0: dropped msg from cache"); free(rep); - /* if the message is SERVFAIL in cache, remove that SERVFAIL, + /* if the message is in then cache, remove that msg, * so that the TTL 0 response can be returned for future - * responses (i.e. don't get answered by the servfail from + * responses (i.e. don't get answered from * cache, but instead go to recursion to get this TTL0 - * response). */ - msg_del_servfail(env, qinfo, flags); + * response). + * Possible messages that could be in the cache: + * - SERVFAIL + * - NXDOMAIN + * - NODATA + * - an older record that is expired + * - an older record that did not yet expire */ + msg_del_for_0ttl(env, qinfo, flags); return; } From 072be3300f49d75a9dfd35c346bc9020f9623f9e Mon Sep 17 00:00:00 2001 From: Philip Homburg Date: Wed, 22 Mar 2023 15:21:19 +0100 Subject: [PATCH 2/4] Tests for serve-expired in combination with new 0 TTL data. --- testdata/serve_expired_0ttl_nodata.rpl | 154 +++++++++++++++++++++++ testdata/serve_expired_0ttl_nxdomain.rpl | 154 +++++++++++++++++++++++ testdata/serve_expired_0ttl_servfail.rpl | 129 +++++++++++++++++++ 3 files changed, 437 insertions(+) create mode 100644 testdata/serve_expired_0ttl_nodata.rpl create mode 100644 testdata/serve_expired_0ttl_nxdomain.rpl create mode 100644 testdata/serve_expired_0ttl_servfail.rpl diff --git a/testdata/serve_expired_0ttl_nodata.rpl b/testdata/serve_expired_0ttl_nodata.rpl new file mode 100644 index 000000000..e28e35255 --- /dev/null +++ b/testdata/serve_expired_0ttl_nodata.rpl @@ -0,0 +1,154 @@ +; config options +server: + module-config: "validator iterator" + qname-minimisation: "no" + minimal-responses: no + serve-expired: yes + log-servfail: yes + ede: yes + ede-serve-expired: yes + + +stub-zone: + name: "example.com" + stub-addr: 1.2.3.4 +CONFIG_END + +SCENARIO_BEGIN Test serve-expired with NXDOMAIN followed by 0 TTL +; Scenario overview: +; - query for 0ttl.example.com. IN A +; - answer from upstream is NODATA; will be cached for NORR_TTL(5) +; - check that the client gets the NODATA; also cached +; - query again right after the TTL expired +; - this time the server answers with a 0 TTL RRset +; - check that we get the correct answer + +; ns.example.com. +RANGE_BEGIN 0 20 + ADDRESS 1.2.3.4 + ; response to A query + ENTRY_BEGIN + MATCH opcode qtype qname + ADJUST copy_id + REPLY QR AA NOERROR + SECTION QUESTION + 0ttl.example.com. IN A + SECTION AUTHORITY + example.com IN SOA ns.example.com dns.example.com 1 7200 3600 2419200 10 + ENTRY_END +RANGE_END + +; ns.example.com. +RANGE_BEGIN 30 100 + ADDRESS 1.2.3.4 + ENTRY_BEGIN + MATCH opcode qtype qname + ADJUST copy_id + REPLY QR NOERROR + SECTION QUESTION + example.com. 10 IN NS + SECTION ANSWER + example.com. 10 IN NS ns.example.com. + SECTION ADDITIONAL + ns.example.com. 10 IN A 1.2.3.4 + ENTRY_END + + ENTRY_BEGIN + MATCH opcode qtype qname + ADJUST copy_id + REPLY QR NOERROR + SECTION QUESTION + 0ttl.example.com. IN A + SECTION ANSWER + 0ttl.example.com. 0 IN A 5.6.7.8 + SECTION AUTHORITY + example.com. 10 IN NS ns.example.com. + SECTION ADDITIONAL + ns.example.com. 10 IN A 1.2.3.4 + ENTRY_END +RANGE_END + +; Query with RD flag +STEP 0 QUERY +ENTRY_BEGIN + REPLY RD + SECTION QUESTION + 0ttl.example.com. IN A +ENTRY_END + +; Check that we get the NODATA (will be cached) +STEP 10 CHECK_ANSWER +ENTRY_BEGIN + MATCH all + REPLY QR RD RA NOERROR + SECTION QUESTION + 0ttl.example.com. IN A + SECTION AUTHORITY + example.com IN SOA ns.example.com dns.example.com 1 7200 3600 2419200 10 +ENTRY_END + +; Query again +STEP 20 QUERY +ENTRY_BEGIN + REPLY RD + SECTION QUESTION + 0ttl.example.com. IN A +ENTRY_END + +; Check that we get the cached NODATA +STEP 30 CHECK_ANSWER +ENTRY_BEGIN + MATCH all + REPLY QR RD RA NOERROR + SECTION QUESTION + 0ttl.example.com. IN A + SECTION AUTHORITY + example.com IN SOA ns.example.com dns.example.com 1 7200 3600 2419200 10 +ENTRY_END + +; Wait for the NXDOMAIN to expire +STEP 31 TIME_PASSES ELAPSE 32 + +; Query again +STEP 40 QUERY +ENTRY_BEGIN + REPLY RD + SECTION QUESTION + 0ttl.example.com. IN A +ENTRY_END + +; Check that we get the cached NODATA +STEP 50 CHECK_ANSWER +ENTRY_BEGIN + MATCH all + REPLY QR RD RA NOERROR + SECTION QUESTION + 0ttl.example.com. IN A + SECTION AUTHORITY + example.com IN SOA ns.example.com dns.example.com 1 7200 3600 2419200 10 +ENTRY_END + +; Query again +STEP 60 QUERY +ENTRY_BEGIN + REPLY RD + SECTION QUESTION + 0ttl.example.com. IN A +ENTRY_END + +; Check that we got the correct answer +STEP 70 CHECK_ANSWER +ENTRY_BEGIN + MATCH all ttl + REPLY QR RD RA NOERROR + SECTION QUESTION + 0ttl.example.com. IN A + SECTION ANSWER + 0ttl.example.com. 0 IN A 5.6.7.8 + SECTION AUTHORITY + example.com. 10 IN NS ns.example.com. + SECTION ADDITIONAL + ns.example.com. 10 IN A 1.2.3.4 +ENTRY_END + +SCENARIO_END diff --git a/testdata/serve_expired_0ttl_nxdomain.rpl b/testdata/serve_expired_0ttl_nxdomain.rpl new file mode 100644 index 000000000..55b7605ad --- /dev/null +++ b/testdata/serve_expired_0ttl_nxdomain.rpl @@ -0,0 +1,154 @@ +; config options +server: + module-config: "validator iterator" + qname-minimisation: "no" + minimal-responses: no + serve-expired: yes + log-servfail: yes + ede: yes + ede-serve-expired: yes + + +stub-zone: + name: "example.com" + stub-addr: 1.2.3.4 +CONFIG_END + +SCENARIO_BEGIN Test serve-expired with NXDOMAIN followed by 0 TTL +; Scenario overview: +; - query for 0ttl.example.com. IN A +; - answer from upstream is NXDOMAIN; will be cached for NORR_TTL(5) +; - check that the client gets the NXDOMAIN; also cached +; - query again right after the TTL expired +; - this time the server answers with a 0 TTL RRset +; - check that we get the correct answer + +; ns.example.com. +RANGE_BEGIN 0 20 + ADDRESS 1.2.3.4 + ; response to A query + ENTRY_BEGIN + MATCH opcode qtype qname + ADJUST copy_id + REPLY QR AA NXDOMAIN + SECTION QUESTION + 0ttl.example.com. IN A + SECTION AUTHORITY + example.com IN SOA ns.example.com dns.example.com 1 7200 3600 2419200 10 + ENTRY_END +RANGE_END + +; ns.example.com. +RANGE_BEGIN 30 100 + ADDRESS 1.2.3.4 + ENTRY_BEGIN + MATCH opcode qtype qname + ADJUST copy_id + REPLY QR NOERROR + SECTION QUESTION + example.com. 10 IN NS + SECTION ANSWER + example.com. 10 IN NS ns.example.com. + SECTION ADDITIONAL + ns.example.com. 10 IN A 1.2.3.4 + ENTRY_END + + ENTRY_BEGIN + MATCH opcode qtype qname + ADJUST copy_id + REPLY QR NOERROR + SECTION QUESTION + 0ttl.example.com. IN A + SECTION ANSWER + 0ttl.example.com. 0 IN A 5.6.7.8 + SECTION AUTHORITY + example.com. 10 IN NS ns.example.com. + SECTION ADDITIONAL + ns.example.com. 10 IN A 1.2.3.4 + ENTRY_END +RANGE_END + +; Query with RD flag +STEP 0 QUERY +ENTRY_BEGIN + REPLY RD + SECTION QUESTION + 0ttl.example.com. IN A +ENTRY_END + +; Check that we get the SERVFAIL (will be cached) +STEP 10 CHECK_ANSWER +ENTRY_BEGIN + MATCH all + REPLY QR RD RA NXDOMAIN + SECTION QUESTION + 0ttl.example.com. IN A + SECTION AUTHORITY + example.com IN SOA ns.example.com dns.example.com 1 7200 3600 2419200 10 +ENTRY_END + +; Query again +STEP 20 QUERY +ENTRY_BEGIN + REPLY RD + SECTION QUESTION + 0ttl.example.com. IN A +ENTRY_END + +; Check that we get the cached NXDOMAIN +STEP 30 CHECK_ANSWER +ENTRY_BEGIN + MATCH all + REPLY QR RD RA NXDOMAIN + SECTION QUESTION + 0ttl.example.com. IN A + SECTION AUTHORITY + example.com IN SOA ns.example.com dns.example.com 1 7200 3600 2419200 10 +ENTRY_END + +; Wait for the NXDOMAIN to expire +STEP 31 TIME_PASSES ELAPSE 32 + +; Query again +STEP 40 QUERY +ENTRY_BEGIN + REPLY RD + SECTION QUESTION + 0ttl.example.com. IN A +ENTRY_END + +; Check that we get the cached NXDOMAIN +STEP 50 CHECK_ANSWER +ENTRY_BEGIN + MATCH all + REPLY QR RD RA NXDOMAIN + SECTION QUESTION + 0ttl.example.com. IN A + SECTION AUTHORITY + example.com IN SOA ns.example.com dns.example.com 1 7200 3600 2419200 10 +ENTRY_END + +; Query again +STEP 60 QUERY +ENTRY_BEGIN + REPLY RD + SECTION QUESTION + 0ttl.example.com. IN A +ENTRY_END + +; Check that we got the correct answer +STEP 70 CHECK_ANSWER +ENTRY_BEGIN + MATCH all ttl + REPLY QR RD RA NOERROR + SECTION QUESTION + 0ttl.example.com. IN A + SECTION ANSWER + 0ttl.example.com. 0 IN A 5.6.7.8 + SECTION AUTHORITY + example.com. 10 IN NS ns.example.com. + SECTION ADDITIONAL + ns.example.com. 10 IN A 1.2.3.4 +ENTRY_END + +SCENARIO_END diff --git a/testdata/serve_expired_0ttl_servfail.rpl b/testdata/serve_expired_0ttl_servfail.rpl new file mode 100644 index 000000000..aad7aa8c9 --- /dev/null +++ b/testdata/serve_expired_0ttl_servfail.rpl @@ -0,0 +1,129 @@ +; config options +server: + module-config: "validator iterator" + qname-minimisation: "no" + minimal-responses: no + serve-expired: yes + log-servfail: yes + ede: yes + ede-serve-expired: yes + + +stub-zone: + name: "example.com" + stub-addr: 1.2.3.4 +CONFIG_END + +SCENARIO_BEGIN Test serve-expired with SERVFAIL followed by 0 TTL +; Scenario overview: +; - query for 0ttl.example.com. IN A +; - answer from upstream is SERVFAIL; will be cached for NORR_TTL(5) +; - check that the client gets the SERVFAIL; also cached +; - query again right after the TTL expired +; - this time the server answers with a 0 TTL RRset +; - check that we get the correct answer + +; ns.example.com. +RANGE_BEGIN 0 20 + ADDRESS 1.2.3.4 + ; response to A query + ENTRY_BEGIN + MATCH opcode qtype qname + ADJUST copy_id + REPLY QR AA SERVFAIL + SECTION QUESTION + 0ttl.example.com. IN A + ENTRY_END +RANGE_END + +; ns.example.com. +RANGE_BEGIN 30 100 + ADDRESS 1.2.3.4 + ENTRY_BEGIN + MATCH opcode qtype qname + ADJUST copy_id + REPLY QR NOERROR + SECTION QUESTION + example.com. 10 IN NS + SECTION ANSWER + example.com. 10 IN NS ns.example.com. + SECTION ADDITIONAL + ns.example.com. 10 IN A 1.2.3.4 + ENTRY_END + + ENTRY_BEGIN + MATCH opcode qtype qname + ADJUST copy_id + REPLY QR NOERROR + SECTION QUESTION + 0ttl.example.com. IN A + SECTION ANSWER + 0ttl.example.com. 0 IN A 5.6.7.8 + SECTION AUTHORITY + example.com. 10 IN NS ns.example.com. + SECTION ADDITIONAL + ns.example.com. 10 IN A 1.2.3.4 + ENTRY_END +RANGE_END + +; Query with RD flag +STEP 0 QUERY +ENTRY_BEGIN + REPLY RD + SECTION QUESTION + 0ttl.example.com. IN A +ENTRY_END + +; Check that we get the SERVFAIL (will be cached) +STEP 10 CHECK_ANSWER +ENTRY_BEGIN + MATCH all + REPLY QR RD RA SERVFAIL + SECTION QUESTION + 0ttl.example.com. IN A +ENTRY_END + +; Query again +STEP 20 QUERY +ENTRY_BEGIN + REPLY RD + SECTION QUESTION + 0ttl.example.com. IN A +ENTRY_END + +; Check that we get the cached SERVFAIL +STEP 30 CHECK_ANSWER +ENTRY_BEGIN + MATCH all + REPLY QR RD RA SERVFAIL + SECTION QUESTION + 0ttl.example.com. IN A +ENTRY_END + +; Wait for the SERVFAIL to expire +STEP 31 TIME_PASSES ELAPSE 32 + +; Query again +STEP 40 QUERY +ENTRY_BEGIN + REPLY RD + SECTION QUESTION + 0ttl.example.com. IN A +ENTRY_END + +; Check that we got the correct answer +STEP 50 CHECK_ANSWER +ENTRY_BEGIN + MATCH all ttl + REPLY QR RD RA NOERROR + SECTION QUESTION + 0ttl.example.com. IN A + SECTION ANSWER + 0ttl.example.com. 0 IN A 5.6.7.8 + SECTION AUTHORITY + example.com. 10 IN NS ns.example.com. + SECTION ADDITIONAL + ns.example.com. 10 IN A 1.2.3.4 +ENTRY_END + +SCENARIO_END From 1ac9b7548b5a5f62cbb05dfa85cdda16677986d3 Mon Sep 17 00:00:00 2001 From: Philip Homburg Date: Thu, 23 Mar 2023 15:15:54 +0100 Subject: [PATCH 3/4] Small fixes from Wouter's review --- services/cache/dns.c | 2 +- testdata/serve_expired_0ttl_nodata.rpl | 2 +- testdata/serve_expired_0ttl_nxdomain.rpl | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/services/cache/dns.c b/services/cache/dns.c index 1a4fbdd2f..9fbcb5f0f 100644 --- a/services/cache/dns.c +++ b/services/cache/dns.c @@ -178,7 +178,7 @@ dns_cache_store_msg(struct module_env* env, struct query_info* qinfo, * which could be useful for delegation information */ verbose(VERB_ALGO, "TTL 0: dropped msg from cache"); free(rep); - /* if the message is in then cache, remove that msg, + /* if the message is in the cache, remove that msg, * so that the TTL 0 response can be returned for future * responses (i.e. don't get answered from * cache, but instead go to recursion to get this TTL0 diff --git a/testdata/serve_expired_0ttl_nodata.rpl b/testdata/serve_expired_0ttl_nodata.rpl index e28e35255..45b51444b 100644 --- a/testdata/serve_expired_0ttl_nodata.rpl +++ b/testdata/serve_expired_0ttl_nodata.rpl @@ -17,7 +17,7 @@ CONFIG_END SCENARIO_BEGIN Test serve-expired with NXDOMAIN followed by 0 TTL ; Scenario overview: ; - query for 0ttl.example.com. IN A -; - answer from upstream is NODATA; will be cached for NORR_TTL(5) +; - answer from upstream is NODATA; will be cached for the SOA negative TTL. ; - check that the client gets the NODATA; also cached ; - query again right after the TTL expired ; - this time the server answers with a 0 TTL RRset diff --git a/testdata/serve_expired_0ttl_nxdomain.rpl b/testdata/serve_expired_0ttl_nxdomain.rpl index 55b7605ad..0fcde9f2d 100644 --- a/testdata/serve_expired_0ttl_nxdomain.rpl +++ b/testdata/serve_expired_0ttl_nxdomain.rpl @@ -17,7 +17,7 @@ CONFIG_END SCENARIO_BEGIN Test serve-expired with NXDOMAIN followed by 0 TTL ; Scenario overview: ; - query for 0ttl.example.com. IN A -; - answer from upstream is NXDOMAIN; will be cached for NORR_TTL(5) +; - answer from upstream is NXDOMAIN; will be cached for the SOA negative TTL. ; - check that the client gets the NXDOMAIN; also cached ; - query again right after the TTL expired ; - this time the server answers with a 0 TTL RRset From 1aa2c318e7f3720e54f401c0945a3093a5b7fbcb Mon Sep 17 00:00:00 2001 From: Philip Homburg Date: Wed, 26 Apr 2023 17:11:29 +0200 Subject: [PATCH 4/4] Remove msg_del_for_0ttl, call msg_cache_remove directly --- services/cache/dns.c | 23 ++--------------------- 1 file changed, 2 insertions(+), 21 deletions(-) diff --git a/services/cache/dns.c b/services/cache/dns.c index 9fbcb5f0f..11ea40b7f 100644 --- a/services/cache/dns.c +++ b/services/cache/dns.c @@ -132,26 +132,6 @@ msg_cache_remove(struct module_env* env, uint8_t* qname, size_t qnamelen, slabhash_remove(env->msg_cache, h, &k); } -/** remove servfail msg cache entry */ -static void -msg_del_for_0ttl(struct module_env* env, struct query_info* qinfo, - uint32_t flags) -{ - struct msgreply_entry* e; - /* see if there is an existing entry, and then remove it, so that - * lookups move from the cacheresponse stage to the recursionresponse - * stage */ - e = msg_cache_lookup(env, qinfo->qname, qinfo->qname_len, - qinfo->qtype, qinfo->qclass, flags, 0, 0); - if(!e) return; - /* we don't check for the ttl here, also expired entries - * are removed. If the user uses serve-expired, they would still be - * used to answer from cache */ - lock_rw_unlock(&e->entry.lock); - msg_cache_remove(env, qinfo->qname, qinfo->qname_len, qinfo->qtype, - qinfo->qclass, flags); -} - void dns_cache_store_msg(struct module_env* env, struct query_info* qinfo, hashvalue_type hash, struct reply_info* rep, time_t leeway, int pside, @@ -189,7 +169,8 @@ dns_cache_store_msg(struct module_env* env, struct query_info* qinfo, * - NODATA * - an older record that is expired * - an older record that did not yet expire */ - msg_del_for_0ttl(env, qinfo, flags); + msg_cache_remove(env, qinfo->qname, qinfo->qname_len, + qinfo->qtype, qinfo->qclass, flags); return; }