From 07d024a4daba755d817131dacfa2df4812c4c460 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 27 Feb 2019 10:35:53 +1100 Subject: [PATCH 1/3] check flags for no key in fromwire for *KEY (cherry picked from commit 2592e91516a44368ef86c17010eb56db017523ca) --- lib/dns/rdata/generic/key_25.c | 43 ++++++++-- lib/dns/tests/master_test.c | 8 +- lib/dns/tests/rdata_test.c | 91 ++++++++++++++++++++++ lib/dns/tests/testdata/master/master8.data | 4 +- 4 files changed, 136 insertions(+), 10 deletions(-) diff --git a/lib/dns/rdata/generic/key_25.c b/lib/dns/rdata/generic/key_25.c index 69b26add43..186351282e 100644 --- a/lib/dns/rdata/generic/key_25.c +++ b/lib/dns/rdata/generic/key_25.c @@ -19,6 +19,29 @@ #define RRTYPE_KEY_ATTRIBUTES \ ( DNS_RDATATYPEATTR_ATCNAME | DNS_RDATATYPEATTR_ZONECUTAUTH ) +/* + * RFC 2535 section 3.1.2 says that if bits 0-1 of the Flags field are + * both set, it means there is no key information and the RR stops after + * the algorithm octet. However, this only applies to KEY records, as + * indicated by the specifications of the RR types based on KEY: + * + * CDNSKEY - RFC 7344 + * DNSKEY - RFC 4034 + * RKEY - draft-reid-dnsext-rkey-00 + */ +static inline bool +generic_key_nokey(dns_rdatatype_t type, unsigned int flags) { + switch (type) { + case dns_rdatatype_cdnskey: + case dns_rdatatype_dnskey: + case dns_rdatatype_rkey: + return (false); + case dns_rdatatype_key: + default: + return ((flags & DNS_KEYFLAG_TYPEMASK) == DNS_KEYTYPE_NOKEY); + } +} + static inline isc_result_t generic_fromtext_key(ARGS_FROMTEXT) { isc_token_t token; @@ -26,7 +49,6 @@ generic_fromtext_key(ARGS_FROMTEXT) { dns_secproto_t proto; dns_keyflags_t flags; - UNUSED(type); UNUSED(rdclass); UNUSED(origin); UNUSED(options); @@ -51,8 +73,9 @@ generic_fromtext_key(ARGS_FROMTEXT) { RETERR(mem_tobuffer(target, &alg, 1)); /* No Key? */ - if ((flags & 0xc000) == 0xc000) + if (generic_key_nokey(type, flags)) { return (ISC_R_SUCCESS); + } return (isc_base64_tobuffer(lexer, target, -2)); } @@ -99,8 +122,9 @@ generic_totext_key(ARGS_TOTEXT) { RETERR(str_totext(buf, target)); /* No Key? */ - if ((flags & 0xc000) == 0xc000) + if (generic_key_nokey(rdata->type, flags)) { return (ISC_R_SUCCESS); + } if ((tctx->flags & DNS_STYLEFLAG_RRCOMMENT) != 0 && algorithm == DNS_KEYALG_PRIVATEDNS) { @@ -160,22 +184,31 @@ generic_totext_key(ARGS_TOTEXT) { static inline isc_result_t generic_fromwire_key(ARGS_FROMWIRE) { unsigned char algorithm; + uint16_t flags; isc_region_t sr; - UNUSED(type); UNUSED(rdclass); UNUSED(dctx); UNUSED(options); isc_buffer_activeregion(source, &sr); - if (sr.length < 4) + if (sr.length < 4) { return (ISC_R_UNEXPECTEDEND); + } + flags = (sr.base[0] << 8) | sr.base[1]; algorithm = sr.base[3]; RETERR(mem_tobuffer(target, sr.base, 4)); isc_region_consume(&sr, 4); isc_buffer_forward(source, 4); + if (generic_key_nokey(type, flags)) { + return (ISC_R_SUCCESS); + } + if (sr.length == 0) { + return (ISC_R_UNEXPECTEDEND); + } + if (algorithm == DNS_KEYALG_PRIVATEDNS) { dns_name_t name; dns_decompress_setmethods(dctx, DNS_COMPRESS_NONE); diff --git a/lib/dns/tests/master_test.c b/lib/dns/tests/master_test.c index 859889f2bf..c407e3c23d 100644 --- a/lib/dns/tests/master_test.c +++ b/lib/dns/tests/master_test.c @@ -308,10 +308,12 @@ dnskey_test(void **state) { assert_int_equal(result, ISC_R_SUCCESS); } - /* * DNSKEY with no key material test: * dns_master_loadfile() understands DNSKEY with no key material + * + * RFC 4034 removed the ability to signal NOKEY, so empty key material should + * be rejected. */ static void dnsnokey_test(void **state) { @@ -321,7 +323,7 @@ dnsnokey_test(void **state) { result = test_master("testdata/master/master7.data", dns_masterformat_text, nullmsg, nullmsg); - assert_int_equal(result, ISC_R_SUCCESS); + assert_int_equal(result, ISC_R_UNEXPECTEDEND); } /* @@ -361,7 +363,7 @@ master_includelist_test(void **state) { assert_int_equal(result, DNS_R_SEENINCLUDE); assert_non_null(filename); if (filename != NULL) { - assert_string_equal(filename, "testdata/master/master7.data"); + assert_string_equal(filename, "testdata/master/master6.data"); isc_mem_free(mctx, filename); } } diff --git a/lib/dns/tests/rdata_test.c b/lib/dns/tests/rdata_test.c index 6b16d33707..0a12ac0700 100644 --- a/lib/dns/tests/rdata_test.c +++ b/lib/dns/tests/rdata_test.c @@ -398,6 +398,43 @@ check_rdata(const text_ok_t *text_ok, const wire_ok_t *wire_ok, } } +/* + * Common tests for RR types based on KEY that require key data: + * + * - CDNSKEY (RFC 7344) + * - DNSKEY (RFC 4034) + * - RKEY (draft-reid-dnsext-rkey-00) + */ +static void +key_required(void **state, dns_rdatatype_t type, size_t size) { + wire_ok_t wire_ok[] = { + /* + * RDATA must be at least 5 octets in size: + * + * - 2 octets for Flags, + * - 1 octet for Protocol, + * - 1 octet for Algorithm, + * - Public Key must not be empty. + * + * RFC 2535 section 3.1.2 allows the Public Key to be empty if + * bits 0-1 of Flags are both set, but that only applies to KEY + * records: for the RR types tested here, the Public Key must + * not be empty. + */ + WIRE_INVALID(0x00), + WIRE_INVALID(0x00, 0x00), + WIRE_INVALID(0x00, 0x00, 0x00), + WIRE_INVALID(0xc0, 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, type, size); +} + /* APL RDATA manipulations */ static void apl(void **state) { @@ -656,6 +693,11 @@ amtrelay(void **state) { dns_rdatatype_amtrelay, sizeof(dns_rdata_amtrelay_t)); } +static void +cdnskey(void **state) { + key_required(state, dns_rdatatype_cdnskey, sizeof(dns_rdata_cdnskey_t)); +} + /* * CSYNC tests. * @@ -784,6 +826,11 @@ csync(void **state) { dns_rdatatype_csync, sizeof(dns_rdata_csync_t)); } +static void +dnskey(void **state) { + key_required(state, dns_rdatatype_dnskey, sizeof(dns_rdata_dnskey_t)); +} + /* * DOA tests. * @@ -1329,6 +1376,41 @@ isdn(void **state) { dns_rdatatype_isdn, sizeof(dns_rdata_isdn_t)); } +/* + * KEY tests. + */ +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_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() + }; + + UNUSED(state); + + check_rdata(NULL, wire_ok, NULL, false, dns_rdataclass_in, + dns_rdatatype_key, sizeof(dns_rdata_key_t)); +} + /* * http://ana-3.lcs.mit.edu/~jnc/nimrod/dns.txt * @@ -1509,6 +1591,11 @@ nxt(void **state) { dns_rdatatype_nxt, sizeof(dns_rdata_nxt_t)); } +static void +rkey(void **state) { + key_required(state, dns_rdatatype_rkey, sizeof(dns_rdata_rkey_t)); +} + /* * WKS tests. * @@ -1830,18 +1917,22 @@ main(void) { cmocka_unit_test_setup_teardown(amtrelay, _setup, _teardown), cmocka_unit_test_setup_teardown(apl, _setup, _teardown), cmocka_unit_test_setup_teardown(atma, _setup, _teardown), + cmocka_unit_test_setup_teardown(cdnskey, _setup, _teardown), cmocka_unit_test_setup_teardown(csync, _setup, _teardown), cmocka_unit_test_setup_teardown(doa, _setup, _teardown), + cmocka_unit_test_setup_teardown(dnskey, _setup, _teardown), cmocka_unit_test_setup_teardown(eid, _setup, _teardown), cmocka_unit_test_setup_teardown(edns_client_subnet, _setup, _teardown), cmocka_unit_test_setup_teardown(hip, _setup, _teardown), cmocka_unit_test_setup_teardown(isdn, _setup, _teardown), + cmocka_unit_test_setup_teardown(key, _setup, _teardown), cmocka_unit_test_setup_teardown(nimloc, _setup, _teardown), cmocka_unit_test_setup_teardown(nsec, _setup, _teardown), cmocka_unit_test_setup_teardown(nsec3, _setup, _teardown), cmocka_unit_test_setup_teardown(nxt, _setup, _teardown), cmocka_unit_test_setup_teardown(wks, _setup, _teardown), + cmocka_unit_test_setup_teardown(rkey, _setup, _teardown), cmocka_unit_test_setup_teardown(zonemd, _setup, _teardown), cmocka_unit_test_setup_teardown(atcname, NULL, NULL), cmocka_unit_test_setup_teardown(atparent, NULL, NULL), diff --git a/lib/dns/tests/testdata/master/master8.data b/lib/dns/tests/testdata/master/master8.data index 2210f42354..d16b6f3d6e 100644 --- a/lib/dns/tests/testdata/master/master8.data +++ b/lib/dns/tests/testdata/master/master8.data @@ -1,4 +1,4 @@ ; -; master7.data contains a good zone file +; master6.data contains a good zone file ; -$include testdata/master/master7.data +$include testdata/master/master6.data From 53a62e2977f032c4fbf6403c0085849ba18c1017 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Sun, 24 Mar 2019 17:48:22 +1100 Subject: [PATCH 2/3] for rkey flags MUST be zero (cherry picked from commit 82d4931440d244df52f23a37412bd8d96d7be206) --- bin/tests/system/genzone.sh | 2 +- bin/tests/system/xfer/dig1.good | 2 +- bin/tests/system/xfer/dig2.good | 2 +- lib/dns/rdata/generic/key_25.c | 11 +++++++++++ lib/dns/tests/rdata_test.c | 32 ++++++++++++++++++++++++++++++++ 5 files changed, 46 insertions(+), 3 deletions(-) diff --git a/bin/tests/system/genzone.sh b/bin/tests/system/genzone.sh index a4cf7c1c3e..d2cfb9f4ff 100644 --- a/bin/tests/system/genzone.sh +++ b/bin/tests/system/genzone.sh @@ -337,7 +337,7 @@ ninfo14 NINFO "foo\;" ninfo15 NINFO "bar\\;" ; type 57 -rkey01 RKEY 512 ( 255 1 AQMFD5raczCJHViKtLYhWGz8hMY +rkey01 RKEY 0 ( 255 1 AQMFD5raczCJHViKtLYhWGz8hMY 9UGRuniJDBzC7w0aRyzWZriO6i2odGWWQVucZqKV sENW91IOW4vqudngPZsY3GvQ/xVA8/7pyFj6b7Esg a60zyGW6LFe9r8n6paHrlG5ojqf0BaqHT+8= ) diff --git a/bin/tests/system/xfer/dig1.good b/bin/tests/system/xfer/dig1.good index 7e4acf5c4a..780b9e712c 100644 --- a/bin/tests/system/xfer/dig1.good +++ b/bin/tests/system/xfer/dig1.good @@ -121,7 +121,7 @@ openpgpkey.example. 3600 IN OPENPGPKEY AQMFD5raczCJHViKtLYhWGz8hMY9UGRuniJDBzC7 ptr01.example. 3600 IN PTR example. px01.example. 3600 IN PX 65535 foo. bar. px02.example. 3600 IN PX 65535 . . -rkey01.example. 3600 IN RKEY 512 255 1 AQMFD5raczCJHViKtLYhWGz8hMY9UGRuniJDBzC7w0aRyzWZriO6i2od GWWQVucZqKVsENW91IOW4vqudngPZsY3GvQ/xVA8/7pyFj6b7Esga60z yGW6LFe9r8n6paHrlG5ojqf0BaqHT+8= +rkey01.example. 3600 IN RKEY 0 255 1 AQMFD5raczCJHViKtLYhWGz8hMY9UGRuniJDBzC7w0aRyzWZriO6i2od GWWQVucZqKVsENW91IOW4vqudngPZsY3GvQ/xVA8/7pyFj6b7Esga60z yGW6LFe9r8n6paHrlG5ojqf0BaqHT+8= rp01.example. 3600 IN RP mbox-dname.example. txt-dname.example. rp02.example. 3600 IN RP . . rt01.example. 3600 IN RT 0 intermediate-host.example. diff --git a/bin/tests/system/xfer/dig2.good b/bin/tests/system/xfer/dig2.good index 6d01d35d68..3a32309bc2 100644 --- a/bin/tests/system/xfer/dig2.good +++ b/bin/tests/system/xfer/dig2.good @@ -121,7 +121,7 @@ openpgpkey.example. 3600 IN OPENPGPKEY AQMFD5raczCJHViKtLYhWGz8hMY9UGRuniJDBzC7 ptr01.example. 3600 IN PTR example. px01.example. 3600 IN PX 65535 foo. bar. px02.example. 3600 IN PX 65535 . . -rkey01.example. 3600 IN RKEY 512 255 1 AQMFD5raczCJHViKtLYhWGz8hMY9UGRuniJDBzC7w0aRyzWZriO6i2od GWWQVucZqKVsENW91IOW4vqudngPZsY3GvQ/xVA8/7pyFj6b7Esga60z yGW6LFe9r8n6paHrlG5ojqf0BaqHT+8= +rkey01.example. 3600 IN RKEY 0 255 1 AQMFD5raczCJHViKtLYhWGz8hMY9UGRuniJDBzC7w0aRyzWZriO6i2od GWWQVucZqKVsENW91IOW4vqudngPZsY3GvQ/xVA8/7pyFj6b7Esga60z yGW6LFe9r8n6paHrlG5ojqf0BaqHT+8= rp01.example. 3600 IN RP mbox-dname.example. txt-dname.example. rp02.example. 3600 IN RP . . rt01.example. 3600 IN RT 0 intermediate-host.example. diff --git a/lib/dns/rdata/generic/key_25.c b/lib/dns/rdata/generic/key_25.c index 186351282e..0cc8b96c50 100644 --- a/lib/dns/rdata/generic/key_25.c +++ b/lib/dns/rdata/generic/key_25.c @@ -58,6 +58,9 @@ generic_fromtext_key(ARGS_FROMTEXT) { RETERR(isc_lex_getmastertoken(lexer, &token, isc_tokentype_string, false)); RETTOK(dns_keyflags_fromtext(&flags, &token.value.as_textregion)); + if (type == dns_rdatatype_rkey && flags != 0U) { + RETTOK(DNS_R_FORMERR); + } RETERR(uint16_tobuffer(flags, target)); /* protocol */ @@ -197,6 +200,10 @@ generic_fromwire_key(ARGS_FROMWIRE) { } flags = (sr.base[0] << 8) | sr.base[1]; + if (type == dns_rdatatype_rkey && flags != 0U) { + return (DNS_R_FORMERR); + } + algorithm = sr.base[3]; RETERR(mem_tobuffer(target, sr.base, 4)); isc_region_consume(&sr, 4); @@ -291,6 +298,10 @@ generic_fromstruct_key(ARGS_FROMSTRUCT) { UNUSED(type); UNUSED(rdclass); + if (type == dns_rdatatype_rkey) { + INSIST(key->flags == 0U); + } + /* Flags */ RETERR(uint16_tobuffer(key->flags, target)); diff --git a/lib/dns/tests/rdata_test.c b/lib/dns/tests/rdata_test.c index 0a12ac0700..9928803011 100644 --- a/lib/dns/tests/rdata_test.c +++ b/lib/dns/tests/rdata_test.c @@ -1593,7 +1593,39 @@ nxt(void **state) { static void rkey(void **state) { + text_ok_t text_ok[] = { + /* + * Valid, flags set to 0 and a key is present. + */ + TEXT_VALID("0 0 0 aaaa"), + /* + * Invalid, non-zero flags. + */ + TEXT_INVALID("1 0 0 aaaa"), + TEXT_INVALID("65535 0 0 aaaa"), + /* + * Sentinel. + */ + TEXT_SENTINEL() + }; + wire_ok_t wire_ok[] = { + /* + * Valid, flags set to 0 and a key is present. + */ + WIRE_VALID(0x00, 0x00, 0x00, 0x00, 0x00), + /* + * Invalid, non-zero flags. + */ + WIRE_INVALID(0x00, 0x01, 0x00, 0x00, 0x00), + WIRE_INVALID(0xff, 0xff, 0x00, 0x00, 0x00), + /* + * Sentinel. + */ + WIRE_SENTINEL() + }; key_required(state, dns_rdatatype_rkey, sizeof(dns_rdata_rkey_t)); + check_rdata(text_ok, wire_ok, NULL, false, dns_rdataclass_in, + dns_rdatatype_rkey, sizeof(dns_rdata_rkey_t)); } /* From 3c32b765c10d9b0c994d5ff0ad4ed978c24b6537 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 21 Mar 2019 22:13:33 +1100 Subject: [PATCH 3/3] add CHANGES (cherry picked from commit f78c688c4f6d8b0072fe0115093965a093227a00) --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index 748a42689d..c237e9ef12 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +5203. [bug] Enforce whether key rdata exists or not in KEY, + DNSKEY, CDNSKEY and RKEY. [GL #899] + 5202. [bug] was missing ISC_LANG_ENDDECLS. [GL #976] 5201. [bug] Fix a possible deadlock in RPZ update code. [GL #973]