Improve error handling of HMAC computations

This is similar to b69aba7, except that this completes the work for
HMAC with a new routine called pg_hmac_error() that would provide more
context about the type of error that happened during a HMAC computation:
- The fallback HMAC implementation in hmac.c relies on cryptohashes, so
in some code paths it is necessary to return back the error generated by
cryptohashes.
- For the OpenSSL implementation (hmac_openssl.c), the logic is very
similar to cryptohash_openssl.c, where the error context comes from
OpenSSL if one of its internal routines failed, with different error
codes if something internal to hmac_openssl.c failed or was incorrect.

Any in-core code paths that use the centralized HMAC interface are
related to SCRAM, for errors that are unlikely going to happen, with
only SHA-256.  It would be possible to see errors when computing some
HMACs with MD5 for example and OpenSSL FIPS enabled, and this commit
would help in reporting the correct errors but nothing in core uses
that.  So, at the end, no backpatch to v14 is done, at least for now.

Errors in SCRAM related to the computation of the server key, stored
key, etc. need to pass down the potential error context string across
more layers of their respective call stacks for the frontend and the
backend, so each surrounding routine is adapted for this purpose.

Reviewed-by: Sergey Shinderuk
Discussion: https://postgr.es/m/Yd0N9tSAIIkFd+qi@paquier.xyz

1 files changed, 64 insertions, 0 deletions

diff --git a/src/common/hmac.c b/src/common/hmac.c
index d40026d3e99..a27778e86b3 100644
--- a/src/common/hmac.c
+++ b/src/common/hmac.c
@@ -38,11 +38,21 @@
 #define FREE(ptr) free(ptr)
 #endif
 
+/* Set of error states */
+typedef enum pg_hmac_errno
+{
+	PG_HMAC_ERROR_NONE = 0,
+	PG_HMAC_ERROR_OOM,
+	PG_HMAC_ERROR_INTERNAL
+} pg_hmac_errno;
+
 /* Internal pg_hmac_ctx structure */
 struct pg_hmac_ctx
 {
 	pg_cryptohash_ctx *hash;
 	pg_cryptohash_type type;
+	pg_hmac_errno error;
+	const char *errreason;
 	int			block_size;
 	int			digest_size;
 
@@ -73,6 +83,8 @@ pg_hmac_create(pg_cryptohash_type type)
 		return NULL;
 	memset(ctx, 0, sizeof(pg_hmac_ctx));
 	ctx->type = type;
+	ctx->error = PG_HMAC_ERROR_NONE;
+	ctx->errreason = NULL;
 
 	/*
 	 * Initialize the context data.  This requires to know the digest and
@@ -150,12 +162,16 @@ pg_hmac_init(pg_hmac_ctx *ctx, const uint8 *key, size_t len)
 		/* temporary buffer for one-time shrink */
 		shrinkbuf = ALLOC(digest_size);
 		if (shrinkbuf == NULL)
+		{
+			ctx->error = PG_HMAC_ERROR_OOM;
 			return -1;
+		}
 		memset(shrinkbuf, 0, digest_size);
 
 		hash_ctx = pg_cryptohash_create(ctx->type);
 		if (hash_ctx == NULL)
 		{
+			ctx->error = PG_HMAC_ERROR_OOM;
 			FREE(shrinkbuf);
 			return -1;
 		}
@@ -164,6 +180,8 @@ pg_hmac_init(pg_hmac_ctx *ctx, const uint8 *key, size_t len)
 			pg_cryptohash_update(hash_ctx, key, len) < 0 ||
 			pg_cryptohash_final(hash_ctx, shrinkbuf, digest_size) < 0)
 		{
+			ctx->error = PG_HMAC_ERROR_INTERNAL;
+			ctx->errreason = pg_cryptohash_error(hash_ctx);
 			pg_cryptohash_free(hash_ctx);
 			FREE(shrinkbuf);
 			return -1;
@@ -184,6 +202,8 @@ pg_hmac_init(pg_hmac_ctx *ctx, const uint8 *key, size_t len)
 	if (pg_cryptohash_init(ctx->hash) < 0 ||
 		pg_cryptohash_update(ctx->hash, ctx->k_ipad, ctx->block_size) < 0)
 	{
+		ctx->error = PG_HMAC_ERROR_INTERNAL;
+		ctx->errreason = pg_cryptohash_error(ctx->hash);
 		if (shrinkbuf)
 			FREE(shrinkbuf);
 		return -1;
@@ -206,7 +226,11 @@ pg_hmac_update(pg_hmac_ctx *ctx, const uint8 *data, size_t len)
 		return -1;
 
 	if (pg_cryptohash_update(ctx->hash, data, len) < 0)
+	{
+		ctx->error = PG_HMAC_ERROR_INTERNAL;
+		ctx->errreason = pg_cryptohash_error(ctx->hash);
 		return -1;
+	}
 
 	return 0;
 }
@@ -226,11 +250,16 @@ pg_hmac_final(pg_hmac_ctx *ctx, uint8 *dest, size_t len)
 
 	h = ALLOC(ctx->digest_size);
 	if (h == NULL)
+	{
+		ctx->error = PG_HMAC_ERROR_OOM;
 		return -1;
+	}
 	memset(h, 0, ctx->digest_size);
 
 	if (pg_cryptohash_final(ctx->hash, h, ctx->digest_size) < 0)
 	{
+		ctx->error = PG_HMAC_ERROR_INTERNAL;
+		ctx->errreason = pg_cryptohash_error(ctx->hash);
 		FREE(h);
 		return -1;
 	}
@@ -241,6 +270,8 @@ pg_hmac_final(pg_hmac_ctx *ctx, uint8 *dest, size_t len)
 		pg_cryptohash_update(ctx->hash, h, ctx->digest_size) < 0 ||
 		pg_cryptohash_final(ctx->hash, dest, len) < 0)
 	{
+		ctx->error = PG_HMAC_ERROR_INTERNAL;
+		ctx->errreason = pg_cryptohash_error(ctx->hash);
 		FREE(h);
 		return -1;
 	}
@@ -264,3 +295,36 @@ pg_hmac_free(pg_hmac_ctx *ctx)
 	explicit_bzero(ctx, sizeof(pg_hmac_ctx));
 	FREE(ctx);
 }
+
+/*
+ * pg_hmac_error
+ *
+ * Returns a static string providing details about an error that happened
+ * during a HMAC computation.
+ */
+const char *
+pg_hmac_error(pg_hmac_ctx *ctx)
+{
+	if (ctx == NULL)
+		return _("out of memory");
+
+	/*
+	 * If a reason is provided, rely on it, else fallback to any error code
+	 * set.
+	 */
+	if (ctx->errreason)
+		return ctx->errreason;
+
+	switch (ctx->error)
+	{
+		case PG_HMAC_ERROR_NONE:
+			return _("success");
+		case PG_HMAC_ERROR_INTERNAL:
+			return _("internal error");
+		case PG_HMAC_ERROR_OOM:
+			return _("out of memory");
+	}
+
+	Assert(false);				/* cannot be reached */
+	return _("success");
+}