Re: Bug with memory leak on cert validation in libpq - Mailing list pgsql-bugs

From Hamid Akhtar
Subject Re: Bug with memory leak on cert validation in libpq
Date
Msg-id CANugjhs+5LHeRpNwsK4+vxisxe9LkLKybYQWSGVCbUyt5j0SXA@mail.gmail.com
Whole thread Raw
In response to Re: Bug with memory leak on cert validation in libpq  (Michael Paquier <michael@paquier.xyz>)
List pgsql-bugs
+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@paquier.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.akhtar@highgo.ca
SKYPE: engineeredvirus

pgsql-bugs by date:

Previous
From: Michael Paquier
Date:
Subject: Re: [BUG] non archived WAL removed during production crash recovery
Next
From: Kyotaro Horiguchi
Date:
Subject: Re: [BUG] non archived WAL removed during production crash recovery