Re: md5 issues Postgres14 on OL7

On Tue, Jan 04, 2022 at 12:54:35PM -0500, Tom Lane wrote:
> I reproduced this on Fedora 35 with FIPS mode enabled. The problem
> is that OpenSSL treats MD5 as a disallowed cipher type under FIPS
> mode, so this call in pg_cryptohash_init fails:

Is that 3.0.0 or 1.1.1? I can see the following, telling that Fedora
35 uses OpenSSL 1.1.1:
https://packages.fedoraproject.org/pkgs/openssl/openssl/

And there is FIPS 2.0 for 1.0.1 and 1.0.2 (funnily, I recall that
1.0.2+FIPS allows MD5 to work with the EVP routines), but the module
of FIPS 3.0 does not work with 1.1.1 AFAIK, so I may be confused. Or
perhaps this is OpenSSL 1.1.1 with a separate module? The upstream
code does nothing special with EVP_DigestInit_ex() in 1.1.1 (see
digest.c), contrary to 3.0.0 where there is specific knowledge of
FIPS.

> status = EVP_DigestInit_ex(ctx->evpctx, EVP_md5(), NULL);
>
> and then we come back to this in md5_text():
>
> /* get the hash result */
> if (pg_md5_hash(VARDATA_ANY(in_text), len, hexsum) == false)
> ereport(ERROR,
> (errcode(ERRCODE_OUT_OF_MEMORY),
> errmsg("out of memory")));
>
> So there's nothing actually misbehaving, but our error reportage sucks:
> the hash functions have no way to report a specific failure code,
> and the caller(s) think the only possible failure mode is OOM.

Indeed, this error is a pilot error with the cryptohash integration of
14. In ~13, the custom MD5 implementation would only fail on OOM, but
more failure modes are possible now.

> I suppose we could get around the error by using our own MD5 code
> even in OpenSSL-enabled builds, but that'd violate both the spirit
> and the letter of FIPS certification.

I don't think we should go back to an integration of our custom MD5 if
we have external libraries that provide support for it. That's
against the set of FIPS policies.

> I think the right response is
> to upgrade the error-reporting API in this area, so that the message
> could look more like "MD5 is disallowed in FIPS mode".

Hmm. I am not sure how much we should try to make the backend, or
even the frontend, FIPS-aware (remember for example 0182438 where we
avoided this kind of complexity), and not all SSL libraries we would
potentially add support for may care about such error states. The
cleanest approach may be to extend the APIs to store an **errstr so
as implementations are free to assign the error string they want to
send back for the error reporting, rather than using more error codes.
If we want to improve things in this area with FIPS (aka allow
check-world to pass in this case), we would need more in terms of
alternate test output, and extra tweaks for the authentication tests,
as well. Perhaps the best thing to do in the long term would be to
drop MD5, but we are not there yet IMO even if password_encryption
default has changed, and upgrade scenarios get hairy.

At the end, I agree that we should improve the error message in these
two cases. However, I would stick to simplicity by not assuming that
those two code paths fail only on OOM, and reword things in md5_text()
and md5_bytea() with a simple "could not compute MD5 hash". Any code
paths calling the routines of md5_common.c just do that as well for
ages when the computation fails, and that's what we care about here.
--
Michael