From: Ralph Dolmans <ralph@nlnetlabs.nl>
Date: Wed, 15 Jan 2020 21:45:29 +0000 (+0100)
Subject: - processed RPZ review feedback
X-Git-Tag: release-1.10.0rc1~28^2~20
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=14913d75c081024a66c553de1d03c7bfdc569755;p=thirdparty%2Funbound.git

- processed RPZ review feedback
  - fix potential locking issue
  - add extra out of bound checks
---

diff --git a/services/rpz.c b/services/rpz.c
index 92d3b8663..3bb41916b 100644
--- a/services/rpz.c
+++ b/services/rpz.c
@@ -529,7 +529,7 @@ rpz_insert_qname_trigger(struct rpz* r, uint8_t* dname, size_t dnamelen,
 
 /** Insert RR into RPZ's respip_set */
 static int
-rpz_insert_response_ip_trigger(struct rpz* r, uint8_t* dname,
+rpz_insert_response_ip_trigger(struct rpz* r, uint8_t* dname, size_t dnamelen,
 	enum rpz_action a, uint16_t rrtype, uint16_t rrclass, uint32_t ttl,
 	uint8_t* rdata, size_t rdata_len, uint8_t* rr, size_t rr_len)
 {
@@ -547,7 +547,7 @@ rpz_insert_response_ip_trigger(struct rpz* r, uint8_t* dname,
 		return 0;
 	}
 
-	if(!netblockdnametoaddr(dname, &addr, &addrlen, &net, &af))
+	if(!netblockdnametoaddr(dname, dnamelen, &addr, &addrlen, &net, &af))
 		return 0;
 
 	lock_rw_wrlock(&r->respip_set->lock);
@@ -599,8 +599,6 @@ rpz_insert_rr(struct rpz* r, size_t aznamelen, uint8_t* dname,
 		return 0;
 	}
 	t = rpz_dname_to_trigger(policydname, policydnamelen);
-	verbose(VERB_OPS, "RPZ: found trigger: %s",
-			rpz_trigger_to_string(t));
 	if(t == RPZ_INVALID_TRIGGER) {
 		free(policydname);
 		verbose(VERB_ALGO, "RPZ: skipping invalid trigger");
@@ -612,7 +610,7 @@ rpz_insert_rr(struct rpz* r, size_t aznamelen, uint8_t* dname,
 			rr_len);
 	}
 	else if(t == RPZ_RESPONSE_IP_TRIGGER) {
-		rpz_insert_response_ip_trigger(r, policydname,
+		rpz_insert_response_ip_trigger(r, policydname, policydnamelen,
 			a, rr_type, rr_class, rr_ttl, rdatawl, rdatalen, rr,
 			rr_len);
 		free(policydname);
@@ -633,11 +631,13 @@ rpz_insert_rr(struct rpz* r, size_t aznamelen, uint8_t* dname,
  * @param qclass: qclass
  * @param only_exact: if 1 only excact (non wildcard) matches are returned
  * @param wr: get write lock for local-zone if 1, read lock if 0
+ * @param zones_keep_lock: if set do not release the r->local_zones lock, this
+ * 	  makes the caller of this function responsible for releasing the lock.
  * @return: NULL or local-zone holding rd or wr lock
  */
 static struct local_zone*
 rpz_find_zone(struct rpz* r, uint8_t* qname, size_t qname_len, uint16_t qclass,
-	int only_exact, int wr)
+	int only_exact, int wr, int zones_keep_lock)
 {
 	uint8_t* ce;
 	size_t ce_len, ce_labs;
@@ -661,7 +661,9 @@ rpz_find_zone(struct rpz* r, uint8_t* qname, size_t qname_len, uint16_t qclass,
 	} else {
 		lock_rw_rdlock(&z->lock);
 	}
-	lock_rw_unlock(&r->local_zones->lock);
+	if(!zones_keep_lock) {
+		lock_rw_unlock(&r->local_zones->lock);
+	}
 
 	if(exact)
 		return z;
@@ -685,10 +687,12 @@ rpz_find_zone(struct rpz* r, uint8_t* qname, size_t qname_len, uint16_t qclass,
 	memmove(wc+2, ce, ce_len);
 	lock_rw_unlock(&z->lock);
 
-	if(wr) {
-		lock_rw_wrlock(&r->local_zones->lock);
-	} else {
-		lock_rw_rdlock(&r->local_zones->lock);
+	if(!zones_keep_lock) {
+		if(wr) {
+			lock_rw_wrlock(&r->local_zones->lock);
+		} else {
+			lock_rw_rdlock(&r->local_zones->lock);
+		}
 	}
 	z = local_zones_find_le(r->local_zones, wc,
 		ce_len+2, ce_labs+1, qclass, &exact);
@@ -701,7 +705,9 @@ rpz_find_zone(struct rpz* r, uint8_t* qname, size_t qname_len, uint16_t qclass,
 	} else {
 		lock_rw_rdlock(&z->lock);
 	}
-	lock_rw_unlock(&r->local_zones->lock);
+	if(!zones_keep_lock) {
+		lock_rw_unlock(&r->local_zones->lock);
+	}
 	return z;
 }
 
@@ -739,7 +745,6 @@ rpz_data_delete_rr(struct local_zone* z, uint8_t* policydname,
 				/* no memory recycling for zone deletions ... */
 				if(prev) prev->next = p->next;
 				else ld->rrsets = p->next;
-
 			}
 			if(d->count > 1) {
 				if(!local_rrset_remove_rr(d, index))
@@ -797,7 +802,7 @@ rpz_remove_qname_trigger(struct rpz* r, uint8_t* dname, size_t dnamelen,
 	struct local_zone* z;
 	int delete_zone = 1;
 	z = rpz_find_zone(r, dname, dnamelen, rr_class,
-		1 /* only exact */, 1 /* wr lock */);
+		1 /* only exact */, 1 /* wr lock */, 1 /* keep lock*/);
 	if(!z) {
 		verbose(VERB_ALGO, "RPZ: cannot remove RR from IXFR, "
 			"RPZ domain not found");
@@ -813,12 +818,13 @@ rpz_remove_qname_trigger(struct rpz* r, uint8_t* dname, size_t dnamelen,
 	if(delete_zone) {
 		local_zones_del_zone(r->local_zones, z);
 	}
+	lock_rw_unlock(&r->local_zones->lock); 
 	return;
 }
 
 static void
-rpz_remove_response_ip_trigger(struct rpz* r, uint8_t* dname, enum rpz_action a,
-	uint16_t rr_type, uint8_t* rdatawl, size_t rdatalen)
+rpz_remove_response_ip_trigger(struct rpz* r, uint8_t* dname, size_t dnamelen,
+	enum rpz_action a, uint16_t rr_type, uint8_t* rdatawl, size_t rdatalen)
 {
 	struct resp_addr* node;
 	struct sockaddr_storage addr;
@@ -826,7 +832,7 @@ rpz_remove_response_ip_trigger(struct rpz* r, uint8_t* dname, enum rpz_action a,
 	int net, af;
 	int delete_respip = 1;
 
-	if(!netblockdnametoaddr(dname, &addr, &addrlen, &net, &af))
+	if(!netblockdnametoaddr(dname, dnamelen, &addr, &addrlen, &net, &af))
 		return;
 
 	lock_rw_wrlock(&r->respip_set->lock);
@@ -877,8 +883,8 @@ rpz_remove_rr(struct rpz* r, size_t aznamelen, uint8_t* dname, size_t dnamelen,
 		rpz_remove_qname_trigger(r, policydname, policydnamelen, a,
 			rr_type, rr_class, rdatawl, rdatalen);
 	} else if(t == RPZ_RESPONSE_IP_TRIGGER) {
-		rpz_remove_response_ip_trigger(r, policydname, a, rr_type,
-			rdatawl, rdatalen);
+		rpz_remove_response_ip_trigger(r, policydname, policydnamelen,
+			a, rr_type, rdatawl, rdatalen);
 	}
 	free(policydname);
 }
@@ -921,7 +927,7 @@ rpz_apply_qname_trigger(struct auth_zones* az, struct module_env* env,
 		if(!r->taglist || taglist_intersect(r->taglist, 
 			r->taglistlen, taglist, taglen)) {
 			z = rpz_find_zone(r, qinfo->qname, qinfo->qname_len,
-				qinfo->qclass, 0, 0);
+				qinfo->qclass, 0, 0, 0);
 			if(z && r->action_override == RPZ_DISABLED_ACTION) {
 				if(r->log)
 					log_rpz_apply(z->name,
diff --git a/util/data/dname.c b/util/data/dname.c
index d55b1358d..7c119e439 100644
--- a/util/data/dname.c
+++ b/util/data/dname.c
@@ -547,14 +547,14 @@ dname_lab_startswith(uint8_t* label, char* prefix, char** endptr)
 }
 
 int
-dname_has_label(uint8_t* dname, uint8_t* label)
+dname_has_label(uint8_t* dname, size_t dnamelen, uint8_t* label)
 {
-	uint8_t lablen = *dname;
-	while(lablen) {
-		if(lablen == *label && memlowercmp(dname, label, lablen) == 0)
+	int len = *dname;
+	while(*dname && len <= dnamelen) {
+		if(*dname == *label && memlowercmp(dname, label, *dname) == 0)
 			return 1;
-		dname += lablen;
-		lablen = *dname;
+		len += *dname;
+		dname += *dname;
 	}
 	return 0;
 }
diff --git a/util/data/dname.h b/util/data/dname.h
index a7939fb6e..e37c11822 100644
--- a/util/data/dname.h
+++ b/util/data/dname.h
@@ -199,10 +199,11 @@ int dname_lab_startswith(uint8_t* label, char* prefix, char** endptr);
 /**
  * Check if dname contains label
  * @param dname: dname
+ * @param dnamelen: length of dname
  * @param label: label to be checked for presence in dname
  * @return: 1 if dname has this label, 0 otherwise
  */
-int dname_has_label(uint8_t* dname, uint8_t* label);
+int dname_has_label(uint8_t* dname, size_t dnamelen, uint8_t* label);
 
 /**
  * See if domain name d1 is a strict subdomain of d2.
diff --git a/util/data/packed_rrset.h b/util/data/packed_rrset.h
index 59693dbf6..729877bab 100644
--- a/util/data/packed_rrset.h
+++ b/util/data/packed_rrset.h
@@ -448,6 +448,7 @@ struct ub_packed_rrset_key* packed_rrset_copy_alloc(
 
 /**
  * Find RR index in packed rrset
+ * Raw comparison, does not canonicalize RDATA
  * @param d: packed rrset
  * @param rdata: RDATA of RR to find
  * @param len: length of rdata
diff --git a/util/net_help.c b/util/net_help.c
index 3e5965882..be284d581 100644
--- a/util/net_help.c
+++ b/util/net_help.c
@@ -285,19 +285,22 @@ int netblockstrtoaddr(const char* str, int port, struct sockaddr_storage* addr,
 }
 
 /* RPZ format address dname to network byte order address */
-static int ipdnametoaddr(uint8_t* dname, struct sockaddr_storage* addr,
-	socklen_t* addrlen, int* af)
+static int ipdnametoaddr(uint8_t* dname, size_t dnamelen,
+	struct sockaddr_storage* addr, socklen_t* addrlen, int* af)
 {
 	uint8_t* ia;
 	size_t dnamelabs = dname_count_labels(dname);
 	uint8_t lablen;
 	char* e = NULL;
 	int z = 0;
+	int len = 0;
 	int i;
 	*af = AF_INET;
-	if(dnamelabs > 6 || dname_has_label(dname, (uint8_t*)"\002zz")) {
+	if(dnamelabs > 6 ||
+		dname_has_label(dname, dnamelen, (uint8_t*)"\002zz")) {
 		*af = AF_INET6;
 	}
+	len = *dname;
 	lablen = *dname++;
 	i = (*af == AF_INET) ? 3 : 15;
 	if(*af == AF_INET6) {
@@ -313,21 +316,21 @@ static int ipdnametoaddr(uint8_t* dname, struct sockaddr_storage* addr,
 		sa->sin_family = AF_INET;
 		ia = (uint8_t*)&sa->sin_addr;
 	}
-	while(lablen && i >= 0) {
-		char buff[lablen+1];
+	while(lablen && i >= 0 && len <= dnamelen) {
+		char buff[LDNS_MAX_LABELLEN+1];
 		uint16_t chunk; /* big enough to not overflow on IPv6 hextet */
 		if((*af == AF_INET && (lablen > 3 || dnamelabs > 6)) ||
 			(*af == AF_INET6 && (lablen > 4 || dnamelabs > 10))) {
 			return 0;
 		}
 		if(memcmp(dname, "zz", 2) == 0 && *af == AF_INET6) {
-			/* add one or more 0 labels */
+			/* Add one or more 0 labels. Address is initialised at
+			 * 0, so just skip the zero part. */
 			int zl = 11 - dnamelabs;
 			if(z || zl < 0)
 				return 0;
 			z = 1;
 			i -= (zl*2);
-			memset(ia+(i+1), 0, zl*2);
 		} else {
 			memcpy(buff, dname, lablen);
 			buff[lablen] = '\0';
@@ -335,9 +338,11 @@ static int ipdnametoaddr(uint8_t* dname, struct sockaddr_storage* addr,
 			if(!e || *e != '\0' || (*af == AF_INET && chunk > 255))
 				return 0;
 			if(*af == AF_INET) {
+				log_assert(i < 4 && i >= 0);
 				ia[i] = (uint8_t)chunk;
 				i--;
 			} else {
+				log_assert(i < 15 && i >= 1);
 				/* ia in network byte order */
 				ia[i-1] = (uint8_t)(chunk >> 8);
 				ia[i] = (uint8_t)(chunk & 0x00FF);
@@ -346,6 +351,7 @@ static int ipdnametoaddr(uint8_t* dname, struct sockaddr_storage* addr,
 		}
 		dname += lablen;
 		lablen = *dname++;
+		len += lablen;
 	}
 	if(i != -1)
 		/* input too short */
@@ -353,8 +359,8 @@ static int ipdnametoaddr(uint8_t* dname, struct sockaddr_storage* addr,
 	return 1;
 }
 
-int netblockdnametoaddr(uint8_t* dname, struct sockaddr_storage* addr,
-	socklen_t* addrlen, int* net, int* af)
+int netblockdnametoaddr(uint8_t* dname, size_t dnamelen,
+	struct sockaddr_storage* addr, socklen_t* addrlen, int* net, int* af)
 {
 	char buff[3 /* 3 digit netblock */ + 1];
 	if(*dname > 3)
@@ -363,9 +369,13 @@ int netblockdnametoaddr(uint8_t* dname, struct sockaddr_storage* addr,
 	memcpy(buff, dname+1, *dname);
 	buff[*dname] = '\0';
 	*net = atoi(buff);
+	if(*net == 0 && strcmp(buff, "0") != 0)
+		return 0;
 	dname += *dname;
 	dname++;
-	if(!ipdnametoaddr(dname, addr, addrlen, af))
+	if(!ipdnametoaddr(dname, dnamelen, addr, addrlen, af))
+		return 0;
+	if((*af == AF_INET6 && *net > 128) || (*af == AF_INET && *net > 32))
 		return 0;
 	return 1;
 }
diff --git a/util/net_help.h b/util/net_help.h
index c7216817f..7a33a7203 100644
--- a/util/net_help.h
+++ b/util/net_help.h
@@ -477,12 +477,13 @@ void listen_sslctx_delete_ticket_keys(void);
  *  - 24.10.100.51.198.rpz-ip -> 198.51.100.10/24
  *  - 32.10.zz.db8.2001.rpz-ip -> 2001:db8:0:0:0:0:0:10/32
  * @param dname: the dname containing RPZ format netblock
+ * @param dnamelen: length of dname
  * @param addr: where to store sockaddr.
  * @param addrlen: length of stored sockaddr is returned.
  * @param net: where to store netmask
  * @param af: where to store address family.
  * @return 0 on error.
  */
-int netblockdnametoaddr(uint8_t* dname, struct sockaddr_storage* addr,
-	socklen_t* addrlen, int* net, int* af);
+int netblockdnametoaddr(uint8_t* dname, size_t dnamelen,
+	struct sockaddr_storage* addr, socklen_t* addrlen, int* net, int* af);
 #endif /* NET_HELP_H */