Fix RPZ radix tree search() for CLIENT-IP triggers (#39481)

CHANGES

@@ -1,3 +1,8 @@
+4122.	[bug]		The server could match a shorter prefix than what was
+			available in CLIENT-IP policy triggers, and so, an
+			unexpected action could be taken. This has been
+			corrected. [RT #39481]
+
 4121.	[bug]		When updating a response-policy zone via AXFR,
 			summary data about other policy zones could fall
 			out of sync. Ultimately this could trigger an

bin/named/query.c

@@ -5166,6 +5166,7 @@ rpz_rewrite(ns_client_t *client, dns_rdatatype_t qtype,
 		st->m.type = DNS_RPZ_TYPE_BAD;
 		st->m.policy = DNS_RPZ_POLICY_MISS;
 		st->m.ttl = ~0;
+		st->m.prefix = 0;
 		memset(&st->r, 0, sizeof(st->r));
 		memset(&st->q, 0, sizeof(st->q));
 		dns_fixedname_init(&st->_p_namef);

bin/tests/system/rpzrecurse/ns2/db.clientip1

@@ -0,0 +1,6 @@
+$TTL 60
+@ IN SOA root.ns ns 1996072700 3600 1800 86400 60
+     NS ns
+ns A 127.0.0.1
+32.4.0.53.10.rpz-client-ip A 10.53.0.2
+24.0.0.53.10.rpz-client-ip A 10.53.0.1

bin/tests/system/rpzrecurse/ns2/db.clientip2

@@ -0,0 +1,5 @@
+$TTL 60
+@ IN SOA root.ns ns 1996072700 3600 1800 86400 60
+     NS ns
+ns A 127.0.0.1
+24.0.0.53.10.rpz-client-ip A 10.53.0.3

bin/tests/system/rpzrecurse/ns2/named.clientip.conf

@@ -0,0 +1,19 @@
+# common configuration
+include "named.conf.header";
+
+view "recursive" {
+    zone "." {
+        type hint;
+        file "root.hint";
+    };
+
+    # policy configuration to be tested
+    response-policy {
+        zone "clientip1";
+        zone "clientip2";
+    } qname-wait-recurse no;
+
+    # policy zones to be tested
+    zone "clientip1" { type master; file "db.clientip1"; };
+    zone "clientip2" { type master; file "db.clientip2"; };
+};

bin/tests/system/rpzrecurse/tests.sh

@@ -231,4 +231,18 @@ for n in 1 2 3 4 5 6 7 8 9; do
     }
 done
 
+# Check CLIENT-IP behavior
+t=`expr $t + 1`
+echo "I:testing CLIENT-IP behavior (${t})"
+run_server clientip
+$DIG $DIGOPTS l2.l1.l0 a @10.53.0.2 -p 5300 -b 10.53.0.4 > dig.out.${t}
+grep "status: NOERROR" dig.out.${t} > /dev/null 2>&1 || {
+    echo "I:test $t failed: query failed"
+    status=1
+}
+grep "^l2.l1.l0.[[:space:]]*[0-9]*[[:space:]]*IN[[:space:]]*A[[:space:]]*10.53.0.2" dig.out.${t} > /dev/null 2>&1 || {
+    echo "I:test $t failed: didn't get expected answer"
+    status=1
+}
+
 exit $status

doc/arm/notes.xml

@@ -652,6 +652,14 @@
 	    [RT #39567]
 	  </para>
 	</listitem>
+	<listitem>
+	  <para>
+	    The server could match a shorter prefix than what was
+	    available in CLIENT-IP policy triggers, and so, an
+	    unexpected action could be taken. This has been
+	    corrected. [RT #39481]
+	  </para>
+	</listitem>
       </itemizedlist>
     </itemizedlist>
   </sect2>

lib/dns/rpz.c

@@ -827,7 +827,7 @@ name2ipkey(int log_level,
 	if (--ip_labels == 4 && !strchr(cp, 'z')) {
 		/*
 		 * Convert an IPv4 address
-		 * from the form "prefix.w.z.y.x"
+		 * from the form "prefix.z.y.x.w"
 		 */
 		if (prefix_num > 32U) {
 			badname(log_level, src_name,
@@ -911,6 +911,12 @@ name2ipkey(int log_level,
 		prefix += DNS_RPZ_CIDR_WORD_BITS;
 	}
 
+	/*
+	 * XXXMUKS: Should the following check be enabled in a
+	 * production build?  It can be expensive for large IP zones
+	 * from 3rd parties.
+	 */
+
 	/*
 	 * Convert the address back to a canonical domain name
 	 * to ensure that the original name is in canonical form.
@@ -1093,7 +1099,7 @@ search(dns_rpz_zones_t *rpzs,
 			child->set.ip |= tgt_set->ip;
 			child->set.nsip |= tgt_set->nsip;
 			set_sum_pair(child);
-			*found = cur;
+			*found = child;
 			return (ISC_R_SUCCESS);
 		}
 
@@ -1186,8 +1192,8 @@ search(dns_rpz_zones_t *rpzs,
 				 */
 				find_result = DNS_R_PARTIALMATCH;
 				*found = cur;
-				set.client_ip = trim_zbits(set.ip,
+				set.client_ip = trim_zbits(set.client_ip,
-							cur->set.client_ip);
+							   cur->set.client_ip);
 				set.ip = trim_zbits(set.ip,
 						    cur->set.ip);
 				set.nsip = trim_zbits(set.nsip,