From e09eb2ff218dae1b7ac19610ceae680f7735ead0 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 3 Apr 2025 12:49:39 +1100 Subject: [PATCH] Fix OID check for PRIVATEOID keys and signatures We were failing to account for the length byte before the OID. See RFC 4034. Algorithm number 254 is reserved for private use and will never be assigned to a specific algorithm. The public key area in the DNSKEY RR and the signature area in the RRSIG RR begin with an unsigned length byte followed by a BER encoded Object Identifier (ISO OID) of that length. The OID indicates the private algorithm in use, and the remainder of the area is whatever is required by that algorithm. Entities should only use OIDs they control to designate their private algorithms. (cherry picked from commit ca7355b7d064154a348d81002504b1092bf55937) --- lib/dns/rdata.c | 13 ++++++++++--- lib/dns/rdata/generic/key_25.c | 7 +++---- tests/dns/rdata_test.c | 20 ++++++++++---------- 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/lib/dns/rdata.c b/lib/dns/rdata.c index e88c289aed..dd1e990251 100644 --- a/lib/dns/rdata.c +++ b/lib/dns/rdata.c @@ -609,19 +609,26 @@ check_private(isc_buffer_t *source, dns_secalg_t alg) { } else if (alg == DNS_KEYALG_PRIVATEOID) { /* * Check that we can extract the OID from the start of the - * key data. + * key data. We have a length byte followed by the OID BER + * encoded. */ 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 (sr.length < 1 || (unsigned int)*sr.base + 1 > sr.length) { + RETERR(DNS_R_FORMERR); + } + in = sr.base + 1; + obj = d2i_ASN1_OBJECT(NULL, &in, *sr.base); if (obj == NULL) { ERR_clear_error(); RETERR(DNS_R_FORMERR); } ASN1_OBJECT_free(obj); + if ((in - sr.base) != (*sr.base + 1)) { + RETERR(DNS_R_FORMERR); + } } return ISC_R_SUCCESS; } diff --git a/lib/dns/rdata/generic/key_25.c b/lib/dns/rdata/generic/key_25.c index 12b5095553..351c1149d7 100644 --- a/lib/dns/rdata/generic/key_25.c +++ b/lib/dns/rdata/generic/key_25.c @@ -164,11 +164,10 @@ generic_totext_key(ARGS_TOTEXT) { } 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; + const unsigned char *in = sr.base + 1; + ASN1_OBJECT *obj = d2i_ASN1_OBJECT(NULL, &in, *sr.base); INSIST(obj != NULL); - n = i2t_ASN1_OBJECT(algbuf, sizeof(buf), obj); + int 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, diff --git a/tests/dns/rdata_test.c b/tests/dns/rdata_test.c index 96b3140fc6..83b4711d47 100644 --- a/tests/dns/rdata_test.c +++ b/tests/dns/rdata_test.c @@ -2054,17 +2054,17 @@ ISC_RUN_TEST_IMPL(key) { /* PRIVATEOID */ WIRE_INVALID(0x00, 0x00, 0x00, 254, 0x00), /* PRIVATEOID 1.3.6.1.4.1.2495 without key data */ - WIRE_VALID(0x00, 0x00, 0x00, 254, 0x06, 0x07, 0x2b, 0x06, 0x01, - 0x04, 0x01, 0x93, 0x3f), + WIRE_VALID(0x00, 0x00, 0x00, 254, 0x09, 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), + WIRE_VALID(0x00, 0x00, 0x00, 254, 0x09, 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_INVALID(0x00, 0x00, 0x00, 254, 0x09, 0x07, 0x07, 0x2b, + 0x06, 0x01, 0x04, 0x01, 0x93, 0x3f, 0x00), WIRE_SENTINEL() }; text_ok_t text_ok[] = { /* PRIVATEDNS example. */ @@ -2076,14 +2076,14 @@ ISC_RUN_TEST_IMPL(key) { /* PRIVATEOID */ TEXT_INVALID("0 0 254 AA=="), /* PRIVATEOID 1.3.6.1.4.1.2495 */ - TEXT_VALID("0 0 254 BgcrBgEEAZM/"), + TEXT_VALID("0 0 254 CQYHKwYBBAGTPw=="), /* PRIVATEOID 1.3.6.1.4.1.2495 + keydata */ - TEXT_VALID("0 0 254 BgcrBgEEAZM/AA=="), + TEXT_VALID("0 0 254 CQYHKwYBBAGTPwA="), /* PRIVATEOID malformed OID - high-bit set on last octet */ - TEXT_INVALID("0 0 254 BgcrBgEEAZO/AA=="), + TEXT_INVALID("0 0 254 CQYHKwYBBAGTvwA="), /* PRIVATEOID malformed OID - wrong tag */ - TEXT_INVALID("0 0 254 BwcrBgEEAZM/AA=="), + TEXT_INVALID("0 0 254 CQcHKwYBBAGTPwA="), /* * Sentinel. */