diff --git a/CHANGES b/CHANGES index 1166e4eea3..f4b80b703c 100644 --- a/CHANGES +++ b/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] diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 8f7475b19e..7e06088452 100644 --- a/doc/notes/notes-current.rst +++ b/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 ~~~~~~~~~~~~~~~~ diff --git a/lib/dns/rdata/generic/key_25.c b/lib/dns/rdata/generic/key_25.c index ca21a13b3f..efee3a32dc 100644 --- a/lib/dns/rdata/generic/key_25.c +++ b/lib/dns/rdata/generic/key_25.c @@ -16,6 +16,8 @@ #ifndef RDATA_GENERIC_KEY_25_C #define RDATA_GENERIC_KEY_25_C +#include + #include #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) { - dns_name_t name; - dns_decompress_setmethods(dctx, DNS_COMPRESS_NONE); - dns_name_init(&name, NULL); - RETERR(dns_name_fromwire(&name, source, dctx, options, target)); + if (algorithm == DNS_KEYALG_PRIVATEDNS || + algorithm == DNS_KEYALG_PRIVATEOID) { + isc_buffer_t b = *source; + RETERR(check_private(&b, algorithm)); } isc_buffer_activeregion(source, &sr); diff --git a/lib/dns/tests/rdata_test.c b/lib/dns/tests/rdata_test.c index 621832239e..8066ed1010 100644 --- a/lib/dns/tests/rdata_test.c +++ b/lib/dns/tests/rdata_test.c @@ -2072,33 +2072,81 @@ isdn(void **state) { */ static void key(void **state) { - wire_ok_t wire_ok[] = { /* - * RDATA is comprised of: - * - * - 2 octets for Flags, - * - 1 octet for Protocol, - * - 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 - * the algorithm octet and thus its length must - * be 4 octets. In any other case, though, the - * Public Key part must not be empty. + wire_ok_t wire_ok[] = { + /* + * RDATA is comprised of: + * + * - 2 octets for Flags, + * - 1 octet for Protocol, + * - 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 + * the algorithm octet and thus its length must + * 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), - 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() + TEXT_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)); }