Merge branch '3234-check-the-oid-in-privateoid-keys' into 'main'

Resolve "Check the OID in PRIVATEOID keys"

Closes #3234

See merge request isc-projects/bind9!6045

CHANGES

@@ -1,3 +1,11 @@
+5864.	[func]		The OID embedded at the start of a PRIVATEOID public
+			key in a KEY, DNSKEY, CDNSKEY, or RKEY RR is now
+			checked for validity when reading from wire or from
+			zone files, and the OID is printed when
+			'dig +rrcomments' is used. Similarly, the name
+			embedded at the start of a PRIVATEDNS public key
+			is also checked for validity. [GL #3234]
+
 5863.	[bug]		If there was a pending negative cache DS entry,
 			validations depending upon it could fail. [GL #3279]
 

doc/notes/notes-current.rst

@@ -27,6 +27,13 @@ New Features
 
 - None.
 
+- The Object Identifier (OID) embedded at the start of a PRIVATEOID public
+  key in a KEY, DNSKEY, CDNSKEY, or RKEY resource record is now checked to
+  ensure that it is valid when reading from zone files or receiving data
+  on the wire, and the OID is now printed when the ``dig +rrcomments``
+  option is used. Similarly, the name embedded at the start of a PRIVATEDNS
+  public key is also checked for validity. :gl:`#3234`
+
 Removed Features
 ~~~~~~~~~~~~~~~~
 

lib/dns/rdata/generic/key_25.c

@@ -16,6 +16,8 @@
 #ifndef RDATA_GENERIC_KEY_25_C
 #define RDATA_GENERIC_KEY_25_C
 
+#include <openssl/objects.h>
+
 #include <dst/dst.h>
 
 #define RRTYPE_KEY_ATTRIBUTES \
@@ -44,12 +46,51 @@ generic_key_nokey(dns_rdatatype_t type, unsigned int flags) {
 	}
 }
 
+static isc_result_t
+check_private(isc_buffer_t *source, dns_secalg_t alg) {
+	isc_region_t sr;
+	if (alg == DNS_KEYALG_PRIVATEDNS) {
+		dns_fixedname_t fixed;
+		dns_decompress_t dctx;
+
+		dns_decompress_init(&dctx, -1, DNS_DECOMPRESS_STRICT);
+		RETERR(dns_name_fromwire(dns_fixedname_initname(&fixed), source,
+					 &dctx, 0, NULL));
+		/* There should be a public key after the key name. */
+		isc_buffer_activeregion(source, &sr);
+		if (sr.length == 0) {
+			return (ISC_R_UNEXPECTEDEND);
+		}
+	} else if (alg == DNS_KEYALG_PRIVATEOID) {
+		/*
+		 * Check that we can extract the OID from the start of the
+		 * key data.
+		 */
+		const unsigned char *in = NULL;
+		ASN1_OBJECT *obj = NULL;
+
+		isc_buffer_activeregion(source, &sr);
+		in = sr.base;
+		obj = d2i_ASN1_OBJECT(NULL, &in, sr.length);
+		if (obj == NULL) {
+			RETERR(DNS_R_FORMERR);
+		}
+		ASN1_OBJECT_free(obj);
+		/* There should be a public key after the OID. */
+		if (in >= sr.base + sr.length) {
+			return (ISC_R_UNEXPECTEDEND);
+		}
+	}
+	return (ISC_R_SUCCESS);
+}
+
 static isc_result_t
 generic_fromtext_key(ARGS_FROMTEXT) {
 	isc_token_t token;
 	dns_secalg_t alg;
 	dns_secproto_t proto;
 	dns_keyflags_t flags;
+	unsigned int used;
 
 	UNUSED(rdclass);
 	UNUSED(origin);
@@ -82,7 +123,28 @@ generic_fromtext_key(ARGS_FROMTEXT) {
 		return (ISC_R_SUCCESS);
 	}
 
-	return (isc_base64_tobuffer(lexer, target, -2));
+	/*
+	 * Save the current used value. It will become the current
+	 * value when we parse the keydata field.
+	 */
+	used = isc_buffer_usedlength(target);
+
+	RETERR(isc_base64_tobuffer(lexer, target, -2));
+
+	if (alg == DNS_KEYALG_PRIVATEDNS || alg == DNS_KEYALG_PRIVATEOID) {
+		isc_buffer_t b;
+
+		/*
+		 * Set up 'b' so that the key data can be parsed.
+		 */
+		b = *target;
+		b.active = b.used;
+		b.current = used;
+
+		RETERR(check_private(&b, alg));
+	}
+
+	return (ISC_R_SUCCESS);
 }
 
 static isc_result_t
@@ -139,6 +201,19 @@ generic_totext_key(ARGS_TOTEXT) {
 		dns_name_init(&name, NULL);
 		dns_name_fromregion(&name, &sr);
 		dns_name_format(&name, algbuf, sizeof(algbuf));
+	} else if ((tctx->flags & DNS_STYLEFLAG_RRCOMMENT) != 0 &&
+		   algorithm == DNS_KEYALG_PRIVATEOID)
+	{
+		const unsigned char *in = sr.base;
+		ASN1_OBJECT *obj = d2i_ASN1_OBJECT(NULL, &in, sr.length);
+		int n;
+		INSIST(obj != NULL);
+		n = i2t_ASN1_OBJECT(algbuf, sizeof(buf), obj);
+		ASN1_OBJECT_free(obj);
+		if (n == -1 || (size_t)n >= sizeof(algbuf)) {
+			dns_secalg_format((dns_secalg_t)algorithm, algbuf,
+					  sizeof(algbuf));
+		}
 	} else {
 		dns_secalg_format((dns_secalg_t)algorithm, algbuf,
 				  sizeof(algbuf));
@@ -222,11 +297,10 @@ generic_fromwire_key(ARGS_FROMWIRE) {
 		return (ISC_R_UNEXPECTEDEND);
 	}
 
-	if (algorithm == DNS_KEYALG_PRIVATEDNS) {
+	if (algorithm == DNS_KEYALG_PRIVATEDNS ||
-		dns_name_t name;
+	    algorithm == DNS_KEYALG_PRIVATEOID) {
-		dns_decompress_setmethods(dctx, DNS_COMPRESS_NONE);
+		isc_buffer_t b = *source;
-		dns_name_init(&name, NULL);
+		RETERR(check_private(&b, algorithm));
-		RETERR(dns_name_fromwire(&name, source, dctx, options, target));
 	}
 
 	isc_buffer_activeregion(source, &sr);

lib/dns/tests/rdata_test.c

@@ -2072,33 +2072,81 @@ isdn(void **state) {
  */
 static void
 key(void **state) {
-	wire_ok_t wire_ok[] = { /*
+	wire_ok_t wire_ok[] = {
-				 * RDATA is comprised of:
+		/*
-				 *
+		 * RDATA is comprised of:
-				 *   - 2 octets for Flags,
+		 *
-				 *   - 1 octet for Protocol,
+		 *   - 2 octets for Flags,
-				 *   - 1 octet for Algorithm,
+		 *   - 1 octet for Protocol,
-				 *   - variable number of octets for Public Key.
+		 *   - 1 octet for Algorithm,
-				 *
+		 *   - variable number of octets for Public Key.
-				 * RFC 2535 section 3.1.2 states that if bits
+		 *
-				 * 0-1 of Flags are both set, the RR stops after
+		 * RFC 2535 section 3.1.2 states that if bits
-				 * the algorithm octet and thus its length must
+		 * 0-1 of Flags are both set, the RR stops after
-				 * be 4 octets.  In any other case, though, the
+		 * the algorithm octet and thus its length must
-				 * Public Key part must not be empty.
+		 * be 4 octets.  In any other case, though, the
+		 * Public Key part must not be empty.
+		 *
+		 * Algorithms PRIVATEDNS (253) and PRIVATEOID (254)
+		 * have an algorithm identifier embedded and the start
+		 * of the public key.
+		 */
+		WIRE_INVALID(0x00), WIRE_INVALID(0x00, 0x00),
+		WIRE_INVALID(0x00, 0x00, 0x00),
+		WIRE_VALID(0xc0, 0x00, 0x00, 0x00),
+		WIRE_INVALID(0xc0, 0x00, 0x00, 0x00, 0x00),
+		WIRE_INVALID(0x00, 0x00, 0x00, 0x00),
+		WIRE_VALID(0x00, 0x00, 0x00, 0x00, 0x00),
+		/* PRIVATEDNS example. */
+		WIRE_INVALID(0x00, 0x00, 0x00, 253, 0x07, 'e', 'x', 'a', 'm',
+			     'p', 'l', 'e', 0x00),
+		/* PRIVATEDNS example. + keydata */
+		WIRE_VALID(0x00, 0x00, 0x00, 253, 0x07, 'e', 'x', 'a', 'm', 'p',
+			   'l', 'e', 0x00, 0x00),
+		/* PRIVATEDNS compression pointer. */
+		WIRE_INVALID(0x00, 0x00, 0x00, 253, 0xc0, 0x00, 0x00),
+		/* PRIVATEOID */
+		WIRE_INVALID(0x00, 0x00, 0x00, 254, 0x00),
+		/* PRIVATEOID 1.3.6.1.4.1.2495 */
+		WIRE_INVALID(0x00, 0x00, 0x00, 254, 0x06, 0x07, 0x2b, 0x06,
+			     0x01, 0x04, 0x01, 0x93, 0x3f),
+		/* PRIVATEOID 1.3.6.1.4.1.2495 + keydata */
+		WIRE_VALID(0x00, 0x00, 0x00, 254, 0x06, 0x07, 0x2b, 0x06, 0x01,
+			   0x04, 0x01, 0x93, 0x3f, 0x00),
+		/* PRIVATEOID malformed OID - high-bit set on last octet */
+		WIRE_INVALID(0x00, 0x00, 0x00, 254, 0x06, 0x07, 0x2b, 0x06,
+			     0x01, 0x04, 0x01, 0x93, 0xbf, 0x00),
+		/* PRIVATEOID malformed OID - wrong tag */
+		WIRE_INVALID(0x00, 0x00, 0x00, 254, 0x07, 0x07, 0x2b, 0x06,
+			     0x01, 0x04, 0x01, 0x93, 0x3f, 0x00),
+		WIRE_SENTINEL()
+	};
+	text_ok_t text_ok[] = { /* PRIVATEDNS example. */
+				TEXT_INVALID("0 0 253 B2V4YW1wbGUA"),
+				/* PRIVATEDNS example. + keydata */
+				TEXT_VALID("0 0 253 B2V4YW1wbGUAAA=="),
+				/* PRIVATEDNS compression pointer. */
+				TEXT_INVALID("0 0 253 wAAA"),
+				/* PRIVATEOID */
+				TEXT_INVALID("0 0 254 AA=="),
+				/* PRIVATEOID 1.3.6.1.4.1.2495 */
+				TEXT_INVALID("0 0 254 BgcrBgEEAZM/"),
+				/* PRIVATEOID 1.3.6.1.4.1.2495 + keydata */
+				TEXT_VALID("0 0 254 BgcrBgEEAZM/AA=="),
+				/* PRIVATEOID malformed OID - high-bit set on
+				   last octet */
+				TEXT_INVALID("0 0 254 BgcrBgEEAZO/AA=="),
+				/* PRIVATEOID malformed OID - wrong tag */
+				TEXT_INVALID("0 0 254 BwcrBgEEAZM/AA=="),
+				/*
+				 * Sentinel.
 				 */
-				WIRE_INVALID(0x00),
+				TEXT_SENTINEL()
-				WIRE_INVALID(0x00, 0x00),
-				WIRE_INVALID(0x00, 0x00, 0x00),
-				WIRE_VALID(0xc0, 0x00, 0x00, 0x00),
-				WIRE_INVALID(0xc0, 0x00, 0x00, 0x00, 0x00),
-				WIRE_INVALID(0x00, 0x00, 0x00, 0x00),
-				WIRE_VALID(0x00, 0x00, 0x00, 0x00, 0x00),
-				WIRE_SENTINEL()
 	};
 
 	UNUSED(state);
 
-	check_rdata(NULL, wire_ok, NULL, false, dns_rdataclass_in,
+	check_rdata(text_ok, wire_ok, NULL, false, dns_rdataclass_in,
 		    dns_rdatatype_key, sizeof(dns_rdata_key_t));
 }