From 93b8bd39145566053ad8b22cef597146e9175ea4 Mon Sep 17 00:00:00 2001

From: Evan Hunt <each@isc.org>

Date: Tue, 8 Nov 2022 17:32:41 -0800

Subject: [PATCH] move update ACL and update-policy checks before quota

check allow-update, update-policy, and allow-update-forwarding before

consuming quota slots, so that unauthorized clients can't fill the

quota.

(this moves the access check before the prerequisite check, which

violates the precise wording of RFC 2136. however, RFC co-author Paul

Vixie has stated that the RFC is mistaken on this point; it should have

said that access checking must happen *no later than* the completion of

prerequisite checks, not that it must happen exactly then.)

(cherry picked from commit 964f559edb5036880b8e463b8f190b9007ee055d)

---

 lib/ns/update.c | 335 ++++++++++++++++++++++++++----------------------

 1 file changed, 181 insertions(+), 154 deletions(-)

diff --git a/lib/ns/update.c b/lib/ns/update.c

index 9a8c309..036184b 100644

--- a/lib/ns/update.c

+++ b/lib/ns/update.c

@@ -261,6 +261,9 @@ static void

 forward_done(isc_task_t *task, isc_event_t *event);

 static isc_result_t

 add_rr_prepare_action(void *data, rr_t *rr);

+static isc_result_t

+rr_exists(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name,

+	  const dns_rdata_t *rdata, bool *flag);

 /**************************************************************************/

