From: William Lallemand <wlallemand@haproxy.org>
Date: Wed, 31 Aug 2022 12:26:49 +0000 (+0200)
Subject: BUG/MINOR: ssl: leak of ckch_inst_link in ckch_inst_free() v2
X-Git-Tag: v2.7-dev5~29
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e0fa91ffe16a91d758154ff900d16b16b368e81b;p=thirdparty%2Fhaproxy.git

BUG/MINOR: ssl: leak of ckch_inst_link in ckch_inst_free() v2

ckch_inst_free() unlink the ckch_inst_link structure but never free it.

It can't be fixed simply because cli_io_handler_commit_cafile_crlfile()
is using a cafile_entry list to iterate a list of ckch_inst entries
to free. So both cli_io_handler_commit_cafile_crlfile() and
ckch_inst_free() would modify the list at the same time.

In order to let the caller manipulate the ckch_inst_link,
ckch_inst_free() now checks if the element is still attached before
trying to detach and free it.

For this trick to work, the caller need to do a LIST_DEL_INIT() during
the iteration over the ckch_inst_link.

list_for_each_entry was also replace by a while (!LIST_ISEMPTY()) on the
head list in cli_io_handler_commit_cafile_crlfile() so the iteration
works correctly, because it could have been stuck on the first detached
element. list_for_each_entry_safe() is not enough to fix the issue since
multiple element could have been removed.

Must be backported as far as 2.5.
---

diff --git a/src/ssl_ckch.c b/src/ssl_ckch.c
index 6db62e193c..ae206447c7 100644
--- a/src/ssl_ckch.c
+++ b/src/ssl_ckch.c
@@ -986,8 +986,15 @@ void ckch_inst_free(struct ckch_inst *inst)
 	LIST_DELETE(&inst->by_ckchs);
 	LIST_DELETE(&inst->by_crtlist_entry);
 
+	/* Free the cafile_link_refs list */
 	list_for_each_entry_safe(link_ref, link_ref_s, &inst->cafile_link_refs, list) {
-		LIST_DELETE(&link_ref->link->list);
+		if (link_ref->link && LIST_INLIST(&link_ref->link->list)) {
+			/* Try to detach and free the ckch_inst_link only if it
+			 * was attached, this way it can be used to loop from
+			 * the caller */
+			LIST_DEL_INIT(&link_ref->link->list);
+			ha_free(&link_ref->link);
+		}
 		LIST_DELETE(&link_ref->list);
 		free(link_ref);
 	}
@@ -2908,12 +2915,18 @@ static int cli_io_handler_commit_cafile_crlfile(struct appctx *appctx)
 					__ssl_sock_load_new_ckch_instance(ckchi_link->ckch_inst);
 				}
 
-				/* delete the old sni_ctx, the old ckch_insts and the ckch_store */
-				list_for_each_entry(ckchi_link, &old_cafile_entry->ckch_inst_link, list) {
+				/* delete the old sni_ctx, the old ckch_insts
+				 * and the ckch_store. ckch_inst_free() also
+				 * manipulates the list so it's cleaner to loop
+				 * until it's empty  */
+				while (!LIST_ISEMPTY(&old_cafile_entry->ckch_inst_link)) {
+					ckchi_link = LIST_ELEM(old_cafile_entry->ckch_inst_link.n, typeof(ckchi_link), list);
+
+					LIST_DEL_INIT(&ckchi_link->list); /* must reinit because ckch_inst checks the list */
 					__ckch_inst_free_locked(ckchi_link->ckch_inst);
+					free(ckchi_link);
 				}
 
-
 				/* Remove the old cafile entry from the tree */
 				ebmb_delete(&old_cafile_entry->node);
 				ssl_store_delete_cafile_entry(old_cafile_entry);