Re: BUG #16160: Minor memory leak in case of starting postgres server with SSL encryption

Michael Paquier <[email protected]> Fri, 13 Dec 2019 15:39:15 +0900

Newsgroups	gmane.comp.db.postgresql.bugs
Message-ID	<[email protected]>

On Wed, Dec 11, 2019 at 04:22:03PM +0000, PG Bug reporting form wrote:
> According to the following information
> 
> https://wiki.openssl.org/index.php/Diffie-Hellman_parameters
> 
> DH_free function must be called after SSL_CTX_set_tmp_dh

That's not directly mentioned on their docs actually:
https://www.openssl.org/docs/man1.0.2/man3/SSL_CTX_set_tmp_dh.html

But it seems to me that you are right.  If I look at the OpenSSL code,
ssl3_ctrl() does a DH_free() on error when going though
SSL_CTRL_SET_TMP_DH, and the code copies the DH directly using
DHparams_dup() so we don't need to keep any reference to it in our
code.

One more disturbing issue is that we can would accumulate garbage if
we keep reloading the SSL context in the postmaster.  For this reason,
it could justify a backpatch down to the point where SSL parameters
are reloadable.  On the other hand, the leak is small, so my take
is actually to just fix HEAD and call it a day.

Attached is a patch, I'll go commit that if there are no objections.
The DH handling does not really change regarding the way it gets
free'd or not down to 0.9.8.
--
Michael

bessl-dh-free.patch (text/x-diff, 391 B)

diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 629919cc6e..4a6045112d 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -1017,6 +1017,8 @@ initialize_dh(SSL_CTX *context, bool isServerStart)
 						 SSLerrmessage(ERR_get_error())))));
 		return false;
 	}
+
+	DH_free(dh);
 	return true;
 }

signature.asc (application/pgp-signature, 833 B)