4885. [security] update-policy rules that otherwise ignore the name

field now require that it be set to "." to ensure
                        that any type list present is properly interpreted.
                        [RT #47126]

(cherry picked from commit ec771bbdc8)

CHANGES

@@ -1,3 +1,8 @@
+4885.	[security]	update-policy rules that otherwise ignore the name
+			field now require that it be set to "." to ensure
+			that any type list present is properly interpreted.
+			[RT #47126]
+
 4884.	[bug]		named could crash on shutdown due to a race between
 			shutdown_server() and ns__client_request(). [RT #47120]
 

bin/named/zoneconf.c

@@ -233,37 +233,10 @@ configure_zone_ssutable(const cfg_obj_t *zconfig, dns_zone_t *zone,
 			INSIST(0);
 
 		str = cfg_obj_asstring(matchtype);
-		if (strcasecmp(str, "name") == 0)
-			mtype = dns_ssumatchtype_name;
-		else if (strcasecmp(str, "subdomain") == 0)
-			mtype = dns_ssumatchtype_subdomain;
-		else if (strcasecmp(str, "wildcard") == 0)
-			mtype = dns_ssumatchtype_wildcard;
-		else if (strcasecmp(str, "self") == 0)
-			mtype = dns_ssumatchtype_self;
-		else if (strcasecmp(str, "selfsub") == 0)
-			mtype = dns_ssumatchtype_selfsub;
-		else if (strcasecmp(str, "selfwild") == 0)
-			mtype = dns_ssumatchtype_selfwild;
-		else if (strcasecmp(str, "ms-self") == 0)
-			mtype = dns_ssumatchtype_selfms;
-		else if (strcasecmp(str, "krb5-self") == 0)
-			mtype = dns_ssumatchtype_selfkrb5;
-		else if (strcasecmp(str, "ms-subdomain") == 0)
-			mtype = dns_ssumatchtype_subdomainms;
-		else if (strcasecmp(str, "krb5-subdomain") == 0)
-			mtype = dns_ssumatchtype_subdomainkrb5;
-		else if (strcasecmp(str, "tcp-self") == 0)
-			mtype = dns_ssumatchtype_tcpself;
-		else if (strcasecmp(str, "6to4-self") == 0)
-			mtype = dns_ssumatchtype_6to4self;
-		else if (strcasecmp(str, "zonesub") == 0) {
-			mtype = dns_ssumatchtype_subdomain;
+		CHECK(dns_ssu_mtypefromstring(str, &mtype));
+		if (mtype == dns_ssumatchtype_subdomain) {
 			usezone = ISC_TRUE;
-		} else if (strcasecmp(str, "external") == 0)
-			mtype = dns_ssumatchtype_external;
-		else
-			INSIST(0);
+		}
 
 		dns_fixedname_init(&fident);
 		str = cfg_obj_asstring(identity);

bin/tests/system/checkconf/bad-update-policy1.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * self TXT;
+	};
+};

bin/tests/system/checkconf/bad-update-policy2.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * selfsub TXT;
+	};
+};

bin/tests/system/checkconf/bad-update-policy3.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * selfwild TXT;
+	};
+};

bin/tests/system/checkconf/bad-update-policy4.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * ms-self TXT;
+	};
+};

bin/tests/system/checkconf/bad-update-policy5.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * krb5-self TXT;
+	};
+};

bin/tests/system/checkconf/bad-update-policy6.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * ms-subdomain TXT;
+	};
+};

bin/tests/system/checkconf/bad-update-policy7.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * krb5-subdomain TXT;
+	};
+};

bin/tests/system/checkconf/bad-update-policy8.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * tcp-self TXT;
+	};
+};

bin/tests/system/checkconf/bad-update-policy9.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * 6to4-self TXT;
+	};
+};

bin/tests/system/checkconf/good-update-policy1.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * self * TXT;
+	};
+};

bin/tests/system/checkconf/good-update-policy10.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * krb5-subdomain . TXT;
+	};
+};

bin/tests/system/checkconf/good-update-policy11.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * tcp-self . TXT;
+	};
+};

bin/tests/system/checkconf/good-update-policy12.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * 6to4-self . TXT;
+	};
+};

bin/tests/system/checkconf/good-update-policy2.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * self . TXT;
+	};
+};

bin/tests/system/checkconf/good-update-policy3.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * selfsub . TXT;
+	};
+};

bin/tests/system/checkconf/good-update-policy4.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * selfsub * TXT;
+	};
+};

bin/tests/system/checkconf/good-update-policy5.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * selfwild * TXT;
+	};
+};

