Proccess more review feedback

daemon/stats.c

@@ -271,7 +271,7 @@ server_stats_compile(struct worker* worker, struct ub_stats_info* s, int reset)
 	s->svr.ans_secure += (long long)worker->env.mesh->ans_secure;
 	s->svr.ans_bogus += (long long)worker->env.mesh->ans_bogus;
 	s->svr.ans_rcode_nodata += (long long)worker->env.mesh->ans_nodata;
-	for(i=0; i<16; i++)
+	for(i=0; i<UB_STATS_RCODE_NUM; i++)
 		s->svr.ans_rcode[i] += (long long)worker->env.mesh->ans_rcode[i];
 	for(i=0; i<UB_STATS_RPZ_ACTION_NUM; i++)
 		s->svr.rpz_action[i] += (long long)worker->env.mesh->rpz_action[i];

services/authzone.c

@@ -1176,8 +1176,10 @@ az_insert_rr(struct auth_zone* z, uint8_t* rr, size_t rr_len,
 		return 0;
 	}
 	if(z->rpz) {
-		rpz_insert_rr(z->rpz, z->namelen, dname, dname_len, rr_type, rr_class, rr_ttl, rdata,
+		if(!(rpz_insert_rr(z->rpz, z->namelen, dname, dname_len, 
-			rdatalen, rr, rr_len);
+			rr_type, rr_class, rr_ttl, rdata, rdatalen, rr,
+			rr_len)))
+			return 0;
 	}
 	return 1;
 }

services/localzone.c

