From: Mark Andrews Date: Thu, 20 Dec 2007 01:48:29 +0000 (+0000) Subject: 2282. [bug] Acl code fixups. [RT #17346] X-Git-Tag: v9.5.0-P1^4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=604419a812b491cd35fb6fad129c3c39da7200a1;p=thirdparty%2Fbind9.git 2282. [bug] Acl code fixups. [RT #17346] --- diff --git a/CHANGES b/CHANGES index 75785d52e1c..8ff50008662 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ +2282. [bug] Acl code fixups. [RT #17346] + 2281. [bug] Attempts to use undefined acls were not being logged. [RT #17307] diff --git a/bin/tests/system/xfer/ns2/named.conf b/bin/tests/system/xfer/ns2/named.conf index 2bf1b522165..53a64ab9626 100644 --- a/bin/tests/system/xfer/ns2/named.conf +++ b/bin/tests/system/xfer/ns2/named.conf @@ -15,7 +15,7 @@ * PERFORMANCE OF THIS SOFTWARE. */ -/* $Id: named.conf,v 1.23 2007/06/19 23:47:07 tbox Exp $ */ +/* $Id: named.conf,v 1.24 2007/12/20 01:48:29 marka Exp $ */ controls { /* empty */ }; @@ -36,8 +36,12 @@ options { include "../../common/controls.conf"; key tsigzone. { - algorithm hmac-md5; - secret "1234abcd8765"; + algorithm hmac-md5; + secret "1234abcd8765"; +}; + +acl tzkey { + key tsigzone.; }; zone "." { @@ -53,5 +57,5 @@ zone "example" { zone "tsigzone" { type master; file "tsigzone.db"; - allow-transfer { key tsigzone.; }; + allow-transfer { tzkey; }; }; diff --git a/lib/dns/acl.c b/lib/dns/acl.c index 6bb169c7959..908a315de82 100644 --- a/lib/dns/acl.c +++ b/lib/dns/acl.c @@ -15,7 +15,7 @@ * PERFORMANCE OF THIS SOFTWARE. */ -/* $Id: acl.c,v 1.35 2007/09/19 03:03:29 marka Exp $ */ +/* $Id: acl.c,v 1.36 2007/12/20 01:48:29 marka Exp $ */ /*! \file */ @@ -57,12 +57,12 @@ dns_acl_create(isc_mem_t *mctx, int n, dns_acl_t **target) { return (result); } - result = dns_iptable_create(mctx, &acl->iptable); + result = dns_iptable_create(mctx, &acl->iptable); if (result != ISC_R_SUCCESS) { isc_mem_put(mctx, acl, sizeof(*acl)); return (result); } - + acl->elements = NULL; acl->alloc = 0; acl->length = 0; @@ -101,7 +101,7 @@ dns_acl_anyornone(isc_mem_t *mctx, isc_boolean_t neg, dns_acl_t **target) { result = dns_acl_create(mctx, 0, &acl); if (result != ISC_R_SUCCESS) return (result); - dns_iptable_addprefix(acl->iptable, NULL, 0, ISC_TF(!neg)); + dns_iptable_addprefix(acl->iptable, NULL, 0, ISC_TF(!neg)); *target = acl; return (result); } @@ -129,13 +129,13 @@ dns_acl_none(isc_mem_t *mctx, dns_acl_t **target) { static isc_boolean_t dns_acl_isanyornone(dns_acl_t *acl, isc_boolean_t pos) { - /* Should never happen but let's be safe */ - if (acl == NULL || - acl->iptable == NULL || - acl->iptable->radix == NULL || - acl->iptable->radix->head == NULL || - acl->iptable->radix->head->prefix == NULL) - return (ISC_FALSE); + /* Should never happen but let's be safe */ + if (acl == NULL || + acl->iptable == NULL || + acl->iptable->radix == NULL || + acl->iptable->radix->head == NULL || + acl->iptable->radix->head->prefix == NULL) + return (ISC_FALSE); if (acl->length != 0 && acl->node_count != 1) return (ISC_FALSE); @@ -153,7 +153,7 @@ dns_acl_isanyornone(dns_acl_t *acl, isc_boolean_t pos) isc_boolean_t dns_acl_isany(dns_acl_t *acl) { - return (dns_acl_isanyornone(acl, ISC_TRUE)); + return (dns_acl_isanyornone(acl, ISC_TRUE)); } /* @@ -162,7 +162,7 @@ dns_acl_isany(dns_acl_t *acl) isc_boolean_t dns_acl_isnone(dns_acl_t *acl) { - return (dns_acl_isanyornone(acl, ISC_FALSE)); + return (dns_acl_isanyornone(acl, ISC_FALSE)); } /* @@ -182,10 +182,10 @@ dns_acl_match(const isc_netaddr_t *reqaddr, isc_uint16_t bitlen; isc_prefix_t pfx; isc_radix_node_t *node; - const isc_netaddr_t *addr; - isc_netaddr_t v4addr; + const isc_netaddr_t *addr; + isc_netaddr_t v4addr; isc_result_t result; - int match_num = -1; + int match_num = -1; unsigned int i; REQUIRE(reqaddr != NULL); @@ -194,7 +194,7 @@ dns_acl_match(const isc_netaddr_t *reqaddr, if (env == NULL || env->match_mapped == ISC_FALSE || reqaddr->family != AF_INET6 || !IN6_IS_ADDR_V4MAPPED(&reqaddr->type.in6)) - addr = reqaddr; + addr = reqaddr; else { isc_netaddr_fromv4mapped(&v4addr, reqaddr); addr = &v4addr; @@ -213,24 +213,24 @@ dns_acl_match(const isc_netaddr_t *reqaddr, /* Found a match. */ if (result == ISC_R_SUCCESS && node != NULL) { match_num = node->node_num; - if (*(isc_boolean_t *) node->data == ISC_TRUE) - *match = match_num; - else - *match = -match_num; - } + if (*(isc_boolean_t *) node->data == ISC_TRUE) + *match = match_num; + else + *match = -match_num; + } - /* Now search non-radix elements for a match with a lower node_num. */ + /* Now search non-radix elements for a match with a lower node_num. */ for (i = 0; i < acl->length; i++) { dns_aclelement_t *e = &acl->elements[i]; if (dns_aclelement_match(reqaddr, reqsigner, e, env, matchelt)) { - if (match_num == -1 || e->node_num < match_num) { - if (e->negative) - *match = -e->node_num; - else - *match = e->node_num; - } + if (match_num == -1 || e->node_num < match_num) { + if (e->negative == ISC_TRUE) + *match = -e->node_num; + else + *match = e->node_num; + } return (ISC_R_SUCCESS); } } @@ -251,63 +251,90 @@ isc_result_t dns_acl_merge(dns_acl_t *dest, dns_acl_t *source, isc_boolean_t pos) { isc_result_t result; - unsigned int newalloc, nelem, i; - int max_node = 0, nodes; - - /* Resize the element array if needed. */ - if (dest->length + source->length > dest->alloc) { - void *newmem; - - newalloc = dest->alloc + source->alloc; - if (newalloc < 4) - newalloc = 4; - - newmem = isc_mem_get(dest->mctx, - newalloc * sizeof(dns_aclelement_t)); - if (newmem == NULL) - return (ISC_R_NOMEMORY); - - /* Copy in the original elements */ - memcpy(newmem, dest->elements, - dest->length * sizeof(dns_aclelement_t)); - - /* Release the memory for the old elements array */ - isc_mem_put(dest->mctx, dest->elements, - dest->alloc * sizeof(dns_aclelement_t)); - dest->elements = newmem; - dest->alloc = newalloc; - } - - /* - * Now copy in the new elements, increasing their node_num - * values so as to keep the new ACL consistent. If we're - * negating, then negate positive elements, but keep negative - * elements the same for security reasons. - */ - nelem = dest->length; - memcpy(&dest->elements[nelem], source->elements, - (source->length * sizeof(dns_aclelement_t))); - for (i = 0; i < source->length; i++) { - dest->elements[nelem + i].node_num = - source->elements[i].node_num + dest->node_count; - if (source->elements[i].node_num > max_node) - max_node = source->elements[i].node_num; - if (!pos && source->elements[i].negative == ISC_FALSE) - dest->elements[nelem + i].negative = ISC_TRUE; - } - - /* - * Merge the iptables. Make sure the destination ACL's - * node_count value is set correctly afterward. - */ - nodes = max_node + dest->node_count; - result = dns_iptable_merge(dest->iptable, source->iptable, pos); - if (result != ISC_R_SUCCESS) - return (result); - if (nodes > dest->node_count) - dest->node_count = nodes; - - return (ISC_R_SUCCESS); + unsigned int newalloc, nelem, i; + int max_node = 0, nodes; + + /* Resize the element array if needed. */ + if (dest->length + source->length > dest->alloc) { + void *newmem; + + newalloc = dest->alloc + source->alloc; + if (newalloc < 4) + newalloc = 4; + + newmem = isc_mem_get(dest->mctx, + newalloc * sizeof(dns_aclelement_t)); + if (newmem == NULL) + return (ISC_R_NOMEMORY); + + /* Copy in the original elements */ + memcpy(newmem, dest->elements, + dest->length * sizeof(dns_aclelement_t)); + + /* Release the memory for the old elements array */ + isc_mem_put(dest->mctx, dest->elements, + dest->alloc * sizeof(dns_aclelement_t)); + dest->elements = newmem; + dest->alloc = newalloc; + } + + /* + * Now copy in the new elements, increasing their node_num + * values so as to keep the new ACL consistent. If we're + * negating, then negate positive elements, but keep negative + * elements the same for security reasons. + */ + nelem = dest->length; + dest->length += source->length; + for (i = 0; i < source->length; i++) { + if (source->elements[i].node_num > max_node) + max_node = source->elements[i].node_num; + + /* Copy type. */ + dest->elements[nelem + i].type = source->elements[i].type; + + /* Adjust node numbering. */ + dest->elements[nelem + i].node_num = + source->elements[i].node_num + dest->node_count; + + /* Duplicate nested acl. */ + if(source->elements[i].type == dns_aclelementtype_nestedacl && + source->elements[i].nestedacl != NULL) + dns_acl_attach(source->elements[i].nestedacl, + &dest->elements[nelem + i].nestedacl); + + /* Duplicate key name. */ + if (source->elements[i].type == dns_aclelementtype_keyname) { + dns_name_init(&dest->elements[nelem+i].keyname, NULL); + result = dns_name_dup(&source->elements[i].keyname, + dest->mctx, + &dest->elements[nelem+i].keyname); + if (result != ISC_R_SUCCESS) + return result; + } + + /* reverse sense of positives if this is a negative acl */ + if (!pos && source->elements[i].negative == ISC_FALSE) { + dest->elements[nelem + i].negative = ISC_TRUE; + } else { + dest->elements[nelem + i].negative = + source->elements[i].negative; + } + } + + + /* + * Merge the iptables. Make sure the destination ACL's + * node_count value is set correctly afterward. + */ + nodes = max_node + dest->node_count; + result = dns_iptable_merge(dest->iptable, source->iptable, pos); + if (result != ISC_R_SUCCESS) + return (result); + if (nodes > dest->node_count) + dest->node_count = nodes; + + return (ISC_R_SUCCESS); } /* @@ -330,62 +357,62 @@ dns_aclelement_match(const isc_netaddr_t *reqaddr, int indirectmatch; isc_result_t result; - switch (e->type) { - case dns_aclelementtype_keyname: + switch (e->type) { + case dns_aclelementtype_keyname: if (reqsigner != NULL && dns_name_equal(reqsigner, &e->keyname)) { - if (matchelt != NULL) - *matchelt = e; - return (ISC_TRUE); - } else { - return (ISC_FALSE); - } - - case dns_aclelementtype_nestedacl: - inner = e->nestedacl; - break; - - case dns_aclelementtype_localhost: - if (env == NULL || env->localhost == NULL) - return (ISC_FALSE); - inner = env->localhost; - break; - - case dns_aclelementtype_localnets: - if (env == NULL || env->localnets == NULL) - return (ISC_FALSE); - inner = env->localnets; - break; - - default: - /* Should be impossible */ - INSIST(0); - } + if (matchelt != NULL) + *matchelt = e; + return (ISC_TRUE); + } else { + return (ISC_FALSE); + } + + case dns_aclelementtype_nestedacl: + inner = e->nestedacl; + break; + + case dns_aclelementtype_localhost: + if (env == NULL || env->localhost == NULL) + return (ISC_FALSE); + inner = env->localhost; + break; + + case dns_aclelementtype_localnets: + if (env == NULL || env->localnets == NULL) + return (ISC_FALSE); + inner = env->localnets; + break; + + default: + /* Should be impossible. */ + INSIST(0); + } - result = dns_acl_match(reqaddr, reqsigner, inner, env, - &indirectmatch, matchelt); - INSIST(result == ISC_R_SUCCESS); - - /* - * Treat negative matches in indirect ACLs as "no match". - * That way, a negated indirect ACL will never become a - * surprise positive match through double negation. - * XXXDCL this should be documented. - */ - - if (indirectmatch > 0) { - if (matchelt != NULL) - *matchelt = e; - return (ISC_TRUE); - } + result = dns_acl_match(reqaddr, reqsigner, inner, env, + &indirectmatch, matchelt); + INSIST(result == ISC_R_SUCCESS); + + /* + * Treat negative matches in indirect ACLs as "no match". + * That way, a negated indirect ACL will never become a + * surprise positive match through double negation. + * XXXDCL this should be documented. + */ + + if (indirectmatch > 0) { + if (matchelt != NULL) + *matchelt = e; + return (ISC_TRUE); + } - /* - * A negative indirect match may have set *matchelt, but we don't - * want it set when we return. - */ + /* + * A negative indirect match may have set *matchelt, but we don't + * want it set when we return. + */ - if (matchelt != NULL) - *matchelt = NULL; + if (matchelt != NULL) + *matchelt = NULL; return (ISC_FALSE); } @@ -413,8 +440,8 @@ destroy(dns_acl_t *dacl) { dacl->alloc * sizeof(dns_aclelement_t)); if (dacl->name != NULL) isc_mem_free(dacl->mctx, dacl->name); - if (dacl->iptable != NULL) - dns_iptable_detach(&dacl->iptable); + if (dacl->iptable != NULL) + dns_iptable_detach(&dacl->iptable); isc_refcount_destroy(&dacl->refcount); dacl->magic = 0; isc_mem_put(dacl->mctx, dacl, sizeof(*dacl)); @@ -438,7 +465,7 @@ static isc_boolean_t insecure_prefix_found; static void initialize_action(void) { - RUNTIME_CHECK(isc_mutex_init(&insecure_prefix_lock) == ISC_R_SUCCESS); + RUNTIME_CHECK(isc_mutex_init(&insecure_prefix_lock) == ISC_R_SUCCESS); } /* @@ -449,30 +476,30 @@ static void is_insecure(isc_prefix_t *prefix, void *data) { isc_boolean_t secure = * (isc_boolean_t *)data; - /* Negated entries are always secure */ - if (!secure) { - return; - } + /* Negated entries are always secure. */ + if (!secure) { + return; + } - /* If loopback prefix found, return */ + /* If loopback prefix found, return */ switch (prefix->family) { - case AF_INET: + case AF_INET: if (prefix->bitlen == 32 && htonl(prefix->add.sin.s_addr) == INADDR_LOOPBACK) - return; + return; break; - case AF_INET6: + case AF_INET6: if (prefix->bitlen == 128 && IN6_IS_ADDR_LOOPBACK(&prefix->add.sin6)) - return; + return; break; - default: + default: break; } - /* Non-negated, non-loopback */ - insecure_prefix_found = ISC_TRUE; - return; + /* Non-negated, non-loopback */ + insecure_prefix_found = ISC_TRUE; + return; } /* @@ -491,19 +518,19 @@ dns_acl_isinsecure(const dns_acl_t *a) { RUNTIME_CHECK(isc_once_do(&insecure_prefix_once, initialize_action) == ISC_R_SUCCESS); - /* - * Walk radix tree to find out if there are any non-negated, - * non-loopback prefixes. - */ + /* + * Walk radix tree to find out if there are any non-negated, + * non-loopback prefixes. + */ LOCK(&insecure_prefix_lock); - insecure_prefix_found = ISC_FALSE; - isc_radix_process(a->iptable->radix, is_insecure); + insecure_prefix_found = ISC_FALSE; + isc_radix_process(a->iptable->radix, is_insecure); insecure = insecure_prefix_found; UNLOCK(&insecure_prefix_lock); - if (insecure) - return(ISC_TRUE); + if (insecure) + return(ISC_TRUE); - /* Now check non-radix elements */ + /* Now check non-radix elements */ for (i = 0; i < a->length; i++) { dns_aclelement_t *e = &a->elements[i]; @@ -512,19 +539,19 @@ dns_acl_isinsecure(const dns_acl_t *a) { continue; switch (e->type) { - case dns_aclelementtype_keyname: - case dns_aclelementtype_localhost: + case dns_aclelementtype_keyname: + case dns_aclelementtype_localhost: continue; - case dns_aclelementtype_nestedacl: - if (dns_acl_isinsecure(e->nestedacl)) - return (ISC_TRUE); - continue; + case dns_aclelementtype_nestedacl: + if (dns_acl_isinsecure(e->nestedacl)) + return (ISC_TRUE); + continue; - case dns_aclelementtype_localnets: + case dns_aclelementtype_localnets: return (ISC_TRUE); - default: + default: INSIST(0); return (ISC_TRUE); } diff --git a/lib/isc/radix.c b/lib/isc/radix.c index f9a2c507539..dc01dede269 100644 --- a/lib/isc/radix.c +++ b/lib/isc/radix.c @@ -14,7 +14,7 @@ * PERFORMANCE OF THIS SOFTWARE. */ -/* $Id: radix.c,v 1.8 2007/11/27 19:14:45 each Exp $ */ +/* $Id: radix.c,v 1.9 2007/12/20 01:48:29 marka Exp $ */ /* * This source was adapted from MRT's RCS Ids: @@ -115,9 +115,9 @@ _ref_prefix(isc_mem_t *mctx, isc_prefix_t **target, isc_prefix_t *prefix) { static int _comp_with_mask(void *addr, void *dest, u_int mask) { - /* Mask length of zero matches everything */ - if (mask == 0) - return (1); + /* Mask length of zero matches everything */ + if (mask == 0) + return (1); if (memcmp(addr, dest, mask / 8) == 0) { int n = mask / 8; @@ -315,7 +315,20 @@ isc_radix_insert(isc_radix_tree_t *radix, isc_radix_node_t **target, node->parent = NULL; node->l = node->r = NULL; node->data = NULL; - node->node_num = ++radix->num_added_node; + if (source != NULL) { + /* + * If source is non-NULL, then we're merging in a + * node from an existing radix tree. To keep + * the node_num values consistent, the calling + * function will add the total number of nodes + * 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 + + source->node_num; + } else { + node->node_num = ++radix->num_added_node; + } radix->head = node; radix->num_active_node++; *target = node; @@ -382,7 +395,13 @@ isc_radix_insert(isc_radix_tree_t *radix, isc_radix_node_t **target, if (result != ISC_R_SUCCESS) return (result); INSIST(node->data == NULL && node->node_num == -1); - node->node_num = ++radix->num_added_node; + if (source != NULL) { + /* Merging node */ + node->node_num = radix->num_added_node + + source->node_num; + } else { + node->node_num = ++radix->num_added_node; + } *target = node; return (ISC_R_SUCCESS); } @@ -412,17 +431,7 @@ isc_radix_insert(isc_radix_tree_t *radix, isc_radix_node_t **target, radix->num_active_node++; if (source != NULL) { - /* - * If source is non-NULL, then we're merging in a node - * from an existing radix tree. Node_num values have to - * remain consistent; they can't just be added in whatever - * order came from walking the tree. So we don't increment - * num_added_node here; instead, we add it to the node-num - * values for each node from the nested tree, and then when - * the whole tree is done, the calling function will bump - * num_added_node by the highest value of node_num in the - * tree. - */ + /* Merging node */ new_node->node_num = radix->num_added_node + source->node_num; new_node->data = source->data; } else {