From fad9b3771f1ca231eb3fcf0a72c5fdaa52f25198 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 19 Feb 2025 17:51:53 -0800 Subject: [PATCH 1/2] Check whether a rejected rrset is different Add a new dns_rdataset_equals() function to check whether two rdatasets are equal in DNSSEC terms. When an rdataset being cached is rejected because its trust level is lower than the existing rdataset, we now check to see whether the rejected data was identical to the existing data. This allows us to cache a potentially useful RRSIG when handling CD=1 queries, while still rejecting RRSIGs that would definitely have resulted in a validation failure. (cherry picked from commit 6aba56ae89cde535fcc6fbee0366c843cdf47845) --- lib/dns/include/dns/rdataset.h | 12 ++++++++++++ lib/dns/rdataset.c | 15 +++++++++++++++ lib/dns/rdataslab.c | 19 +++++++++++++++++++ lib/dns/resolver.c | 34 ++++++++++++++++++++++------------ 4 files changed, 68 insertions(+), 12 deletions(-) diff --git a/lib/dns/include/dns/rdataset.h b/lib/dns/include/dns/rdataset.h index 3c55717144..e81e195f90 100644 --- a/lib/dns/include/dns/rdataset.h +++ b/lib/dns/include/dns/rdataset.h @@ -101,6 +101,8 @@ typedef struct dns_rdatasetmethods { void (*getownercase)(const dns_rdataset_t *rdataset, dns_name_t *name); isc_result_t (*addglue)(dns_rdataset_t *rdataset, dns_dbversion_t *version, dns_message_t *msg); + bool (*equals)(const dns_rdataset_t *rdataset1, + const dns_rdataset_t *rdataset2); } dns_rdatasetmethods_t; #define DNS_RDATASET_MAGIC ISC_MAGIC('D', 'N', 'S', 'R') @@ -696,4 +698,14 @@ dns_trust_totext(dns_trust_t trust); * Display trust in textual form. */ +bool +dns_rdataset_equals(const dns_rdataset_t *rdataset1, + const dns_rdataset_t *rdataset2); +/*%< + * Returns true if the rdata in the rdataset is equal. + * + * Requires: + * \li 'rdataset1' is a valid rdataset. + * \li 'rdataset2' is a valid rdataset. + */ ISC_LANG_ENDDECLS diff --git a/lib/dns/rdataset.c b/lib/dns/rdataset.c index 1a6dc4d551..123cceb3ef 100644 --- a/lib/dns/rdataset.c +++ b/lib/dns/rdataset.c @@ -676,3 +676,18 @@ dns_rdataset_trimttl(dns_rdataset_t *rdataset, dns_rdataset_t *sigrdataset, rdataset->ttl = ttl; sigrdataset->ttl = ttl; } + +bool +dns_rdataset_equals(const dns_rdataset_t *rdataset1, + const dns_rdataset_t *rdataset2) { + REQUIRE(DNS_RDATASET_VALID(rdataset1)); + REQUIRE(DNS_RDATASET_VALID(rdataset2)); + + if (rdataset1->methods->equals != NULL && + rdataset1->methods->equals == rdataset2->methods->equals) + { + return (rdataset1->methods->equals)(rdataset1, rdataset2); + } + + return false; +} diff --git a/lib/dns/rdataslab.c b/lib/dns/rdataslab.c index 3da416e2ef..74de47ab90 100644 --- a/lib/dns/rdataslab.c +++ b/lib/dns/rdataslab.c @@ -125,6 +125,9 @@ static void rdataset_setownercase(dns_rdataset_t *rdataset, const dns_name_t *name); static void rdataset_getownercase(const dns_rdataset_t *rdataset, dns_name_t *name); +static bool +rdataset_equals(const dns_rdataset_t *rdataset1, + const dns_rdataset_t *rdataset2); /*% Note: the "const void *" are just to make qsort happy. */ static int @@ -1156,6 +1159,7 @@ dns_rdatasetmethods_t dns_rdataslab_rdatasetmethods = { .clearprefetch = rdataset_clearprefetch, .setownercase = rdataset_setownercase, .getownercase = rdataset_getownercase, + .equals = rdataset_equals, }; /* Fixed RRSet helper macros */ @@ -1474,3 +1478,18 @@ rdataset_getownercase(const dns_rdataset_t *rdataset, dns_name_t *name) { unlock: dns_db_unlocknode(header->db, header->node, isc_rwlocktype_read); } + +static bool +rdataset_equals(const dns_rdataset_t *rdataset1, + const dns_rdataset_t *rdataset2) { + if (rdataset1->rdclass != rdataset2->rdclass || + rdataset1->type != rdataset2->type) + { + return false; + } + + unsigned char *header1 = rdataset1->slab.raw - sizeof(dns_slabheader_t); + unsigned char *header2 = rdataset2->slab.raw - sizeof(dns_slabheader_t); + return dns_rdataslab_equalx(header1, header2, sizeof(dns_slabheader_t), + rdataset1->rdclass, rdataset2->type); +} diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 0089b69aa3..b6ba7a5a3f 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -6096,33 +6096,43 @@ cache_name(fetchctx_t *fctx, dns_name_t *name, dns_message_t *message, if (result == DNS_R_UNCHANGED) { result = ISC_R_SUCCESS; if (!need_validation && - ardataset != NULL && - NEGATIVE(ardataset)) + ardataset != NULL) { /* * The answer in the * cache is better than - * the answer we found, - * and is a negative - * cache entry, so we + * the answer we found. + * If it's a negative + * cache entry, we * must set eresult * appropriately. */ if (NXDOMAIN(ardataset)) { eresult = DNS_R_NCACHENXDOMAIN; - } else { + } else if (NEGATIVE(ardataset)) + { eresult = DNS_R_NCACHENXRRSET; } + /* - * We have a negative - * response from the - * cache so don't - * attempt to add the - * RRSIG rrset. + * The cache wasn't updated + * because something was + * already there. If the + * data was the same as what + * we were trying to add, + * then sigrdataset might + * still be useful. If + * not, move on. */ - continue; + if (sigrdataset != NULL && + !dns_rdataset_equals( + rdataset, + addedrdataset)) + { + continue; + } } } if (result != ISC_R_SUCCESS) { From a6a75f82625c86614443697dcf08c261c1bdb8e8 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Mon, 27 Jan 2025 21:44:51 -0800 Subject: [PATCH 2/2] add a test with an inconsistent NS RRset add a zone with different NS RRsets in the parent and child, and test resolver and forwarder behavior with and without +CD. (cherry picked from commit e4652a0444a514773686e75752ad5c65daa753d5) --- bin/tests/system/dnssec/ns1/root.db.in | 2 ++ bin/tests/system/dnssec/ns1/sign.sh | 1 + .../system/dnssec/ns2/inconsistent.db.in | 24 +++++++++++++++++++ bin/tests/system/dnssec/ns2/named.conf.in | 5 ++++ bin/tests/system/dnssec/ns2/sign.sh | 11 +++++++++ bin/tests/system/dnssec/ns4/named3.conf.in | 2 ++ bin/tests/system/dnssec/ns9/named.conf.in | 1 + bin/tests/system/dnssec/tests.sh | 16 +++++++++++++ bin/tests/system/dnssec/tests_sh_dnssec.py | 1 + 9 files changed, 63 insertions(+) create mode 100644 bin/tests/system/dnssec/ns2/inconsistent.db.in diff --git a/bin/tests/system/dnssec/ns1/root.db.in b/bin/tests/system/dnssec/ns1/root.db.in index d76c89f455..52d88ab500 100644 --- a/bin/tests/system/dnssec/ns1/root.db.in +++ b/bin/tests/system/dnssec/ns1/root.db.in @@ -41,3 +41,5 @@ dnskey-rrsigs-stripped. NS ns2.dnskey-rrsigs-stripped. ns2.dnskey-rrsigs-stripped. A 10.53.0.2 ds-rrsigs-stripped. NS ns2.ds-rrsigs-stripped. ns2.ds-rrsigs-stripped. A 10.53.0.2 +inconsistent. NS ns2.inconsistent. +ns2.inconsistent. A 10.53.0.2 diff --git a/bin/tests/system/dnssec/ns1/sign.sh b/bin/tests/system/dnssec/ns1/sign.sh index d5e4e5ff53..62331d6cdc 100644 --- a/bin/tests/system/dnssec/ns1/sign.sh +++ b/bin/tests/system/dnssec/ns1/sign.sh @@ -32,6 +32,7 @@ cp "../ns2/dsset-too-many-iterations." . cp "../ns2/dsset-lazy-ksk." . cp "../ns2/dsset-dnskey-rrsigs-stripped." . cp "../ns2/dsset-ds-rrsigs-stripped." . +cp "../ns2/dsset-inconsistent." . grep "$DEFAULT_ALGORITHM_NUMBER [12] " "../ns2/dsset-algroll." >"dsset-algroll." cp "../ns6/dsset-optout-tld." . diff --git a/bin/tests/system/dnssec/ns2/inconsistent.db.in b/bin/tests/system/dnssec/ns2/inconsistent.db.in new file mode 100644 index 0000000000..0f9c211544 --- /dev/null +++ b/bin/tests/system/dnssec/ns2/inconsistent.db.in @@ -0,0 +1,24 @@ +; Copyright (C) Internet Systems Consortium, Inc. ("ISC") +; +; SPDX-License-Identifier: MPL-2.0 +; +; This Source Code Form is subject to the terms of the Mozilla Public +; License, v. 2.0. If a copy of the MPL was not distributed with this +; file, you can obtain one at https://mozilla.org/MPL/2.0/. +; +; See the COPYRIGHT file distributed with this work for additional +; information regarding copyright ownership. + +$TTL 300 ; 5 minutes +@ IN SOA ns2.example. . ( + 2010042407 ; serial + 20 ; refresh (20 seconds) + 20 ; retry (20 seconds) + 1814400 ; expire (3 weeks) + 3600 ; minimum (1 hour) + ) + NS ns2.example. + NS ns3.example. + A 10.53.0.1 +ns2 A 10.53.0.2 +ns3 A 10.53.0.3 diff --git a/bin/tests/system/dnssec/ns2/named.conf.in b/bin/tests/system/dnssec/ns2/named.conf.in index 1d3274ad71..1eb3b592ed 100644 --- a/bin/tests/system/dnssec/ns2/named.conf.in +++ b/bin/tests/system/dnssec/ns2/named.conf.in @@ -207,6 +207,11 @@ zone "too-many-iterations" { file "too-many-iterations.db.signed"; }; +zone "inconsistent" { + type primary; + file "inconsistent.db.signed"; +}; + zone "lazy-ksk" { type primary; file "lazy-ksk.db"; diff --git a/bin/tests/system/dnssec/ns2/sign.sh b/bin/tests/system/dnssec/ns2/sign.sh index 3be827090f..c5d9ddbb8f 100644 --- a/bin/tests/system/dnssec/ns2/sign.sh +++ b/bin/tests/system/dnssec/ns2/sign.sh @@ -399,3 +399,14 @@ cat "$infile" "$ksk.key" "$zsk.key" >"$zonefile" | awk '$4 == "SOA" { $7 = $7 + 1; print; next } { print }' >"$zonefile.next" "$SIGNER" -g -o "$zone" -f "$zonefile.next" "$zonefile.next" >/dev/null 2>&1 cp "$zonefile.stripped" "$zonefile.signed" + +# +# Inconsistent NS RRset between parent and child +# +zone=inconsistent +infile=inconsistent.db.in +zonefile=inconsistent.db +key1=$("$KEYGEN" -q -a "$DEFAULT_ALGORITHM" -b "$DEFAULT_BITS" -n zone -f KSK "$zone") +key2=$("$KEYGEN" -q -a "$DEFAULT_ALGORITHM" -b "$DEFAULT_BITS" -n zone "$zone") +cat "$infile" "$key1.key" "$key2.key" >"$zonefile" +"$SIGNER" -3 - -g -o "$zone" "$zonefile" >/dev/null 2>&1 diff --git a/bin/tests/system/dnssec/ns4/named3.conf.in b/bin/tests/system/dnssec/ns4/named3.conf.in index 21fb38db9d..d6a44c799d 100644 --- a/bin/tests/system/dnssec/ns4/named3.conf.in +++ b/bin/tests/system/dnssec/ns4/named3.conf.in @@ -26,6 +26,8 @@ options { bindkeys-file "managed.conf"; dnssec-accept-expired yes; minimal-responses no; + servfail-ttl 0; + disable-algorithms "digest-alg-unsupported.example." { ECDSAP384SHA384; }; disable-ds-digests "digest-alg-unsupported.example." { "SHA384"; "SHA-384";}; disable-ds-digests "ds-unsupported.example." { "SHA256"; "SHA-256"; "SHA384"; "SHA-384"; }; diff --git a/bin/tests/system/dnssec/ns9/named.conf.in b/bin/tests/system/dnssec/ns9/named.conf.in index 06530a08c1..cdbe7ec8ea 100644 --- a/bin/tests/system/dnssec/ns9/named.conf.in +++ b/bin/tests/system/dnssec/ns9/named.conf.in @@ -25,6 +25,7 @@ options { dnssec-validation yes; forward only; forwarders { 10.53.0.4; }; + servfail-ttl 0; }; key rndc_key { diff --git a/bin/tests/system/dnssec/tests.sh b/bin/tests/system/dnssec/tests.sh index da8e7fe36c..0d10115628 100644 --- a/bin/tests/system/dnssec/tests.sh +++ b/bin/tests/system/dnssec/tests.sh @@ -4616,5 +4616,21 @@ n=$((n + 1)) if [ "$ret" -ne 0 ]; then echo_i "failed"; fi status=$((status + ret)) +echo_i "checking validator behavior with mismatching NS ($n)" +ret=0 +rndccmd 10.53.0.4 flush 2>&1 | sed 's/^/ns4 /' | cat_i +$DIG +tcp +cd -p "$PORT" -t ns inconsistent @10.53.0.4 >dig.out.ns4.test$n.1 || ret=1 +grep "ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 2" dig.out.ns4.test$n.1 >/dev/null || ret=1 +grep "flags:.*ad.*QUERY" dig.out.ns4.test$n.1 >/dev/null && ret=1 +$DIG +tcp +cd +dnssec -p "$PORT" -t ns inconsistent @10.53.0.4 >dig.out.ns4.test$n.2 || ret=1 +grep "ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 2" dig.out.ns4.test$n.2 >/dev/null || ret=1 +grep "flags:.*ad.*QUERY" dig.out.ns4.test$n.2 >/dev/null && ret=1 +$DIG +tcp +dnssec -p "$PORT" -t ns inconsistent @10.53.0.4 >dig.out.ns4.test$n.3 || ret=1 +grep "ANSWER: 3, AUTHORITY: 0, ADDITIONAL: 1" dig.out.ns4.test$n.3 >/dev/null || ret=1 +grep "flags:.*ad.*QUERY" dig.out.ns4.test$n.3 >/dev/null || ret=1 +n=$((n + 1)) +if [ "$ret" -ne 0 ]; then echo_i "failed"; fi +status=$((status + ret)) + echo_i "exit status: $status" [ $status -eq 0 ] || exit 1 diff --git a/bin/tests/system/dnssec/tests_sh_dnssec.py b/bin/tests/system/dnssec/tests_sh_dnssec.py index 2bad6653a9..c76bc9f009 100644 --- a/bin/tests/system/dnssec/tests_sh_dnssec.py +++ b/bin/tests/system/dnssec/tests_sh_dnssec.py @@ -66,6 +66,7 @@ pytestmark = pytest.mark.extra_artifacts( "ns2/settime.out.updatecheck-kskonly.secure.zsk", "ns2/single-nsec3.db", "ns2/too-many-iterations.db", + "ns2/inconsistent.db", "ns2/trusted.db", "ns2/updatecheck-kskonly.secure.ksk.id", "ns2/updatecheck-kskonly.secure.ksk.key",