From 3e857065de5513e81a4af3a25a562d12be5433f0 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 21 Apr 2022 19:49:29 +1000 Subject: [PATCH] Check that SIG and RRSIG records for private algorithms are valid SIG and RRSIG records for private algorithms are supposed to contain the name / OID of the algorithm used to generate them at the start of the signature field. --- lib/dns/rdata.c | 42 +++++++++++++ lib/dns/rdata/generic/key_25.c | 40 ------------- lib/dns/rdata/generic/rrsig_46.c | 36 +++++++++-- lib/dns/rdata/generic/sig_24.c | 36 +++++++++-- lib/dns/tests/rdata_test.c | 100 +++++++++++++++++++++++++++++++ 5 files changed, 206 insertions(+), 48 deletions(-) diff --git a/lib/dns/rdata.c b/lib/dns/rdata.c index 8dc7e6e4da..1665f87d93 100644 --- a/lib/dns/rdata.c +++ b/lib/dns/rdata.c @@ -17,6 +17,8 @@ #include #include +#include + #include #include #include @@ -604,6 +606,46 @@ typemap_test(isc_region_t *sr, bool allow_empty) { static const char hexdigits[] = "0123456789abcdef"; static const char decdigits[] = "0123456789"; +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 or signature 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 or signature after the OID. */ + if (in >= sr.base + sr.length) { + return (ISC_R_UNEXPECTEDEND); + } + } + return (ISC_R_SUCCESS); +} + #include "code.h" #define META 0x0001 diff --git a/lib/dns/rdata/generic/key_25.c b/lib/dns/rdata/generic/key_25.c index efee3a32dc..6b8a14d9ca 100644 --- a/lib/dns/rdata/generic/key_25.c +++ b/lib/dns/rdata/generic/key_25.c @@ -16,8 +16,6 @@ #ifndef RDATA_GENERIC_KEY_25_C #define RDATA_GENERIC_KEY_25_C -#include - #include #define RRTYPE_KEY_ATTRIBUTES \ @@ -46,44 +44,6 @@ 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; diff --git a/lib/dns/rdata/generic/rrsig_46.c b/lib/dns/rdata/generic/rrsig_46.c index 7c4159a752..bb9190431d 100644 --- a/lib/dns/rdata/generic/rrsig_46.c +++ b/lib/dns/rdata/generic/rrsig_46.c @@ -23,7 +23,7 @@ static isc_result_t fromtext_rrsig(ARGS_FROMTEXT) { isc_token_t token; - unsigned char c; + unsigned char alg, c; long i; dns_rdatatype_t covered; char *e; @@ -31,6 +31,7 @@ fromtext_rrsig(ARGS_FROMTEXT) { dns_name_t name; isc_buffer_t buffer; uint32_t time_signed, time_expire; + unsigned int used; REQUIRE(type == dns_rdatatype_rrsig); @@ -61,8 +62,8 @@ fromtext_rrsig(ARGS_FROMTEXT) { */ RETERR(isc_lex_getmastertoken(lexer, &token, isc_tokentype_string, false)); - RETTOK(dns_secalg_fromtext(&c, &token.value.as_textregion)); - RETERR(mem_tobuffer(target, &c, 1)); + RETTOK(dns_secalg_fromtext(&alg, &token.value.as_textregion)); + RETERR(mem_tobuffer(target, &alg, 1)); /* * Labels. @@ -154,7 +155,24 @@ fromtext_rrsig(ARGS_FROMTEXT) { /* * Sig. */ - return (isc_base64_tobuffer(lexer, target, -2)); + 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 signature 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 @@ -278,6 +296,7 @@ static isc_result_t fromwire_rrsig(ARGS_FROMWIRE) { isc_region_t sr; dns_name_t name; + unsigned char algorithm; REQUIRE(type == dns_rdatatype_rrsig); @@ -300,6 +319,8 @@ fromwire_rrsig(ARGS_FROMWIRE) { return (ISC_R_UNEXPECTEDEND); } + algorithm = sr.base[2]; + isc_buffer_forward(source, 18); RETERR(mem_tobuffer(target, sr.base, 18)); @@ -316,6 +337,13 @@ fromwire_rrsig(ARGS_FROMWIRE) { if (sr.length < 1) { return (DNS_R_FORMERR); } + + if (algorithm == DNS_KEYALG_PRIVATEDNS || + algorithm == DNS_KEYALG_PRIVATEOID) { + isc_buffer_t b = *source; + RETERR(check_private(&b, algorithm)); + } + isc_buffer_forward(source, sr.length); return (mem_tobuffer(target, sr.base, sr.length)); } diff --git a/lib/dns/rdata/generic/sig_24.c b/lib/dns/rdata/generic/sig_24.c index d80beb63c5..7cfec7bfed 100644 --- a/lib/dns/rdata/generic/sig_24.c +++ b/lib/dns/rdata/generic/sig_24.c @@ -21,7 +21,7 @@ static isc_result_t fromtext_sig(ARGS_FROMTEXT) { isc_token_t token; - unsigned char c; + unsigned char alg, c; long i; dns_rdatatype_t covered; char *e; @@ -29,6 +29,7 @@ fromtext_sig(ARGS_FROMTEXT) { dns_name_t name; isc_buffer_t buffer; uint32_t time_signed, time_expire; + unsigned int used; REQUIRE(type == dns_rdatatype_sig); @@ -59,8 +60,8 @@ fromtext_sig(ARGS_FROMTEXT) { */ RETERR(isc_lex_getmastertoken(lexer, &token, isc_tokentype_string, false)); - RETTOK(dns_secalg_fromtext(&c, &token.value.as_textregion)); - RETERR(mem_tobuffer(target, &c, 1)); + RETTOK(dns_secalg_fromtext(&alg, &token.value.as_textregion)); + RETERR(mem_tobuffer(target, &alg, 1)); /* * Labels. @@ -118,7 +119,24 @@ fromtext_sig(ARGS_FROMTEXT) { /* * Sig. */ - return (isc_base64_tobuffer(lexer, target, -2)); + 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 signature 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 @@ -241,6 +259,7 @@ static isc_result_t fromwire_sig(ARGS_FROMWIRE) { isc_region_t sr; dns_name_t name; + unsigned char algorithm; REQUIRE(type == dns_rdatatype_sig); @@ -263,6 +282,8 @@ fromwire_sig(ARGS_FROMWIRE) { return (ISC_R_UNEXPECTEDEND); } + algorithm = sr.base[2]; + isc_buffer_forward(source, 18); RETERR(mem_tobuffer(target, sr.base, 18)); @@ -279,6 +300,13 @@ fromwire_sig(ARGS_FROMWIRE) { if (sr.length == 0) { return (ISC_R_UNEXPECTEDEND); } + + if (algorithm == DNS_KEYALG_PRIVATEDNS || + algorithm == DNS_KEYALG_PRIVATEOID) { + isc_buffer_t b = *source; + RETERR(check_private(&b, algorithm)); + } + isc_buffer_forward(source, sr.length); return (mem_tobuffer(target, sr.base, sr.length)); } diff --git a/lib/dns/tests/rdata_test.c b/lib/dns/tests/rdata_test.c index 8066ed1010..33b118d87d 100644 --- a/lib/dns/tests/rdata_test.c +++ b/lib/dns/tests/rdata_test.c @@ -2420,6 +2420,105 @@ rkey(void **state) { dns_rdatatype_rkey, sizeof(dns_rdata_rkey_t)); } +/* + * rrsig (sig) tests. + */ +static void +sig_rrsig(void **state) { + wire_ok_t wire_ok[] = { + /* + * RDATA is comprised of: + * + * type covered: 2 + * algorithm: 1 + * labels: 1 + * original ttl: 4 + * signature expiration: 4 + * time signed: 4 + * key footprint: 2 + * signer: variable + * signature: variable + * - if algorithm is PRIVATEDNS the algorithm name is embedded + * at the start of the signature + * - if algorithm is PRIVATEOID the algorithm OID is embedded + * at the start of the signature + */ + /* PRIVATEDNS example. */ + WIRE_INVALID(0x00, 0x01, 253, 0x01, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x06, 's', 'i', 'g', 'n', 'e', 'r', + 0x00, 0x07, 'e', 'x', 'a', 'm', 'p', 'l', 'e', + 0x00), + /* PRIVATEDNS example. + sigdata */ + WIRE_VALID(0x00, 0x01, 253, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x06, 's', 'i', 'g', 'n', 'e', 'r', 0x00, 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, 0x01, 254, 0x01, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x06, 's', 'i', 'g', 'n', 'e', 'r', + 0x00, 0x00), + /* PRIVATEOID 1.3.6.1.4.1.2495 */ + WIRE_INVALID(0x00, 0x01, 254, 0x01, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x06, 's', 'i', 'g', 'n', 'e', 'r', + 0x00, 0x06, 0x07, 0x2b, 0x06, 0x01, 0x04, 0x01, + 0x93, 0x3f), + /* PRIVATEOID 1.3.6.1.4.1.2495 + sigdata */ + WIRE_VALID(0x00, 0x01, 254, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x06, 's', 'i', 'g', 'n', 'e', 'r', 0x00, 0x06, 0x07, + 0x2b, 0x06, 0x01, 0x04, 0x01, 0x93, 0x3f, 0x00), + /* PRIVATEOID malformed OID - high-bit set on last octet */ + WIRE_INVALID(0x00, 0x01, 254, 0x01, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x06, 's', 'i', 'g', 'n', 'e', 'r', + 0x00, 0x06, 0x07, 0x2b, 0x06, 0x01, 0x04, 0x01, + 0x93, 0xbf, 0x00), + WIRE_SENTINEL() + }; + text_ok_t text_ok[] = { + /* PRIVATEDNS example. */ + TEXT_INVALID("A 253 1 0 19700101000001 19700101000000 0 " + "signer. B2V4YW1wbGUA"), + /* PRIVATEDNS example. + sigdata */ + TEXT_VALID("A 253 1 0 19700101000001 19700101000000 0 signer. " + "B2V4YW1wbGUAAA=="), + /* PRIVATEDNS compression pointer. */ + TEXT_INVALID("A 253 1 0 19700101000001 19700101000000 0 " + "signer. wAAA"), + /* PRIVATEOID */ + TEXT_INVALID("A 254 1 0 19700101000001 19700101000000 0 " + "signer. AA=="), + /* PRIVATEOID 1.3.6.1.4.1.2495 */ + TEXT_INVALID("A 254 1 0 19700101000001 19700101000000 0 " + "signer. BgcrBgEEAZM/"), + /* PRIVATEOID 1.3.6.1.4.1.2495 + sigdata */ + TEXT_VALID("A 254 1 0 19700101000001 19700101000000 0 signer. " + "BgcrBgEEAZM/AA=="), + /* PRIVATEOID malformed OID - high-bit set on last octet */ + TEXT_INVALID("A 254 1 0 19700101000001 19700101000000 0 " + "signer. BgcrBgEEAZO/AA=="), + /* PRIVATEOID malformed OID - wrong tag */ + TEXT_INVALID("A 254 1 0 19700101000001 19700101000000 0 " + "signer. BwcrBgEEAZM/AA=="), + /* + * Sentinel. + */ + TEXT_SENTINEL() + }; + + UNUSED(state); + + check_rdata(text_ok, wire_ok, NULL, false, dns_rdataclass_in, + dns_rdatatype_sig, sizeof(dns_rdata_sig_t)); + check_rdata(text_ok, wire_ok, NULL, false, dns_rdataclass_in, + dns_rdatatype_rrsig, sizeof(dns_rdata_rrsig_t)); +} + /* SSHFP RDATA manipulations */ static void sshfp(void **state) { @@ -3209,6 +3308,7 @@ main(int argc, char **argv) { cmocka_unit_test_setup_teardown(nsec3, _setup, _teardown), cmocka_unit_test_setup_teardown(nxt, _setup, _teardown), cmocka_unit_test_setup_teardown(rkey, _setup, _teardown), + cmocka_unit_test_setup_teardown(sig_rrsig, _setup, _teardown), cmocka_unit_test_setup_teardown(sshfp, _setup, _teardown), cmocka_unit_test_setup_teardown(wks, _setup, _teardown), cmocka_unit_test_setup_teardown(zonemd, _setup, _teardown),