Horton

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

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Jelte Fennema <postgres@jeltef.nl>, Michael Paquier <michael@paquier.xyz>, duspensky@ya.ru, pgsql-bugs@lists.postgresql.org
Date: 2021-03-16T18:01:09Z
Lists: pgsql-bugs 
Hi,

On 2021-03-16 12:31:17 -0400, Tom Lane wrote:
> However, how sure are you really that this specific bug accounts for
> all of the leakage you saw?

I'd not be surprised if there were more...


> I'm wondering about that because I see some other stuff in be_tls_init()
> that looks like it might get leaked, notably the root_cert_list read
> from the ssl_ca_file.

I think that specific instance should be, at least in the non-error
paths, fine though:

https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_client_CA_list.html
> SSL_CTX_set_client_CA_list() sets the list of CAs sent to the client
> when requesting a client certificate for ctx. Ownership of list is
> transferred to ctx and it should not be freed by the caller.

So it seems like we'd need cleanup root_cert_list in case of errors
(we'd not reach the SSL_CTX_set_client_CA_list), but not otherwise?


Given that we're careful to destroy the "temporary" ssl context in case
of error, perhaps the best way to deal with root_cert_list being freed
in case of error would be to assign it to the context as soon as its
loaded?


> This code was originally meant to be run exactly
> once at postmaster start, so it's not too surprising that it's not as
> careful as it now needs to be.

Yea.

Greetings,

Andres Freund

Commits

  1. Fix memory leak when rejecting bogus DH parameters.

  2. Fix memory leak when initializing DH parameters in backend

  3. Avoid corner-case memory leak in SSL parameter processing.