@@ -333,25 +336,26 @@ inc_stats(ns_client_t *client, dns_zone_t *zone, isc_statscounter_t counter) {

 static isc_result_t

 checkqueryacl(ns_client_t *client, dns_acl_t *queryacl, dns_name_t *zonename,

 	      dns_acl_t *updateacl, dns_ssutable_t *ssutable) {

+	isc_result_t result;

 	char namebuf[DNS_NAME_FORMATSIZE];

 	char classbuf[DNS_RDATACLASS_FORMATSIZE];

-	int level;

-	isc_result_t result;

+	bool update_possible =

+		((updateacl != NULL && !dns_acl_isnone(updateacl)) ||

+		 ssutable != NULL);

 	result = ns_client_checkaclsilent(client, NULL, queryacl, true);

 	if (result != ISC_R_SUCCESS) {

+		int level = update_possible ? ISC_LOG_ERROR : ISC_LOG_INFO;

+

 		dns_name_format(zonename, namebuf, sizeof(namebuf));

 		dns_rdataclass_format(client->view->rdclass, classbuf,

 				      sizeof(classbuf));

-		level = (updateacl == NULL && ssutable == NULL) ? ISC_LOG_INFO

-								: ISC_LOG_ERROR;

-

 		ns_client_log(client, NS_LOGCATEGORY_UPDATE_SECURITY,

 			      NS_LOGMODULE_UPDATE, level,

 			      "update '%s/%s' denied due to allow-query",

 			      namebuf, classbuf);

-	} else if (updateacl == NULL && ssutable == NULL) {

+	} else if (!update_possible) {

 		dns_name_format(zonename, namebuf, sizeof(namebuf));

 		dns_rdataclass_format(client->view->rdclass, classbuf,

 				      sizeof(classbuf));

@@ -1543,6 +1547,156 @@ send_update_event(ns_client_t *client, dns_zone_t *zone) {

 	isc_result_t result = ISC_R_SUCCESS;

 	update_event_t *event = NULL;

 	isc_task_t *zonetask = NULL;

+	dns_ssutable_t *ssutable = NULL;

+	dns_message_t *request = client->message;

+	dns_aclenv_t *env =

+		ns_interfacemgr_getaclenv(client->manager->interface->mgr);

+	dns_rdataclass_t zoneclass;

+	dns_rdatatype_t covers;

+	dns_name_t *zonename = NULL;

+	dns_db_t *db = NULL;

+	dns_dbversion_t *ver = NULL;

+

+	CHECK(dns_zone_getdb(zone, &db);;

+	zonename = dns_db_origin(db);

+	zoneclass = dns_db_class(db);

+	dns_zone_getssutable(zone, &ssutable);

+	dns_db_currentversion(db, &ver);

+

+	/*

+	 * Update message processing can leak record existence information

+	 * so check that we are allowed to query this zone.  Additionally,

+	 * if we would refuse all updates for this zone, we bail out here.

+	 */

+	CHECK(checkqueryacl(client, dns_zone_getqueryacl(zone),

+			    dns_zone_getorigin(zone),

+			    dns_zone_getupdateacl(zone), ssutable));

+

+	/*

+	 * Check requestor's permissions.

+	 */

+	if (ssutable == NULL) {

+		CHECK(checkupdateacl(client, dns_zone_getupdateacl(zone),

+				     "update", dns_zone_getorigin(zone), false,

+				     false));

+	} else if (client->signer == NULL && !TCPCLIENT(client)) {

+		CHECK(checkupdateacl(client, NULL, "update",

+				     dns_zone_getorigin(zone), false, true));

+	}

+

+	if (dns_zone_getupdatedisabled(zone)) {

+		FAILC(DNS_R_REFUSED, "dynamic update temporarily disabled "

+				     "because the zone is frozen.  Use "

+				     "'rndc thaw' to re-enable updates.");

+	}

+

+	/*

+	 * Prescan the update section, checking for updates that

+	 * are illegal or violate policy.

+	 */

+	for (result = dns_message_firstname(request, DNS_SECTION_UPDATE);

+	     result == ISC_R_SUCCESS;

+	     result = dns_message_nextname(request, DNS_SECTION_UPDATE))

+	{

+		dns_name_t *name = NULL;

+		dns_rdata_t rdata = DNS_RDATA_INIT;

+		dns_ttl_t ttl;

+		dns_rdataclass_t update_class;

+

+		get_current_rr(request, DNS_SECTION_UPDATE, zoneclass, &name,

+			       &rdata, &covers, &ttl, &update_class);

+

+		if (!dns_name_issubdomain(name, zonename)) {

+			FAILC(DNS_R_NOTZONE, "update RR is outside zone");

+		}

+		if (update_class == zoneclass) {

+			/*

+			 * Check for meta-RRs.  The RFC2136 pseudocode says

+			 * check for ANY|AXFR|MAILA|MAILB, but the text adds

+			 * "or any other QUERY metatype"

+			 */

+			if (dns_rdatatype_ismeta(rdata.type)) {

+				FAILC(DNS_R_FORMERR, "meta-RR in update");

+			}

+			result = dns_zone_checknames(zone, name, &rdata);

+			if (result != ISC_R_SUCCESS) {

+				FAIL(DNS_R_REFUSED);

+			}

+		} else if (update_class == dns_rdataclass_any) {

+			if (ttl != 0 || rdata.length != 0 ||

+			    (dns_rdatatype_ismeta(rdata.type) &&

+			     rdata.type != dns_rdatatype_any))

+			{

+				FAILC(DNS_R_FORMERR, "meta-RR in update");

+			}

+		} else if (update_class == dns_rdataclass_none) {

+			if (ttl != 0 || dns_rdatatype_ismeta(rdata.type)) {

+				FAILC(DNS_R_FORMERR, "meta-RR in update");

+			}

+		} else {

+			update_log(client, zone, ISC_LOG_WARNING,

+				   "update RR has incorrect class %d",

+				   update_class);

+			FAIL(DNS_R_FORMERR);

+		}

+

+		/*

+		 * draft-ietf-dnsind-simple-secure-update-01 says

+		 * "Unlike traditional dynamic update, the client

+		 * is forbidden from updating NSEC records."

+		 */

+		if (rdata.type == dns_rdatatype_nsec3) {

+			FAILC(DNS_R_REFUSED, "explicit NSEC3 updates are not "

+					     "allowed "

+					     "in secure zones");

+		} else if (rdata.type == dns_rdatatype_nsec) {

+			FAILC(DNS_R_REFUSED, "explicit NSEC updates are not "

+					     "allowed "

+					     "in secure zones");

+		} else if (rdata.type == dns_rdatatype_rrsig &&

+			   !dns_name_equal(name, zonename))

+		{

+			FAILC(DNS_R_REFUSED, "explicit RRSIG updates are "

+					     "currently "

+					     "not supported in secure zones "

+					     "except "

+					     "at the apex");

+		}

+

+		if (ssutable != NULL) {

+			isc_netaddr_t netaddr;

+			dst_key_t *tsigkey = NULL;

+			isc_netaddr_fromsockaddr(&netaddr, &client->peeraddr);

+

+			if (client->message->tsigkey != NULL) {

+				tsigkey = client->message->tsigkey->key;

+			}

+

+			if (rdata.type != dns_rdatatype_any) {

+				if (!dns_ssutable_checkrules(

+					    ssutable, client->signer, name,

+					    &netaddr, TCPCLIENT(client), env,

+					    rdata.type, tsigkey))

+				{

+					FAILC(DNS_R_REFUSED, "rejected by "

+							     "secure update");

+				}

+			} else {

+				if (!ssu_checkall(db, ver, name, ssutable,

+						  client->signer, &netaddr, env,

+						  TCPCLIENT(client), tsigkey))

+				{

+					FAILC(DNS_R_REFUSED, "rejected by "

+							     "secure update");

+				}

+			}

+		}

+	}

+	if (result != ISC_R_NOMORE) {

+		FAIL(result);

+	}

+

+	update_log(client, zone, LOGLEVEL_DEBUG, "update section prescan OK");

 	result = isc_quota_attach(&client->manager->sctx->updquota,

 				  &(isc_quota_t *){ NULL });

@@ -1552,9 +1706,7 @@ send_update_event(ns_client_t *client, dns_zone_t *zone) {

 			   isc_result_totext(result));

 		ns_stats_increment(client->manager->sctx->nsstats,

 				   ns_statscounter_updatequota);

-		ns_client_drop(client, result);

-		isc_nmhandle_detach(&client->reqhandle);

-		return (DNS_R_DROP);

+		CHECK(DNS_R_DROP);

 	}

 	event = (update_event_t *)isc_event_allocate(

@@ -1571,6 +1723,16 @@ send_update_event(ns_client_t *client, dns_zone_t *zone) {

 	dns_zone_gettask(zone, &zonetask);

 	isc_task_send(zonetask, ISC_EVENT_PTR(&event));

+failure:

+	if (db != NULL) {

+		dns_db_closeversion(db, &ver, false);

+		dns_db_detach(&db);

+	}

+

+	if (ssutable != NULL) {

+		dns_ssutable_detach(&ssutable);

+	}

+

 	return (result);

 }

@@ -1671,9 +1833,6 @@ ns_update_start(ns_client_t *client, isc_nmhandle_t *handle,

 		break;

 	case dns_zone_secondary:

 	case dns_zone_mirror:

-		CHECK(checkupdateacl(client, dns_zone_getforwardacl(zone),

-				     "update forwarding", zonename, true,

-				     false));

 		CHECK(send_forward_event(client, zone));

 		break;

 	default:

@@ -1685,8 +1844,6 @@ ns_update_start(ns_client_t *client, isc_nmhandle_t *handle,

 failure:

 	if (result == DNS_R_REFUSED) {

-		INSIST(dns_zone_gettype(zone) == dns_zone_secondary ||

-		       dns_zone_gettype(zone) == dns_zone_mirror);

 		inc_stats(client, zone, ns_statscounter_updaterej);

 	}

@@ -2578,7 +2735,7 @@ update_action(isc_task_t *task, isc_event_t *event) {

 	dns_rdatatype_t covers;

 	dns_message_t *request = client->message;

 	dns_rdataclass_t zoneclass;

-	dns_name_t *zonename;

+	dns_name_t *zonename = NULL;

 	dns_ssutable_t *ssutable = NULL;

 	dns_fixedname_t tmpnamefixed;

 	dns_name_t *tmpname = NULL;

@@ -2590,8 +2747,6 @@ update_action(isc_task_t *task, isc_event_t *event) {

 	dns_ttl_t maxttl = 0;

 	uint32_t maxrecords;

 	uint64_t records;

-	dns_aclenv_t *env =

-		ns_interfacemgr_getaclenv(client->manager->interface->mgr);

 	INSIST(event->ev_type == DNS_EVENT_UPDATE);

@@ -2602,14 +2757,7 @@ update_action(isc_task_t *task, isc_event_t *event) {

 	zonename = dns_db_origin(db);

 	zoneclass = dns_db_class(db);

 	dns_zone_getssutable(zone, &ssutable);

-

-	/*

-	 * Update message processing can leak record existence information

-	 * so check that we are allowed to query this zone.  Additionally

-	 * if we would refuse all updates for this zone we bail out here.

-	 */

-	CHECK(checkqueryacl(client, dns_zone_getqueryacl(zone), zonename,

-			    dns_zone_getupdateacl(zone), ssutable));

+	options = dns_zone_getoptions(zone);

 	/*

 	 * Get old and new versions now that queryacl has been checked.

@@ -2745,135 +2893,10 @@ update_action(isc_task_t *task, isc_event_t *event) {

 	update_log(client, zone, LOGLEVEL_DEBUG, "prerequisites are OK");

-	/*

-	 * Check Requestor's Permissions.  It seems a bit silly to do this

-	 * only after prerequisite testing, but that is what RFC2136 says.

-	 */

-	if (ssutable == NULL) {

-		CHECK(checkupdateacl(client, dns_zone_getupdateacl(zone),

-				     "update", zonename, false, false));

-	} else if (client->signer == NULL && !TCPCLIENT(client)) {

-		CHECK(checkupdateacl(client, NULL, "update", zonename, false,

-				     true));

-	}

-

-	if (dns_zone_getupdatedisabled(zone)) {

-		FAILC(DNS_R_REFUSED, "dynamic update temporarily disabled "

-				     "because the zone is frozen.  Use "

-				     "'rndc thaw' to re-enable updates.");

-	}

-

-	/*

-	 * Perform the Update Section Prescan.

-	 */

-

-	for (result = dns_message_firstname(request, DNS_SECTION_UPDATE);

-	     result == ISC_R_SUCCESS;

-	     result = dns_message_nextname(request, DNS_SECTION_UPDATE))

-	{

-		dns_name_t *name = NULL;

-		dns_rdata_t rdata = DNS_RDATA_INIT;

-		dns_ttl_t ttl;

-		dns_rdataclass_t update_class;

-		get_current_rr(request, DNS_SECTION_UPDATE, zoneclass, &name,

-			       &rdata, &covers, &ttl, &update_class);

-

-		if (!dns_name_issubdomain(name, zonename)) {

-			FAILC(DNS_R_NOTZONE, "update RR is outside zone");

-		}

-		if (update_class == zoneclass) {

-			/*

-			 * Check for meta-RRs.  The RFC2136 pseudocode says

-			 * check for ANY|AXFR|MAILA|MAILB, but the text adds

-			 * "or any other QUERY metatype"

-			 */

-			if (dns_rdatatype_ismeta(rdata.type)) {

-				FAILC(DNS_R_FORMERR, "meta-RR in update");

-			}

-			result = dns_zone_checknames(zone, name, &rdata);

-			if (result != ISC_R_SUCCESS) {

-				FAIL(DNS_R_REFUSED);

-			}

-		} else if (update_class == dns_rdataclass_any) {

-			if (ttl != 0 || rdata.length != 0 ||

-			    (dns_rdatatype_ismeta(rdata.type) &&

-			     rdata.type != dns_rdatatype_any))

-			{

-				FAILC(DNS_R_FORMERR, "meta-RR in update");

-			}

-		} else if (update_class == dns_rdataclass_none) {

-			if (ttl != 0 || dns_rdatatype_ismeta(rdata.type)) {

-				FAILC(DNS_R_FORMERR, "meta-RR in update");

-			}

-		} else {

-			update_log(client, zone, ISC_LOG_WARNING,

-				   "update RR has incorrect class %d",

-				   update_class);

-			FAIL(DNS_R_FORMERR);

-		}

-

-		/*

-		 * draft-ietf-dnsind-simple-secure-update-01 says

-		 * "Unlike traditional dynamic update, the client

-		 * is forbidden from updating NSEC records."

-		 */

-		if (rdata.type == dns_rdatatype_nsec3) {

-			FAILC(DNS_R_REFUSED, "explicit NSEC3 updates are not "

-					     "allowed "

-					     "in secure zones");

-		} else if (rdata.type == dns_rdatatype_nsec) {

-			FAILC(DNS_R_REFUSED, "explicit NSEC updates are not "

-					     "allowed "

-					     "in secure zones");

-		} else if (rdata.type == dns_rdatatype_rrsig &&

-			   !dns_name_equal(name, zonename)) {

-			FAILC(DNS_R_REFUSED, "explicit RRSIG updates are "

-					     "currently "

-					     "not supported in secure zones "

-					     "except "

-					     "at the apex");

-		}

-

-		if (ssutable != NULL) {

-			isc_netaddr_t netaddr;

-			dst_key_t *tsigkey = NULL;

-			isc_netaddr_fromsockaddr(&netaddr, &client->peeraddr);

-

-			if (client->message->tsigkey != NULL) {

-				tsigkey = client->message->tsigkey->key;

-			}

-

-			if (rdata.type != dns_rdatatype_any) {

-				if (!dns_ssutable_checkrules(

-					    ssutable, client->signer, name,

-					    &netaddr, TCPCLIENT(client), env,

-					    rdata.type, tsigkey))

-				{

-					FAILC(DNS_R_REFUSED, "rejected by "

-							     "secure update");

-				}

-			} else {

-				if (!ssu_checkall(db, ver, name, ssutable,

-						  client->signer, &netaddr, env,

-						  TCPCLIENT(client), tsigkey))

-				{

-					FAILC(DNS_R_REFUSED, "rejected by "

-							     "secure update");

-				}

-			}

-		}

-	}

-	if (result != ISC_R_NOMORE) {

-		FAIL(result);

-	}

-

-	update_log(client, zone, LOGLEVEL_DEBUG, "update section prescan OK");

-

 	/*

 	 * Process the Update Section.

 	 */

-	options = dns_zone_getoptions(zone);

 	for (result = dns_message_firstname(request, DNS_SECTION_UPDATE);

 	     result == ISC_R_SUCCESS;

 	     result = dns_message_nextname(request, DNS_SECTION_UPDATE))

@@ -3307,10 +3330,7 @@ update_action(isc_task_t *task, isc_event_t *event) {

 			if (result == ISC_R_SUCCESS && records > maxrecords) {

 				update_log(client, zone, ISC_LOG_ERROR,

 					   "records in zone (%" PRIu64 ") "

-					   "exceeds"

-					   " max-"

-					   "records"

-					   " (%u)",

+					   "exceeds max-records (%u)",

 					   records, maxrecords);

 				result = DNS_R_TOOMANYRECORDS;

 				goto failure;

@@ -3601,6 +3621,13 @@ send_forward_event(ns_client_t *client, dns_zone_t *zone) {

 	update_event_t *event = NULL;

 	isc_task_t *zonetask = NULL;

+	result = checkupdateacl(client, dns_zone_getforwardacl(zone),

+				"update forwarding", dns_zone_getorigin(zone),

+				true, false);

+	if (result != ISC_R_SUCCESS) {

+		return (result);

+	}

+

 	result = isc_quota_attach(&client->manager->sctx->updquota,

 				  &(isc_quota_t *){ NULL });

 	if (result != ISC_R_SUCCESS) {

-- 

2.39.1