Re: Cryptohash OpenSSL error queue in FIPS enabled builds

On Sat, Apr 23, 2022 at 11:40:19PM +0200, Daniel Gustafsson wrote:
> On 22 Apr 2022, at 19:01, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
>>> Consuming all (both) errors and creating a concatenated string seems overkill
>>> as it would alter the API from a const error string to something that needs
>>> freeing etc (also, very few OpenSSL consumers actually drain the queue, OpenSSL
>>> themselves don't). Skimming the OpenSSL code I was unable to find another
>>> example of two errors generated. The attached calls ERR_clear_error() as how
>>> we do in libpq in order to avoid consuming earlier errors.

It looks like the initialization error would come only from
evp_md_init_internal() in digest.c.

>> This seems quite messy. How would clearing the queue *before* creating
>> the object improve matters?
>
> We know there won't be any leftovers which would make us display the wrong
> message.

Yeah.

>> It seems like that solution means you're leaving an extra error in the queue to
>> break unrelated code. Wouldn't it be better to clear after grabbing the error?
>> (Or maybe do both.)
>
> That's a very good point, doing it in both ends of the operation is better
> here.

Error queues are cleaned with ERR_clear_error() before specific SSL
calls in the frontend and the backend, never after the fact. If we
assume that multiple errors can be stacked in the OpenSSL error queue,
shouldn't we worry about cleaning up the error queue in code paths
like pgtls_read/write(), be_tls_read/write() and be_tls_open_server()?
So it seems to me that SSLerrmessage() should be treated the same way
for the backend and the frontend. Any opinions?

pgcrypto's openssl.c has the same problem under FIPS as it includes
EVP calls. Saying that, putting a cleanup in pg_cryptohash_create()
before the fact, and one in SSLerrmessage() after consuming the error
code should be fine to keep a clean queue.

Daniel, were you planning to write a patch? The other parts of the
code are older than the hmac and cryptohash business, but I would not
mind writing something for the whole.
--
Michael