IPv6 addresses could match IPv4 ACL entries and vice versa. [RT #17462]

CHANGES

@@ -1,3 +1,6 @@
+2311.   [bug]           IPv6 addresses could match IPv4 ACL entries and
+                        vice versa. [RT #17462]
+
 2310	[bug]		dig, host, nslookup: flush stdout before emitting
 			debug/fatal messages.  [RT #17501]
 

bin/tests/system/acl/ns2/named2.conf

@@ -14,7 +14,7 @@
  * PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: named2.conf,v 1.2 2008/01/10 01:10:01 marka Exp $ */
+/* $Id: named2.conf,v 1.3 2008/01/21 20:38:54 each Exp $ */
 
 controls { /* empty */ };
 
@@ -57,5 +57,9 @@ zone "example" {
 zone "tsigzone" {
 	type master;
 	file "tsigzone.db";
-        allow-transfer { !10/8; key one; };
+	/*
+	 * 0a00::/8 and 10/8 are the same bits, but different address
+	 * families.  This should *not* match IPv4 queries from 10.*.
+	 */
+	allow-transfer { 0a00::/8; !10/8; key one; };
 };

lib/dns/acl.c

@@ -15,7 +15,7 @@
  * PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: acl.c,v 1.40 2008/01/18 23:46:58 tbox Exp $ */
+/* $Id: acl.c,v 1.41 2008/01/21 20:38:54 each Exp $ */
 
 /*! \file */
 
@@ -186,7 +186,7 @@ dns_acl_match(const isc_netaddr_t *reqaddr,
 	      int *match,
 	      const dns_aclelement_t **matchelt)
 {
-	isc_uint16_t bitlen;
+	isc_uint16_t bitlen, family;
 	isc_prefix_t pfx;
 	isc_radix_node_t *node;
 	const isc_netaddr_t *addr;
@@ -208,7 +208,8 @@ dns_acl_match(const isc_netaddr_t *reqaddr,
 	}
 
 	/* Always match with host addresses. */
-	bitlen = reqaddr->family == AF_INET6 ? 128 : 32;
+	family = reqaddr->family;
+	bitlen = family == AF_INET6 ? 128 : 32;
 	NETADDR_TO_PREFIX_T(addr, pfx, bitlen);
 
 	/* Assume no match. */
@@ -219,8 +220,8 @@ dns_acl_match(const isc_netaddr_t *reqaddr,
 
 	/* Found a match. */
 	if (result == ISC_R_SUCCESS && node != NULL) {
-		match_num = node->node_num;
+		match_num = node->node_num[ISC_IS6(family)];
-		if (*(isc_boolean_t *) node->data == ISC_TRUE)
+		if (*(isc_boolean_t *) node->data[ISC_IS6(family)] == ISC_TRUE)
 			*match = match_num;
 		else
 			*match = -match_num;
@@ -484,24 +485,29 @@ initialize_action(void) {
  * insecure.
  */
 static void
-is_insecure(isc_prefix_t *prefix, void *data) {
+is_insecure(isc_prefix_t *prefix, void **data) {
-	isc_boolean_t secure = * (isc_boolean_t *)data;
+	isc_boolean_t secure;
+	int bitlen, family;
+
+	/* Bitlen 0 means "any" or "none", which is always treated as IPv4 */
+	bitlen = prefix->bitlen;
+	family = bitlen ? prefix->family : AF_INET;
   
 	/* Negated entries are always secure. */
+	secure = * (isc_boolean_t *)data[ISC_IS6(family)];
 	if (!secure) {
 		return; 
 	}
   
 	/* If loopback prefix found, return */
-	switch (prefix->family) {
+	switch (family) {
 	case AF_INET:
-		if (prefix->bitlen == 32 &&
+		if (bitlen == 32 &&
 		    htonl(prefix->add.sin.s_addr) == INADDR_LOOPBACK)
 			return;
 		break;
 	case AF_INET6:
-		if (prefix->bitlen == 128 &&
+		if (bitlen == 128 && IN6_IS_ADDR_LOOPBACK(&prefix->add.sin6))
-		    IN6_IS_ADDR_LOOPBACK(&prefix->add.sin6))
 			return;
 		break;
 	default:
@@ -509,7 +515,7 @@ is_insecure(isc_prefix_t *prefix, void *data) {
 	}
 
 	/* Non-negated, non-loopback */
-	insecure_prefix_found = ISC_TRUE;
+	insecure_prefix_found = ISC_TRUE;	/* LOCKED */
 	return;
 }
 

lib/dns/iptable.c

@@ -14,7 +14,7 @@
  * PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: iptable.c,v 1.8 2008/01/18 23:46:58 tbox Exp $ */
+/* $Id: iptable.c,v 1.9 2008/01/21 20:38:54 each Exp $ */
 
 #include <isc/mem.h>
 #include <isc/radix.h>
@@ -63,24 +63,27 @@ dns_iptable_addprefix(dns_iptable_t *tab, isc_netaddr_t *addr,
 	isc_result_t result;
 	isc_prefix_t pfx;
 	isc_radix_node_t *node;
+	int family;
 
 	INSIST(DNS_IPTABLE_VALID(tab));
 	INSIST(tab->radix);
-	INSIST(bitlen <= 32 || (addr->family == AF_INET6 && bitlen <= 128));
 
 	NETADDR_TO_PREFIX_T(addr, pfx, bitlen);
 
+	/* Bitlen 0 means "any" or "none", which is always treated as IPv4 */
+	family = bitlen ? pfx.family : AF_INET;
+
 	result = isc_radix_insert(tab->radix, &node, NULL, &pfx);
 
 	if (result != ISC_R_SUCCESS)
 		return(result);
 
 	/* If the node already contains data, don't overwrite it */
-	if (node->data == NULL) {
+	if (node->data[ISC_IS6(family)] == NULL) {
 		if (pos)
-			node->data = &dns_iptable_pos;
+			node->data[ISC_IS6(family)] = &dns_iptable_pos;
 		else
-			node->data = &dns_iptable_neg;
+			node->data[ISC_IS6(family)] = &dns_iptable_neg;
 	}
 
 	return (ISC_R_SUCCESS);
@@ -110,15 +113,24 @@ dns_iptable_merge(dns_iptable_t *tab, dns_iptable_t *source, isc_boolean_t pos)
 		 * could be a security risk.  To prevent this, we
 		 * just leave the negative nodes negative.
 		 */
-		if (!pos &&
+		if (!pos) {
-		    node->data &&
+			if (node->data[0] &&
-		    *(isc_boolean_t *) node->data == ISC_TRUE)
+			    *(isc_boolean_t *) node->data[0] == ISC_TRUE)
-			new_node->data = &dns_iptable_neg;
+				new_node->data[0] = &dns_iptable_neg;
 			else
-			new_node->data = node->data;
+				new_node->data[0] = node->data[0];
 
-		if (node->node_num > max_node)
+			if (node->data[1] &&
-			max_node = node->node_num;
+			    *(isc_boolean_t *) node->data[1] == ISC_TRUE)
+				new_node->data[1] = &dns_iptable_neg;
+			else
+				new_node->data[1] = node->data[0];
+		}
+
+		if (node->node_num[0] > max_node)
+			max_node = node->node_num[0];
+		if (node->node_num[1] > max_node)
+			max_node = node->node_num[1];
 	} RADIX_WALK_END;
 
 	tab->radix->num_added_node += max_node;

lib/isc/include/isc/radix.h

@@ -14,7 +14,7 @@
  * PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: radix.h,v 1.8 2008/01/18 23:46:58 tbox Exp $ */
+/* $Id: radix.h,v 1.9 2008/01/21 20:38:54 each Exp $ */
 
 /*
  * This source was adapted from MRT's RCS Ids:
@@ -62,7 +62,7 @@ typedef struct isc_prefix {
 } isc_prefix_t;
 
 typedef void (*isc_radix_destroyfunc_t)(void *);
-typedef void (*isc_radix_processfunc_t)(isc_prefix_t *, void *);
+typedef void (*isc_radix_processfunc_t)(isc_prefix_t *, void **);
 
 #define isc_prefix_tochar(prefix) ((char *)&(prefix)->add.sin)
 #define isc_prefix_touchar(prefix) ((u_char *)&(prefix)->add.sin)
@@ -72,19 +72,28 @@ typedef void (*isc_radix_processfunc_t)(isc_prefix_t *, void *);
 /*
  * We need "first match" when we search the radix tree to preserve
  * compatibility with the existing ACL implementation. Radix trees
- * naturally lend themselves to "best match". In order to get "first
+ * naturally lend themselves to "best match". In order to get "first match"
- * match" behavior, we remember the entries are added to the tree,
+ * behavior, we keep track of the order in which entries are added to the
- * and when a search is made, we find all matching entries, and return
+ * tree--and when a search is made, we find all matching entries, and
- * the one that was added first.
+ * return the one that was added first.
+ *
+ * An IPv4 prefix and an IPv6 prefix may share a radix tree node if they
+ * have the same length and bit pattern (e.g., 127/8 and 7f::/8).  To
+ * disambiguate between them, node_num and data are two-element arrays;
+ * node_num[0] and data[0] are used for IPv4 addresses, node_num[1]
+ * and data[1] for IPv6 addresses.  The only exception is a prefix of
+ * 0/0 (aka "any" or "none"), which is always stored as IPv4 but matches
+ * IPv6 addresses too.
  */
   
+#define ISC_IS6(family) ((family) == AF_INET6 ? 1 : 0)
 typedef struct isc_radix_node {
    isc_uint32_t bit;			/* bit length of the prefix */
    isc_prefix_t *prefix;		/* who we are in radix tree */
    struct isc_radix_node *l, *r;	/* left and right children */
    struct isc_radix_node *parent;	/* may be used */
-   void *data;				/* pointer to data */
+   void *data[2];			/* pointers to IPv4 and IPV6 data */
-   int node_num;			/* which node this was in the tree,
+   int node_num[2];			/* which node this was in the tree, 
 					   or -1 for glue nodes */
 } isc_radix_node_t;
 

lib/isc/radix.c

@@ -14,7 +14,7 @@
  * PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: radix.c,v 1.9 2007/12/20 01:48:29 marka Exp $ */
+/* $Id: radix.c,v 1.10 2008/01/21 20:38:54 each Exp $ */
 
 /*
  * This source was adapted from MRT's RCS Ids:
@@ -173,10 +173,12 @@ _clear_radix(isc_radix_tree_t *radix, isc_radix_destroyfunc_t func) {
 
 			if (Xrn->prefix != NULL) {
 				_deref_prefix(radix->mctx, Xrn->prefix);
-				if (Xrn->data != NULL && func != NULL)
+				if (func != NULL && (Xrn->data[0] != NULL ||
+						     Xrn->data[1] != NULL)) 
 					func(Xrn->data);
 			} else {
-				INSIST(Xrn->data == NULL);
+				INSIST(Xrn->data[0] == NULL &&
+			 	       Xrn->data[1] == NULL);
 			}
 
 			isc_mem_put(radix->mctx, Xrn, sizeof(*Xrn));
@@ -231,7 +233,7 @@ isc_radix_search(isc_radix_tree_t *radix, isc_radix_node_t **target,
 	isc_radix_node_t *node;
 	isc_radix_node_t *stack[RADIX_MAXBITS + 1];
 	u_char *addr;
-	isc_uint32_t bitlen;
+	isc_uint32_t bitlen, family;
 	int cnt = 0;
 
 	REQUIRE(radix != NULL);
@@ -248,6 +250,9 @@ isc_radix_search(isc_radix_tree_t *radix, isc_radix_node_t **target,
 	addr = isc_prefix_touchar(prefix);
 	bitlen = prefix->bitlen;
 
+        /* Bitlen 0 means "any" or "none", which is always treated as IPv4 */
+	family = bitlen ? prefix->family : AF_INET;
+
 	while (node->bit < bitlen) {
 		if (node->prefix)
 			stack[cnt++] = node;
@@ -270,8 +275,10 @@ isc_radix_search(isc_radix_tree_t *radix, isc_radix_node_t **target,
 		if (_comp_with_mask(isc_prefix_tochar(node->prefix), 
 				    isc_prefix_tochar(prefix),
 				    node->prefix->bitlen)) {
-			if ((*target == NULL) ||
+			if (node->node_num[ISC_IS6(family)] != -1 &&
-			    (*target)->node_num > node->node_num) 
+			    ((*target == NULL) ||
+			     (*target)->node_num[ISC_IS6(family)] >
+				   node->node_num[ISC_IS6(family)]))
 				*target = node;
 		}
 	}
@@ -289,7 +296,7 @@ isc_radix_insert(isc_radix_tree_t *radix, isc_radix_node_t **target,
 {
 	isc_radix_node_t *node, *new_node, *parent, *glue = NULL;
 	u_char *addr, *test_addr;
-	isc_uint32_t bitlen, check_bit, differ_bit;
+	isc_uint32_t bitlen, family, check_bit, differ_bit;
 	isc_uint32_t i, j, r;
 	isc_result_t result;
 
@@ -301,11 +308,17 @@ isc_radix_insert(isc_radix_tree_t *radix, isc_radix_node_t **target,
 		prefix = source->prefix;
 
 	INSIST(prefix != NULL);
+
+	bitlen = prefix->bitlen;
+
+        /* Bitlen 0 means "any" or "none", which is always treated as IPv4 */
+	family = bitlen ? prefix->family : AF_INET;
+
 	if (radix->head == NULL) {
 		node = isc_mem_get(radix->mctx, sizeof(isc_radix_node_t));
 		if (node == NULL)
 			return (ISC_R_NOMEMORY);
-		node->bit = prefix->bitlen;
+		node->bit = bitlen;
 		result = _ref_prefix(radix->mctx, &node->prefix, prefix);
 		if (result != ISC_R_SUCCESS) {
 			isc_mem_put(radix->mctx, node,
@@ -314,7 +327,6 @@ isc_radix_insert(isc_radix_tree_t *radix, isc_radix_node_t **target,
 		}
 		node->parent = NULL;
 		node->l = node->r = NULL;
-		node->data = NULL;
 		if (source != NULL) {
 			/*
 			 * If source is non-NULL, then we're merging in a
@@ -324,10 +336,20 @@ isc_radix_insert(isc_radix_tree_t *radix, isc_radix_node_t **target,
 			 * added to num_added_node at the end of
 			 * the merge operation--we don't do it here.
 			 */
-			node->node_num = radix->num_added_node +
+			if (source->node_num[0] != -1)
-					 source->node_num;
+				node->node_num[0] = radix->num_added_node +
+						    source->node_num[0];
+			if (source->node_num[1] != -1)
+				node->node_num[1] = radix->num_added_node +
+						    source->node_num[1];
+			node->data[0] = source->data[0];
+			node->data[1] = source->data[1];
 		} else {
-			node->node_num = ++radix->num_added_node;
+			node->node_num[ISC_IS6(family)] =
+				++radix->num_added_node;
+			node->node_num[!ISC_IS6(family)] = -1;
+			node->data[0] = NULL;
+			node->data[1] = NULL;
 		}
 		radix->head = node;
 		radix->num_active_node++;
@@ -336,7 +358,6 @@ isc_radix_insert(isc_radix_tree_t *radix, isc_radix_node_t **target,
 	}
 
 	addr = isc_prefix_touchar(prefix);
-	bitlen = prefix->bitlen;
 	node = radix->head;
 
 	while (node->bit < bitlen || node->prefix == NULL) {
@@ -388,19 +409,26 @@ isc_radix_insert(isc_radix_tree_t *radix, isc_radix_node_t **target,
 
 	if (differ_bit == bitlen && node->bit == bitlen) {
 		if (node->prefix != NULL) {
+			/* Set node_num only if it hasn't been set before */
+			if (node->node_num[ISC_IS6(family)] == -1)
+				node->node_num[ISC_IS6(family)] =
+					 ++radix->num_added_node;
 			*target = node;
 			return (ISC_R_SUCCESS);
 		}
 		result = _ref_prefix(radix->mctx, &node->prefix, prefix);
 		if (result != ISC_R_SUCCESS)
 			return (result);
-		INSIST(node->data == NULL && node->node_num == -1);
+		INSIST(node->data[0] == NULL && node->node_num[0] == -1 &&
+		       node->data[1] == NULL && node->node_num[1] == -1);
 		if (source != NULL) {
 			/* Merging node */
-			node->node_num = radix->num_added_node +
+			node->node_num[ISC_IS6(family)] =
-					 source->node_num;
+				radix->num_added_node +
+				source->node_num[ISC_IS6(family)];
 		} else {
-			node->node_num = ++radix->num_added_node;
+			node->node_num[ISC_IS6(family)] =
+				++radix->num_added_node;
 		}
 		*target = node;
 		return (ISC_R_SUCCESS);
@@ -417,7 +445,7 @@ isc_radix_insert(isc_radix_tree_t *radix, isc_radix_node_t **target,
 			return (ISC_R_NOMEMORY);
 		}
 	}
-	new_node->bit = prefix->bitlen;
+	new_node->bit = bitlen;
 	result = _ref_prefix(radix->mctx, &new_node->prefix, prefix);
 	if (result != ISC_R_SUCCESS) {
 		isc_mem_put(radix->mctx, new_node, sizeof(isc_radix_node_t));
@@ -428,15 +456,23 @@ isc_radix_insert(isc_radix_tree_t *radix, isc_radix_node_t **target,
 	}
 	new_node->parent = NULL;
 	new_node->l = new_node->r = NULL;
+	new_node->node_num[0] = new_node->node_num[1] = -1;
 	radix->num_active_node++;
 
 	if (source != NULL) {
 		/* Merging node */
-		new_node->node_num = radix->num_added_node + source->node_num;
+		if (source->node_num[0] != -1)
-		new_node->data = source->data;
+			new_node->node_num[0] = radix->num_added_node +
+						source->node_num[0];
+		if (source->node_num[1] != -1)
+			new_node->node_num[1] = radix->num_added_node +
+						source->node_num[1];
+		new_node->data[0] = source->data[0];
+		new_node->data[1] = source->data[1];
 	} else {
-		new_node->node_num = ++radix->num_added_node;
+		new_node->node_num[ISC_IS6(family)] = ++radix->num_added_node;
-		new_node->data = NULL;
+		new_node->data[0] = NULL;
+		new_node->data[1] = NULL;
 	}
 
 	if (node->bit == differ_bit) {
@@ -478,8 +514,8 @@ isc_radix_insert(isc_radix_tree_t *radix, isc_radix_node_t **target,
 		glue->bit = differ_bit;
 		glue->prefix = NULL;
 		glue->parent = node->parent;
-		glue->data = NULL;
+		glue->data[0] = glue->data[1] = NULL;
-		glue->node_num = -1;
+		glue->node_num[0] = glue->node_num[1] = -1;
 		radix->num_active_node++;
 		if (differ_bit < radix->maxbits &&
 		    BIT_TEST(addr[differ_bit >> 3], 0x80 >> (differ_bit & 0x07))) {
@@ -522,8 +558,7 @@ isc_radix_remove(isc_radix_tree_t *radix, isc_radix_node_t *node) {
 			_deref_prefix(radix->mctx, node->prefix);
 
 		node->prefix = NULL;
-		/* Also I needed to clear data pointer -- masaki */
+		node->data[0] = node->data[1] = NULL;
-		node->data = NULL;
 		return;
 	}