@@ -399,17 +399,18 @@ rrset_insert_rr(struct regional* region, struct packed_rrset_data* pd,
 int
 local_rrset_remove_rr(struct packed_rrset_data* pd, size_t index)
 {
+	log_assert(pd->count > 0);
 	if(index >= pd->count) {
 		log_warn("Trying to remove RR with out of bound index");
 		return 0;
 	}
-	if(index - 1 < pd->count) {
+	if(index + 1 < pd->count) {
 		/* not removing last element */
 		size_t nexti = index + 1;
 		size_t num = pd->count - nexti;
-		memcpy(pd->rr_len+index, pd->rr_len+nexti, sizeof(*pd->rr_len)*num);
+		memmove(pd->rr_len+index, pd->rr_len+nexti, sizeof(*pd->rr_len)*num);
-		memcpy(pd->rr_ttl+index, pd->rr_ttl+nexti, sizeof(*pd->rr_ttl)*num);
+		memmove(pd->rr_ttl+index, pd->rr_ttl+nexti, sizeof(*pd->rr_ttl)*num);
-		memcpy(pd->rr_data+index, pd->rr_data+nexti, sizeof(*pd->rr_data)*num);
+		memmove(pd->rr_data+index, pd->rr_data+nexti, sizeof(*pd->rr_data)*num);
 	}
 	pd->count--;
 	return 1;
@@ -1354,7 +1355,6 @@ find_tag_datas(struct query_info* qinfo, struct config_strlist* list,
 	return result;
 }
 
-/** answer local data match */
 int
 local_data_answer(struct local_zone* z, struct module_env* env,
 	struct query_info* qinfo, struct edns_data* edns,

services/localzone.h

@@ -572,6 +572,25 @@ enum respip_action {
 	respip_invalid = local_zone_invalid,
 };
 
+/**
+ * Get local data from local zone and encode answer.
+ * @param z: local zone to use
+ * @param env: module env
+ * @param qinfo: qinfo
+ * @param edns: edns data, for message encoding
+ * @param repinfo: reply info, for message encoding
+ * @param buf: commpoint buffer
+ * @param temp: scratchpad region
+ * @param labs: number of labels in qname
+ * @param ldp: where to store local data
+ * @param lzt: type of local zone
+ * @param tag: matching tag index
+ * @param tag_datas: alc specific tag data list
+ * @param tag_datas_size: size of tag_datas
+ * @param tagname: list of names of tags, for logging purpose
+ * @param num_tags: number of tags
+ * @return 1 on success
+ */
 int
 local_data_answer(struct local_zone* z, struct module_env* env,
 	struct query_info* qinfo, struct edns_data* edns,
@@ -580,6 +599,20 @@ local_data_answer(struct local_zone* z, struct module_env* env,
 	enum localzone_type lz_type, int tag, struct config_strlist** tag_datas,
 	size_t tag_datas_size, char** tagname, int num_tags);
 
+/**
+ * Add RR to local zone.
+ * @param z: local zone to add RR to
+ * @param nm: dname of RR
+ * @param nmlen: length of nm
+ * @param nmlabs: number of labels of nm
+ * @param rrtype: RR type
+ * @param rrclass: RR class
+ * @param ttl: TTL of RR to add
+ * @param rdata: RDATA of RR to add
+ * @param rdata_len: length of rdata
+ * @param rrstr: RR in string format, for logging
+ * @return: 1 on success
+ */
 int
 local_zone_enter_rr(struct local_zone* z, uint8_t* nm, size_t nmlen,
 	int nmlabs, uint16_t rrtype, uint16_t rrclass, time_t ttl,

services/mesh.c

@@ -1592,8 +1592,8 @@ mesh_stats_clear(struct mesh_area* mesh)
 	timehist_clear(mesh->histogram);
 	mesh->ans_secure = 0;
 	mesh->ans_bogus = 0;
-	memset(&mesh->ans_rcode[0], 0, sizeof(size_t)*16);
+	memset(&mesh->ans_rcode[0], 0, sizeof(size_t)*UB_STATS_RCODE_NUM);
-	memset(&mesh->rpz_action[0], 0, sizeof(size_t)*10);
+	memset(&mesh->rpz_action[0], 0, sizeof(size_t)*UB_STATS_RPZ_ACTION_NUM);
 	mesh->ans_nodata = 0;
 }
 

services/mesh.h

@@ -52,6 +52,7 @@
 #include "util/module.h"
 #include "services/modstack.h"
 #include "services/rpz.h"
+#include "libunbound/unbound.h"
 struct sldns_buffer;
 struct mesh_state;
 struct mesh_reply;
@@ -122,11 +123,11 @@ struct mesh_area {
 	/** (extended stats) bogus replies */
 	size_t ans_bogus;
 	/** (extended stats) rcodes in replies */
-	size_t ans_rcode[16];
+	size_t ans_rcode[UB_STATS_RCODE_NUM];
 	/** (extended stats) rcode nodata in replies */
 	size_t ans_nodata;
 	/** (extended stats) type of applied RPZ action */
-	size_t rpz_action[10];
+	size_t rpz_action[UB_STATS_RPZ_ACTION_NUM];
 
 	/** backup of query if other operations recurse and need the
 	 * network buffers */

services/rpz.c

@@ -51,6 +51,7 @@
 #include "util/locks.h"
 #include "util/regional.h"
 
+/** string for RPZ action enum */
 const char*
 rpz_action_to_string(enum rpz_action a)
 {
@@ -69,6 +70,7 @@ rpz_action_to_string(enum rpz_action a)
 	return "unknown";
 }
 
+/** RPZ action enum for config string */
 static enum rpz_action
 rpz_config_to_action(char* a)
 {
@@ -106,19 +108,24 @@ rpz_trigger_to_string(enum rpz_trigger r)
 /**
  * Get the label that is just before the root label.
  * @param dname: dname to work on
- * @return: pointer to TLD label
+ * @param maxdnamelen: maximum length of the dname
+ * @return: pointer to TLD label, NULL if not found or invalid dname
  */
 static uint8_t*
-get_tld_label(uint8_t* dname)
+get_tld_label(uint8_t* dname, size_t maxdnamelen)
 {
 	uint8_t* prevlab = dname;
+	size_t dnamelen = 0;
 
 	/* only root label */
 	if(*dname == 0)
 		return NULL;
 
 	while(*dname) {
-		dname = dname+*dname+1;
+		dnamelen += ((size_t)*dname)+1;
+		if(dnamelen > maxdnamelen)
+			return NULL;
+		dname = dname+((size_t)*dname)+1;
 		if(*dname != 0)
 			prevlab = dname;
 	}
@@ -183,7 +190,7 @@ rpz_rr_to_action(uint16_t rr_type, uint8_t* rdatawl, size_t rdatalen)
 	}
 
 	/* all other TLDs starting with "rpz-" are invalid */
-	tldlab = get_tld_label(rdata);
+	tldlab = get_tld_label(rdata, rdatalen-2);
 	if(tldlab && dname_lab_startswith(tldlab, "rpz-", &endptr))
 		return RPZ_INVALID_ACTION;
 
@@ -199,13 +206,11 @@ rpz_action_to_localzone_type(enum rpz_action a)
 	case RPZ_NODATA_ACTION:		return local_zone_always_nodata;
 	case RPZ_DROP_ACTION:		return local_zone_always_deny;
 	case RPZ_PASSTHRU_ACTION:	return local_zone_always_transparent;
-	case RPZ_LOCAL_DATA_ACTION:
+	case RPZ_LOCAL_DATA_ACTION:	/* fallthrough */
-	case RPZ_CNAME_OVERRIDE_ACTION:
+	case RPZ_CNAME_OVERRIDE_ACTION: return local_zone_redirect;
-					return local_zone_redirect;
+	case RPZ_INVALID_ACTION: 	/* fallthrough */
-	case RPZ_INVALID_ACTION:
+	case RPZ_TCP_ONLY_ACTION:	/* fallthrough */
-	case RPZ_TCP_ONLY_ACTION:
+	default:			return local_zone_invalid;
-	default:
-		return local_zone_invalid;
 	}
 }
 
@@ -217,13 +222,11 @@ rpz_action_to_respip_action(enum rpz_action a)
 	case RPZ_NODATA_ACTION:		return respip_always_nodata;
 	case RPZ_DROP_ACTION:		return respip_always_deny;
 	case RPZ_PASSTHRU_ACTION:	return respip_always_transparent;
-	case RPZ_LOCAL_DATA_ACTION:
+	case RPZ_LOCAL_DATA_ACTION:	/* fallthrough */
-	case RPZ_CNAME_OVERRIDE_ACTION:
+	case RPZ_CNAME_OVERRIDE_ACTION: return respip_redirect;
-					return respip_redirect;
+	case RPZ_INVALID_ACTION:	/* fallthrough */
-	case RPZ_INVALID_ACTION:
+	case RPZ_TCP_ONLY_ACTION:	/* fallthrough */
-	case RPZ_TCP_ONLY_ACTION:
+	default:			return respip_invalid;
-	default:
-		return respip_invalid;
 	}
 }
 
@@ -260,14 +263,19 @@ respip_action_to_rpz_action(enum respip_action a)
 /**
  * Get RPZ trigger for dname
  * @param dname: dname containing RPZ trigger
+ * @param dname_len: length of the dname
  * @return: RPZ trigger enum
  */
 static enum rpz_trigger
-rpz_dname_to_trigger(uint8_t* dname)
+rpz_dname_to_trigger(uint8_t* dname, size_t dname_len)
 {
 	uint8_t* tldlab;
 	char* endptr;
-	tldlab = get_tld_label(dname);
+
+	if(dname_valid(dname, dname_len) != dname_len)
+		return RPZ_INVALID_TRIGGER;
+
+	tldlab = get_tld_label(dname, dname_len);
 	if(!tldlab || !dname_lab_startswith(tldlab, "rpz-", &endptr))
 		return RPZ_QNAME_TRIGGER;
 
@@ -350,20 +358,21 @@ new_cname_override(struct regional* region, uint8_t* ct, size_t ctlen)
 		return NULL;
 	}
 	pd->rr_len[0] = ctlen+2;
-	pd->rr_ttl[0] = 3600; /* TODO, what should this be? */
+	pd->rr_ttl[0] = 3600;
 	pd->rr_data[0] = regional_alloc_zero(region, 2 /* rdlength */ + ctlen);
 	if(!pd->rr_data[0]) {
 		log_err("out of memory");
 		return NULL;
 	}
-	memcpy(pd->rr_data[0], &rdlength, 2);
+	memmove(pd->rr_data[0], &rdlength, 2);
-	memcpy(pd->rr_data[0]+2, ct, ctlen);
+	memmove(pd->rr_data[0]+2, ct, ctlen);
 
 	rrset->entry.data = pd;
 	rrset->rk.type = htons(LDNS_RR_TYPE_CNAME);
 	rrset->rk.rrset_class = htons(LDNS_RR_CLASS_IN);
 	return rrset;
 }
+
 struct rpz*
 rpz_create(struct config_auth* p)
 {
@@ -395,7 +404,7 @@ rpz_create(struct config_auth* p)
 		size_t nmlen = sizeof(nm);
 
 		if(!p->rpz_cname) {
-			log_err("RPZ override with cname action found, but not "
+			log_err("RPZ override with cname action found, but no "
 				"rpz-cname-override configured");
 			goto err;
 		}
@@ -411,8 +420,11 @@ rpz_create(struct config_auth* p)
 		}
 	}
 	r->log = p->rpz_log;
-	if(p->rpz_log_name)
+	if(p->rpz_log_name) {
-		r->log_name = strdup(p->rpz_log_name);
+		if(!(r->log_name = strdup(p->rpz_log_name)))
+			log_err("malloc failure on RPZ log_name strdup");
+			goto err;
+	}
 	return r;
 err:
 	if(r) {
@@ -427,16 +439,22 @@ err:
 	return NULL;
 }
 
-/** Remove RPZ zone name from dname */
+/**
+ * Remove RPZ zone name from dname
+ * Copy dname to newdname, without the originlen number of trailing bytes
+ */
 static size_t
 strip_dname_origin(uint8_t* dname, size_t dnamelen, size_t originlen,
-	uint8_t* newdname)
+	uint8_t* newdname, size_t maxnewdnamelen)
 {
 	size_t newdnamelen;
 	if(dnamelen < originlen)
 		return 0;
 	newdnamelen = dnamelen - originlen;
+	if(newdnamelen+1 > maxnewdnamelen)
+		return 0;
 	memmove(newdname, dname, newdnamelen);
+	memset(newdname+newdnamelen, 0, 1);
 	return newdnamelen + 1;	/* + 1 for root label */
 }
 
@@ -465,6 +483,13 @@ rpz_insert_qname_trigger(struct rpz* r, uint8_t* dname, size_t dnamelen,
 		LDNS_RR_CLASS_IN);
 	if(z && a != RPZ_LOCAL_DATA_ACTION) {
 		rrstr = sldns_wire2str_rr(rr, rr_len);
+		if(!rrstr) {
+			log_err("malloc error while inserting RPZ qname "
+				"trigger");
+			free(dname);
+			lock_rw_unlock(&r->local_zones->lock);
+			return;
+		}
 		verbose(VERB_ALGO, "RPZ: skipping duplicate record: '%s'",
 			rrstr);
 		free(rrstr);
@@ -485,6 +510,13 @@ rpz_insert_qname_trigger(struct rpz* r, uint8_t* dname, size_t dnamelen,
 	}
 	if(a == RPZ_LOCAL_DATA_ACTION) {
 		rrstr = sldns_wire2str_rr(rr, rr_len);
+		if(!rrstr) {
+			log_err("malloc error while inserting RPZ qname "
+				"trigger");
+			free(dname);
+			lock_rw_unlock(&r->local_zones->lock);
+			return;
+		}
 		local_zone_enter_rr(z, dname, dnamelen, dnamelabs,
 			rrtype, rrclass, ttl, rdata, rdata_len, rrstr);
 		free(rrstr);
@@ -520,6 +552,11 @@ rpz_insert_response_ip_trigger(struct rpz* r, uint8_t* dname,
 
 	lock_rw_wrlock(&r->respip_set->lock);
 	rrstr = sldns_wire2str_rr(rr, rr_len);
+	if(!rrstr) {
+		log_err("malloc error while inserting RPZ respip trigger");
+		lock_rw_unlock(&r->respip_set->lock);
+		return 0;
+	}
 	if(!(node=respip_sockaddr_find_or_create(r->respip_set, &addr, addrlen,
 		net, 1, rrstr))) {
 		lock_rw_unlock(&r->respip_set->lock);
@@ -540,24 +577,35 @@ rpz_insert_response_ip_trigger(struct rpz* r, uint8_t* dname,
 	return 1;
 }
 
-void
+int
 rpz_insert_rr(struct rpz* r, size_t aznamelen, uint8_t* dname,
 	size_t dnamelen, uint16_t rr_type, uint16_t rr_class, uint32_t rr_ttl,
 	uint8_t* rdatawl, size_t rdatalen, uint8_t* rr, size_t rr_len)
 {
 	size_t policydnamelen;
 	/* name is free'd in local_zone delete */
-	uint8_t* policydname = calloc(1, LDNS_MAX_DOMAINLEN + 1);
 	enum rpz_trigger t;
 	enum rpz_action a;
-	
+	uint8_t* policydname;
+
+	log_assert(dnamelen >= aznamelen);
+	if(!(policydname = calloc(1, (dnamelen-aznamelen)+1)))
+		return 0;
+
 	a = rpz_rr_to_action(rr_type, rdatawl, rdatalen);
 	if(!(policydnamelen = strip_dname_origin(dname, dnamelen, aznamelen,
-		policydname))) {
+		policydname, (dnamelen-aznamelen)+1))) {
 		free(policydname);
-		return;
+		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");
+		return 1;
 	}
-	t = rpz_dname_to_trigger(policydname);
 	if(t == RPZ_QNAME_TRIGGER) {
 		rpz_insert_qname_trigger(r, policydname, policydnamelen,
 			a, rr_type, rr_class, rr_ttl, rdatawl, rdatalen, rr,
@@ -574,6 +622,7 @@ rpz_insert_rr(struct rpz* r, size_t aznamelen, uint8_t* dname,
 		verbose(VERB_ALGO, "RPZ: skipping unusupported trigger: %s",
 			rpz_trigger_to_string(t));
 	}
+	return 1;
 }
 
 /**
@@ -595,10 +644,11 @@ rpz_find_zone(struct rpz* r, uint8_t* qname, size_t qname_len, uint16_t qclass,
 	uint8_t wc[LDNS_MAX_DOMAINLEN];
 	int exact;
 	struct local_zone* z = NULL;
-	if(wr)
+	if(wr) {
 		lock_rw_wrlock(&r->local_zones->lock);
-	else
+	} else {
 		lock_rw_rdlock(&r->local_zones->lock);
+	}
 	z = local_zones_find_le(r->local_zones, qname, qname_len,
 		dname_count_labels(qname),
 		LDNS_RR_CLASS_IN, &exact);
@@ -606,10 +656,11 @@ rpz_find_zone(struct rpz* r, uint8_t* qname, size_t qname_len, uint16_t qclass,
 		lock_rw_unlock(&r->local_zones->lock);
 		return NULL;
 	}
-	if(wr)
+	if(wr) {
 		lock_rw_wrlock(&z->lock);
-	else
+	} else {
 		lock_rw_rdlock(&z->lock);
+	}
 	lock_rw_unlock(&r->local_zones->lock);
 
 	if(exact)
@@ -634,20 +685,22 @@ 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)
+	if(wr) {
 		lock_rw_wrlock(&r->local_zones->lock);
-	else
+	} 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);
 	if(!z || !exact) {
 		lock_rw_unlock(&r->local_zones->lock);
 		return NULL;
 	}
-	if(wr)
+	if(wr) {
 		lock_rw_wrlock(&z->lock);
-	else
+	} else {
 		lock_rw_rdlock(&z->lock);
+	}
 	lock_rw_unlock(&r->local_zones->lock);
 	return z;
 }
@@ -815,11 +868,11 @@ rpz_remove_rr(struct rpz* r, size_t aznamelen, uint8_t* dname, size_t dnamelen,
 		return;
 	}
 	if(!(policydnamelen = strip_dname_origin(dname, dnamelen, aznamelen,
-		policydname))) {
+		policydname, LDNS_MAX_DOMAINLEN + 1))) {
 		free(policydname);
 		return;
 	}
-	t = rpz_dname_to_trigger(policydname);
+	t = rpz_dname_to_trigger(policydname, policydnamelen);
 	if(t == RPZ_QNAME_TRIGGER) {
 		rpz_remove_qname_trigger(r, policydname, policydnamelen, a,
 			rr_type, rr_class, rdatawl, rdatalen);

services/rpz.h

@@ -61,6 +61,7 @@ enum rpz_trigger {
 	RPZ_RESPONSE_IP_TRIGGER, /* rpz-ip */
 	RPZ_NSDNAME_TRIGGER,	 /* rpz-nsdname */
 	RPZ_NSIP_TRIGGER,	 /* rpz-nsip */
+	RPZ_INVALID_TRIGGER, 	 /* dname does not contain valid trigger */
 };
 
 /**
@@ -114,8 +115,9 @@ struct rpz {
  * @param rdatalen: length if the RR, including the prepended rdata size
  * @param rr: the complete RR, for logging purposes
  * @param rr_len: the length of the complete RR
+ * @return: 0 on error
  */
-void rpz_insert_rr(struct rpz* r, size_t aznamelen, uint8_t* dname,
+int rpz_insert_rr(struct rpz* r, size_t aznamelen, uint8_t* dname,
 	size_t dnamelen, uint16_t rr_type, uint16_t rr_class, uint32_t rr_ttl,
 	uint8_t* rdatawl, size_t rdatalen, uint8_t* rr, size_t rr_len);
 

testdata/rpz_qname.rpl

@@ -28,7 +28,7 @@ rpz:
 TEMPFILE_NAME rpz2.example.com
 TEMPFILE_CONTENTS rpz2.example.com
 $ORIGIN example.com.
-rpz	3600	IN	SOA	ns1.rpz.example.com. hostmaster.rpz.example.com. (
+rpz2	3600	IN	SOA	ns1.rpz.example.com. hostmaster.rpz.example.com. (
 		1379078166 28800 7200 604800 7200 )
 	3600	IN	NS	ns1.rpz.example.com.
 	3600	IN	NS	ns2.rpz.example.com.

util/configparser.y

@@ -351,10 +351,9 @@ content_auth: auth_name | auth_zonefile | auth_master | auth_url |
 
 rpz_tag: VAR_TAGS STRING_ARG
 	{
-		size_t len;
 		uint8_t* bitlist;
+		size_t len = 0;
 		OUTYY(("P(server_local_zone_tag:%s)\n", $2));
-		len = 0;
 		bitlist = config_parse_taglist(cfg_parser->cfg, $2,
 			&len);
 		free($2);
@@ -376,7 +375,7 @@ rpz_action_override: VAR_RPZ_ACTION_OVERRIDE STRING_ARG
 		   strcmp($2, "passthru")!=0 && strcmp($2, "drop")!=0 &&
 		   strcmp($2, "cname")!=0 && strcmp($2, "disabled")!=0) {
 			yyerror("rpz-action-override action: expected nxdomain, "
-				"nodata, passthru, drop cname or disabled");
+				"nodata, passthru, drop, cname or disabled");
 			free($2);
 			cfg_parser->cfg->auths->rpz_action_override = NULL;
 		}

util/data/dname.c

@@ -550,12 +550,8 @@ int
 dname_has_label(uint8_t* dname, uint8_t* label)
 {
 	uint8_t lablen = *dname++;
-	while(lablen) {
+	if(memlowercmp(dname, label, lablen) == 0)
-		if(*label == lablen && memcmp(dname, label+1, lablen) == 0)
+		return 1;
-			return 1;
-		dname += lablen;
-		lablen = *dname++;
-	}
 	return 0;
 }
 

util/net_help.h

@@ -473,6 +473,9 @@ void listen_sslctx_delete_ticket_keys(void);
 
 /**
  * RPZ format netblock to network byte order address and netblock
+ * example RPZ netblock format dnames:
+ *  - 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 addr: where to store sockaddr.
  * @param addrlen: length of stored sockaddr is returned.