From: Remi Tricot-Le Breton <rlebreton@haproxy.com>
Date: Thu, 15 Dec 2022 14:44:37 +0000 (+0100)
Subject: BUG/MINOR: ssl: Fix memory leak of find_chain in ssl_sock_load_cert_chain
X-Git-Tag: v2.8-dev1~116
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4cf0d3f1e8e2ffc6901dc36644efa31b67d172f1;p=thirdparty%2Fhaproxy.git

BUG/MINOR: ssl: Fix memory leak of find_chain in ssl_sock_load_cert_chain

The certificate chain that gets passed in the SSL_CTX through
SSL_CTX_set1_chain has its reference counter increased by OpenSSL
itself. But since the ssl_sock_load_cert_chain function might create a
brand new certificate chain if none exists in the ckch_data
(sk_X509_new_null), then we ended up returning a new certificate chain
to the caller that was never destroyed.

This patch can be backported to all stable branches but it might need to
be reworked for branches older than 2.4 because of commit ec805a32b9
that refactorized the modified code.
---

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 65d0c857dc..91e7dffa8b 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -3639,6 +3639,8 @@ end:
  *     ERR_FATAL in any fatal error case
  *     ERR_ALERT if the reason of the error is available in err
  *     ERR_WARN if a warning is available into err
+ * The caller is responsible of freeing the newly built or newly refcounted
+ * find_chain element.
  * The value 0 means there is no error nor warning and
  * the operation succeed.
  */
@@ -3664,13 +3666,13 @@ static int ssl_sock_load_cert_chain(const char *path, const struct ckch_data *da
 	}
 
 	if (data->chain) {
-		*find_chain = data->chain;
+		*find_chain = X509_chain_up_ref(data->chain);
 	} else {
 		/* Find Certificate Chain in global */
 		struct issuer_chain *issuer;
 		issuer = ssl_get0_issuer_chain(data->cert);
 		if (issuer)
-			*find_chain = issuer->chain;
+			*find_chain = X509_chain_up_ref(issuer->chain);
 	}
 
 	if (!*find_chain) {
@@ -3691,14 +3693,11 @@ static int ssl_sock_load_cert_chain(const char *path, const struct ckch_data *da
 #else
 	{ /* legacy compat (< openssl 1.0.2) */
 		X509 *ca;
-		STACK_OF(X509) *chain;
-		chain = X509_chain_up_ref(*find_chain);
-		while ((ca = sk_X509_shift(chain)))
+		while ((ca = sk_X509_shift(*find_chain)))
 			if (!SSL_CTX_add_extra_chain_cert(ctx, ca)) {
 				memprintf(err, "%sunable to load chain certificate into SSL Context '%s'.\n",
 					  err && *err ? *err : "", path);
 				X509_free(ca);
-				sk_X509_pop_free(chain, X509_free);
 				errcode |= ERR_ALERT | ERR_FATAL;
 				goto end;
 			}
@@ -3791,6 +3790,7 @@ static int ssl_sock_put_ckch_into_ctx(const char *path, const struct ckch_data *
 #endif
 
  end:
+	sk_X509_pop_free(find_chain, X509_free);
 	return errcode;
 }
 
@@ -3828,6 +3828,7 @@ static int ssl_sock_put_srv_ckch_into_ctx(const char *path, const struct ckch_da
 	}
 
 end:
+	sk_X509_pop_free(find_chain, X509_free);
 	return errcode;
 }