Bug in the search of the RPZ summary db?

From Support ticket #14948:

It is believed that there may be a subtle bug in the search of the summary db.

in lib/dns/rpz.c, in the function dns_rpz_find_name, we see the following code:

nmnode = NULL;
result = dns_rbt_findnode(rpzs->rbt, trig_name, NULL, &nmnode, NULL,
			  DNS_RBTFIND_EMPTYDATA, NULL, NULL);
switch (result) {
case ISC_R_SUCCESS:
	nm_data = nmnode->data;
	if (nm_data != NULL) {
		if (rpz_type == DNS_RPZ_TYPE_QNAME)
			found_zbits = nm_data->set.qname;
		else
			found_zbits = nm_data->set.ns;
	}
	nmnode = nmnode->parent;
	/* fall thru */
case DNS_R_PARTIALMATCH:
	while (nmnode != NULL) {
		nm_data = nmnode->data;
		if (nm_data != NULL) {
			if (rpz_type == DNS_RPZ_TYPE_QNAME)
				found_zbits |= nm_data->wild.qname;
			else
				found_zbits |= nm_data->wild.ns;
		}
		nmnode = nmnode->parent;
	}
	break;

when we encounter a match or a partial match we run up the rbt trees and append zbit data for every parent node. It is important to note here that we are not just looking at the parents of the subtrees, but every parent node, so we can have a tree that looks like the following:

         .
         ||
         ||
        bar
       /    \
 com    foo

where the double lines are the boundaries of the subtrees, and in this example, if there is any (QNAME) rpz trigger for *.bar, that rule will also return a found_zbit flag for everything under 'com' causing the later processing to do unneeded lookups in the rpz zones themselves, lessening (and indeed in this case, mostly eliminating) the benefits of having a summary db

I wonder why this code checks all "parents" for a given node, rather than just the ancestors (in terms of sub- and super- domain relationships)? A co-worker points out that this should be possible by getting an "rbtnodechain" from dns_rbt_findnode() or by skipping unrelated "parents" in the traversal.

======

Before discussing a "solution", I'd first like to understand why the current code examines all rbt "parent"s for the found node:

dns_rpz_zbits_t
dns_rpz_find_name(dns_rpz_zones_t *rpzs, dns_rpz_type_t rpz_type,
		  dns_rpz_zbits_t zbits, dns_name_t *trig_name)
{
...
	result = dns_rbt_findnode(rpzs->rbt, trig_name, NULL, &nmnode, NULL,
				  DNS_RBTFIND_EMPTYDATA, NULL, NULL);
...
	case DNS_R_PARTIALMATCH:
		while (nmnode != NULL) {
			nm_data = nmnode->data;
			if (nm_data != NULL) {
				if (rpz_type == DNS_RPZ_TYPE_QNAME)
					found_zbits |= nm_data->wild.qname;
				else
					found_zbits |= nm_data->wild.ns;
			}
			nmnode = nmnode->parent;
		}
 ...

It didn't make sense to me, but if it was intentional for some reason that I can't see, a "solution" should probably respect that intent.

That said, assuming for now that it was rather misunderstanding of how rbt works than intentional behavior, one obvious solution is to only examine the ancestor nodes that are really superdomains of 'trig_name'.

But on thinking about it more, I'm now unsure whether that even makes sense. If you have the following in an RPZ:

*.example.com CNAME . bad.child.example.com CNAME .

and query for child.example.com, the current implementation (or even the "solution" version) of dns_rpz_find_name() causes a search in the RPZ, but it ends up with no-match in the RPZ, since child.example.com actually doesn't match *.example.com. Not matching looks sensible, but is it intentional to trigger the search in the RPZ in the first place?

If it's not even intentional, perhaps the simplest "solution" might be as simple as the patch pasted below (it's just a PoC patch and is likely to have pitfalls; don't report them to me:-).

One other thing: I also wonder why the current implementation keeps the content of the RPZ summary database even after it notices the zone expiration and unload the zone. It doesn't make much sense to me either, and a more sensible behavior seems to be to either

• also delete the corresponding portion of RPZ summary DB, or
• intentionally keep both the summary RPZ and expired zone RRs, and keeps using them at least for RPZ purposes

If either of this is done, the originally problem reported in this ticket wouldn't have happened, so in that sense it could also be a "solution".

diff --git a/lib/dns/rpz.c b/lib/dns/rpz.c
index 5aaab10d1d..ecb9729890 100644
--- a/lib/dns/rpz.c
+++ b/lib/dns/rpz.c
@@ -2619,18 +2619,14 @@ dns_rpz_find_name(dns_rpz_zones_t *rpzs, dns_rpz_type_t rpz_type,
 			else
 				found_zbits = nm_data->set.ns;
 		}
-		nmnode = nmnode->parent;
-		/* fall thru */
+		break;
 	case DNS_R_PARTIALMATCH:
-		while (nmnode != NULL) {
-			nm_data = nmnode->data;
-			if (nm_data != NULL) {
-				if (rpz_type == DNS_RPZ_TYPE_QNAME)
-					found_zbits |= nm_data->wild.qname;
-				else
-					found_zbits |= nm_data->wild.ns;
-			}
-			nmnode = nmnode->parent;
+		nm_data = nmnode->data;
+		if (nm_data != NULL) {
+			if (rpz_type == DNS_RPZ_TYPE_QNAME)
+				found_zbits |= nm_data->wild.qname;
+			else
+				found_zbits |= nm_data->wild.ns;
 		}
 		break;