Re: md5 issues Postgres14 on OL7

On Wed, Jan 05, 2022 at 04:09:12PM +0900, Michael Paquier wrote:
> In order to make things portable with 14 in cryptohash.c, we don't
> have any need to change the existing cryptohash APIs. We could just
> store in each implementation context a location to a static string,
> and add a new routine to extract it if there is an error, defaulting
> to OOM.

I have been looking at that, and finished with the attached. It is
close to the end of the day, so this needs an extra lookup, but I have
finished by using the idea of an extra routine, called
pg_cryptohash_error(), able to grab the error saved in the private
contexts, so as any callers from the backend or the frontend can feed
on that. This way, it is possible to make the difference between
several class of errors: OOMs, a too short destination buffer, OpenSSL
internal error, etc.

There are a couple of things worth noting here:
- Two code paths of src/backend/libpq/crypt.c rely on the result of
pg_md5_encrypt() to always be an OOM, so as this skips one
psnprintf(). This has been let as-is in the backend for now, but we
don't pfree() the *logdetail strings passed across the various layers,
so we could just pass down the cryptohash error as-is.. We'd better
mention that logdetail may not be palloc'd all the time, once we do
that. libpq is able to use that properly.
- The routines of md5_common.c need to pass down an extra *errstr for
their respective callers. That's an ABI breakage but I'd like to
think that nobody uses that out-of-core. (I need to double-check this
part, as well).
- HMAC (hmac_openssl.c and hmac.c) could use the same infra, but I did
not see a use for that yet. It is possible to compile HMACs with MD5s,
but we don't have any in-core callers, and failures are just part of
the SCRAM workflow with dedicated error messages.

I am still not sure about the FIPS part, as per the argument of
OpenSSL using something different in 3.0.0, and Fedora that patches
upstream in its own way, but this could be extended in
cryptohash_openssl.c to provide even more context. For now, this
allows reports about OpenSSL internal failures, taking care of the
disturbance.

Thoughts?
--
Michael