- 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 92d3b8663df043c085550f27f0c9872b3698374c..3bb41916b789c3e788672ff9e8bc66c308e80d03 100644 (file)
--- 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 d55b1358dfb389c7d3fe4b66dc0797c225aa9a9a..7c119e4391ec3509a683beee0e4c73314fb509d2 100644 (file)
--- 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 a7939fb6e8bef9f8bfaf77374e9e67c9eacbc634..e37c11822b365f6b13bd07b538022f17ec97ef12 100644 (file)
--- 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 59693dbf60d3f4226d56a1f07914f75b9e56bdaf..729877bab5ecb63f31d032062313fe4b5384a267 100644 (file)
--- 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 3e5965882225f45dd1b3746933b2cf78f2089c08..be284d5811474d145cd9b3d4a6ef87b003cbc5e8 100644 (file)
--- 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 c7216817f863337f3c8c831193731b6625e54dff..7a33a72035d59e3afc0a7e676f03f6d506d1e17e 100644 (file)
--- 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 */