From ad4bab306c797088cc254d06c99bd41cbad9ee3b Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 19 Dec 2024 14:19:43 -0800 Subject: [PATCH 1/5] qpzone find() function could set foundname incorrectly when a requested name is found in the QP trie during a lookup, but its records have been marked as nonexistent by a previous deletion, then it's treated as a partial match, but the foundname could be left pointing to the original qname rather than the parent. this could lead to an assertion failure in query_findclosestnsec3(). --- lib/dns/qpzone.c | 20 +++++++++++++------- tests/dns/dbversion_test.c | 9 ++++++++- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/lib/dns/qpzone.c b/lib/dns/qpzone.c index fa3ac6ac44..875c39920f 100644 --- a/lib/dns/qpzone.c +++ b/lib/dns/qpzone.c @@ -3407,10 +3407,8 @@ find(dns_db_t *db, const dns_name_t *name, dns_dbversion_t *version, if (tresult != DNS_R_CONTINUE) { result = tresult; search.chain.len = i - 1; + dns_name_copy(&n->name, foundname); node = n; - if (foundname != NULL) { - dns_name_copy(&node->name, foundname); - } } } @@ -3678,12 +3676,20 @@ found: /* * We have an exact match for the name, but there are no * active rdatasets in the desired version. That means that - * this node doesn't exist in the desired version, and that - * we really have a partial match. + * this node doesn't exist in the desired version. + * If there's a node above this one, reassign the + * foundname to the parent and treat this as a partial + * match. */ if (!wild) { - NODE_UNLOCK(lock, &nlocktype); - goto partial_match; + unsigned int len = search.chain.len - 1; + if (len > 0) { + NODE_UNLOCK(lock, &nlocktype); + dns_qpchain_node(&search.chain, len - 1, NULL, + (void **)&node, NULL); + dns_name_copy(&node->name, foundname); + goto partial_match; + } } } diff --git a/tests/dns/dbversion_test.c b/tests/dns/dbversion_test.c index ac6ebca31d..690cca8036 100644 --- a/tests/dns/dbversion_test.c +++ b/tests/dns/dbversion_test.c @@ -170,7 +170,14 @@ ISC_RUN_TEST_IMPL(find) { dns_rdataset_init(&rdataset); res = dns_db_find(db1, dns_rootname, v1, dns_rdatatype_soa, 0, 0, NULL, name, &rdataset, NULL); - assert_int_equal(res, DNS_R_NXDOMAIN); + /* + * Note: in the QPzone database, the root node always exists, + * even if it's empty, so we would get DNS_R_NXRRSET from this + * query. In other databases (including the old RBTDB) the root + * node can be nonexistent, and the query would then return + * DNS_R_NXDOMAIN. Allow for both possibilities. + */ + assert_true(res == DNS_R_NXRRSET || res == DNS_R_NXDOMAIN); if (dns_rdataset_isassociated(&rdataset)) { dns_rdataset_disassociate(&rdataset); From 7b94c34965b8b3b8364082f25bd6c3b692048c5a Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Fri, 20 Dec 2024 16:39:49 -0800 Subject: [PATCH 2/5] add a regression test for record deletion test that there's no crash when querying for a newly-deleted node. (incidentally also renamed ns3/named.conf.in to ns3/named1.conf.in, because named2.conf.in does exist, and they should match.) --- .../nsec3/ns3/{named.conf.in => named1.conf.in} | 0 bin/tests/system/nsec3/ns3/named2-fips.conf.in | 10 ++++++++++ bin/tests/system/nsec3/ns3/setup.sh | 4 ++-- bin/tests/system/nsec3/setup.sh | 2 +- bin/tests/system/nsec3/tests.sh | 16 ++++++++++++++++ 5 files changed, 29 insertions(+), 3 deletions(-) rename bin/tests/system/nsec3/ns3/{named.conf.in => named1.conf.in} (100%) diff --git a/bin/tests/system/nsec3/ns3/named.conf.in b/bin/tests/system/nsec3/ns3/named1.conf.in similarity index 100% rename from bin/tests/system/nsec3/ns3/named.conf.in rename to bin/tests/system/nsec3/ns3/named1.conf.in diff --git a/bin/tests/system/nsec3/ns3/named2-fips.conf.in b/bin/tests/system/nsec3/ns3/named2-fips.conf.in index 8b42abbcce..6c7499d01f 100644 --- a/bin/tests/system/nsec3/ns3/named2-fips.conf.in +++ b/bin/tests/system/nsec3/ns3/named2-fips.conf.in @@ -145,3 +145,13 @@ zone "nsec3-inline-to-dynamic.kasp" { dnssec-policy "nsec3"; allow-update { any; }; }; + +/* + * This zone will have a node deleted. + */ +zone "nsec3-ent.kasp" { + type primary; + file "nsec3-ent.kasp.db"; + dnssec-policy "nsec3"; + inline-signing yes; +}; diff --git a/bin/tests/system/nsec3/ns3/setup.sh b/bin/tests/system/nsec3/ns3/setup.sh index 7db6d3910c..32ddf5e9c5 100644 --- a/bin/tests/system/nsec3/ns3/setup.sh +++ b/bin/tests/system/nsec3/ns3/setup.sh @@ -20,14 +20,14 @@ setup() { zone="$1" echo_i "setting up zone: $zone" zonefile="${zone}.db" - infile="${zone}.db.infile" cp template.db.in "$zonefile" } for zn in nsec-to-nsec3 nsec3 nsec3-other nsec3-change nsec3-to-nsec \ nsec3-to-optout nsec3-from-optout nsec3-dynamic \ nsec3-dynamic-change nsec3-dynamic-to-inline \ - nsec3-inline-to-dynamic nsec3-dynamic-update-inline; do + nsec3-inline-to-dynamic nsec3-dynamic-update-inline \ + nsec3-ent; do setup "${zn}.kasp" done diff --git a/bin/tests/system/nsec3/setup.sh b/bin/tests/system/nsec3/setup.sh index f995033e5f..56c3ac2eef 100644 --- a/bin/tests/system/nsec3/setup.sh +++ b/bin/tests/system/nsec3/setup.sh @@ -27,7 +27,7 @@ if [ $RSASHA1_SUPPORTED = 0 ]; then else copy_setports ns3/named-fips.conf.in ns3/named-fips.conf # includes named-fips.conf - cp ns3/named.conf.in ns3/named.conf + cp ns3/named1.conf.in ns3/named.conf fi ( cd ns3 diff --git a/bin/tests/system/nsec3/tests.sh b/bin/tests/system/nsec3/tests.sh index bae2279aa4..37a9faa849 100644 --- a/bin/tests/system/nsec3/tests.sh +++ b/bin/tests/system/nsec3/tests.sh @@ -584,5 +584,21 @@ set_key_default_values "KEY1" echo_i "check zone ${ZONE} after reload" check_nsec3 +# Zone: nsec3-ent.kasp (regression test for #5108) +n=$((n + 1)) +echo_i "check queries for newly empty names do not crash ($n)" +set_zone_policy "nsec3-ent.kasp" +set_server "ns3" "10.53.0.3" +# confirm the pre-existing name still exists +dig_with_opts +noquestion "@${SERVER}" c.$ZONE >"dig.out.$ZONE.test$n.1" || ret=1 +grep "c\.nsec3-ent\.kasp\..*IN.*A.*10\.0\.0\.3" "dig.out.$ZONE.test$n.1" >/dev/null || ret=1 +# remove a name, bump the SOA, reload, and try the query again +sed -e 's/1 *; serial/2/' -e '/^c/d' ns3/template.db.in >ns3/nsec3-ent.kasp.db +rndc_reload ns3 10.53.0.3 +dig_with_opts +noquestion "@${SERVER}" c.$ZONE >"dig.out.$ZONE.test$n.2" || ret=1 +grep "status: NXDOMAIN" "dig.out.$ZONE.test$n.2" >/dev/null || ret=1 +if [ "$ret" -ne 0 ]; then echo_i "failed"; fi +status=$((status + ret)) + echo_i "exit status: $status" [ $status -eq 0 ] || exit 1 From 3e367a23f95569a8e08561ff7424666073b5edb2 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Fri, 20 Dec 2024 17:25:10 -0800 Subject: [PATCH 3/5] add a regression test for a new ENT node this test adds a record with empty non-terminal nodes above it. this has also been observed to trigger the crash in NSEC3 zones. NOTE: the test currently fails, because while there is no crash, the query results are not as expected. when we add a node below an ENT, receive_secure_serial() gets DNS_R_PARTIALMATCH, and the signed zone is never updated. this is not a regression from fixing the crash bug; it's a separate inline-signing bug. --- .../system/nsec3/ns3/named2-fips.conf.in | 2 +- bin/tests/system/nsec3/tests.sh | 22 +++++++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/bin/tests/system/nsec3/ns3/named2-fips.conf.in b/bin/tests/system/nsec3/ns3/named2-fips.conf.in index 6c7499d01f..2c9a2b7e20 100644 --- a/bin/tests/system/nsec3/ns3/named2-fips.conf.in +++ b/bin/tests/system/nsec3/ns3/named2-fips.conf.in @@ -147,7 +147,7 @@ zone "nsec3-inline-to-dynamic.kasp" { }; /* - * This zone will have a node deleted. + * This zone will have an empty nonterminal node added and a node deleted. */ zone "nsec3-ent.kasp" { type primary; diff --git a/bin/tests/system/nsec3/tests.sh b/bin/tests/system/nsec3/tests.sh index 37a9faa849..757d181203 100644 --- a/bin/tests/system/nsec3/tests.sh +++ b/bin/tests/system/nsec3/tests.sh @@ -586,19 +586,37 @@ check_nsec3 # Zone: nsec3-ent.kasp (regression test for #5108) n=$((n + 1)) -echo_i "check queries for newly empty names do not crash ($n)" +echo_i "check query for newly empty name does not crash ($n)" set_zone_policy "nsec3-ent.kasp" set_server "ns3" "10.53.0.3" # confirm the pre-existing name still exists dig_with_opts +noquestion "@${SERVER}" c.$ZONE >"dig.out.$ZONE.test$n.1" || ret=1 grep "c\.nsec3-ent\.kasp\..*IN.*A.*10\.0\.0\.3" "dig.out.$ZONE.test$n.1" >/dev/null || ret=1 -# remove a name, bump the SOA, reload, and try the query again +# remove a name, bump the SOA, and reload sed -e 's/1 *; serial/2/' -e '/^c/d' ns3/template.db.in >ns3/nsec3-ent.kasp.db rndc_reload ns3 10.53.0.3 +# try the query again dig_with_opts +noquestion "@${SERVER}" c.$ZONE >"dig.out.$ZONE.test$n.2" || ret=1 grep "status: NXDOMAIN" "dig.out.$ZONE.test$n.2" >/dev/null || ret=1 if [ "$ret" -ne 0 ]; then echo_i "failed"; fi status=$((status + ret)) +n=$((n + 1)) +echo_i "check queries for new names below ENT do not crash ($n)" +set_zone_policy "nsec3-ent.kasp" +set_server "ns3" "10.53.0.3" +# confirm the ENT name does not exist yet +dig_with_opts +noquestion "@${SERVER}" x.y.z.$ZONE >"dig.out.$ZONE.test$n.1" || ret=1 +grep "status: NXDOMAIN" "dig.out.$ZONE.test$n.1" >/dev/null || ret=1 +# add a name with an ENT, bump the SOA, and reload +sed -e 's/1 *; serial/3/' ns3/template.db.in >ns3/nsec3-ent.kasp.db +echo "x.y.z A 10.0.0.4" >>ns3/nsec3-ent.kasp.db +rndc_reload ns3 10.53.0.3 +# try the query again +dig_with_opts +noquestion "@${SERVER}" x.y.z.$ZONE >"dig.out.$ZONE.test$n.2" || ret=1 +grep "x\.y\.z\.nsec3-ent\.kasp\..*IN.*A.*10\.0\.0\.4" "dig.out.$ZONE.test$n.2" >/dev/null || ret=1 +if [ "$ret" -ne 0 ]; then echo_i "failed"; fi +status=$((status + ret)) + echo_i "exit status: $status" [ $status -eq 0 ] || exit 1 From 71e1c91695bacb39c649bf79b73d808e31fc5d9e Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Fri, 20 Dec 2024 18:31:30 -0800 Subject: [PATCH 4/5] dns_nsec3_addnsec3() can fail when iterating back when adding a new NSEC3 record, dns_nsec3_addnsec3() uses a dbiterator to seek to the newly created node and then find its predecessor. dbiterators in the qpzone use snapshots, so changes to the database are not reflected in an already-existing iterator. consequently, when we add a new node, we have to create a new iterator before we can seek to it. --- lib/dns/nsec3.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/dns/nsec3.c b/lib/dns/nsec3.c index a4942b3a5b..e825ffea85 100644 --- a/lib/dns/nsec3.c +++ b/lib/dns/nsec3.c @@ -776,7 +776,7 @@ addnsec3: /* * Create the node if it doesn't exist and hold * a reference to it until we have added the NSEC3 - * or we discover we don't need to add make a change. + * or we discover we don't need to make a change. */ CHECK(dns_db_findnsec3node(db, hashname, true, &newnode)); result = dns_db_findrdataset(db, newnode, version, @@ -792,6 +792,17 @@ addnsec3: if (result != ISC_R_NOMORE) { goto failure; } + } else if (result == ISC_R_NOTFOUND) { + /* + * If we didn't find an NSEC3 in the node, + * then the node must have been newly created + * by dns_db_findnsec3node(). The iterator + * needs to be updated so we can seek for + * the node's predecessor. + */ + dns_dbiterator_destroy(&dbit); + CHECK(dns_db_createiterator(db, DNS_DB_NSEC3ONLY, + &dbit)); } /* From 232dac8cd596f91267c61c5743184c483e694b7a Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 8 Jan 2025 18:08:05 -0800 Subject: [PATCH 5/5] detect when closest-encloser name is too long there was a database bug in which dns_db_find() could get a partial match for the query name, but still set foundname to match the full query name. this triggered an assertion when query_addwildcardproof() assumed that foundname would be shorter. the database bug has been fixed, but in case it happens again, we can just copy the name instead of splitting it. we will also log a warning that the closest-encloser name was invalid. --- lib/ns/query.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/ns/query.c b/lib/ns/query.c index cc57bcf684..8464e782d9 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -11167,7 +11167,15 @@ again: * Add no qname proof. */ labels = dns_name_countlabels(cname) + 1; - if (dns_name_countlabels(name) == labels) { + if (labels > maxlabels) { + char namebuf[DNS_NAME_FORMATSIZE]; + dns_name_format(cname, namebuf, sizeof(namebuf)); + ns_client_log(qctx->client, DNS_LOGCATEGORY_DNSSEC, + NS_LOGMODULE_QUERY, ISC_LOG_WARNING, + "closest-encloser name too long: %s", + namebuf); + dns_name_copy(name, wname); + } else if (labels == maxlabels) { dns_name_copy(name, wname); } else { dns_name_split(name, labels, NULL, wname);