From d97dc03b8e933dc3dfc939934354362b8d7567bb Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 20 Sep 2023 12:53:35 +1000 Subject: [PATCH 1/3] Detect duplicate use of control sockets in named.conf Specifying duplicate control sockets can lead to hard to diagnose rndc connection failures. --- lib/isccfg/check.c | 66 +++++++++++++++++++++++++++++++++------------- 1 file changed, 47 insertions(+), 19 deletions(-) diff --git a/lib/isccfg/check.c b/lib/isccfg/check.c index 2575999adb..148d5c6c4a 100644 --- a/lib/isccfg/check.c +++ b/lib/isccfg/check.c @@ -69,6 +69,8 @@ #include +#define NAMED_CONTROL_PORT 953 + static in_port_t dnsport = 53; static isc_result_t @@ -451,9 +453,8 @@ disabled_ds_digests(const cfg_obj_t *disabled, isc_log_t *logctx) { } static isc_result_t -nameexist(const cfg_obj_t *obj, const char *name, int value, - isc_symtab_t *symtab, const char *fmt, isc_log_t *logctx, - isc_mem_t *mctx) { +exists(const cfg_obj_t *obj, const char *name, int value, isc_symtab_t *symtab, + const char *fmt, isc_log_t *logctx, isc_mem_t *mctx) { char *key; const char *file; unsigned int line; @@ -504,10 +505,10 @@ mustbesecure(const cfg_obj_t *secure, isc_symtab_t *symtab, isc_log_t *logctx, str); } else { dns_name_format(name, namebuf, sizeof(namebuf)); - result = nameexist(secure, namebuf, 1, symtab, - "dnssec-must-be-secure '%s': already " - "exists previous definition: %s:%u", - logctx, mctx); + result = exists(secure, namebuf, 1, symtab, + "dnssec-must-be-secure '%s': already exists " + "previous definition: %s:%u", + logctx, mctx); } return (result); } @@ -2911,14 +2912,14 @@ check_zoneconf(const cfg_obj_t *zconfig, const cfg_obj_t *voptions, zname = dns_fixedname_name(&fixedname); dns_name_format(zname, namebuf, sizeof(namebuf)); - tresult = nameexist(zconfig, namebuf, - ztype == CFG_ZONE_HINT ? 1 - : ztype == CFG_ZONE_REDIRECT ? 2 - : 3, - symtab, - "zone '%s': already exists " - "previous definition: %s:%u", - logctx, mctx); + tresult = exists( + zconfig, namebuf, + ztype == CFG_ZONE_HINT ? 1 + : ztype == CFG_ZONE_REDIRECT ? 2 + : 3, + symtab, + "zone '%s': already exists previous definition: %s:%u", + logctx, mctx); if (tresult != ISC_R_SUCCESS) { result = tresult; } @@ -4932,10 +4933,9 @@ check_catz(const cfg_obj_t *catz_obj, const char *viewname, isc_mem_t *mctx, } dns_name_format(name, namebuf, sizeof(namebuf)); - tresult = - nameexist(nameobj, namebuf, 1, symtab, - "catalog zone '%s': already added here %s:%u", - logctx, mctx); + tresult = exists(nameobj, namebuf, 1, symtab, + "catalog zone '%s': already added here %s:%u", + logctx, mctx); if (tresult != ISC_R_SUCCESS) { result = tresult; continue; @@ -5637,8 +5637,10 @@ check_controls(const cfg_obj_t *config, isc_log_t *logctx, isc_mem_t *mctx) { const cfg_obj_t *inetcontrols; const cfg_obj_t *unixcontrols; const cfg_obj_t *keylist = NULL; + const cfg_obj_t *obj = NULL; const char *path; dns_acl_t *acl = NULL; + isc_symtab_t *symtab = NULL; (void)cfg_map_get(config, "controls", &controlslist); if (controlslist == NULL) { @@ -5649,6 +5651,11 @@ check_controls(const cfg_obj_t *config, isc_log_t *logctx, isc_mem_t *mctx) { cfg_aclconfctx_create(mctx, &actx); + result = isc_symtab_create(mctx, 100, freekey, mctx, true, &symtab); + if (result != ISC_R_SUCCESS) { + goto cleanup; + } + /* * INET: Check allow clause. * UNIX: Not supported. @@ -5664,6 +5671,9 @@ check_controls(const cfg_obj_t *config, isc_log_t *logctx, isc_mem_t *mctx) { for (element2 = cfg_list_first(inetcontrols); element2 != NULL; element2 = cfg_list_next(element2)) { + char socktext[ISC_SOCKADDR_FORMATSIZE]; + isc_sockaddr_t addr; + control = cfg_listelt_value(element2); allow = cfg_tuple_get(control, "allow"); tresult = cfg_acl_fromconfig(allow, config, logctx, @@ -5678,6 +5688,20 @@ check_controls(const cfg_obj_t *config, isc_log_t *logctx, isc_mem_t *mctx) { if (tresult != ISC_R_SUCCESS) { result = tresult; } + obj = cfg_tuple_get(control, "address"); + addr = *cfg_obj_assockaddr(obj); + if (isc_sockaddr_getport(&addr) == 0) { + isc_sockaddr_setport(&addr, NAMED_CONTROL_PORT); + } + isc_sockaddr_format(&addr, socktext, sizeof(socktext)); + tresult = exists( + obj, socktext, 1, symtab, + "inet control socket '%s': already defined, " + "previous definition: %s:%u", + logctx, mctx); + if (tresult != ISC_R_SUCCESS) { + result = tresult; + } } for (element2 = cfg_list_first(unixcontrols); element2 != NULL; element2 = cfg_list_next(element2)) @@ -5689,7 +5713,11 @@ check_controls(const cfg_obj_t *config, isc_log_t *logctx, isc_mem_t *mctx) { result = ISC_R_FAMILYNOSUPPORT; } } +cleanup: cfg_aclconfctx_detach(&actx); + if (symtab != NULL) { + isc_symtab_destroy(&symtab); + } return (result); } From 1bf62b1c88fb04527d90559da6e0502b3105f9e9 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 20 Sep 2023 12:58:14 +1000 Subject: [PATCH 2/3] Check that duplicate control sockets are caught --- .../checkconf/bad-controls-duplicate.conf | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 bin/tests/system/checkconf/bad-controls-duplicate.conf diff --git a/bin/tests/system/checkconf/bad-controls-duplicate.conf b/bin/tests/system/checkconf/bad-controls-duplicate.conf new file mode 100644 index 0000000000..d53a7cfb06 --- /dev/null +++ b/bin/tests/system/checkconf/bad-controls-duplicate.conf @@ -0,0 +1,30 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * SPDX-License-Identifier: MPL-2.0 + * + * 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 https://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +key rndc-key { + algorithm "hmac-sha256"; + secret "xxxxxxxxxxxxxxxxxxxxxxxx"; +}; + +key ddns-key { + algorithm "hmac-sha256"; + secret "yyyyyyyyyyyyyyyyyyyyyyyy"; +}; + +controls { + inet 127.0.0.1 port 953 allow { 127.0.0.1; } keys { "rndc-key"; }; +}; + +controls { + inet 127.0.0.1 allow { 127.0.0.1; } keys { ddns-key; }; +}; From e8a822d0a713e24a2f0cf9abb81bbdf906072af3 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 20 Sep 2023 13:03:10 +1000 Subject: [PATCH 3/3] Add CHANGES note for [GL #4253] --- CHANGES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index 362f7bc2a7..0a348dd1f3 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +6262. [bug] Duplicate control sockets didn't generate a + configuration failure leading to hard to diagnose + rndc connection errors. These are now caught by + named-checkconf and named. [GL #4253] + 6261. [bug] Fix a possible assertion failure on an error path in resolver.c:fctx_query(), when using an uninitialized link. [GL #4331]