tags 501800 +patch thanks
After further discussions with upstream authors : Evan Hunt wrotes : --8<-------------------------------- Thanks to Emmanuel Bouthenot for the assistance, I was able to reproduce the issue with his instructions. It turned out to be a bug that was fixed in 9.5.1b2. There are several other ACL problems that have been fixed in that release and in 9.5.1b3, which is due out in a few days. I'd recommend using 9.5.1 when it's complete (in about a month, most likely), but I'm told Debian is planning is to release 9.5.0-P2 plus patches instead. So I've rolled all the ACL fixes in the 9.5.1 pipeline up into a single patch and attached it to this email. This reflects the following changes: 2474. [bug] ACL structures could be allocated with insufficient space, causing an array overrun. [RT #18765] 2470. [bug] Elements of the isc_radix_node_t could be incorrectly overwritten. [RT# 18719] 2456. [bug] In ACLs, ::/0 and 0.0.0.0/0 would both match any address, regardless of family. They now correctly distinguish IPv4 from IPv6. [RT #18559] 2441. [bug] isc_radix_insert() could copy radix tree nodes incompletely. [RT #18573] 2439. [bug] Potential NULL dereference in dns_acl_isanyornone(). [RT #18559] (For the record, the bug you hit was 2441.) Thanks again and let me know if you have any questions. -------------------------------->8-- The patch is attached. Cheers, -- Emmanuel Bouthenot mail : [EMAIL PROTECTED] gpg : 0x414EC36E jid : [EMAIL PROTECTED] irc : kolter@(freenode|oftc)
Index: lib/dns/acl.c =================================================================== RCS file: /proj/cvs/prod/bind9/lib/dns/acl.c,v retrieving revision 1.37.2.7 diff -u -u -r1.37.2.7 acl.c --- lib/dns/acl.c 29 Apr 2008 01:04:14 -0000 1.37.2.7 +++ lib/dns/acl.c 28 Oct 2008 00:25:02 -0000 @@ -148,7 +148,10 @@ return (ISC_FALSE); if (acl->iptable->radix->head->prefix->bitlen == 0 && - *(isc_boolean_t *) (acl->iptable->radix->head->data[0]) == pos) + acl->iptable->radix->head->data[0] != NULL && + acl->iptable->radix->head->data[0] == + acl->iptable->radix->head->data[1] && + *(isc_boolean_t *) (acl->iptable->radix->head->data[0]) == pos) return (ISC_TRUE); return (ISC_FALSE); /* All others */ @@ -220,8 +223,6 @@ /* Found a match. */ if (result == ISC_R_SUCCESS && node != NULL) { - if (node->bit == 0) - family = AF_INET; match_num = node->node_num[ISC_IS6(family)]; if (*(isc_boolean_t *) node->data[ISC_IS6(family)] == ISC_TRUE) *match = match_num; @@ -491,9 +492,8 @@ 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; + family = prefix->family; /* Negated entries are always secure. */ secure = * (isc_boolean_t *)data[ISC_IS6(family)]; Index: lib/dns/iptable.c =================================================================== RCS file: /proj/cvs/prod/bind9/lib/dns/iptable.c,v retrieving revision 1.5.46.3 diff -u -u -r1.5.46.3 iptable.c --- lib/dns/iptable.c 21 Jan 2008 21:02:24 -0000 1.5.46.3 +++ lib/dns/iptable.c 28 Oct 2008 00:25:02 -0000 @@ -70,22 +70,39 @@ 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) + if (result != ISC_R_SUCCESS) { + isc_refcount_destroy(&pfx.refcount); return(result); + } - /* If the node already contains data, don't overwrite it */ - if (node->data[ISC_IS6(family)] == NULL) { - if (pos) - node->data[ISC_IS6(family)] = &dns_iptable_pos; - else - node->data[ISC_IS6(family)] = &dns_iptable_neg; + /* If a node already contains data, don't overwrite it */ + family = pfx.family; + if (family == AF_UNSPEC) { + /* "any" or "none" */ + INSIST(pfx.bitlen == 0); + if (pos) { + if (node->data[0] == NULL) + node->data[0] = &dns_iptable_pos; + if (node->data[1] == NULL) + node->data[1] = &dns_iptable_pos; + } else { + if (node->data[0] == NULL) + node->data[0] = &dns_iptable_neg; + if (node->data[1] == NULL) + node->data[1] = &dns_iptable_neg; + } + } else { + /* any other prefix */ + if (node->data[ISC_IS6(family)] == NULL) { + if (pos) + node->data[ISC_IS6(family)] = &dns_iptable_pos; + else + node->data[ISC_IS6(family)] = &dns_iptable_neg; + } } + isc_refcount_destroy(&pfx.refcount); return (ISC_R_SUCCESS); } Index: lib/isc/radix.c =================================================================== RCS file: /proj/cvs/prod/bind9/lib/isc/radix.c,v retrieving revision 1.9.6.5.2.1 diff -u -u -r1.9.6.5.2.1 radix.c --- lib/isc/radix.c 24 Jul 2008 02:03:22 -0000 1.9.6.5.2.1 +++ lib/isc/radix.c 28 Oct 2008 00:25:03 -0000 @@ -53,7 +53,7 @@ REQUIRE(target != NULL); - if (family != AF_INET6 && family != AF_INET) + if (family != AF_INET6 && family != AF_INET && family != AF_UNSPEC) return (ISC_R_NOTIMPLEMENTED); prefix = isc_mem_get(mctx, sizeof(isc_prefix_t)); @@ -64,6 +64,7 @@ prefix->bitlen = (bitlen >= 0) ? bitlen : 128; memcpy(&prefix->add.sin6, dest, 16); } else { + /* AF_UNSPEC is "any" or "none"--treat it as AF_INET */ prefix->bitlen = (bitlen >= 0) ? bitlen : 32; memcpy(&prefix->add.sin, dest, 4); } @@ -95,7 +96,8 @@ _ref_prefix(isc_mem_t *mctx, isc_prefix_t **target, isc_prefix_t *prefix) { INSIST(prefix != NULL); INSIST((prefix->family == AF_INET && prefix->bitlen <= 32) || - (prefix->family == AF_INET6 && prefix->bitlen <= 128)); + (prefix->family == AF_INET6 && prefix->bitlen <= 128) || + (prefix->family == AF_UNSPEC && prefix->bitlen == 0)); REQUIRE(target != NULL); /* If this prefix is a static allocation, copy it into new memory */ @@ -236,7 +238,7 @@ isc_radix_node_t *stack[RADIX_MAXBITS + 1]; u_char *addr; isc_uint32_t bitlen; - int family, tfamily = -1; + int tfamily = -1; int cnt = 0; REQUIRE(radix != NULL); @@ -276,16 +278,12 @@ if (_comp_with_mask(isc_prefix_tochar(node->prefix), isc_prefix_tochar(prefix), node->prefix->bitlen)) { - /* Bitlen 0 means "any" or "none", - which is always treated as IPv4 */ - family = node->prefix->bitlen ? - prefix->family : AF_INET; - if (node->node_num[ISC_IS6(family)] != -1 && + if (node->node_num[ISC_IS6(prefix->family)] != -1 && ((*target == NULL) || (*target)->node_num[ISC_IS6(tfamily)] > - node->node_num[ISC_IS6(family)])) { + node->node_num[ISC_IS6(prefix->family)])) { *target = node; - tfamily = family; + tfamily = prefix->family; } } } @@ -303,7 +301,7 @@ { isc_radix_node_t *node, *new_node, *parent, *glue = NULL; u_char *addr, *test_addr; - isc_uint32_t bitlen, family, check_bit, differ_bit; + isc_uint32_t bitlen, fam, check_bit, differ_bit; isc_uint32_t i, j, r; isc_result_t result; @@ -317,9 +315,7 @@ INSIST(prefix != NULL); bitlen = prefix->bitlen; - - /* Bitlen 0 means "any" or "none", which is always treated as IPv4 */ - family = bitlen ? prefix->family : AF_INET; + fam = prefix->family; if (radix->head == NULL) { node = isc_mem_get(radix->mctx, sizeof(isc_radix_node_t)); @@ -353,8 +349,14 @@ node->data[0] = source->data[0]; node->data[1] = source->data[1]; } else { - node->node_num[ISC_IS6(family)] = - ++radix->num_added_node; + if (fam == AF_UNSPEC) { + /* "any" or "none" */ + node->node_num[0] = node->node_num[1] = + ++radix->num_added_node; + } else { + node->node_num[ISC_IS6(fam)] = + ++radix->num_added_node; + } node->data[0] = NULL; node->data[1] = NULL; } @@ -417,25 +419,71 @@ 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; + if (source != NULL) { + /* Merging node */ + if (node->node_num[0] == -1 && + source->node_num[0] != -1) { + node->node_num[0] = + radix->num_added_node + + source->node_num[0]; + node->data[0] = source->data[0]; + } + if (node->node_num[1] == -1 && + source->node_num[0] != -1) { + node->node_num[1] = + radix->num_added_node + + source->node_num[1]; + node->data[1] = source->data[1]; + } + } else { + if (fam == AF_UNSPEC) { + /* "any" or "none" */ + int next = radix->num_added_node + 1; + if (node->node_num[0] == -1) { + node->node_num[0] = next; + radix->num_added_node = next; + } + if (node->node_num[1] == -1) { + node->node_num[1] = next; + radix->num_added_node = next; + } + } else { + if (node->node_num[ISC_IS6(fam)] == -1) + node->node_num[ISC_IS6(fam)] + = ++radix->num_added_node; + } + } *target = node; return (ISC_R_SUCCESS); + } else { + result = + _ref_prefix(radix->mctx, &node->prefix, prefix); + if (result != ISC_R_SUCCESS) + return (result); } - result = _ref_prefix(radix->mctx, &node->prefix, prefix); - if (result != ISC_R_SUCCESS) - return (result); 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[ISC_IS6(family)] = - radix->num_added_node + - source->node_num[ISC_IS6(family)]; + if (source->node_num[0] != -1) { + node->node_num[0] = radix->num_added_node + + source->node_num[0]; + node->data[0] = source->data[0]; + } + if (source->node_num[1] != -1) { + node->node_num[1] = radix->num_added_node + + source->node_num[1]; + node->data[1] = source->data[1]; + } } else { - node->node_num[ISC_IS6(family)] = - ++radix->num_added_node; + if (fam == AF_UNSPEC) { + /* "any" or "none" */ + node->node_num[0] = node->node_num[1] = + ++radix->num_added_node; + } else { + node->node_num[ISC_IS6(fam)] = + ++radix->num_added_node; + } } *target = node; return (ISC_R_SUCCESS); @@ -477,7 +525,14 @@ new_node->data[0] = source->data[0]; new_node->data[1] = source->data[1]; } else { - new_node->node_num[ISC_IS6(family)] = ++radix->num_added_node; + if (fam == AF_UNSPEC) { + /* "any" or "none" */ + new_node->node_num[0] = new_node->node_num[1] = + ++radix->num_added_node; + } else { + new_node->node_num[ISC_IS6(fam)] = + ++radix->num_added_node; + } new_node->data[0] = NULL; new_node->data[1] = NULL; } @@ -525,7 +580,7 @@ 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))) { + BIT_TEST(addr[differ_bit>>3], 0x80 >> (differ_bit & 07))) { glue->r = new_node; glue->l = node; } else { Index: lib/isc/include/isc/radix.h =================================================================== RCS file: /proj/cvs/prod/bind9/lib/isc/include/isc/radix.h,v retrieving revision 1.5.46.4 diff -u -u -r1.5.46.4 radix.h --- lib/isc/include/isc/radix.h 21 Jan 2008 23:46:23 -0000 1.5.46.4 +++ lib/isc/include/isc/radix.h 28 Oct 2008 00:25:03 -0000 @@ -37,7 +37,7 @@ #define NETADDR_TO_PREFIX_T(na,pt,bits) \ do { \ memset(&(pt), 0, sizeof(pt)); \ - if((bits) && (na) != NULL) { \ + if((na) != NULL) { \ (pt).family = (na)->family; \ (pt).bitlen = (bits); \ if ((pt).family == AF_INET6) { \ @@ -46,14 +46,16 @@ } else \ memcpy(&(pt).add.sin, &(na)->type.in, \ ((bits)+7)/8); \ - } else \ - (pt).family = AF_INET; \ + } else { \ + (pt).family = AF_UNSPEC; \ + (pt).bitlen = 0; \ + } \ isc_refcount_init(&(pt).refcount, 0); \ } while(0) typedef struct isc_prefix { - unsigned int family; /* AF_INET | AF_INET6 */ - unsigned int bitlen; + unsigned int family; /* AF_INET | AF_INET6, or AF_UNSPEC for "any" */ + unsigned int bitlen; /* 0 for "any" */ isc_refcount_t refcount; union { struct in_addr sin; Index: lib/isccfg/aclconf.c =================================================================== RCS file: /proj/cvs/prod/bind9/lib/isccfg/aclconf.c,v retrieving revision 1.17.100.2 diff -u -u -r1.17.100.2 aclconf.c --- lib/isccfg/aclconf.c 24 Jul 2008 23:48:39 -0000 1.17.100.2 +++ lib/isccfg/aclconf.c 28 Oct 2008 00:25:03 -0000 @@ -160,6 +160,51 @@ return (dns_name_dup(dns_fixedname_name(&fixname), mctx, dnsname)); } +/* + * Recursively pre-parse an ACL definition to find the total number + * of non-IP-prefix elements (localhost, localnets, key) in all nested + * ACLs, so that the parent will have enough space allocated for the + * elements table after all the nested ACLs have been merged in to the + * parent. + */ +static int +count_acl_elements(const cfg_obj_t *caml, const cfg_obj_t *cctx) +{ + const cfg_listelt_t *elt; + const cfg_obj_t *cacl = NULL; + isc_result_t result; + int n = 0; + + for (elt = cfg_list_first(caml); + elt != NULL; + elt = cfg_list_next(elt)) { + const cfg_obj_t *ce = cfg_listelt_value(elt); + + /* negated element; just get the value. */ + if (cfg_obj_istuple(ce)) + ce = cfg_tuple_get(ce, "value"); + + if (cfg_obj_istype(ce, &cfg_type_keyref)) { + n++; + } else if (cfg_obj_islist(ce)) { + n += count_acl_elements(ce, cctx); + } else if (cfg_obj_isstring(ce)) { + const char *name = cfg_obj_asstring(ce); + if (strcasecmp(name, "localhost") == 0 || + strcasecmp(name, "localnets") == 0) { + n++; + } else if (strcasecmp(name, "any") != 0 && + strcasecmp(name, "none") != 0) { + result = get_acl_def(cctx, name, &cacl); + if (result == ISC_R_SUCCESS) + n += count_acl_elements(cacl, cctx) + 1; + } + } + } + + return n; +} + isc_result_t cfg_acl_fromconfig(const cfg_obj_t *caml, const cfg_obj_t *cctx, @@ -194,14 +239,18 @@ } else { /* * Need to allocate a new ACL structure. Count the items - * in the ACL definition and allocate space for that many - * elements (even though some or all of them may end up in - * the iptable instead of the element array). + * in the ACL definition that will require space in the + * elemnts table. (Note that if nest_level is nonzero, + * *everything* goes in the elements table.) */ - isc_boolean_t recurse = ISC_TF(nest_level == 0); - result = dns_acl_create(mctx, - cfg_list_length(caml, recurse), - &dacl); + int nelem; + + if (nest_level == 0) + nelem = count_acl_elements(caml, cctx); + else + nelem = cfg_list_length(caml, ISC_FALSE); + + result = dns_acl_create(mctx, nelem, &dacl); if (result != ISC_R_SUCCESS) return (result); } @@ -209,8 +258,7 @@ de = dacl->elements; for (elt = cfg_list_first(caml); elt != NULL; - elt = cfg_list_next(elt)) - { + elt = cfg_list_next(elt)) { const cfg_obj_t *ce = cfg_listelt_value(elt); isc_boolean_t neg;