Re: md5 issues Postgres14 on OL7
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael@paquier.xyz>
Cc: Christoph Moench-Tegeder <cmt@burggraben.net>, Michael Mühlbeyer <Michael.Muehlbeyer@trivadis.com>, "pgsql-general@lists.postgresql.org" <pgsql-general@lists.postgresql.org>
Date: 2022-01-06T16:40:04Z
Lists: pgsql-general
Attachments
- openssl-error-delta.patch (text/x-diff) patch
Michael Paquier <michael@paquier.xyz> writes:
> 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.
I don't like the end result of this at all:
postgres=# select md5('foo');
ERROR: could not compute MD5 hash: OpenSSL failure
Maybe we've successfully laid off blame somewhere else, but this
doesn't move the user one inch towards understanding the cause.
I think we need to report the actual OpenSSL error reason.
I experimented with the attached, very rough delta on top of your
patch, and got
postgres=# select md5('foo');
ERROR: could not compute MD5 hash: disabled for FIPS
which seems far better, plus it'd work for other sorts of OpenSSL
failures.
There are two problems with my delta as it stands:
1. It draws a cast-away-const warning. We'd have to make the result
of pg_cryptohash_error be "const char *", which would be better
practice anyway, but that propagates into some other APIs and I didn't
take the trouble to chase it to the end.
2. It feels a bit bogus to be fetching ERR_get_error() at this point.
Maybe it's all right to assume that the OpenSSL error stack won't
change state before we get to pg_cryptohash_error, but I don't like
the idea much. I think it'd be better to capture ERR_get_error()
sooner and store it in an additional field in pg_cryptohash_ctx.
Also, I wonder if this shouldn't be unified with the SSLerrmessage()
support found in be-secure-openssl.c and fe-secure-openssl.c.
regards, tom lane
Commits
-
Improve error handling of cryptohash computations
- 3a0cced86d3b 14.2 landed
- b69aba74578a 15.0 landed