Re: Bug with memory leak on cert validation in libpq

+1

Specifically the openssl documentation says:

void sk_free(STACK *);
This function free()'s a stack structure. The elements in the
stack will not be freed so one should 'pop' and free all elements from the
stack before calling this function or call sk_pop_free() instead.

void sk_pop_free(STACK *st; void (*func)());
This function calls 'func' for each element on the stack, passing
the element as the argument. sk_free() is then called to free the 'stack'
structure.

On Tue, Apr 21, 2020 at 9:42 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> On Mon, Apr 20, 2020 at 08:40:06PM +0300, Roman Peshkurov wrote:
> > The problem is incorrect usage of openssl stack "destructor". We noticed
> it
> > on a graphs for the few months. It looked lika an increasing line of res
> > memory usage.
> >
> > Sorry, I've already made pull request (wasn't familiar with your flow).
> The
> > all information is described there:
> > https://github.com/postgres/postgres/pull/52
>
> Thanks for taking the time to send an email here. I suspect that few
> people around here look at the cloned repo on github (I don't FWIW).
>
> If this reference goes away, then then community archives would lose a
> part of its history. So, first, here is the patch for reference:
> --- a/src/interfaces/libpq/fe-secure-openssl.c
> +++ b/src/interfaces/libpq/fe-secure-openssl.c
> @@ -552,7 +552,7 @@ pgtls_verify_peer_name_matches_certificate_guts(PGconn
> *conn,
> if (rc != 0)
> break;
> }
> - sk_GENERAL_NAME_free(peer_san);
> + sk_GENERAL_NAME_pop_free(peer_san, GENERAL_NAME_free);
> }
>
> Looking at the samples of the OpenSSL wiki, it looks that your guess
> is right:
> https://wiki.openssl.org/index.php/Hostname_validation
> After extracting the SAN data from the certificate using
> X509_get_ext_d2i(), we get to use sk_GENERAL_NAME_pop_free() for a
> correct in-depth free().
>
> Now, we have in core a set of TAP tests in src/test/ssl with
> certificates that include some SAN configuration for
> sslmode=verify-full, able to trigger the code path you are mentioning
> for pgtls_verify_peer_name_matches_certificate_guts() within
> fe-secure-openssl.c. In order to see the leak, (I have been lazy with
> the method and did not use valgrind with a custom C program or just
> psql, but the result is the same), I have put a custom hack in the
> code path involved here by adding a tight loop repeating
> X509_get_ext_d2i() + sk_GENERAL_NAME_free(), then ran the TAP tests of
> src/test/ssl/. After that, the leak becomes really clear after 5k
> loops or such in psql, with sk_GENERAL_NAME_pop_free() preventing the
> issue any rise of memory usage.
>
> sk_GENERAL_NAME_pop_free() exists down to OpenSSL 0.9.6, which is what
> I recall we officially support down to Postgres 9.5, so we are safe.
>
> And finally, this needs to be backpatched all the way down to 9.5, so
> I'll take care of it if there are no objections.
>
> Thanks for the report, Roman!
> --
> Michael
>

--
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
CELL：+923335449950 EMAIL: mailto:hamid(dot)akhtar(at)highgo(dot)ca
SKYPE: engineeredvirus