Merge branch '1936-blackhole-fix' into 'main'

Resolve "blackhole ACL broken"

Closes #1936

See merge request isc-projects/bind9!3751

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]

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 {

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)"

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]

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

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;

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;

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));

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

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);
 
 /*%<

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 *

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;

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