Re: BUG #14329: libpq doesn't send complete client certificate chain on first SSL connection

The following bug has been logged on the website:

Bug reference: 14329
Logged by: Kacper Zuk
Email address: kzuk(at)akamai(dot)com
PostgreSQL version: 9.5.4
Operating system: GNU/Linux 4.1.20
Description:

I have PostgreSQL 9.5.4 compiled with OpenSSL 1.0.2h. Server is configured
to require SSL certificate for client. Server's root.crt only contains
top-level CA certificate and pg_hba.conf contains:

hostssl dbname dbuser 127.0.0.1/32 cert map=usermap

I'm trying to connect using psql:

psql "dbname=dbname user=dbuser host=127.0.0.1 sslmode=verify-ca
sslkey=certs/client.key sslrootcert=certs/root.crt
sslcert=certs/client-int-chain.crt"

client-int-chain.crt contains client certificate signed by intermediate
certificate and intermediate certificate signed by our top-level CA
certificate (the same as in server's root.crt).

Server rejects this connection. psql reports error "SSL error: tlsv1 alert
unknown ca". Server reports "could not accept SSL connection: certificate
verify failed" in logs.

tcpdump of traffic confirms that psql only presents client certificate,
without intermediate.

When using psycopg2 in Python (psycopg2 uses libpq and I'm testing with the
same settings and certificates as with psql), the first connection fails
with the same error, but it succeeds if it's retried. tcpdump of traffic
confirms, that on the second try both client and intermediate certificates
are sent.

It works correctly (psql connects successfully and psycopg2 connects on
first try) when PostgreSQL is compiled with OpenSSL 1.0.1. It also works
correctly if server is compiled with OpenSSL 1.0.2, but client with OpenSSL
1.0.1.

My educated guess is that in fe-secure-openssl.c in initialize_SSL function
whole certificate chain is loaded into SSL_context, but only client
certificate is loaded to SSL object. SSL object is created before loading
certificate chain into SSL_context, so it doesn't see this update. Only the
next connection, with new SSL object, picks up the certificate chain from
SSL_context. It doesn't explain why it works with OpenSSL 1.0.1 though, so
that may be a false trail.

On 09/20/2016 01:10 PM, kzuk(at)akamai(dot)com wrote:
> My educated guess is that in fe-secure-openssl.c in initialize_SSL function
> whole certificate chain is loaded into SSL_context, but only client
> certificate is loaded to SSL object. SSL object is created before loading
> certificate chain into SSL_context, so it doesn't see this update. Only the
> next connection, with new SSL object, picks up the certificate chain from
> SSL_context. It doesn't explain why it works with OpenSSL 1.0.1 though, so
> that may be a false trail.

Yeah, that's probably what's happening. The OpenSSL man page for
SSL_CTX_use_certificate() says:

> The SSL_CTX_* class of functions loads the certificates and keys into
> the SSL_CTX object ctx. The information is passed to SSL objects ssl
> created from ctx with SSL_new by copying, so that changes applied to
> ctx do not propagate to already existing SSL objects.

It says the same in both 1.0.1 and 1.0.2 versions, though. I guess we
have been relying on a bug that was fixed in 1.0.2, in that the
intermediate CA certs were actually propagated from the context to the
existing SSL object, contrary to what the man page says. I don't
immediately see any relevant change in the OpenSSL commit logs between
1.0.1 and 1.0.2, though.

I think we need to rearrange the code so that we call
SSL_CTX_use_certificate() first, and SSL_new() after that. I wonder if
that's going to break 1.0.1, though.

Setting up a test environment with the required certificates and CAs is
a bit tedious. Would you be interested in adding a test case for this in
the SSL test suite, in src/test/ssl, and posting a patch for that? I can
take a stab at fixing this, but having a test case ready would give me a
head start.

- Heikki

Heikki -

Would also be happy to set up a test case for this.... Impacts us directly.

Need a couple of days to do so, though. Please let me know your timeline.

Lou Picciano

----- Original Message -----

From: "Heikki Linnakangas" <hlinnaka(at)iki(dot)fi>
To: kzuk(at)akamai(dot)com, pgsql-bugs(at)postgresql(dot)org
Sent: Wednesday, September 21, 2016 4:06:33 AM
Subject: Re: [BUGS] BUG #14329: libpq doesn't send complete client certificate chain on first SSL connection

On 09/20/2016 01:10 PM, kzuk(at)akamai(dot)com wrote:
> My educated guess is that in fe-secure-openssl.c in initialize_SSL function
> whole certificate chain is loaded into SSL_context, but only client
> certificate is loaded to SSL object. SSL object is created before loading
> certificate chain into SSL_context, so it doesn't see this update. Only the
> next connection, with new SSL object, picks up the certificate chain from
> SSL_context. It doesn't explain why it works with OpenSSL 1.0.1 though, so
> that may be a false trail.

Yeah, that's probably what's happening. The OpenSSL man page for
SSL_CTX_use_certificate() says:

> The SSL_CTX_* class of functions loads the certificates and keys into
> the SSL_CTX object ctx. The information is passed to SSL objects ssl
> created from ctx with SSL_new by copying, so that changes applied to
> ctx do not propagate to already existing SSL objects.

It says the same in both 1.0.1 and 1.0.2 versions, though. I guess we
have been relying on a bug that was fixed in 1.0.2, in that the
intermediate CA certs were actually propagated from the context to the
existing SSL object, contrary to what the man page says. I don't
immediately see any relevant change in the OpenSSL commit logs between
1.0.1 and 1.0.2, though.

I think we need to rearrange the code so that we call
SSL_CTX_use_certificate() first, and SSL_new() after that. I wonder if
that's going to break 1.0.1, though.

Setting up a test environment with the required certificates and CAs is
a bit tedious. Would you be interested in adding a test case for this in
the SSL test suite, in src/test/ssl, and posting a patch for that? I can
take a stab at fixing this, but having a test case ready would give me a
head start.

- Heikki

--
Sent via pgsql-bugs mailing list (pgsql-bugs(at)postgresql(dot)org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On 09/21/2016 02:16 PM, Lou Picciano wrote:
> Heikki -
>
> Would also be happy to set up a test case for this.... Impacts us directly.

Great, thanks!

> Need a couple of days to do so, though. Please let me know your timeline.

Sure, I'm not in a hurry :-).

- Heikki

On 9/21/16, 1:16 PM, Lou Picciano wrote:
> Would also be happy to set up a test case for this.... Impacts us directly.

I’ll work on a test case at the beginning of the next week if Lou doesn’t finish it earlier.

Thanks,
Kacper

On 9/21/16, 10:06 AM, "Heikki Linnakangas" <hlinnaka(at)gmail(dot)com on behalf of hlinnaka(at)iki(dot)fi> wrote:
> Setting up a test environment with the required certificates and CAs is
> a bit tedious. Would you be interested in adding a test case for this in
> the SSL test suite, in src/test/ssl, and posting a patch for that? I can
> take a stab at fixing this, but having a test case ready would give me a
> head start.

I’ve attached the patch that adds a test case. Please let me know if I should send it to pgsql-hackers instead. Remember to run make sslfiles, as I didn’t include generated certificate in patch.

Thanks
Kacper

On 10/04/2016 02:08 PM, Zuk, Kacper wrote:
> On 9/21/16, 10:06 AM, "Heikki Linnakangas" <hlinnaka(at)gmail(dot)com on behalf of hlinnaka(at)iki(dot)fi> wrote:
>> Setting up a test environment with the required certificates and CAs is
>> a bit tedious. Would you be interested in adding a test case for this in
>> the SSL test suite, in src/test/ssl, and posting a patch for that? I can
>> take a stab at fixing this, but having a test case ready would give me a
>> head start.
>
> I’ve attached the patch that adds a test case. Please let me know if I should send it to pgsql-hackers instead. Remember to run make sslfiles, as I didn’t include generated certificate in patch.

Thanks!

Now that I look closer into this, I can see more trouble:

1. Since the SSL context is shared between all connections and all
threads, there's a race condition if two threads try to open a
connection at the same time. The SSL_CTX_use_certificate_chain_file()
call, and SSL_CTX_load_verify_locations() later in the function, are
protected by the global mutex, but we only hold the mutex for the
duration of those calls. It doesn't protect from the fact that while one
thread is opening a connection with one set of options, another thread
opens a connection with different options. They might get mixed up.
That's a problem if the connections use different sslcert or sslrootcert
settings.

2. Even with a single thread, using different sslrootcert options in
different PQconnectdb() calls fails outright, with error:

SSL error: block type is not 01

I got that with a test program that simply calls:

PQconnectdb(<connection string 1>);
PQfinish();
PQconnectdb(<connection string 2>);
PQfinish();

when the two connection strings point to different servers, with
different certificates signed by different root CA, and the connection
strings have different (correct) sslrootcert options for the servers.
Either PQconnectdb() call works separately, but put them in the same
program, you get that error.

I don't understand why that second error happens. I don't think we're
doing anything illegal by reusing the same SSL_CTX object for two
different connections.

I'm starting to feel that using the same SSL_CTX object for multiple
connections is just too fragile. Perhaps we could share one SSL_CTX
object for all the connections with no sslcert and no sslrootcert, but
I'm not sure if even that is worth it.

In quick testing, calling SSL_CTX_new() for each connection adds about
3% of overhead to establishing a new connection, with the default
OpenSSL settings (seems to use ECDHE-RSA-AES256-GCM-SHA384 cipher here).
I also tested memory usage with a program that opens 10000 connections,
and it used about 15% more memory, when SSL_CTX_new() is called for each
connection. I think that's acceptable. Barring objections, I'm going to
write a patch to use a separate SSL context for every connection.

- Heikki

On Tue, 4 Oct 2016 21:55:52 +0300
Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:

> 1. Since the SSL context is shared between all connections and all
> threads, there's a race condition if two threads try to open a
> connection at the same time. The SSL_CTX_use_certificate_chain_file()
> call, and SSL_CTX_load_verify_locations() later in the function, are
> protected by the global mutex, but we only hold the mutex for the
> duration of those calls. It doesn't protect from the fact that while one
> thread is opening a connection with one set of options, another thread
> opens a connection with different options. They might get mixed up.
> That's a problem if the connections use different sslcert or sslrootcert
> settings.
>
> 2. Even with a single thread, using different sslrootcert options in
> different PQconnectdb() calls fails outright, with error:
>
> SSL error: block type is not 01
>
> I got that with a test program that simply calls:
>
> PQconnectdb(<connection string 1>);
> PQfinish();
> PQconnectdb(<connection string 2>);
> PQfinish();

> I'm starting to feel that using the same SSL_CTX object for multiple
> connections is just too fragile. Perhaps we could share one SSL_CTX
> object for all the connections with no sslcert and no sslrootcert, but
> I'm not sure if even that is worth it.
>
> In quick testing, calling SSL_CTX_new() for each connection adds about
> 3% of overhead to establishing a new connection, with the default
> OpenSSL settings (seems to use ECDHE-RSA-AES256-GCM-SHA384 cipher here).
> I also tested memory usage with a program that opens 10000 connections,
> and it used about 15% more memory, when SSL_CTX_new() is called for each
> connection. I think that's acceptable. Barring objections, I'm going to
> write a patch to use a separate SSL context for every connection.

What about keeping a table of connection strings and SSL contexts so that all
connections using the same connection string could share the SSL context? It
seems likely that most applications reuse the same connection string and
could avoid the penalty.

-dg

--
David Gould 510 282 0869 daveg(at)sonic(dot)net
If simplicity worked, the world would be overrun with insects.

On 10/04/2016 10:53 PM, David Gould wrote:
> On Tue, 4 Oct 2016 21:55:52 +0300
> Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
>> I'm starting to feel that using the same SSL_CTX object for multiple
>> connections is just too fragile. Perhaps we could share one SSL_CTX
>> object for all the connections with no sslcert and no sslrootcert, but
>> I'm not sure if even that is worth it.
>>
>> In quick testing, calling SSL_CTX_new() for each connection adds about
>> 3% of overhead to establishing a new connection, with the default
>> OpenSSL settings (seems to use ECDHE-RSA-AES256-GCM-SHA384 cipher here).
>> I also tested memory usage with a program that opens 10000 connections,
>> and it used about 15% more memory, when SSL_CTX_new() is called for each
>> connection. I think that's acceptable. Barring objections, I'm going to
>> write a patch to use a separate SSL context for every connection.
>
> What about keeping a table of connection strings and SSL contexts so that all
> connections using the same connection string could share the SSL context? It
> seems likely that most applications reuse the same connection string and
> could avoid the penalty.

Yeah, could do that. Then again, I don't think it's worth the trouble.

- Heikki

On 10/04/2016 09:55 PM, Heikki Linnakangas wrote:
> I'm starting to feel that using the same SSL_CTX object for multiple
> connections is just too fragile. Perhaps we could share one SSL_CTX
> object for all the connections with no sslcert and no sslrootcert, but
> I'm not sure if even that is worth it.
>
> In quick testing, calling SSL_CTX_new() for each connection adds about
> 3% of overhead to establishing a new connection, with the default
> OpenSSL settings (seems to use ECDHE-RSA-AES256-GCM-SHA384 cipher here).
> I also tested memory usage with a program that opens 10000 connections,
> and it used about 15% more memory, when SSL_CTX_new() is called for each
> connection. I think that's acceptable. Barring objections, I'm going to
> write a patch to use a separate SSL context for every connection.

I came up with the attached patch for this. As threatened, it uses a
separate SSL context for each connection. That simplifies the code
somewhat, and fixes the bugs. Kacper's test case is included in this.
(This is for git master, stable branches will need small tweaking to
make the patch apply.)

Did some more testing with "pgbench -C". The overhead on establishing a
connection is a bit higher than I saw initially, about 6%, when
sslmode=verify-ca is used. Might be more with more complex certificate
chains. I think that's still acceptable. If you have an application that
establishes SSL connections so frequently that that matters, you should
reconsider your design.

- Heikki

On 10/5/2016 2:10 AM, Heikki Linnakangas wrote:
> If you have an application that establishes SSL connections so
> frequently that that matters, you should reconsider your design.

Hah!

(totally agree).

--
john r pierce, recycling bits in santa cruz

On 10/05/2016 12:10 PM, Heikki Linnakangas wrote:
> I came up with the attached patch for this. As threatened, it uses a
> separate SSL context for each connection. That simplifies the code
> somewhat, and fixes the bugs. Kacper's test case is included in this.
> (This is for git master, stable branches will need small tweaking to
> make the patch apply.)

I hear no objections, so committed and back-patched that.

Thanks for the report and analysis, Kacper!

- Heikki

On 10/7/16, 11:26 AM, "Heikki Linnakangas" <hlinnaka(at)gmail(dot)com on behalf of hlinnaka(at)iki(dot)fi> wrote:
> On 10/05/2016 12:10 PM, Heikki Linnakangas wrote:
> > I came up with the attached patch for this.
>
> I hear no objections, so committed and back-patched that.
>
> Thanks for the report and analysis, Kacper!

Thank you for a quick fix :)

Kacper