bin/tests/system/checkconf/good-update-policy6.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * selfwild . TXT;
+	};
+};

bin/tests/system/checkconf/good-update-policy7.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * krb5-self . TXT;
+	};
+};

bin/tests/system/checkconf/good-update-policy8.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * ms-self . TXT;
+	};
+};

bin/tests/system/checkconf/good-update-policy9.conf

@@ -0,0 +1,7 @@
+zone "example.com" {
+	type master;
+	file "example.com.db";
+	update-policy {
+		grant * ms-subdomain . TXT;
+	};
+};

bin/tests/system/checkconf/tests.sh

@@ -40,14 +40,24 @@ status=`expr $status + $ret`
 
 for bad in bad-*.conf
 do
-	n=`expr $n + 1`
-	echo "I: checking that named-checkconf detects error in $bad ($n)"
-	ret=0
-	$CHECKCONF $bad > checkconf.out 2>&1
-	if [ $? != 1 ]; then ret=1; fi
-	grep "^$bad:[0-9]*: " checkconf.out > /dev/null || ret=1
-	if [ $ret != 0 ]; then echo "I:failed"; fi
-	status=`expr $status + $ret`
+    n=`expr $n + 1`
+    echo "I: checking that named-checkconf detects error in $bad ($n)"
+    ret=0
+    $CHECKCONF $bad > checkconf.out 2>&1
+    if [ $? != 1 ]; then ret=1; fi
+    grep "^$bad:[0-9]*: " checkconf.out > /dev/null || ret=1
+    case $bad in
+    bad-update-policy[123].conf)
+	pat="identity and name fields are not the same"
+	grep "$pat" checkconf.out > /dev/null || ret=1
+	;;
+    bad-update-policy*.conf)
+	pat="name field not set to placeholder value"
+	grep "$pat" checkconf.out > /dev/null || ret=1
+	;;
+    esac
+    if [ $ret != 0 ]; then echo "I:failed"; fi
+    status=`expr $status + $ret`
 done
 
 for good in good-*.conf

doc/arm/Bv9ARM-book.xml

@@ -12613,7 +12613,8 @@ example.com. NS ns2.example.net.
 			<replaceable>identity</replaceable> field.
 			The <replaceable>name</replaceable> field
 			is ignored, but should be the same as the
-			<replaceable>identity</replaceable> field.
+			<replaceable>identity</replaceable> field or
+			"."
 			The <varname>self</varname> nametype is
 			most useful when allowing using one key per
 			name to update, where the key has the same
@@ -12662,7 +12663,7 @@ example.com. NS ns2.example.net.
 			and converts it machine.realm allowing the machine
 			to update machine.realm.  The REALM to be matched
 			is specified in the <replaceable>identity</replaceable>
-			field.
+			field.  The name field should be set to "."
 		      </para>
 		    </entry>
 		  </row>
@@ -12694,7 +12695,7 @@ example.com. NS ns2.example.net.
 			and converts it machine.realm allowing the machine
 			to update machine.realm.  The REALM to be matched
 			is specified in the <replaceable>identity</replaceable>
-			field.
+			field. The name field should be set to "."
 		      </para>
 		    </entry>
 		  </row>
@@ -12710,7 +12711,8 @@ example.com. NS ns2.example.net.
 			converts it to machine.realm allowing the machine
 			to update subdomains of machine.realm.  The REALM
 			to be matched is specified in the
-			<replaceable>identity</replaceable> field.
+			<replaceable>identity</replaceable> field. The
+			name field should be set to "."
 		      </para>
 		    </entry>
 		  </row>
@@ -12724,7 +12726,8 @@ example.com. NS ns2.example.net.
 			Allow updates that have been sent via TCP and
 			for which the standard mapping from the initiating
 			IP address into the IN-ADDR.ARPA and IP6.ARPA
-			namespaces match the name to be updated.
+			namespaces match the name to be updated.  The
+			name field should be set to "."
 		      </para>
 		      <note>
 			It is theoretically possible to spoof these TCP

lib/bind9/check.c

@@ -41,6 +41,7 @@
 #include <dns/rdatatype.h>
 #include <dns/rrl.h>
 #include <dns/secalg.h>
+#include <dns/ssu.h>
 
 #include <dst/dst.h>
 
