Re: md5 issues Postgres14 on OL7 - Mailing list pgsql-general

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

diff -u cryptohash_openssl.c.orig cryptohash_openssl.c
--- cryptohash_openssl.c.orig    2022-01-06 11:15:59.268256281 -0500
+++ cryptohash_openssl.c    2022-01-06 11:22:28.602734304 -0500
@@ -21,6 +21,7 @@
 #include "postgres_fe.h"
 #endif
 
+#include <openssl/err.h>
 #include <openssl/evp.h>
 
 #include "common/cryptohash.h"
@@ -309,7 +310,14 @@
         case PG_CRYPTOHASH_ERROR_DEST_LEN:
             return _("destination buffer too small");
         case PG_CRYPTOHASH_ERROR_OPENSSL:
-            return _("OpenSSL failure");
+            {
+                unsigned long ecode = ERR_get_error();
+                const char *errreason = ERR_reason_error_string(ecode);
+
+                if (errreason)
+                    return errreason;
+                return _("OpenSSL failure");
+            }
     }
 
     /* assume that the default is out-of-memory, anyway */