From ae997d9e210033c6dea7a236a2a4783bfd31b376 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 24 Apr 2023 12:40:33 +0200 Subject: [PATCH 1/3] Add ISC_LIST_FOREACH(_SAFE) macros There's a recurring pattern walking the ISC_LISTs that just repeats over and over. Add two macros: * ISC_LIST_FOREACH(list, elt, link) - walk the static list * ISC_LIST_FOREACH_SAFE(list, elt, link, next) - walk the list in a manner that's safe against list member deletions --- .clang-format | 2 +- lib/isc/include/isc/list.h | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/.clang-format b/.clang-format index 6a8388bc91..124d82a269 100644 --- a/.clang-format +++ b/.clang-format @@ -78,4 +78,4 @@ PenaltyBreakString: 80 PenaltyExcessCharacter: 100 Standard: Cpp11 ContinuationIndentWidth: 8 -ForEachMacros: [ 'cds_lfs_for_each', 'cds_lfs_for_each_safe', 'cds_list_for_each_entry_safe' ] +ForEachMacros: [ 'cds_lfs_for_each', 'cds_lfs_for_each_safe', 'cds_list_for_each_entry_safe', 'ISC_LIST_FOREACH', 'ISC_LIST_FOREACH_SAFE' ] diff --git a/lib/isc/include/isc/list.h b/lib/isc/include/isc/list.h index 2cf4437542..a168254d40 100644 --- a/lib/isc/include/isc/list.h +++ b/lib/isc/include/isc/list.h @@ -227,3 +227,17 @@ INSIST(ISC_LIST_EMPTY(dest)); \ ISC_LIST_MOVEUNSAFE(dest, src); \ } + +/* clang-format off */ +#define ISC_LIST_FOREACH(list, elt, link) \ + for (elt = ISC_LIST_HEAD(list); \ + elt != NULL; \ + elt = ISC_LIST_NEXT(elt, link)) +/* clang-format on */ + +/* clang-format off */ +#define ISC_LIST_FOREACH_SAFE(list, elt, link, next) \ + for (elt = ISC_LIST_HEAD(list), next = (elt != NULL) ? ISC_LIST_NEXT(elt, link) : NULL; \ + elt != NULL; \ + elt = next, next = (elt != NULL) ? ISC_LIST_NEXT(elt, link) : NULL) +/* clang-format on */ From 27ad3a65f9cc6a26c626fe982d28be966461a2d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 24 Apr 2023 12:45:54 +0200 Subject: [PATCH 2/3] Fix potential UAF when shutting down isc_httpd Use the ISC_LIST_FOREACH_SAFE() macro to safely walk the running https and shut them down in a manner safe from deletion. --- lib/isc/httpd.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/lib/isc/httpd.c b/lib/isc/httpd.c index ef92a3e093..2dd7061f82 100644 --- a/lib/isc/httpd.c +++ b/lib/isc/httpd.c @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -278,8 +279,6 @@ httpdmgr_detach(isc_httpdmgr_t **httpdmgrp) { static void destroy_httpdmgr(isc_httpdmgr_t *httpdmgr) { - isc_httpdurl_t *url; - isc_refcount_destroy(&httpdmgr->references); LOCK(&httpdmgr->lock); @@ -297,12 +296,11 @@ destroy_httpdmgr(isc_httpdmgr_t *httpdmgr) { * Clear out the list of all actions we know about. Just free the * memory. */ - url = ISC_LIST_HEAD(httpdmgr->urls); - while (url != NULL) { + isc_httpdurl_t *url, *next; + ISC_LIST_FOREACH_SAFE (httpdmgr->urls, url, link, next) { isc_mem_free(httpdmgr->mctx, url->url); ISC_LIST_UNLINK(httpdmgr->urls, url, link); isc_mem_put(httpdmgr->mctx, url, sizeof(isc_httpdurl_t)); - url = ISC_LIST_HEAD(httpdmgr->urls); } UNLOCK(&httpdmgr->lock); @@ -746,12 +744,10 @@ prepare_response(isc_httpdmgr_t *mgr, isc_httpd_t *httpd, } LOCK(&mgr->lock); - url = ISC_LIST_HEAD(mgr->urls); - while (url != NULL) { + ISC_LIST_FOREACH (mgr->urls, url, link) { if (strncmp(path, url->url, path_len) == 0) { break; } - url = ISC_LIST_NEXT(url, link); } UNLOCK(&mgr->lock); @@ -934,7 +930,6 @@ close_readhandle: void isc_httpdmgr_shutdown(isc_httpdmgr_t **httpdmgrp) { isc_httpdmgr_t *httpdmgr = NULL; - isc_httpd_t *httpd = NULL; REQUIRE(httpdmgrp != NULL); REQUIRE(VALID_HTTPDMGR(*httpdmgrp)); @@ -947,13 +942,12 @@ isc_httpdmgr_shutdown(isc_httpdmgr_t **httpdmgrp) { LOCK(&httpdmgr->lock); httpdmgr->flags |= ISC_HTTPDMGR_SHUTTINGDOWN; - httpd = ISC_LIST_HEAD(httpdmgr->running); - while (httpd != NULL) { + isc_httpd_t *httpd, *next; + ISC_LIST_FOREACH_SAFE (httpdmgr->running, httpd, link, next) { if (httpd->handle != NULL) { httpd_request(httpd->handle, ISC_R_SUCCESS, NULL, httpd); } - httpd = ISC_LIST_NEXT(httpd, link); } UNLOCK(&httpdmgr->lock); From 6f0d0c49f906be37d935ee0117dc3cedbecc352c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 24 Apr 2023 12:48:46 +0200 Subject: [PATCH 3/3] Add CHANGES note for [GL #4031] --- CHANGES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index f72a05822e..1024fb8453 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +6158. [func] Add ISC_LIST_FOREACH() and ISC_LIST_FOREACH_SAFE() + to walk the ISC_LIST() in a unified manner and use + the safe macro to fix the potential UAF when shutting + down the isc_httpd. [GL #4031] + 6157. [bug] When removing delegations in an OPTOUT range empty-non-terminal NSEC3 records generated by those delegations where not removed. [GL #4027]