fix: usr: querying an NSEC3-signed zone for an empty record could trigger an assertion

A bug in the qpzone database could trigger a crash when querying for a deleted name, or a newly-added empty non-terminal name, in an NSEC3-signed zone. This has been fixed.

Closes #5108

Merge branch '5108-nsec3-empty-node' into 'main'

See merge request isc-projects/bind9!9928

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 an empty nonterminal node added and a node deleted.
+ */
+zone "nsec3-ent.kasp" {
+	type primary;
+	file "nsec3-ent.kasp.db";
+	dnssec-policy "nsec3";
+	inline-signing yes;
+};

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
 

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

bin/tests/system/nsec3/tests.sh

@@ -584,5 +584,39 @@ 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 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, 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

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));
 		}
 
 		/*

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;
+			}
 		}
 	}
 

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);

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);