From e3ee138098897e0f53b2950dfe8a234543ec134e Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 17 Jun 2020 15:30:59 -0700 Subject: [PATCH 1/3] update the acl system test to include a blackhole test case this ACL was previously untested, which allowed a regression to go undetected. --- bin/tests/system/acl/ns2/named5.conf.in | 1 + bin/tests/system/acl/tests.sh | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/bin/tests/system/acl/ns2/named5.conf.in b/bin/tests/system/acl/ns2/named5.conf.in index 4b4e05027a..7e20bac49d 100644 --- a/bin/tests/system/acl/ns2/named5.conf.in +++ b/bin/tests/system/acl/ns2/named5.conf.in @@ -31,6 +31,7 @@ options { ixfr-from-differences yes; check-integrity no; allow-query-on { 10.53.0.2; }; + blackhole { 10.53.0.8; }; }; key one { diff --git a/bin/tests/system/acl/tests.sh b/bin/tests/system/acl/tests.sh index 6f291bfc61..4d915244ba 100644 --- a/bin/tests/system/acl/tests.sh +++ b/bin/tests/system/acl/tests.sh @@ -143,6 +143,26 @@ $DIG -p ${PORT} +tcp soa example. \ @10.53.0.2 -b 10.53.0.3 > dig.out.${t} grep "status: NOERROR" dig.out.${t} > /dev/null 2>&1 || { echo_i "test $t failed" ; status=1; } +echo_i "testing blackhole ACL processing" +t=`expr $t + 1` +ret=0 +$DIG -p ${PORT} +tcp soa example. \ + @10.53.0.2 -b 10.53.0.3 > dig.out.1.${t} +grep "status: NOERROR" dig.out.1.${t} > /dev/null 2>&1 || ret=1 +$DIG -p ${PORT} +tcp soa example. \ + @10.53.0.2 -b 10.53.0.8 > dig.out.2.${t} +grep "status: NOERROR" dig.out.2.${t} > /dev/null 2>&1 && ret=1 +grep "communications error" dig.out.2.${t} > /dev/null 2>&1 || ret=1 +$DIG -p ${PORT} soa example. \ + @10.53.0.2 -b 10.53.0.3 > dig.out.3.${t} +grep "status: NOERROR" dig.out.3.${t} > /dev/null 2>&1 || ret=1 +$DIG -p ${PORT} soa example. \ + @10.53.0.2 -b 10.53.0.8 > dig.out.4.${t} +grep "status: NOERROR" dig.out.4.${t} > /dev/null 2>&1 && ret=1 +grep "connection timed out" dig.out.4.${t} > /dev/null 2>&1 || ret=1 +[ $ret -eq 0 ] || echo_i "failed" +status=`expr $status + $ret` + # AXFR tests against ns3 echo_i "testing allow-transfer ACLs against ns3 (no existing zones)" From 23c7373d68d2b5aca838aab2ebb16e1c9156ed60 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 17 Jun 2020 12:09:10 -0700 Subject: [PATCH 2/3] restore "blackhole" functionality the blackhole ACL was accidentally disabled with respect to client queries during the netmgr conversion. in order to make this work for TCP, it was necessary to add a return code to the accept callback functions passed to isc_nm_listentcp() and isc_nm_listentcpdns(). --- lib/isc/include/isc/netmgr.h | 87 +++++++++++++++++++------------- lib/isc/netmgr/netmgr-int.h | 4 +- lib/isc/netmgr/tcp.c | 11 ++-- lib/isc/netmgr/tcpdns.c | 21 +++++--- lib/ns/client.c | 58 ++++++++++++--------- lib/ns/include/ns/client.h | 2 +- lib/ns/include/ns/interfacemgr.h | 6 +++ lib/ns/interfacemgr.c | 16 +++++- lib/ns/win32/libns.def | 1 + 9 files changed, 129 insertions(+), 77 deletions(-) diff --git a/lib/isc/include/isc/netmgr.h b/lib/isc/include/isc/netmgr.h index 4cb5b12289..67d95f3876 100644 --- a/lib/isc/include/isc/netmgr.h +++ b/lib/isc/include/isc/netmgr.h @@ -24,6 +24,47 @@ typedef enum { NMEV_SHUTDOWN } isc_nm_eventtype; +typedef void (*isc_nm_recv_cb_t)(isc_nmhandle_t *handle, isc_result_t eresult, + isc_region_t *region, void *cbarg); +/*%< + * Callback function to be used when receiving a packet. + * + * 'handle' the handle that can be used to send back the answer. + * 'eresult' the result of the event. + * 'region' contains the received data, if any. It will be freed + * after return by caller. + * 'cbarg' the callback argument passed to isc_nm_listenudp(), + * isc_nm_listentcpdns(), or isc_nm_read(). + */ +typedef isc_result_t (*isc_nm_accept_cb_t)(isc_nmhandle_t *handle, + isc_result_t result, void *cbarg); +/*%< + * Callback function to be used when accepting a connection. (This differs + * from isc_nm_cb_t below in that it returns a result code.) + * + * 'handle' the handle that can be used to send back the answer. + * 'eresult' the result of the event. + * 'cbarg' the callback argument passed to isc_nm_listentcp() or + * isc_nm_listentcpdns(). + */ + +typedef void (*isc_nm_cb_t)(isc_nmhandle_t *handle, isc_result_t result, + void *cbarg); +/*%< + * Callback function for other network completion events (send, connect). + * + * 'handle' the handle on which the event took place. + * 'eresult' the result of the event. + * 'cbarg' the callback argument passed to isc_nm_send(), + * isc_nm_tcp_connect(), or isc_nm_listentcp() + */ + +typedef void (*isc_nm_opaquecb_t)(void *arg); +/*%< + * Opaque callback function, used for isc_nmhandle 'reset' and 'free' + * callbacks. + */ + isc_nm_t * isc_nm_start(isc_mem_t *mctx, uint32_t workers); /*%< @@ -89,8 +130,6 @@ isc_nmhandle_getdata(isc_nmhandle_t *handle); void * isc_nmhandle_getextra(isc_nmhandle_t *handle); -typedef void (*isc_nm_opaquecb_t)(void *arg); - bool isc_nmhandle_is_stream(isc_nmhandle_t *handle); @@ -122,31 +161,6 @@ isc_nmhandle_netmgr(isc_nmhandle_t *handle); * Return a pointer to the netmgr object for the given handle. */ -typedef void (*isc_nm_recv_cb_t)(isc_nmhandle_t *handle, isc_result_t eresult, - isc_region_t *region, void *cbarg); -/*%< - * Callback function to be used when receiving a packet. - * - * 'handle' the handle that can be used to send back the answer. - * 'eresult' the result of the event. - * 'region' contains the received data, if any. It will be freed - * after return by caller. - * 'cbarg' the callback argument passed to isc_nm_listenudp(), - * isc_nm_listentcpdns(), or isc_nm_read(). - */ - -typedef void (*isc_nm_cb_t)(isc_nmhandle_t *handle, isc_result_t eresult, - void *cbarg); -/*%< - * Callback function for other network completion events (send, connect, - * accept). - * - * 'handle' the handle on which the event took place. - * 'eresult' the result of the event. - * 'cbarg' the callback argument passed to isc_nm_send(), - * isc_nm_tcp_connect(), or isc_nm_listentcp() - */ - isc_result_t isc_nm_listenudp(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_recv_cb_t cb, void *cbarg, size_t extrasize, isc_nmsocket_t **sockp); @@ -226,9 +240,10 @@ isc_nm_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb, */ isc_result_t -isc_nm_listentcp(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_cb_t cb, - void *cbarg, size_t extrahandlesize, int backlog, - isc_quota_t *quota, isc_nmsocket_t **sockp); +isc_nm_listentcp(isc_nm_t *mgr, isc_nmiface_t *iface, + isc_nm_accept_cb_t accept_cb, void *accept_cbarg, + size_t extrahandlesize, int backlog, isc_quota_t *quota, + isc_nmsocket_t **sockp); /*%< * Start listening for raw messages over the TCP interface 'iface', using * net manager 'mgr'. @@ -236,8 +251,8 @@ isc_nm_listentcp(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_cb_t cb, * On success, 'sockp' will be updated to contain a new listening TCP * socket. * - * When a message is received on the socket, 'cb' will be called with 'cbarg' - * as its argument. + * When connection is accepted on the socket, 'accept_cb' will be called with + * 'accept_cbarg' as its argument. The callback is expected to start a read. * * When handles are allocated for the socket, 'extrasize' additional bytes * will be allocated along with the handle for an associated object. @@ -267,9 +282,9 @@ isc_nm_tcpconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer, isc_result_t isc_nm_listentcpdns(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_recv_cb_t cb, - void *cbarg, isc_nm_cb_t accept_cb, void *accept_cbarg, - size_t extrahandlesize, int backlog, isc_quota_t *quota, - isc_nmsocket_t **sockp); + void *cbarg, isc_nm_accept_cb_t accept_cb, + void *accept_cbarg, size_t extrahandlesize, int backlog, + isc_quota_t *quota, isc_nmsocket_t **sockp); /*%< * Start listening for DNS messages over the TCP interface 'iface', using * net manager 'mgr'. @@ -283,7 +298,7 @@ isc_nm_listentcpdns(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_recv_cb_t cb, * When a complete DNS message is received on the socket, 'cb' will be * called with 'cbarg' as its argument. * - * When a new TCP connection is accepted, 'accept_cb' will be called + * When a new TCPDNS connection is accepted, 'accept_cb' will be called * with 'accept_cbarg' as its argument. * * When handles are allocated for the socket, 'extrasize' additional bytes diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 44938e7c56..1637741f51 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -150,7 +150,7 @@ typedef enum isc__netievent_type { */ typedef union { isc_nm_recv_cb_t recv; - isc_nm_cb_t accept; + isc_nm_accept_cb_t accept; } isc__nm_readcb_t; typedef union { @@ -160,7 +160,7 @@ typedef union { typedef union { isc_nm_recv_cb_t recv; - isc_nm_cb_t accept; + isc_nm_accept_cb_t accept; isc_nm_cb_t send; isc_nm_cb_t connect; } isc__nm_cb_t; diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index e3e222dead..5710e38bae 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -243,9 +243,10 @@ isc_nm_tcpconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer, } isc_result_t -isc_nm_listentcp(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_cb_t cb, - void *cbarg, size_t extrahandlesize, int backlog, - isc_quota_t *quota, isc_nmsocket_t **sockp) { +isc_nm_listentcp(isc_nm_t *mgr, isc_nmiface_t *iface, + isc_nm_accept_cb_t accept_cb, void *accept_cbarg, + size_t extrahandlesize, int backlog, isc_quota_t *quota, + isc_nmsocket_t **sockp) { isc_nmsocket_t *nsock = NULL; isc__netievent_tcplisten_t *ievent = NULL; @@ -253,8 +254,8 @@ isc_nm_listentcp(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_cb_t cb, nsock = isc_mem_get(mgr->mctx, sizeof(*nsock)); isc__nmsocket_init(nsock, mgr, isc_nm_tcplistener, iface); - nsock->accept_cb.accept = cb; - nsock->accept_cbarg = cbarg; + nsock->accept_cb.accept = accept_cb; + nsock->accept_cbarg = accept_cbarg; nsock->extrahandlesize = extrahandlesize; nsock->backlog = backlog; nsock->result = ISC_R_SUCCESS; diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index 4e49e47c34..05cc410d68 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -102,7 +102,7 @@ dnstcp_readtimeout(uv_timer_t *timer) { /* * Accept callback for TCP-DNS connection. */ -static void +static isc_result_t dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { isc_nmsocket_t *dnslistensock = (isc_nmsocket_t *)cbarg; isc_nmsocket_t *dnssock = NULL; @@ -110,14 +110,16 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { REQUIRE(VALID_NMSOCK(dnslistensock)); REQUIRE(dnslistensock->type == isc_nm_tcpdnslistener); - /* If accept() was unnsuccessful we can't do anything */ if (result != ISC_R_SUCCESS) { - return; + return (result); } if (dnslistensock->accept_cb.accept != NULL) { - dnslistensock->accept_cb.accept(handle, ISC_R_SUCCESS, - dnslistensock->accept_cbarg); + result = dnslistensock->accept_cb.accept( + handle, ISC_R_SUCCESS, dnslistensock->accept_cbarg); + if (result != ISC_R_SUCCESS) { + return (result); + } } /* We need to create a 'wrapper' dnssocket for this connection */ @@ -144,12 +146,15 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { dnssock->timer_initialized = true; uv_timer_start(&dnssock->timer, dnstcp_readtimeout, dnssock->read_timeout, 0); + isc_nmhandle_ref(handle); result = isc_nm_read(handle, dnslisten_readcb, dnssock); if (result != ISC_R_SUCCESS) { isc_nmhandle_unref(handle); } isc__nmsocket_detach(&dnssock); + + return (ISC_R_SUCCESS); } /* @@ -306,9 +311,9 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult, */ isc_result_t isc_nm_listentcpdns(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_recv_cb_t cb, - void *cbarg, isc_nm_cb_t accept_cb, void *accept_cbarg, - size_t extrahandlesize, int backlog, isc_quota_t *quota, - isc_nmsocket_t **sockp) { + void *cbarg, isc_nm_accept_cb_t accept_cb, + void *accept_cbarg, size_t extrahandlesize, int backlog, + isc_quota_t *quota, isc_nmsocket_t **sockp) { /* A 'wrapper' socket object with outer set to true TCP socket */ isc_nmsocket_t *dnslistensock = isc_mem_get(mgr->mctx, sizeof(*dnslistensock)); diff --git a/lib/ns/client.c b/lib/ns/client.c index d91380aed0..6b00dae889 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -1625,7 +1625,6 @@ void ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region, void *arg) { ns_client_t *client; - bool newclient = false; ns_clientmgr_t *mgr; ns_interface_t *ifp; isc_result_t result; @@ -1724,29 +1723,23 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult, } #endif /* if NS_CLIENT_DROPPORT */ + env = ns_interfacemgr_getaclenv(client->manager->interface->mgr); + if (client->sctx->blackholeacl != NULL && + (dns_acl_match(&netaddr, NULL, client->sctx->blackholeacl, env, + &match, NULL) == ISC_R_SUCCESS) && + match > 0) + { + ns_client_log(client, DNS_LOGCATEGORY_SECURITY, + NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(10), + "dropped request: blackholed peer"); + isc_task_unpause(client->task); + return; + } + ns_client_log(client, NS_LOGCATEGORY_CLIENT, NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(3), "%s request", TCP_CLIENT(client) ? "TCP" : "UDP"); - /* - * Check the blackhole ACL for UDP only, since TCP is done in - * client_newconn. - */ - env = ns_interfacemgr_getaclenv(client->manager->interface->mgr); - if (newclient) { - if (client->sctx->blackholeacl != NULL && - (dns_acl_match(&netaddr, NULL, client->sctx->blackholeacl, - env, &match, NULL) == ISC_R_SUCCESS) && - match > 0) - { - ns_client_log(client, DNS_LOGCATEGORY_SECURITY, - NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(10), - "blackholed UDP datagram"); - isc_task_unpause(client->task); - return; - } - } - result = dns_message_peekheader(buffer, &id, &flags); if (result != ISC_R_SUCCESS) { /* @@ -2213,17 +2206,36 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult, isc_task_unpause(client->task); } -void +isc_result_t ns__client_tcpconn(isc_nmhandle_t *handle, isc_result_t result, void *arg) { - ns_server_t *sctx = (ns_server_t *)arg; + ns_interface_t *ifp = (ns_interface_t *)arg; + dns_aclenv_t *env = ns_interfacemgr_getaclenv(ifp->mgr); + ns_server_t *sctx = ns_interfacemgr_getserver(ifp->mgr); unsigned int tcpquota; + isc_sockaddr_t peeraddr; + isc_netaddr_t netaddr; + int match; - UNUSED(handle); UNUSED(result); + if (handle != NULL) { + peeraddr = isc_nmhandle_peeraddr(handle); + isc_netaddr_fromsockaddr(&netaddr, &peeraddr); + + if (sctx->blackholeacl != NULL && + (dns_acl_match(&netaddr, NULL, sctx->blackholeacl, env, + &match, NULL) == ISC_R_SUCCESS) && + match > 0) + { + return (ISC_R_CONNREFUSED); + } + } + tcpquota = isc_quota_getused(&sctx->tcpquota); ns_stats_update_if_greater(sctx->nsstats, ns_statscounter_tcphighwater, tcpquota); + + return (ISC_R_SUCCESS); } static void diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h index bc6ec751e9..ee5dcef8bc 100644 --- a/lib/ns/include/ns/client.h +++ b/lib/ns/include/ns/client.h @@ -476,7 +476,7 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult, * (Not intended for use outside this module and associated tests.) */ -void +isc_result_t ns__client_tcpconn(isc_nmhandle_t *handle, isc_result_t result, void *arg); /*%< diff --git a/lib/ns/include/ns/interfacemgr.h b/lib/ns/include/ns/interfacemgr.h index e60dc9d412..d55bd1b82f 100644 --- a/lib/ns/include/ns/interfacemgr.h +++ b/lib/ns/include/ns/interfacemgr.h @@ -197,6 +197,12 @@ ns_interfacemgr_dumprecursing(FILE *f, ns_interfacemgr_t *mgr); bool ns_interfacemgr_listeningon(ns_interfacemgr_t *mgr, const isc_sockaddr_t *addr); +ns_server_t * +ns_interfacemgr_getserver(ns_interfacemgr_t *mgr); +/*%< + * Returns the ns_server object associated with the interface manager. + */ + ns_interface_t * ns__interfacemgr_getif(ns_interfacemgr_t *mgr); ns_interface_t * diff --git a/lib/ns/interfacemgr.c b/lib/ns/interfacemgr.c index 30ecbcda29..b343970e3d 100644 --- a/lib/ns/interfacemgr.c +++ b/lib/ns/interfacemgr.c @@ -467,7 +467,7 @@ ns_interface_listentcp(ns_interface_t *ifp) { result = isc_nm_listentcpdns( ifp->mgr->nm, (isc_nmiface_t *)&ifp->addr, ns__client_request, - ifp, ns__client_tcpconn, ifp->mgr->sctx, sizeof(ns_client_t), + ifp, ns__client_tcpconn, ifp, sizeof(ns_client_t), ifp->mgr->backlog, &ifp->mgr->sctx->tcpquota, &ifp->tcplistensocket); if (result != ISC_R_SUCCESS) { @@ -481,7 +481,12 @@ ns_interface_listentcp(ns_interface_t *ifp) { * this is necessary because we are adding to the TCP quota just * by listening. */ - ns__client_tcpconn(NULL, ISC_R_SUCCESS, ifp->mgr->sctx); + result = ns__client_tcpconn(NULL, ISC_R_SUCCESS, ifp); + if (result != ISC_R_SUCCESS) { + isc_log_write(IFMGR_COMMON_LOGARGS, ISC_LOG_ERROR, + "connecting TCP socket: %s", + isc_result_totext(result)); + } #if 0 #ifndef ISC_ALLOW_MAPPED @@ -1267,6 +1272,13 @@ ns_interfacemgr_listeningon(ns_interfacemgr_t *mgr, return (result); } +ns_server_t * +ns_interfacemgr_getserver(ns_interfacemgr_t *mgr) { + REQUIRE(NS_INTERFACEMGR_VALID(mgr)); + + return (mgr->sctx); +} + ns_interface_t * ns__interfacemgr_getif(ns_interfacemgr_t *mgr) { ns_interface_t *head; diff --git a/lib/ns/win32/libns.def b/lib/ns/win32/libns.def index d2b9ad652f..1680e4982d 100644 --- a/lib/ns/win32/libns.def +++ b/lib/ns/win32/libns.def @@ -55,6 +55,7 @@ ns_interfacemgr_create ns_interfacemgr_detach ns_interfacemgr_dumprecursing ns_interfacemgr_getaclenv +ns_interfacemgr_getserver ns_interfacemgr_islistening ns_interfacemgr_listeningon ns_interfacemgr_scan From 08401e38c12594af53073f28119699661c28d519 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Mon, 29 Jun 2020 22:55:13 -0700 Subject: [PATCH 3/3] CHANGES, release note --- CHANGES | 3 +++ doc/notes/notes-current.rst | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/CHANGES b/CHANGES index 19b891cad0..ec4a709c58 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +5452. [bug] The "blackhole" ACL was accidentally disabled with + respect to client queries. [GL #1936] + 5451. [func] Add 'rndc dnssec -status' command. [GL #1612] 5450. [placeholder] diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 7d161d4709..f5c274b16f 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -57,3 +57,8 @@ Bug Fixes unsupported algorithm appeared earlier in the DNSKEY RRset than a supported algorithm. It could also stop if it detected a malformed public key. [GL #1689] + +- The ``blackhole`` ACL was inadvertently disabled with respect to + client queries. Blocked IP addresses were not used for upstream + queries but queries from those addresses could still be answered. + [GL #1936]