Re: scram-sha-256 broken with FIPS and OpenSSL 1.0.2

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Daniel Gustafsson <daniel(at)yesql(dot)se>
Cc: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: scram-sha-256 broken with FIPS and OpenSSL 1.0.2
Date: 2020-12-02 03:03:49
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Tue, Dec 01, 2020 at 10:10:45AM +0100, Daniel Gustafsson wrote:
> That sounds like it would work. Since the cryptohash implementation owns and
> controls the void *data contents it can store whatever it needs to properly
> free it.

That seems to work properly. I have injected some elog(ERROR) with
the attached in some of the SQL functions from cryptohashes.c and the
cleanup happens with the correct resowner references. It felt a bit
strange to use directly the term EVP in resowner.c once I did this
switch, so this is renamed to "CryptoHash" instead.

> Reading through the v6 patch I see nothing sticking out and all review comments
> addressed, +1 on applying that one and then we'll take if from there with the
> remaining ones in the patchset.

Thanks. 0001 has been applied and the buildfarm does not complain, so
it looks like we are good (I'll take care of any issues, like the one
Fujii-san has just reported). Attached are new patches for 0002, the
EVP switch. One thing I noticed is that we need to free the backup
manifest a bit earlier once we begin to use resource owner in
basebackup.c as there is a specific step that may do a double-free.
This would not happen when not using OpenSSL or on HEAD. It would be
easy to separate the resowner and cryptohash portions of the patch
here, but both are tightly linked, so I'd prefer to keep them

Another thing to note is that this makes 0003 for pgcrypto
meaningless, because this would track down only states created by
cryptohash_openssl.c, and not directly EVP_MD_CTX. Consistency in the
backend code is for the best, and considering that pgcrypto has
similar linked lists for ciphers it feels a bit weird to switch only
half of it to use something in resowner.c. So, I am not sure if we
need to do anything here, and I am discarding this part.

Attachment Content-Type Size
v7-0001-Switch-cryptohash_openssl.c-to-use-EVP.patch text/x-diff 11.1 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Noah Misch 2020-12-02 03:51:31 Re: Recent eelpout failures on 9.x branches
Previous Message 2020-12-02 02:21:52 RE: Disable WAL logging to speed up data loading