Date: Thu, 18 Feb 2021 11:04:05 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Jacob Champion <pchampion@vmware.com>
Cc: "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Subject: Re: cryptohash: missing locking functions for OpenSSL <= 1.0.2?
Message-ID: <YC3LFcb5/k2o89Ho@paquier.xyz>
References: <fd3ba610085f1ff54623478cf2f7adf5af193cbb.camel@vmware.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="A4BPO1AEM/bXA+3C"
Content-Disposition: inline
In-Reply-To: <fd3ba610085f1ff54623478cf2f7adf5af193cbb.camel@vmware.com>
Precedence: bulk


--A4BPO1AEM/bXA+3C
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Feb 17, 2021 at 06:34:36PM +0000, Jacob Champion wrote:
> This would only affect threaded libpq clients running OpenSSL 1.0.2 and
> below, and it looks like the most likely code path to be affected is
> the OpenSSL error stack. So if anything went wrong with one of those
> hash calls, it's possible that libpq would crash (or worse, silently
> misbehave somewhere in the TLS stack) instead of gracefully reporting
> an error. [2] is an example of this in the wild.

I have been discussing a bit this issue with Jacob, and that's a
problem we would need to address on HEAD.  First, I have been looking
at this stuff in older versions with MD5 and SHA256 used by SCRAM when
it comes to ~13 with libpq:
- MD5 is based on the internal implementation of Postgres even when
building libpq with OpenSSL, so that would not be an issue.
- SHA256 is a different story though, because when building with
OpenSSL we would go through SHA256_{Init,Update,Final} for SCRAM
authentication.  In the context of a SSL connection, the crypto part
is initialized.  But that would *not* be the case of a connection in a
non-SSL context.  Fortunately, and after looking at the OpenSSL code
(fips_md_init_ctx, HASH_UPDATE, etc.), there is no sign of locking
handling or errors, so I think that we are safe there.

Now comes the case of HEAD that uses EVP for MD5 and SHA256.  A SSL
connection would initialize the crypto part, but that does not happen
for a non-SSL connection.  So, logically, one could run into issues if
using MD5 or SCRAM with OpenSSL <= 1.0.2 (pgbench with a high number
of threads does not complain by the way), and we are not yet in a
stage where we should drop this version either, even if it has been
EOL'd by upstream at the end of 2019.

We have the code in place to properly initialize the crypto locking in
libpq with ENABLE_THREAD_SAFETY, but the root of the issue is that the
SSL and crypto initializations are grouped together.  What we need to
do here is to split those phases of the initialization so as non-SSL
connections can use the crypto part properly, as pqsecure_initialize
gets only called now when libpq negotiates SSL with the postmaster.
It seems to me that we should make sure of a proper reset of the
crypto part within pqDropConnection(), while the initialization needs
to happen in PQconnectPoll().
--
Michael

--A4BPO1AEM/bXA+3C
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmAtyxUACgkQnvQgOdby
QH0rABAAj6aPKbcLJZOQZ+/9/vsgAkOGlyIQzCsuBk/TugQTim7T29reNfcne929
V7ixSvwfrRkR+Tvr7EfFFHjuxSkLyY+OZTzCRx4jEwWX5fOfvPokfH6w5StGvD47
cg4CayYbrMxzCGiRhgpalHO8uF0TtWwkwWATJa+jQC8+rxJi9YKk2k3bQ77IzW3y
QyZn4WfKk+LefxXUyOrguxfxGLTMO/2bNaMyiscIWoXIUTdNb5eGbsfETq/8u9rw
lrQ1dXMcXQAZ+q3oGhzYe+Gc1FmqcHSzQjB0i0v0gzx7Btoc5dnPNN0ug72glquv
N7gLhZWLNejXBXLwV7DghPkhmK41AVNOqAQ3QIYXaLQrqlne3QYJCEENaQhe53MY
yWX0xuaAYKbXPM6LEagsoWUscfAeCS5OWngnbH2/OFXcbcGwDyFMe87ivEglWTQL
6zr0lXRpJI/sfE1jiPiN9qy2nNuLzc42jbK61QOnVntdxsqq1WXzPjKajo8wXLlc
iFB09Qys8j7r1pLmc4m9BfCvDHrDUGK+UUzVJbAqbV5TWT+QgSmWXE/XlJ8RCYja
iNqxCXXgSr6PP7G8q+obwlcx8LpdRDS5/rLaB97cIZBCDCEz31A+p2x2/+4tYLkK
bUCQbgp/QpyomvbrvVv5Yn9WYJlsOyaoMhh9+9bV7broE9URjIA=
=1Rbf
-----END PGP SIGNATURE-----

--A4BPO1AEM/bXA+3C--