From 494e8b2cbd362af682e9699a9c4712cf79e27de8 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Tue, 4 May 2021 15:35:39 +0200 Subject: [PATCH 1/3] Check key-directory duplicates for kasp zones Don't allow the same zone with different dnssec-policies in separate views have the same key-directory. Track zones plus key-directory in a symtab and if there is a match, check the offending zone's dnssec-policy name. If the name is "none" (there is no kasp for the offending zone), or if the name is the same (the zone shares keys), it is fine, otherwise it is an error (zones in views using different policies cannot share the same key-directory). --- lib/bind9/check.c | 111 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 98 insertions(+), 13 deletions(-) diff --git a/lib/bind9/check.c b/lib/bind9/check.c index a7837e6caf..693b55ab8b 100644 --- a/lib/bind9/check.c +++ b/lib/bind9/check.c @@ -72,6 +72,9 @@ static isc_result_t fileexist(const cfg_obj_t *obj, isc_symtab_t *symtab, bool writeable, isc_log_t *logctxlogc); +static isc_result_t +keydirexist(const cfg_obj_t *zcgf, const char *dir, const char *kaspnamestr, + isc_symtab_t *symtab, isc_log_t *logctx, isc_mem_t *mctx); static void freekey(char *key, unsigned int type, isc_symvalue_t value, void *userarg) { UNUSED(type); @@ -2377,9 +2380,9 @@ cleanup: static isc_result_t check_zoneconf(const cfg_obj_t *zconfig, const cfg_obj_t *voptions, const cfg_obj_t *config, isc_symtab_t *symtab, - isc_symtab_t *files, isc_symtab_t *inview, const char *viewname, - dns_rdataclass_t defclass, cfg_aclconfctx_t *actx, - isc_log_t *logctx, isc_mem_t *mctx) { + isc_symtab_t *files, isc_symtab_t *keydirs, isc_symtab_t *inview, + const char *viewname, dns_rdataclass_t defclass, + cfg_aclconfctx_t *actx, isc_log_t *logctx, isc_mem_t *mctx) { const char *znamestr; const char *typestr = NULL; const char *target = NULL; @@ -2404,6 +2407,8 @@ check_zoneconf(const cfg_obj_t *zconfig, const cfg_obj_t *voptions, bool has_dnssecpolicy = false; const void *clauses = NULL; const char *option = NULL; + const char *kaspname = NULL; + const char *dir = NULL; static const char *acls[] = { "allow-notify", "allow-transfer", @@ -2633,8 +2638,8 @@ check_zoneconf(const cfg_obj_t *zconfig, const cfg_obj_t *voptions, (void)cfg_map_get(zoptions, "dnssec-policy", &obj); if (obj != NULL) { const cfg_obj_t *kasps = NULL; - const char *kaspname = cfg_obj_asstring(obj); + kaspname = cfg_obj_asstring(obj); if (strcmp(kaspname, "default") == 0) { has_dnssecpolicy = true; } else if (strcmp(kaspname, "insecure") == 0) { @@ -3188,7 +3193,8 @@ check_zoneconf(const cfg_obj_t *zconfig, const cfg_obj_t *voptions, obj = NULL; tresult = cfg_map_get(zoptions, "key-directory", &obj); if (tresult == ISC_R_SUCCESS) { - const char *dir = cfg_obj_asstring(obj); + dir = cfg_obj_asstring(obj); + tresult = isc_file_isdirectory(dir); switch (tresult) { case ISC_R_SUCCESS: @@ -3210,6 +3216,25 @@ check_zoneconf(const cfg_obj_t *zconfig, const cfg_obj_t *voptions, } } + /* + * Make sure there is no other zone with the same + * key-directory and a different dnssec-policy. + */ + if (zname != NULL) { + char keydirbuf[DNS_NAME_FORMATSIZE + 128]; + char *tmp = keydirbuf; + size_t len = sizeof(keydirbuf); + dns_name_format(zname, keydirbuf, sizeof(keydirbuf)); + tmp += strlen(tmp); + len -= strlen(tmp); + (void)snprintf(tmp, len, "/%s", (dir == NULL) ? "(null)" : dir); + tresult = keydirexist(zconfig, (const char *)keydirbuf, + kaspname, keydirs, logctx, mctx); + if (tresult != ISC_R_SUCCESS) { + result = tresult; + } + } + /* * Check various options. */ @@ -3420,6 +3445,56 @@ fileexist(const cfg_obj_t *obj, isc_symtab_t *symtab, bool writeable, return (result); } +static isc_result_t +keydirexist(const cfg_obj_t *zcfg, const char *keydir, const char *kaspnamestr, + isc_symtab_t *symtab, isc_log_t *logctx, isc_mem_t *mctx) { + isc_result_t result; + isc_symvalue_t symvalue; + char *symkey; + + if (kaspnamestr == NULL || strcmp(kaspnamestr, "none") == 0) { + return (ISC_R_SUCCESS); + } + + result = isc_symtab_lookup(symtab, keydir, 0, &symvalue); + if (result == ISC_R_SUCCESS) { + const cfg_obj_t *kasp = NULL; + const cfg_obj_t *exist = symvalue.as_cpointer; + const char *file = cfg_obj_file(exist); + unsigned int line = cfg_obj_line(exist); + + /* + * Having the same key-directory for the same zone is fine + * iff the zone is using the same policy, or has no policy. + */ + (void)cfg_map_get(cfg_tuple_get(exist, "options"), + "dnssec-policy", &kasp); + if (kasp == NULL || + strcmp(cfg_obj_asstring(kasp), "none") == 0 || + strcmp(cfg_obj_asstring(kasp), kaspnamestr) == 0) + { + return (ISC_R_SUCCESS); + } + + cfg_obj_log(zcfg, logctx, ISC_LOG_ERROR, + "key-directory '%s' already in use by zone %s with " + "policy %s: %s:%u", + keydir, + cfg_obj_asstring(cfg_tuple_get(exist, "name")), + cfg_obj_asstring(kasp), file, line); + return (ISC_R_EXISTS); + } + + /* + * Add the new zone plus key-directory. + */ + symkey = isc_mem_strdup(mctx, keydir); + symvalue.as_cpointer = zcfg; + result = isc_symtab_define(symtab, symkey, 2, symvalue, + isc_symexists_reject); + return (result); +} + /* * Check key list for duplicates key names and that the key names * are valid domain names as these keys are used for TSIG. @@ -4379,8 +4454,8 @@ check_dnstap(const cfg_obj_t *voptions, const cfg_obj_t *config, static isc_result_t check_viewconf(const cfg_obj_t *config, const cfg_obj_t *voptions, const char *viewname, dns_rdataclass_t vclass, - isc_symtab_t *files, bool check_plugins, isc_symtab_t *inview, - isc_log_t *logctx, isc_mem_t *mctx) { + isc_symtab_t *files, isc_symtab_t *keydirs, bool check_plugins, + isc_symtab_t *inview, isc_log_t *logctx, isc_mem_t *mctx) { const cfg_obj_t *zones = NULL; const cfg_obj_t *view_tkeys = NULL, *global_tkeys = NULL; const cfg_obj_t *view_mkeys = NULL, *global_mkeys = NULL; @@ -4437,8 +4512,8 @@ check_viewconf(const cfg_obj_t *config, const cfg_obj_t *voptions, const cfg_obj_t *zone = cfg_listelt_value(element); tresult = check_zoneconf(zone, voptions, config, symtab, files, - inview, viewname, vclass, actx, logctx, - mctx); + keydirs, inview, viewname, vclass, + actx, logctx, mctx); if (tresult != ISC_R_SUCCESS) { result = ISC_R_FAILURE; } @@ -5035,6 +5110,7 @@ bind9_check_namedconf(const cfg_obj_t *config, bool check_plugins, isc_result_t tresult; isc_symtab_t *symtab = NULL; isc_symtab_t *files = NULL; + isc_symtab_t *keydirs = NULL; isc_symtab_t *inview = NULL; static const char *builtin[] = { "localhost", "localnets", "any", @@ -5086,6 +5162,12 @@ bind9_check_namedconf(const cfg_obj_t *config, bool check_plugins, goto cleanup; } + tresult = isc_symtab_create(mctx, 100, freekey, mctx, false, &keydirs); + if (tresult != ISC_R_SUCCESS) { + result = tresult; + goto cleanup; + } + tresult = isc_symtab_create(mctx, 100, freekey, mctx, true, &inview); if (tresult != ISC_R_SUCCESS) { result = tresult; @@ -5094,8 +5176,8 @@ bind9_check_namedconf(const cfg_obj_t *config, bool check_plugins, if (views == NULL) { tresult = check_viewconf(config, NULL, NULL, dns_rdataclass_in, - files, check_plugins, inview, logctx, - mctx); + files, keydirs, check_plugins, inview, + logctx, mctx); if (result == ISC_R_SUCCESS && tresult != ISC_R_SUCCESS) { result = ISC_R_FAILURE; } @@ -5186,8 +5268,8 @@ bind9_check_namedconf(const cfg_obj_t *config, bool check_plugins, } if (tresult == ISC_R_SUCCESS) { tresult = check_viewconf(config, voptions, key, vclass, - files, check_plugins, inview, - logctx, mctx); + files, keydirs, check_plugins, + inview, logctx, mctx); } if (tresult != ISC_R_SUCCESS) { result = ISC_R_FAILURE; @@ -5306,6 +5388,9 @@ cleanup: if (files != NULL) { isc_symtab_destroy(&files); } + if (keydirs != NULL) { + isc_symtab_destroy(&keydirs); + } return (result); } From df1aecd5ffa4a2ffad0d1b392d3855e492087b7e Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Tue, 4 May 2021 16:30:17 +0200 Subject: [PATCH 2/3] Add checkconf tests for [#2463] Add two tests to make sure named-checkconf catches key-directory issues where a zone in multiple views uses the same directory but has different dnssec-policies. One test sets the key-directory specifically, the other inherits the default key-directory (NULL, aka the working directory). Also update the good.conf test to allow zones in different views with the same key-directory if they use the same dnssec-policy. Also allow zones in different views with different key-directories if they use different dnssec-policies. Also allow zones in different views with the same key-directories if only one view uses a dnssec-policy (the other is set to "none"). Also allow zones in different views with the same key-directories if no views uses a dnssec-policy (zone in both views has the dnssec-policy set to "none"). --- .../system/checkconf/bad-kasp-keydir1.conf | 42 +++++++++++++++++ .../system/checkconf/bad-kasp-keydir2.conf | 40 ++++++++++++++++ bin/tests/system/checkconf/good.conf | 46 +++++++++++++++++++ bin/tests/system/checkconf/good.zonelist | 8 ++++ bin/tests/system/checkconf/tests.sh | 4 ++ 5 files changed, 140 insertions(+) create mode 100644 bin/tests/system/checkconf/bad-kasp-keydir1.conf create mode 100644 bin/tests/system/checkconf/bad-kasp-keydir2.conf diff --git a/bin/tests/system/checkconf/bad-kasp-keydir1.conf b/bin/tests/system/checkconf/bad-kasp-keydir1.conf new file mode 100644 index 0000000000..5be13a1feb --- /dev/null +++ b/bin/tests/system/checkconf/bad-kasp-keydir1.conf @@ -0,0 +1,42 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +key "keyforview1" { + algorithm "hmac-sha1"; + secret "YPfMoAk6h+3iN8MDRQC004iSNHY="; +}; + +key "keyforview2" { + algorithm "hmac-sha1"; + secret "4xILSZQnuO1UKubXHkYUsvBRPu8="; +}; + +view "example1" { + match-clients { key "keyforview1"; }; + + zone "example.net" { + type primary; + dnssec-policy "default"; + key-directory "."; + file "example1.db"; + }; +}; + +view "example2" { + match-clients { key "keyforview2"; }; + + zone "example.net" { + type primary; + dnssec-policy "insecure"; + key-directory "."; + file "example2.db"; + }; +}; diff --git a/bin/tests/system/checkconf/bad-kasp-keydir2.conf b/bin/tests/system/checkconf/bad-kasp-keydir2.conf new file mode 100644 index 0000000000..67161a8436 --- /dev/null +++ b/bin/tests/system/checkconf/bad-kasp-keydir2.conf @@ -0,0 +1,40 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +key "keyforview1" { + algorithm "hmac-sha1"; + secret "YPfMoAk6h+3iN8MDRQC004iSNHY="; +}; + +key "keyforview2" { + algorithm "hmac-sha1"; + secret "4xILSZQnuO1UKubXHkYUsvBRPu8="; +}; + +view "example1" { + match-clients { key "keyforview1"; }; + + zone "example.net" { + type primary; + dnssec-policy "default"; + file "example1.db"; + }; +}; + +view "example2" { + match-clients { key "keyforview2"; }; + + zone "example.net" { + type primary; + dnssec-policy "insecure"; + file "example2.db"; + }; +}; diff --git a/bin/tests/system/checkconf/good.conf b/bin/tests/system/checkconf/good.conf index e09b9e802b..6b950996cc 100644 --- a/bin/tests/system/checkconf/good.conf +++ b/bin/tests/system/checkconf/good.conf @@ -192,7 +192,53 @@ view "fourth" { file "dnssec-none.db"; dnssec-policy "none"; }; + zone "dnssec-view1" { + type master; + file "dnssec-view41.db"; + dnssec-policy "test"; + }; + zone "dnssec-view2" { + type master; + file "dnssec-view42.db"; + }; + zone "dnssec-view3" { + type master; + file "dnssec-view43.db"; + dnssec-policy "none"; + key-directory "keys"; + }; + zone "dnssec-view4" { + type master; + file "dnssec-view44.db"; + dnssec-policy "none"; + }; dnssec-policy "default"; + key-directory "."; +}; +view "fifth" { + zone "dnssec-view1" { + type master; + file "dnssec-view51.db"; + dnssec-policy "test"; + }; + zone "dnssec-view2" { + type master; + file "dnssec-view52.db"; + dnssec-policy "test"; + key-directory "keys"; + }; + zone "dnssec-view3" { + type master; + file "dnssec-view53.db"; + dnssec-policy "default"; + key-directory "keys"; + }; + zone "dnssec-view4" { + type master; + file "dnssec-view54.db"; + dnssec-policy "none"; + }; + key-directory "."; }; view "chaos" chaos { zone "hostname.bind" chaos { diff --git a/bin/tests/system/checkconf/good.zonelist b/bin/tests/system/checkconf/good.zonelist index b33d2fc239..08a5665afd 100644 --- a/bin/tests/system/checkconf/good.zonelist +++ b/bin/tests/system/checkconf/good.zonelist @@ -13,4 +13,12 @@ dnssec-test IN fourth master dnssec-default IN fourth master dnssec-inherit IN fourth master dnssec-none IN fourth master +dnssec-view1 IN fourth master +dnssec-view2 IN fourth master +dnssec-view3 IN fourth master +dnssec-view4 IN fourth master +dnssec-view1 IN fifth master +dnssec-view2 IN fifth master +dnssec-view3 IN fifth master +dnssec-view4 IN fifth master hostname.bind chaos chaos master diff --git a/bin/tests/system/checkconf/tests.sh b/bin/tests/system/checkconf/tests.sh index e6b2e16e40..23d2c430f9 100644 --- a/bin/tests/system/checkconf/tests.sh +++ b/bin/tests/system/checkconf/tests.sh @@ -12,6 +12,8 @@ status=0 n=0 +mkdir keys + n=`expr $n + 1` echo_i "checking that named-checkconf handles a known good config ($n)" ret=0 @@ -549,5 +551,7 @@ grep "exceeds 100%" < checkconf.out$n > /dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; ret=1; fi status=`expr $status + $ret` +rmdir keys + echo_i "exit status: $status" [ $status -eq 0 ] || exit 1 From a9f4b074c43ae6d67d2081fb3de511ca0518dcb0 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Tue, 4 May 2021 16:43:40 +0200 Subject: [PATCH 3/3] Release notes and changes for [GL #2463] Mention the bugfix. --- CHANGES | 4 ++++ doc/notes/notes-current.rst | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/CHANGES b/CHANGES index b0c5f899b8..ad32dc08f6 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +5642. [bug] Check "key-directory" conflicts in "named.conf" for + zones in multiple views with different "dnssec-policy". + [GL #2463]. + 5641. [bug] Address potential memory leak in dst_key_fromnamedfile. [GL #2689] diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index c4da0ed151..a1272fb24a 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -98,3 +98,7 @@ Bug Fixes - ``named-checkconf`` now complains if zones with ``dnssec-policy`` reference the same zone file more than once. :gl:`#2603` + +- Check ``key-directory`` conflicts in ``named.conf`` for zones in multiple + views with different ``dnssec-policy``. Using the same ``key-directory`` for + such zones is not allowed. :gl:`#2463`