From ed83fa75f5657ab2394a701f7ccc169dd9ef48fc Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Fri, 24 Sep 2010 05:09:03 +0000 Subject: [PATCH] 2963. [security] The allow-query acl was being applied instead of the allow-query-cache acl to cache lookups. [RT #22114] --- CHANGES | 3 ++ bin/named/client.c | 6 ++-- bin/named/include/named/query.h | 4 ++- bin/named/query.c | 25 ++++++-------- bin/named/server.c | 44 ++++++++++++++---------- bin/tests/system/addzone/ns2/named2.conf | 3 +- bin/tests/system/addzone/tests.sh | 36 +++++++++++++------ lib/dns/include/dns/view.h | 4 ++- lib/dns/view.c | 8 ++++- 9 files changed, 82 insertions(+), 51 deletions(-) diff --git a/CHANGES b/CHANGES index a300cec32f..40635f69f7 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +2963. [security] The allow-query acl was being applied instead of the + allow-query-cache acl to cache lookups. [RT #22114] + 2962. [port] win32: add more dependencies to BINDBuild.dsw. [RT #22062] diff --git a/bin/named/client.c b/bin/named/client.c index 02efe4e37a..940c535d7a 100644 --- a/bin/named/client.c +++ b/bin/named/client.c @@ -15,7 +15,7 @@ * PERFORMANCE OF THIS SOFTWARE. */ -/* $Id: client.c,v 1.268 2010/09/13 23:46:58 tbox Exp $ */ +/* $Id: client.c,v 1.269 2010/09/24 05:09:02 marka Exp $ */ #include @@ -1865,13 +1865,13 @@ client_request(isc_task_t *task, isc_event_t *event) { client->view->recursionacl, ISC_TRUE) == ISC_R_SUCCESS && ns_client_checkaclsilent(client, NULL, - client->view->queryacl, + client->view->cacheacl, ISC_TRUE) == ISC_R_SUCCESS && ns_client_checkaclsilent(client, &client->destaddr, client->view->recursiononacl, ISC_TRUE) == ISC_R_SUCCESS && ns_client_checkaclsilent(client, &client->destaddr, - client->view->queryonacl, + client->view->cacheonacl, ISC_TRUE) == ISC_R_SUCCESS) ra = ISC_TRUE; diff --git a/bin/named/include/named/query.h b/bin/named/include/named/query.h index 500b57714e..5a9bd24cfe 100644 --- a/bin/named/include/named/query.h +++ b/bin/named/include/named/query.h @@ -15,7 +15,7 @@ * PERFORMANCE OF THIS SOFTWARE. */ -/* $Id: query.h,v 1.40 2007/06/19 23:46:59 tbox Exp $ */ +/* $Id: query.h,v 1.41 2010/09/24 05:09:02 marka Exp $ */ #ifndef NAMED_QUERY_H #define NAMED_QUERY_H 1 @@ -71,6 +71,8 @@ struct ns_query { #define NS_QUERYATTR_SECURE 0x0200 #define NS_QUERYATTR_NOAUTHORITY 0x0400 #define NS_QUERYATTR_NOADDITIONAL 0x0800 +#define NS_QUERYATTR_CACHEACLOKVALID 0x1000 +#define NS_QUERYATTR_CACHEACLOK 0x2000 isc_result_t ns_query_init(ns_client_t *client); diff --git a/bin/named/query.c b/bin/named/query.c index da27c58556..eb847edb2b 100644 --- a/bin/named/query.c +++ b/bin/named/query.c @@ -15,7 +15,7 @@ * PERFORMANCE OF THIS SOFTWARE. */ -/* $Id: query.c,v 1.345 2010/09/15 12:07:55 marka Exp $ */ +/* $Id: query.c,v 1.346 2010/09/24 05:09:02 marka Exp $ */ /*! \file */ @@ -820,17 +820,15 @@ query_getcachedb(ns_client_t *client, dns_name_t *name, dns_rdatatype_t qtype, return (DNS_R_REFUSED); dns_db_attach(client->view->cachedb, &db); - if ((client->query.attributes & - NS_QUERYATTR_QUERYOKVALID) != 0) { + if ((client->query.attributes & NS_QUERYATTR_CACHEACLOKVALID) != 0) { /* - * We've evaluated the view's queryacl already. If - * NS_QUERYATTR_QUERYOK is set, then the client is + * We've evaluated the view's cacheacl already. If + * NS_QUERYATTR_CACHEACLOK is set, then the client is * allowed to make queries, otherwise the query should * be refused. */ check_acl = ISC_FALSE; - if ((client->query.attributes & - NS_QUERYATTR_QUERYOK) == 0) + if ((client->query.attributes & NS_QUERYATTR_CACHEACLOK) == 0) goto refuse; } else { /* @@ -844,16 +842,15 @@ query_getcachedb(ns_client_t *client, dns_name_t *name, dns_rdatatype_t qtype, char msg[NS_CLIENT_ACLMSGSIZE("query (cache)")]; result = ns_client_checkaclsilent(client, NULL, - client->view->queryacl, + client->view->cacheacl, ISC_TRUE); if (result == ISC_R_SUCCESS) { /* - * We were allowed by the default - * "allow-query" ACL. Remember this so we - * don't have to check again. + * We were allowed by the "allow-query-cache" ACL. + * Remember this so we don't have to check again. */ client->query.attributes |= - NS_QUERYATTR_QUERYOK; + NS_QUERYATTR_CACHEACLOK; if (log && isc_log_wouldlog(ns_g_lctx, ISC_LOG_DEBUG(3))) { @@ -876,9 +873,9 @@ query_getcachedb(ns_client_t *client, dns_name_t *name, dns_rdatatype_t qtype, } /* * We've now evaluated the view's query ACL, and - * the NS_QUERYATTR_QUERYOK attribute is now valid. + * the NS_QUERYATTR_CACHEACLOKVALID attribute is now valid. */ - client->query.attributes |= NS_QUERYATTR_QUERYOKVALID; + client->query.attributes |= NS_QUERYATTR_CACHEACLOKVALID; if (result != ISC_R_SUCCESS) goto refuse; diff --git a/bin/named/server.c b/bin/named/server.c index 04694b7437..42967757ad 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -15,7 +15,7 @@ * PERFORMANCE OF THIS SOFTWARE. */ -/* $Id: server.c,v 1.583 2010/08/24 01:00:31 marka Exp $ */ +/* $Id: server.c,v 1.584 2010/09/24 05:09:02 marka Exp $ */ /*! \file */ @@ -1444,6 +1444,9 @@ configure_view(dns_view_t *view, cfg_parser_t* parser, dns_acache_setcachesize(view->acache, max_acache_size); } + CHECK(configure_view_acl(NULL, ns_g_config, "allow-query", NULL, actx, + ns_g_mctx, &view->queryacl)); + /* * Configure the zones. */ @@ -2051,16 +2054,14 @@ configure_view(dns_view_t *view, cfg_parser_t* parser, * "allow-recursion", and "allow-recursion-on" acls if * configured in named.conf. */ - if (view->queryacl == NULL) - CHECK(configure_view_acl(vconfig, config, - "allow-query-cache", NULL, - actx, ns_g_mctx, &view->queryacl)); + CHECK(configure_view_acl(vconfig, config, "allow-query-cache", NULL, + actx, ns_g_mctx, &view->cacheacl)); CHECK(configure_view_acl(vconfig, config, "allow-query-cache-on", NULL, - actx, ns_g_mctx, &view->queryonacl)); - if (view->queryonacl == NULL) + actx, ns_g_mctx, &view->cacheonacl)); + if (view->cacheonacl == NULL) CHECK(configure_view_acl(NULL, ns_g_config, "allow-query-cache-on", NULL, actx, - ns_g_mctx, &view->queryonacl)); + ns_g_mctx, &view->cacheonacl)); if (strcmp(view->name, "_bind") != 0) { CHECK(configure_view_acl(vconfig, config, "allow-recursion", NULL, actx, ns_g_mctx, @@ -2076,14 +2077,20 @@ configure_view(dns_view_t *view, cfg_parser_t* parser, * "allow-recursion" inherits from "allow-query-cache" if set, * otherwise from "allow-query" if set. */ - if (view->queryacl == NULL && view->recursionacl != NULL) - dns_acl_attach(view->recursionacl, &view->queryacl); - if (view->queryacl == NULL && view->recursion) + if (view->cacheacl == NULL && view->recursionacl != NULL) + dns_acl_attach(view->recursionacl, &view->cacheacl); + /* + * XXXEACH: This call to configure_view_acl() is redundant. We + * are leaving it as it is because we are making a minimal change + * for a patch release. In the future this should be changed to + * dns_acl_attach(view->queryacl, &view->cacheacl). + */ + if (view->cacheacl == NULL && view->recursion) CHECK(configure_view_acl(vconfig, config, "allow-query", NULL, - actx, ns_g_mctx, &view->queryacl)); + actx, ns_g_mctx, &view->cacheacl)); if (view->recursion && - view->recursionacl == NULL && view->queryacl != NULL) - dns_acl_attach(view->queryacl, &view->recursionacl); + view->recursionacl == NULL && view->cacheacl != NULL) + dns_acl_attach(view->cacheacl, &view->recursionacl); /* * Set default "allow-recursion", "allow-recursion-on" and @@ -2099,15 +2106,14 @@ configure_view(dns_view_t *view, cfg_parser_t* parser, "allow-recursion-on", NULL, actx, ns_g_mctx, &view->recursiononacl)); - if (view->queryacl == NULL) { + if (view->cacheacl == NULL) { if (view->recursion) CHECK(configure_view_acl(NULL, ns_g_config, "allow-query-cache", NULL, actx, ns_g_mctx, - &view->queryacl)); - else if (view->new_zone_file == NULL) { - CHECK(dns_acl_none(ns_g_mctx, &view->queryacl)); - } + &view->cacheacl)); + else + CHECK(dns_acl_none(mctx, &view->cacheacl)); } /* diff --git a/bin/tests/system/addzone/ns2/named2.conf b/bin/tests/system/addzone/ns2/named2.conf index bb4a89e717..0ae37d9389 100644 --- a/bin/tests/system/addzone/ns2/named2.conf +++ b/bin/tests/system/addzone/ns2/named2.conf @@ -14,7 +14,7 @@ * PERFORMANCE OF THIS SOFTWARE. */ -/* $Id: named2.conf,v 1.2 2010/08/11 18:14:19 each Exp $ */ +/* $Id: named2.conf,v 1.3 2010/09/24 05:09:03 marka Exp $ */ controls { /* empty */ }; @@ -31,6 +31,7 @@ options { view internal { match-clients { 10.53.0.2; }; allow-new-zones no; + recursion yes; zone "." { type hint; diff --git a/bin/tests/system/addzone/tests.sh b/bin/tests/system/addzone/tests.sh index 3c98812f78..ccf9d906a9 100644 --- a/bin/tests/system/addzone/tests.sh +++ b/bin/tests/system/addzone/tests.sh @@ -14,7 +14,7 @@ # OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR # PERFORMANCE OF THIS SOFTWARE. -# $Id: tests.sh,v 1.3 2010/09/15 03:32:34 marka Exp $ +# $Id: tests.sh,v 1.4 2010/09/24 05:09:03 marka Exp $ SYSTEMTESTTOP=.. . $SYSTEMTESTTOP/conf.sh @@ -54,11 +54,11 @@ status=`expr $status + $ret` echo "I:adding new zone with missing master file ($n)" ret=0 $DIG $DIGOPTS +all @10.53.0.2 a.missing.example a > dig.out.ns2.pre.$n || ret=1 -grep "status: NOERROR" dig.out.ns2.pre.$n > /dev/null || ret=1 +grep "status: REFUSED" dig.out.ns2.pre.$n > /dev/null || ret=1 $RNDC -c ../common/rndc.conf -s 10.53.0.2 -p 9953 addzone 'missing.example { type master; file "missing.db"; };' 2> rndc.out.ns2.$n grep "file not found" rndc.out.ns2.$n > /dev/null || ret=1 $DIG $DIGOPTS +all @10.53.0.2 a.missing.example a > dig.out.ns2.post.$n || ret=1 -grep "status: NOERROR" dig.out.ns2.post.$n > /dev/null || ret=1 +grep "status: REFUSED" dig.out.ns2.post.$n > /dev/null || ret=1 $PERL ../digcomp.pl dig.out.ns2.pre.$n dig.out.ns2.post.$n || ret=1 n=`expr $n + 1` if [ $ret != 0 ]; then echo "I:failed"; fi @@ -68,7 +68,7 @@ echo "I:deleting previously added zone ($n)" ret=0 $RNDC -c ../common/rndc.conf -s 10.53.0.2 -p 9953 delzone previous.example 2>&1 | sed 's/^/I:ns2 /' $DIG $DIGOPTS @10.53.0.2 a.previous.example a > dig.out.ns2.$n -grep 'status: NOERROR' dig.out.ns2.$n > /dev/null || ret=1 +grep 'status: REFUSED' dig.out.ns2.$n > /dev/null || ret=1 grep '^a.previous.example' dig.out.ns2.$n > /dev/null && ret=1 n=`expr $n + 1` if [ $ret != 0 ]; then echo "I:failed"; fi @@ -78,7 +78,7 @@ echo "I:deleting newly added zone ($n)" ret=0 $RNDC -c ../common/rndc.conf -s 10.53.0.2 -p 9953 delzone added.example 2>&1 | sed 's/^/I:ns2 /' $DIG $DIGOPTS @10.53.0.2 a.added.example a > dig.out.ns2.$n -grep 'status: NOERROR' dig.out.ns2.$n > /dev/null || ret=1 +grep 'status: REFUSED' dig.out.ns2.$n > /dev/null || ret=1 grep '^a.added.example' dig.out.ns2.$n > /dev/null && ret=1 n=`expr $n + 1` if [ $ret != 0 ]; then echo "I:failed"; fi @@ -102,11 +102,21 @@ $RNDC -c ../common/rndc.conf -s 10.53.0.2 -p 9953 reconfig 2>&1 | sed 's/^/I:ns2 sleep 5 echo "I:adding new zone to external view ($n)" +# NOTE: The internal view has "recursion yes" set, and so queries for +# nonexistent zones should return NOERROR. The external view is +# "recursion no", so queries for nonexistent zones should return +# REFUSED. This behavior should be the same regardless of whether +# the zone does not exist because a) it has not yet been loaded, b) +# it failed to load, or c) it has been deleted. ret=0 +$DIG +norec $DIGOPTS @10.53.0.2 -b 10.53.0.2 a.added.example a > dig.out.ns2.intpre.$n || ret=1 +grep 'status: NOERROR' dig.out.ns2.intpre.$n > /dev/null || ret=1 +$DIG +norec $DIGOPTS @10.53.0.4 -b 10.53.0.4 a.added.example a > dig.out.ns2.extpre.$n || ret=1 +grep 'status: REFUSED' dig.out.ns2.extpre.$n > /dev/null || ret=1 $RNDC -c ../common/rndc.conf -s 10.53.0.2 -p 9953 addzone 'added.example in external { type master; file "added.db"; };' 2>&1 | sed 's/^/I:ns2 /' -$DIG $DIGOPTS @10.53.0.2 -b 10.53.0.2 a.added.example a > dig.out.ns2.int.$n || ret=1 -$DIG $DIGOPTS @10.53.0.4 -b 10.53.0.4 a.added.example a > dig.out.ns2.ext.$n || ret=1 -grep 'status: REFUSED' dig.out.ns2.int.$n > /dev/null || ret=1 +$DIG +norec $DIGOPTS @10.53.0.2 -b 10.53.0.2 a.added.example a > dig.out.ns2.int.$n || ret=1 +grep 'status: NOERROR' dig.out.ns2.int.$n > /dev/null || ret=1 +$DIG +norec $DIGOPTS @10.53.0.4 -b 10.53.0.4 a.added.example a > dig.out.ns2.ext.$n || ret=1 grep 'status: NOERROR' dig.out.ns2.ext.$n > /dev/null || ret=1 grep '^a.added.example' dig.out.ns2.ext.$n > /dev/null || ret=1 n=`expr $n + 1` @@ -117,7 +127,7 @@ echo "I:deleting newly added zone ($n)" ret=0 $RNDC -c ../common/rndc.conf -s 10.53.0.2 -p 9953 delzone 'added.example in external' 2>&1 | sed 's/^/I:ns2 /' $DIG $DIGOPTS @10.53.0.4 -b 10.53.0.4 a.added.example a > dig.out.ns2.$n || ret=1 -grep 'status: NOERROR' dig.out.ns2.$n > /dev/null || ret=1 +grep 'status: REFUSED' dig.out.ns2.$n > /dev/null || ret=1 grep '^a.added.example' dig.out.ns2.$n > /dev/null && ret=1 n=`expr $n + 1` if [ $ret != 0 ]; then echo "I:failed"; fi @@ -125,10 +135,14 @@ status=`expr $status + $ret` echo "I:attempting to add zone to internal view ($n)" ret=0 +$DIG +norec $DIGOPTS @10.53.0.2 -b 10.53.0.2 a.added.example a > dig.out.ns2.pre.$n || ret=1 +grep 'status: NOERROR' dig.out.ns2.pre.$n > /dev/null || ret=1 $RNDC -c ../common/rndc.conf -s 10.53.0.2 -p 9953 addzone 'added.example in internal { type master; file "added.db"; };' 2> rndc.out.ns2.$n grep "permission denied" rndc.out.ns2.$n > /dev/null || ret=1 -$DIG $DIGOPTS @10.53.0.2 -b 10.53.0.2 a.added.example a > dig.out.ns2.$n || ret=1 -grep 'status: REFUSED' dig.out.ns2.$n > /dev/null || ret=1 +$DIG $DIGOPTS @10.53.0.2 -b 10.53.0.2 a.added.example a > dig.out.ns2.int.$n || ret=1 +grep 'status: NOERROR' dig.out.ns2.int.$n > /dev/null || ret=1 +$DIG $DIGOPTS @10.53.0.4 -b 10.53.0.4 a.added.example a > dig.out.ns2.ext.$n || ret=1 +grep 'status: REFUSED' dig.out.ns2.ext.$n > /dev/null || ret=1 n=`expr $n + 1` if [ $ret != 0 ]; then echo "I:failed"; fi status=`expr $status + $ret` diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h index 4adfbd2ad4..6835910f3f 100644 --- a/lib/dns/include/dns/view.h +++ b/lib/dns/include/dns/view.h @@ -15,7 +15,7 @@ * PERFORMANCE OF THIS SOFTWARE. */ -/* $Id: view.h,v 1.126 2010/08/11 18:14:19 each Exp $ */ +/* $Id: view.h,v 1.127 2010/09/24 05:09:03 marka Exp $ */ #ifndef DNS_VIEW_H #define DNS_VIEW_H 1 @@ -125,6 +125,8 @@ struct dns_view { isc_boolean_t enablevalidation; isc_boolean_t acceptexpired; dns_transfer_format_t transfer_format; + dns_acl_t * cacheacl; + dns_acl_t * cacheonacl; dns_acl_t * queryacl; dns_acl_t * queryonacl; dns_acl_t * recursionacl; diff --git a/lib/dns/view.c b/lib/dns/view.c index 3b71a4bcec..8ca7513661 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -15,7 +15,7 @@ * PERFORMANCE OF THIS SOFTWARE. */ -/* $Id: view.c,v 1.169 2010/09/06 04:31:11 marka Exp $ */ +/* $Id: view.c,v 1.170 2010/09/24 05:09:03 marka Exp $ */ /*! \file */ @@ -158,6 +158,8 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, view->acceptexpired = ISC_FALSE; view->minimalresponses = ISC_FALSE; view->transfer_format = dns_one_answer; + view->cacheacl = NULL; + view->cacheonacl = NULL; view->queryacl = NULL; view->queryonacl = NULL; view->recursionacl = NULL; @@ -298,6 +300,10 @@ destroy(dns_view_t *view) { dns_acl_detach(&view->matchclients); if (view->matchdestinations != NULL) dns_acl_detach(&view->matchdestinations); + if (view->cacheacl != NULL) + dns_acl_detach(&view->cacheacl); + if (view->cacheonacl != NULL) + dns_acl_detach(&view->cacheonacl); if (view->queryacl != NULL) dns_acl_detach(&view->queryacl); if (view->queryonacl != NULL)