@@ -1723,9 +1724,9 @@ check_update_policy(const cfg_obj_t *policy, isc_log_t *logctx) {
 	isc_result_t tresult;
 	const cfg_listelt_t *element;
 	const cfg_listelt_t *element2;
-	dns_fixedname_t fixed;
+	dns_fixedname_t fixed_id, fixed_name;
+	dns_name_t *id, *name;
 	const char *str;
-	isc_buffer_t b;
 
 	/* Check for "update-policy local;" */
 	if (cfg_obj_isstring(policy) &&
@@ -1742,27 +1743,36 @@ check_update_policy(const cfg_obj_t *policy, isc_log_t *logctx) {
 		const cfg_obj_t *matchtype = cfg_tuple_get(stmt, "matchtype");
 		const cfg_obj_t *dname = cfg_tuple_get(stmt, "name");
 		const cfg_obj_t *typelist = cfg_tuple_get(stmt, "types");
+		dns_ssumatchtype_t mtype;
+
+		dns_fixedname_init(&fixed_id);
+		dns_fixedname_init(&fixed_name);
+		id = dns_fixedname_name(&fixed_id);
+		name = dns_fixedname_name(&fixed_name);
+
+		tresult = dns_ssu_mtypefromstring(cfg_obj_asstring(matchtype),
+						  &mtype);
+		if (tresult != ISC_R_SUCCESS) {
+			cfg_obj_log(identity, logctx, ISC_LOG_ERROR,
+				    "has a bad match-type");
+		}
 
-		dns_fixedname_init(&fixed);
 		str = cfg_obj_asstring(identity);
-		isc_buffer_constinit(&b, str, strlen(str));
-		isc_buffer_add(&b, strlen(str));
-		tresult = dns_name_fromtext(dns_fixedname_name(&fixed), &b,
-					    dns_rootname, 0, NULL);
+		tresult = dns_name_fromstring(id, str, 1, NULL);
 		if (tresult != ISC_R_SUCCESS) {
 			cfg_obj_log(identity, logctx, ISC_LOG_ERROR,
 				    "'%s' is not a valid name", str);
 			result = tresult;
 		}
 
+		/*
+		 * There is no name field for subzone.
+		 */
 		if (tresult == ISC_R_SUCCESS &&
-		    strcasecmp(cfg_obj_asstring(matchtype), "zonesub") != 0) {
-			dns_fixedname_init(&fixed);
+		    mtype != dns_ssumatchtype_subdomain)
+		{
 			str = cfg_obj_asstring(dname);
-			isc_buffer_constinit(&b, str, strlen(str));
-			isc_buffer_add(&b, strlen(str));
-			tresult = dns_name_fromtext(dns_fixedname_name(&fixed),
-						    &b, dns_rootname, 0, NULL);
+			tresult = dns_name_fromstring(name, str, 0, NULL);
 			if (tresult != ISC_R_SUCCESS) {
 				cfg_obj_log(dname, logctx, ISC_LOG_ERROR,
 					    "'%s' is not a valid name", str);
@@ -1771,13 +1781,55 @@ check_update_policy(const cfg_obj_t *policy, isc_log_t *logctx) {
 		}
 
 		if (tresult == ISC_R_SUCCESS &&
-		    strcasecmp(cfg_obj_asstring(matchtype), "wildcard") == 0 &&
-		    !dns_name_iswildcard(dns_fixedname_name(&fixed))) {
+		    mtype == dns_ssumatchtype_wildcard &&
+		    !dns_name_iswildcard(name))
+		{
 			cfg_obj_log(identity, logctx, ISC_LOG_ERROR,
 				    "'%s' is not a wildcard", str);
 			result = ISC_R_FAILURE;
 		}
 
+		/*
+		 * For some match types, the name should be a placeholder
+		 * value, either "." or the same as identity.
+		 */
+		switch (mtype) {
+		case dns_ssumatchtype_self:
+		case dns_ssumatchtype_selfsub:
+		case dns_ssumatchtype_selfwild:
+			if (tresult == ISC_R_SUCCESS &&
+			    (!dns_name_equal(id, name) &&
+			     !dns_name_equal(dns_rootname, name))) {
+				cfg_obj_log(identity, logctx, ISC_LOG_ERROR,
+					    "identity and name fields are not "
+					    "the same");
+				result = ISC_R_FAILURE;
+			}
+			break;
+		case dns_ssumatchtype_selfkrb5:
+		case dns_ssumatchtype_selfms:
+		case dns_ssumatchtype_subdomainms:
+		case dns_ssumatchtype_subdomainkrb5:
+		case dns_ssumatchtype_tcpself:
+		case dns_ssumatchtype_6to4self:
+			if (tresult == ISC_R_SUCCESS &&
+			    !dns_name_equal(dns_rootname, name)) {
+				cfg_obj_log(identity, logctx, ISC_LOG_ERROR,
+					    "name field not set to "
+					    "placeholder value '.'");
+				result = ISC_R_FAILURE;
+			}
+			break;
+		case dns_ssumatchtype_name:
+		case dns_ssumatchtype_subdomain:
+		case dns_ssumatchtype_wildcard:
+		case dns_ssumatchtype_external:
+		case dns_ssumatchtype_local:
+			break;
+		default:
+			INSIST(0);
+		}
+
 		for (element2 = cfg_list_first(typelist);
 		     element2 != NULL;
 		     element2 = cfg_list_next(element2))

lib/dns/include/dns/ssu.h

@@ -206,15 +206,28 @@ isc_result_t	dns_ssutable_nextrule(dns_ssurule_t *rule,
  *\li	#ISC_R_NOMORE
  */
 
-
-/*%<
- * Check a policy rule via an external application
- */
 isc_boolean_t
 dns_ssu_external_match(const dns_name_t *identity, const dns_name_t *signer,
 		       const dns_name_t *name, const isc_netaddr_t *tcpaddr,
 		       dns_rdatatype_t type, const dst_key_t *key,
 		       isc_mem_t *mctx);
+/*%<
+ * Check a policy rule via an external application
+ */
+
+isc_result_t
+dns_ssu_mtypefromstring(const char *str, dns_ssumatchtype_t *mtype);
+/*%<
+ * Set 'mtype' from 'str'
+ *
+ * Requires:
+ *\li		'str' is not NULL.
+ *\li		'mtype' is not NULL,
+ *
+ * Returns:
+ *\li	#ISC_R_SUCCESS
+ *\li	#ISC_R_NOTFOUND
+ */
 
 ISC_LANG_ENDDECLS
 

lib/dns/ssu.c

@@ -642,3 +642,43 @@ dns_ssutable_createdlz(isc_mem_t *mctx, dns_ssutable_t **tablep,
 	*tablep = table;
 	return (ISC_R_SUCCESS);
 }
+
+isc_result_t
+dns_ssu_mtypefromstring(const char *str, dns_ssumatchtype_t *mtype) {
+
+	REQUIRE(str != NULL);
+	REQUIRE(mtype != NULL);
+
+	if (strcasecmp(str, "name") == 0) {
+		*mtype = dns_ssumatchtype_name;
+	} else if (strcasecmp(str, "subdomain") == 0) {
+		*mtype = dns_ssumatchtype_subdomain;
+	} else if (strcasecmp(str, "wildcard") == 0) {
+		*mtype = dns_ssumatchtype_wildcard;
+	} else if (strcasecmp(str, "self") == 0) {
+		*mtype = dns_ssumatchtype_self;
+	} else if (strcasecmp(str, "selfsub") == 0) {
+		*mtype = dns_ssumatchtype_selfsub;
+	} else if (strcasecmp(str, "selfwild") == 0) {
+		*mtype = dns_ssumatchtype_selfwild;
+	} else if (strcasecmp(str, "ms-self") == 0) {
+		*mtype = dns_ssumatchtype_selfms;
+	} else if (strcasecmp(str, "krb5-self") == 0) {
+		*mtype = dns_ssumatchtype_selfkrb5;
+	} else if (strcasecmp(str, "ms-subdomain") == 0) {
+		*mtype = dns_ssumatchtype_subdomainms;
+	} else if (strcasecmp(str, "krb5-subdomain") == 0) {
+		*mtype = dns_ssumatchtype_subdomainkrb5;
+	} else if (strcasecmp(str, "tcp-self") == 0) {
+		*mtype = dns_ssumatchtype_tcpself;
+	} else if (strcasecmp(str, "6to4-self") == 0) {
+		*mtype = dns_ssumatchtype_6to4self;
+	} else if (strcasecmp(str, "zonesub") == 0) {
+		*mtype = dns_ssumatchtype_subdomain;
+	} else if (strcasecmp(str, "external") == 0) {
+		*mtype = dns_ssumatchtype_external;
+	} else {
+		return (ISC_R_NOTFOUND);
+	}
+	return (ISC_R_SUCCESS);
+}

lib/dns/win32/libdns.def.in

@@ -1018,6 +1018,7 @@ dns_soa_setrefresh
 dns_soa_setretry
 dns_soa_setserial
 dns_ssu_external_match
+dns_ssu_mtypefromstring
 dns_ssutable_addrule
 dns_ssutable_attach
 dns_ssutable_checkrules