Incorrect allocation handling for cryptohash functions with OpenSSL

Started by Michael Paquierabout 5 years ago18 messages

Michael Paquier

michael@paquier.xyz

about 5 years ago

2 attachment(s)

Hi all,

As of the work done in 87ae9691, I have played with error injections
in the code paths using this code, but forgot to count for cases where
cascading resowner cleanups are involved. Like other resources (JIT,
DSM, etc.), this requires an allocation in TopMemoryContext to make
sure that nothing gets forgotten or cleaned up on the way until the
resowner that did the cryptohash allocation is handled.

Attached is a small extension I have played with by doing some error
injections, and a patch. If there are no objections, I would like to
commit this fix.

Thanks,
--
Michael

Attachments:

cryptohash-alloc.patchtext/x-diff; charset=us-asciiDownload

diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c
index 118651c415..fa06d70e72 100644
--- a/src/common/cryptohash_openssl.c
+++ b/src/common/cryptohash_openssl.c
@@ -31,11 +31,12 @@
 #endif
 
 /*
- * In backend, use palloc/pfree to ease the error handling.  In frontend,
- * use malloc to be able to return a failure status back to the caller.
+ * In backend, use an allocation in TopMemoryContext to count for resowner
+ * cleanup handling.  In frontend, use malloc to be able to return a failure
+ * status back to the caller.
  */
 #ifndef FRONTEND
-#define ALLOC(size) palloc(size)
+#define ALLOC(size) MemoryContextAlloc(TopMemoryContext, size)
 #define FREE(ptr) pfree(ptr)
 #else
 #define ALLOC(size) malloc(size)

hashfuncs.tar.gzapplication/gzipDownload

Heikki Linnakangas

hlinnaka@iki.fi

about 5 years ago

In reply to: Michael Paquier (#1)

1 attachment(s)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On 18/12/2020 09:35, Michael Paquier wrote:

Hi all,

As of the work done in 87ae9691, I have played with error injections
in the code paths using this code, but forgot to count for cases where
cascading resowner cleanups are involved. Like other resources (JIT,
DSM, etc.), this requires an allocation in TopMemoryContext to make
sure that nothing gets forgotten or cleaned up on the way until the
resowner that did the cryptohash allocation is handled.

Attached is a small extension I have played with by doing some error
injections, and a patch. If there are no objections, I would like to
commit this fix.

pg_cryptohash_create() is now susceptible to leaking memory in
TopMemoryContext, if the allocations fail. I think the attached should
fix it (but I haven't tested it at all).

BTW, looking at pg_cryptohash_ctx and pg_cryptohash_state, why do we
need two structs? They're both allocated and controlled by the
cryptohash implementation. It would seem simpler to have just one.

- Heikki

Attachments:

cryptohash-leaks.patchtext/x-patch; charset=UTF-8; name=cryptohash-leaks.patchDownload

diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c
index 118651c4153..bb303dd6c8d 100644
--- a/src/common/cryptohash_openssl.c
+++ b/src/common/cryptohash_openssl.c
@@ -35,7 +35,7 @@
  * use malloc to be able to return a failure status back to the caller.
  */
 #ifndef FRONTEND
-#define ALLOC(size) palloc(size)
+#define ALLOC(size) MemoryContextAllocExtended(TopMemoryContext, size, MCXT_ALLOC_NO_OOM)
 #define FREE(ptr) pfree(ptr)
 #else
 #define ALLOC(size) malloc(size)
@@ -69,6 +69,10 @@ pg_cryptohash_create(pg_cryptohash_type type)
 	pg_cryptohash_ctx *ctx;
 	pg_cryptohash_state *state;
 
+#ifndef FRONTEND
+	ResourceOwnerEnlargeCryptoHash(CurrentResourceOwner);
+#endif
+
 	ctx = ALLOC(sizeof(pg_cryptohash_ctx));
 	if (ctx == NULL)
 		return NULL;
@@ -84,10 +88,6 @@ pg_cryptohash_create(pg_cryptohash_type type)
 	ctx->data = state;
 	ctx->type = type;
 
-#ifndef FRONTEND
-	ResourceOwnerEnlargeCryptoHash(CurrentResourceOwner);
-#endif
-
 	/*
 	 * Initialization takes care of assigning the correct type for OpenSSL.
 	 */

Heikki Linnakangas

hlinnaka@iki.fi

about 5 years ago

In reply to: Heikki Linnakangas (#2)

1 attachment(s)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On 18/12/2020 11:35, Heikki Linnakangas wrote:

BTW, looking at pg_cryptohash_ctx and pg_cryptohash_state, why do we
need two structs? They're both allocated and controlled by the
cryptohash implementation. It would seem simpler to have just one.

Something like this. Passes regression tests, but otherwise untested.

- Heikki

Attachments:

merge-cryptohash-ctx-and-state-structs.patchtext/x-patch; charset=UTF-8; name=merge-cryptohash-ctx-and-state-structs.patchDownload

diff --git a/src/common/cryptohash.c b/src/common/cryptohash.c
index cf4588bad72..d3fa32888f0 100644
--- a/src/common/cryptohash.c
+++ b/src/common/cryptohash.c
@@ -39,6 +39,20 @@
 #define FREE(ptr) free(ptr)
 #endif
 
+struct pg_cryptohash_ctx
+{
+	pg_cryptohash_type type;
+
+	union
+	{
+		pg_md5_ctx		md5;
+		pg_sha224_ctx	sha224;
+		pg_sha256_ctx	sha256;
+		pg_sha384_ctx	sha384;
+		pg_sha512_ctx	sha512;
+	} data;
+};
+
 /*
  * pg_cryptohash_create
  *
@@ -50,38 +64,15 @@ pg_cryptohash_create(pg_cryptohash_type type)
 {
 	pg_cryptohash_ctx *ctx;
 
+	/*
+	 * FIXME: this always allocates enough space for the largest hash.
+	 * A smaller allocation would be enough for md5, sha224 and sha256.
+	 */
 	ctx = ALLOC(sizeof(pg_cryptohash_ctx));
 	if (ctx == NULL)
 		return NULL;
-
 	ctx->type = type;
 
-	switch (type)
-	{
-		case PG_MD5:
-			ctx->data = ALLOC(sizeof(pg_md5_ctx));
-			break;
-		case PG_SHA224:
-			ctx->data = ALLOC(sizeof(pg_sha224_ctx));
-			break;
-		case PG_SHA256:
-			ctx->data = ALLOC(sizeof(pg_sha256_ctx));
-			break;
-		case PG_SHA384:
-			ctx->data = ALLOC(sizeof(pg_sha384_ctx));
-			break;
-		case PG_SHA512:
-			ctx->data = ALLOC(sizeof(pg_sha512_ctx));
-			break;
-	}
-
-	if (ctx->data == NULL)
-	{
-		explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
-		FREE(ctx);
-		return NULL;
-	}
-
 	return ctx;
 }
 
@@ -100,19 +91,19 @@ pg_cryptohash_init(pg_cryptohash_ctx *ctx)
 	switch (ctx->type)
 	{
 		case PG_MD5:
-			pg_md5_init((pg_md5_ctx *) ctx->data);
+			pg_md5_init(&ctx->data.md5);
 			break;
 		case PG_SHA224:
-			pg_sha224_init((pg_sha224_ctx *) ctx->data);
+			pg_sha224_init(&ctx->data.sha224);
 			break;
 		case PG_SHA256:
-			pg_sha256_init((pg_sha256_ctx *) ctx->data);
+			pg_sha256_init(&ctx->data.sha256);
 			break;
 		case PG_SHA384:
-			pg_sha384_init((pg_sha384_ctx *) ctx->data);
+			pg_sha384_init(&ctx->data.sha384);
 			break;
 		case PG_SHA512:
-			pg_sha512_init((pg_sha512_ctx *) ctx->data);
+			pg_sha512_init(&ctx->data.sha512);
 			break;
 	}
 
@@ -134,19 +125,19 @@ pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
 	switch (ctx->type)
 	{
 		case PG_MD5:
-			pg_md5_update((pg_md5_ctx *) ctx->data, data, len);
+			pg_md5_update(&ctx->data.md5, data, len);
 			break;
 		case PG_SHA224:
-			pg_sha224_update((pg_sha224_ctx *) ctx->data, data, len);
+			pg_sha224_update(&ctx->data.sha224, data, len);
 			break;
 		case PG_SHA256:
-			pg_sha256_update((pg_sha256_ctx *) ctx->data, data, len);
+			pg_sha256_update(&ctx->data.sha256, data, len);
 			break;
 		case PG_SHA384:
-			pg_sha384_update((pg_sha384_ctx *) ctx->data, data, len);
+			pg_sha384_update(&ctx->data.sha384, data, len);
 			break;
 		case PG_SHA512:
-			pg_sha512_update((pg_sha512_ctx *) ctx->data, data, len);
+			pg_sha512_update(&ctx->data.sha512, data, len);
 			break;
 	}
 
@@ -168,19 +159,19 @@ pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
 	switch (ctx->type)
 	{
 		case PG_MD5:
-			pg_md5_final((pg_md5_ctx *) ctx->data, dest);
+			pg_md5_final(&ctx->data.md5, dest);
 			break;
 		case PG_SHA224:
-			pg_sha224_final((pg_sha224_ctx *) ctx->data, dest);
+			pg_sha224_final(&ctx->data.sha224, dest);
 			break;
 		case PG_SHA256:
-			pg_sha256_final((pg_sha256_ctx *) ctx->data, dest);
+			pg_sha256_final(&ctx->data.sha256, dest);
 			break;
 		case PG_SHA384:
-			pg_sha384_final((pg_sha384_ctx *) ctx->data, dest);
+			pg_sha384_final(&ctx->data.sha384, dest);
 			break;
 		case PG_SHA512:
-			pg_sha512_final((pg_sha512_ctx *) ctx->data, dest);
+			pg_sha512_final(&ctx->data.sha512, dest);
 			break;
 	}
 
@@ -198,26 +189,6 @@ pg_cryptohash_free(pg_cryptohash_ctx *ctx)
 	if (ctx == NULL)
 		return;
 
-	switch (ctx->type)
-	{
-		case PG_MD5:
-			explicit_bzero(ctx->data, sizeof(pg_md5_ctx));
-			break;
-		case PG_SHA224:
-			explicit_bzero(ctx->data, sizeof(pg_sha224_ctx));
-			break;
-		case PG_SHA256:
-			explicit_bzero(ctx->data, sizeof(pg_sha256_ctx));
-			break;
-		case PG_SHA384:
-			explicit_bzero(ctx->data, sizeof(pg_sha384_ctx));
-			break;
-		case PG_SHA512:
-			explicit_bzero(ctx->data, sizeof(pg_sha512_ctx));
-			break;
-	}
-
-	FREE(ctx->data);
 	explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
 	FREE(ctx);
 }
diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c
index 118651c4153..24b9636d1ca 100644
--- a/src/common/cryptohash_openssl.c
+++ b/src/common/cryptohash_openssl.c
@@ -35,7 +35,7 @@
  * use malloc to be able to return a failure status back to the caller.
  */
 #ifndef FRONTEND
-#define ALLOC(size) palloc(size)
+#define ALLOC(size) MemoryContextAllocExtended(TopMemoryContext, size, MCXT_ALLOC_NO_OOM)
 #define FREE(ptr) pfree(ptr)
 #else
 #define ALLOC(size) malloc(size)
@@ -43,19 +43,21 @@
 #endif
 
 /*
- * Internal structure for pg_cryptohash_ctx->data.
+ * Internal pg_cryptohash_ctx structure.
  *
  * This tracks the resource owner associated to each EVP context data
  * for the backend.
  */
-typedef struct pg_cryptohash_state
+struct pg_cryptohash_ctx
 {
+	pg_cryptohash_type type;
+
 	EVP_MD_CTX *evpctx;
 
 #ifndef FRONTEND
 	ResourceOwner resowner;
 #endif
-} pg_cryptohash_state;
+};
 
 /*
  * pg_cryptohash_create
@@ -67,49 +69,41 @@ pg_cryptohash_ctx *
 pg_cryptohash_create(pg_cryptohash_type type)
 {
 	pg_cryptohash_ctx *ctx;
-	pg_cryptohash_state *state;
+
+	/*
+	 * Make sure that the resource owner has space to remember this
+	 * reference. This can error out with "out of memory", so do this
+	 * before any other allocation to avoid leaking.
+	 */
+#ifndef FRONTEND
+	ResourceOwnerEnlargeCryptoHash(CurrentResourceOwner);
+#endif
 
 	ctx = ALLOC(sizeof(pg_cryptohash_ctx));
 	if (ctx == NULL)
 		return NULL;
-
-	state = ALLOC(sizeof(pg_cryptohash_state));
-	if (state == NULL)
-	{
-		explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
-		FREE(ctx);
-		return NULL;
-	}
-
-	ctx->data = state;
 	ctx->type = type;
 
-#ifndef FRONTEND
-	ResourceOwnerEnlargeCryptoHash(CurrentResourceOwner);
-#endif
-
 	/*
 	 * Initialization takes care of assigning the correct type for OpenSSL.
 	 */
-	state->evpctx = EVP_MD_CTX_create();
+	ctx->evpctx = EVP_MD_CTX_create();
 
-	if (state->evpctx == NULL)
+	if (ctx->evpctx == NULL)
 	{
-		explicit_bzero(state, sizeof(pg_cryptohash_state));
 		explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
 #ifndef FRONTEND
 		ereport(ERROR,
 				(errcode(ERRCODE_OUT_OF_MEMORY),
 				 errmsg("out of memory")));
 #else
-		FREE(state);
 		FREE(ctx);
 		return NULL;
 #endif
 	}
 
 #ifndef FRONTEND
-	state->resowner = CurrentResourceOwner;
+	ctx->resowner = CurrentResourceOwner;
 	ResourceOwnerRememberCryptoHash(CurrentResourceOwner,
 									PointerGetDatum(ctx));
 #endif
@@ -126,33 +120,30 @@ int
 pg_cryptohash_init(pg_cryptohash_ctx *ctx)
 {
 	int			status = 0;
-	pg_cryptohash_state *state;
 
 	if (ctx == NULL)
 		return 0;
 
-	state = (pg_cryptohash_state *) ctx->data;
-
 	switch (ctx->type)
 	{
 		case PG_MD5:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_md5(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_md5(), NULL);
 			break;
 		case PG_SHA224:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_sha224(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_sha224(), NULL);
 			break;
 		case PG_SHA256:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_sha256(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_sha256(), NULL);
 			break;
 		case PG_SHA384:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_sha384(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_sha384(), NULL);
 			break;
 		case PG_SHA512:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_sha512(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_sha512(), NULL);
 			break;
 	}
 
-	/* OpenSSL internals return 1 on success, 0 on failure */
+	/* openssl internals return 1 on success, 0 on failure */
 	if (status <= 0)
 		return -1;
 	return 0;
@@ -161,21 +152,19 @@ pg_cryptohash_init(pg_cryptohash_ctx *ctx)
 /*
  * pg_cryptohash_update
  *
- * Update a hash context.  Returns 0 on success, and -1 on failure.
+ * update a hash context.  returns 0 on success, and -1 on failure.
  */
 int
 pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
 {
 	int			status = 0;
-	pg_cryptohash_state *state;
 
 	if (ctx == NULL)
 		return 0;
 
-	state = (pg_cryptohash_state *) ctx->data;
-	status = EVP_DigestUpdate(state->evpctx, data, len);
+	status = EVP_DigestUpdate(ctx->evpctx, data, len);
 
-	/* OpenSSL internals return 1 on success, 0 on failure */
+	/* openssl internals return 1 on success, 0 on failure */
 	if (status <= 0)
 		return -1;
 	return 0;
@@ -184,19 +173,17 @@ pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
 /*
  * pg_cryptohash_final
  *
- * Finalize a hash context.  Returns 0 on success, and -1 on failure.
+ * finalize a hash context.  returns 0 on success, and -1 on failure.
  */
 int
 pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
 {
 	int			status = 0;
-	pg_cryptohash_state *state;
 
 	if (ctx == NULL)
 		return 0;
 
-	state = (pg_cryptohash_state *) ctx->data;
-	status = EVP_DigestFinal_ex(state->evpctx, dest, 0);
+	status = EVP_DigestFinal_ex(ctx->evpctx, dest, 0);
 
 	/* OpenSSL internals return 1 on success, 0 on failure */
 	if (status <= 0)
@@ -207,26 +194,21 @@ pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
 /*
  * pg_cryptohash_free
  *
- * Free a hash context.
+ * free a hash context.
  */
 void
 pg_cryptohash_free(pg_cryptohash_ctx *ctx)
 {
-	pg_cryptohash_state *state;
-
 	if (ctx == NULL)
 		return;
 
-	state = (pg_cryptohash_state *) ctx->data;
-	EVP_MD_CTX_destroy(state->evpctx);
+	EVP_MD_CTX_destroy(ctx->evpctx);
 
 #ifndef FRONTEND
-	ResourceOwnerForgetCryptoHash(state->resowner,
+	ResourceOwnerForgetCryptoHash(ctx->resowner,
 								  PointerGetDatum(ctx));
 #endif
 
-	explicit_bzero(state, sizeof(pg_cryptohash_state));
 	explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
-	FREE(state);
 	FREE(ctx);
 }
diff --git a/src/include/common/cryptohash.h b/src/include/common/cryptohash.h
index 6ead1cb8e5b..69e69532d44 100644
--- a/src/include/common/cryptohash.h
+++ b/src/include/common/cryptohash.h
@@ -25,12 +25,8 @@ typedef enum
 	PG_SHA512
 } pg_cryptohash_type;
 
-typedef struct pg_cryptohash_ctx
-{
-	pg_cryptohash_type type;
-	/* private area used by each hash implementation */
-	void	   *data;
-} pg_cryptohash_ctx;
+/* opaque context, private to the cryptohash implementation */
+typedef struct pg_cryptohash_ctx pg_cryptohash_ctx;
 
 extern pg_cryptohash_ctx *pg_cryptohash_create(pg_cryptohash_type type);
 extern int	pg_cryptohash_init(pg_cryptohash_ctx *ctx);

Michael Paquier

michael@paquier.xyz

about 5 years ago

In reply to: Heikki Linnakangas (#2)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On Fri, Dec 18, 2020 at 11:35:14AM +0200, Heikki Linnakangas wrote:

pg_cryptohash_create() is now susceptible to leaking memory in
TopMemoryContext, if the allocations fail. I think the attached should fix
it (but I haven't tested it at all).

Yeah, you are right here. If the second allocation fails the first
one would leak. I don't think that your suggested fix is completely
right though because it ignores that the callers of
pg_cryptohash_create() in the backend expect an error all the time, so
it could crash. Perhaps we should just bite the bullet and switch the
OpenSSL and fallback implementations to use allocation APIs that never
cause an error, and always return NULL? That would have the advantage
to be more consistent with the frontend that relies in malloc(), at
the cost of requiring more changes for the backend code where the
_create() call would need to handle the NULL case properly. The
backend calls are already aware of errors so that would not be
invasive except for the addition of some elog(ERROR) or similar, and
we could change the fallback implementation to use palloc_extended()
with MCXT_ALLOC_NO_OOM.

BTW, looking at pg_cryptohash_ctx and pg_cryptohash_state, why do we need
two structs? They're both allocated and controlled by the cryptohash
implementation. It would seem simpler to have just one.

Depending on the implementation, the data to track can be completely
different, and this split allows to know about the resowner dependency
only in the OpenSSL part of cryptohashes, without having to include
this knowledge in neither cryptohash.h nor in the fallback
implementation that can just use palloc() in the backend.
--
Michael

Michael Paquier

michael@paquier.xyz

about 5 years ago

In reply to: Heikki Linnakangas (#3)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On Fri, Dec 18, 2020 at 11:51:55AM +0200, Heikki Linnakangas wrote:

Something like this. Passes regression tests, but otherwise untested.

... And I wanted to keep track of the type of cryptohash directly in
the shared structure. ;)
--
Michael

Heikki Linnakangas

hlinnaka@iki.fi

about 5 years ago

In reply to: Michael Paquier (#4)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On 18/12/2020 12:10, Michael Paquier wrote:

On Fri, Dec 18, 2020 at 11:35:14AM +0200, Heikki Linnakangas wrote:

pg_cryptohash_create() is now susceptible to leaking memory in
TopMemoryContext, if the allocations fail. I think the attached should fix
it (but I haven't tested it at all).

Yeah, you are right here. If the second allocation fails the first
one would leak. I don't think that your suggested fix is completely
right though because it ignores that the callers of
pg_cryptohash_create() in the backend expect an error all the time, so
it could crash.

Ah, right.

Perhaps we should just bite the bullet and switch the
OpenSSL and fallback implementations to use allocation APIs that never
cause an error, and always return NULL? That would have the advantage
to be more consistent with the frontend that relies in malloc(), at
the cost of requiring more changes for the backend code where the
_create() call would need to handle the NULL case properly. The
backend calls are already aware of errors so that would not be
invasive except for the addition of some elog(ERROR) or similar, and
we could change the fallback implementation to use palloc_extended()
with MCXT_ALLOC_NO_OOM.

-1. On the contrary, I think we should reduce the number of checks
needed in the callers, and prefer throwing errors, if possible. It's too
easy to forget the check, and it makes the code more verbose, too.

In fact, it might be better if pg_cryptohash_init() and
pg_cryptohash_update() didn't return errors either. If an error happens,
they could just set a flag in the pg_cryptohash_ctx, and
pg_cryptohash_final() function would return the error. That way, you
would only need to check for error return in the call to
pg_cryptohash_final().

BTW, looking at pg_cryptohash_ctx and pg_cryptohash_state, why do we need
two structs? They're both allocated and controlled by the cryptohash
implementation. It would seem simpler to have just one.

Depending on the implementation, the data to track can be completely
different, and this split allows to know about the resowner dependency
only in the OpenSSL part of cryptohashes, without having to include
this knowledge in neither cryptohash.h nor in the fallback
implementation that can just use palloc() in the backend.

... And I wanted to keep track of the type of cryptohash directly in
the shared structure. ;)

You could also define a shared header, with the rest of the struct being
implementation-specific:

typedef struct pg_cryptohash_ctx
{
pg_cryptohash_type type;

/* implementation-specific data follows */
} pg_cryptohash_ctx;

- Heikki

Heikki Linnakangas

hlinnaka@iki.fi

about 5 years ago

In reply to: Heikki Linnakangas (#6)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On 18/12/2020 12:55, Heikki Linnakangas wrote:

On 18/12/2020 12:10, Michael Paquier wrote:

On Fri, Dec 18, 2020 at 11:35:14AM +0200, Heikki Linnakangas wrote:

pg_cryptohash_create() is now susceptible to leaking memory in
TopMemoryContext, if the allocations fail. I think the attached should fix
it (but I haven't tested it at all).

Yeah, you are right here. If the second allocation fails the first
one would leak. I don't think that your suggested fix is completely
right though because it ignores that the callers of
pg_cryptohash_create() in the backend expect an error all the time, so
it could crash.

Ah, right.

Perhaps we should just bite the bullet and switch the
OpenSSL and fallback implementations to use allocation APIs that never
cause an error, and always return NULL? That would have the advantage
to be more consistent with the frontend that relies in malloc(), at
the cost of requiring more changes for the backend code where the
_create() call would need to handle the NULL case properly. The
backend calls are already aware of errors so that would not be
invasive except for the addition of some elog(ERROR) or similar, and
we could change the fallback implementation to use palloc_extended()
with MCXT_ALLOC_NO_OOM.

-1. On the contrary, I think we should reduce the number of checks
needed in the callers, and prefer throwing errors, if possible. It's too
easy to forget the check, and it makes the code more verbose, too.

In fact, it might be better if pg_cryptohash_init() and
pg_cryptohash_update() didn't return errors either. If an error happens,
they could just set a flag in the pg_cryptohash_ctx, and
pg_cryptohash_final() function would return the error. That way, you
would only need to check for error return in the call to
pg_cryptohash_final().

BTW, it's a bit weird that the pg_cryptohash_init/update/final()
functions return success, if the ctx argument is NULL. It would seem
more sensible for them to return an error. That way, if a caller forgets
to check for NULL result from pg_cryptohash_create(), but correctly
checks the result of those other functions, it would catch the error. In
fact, if we documented that pg_cryptohash_create() can return NULL, and
pg_cryptohash_final() always returns error on NULL argument, then it
would be sufficient for the callers to only check the return value of
pg_cryptohash_final(). So the usage pattern would be:

ctx = pg_cryptohash_create(PG_MD5);
pg_cryptohash_inti(ctx);
pg_update(ctx, data, size);
pg_update(ctx, moredata, size);
if (pg_cryptohash_final(ctx, &hash) < 0)
elog(ERROR, "md5 calculation failed");
pg_cryptohash_free(ctx);

- Heikki

Michael Paquier

michael@paquier.xyz

almost 5 years ago

In reply to: Heikki Linnakangas (#7)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On Fri, Dec 18, 2020 at 01:04:27PM +0200, Heikki Linnakangas wrote:

BTW, it's a bit weird that the pg_cryptohash_init/update/final() functions
return success, if the ctx argument is NULL. It would seem more sensible for
them to return an error.

Okay.

That way, if a caller forgets to check for NULL
result from pg_cryptohash_create(), but correctly checks the result of those
other functions, it would catch the error. In fact, if we documented that
pg_cryptohash_create() can return NULL, and pg_cryptohash_final() always
returns error on NULL argument, then it would be sufficient for the callers
to only check the return value of pg_cryptohash_final(). So the usage
pattern would be:

ctx = pg_cryptohash_create(PG_MD5);
pg_cryptohash_inti(ctx);
pg_update(ctx, data, size);
pg_update(ctx, moredata, size);
if (pg_cryptohash_final(ctx, &hash) < 0)
elog(ERROR, "md5 calculation failed");
pg_cryptohash_free(ctx);

I'd rather keep the init and update routines to return an error code
directly. This is more consistent with OpenSSL (note that libnss does
not return error codes for the init, update and final but it is
possible to grab for errors then react on that). And we have even in
our tree code paths a-la-pgcrypto that have callbacks for each phase
with some processing in-between. HMAC also gets a bit cleaner by
keeping this flexibility IMO.
--
Michael

Michael Paquier

michael@paquier.xyz

almost 5 years ago

In reply to: Heikki Linnakangas (#3)

1 attachment(s)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On Fri, Dec 18, 2020 at 11:51:55AM +0200, Heikki Linnakangas wrote:

On 18/12/2020 11:35, Heikki Linnakangas wrote:

BTW, looking at pg_cryptohash_ctx and pg_cryptohash_state, why do we
need two structs? They're both allocated and controlled by the
cryptohash implementation. It would seem simpler to have just one.

Something like this. Passes regression tests, but otherwise untested.

Interesting. I have looked at that with a fresh mind, thanks for the
idea. This reduces the number of allocations to one making the error
handling a no-brainer, at the cost of hiding the cryptohash type
directly to the caller. I originally thought that this would be
useful as I recall reading cases in the OpenSSL code doing checks on
hash type used, but perhaps that's just some over-engineered thoughts
from my side. I have found a couple of small-ish issues, please see
my comments below.

+   /*
+    * FIXME: this always allocates enough space for the largest hash.
+    * A smaller allocation would be enough for md5, sha224 and sha256.
+    */
I am not sure that this is worth complicating more, and we are not
talking about a lot of memory (sha512 requires 208 bytes, sha224/256
104 bytes, md5 96 bytes with a quick measurement).  This makes free()
equally more simple.  So just allocating the amount of memory based on
the max size in the union looks fine to me.  I would add a memset(0)
after this allocation though.

-#define ALLOC(size) palloc(size)
+#define ALLOC(size) MemoryContextAllocExtended(TopMemoryContext, size, MCXT_ALLOC_NO_OOM)
As the only allocation in TopMemoryContext is for the context, it
would be fine to not use MCXT_ALLOC_NO_OOM here, and fail so as
callers in the backend don't need to worry about create() returning
NULL.

-       state->evpctx = EVP_MD_CTX_create();
+       ctx->evpctx = EVP_MD_CTX_create();

- if (state->evpctx == NULL)
+ if (ctx->evpctx == NULL)
{
If EVP_MD_CTX_create() fails, you would leak memory with the context
allocated in TopMemoryContext. So this requires a free of the context
before the elog(ERROR).

+   /*
+    * Make sure that the resource owner has space to remember this
+    * reference. This can error out with "out of memory", so do this
+    * before any other allocation to avoid leaking.
+    */
 #ifndef FRONTEND
     ResourceOwnerEnlargeCryptoHash(CurrentResourceOwner);
 #endif
Right.  Good point.

-       /* OpenSSL internals return 1 on success, 0 on failure */
+       /* openssl internals return 1 on success, 0 on failure */
It seems to me that this was not wanted.

At the same time, I have taken care of your comment from upthread to
return a failure if the caller passes NULL for the context, and
adjusted some comments. What do you think of the attached?
--
Michael

Attachments:

merge-cryptohash-ctx-and-state-structs-v2.patchtext/x-diff; charset=us-asciiDownload

diff --git a/src/include/common/cryptohash.h b/src/include/common/cryptohash.h
index 6ead1cb8e5..817e07c9a2 100644
--- a/src/include/common/cryptohash.h
+++ b/src/include/common/cryptohash.h
@@ -25,12 +25,8 @@ typedef enum
 	PG_SHA512
 } pg_cryptohash_type;
 
-typedef struct pg_cryptohash_ctx
-{
-	pg_cryptohash_type type;
-	/* private area used by each hash implementation */
-	void	   *data;
-} pg_cryptohash_ctx;
+/* opaque context, private to each cryptohash implementation */
+typedef struct pg_cryptohash_ctx pg_cryptohash_ctx;
 
 extern pg_cryptohash_ctx *pg_cryptohash_create(pg_cryptohash_type type);
 extern int	pg_cryptohash_init(pg_cryptohash_ctx *ctx);
diff --git a/src/common/cryptohash.c b/src/common/cryptohash.c
index cf4588bad7..f385bd778f 100644
--- a/src/common/cryptohash.c
+++ b/src/common/cryptohash.c
@@ -39,6 +39,21 @@
 #define FREE(ptr) free(ptr)
 #endif
 
+/* Internal pg_cryptohash_ctx structure */
+struct pg_cryptohash_ctx
+{
+	pg_cryptohash_type type;
+
+	union
+	{
+		pg_md5_ctx		md5;
+		pg_sha224_ctx	sha224;
+		pg_sha256_ctx	sha256;
+		pg_sha384_ctx	sha384;
+		pg_sha512_ctx	sha512;
+	} data;
+};
+
 /*
  * pg_cryptohash_create
  *
@@ -50,38 +65,18 @@ pg_cryptohash_create(pg_cryptohash_type type)
 {
 	pg_cryptohash_ctx *ctx;
 
+	/*
+	 * Note that this always allocates enough space for the largest hash.
+	 * A smaller allocation would be enough for md5, sha224 and sha256,
+	 * but the small extra amount of memory does not make it worth
+	 * complicating this code.
+	 */
 	ctx = ALLOC(sizeof(pg_cryptohash_ctx));
 	if (ctx == NULL)
 		return NULL;
-
+	memset(ctx, 0, sizeof(pg_cryptohash_ctx));
 	ctx->type = type;
 
-	switch (type)
-	{
-		case PG_MD5:
-			ctx->data = ALLOC(sizeof(pg_md5_ctx));
-			break;
-		case PG_SHA224:
-			ctx->data = ALLOC(sizeof(pg_sha224_ctx));
-			break;
-		case PG_SHA256:
-			ctx->data = ALLOC(sizeof(pg_sha256_ctx));
-			break;
-		case PG_SHA384:
-			ctx->data = ALLOC(sizeof(pg_sha384_ctx));
-			break;
-		case PG_SHA512:
-			ctx->data = ALLOC(sizeof(pg_sha512_ctx));
-			break;
-	}
-
-	if (ctx->data == NULL)
-	{
-		explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
-		FREE(ctx);
-		return NULL;
-	}
-
 	return ctx;
 }
 
@@ -95,24 +90,24 @@ int
 pg_cryptohash_init(pg_cryptohash_ctx *ctx)
 {
 	if (ctx == NULL)
-		return 0;
+		return -1;
 
 	switch (ctx->type)
 	{
 		case PG_MD5:
-			pg_md5_init((pg_md5_ctx *) ctx->data);
+			pg_md5_init(&ctx->data.md5);
 			break;
 		case PG_SHA224:
-			pg_sha224_init((pg_sha224_ctx *) ctx->data);
+			pg_sha224_init(&ctx->data.sha224);
 			break;
 		case PG_SHA256:
-			pg_sha256_init((pg_sha256_ctx *) ctx->data);
+			pg_sha256_init(&ctx->data.sha256);
 			break;
 		case PG_SHA384:
-			pg_sha384_init((pg_sha384_ctx *) ctx->data);
+			pg_sha384_init(&ctx->data.sha384);
 			break;
 		case PG_SHA512:
-			pg_sha512_init((pg_sha512_ctx *) ctx->data);
+			pg_sha512_init(&ctx->data.sha512);
 			break;
 	}
 
@@ -123,30 +118,31 @@ pg_cryptohash_init(pg_cryptohash_ctx *ctx)
  * pg_cryptohash_update
  *
  * Update a hash context.  Note that this implementation is designed
- * to never fail, so this always returns 0.
+ * to never fail, so this always returns 0 except if the caller has
+ * given a NULL context.
  */
 int
 pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
 {
 	if (ctx == NULL)
-		return 0;
+		return -1;
 
 	switch (ctx->type)
 	{
 		case PG_MD5:
-			pg_md5_update((pg_md5_ctx *) ctx->data, data, len);
+			pg_md5_update(&ctx->data.md5, data, len);
 			break;
 		case PG_SHA224:
-			pg_sha224_update((pg_sha224_ctx *) ctx->data, data, len);
+			pg_sha224_update(&ctx->data.sha224, data, len);
 			break;
 		case PG_SHA256:
-			pg_sha256_update((pg_sha256_ctx *) ctx->data, data, len);
+			pg_sha256_update(&ctx->data.sha256, data, len);
 			break;
 		case PG_SHA384:
-			pg_sha384_update((pg_sha384_ctx *) ctx->data, data, len);
+			pg_sha384_update(&ctx->data.sha384, data, len);
 			break;
 		case PG_SHA512:
-			pg_sha512_update((pg_sha512_ctx *) ctx->data, data, len);
+			pg_sha512_update(&ctx->data.sha512, data, len);
 			break;
 	}
 
@@ -157,30 +153,31 @@ pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
  * pg_cryptohash_final
  *
  * Finalize a hash context.  Note that this implementation is designed
- * to never fail, so this always returns 0.
+ * to never fail, so this always returns 0 except if the caller has
+ * given a NULL context.
  */
 int
 pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
 {
 	if (ctx == NULL)
-		return 0;
+		return -1;
 
 	switch (ctx->type)
 	{
 		case PG_MD5:
-			pg_md5_final((pg_md5_ctx *) ctx->data, dest);
+			pg_md5_final(&ctx->data.md5, dest);
 			break;
 		case PG_SHA224:
-			pg_sha224_final((pg_sha224_ctx *) ctx->data, dest);
+			pg_sha224_final(&ctx->data.sha224, dest);
 			break;
 		case PG_SHA256:
-			pg_sha256_final((pg_sha256_ctx *) ctx->data, dest);
+			pg_sha256_final(&ctx->data.sha256, dest);
 			break;
 		case PG_SHA384:
-			pg_sha384_final((pg_sha384_ctx *) ctx->data, dest);
+			pg_sha384_final(&ctx->data.sha384, dest);
 			break;
 		case PG_SHA512:
-			pg_sha512_final((pg_sha512_ctx *) ctx->data, dest);
+			pg_sha512_final(&ctx->data.sha512, dest);
 			break;
 	}
 
@@ -198,26 +195,6 @@ pg_cryptohash_free(pg_cryptohash_ctx *ctx)
 	if (ctx == NULL)
 		return;
 
-	switch (ctx->type)
-	{
-		case PG_MD5:
-			explicit_bzero(ctx->data, sizeof(pg_md5_ctx));
-			break;
-		case PG_SHA224:
-			explicit_bzero(ctx->data, sizeof(pg_sha224_ctx));
-			break;
-		case PG_SHA256:
-			explicit_bzero(ctx->data, sizeof(pg_sha256_ctx));
-			break;
-		case PG_SHA384:
-			explicit_bzero(ctx->data, sizeof(pg_sha384_ctx));
-			break;
-		case PG_SHA512:
-			explicit_bzero(ctx->data, sizeof(pg_sha512_ctx));
-			break;
-	}
-
-	FREE(ctx->data);
 	explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
 	FREE(ctx);
 }
diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c
index 118651c415..c5077664f5 100644
--- a/src/common/cryptohash_openssl.c
+++ b/src/common/cryptohash_openssl.c
@@ -31,11 +31,12 @@
 #endif
 
 /*
- * In backend, use palloc/pfree to ease the error handling.  In frontend,
- * use malloc to be able to return a failure status back to the caller.
+ * In the backend, use an allocation in TopMemoryContext to count for
+ * resowner cleanup handling.  In the frontend, use malloc to be able
+ * to return a failure status back to the caller.
  */
 #ifndef FRONTEND
-#define ALLOC(size) palloc(size)
+#define ALLOC(size) MemoryContextAlloc(TopMemoryContext, size)
 #define FREE(ptr) pfree(ptr)
 #else
 #define ALLOC(size) malloc(size)
@@ -43,19 +44,21 @@
 #endif
 
 /*
- * Internal structure for pg_cryptohash_ctx->data.
+ * Internal pg_cryptohash_ctx structure.
  *
  * This tracks the resource owner associated to each EVP context data
  * for the backend.
  */
-typedef struct pg_cryptohash_state
+struct pg_cryptohash_ctx
 {
+	pg_cryptohash_type type;
+
 	EVP_MD_CTX *evpctx;
 
 #ifndef FRONTEND
 	ResourceOwner resowner;
 #endif
-} pg_cryptohash_state;
+};
 
 /*
  * pg_cryptohash_create
@@ -67,49 +70,42 @@ pg_cryptohash_ctx *
 pg_cryptohash_create(pg_cryptohash_type type)
 {
 	pg_cryptohash_ctx *ctx;
-	pg_cryptohash_state *state;
-
-	ctx = ALLOC(sizeof(pg_cryptohash_ctx));
-	if (ctx == NULL)
-		return NULL;
-
-	state = ALLOC(sizeof(pg_cryptohash_state));
-	if (state == NULL)
-	{
-		explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
-		FREE(ctx);
-		return NULL;
-	}
-
-	ctx->data = state;
-	ctx->type = type;
 
+	/*
+	 * Make sure that the resource owner has space to remember this
+	 * reference. This can error out with "out of memory", so do this
+	 * before any other allocation to avoid leaking.
+	 */
 #ifndef FRONTEND
 	ResourceOwnerEnlargeCryptoHash(CurrentResourceOwner);
 #endif
 
+	ctx = ALLOC(sizeof(pg_cryptohash_ctx));
+	if (ctx == NULL)
+		return NULL;
+	memset(ctx, 0, sizeof(pg_cryptohash_ctx));
+	ctx->type = type;
+
 	/*
 	 * Initialization takes care of assigning the correct type for OpenSSL.
 	 */
-	state->evpctx = EVP_MD_CTX_create();
+	ctx->evpctx = EVP_MD_CTX_create();
 
-	if (state->evpctx == NULL)
+	if (ctx->evpctx == NULL)
 	{
-		explicit_bzero(state, sizeof(pg_cryptohash_state));
 		explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
+		FREE(ctx);
 #ifndef FRONTEND
 		ereport(ERROR,
 				(errcode(ERRCODE_OUT_OF_MEMORY),
 				 errmsg("out of memory")));
 #else
-		FREE(state);
-		FREE(ctx);
 		return NULL;
 #endif
 	}
 
 #ifndef FRONTEND
-	state->resowner = CurrentResourceOwner;
+	ctx->resowner = CurrentResourceOwner;
 	ResourceOwnerRememberCryptoHash(CurrentResourceOwner,
 									PointerGetDatum(ctx));
 #endif
@@ -126,29 +122,26 @@ int
 pg_cryptohash_init(pg_cryptohash_ctx *ctx)
 {
 	int			status = 0;
-	pg_cryptohash_state *state;
 
 	if (ctx == NULL)
-		return 0;
-
-	state = (pg_cryptohash_state *) ctx->data;
+		return -1;
 
 	switch (ctx->type)
 	{
 		case PG_MD5:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_md5(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_md5(), NULL);
 			break;
 		case PG_SHA224:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_sha224(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_sha224(), NULL);
 			break;
 		case PG_SHA256:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_sha256(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_sha256(), NULL);
 			break;
 		case PG_SHA384:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_sha384(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_sha384(), NULL);
 			break;
 		case PG_SHA512:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_sha512(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_sha512(), NULL);
 			break;
 	}
 
@@ -167,13 +160,11 @@ int
 pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
 {
 	int			status = 0;
-	pg_cryptohash_state *state;
 
 	if (ctx == NULL)
-		return 0;
+		return -1;
 
-	state = (pg_cryptohash_state *) ctx->data;
-	status = EVP_DigestUpdate(state->evpctx, data, len);
+	status = EVP_DigestUpdate(ctx->evpctx, data, len);
 
 	/* OpenSSL internals return 1 on success, 0 on failure */
 	if (status <= 0)
@@ -190,13 +181,11 @@ int
 pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
 {
 	int			status = 0;
-	pg_cryptohash_state *state;
 
 	if (ctx == NULL)
-		return 0;
+		return -1;
 
-	state = (pg_cryptohash_state *) ctx->data;
-	status = EVP_DigestFinal_ex(state->evpctx, dest, 0);
+	status = EVP_DigestFinal_ex(ctx->evpctx, dest, 0);
 
 	/* OpenSSL internals return 1 on success, 0 on failure */
 	if (status <= 0)
@@ -212,21 +201,16 @@ pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
 void
 pg_cryptohash_free(pg_cryptohash_ctx *ctx)
 {
-	pg_cryptohash_state *state;
-
 	if (ctx == NULL)
 		return;
 
-	state = (pg_cryptohash_state *) ctx->data;
-	EVP_MD_CTX_destroy(state->evpctx);
+	EVP_MD_CTX_destroy(ctx->evpctx);
 
 #ifndef FRONTEND
-	ResourceOwnerForgetCryptoHash(state->resowner,
+	ResourceOwnerForgetCryptoHash(ctx->resowner,
 								  PointerGetDatum(ctx));
 #endif
 
-	explicit_bzero(state, sizeof(pg_cryptohash_state));
 	explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
-	FREE(state);
 	FREE(ctx);
 }

#10

Robert Haas

robertmhaas@gmail.com

almost 5 years ago

In reply to: Heikki Linnakangas (#7)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On Fri, Dec 18, 2020 at 6:04 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

BTW, it's a bit weird that the pg_cryptohash_init/update/final()
functions return success, if the ctx argument is NULL. It would seem
more sensible for them to return an error. That way, if a caller forgets
to check for NULL result from pg_cryptohash_create(), but correctly
checks the result of those other functions, it would catch the error. In
fact, if we documented that pg_cryptohash_create() can return NULL, and
pg_cryptohash_final() always returns error on NULL argument, then it
would be sufficient for the callers to only check the return value of
pg_cryptohash_final(). So the usage pattern would be:

ctx = pg_cryptohash_create(PG_MD5);
pg_cryptohash_inti(ctx);
pg_update(ctx, data, size);
pg_update(ctx, moredata, size);
if (pg_cryptohash_final(ctx, &hash) < 0)
elog(ERROR, "md5 calculation failed");
pg_cryptohash_free(ctx);

TBH, I think there's no point in return an error here at all, because
it's totally non-specific. You have no idea what failed, just that
something failed. Blech. If we want to check that ctx is non-NULL, we
should do that with an Assert(). Complicating the code with error
checks that have to be added in multiple places that are far removed
from where the actual problem was detected stinks.

--
Robert Haas
EDB: http://www.enterprisedb.com

#11

Michael Paquier

michael@paquier.xyz

almost 5 years ago

In reply to: Robert Haas (#10)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On Mon, Dec 21, 2020 at 04:28:26PM -0500, Robert Haas wrote:

TBH, I think there's no point in return an error here at all, because
it's totally non-specific. You have no idea what failed, just that
something failed. Blech. If we want to check that ctx is non-NULL, we
should do that with an Assert(). Complicating the code with error
checks that have to be added in multiple places that are far removed
from where the actual problem was detected stinks.

You could technically do that, but only for the backend at the cost of
painting the code of src/common/ with more #ifdef FRONTEND. Even if
we do that, enforcing an error in the backend could be a problem when
it comes to some code paths. One of them is the SCRAM mock
authentication where we had better generate a generic error message.
Using an Assert() or just letting the code go through is not good
either, as we should avoid incorrect computations or crash on OOM, not
to mention that this would fail the detection of bugs coming directly
from OpenSSL or any other SSL library this code plugs with. In short,
I think that there are more benefits in letting the caller control the
error handling.
--
Michael

#12

Michael Paquier

michael@paquier.xyz

almost 5 years ago

In reply to: Michael Paquier (#9)

1 attachment(s)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On Sat, Dec 19, 2020 at 03:13:50PM +0900, Michael Paquier wrote:

At the same time, I have taken care of your comment from upthread to
return a failure if the caller passes NULL for the context, and
adjusted some comments. What do you think of the attached?

I have looked again at this thread with a fresher mind and I did not
see a problem with the previous patch, except some indentation
issues. So if there are no objections, I'd like to commit the
attached.
--
Michael

Attachments:

merge-cryptohash-ctx-and-state-structs-v3.patchtext/x-diff; charset=us-asciiDownload

diff --git a/src/include/common/cryptohash.h b/src/include/common/cryptohash.h
index 4c3df8e5ae..3ecaf62113 100644
--- a/src/include/common/cryptohash.h
+++ b/src/include/common/cryptohash.h
@@ -25,12 +25,8 @@ typedef enum
 	PG_SHA512
 } pg_cryptohash_type;
 
-typedef struct pg_cryptohash_ctx
-{
-	pg_cryptohash_type type;
-	/* private area used by each hash implementation */
-	void	   *data;
-} pg_cryptohash_ctx;
+/* opaque context, private to each cryptohash implementation */
+typedef struct pg_cryptohash_ctx pg_cryptohash_ctx;
 
 extern pg_cryptohash_ctx *pg_cryptohash_create(pg_cryptohash_type type);
 extern int	pg_cryptohash_init(pg_cryptohash_ctx *ctx);
diff --git a/src/common/cryptohash.c b/src/common/cryptohash.c
index 1921a33b34..efedadd626 100644
--- a/src/common/cryptohash.c
+++ b/src/common/cryptohash.c
@@ -39,6 +39,21 @@
 #define FREE(ptr) free(ptr)
 #endif
 
+/* Internal pg_cryptohash_ctx structure */
+struct pg_cryptohash_ctx
+{
+	pg_cryptohash_type type;
+
+	union
+	{
+		pg_md5_ctx	md5;
+		pg_sha224_ctx sha224;
+		pg_sha256_ctx sha256;
+		pg_sha384_ctx sha384;
+		pg_sha512_ctx sha512;
+	}			data;
+};
+
 /*
  * pg_cryptohash_create
  *
@@ -50,38 +65,18 @@ pg_cryptohash_create(pg_cryptohash_type type)
 {
 	pg_cryptohash_ctx *ctx;
 
+	/*
+	 * Note that this always allocates enough space for the largest hash. A
+	 * smaller allocation would be enough for md5, sha224 and sha256, but the
+	 * small extra amount of memory does not make it worth complicating this
+	 * code.
+	 */
 	ctx = ALLOC(sizeof(pg_cryptohash_ctx));
 	if (ctx == NULL)
 		return NULL;
-
+	memset(ctx, 0, sizeof(pg_cryptohash_ctx));
 	ctx->type = type;
 
-	switch (type)
-	{
-		case PG_MD5:
-			ctx->data = ALLOC(sizeof(pg_md5_ctx));
-			break;
-		case PG_SHA224:
-			ctx->data = ALLOC(sizeof(pg_sha224_ctx));
-			break;
-		case PG_SHA256:
-			ctx->data = ALLOC(sizeof(pg_sha256_ctx));
-			break;
-		case PG_SHA384:
-			ctx->data = ALLOC(sizeof(pg_sha384_ctx));
-			break;
-		case PG_SHA512:
-			ctx->data = ALLOC(sizeof(pg_sha512_ctx));
-			break;
-	}
-
-	if (ctx->data == NULL)
-	{
-		explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
-		FREE(ctx);
-		return NULL;
-	}
-
 	return ctx;
 }
 
@@ -95,24 +90,24 @@ int
 pg_cryptohash_init(pg_cryptohash_ctx *ctx)
 {
 	if (ctx == NULL)
-		return 0;
+		return -1;
 
 	switch (ctx->type)
 	{
 		case PG_MD5:
-			pg_md5_init((pg_md5_ctx *) ctx->data);
+			pg_md5_init(&ctx->data.md5);
 			break;
 		case PG_SHA224:
-			pg_sha224_init((pg_sha224_ctx *) ctx->data);
+			pg_sha224_init(&ctx->data.sha224);
 			break;
 		case PG_SHA256:
-			pg_sha256_init((pg_sha256_ctx *) ctx->data);
+			pg_sha256_init(&ctx->data.sha256);
 			break;
 		case PG_SHA384:
-			pg_sha384_init((pg_sha384_ctx *) ctx->data);
+			pg_sha384_init(&ctx->data.sha384);
 			break;
 		case PG_SHA512:
-			pg_sha512_init((pg_sha512_ctx *) ctx->data);
+			pg_sha512_init(&ctx->data.sha512);
 			break;
 	}
 
@@ -123,30 +118,31 @@ pg_cryptohash_init(pg_cryptohash_ctx *ctx)
  * pg_cryptohash_update
  *
  * Update a hash context.  Note that this implementation is designed
- * to never fail, so this always returns 0.
+ * to never fail, so this always returns 0 except if the caller has
+ * given a NULL context.
  */
 int
 pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
 {
 	if (ctx == NULL)
-		return 0;
+		return -1;
 
 	switch (ctx->type)
 	{
 		case PG_MD5:
-			pg_md5_update((pg_md5_ctx *) ctx->data, data, len);
+			pg_md5_update(&ctx->data.md5, data, len);
 			break;
 		case PG_SHA224:
-			pg_sha224_update((pg_sha224_ctx *) ctx->data, data, len);
+			pg_sha224_update(&ctx->data.sha224, data, len);
 			break;
 		case PG_SHA256:
-			pg_sha256_update((pg_sha256_ctx *) ctx->data, data, len);
+			pg_sha256_update(&ctx->data.sha256, data, len);
 			break;
 		case PG_SHA384:
-			pg_sha384_update((pg_sha384_ctx *) ctx->data, data, len);
+			pg_sha384_update(&ctx->data.sha384, data, len);
 			break;
 		case PG_SHA512:
-			pg_sha512_update((pg_sha512_ctx *) ctx->data, data, len);
+			pg_sha512_update(&ctx->data.sha512, data, len);
 			break;
 	}
 
@@ -157,30 +153,31 @@ pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
  * pg_cryptohash_final
  *
  * Finalize a hash context.  Note that this implementation is designed
- * to never fail, so this always returns 0.
+ * to never fail, so this always returns 0 except if the caller has
+ * given a NULL context.
  */
 int
 pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
 {
 	if (ctx == NULL)
-		return 0;
+		return -1;
 
 	switch (ctx->type)
 	{
 		case PG_MD5:
-			pg_md5_final((pg_md5_ctx *) ctx->data, dest);
+			pg_md5_final(&ctx->data.md5, dest);
 			break;
 		case PG_SHA224:
-			pg_sha224_final((pg_sha224_ctx *) ctx->data, dest);
+			pg_sha224_final(&ctx->data.sha224, dest);
 			break;
 		case PG_SHA256:
-			pg_sha256_final((pg_sha256_ctx *) ctx->data, dest);
+			pg_sha256_final(&ctx->data.sha256, dest);
 			break;
 		case PG_SHA384:
-			pg_sha384_final((pg_sha384_ctx *) ctx->data, dest);
+			pg_sha384_final(&ctx->data.sha384, dest);
 			break;
 		case PG_SHA512:
-			pg_sha512_final((pg_sha512_ctx *) ctx->data, dest);
+			pg_sha512_final(&ctx->data.sha512, dest);
 			break;
 	}
 
@@ -198,26 +195,6 @@ pg_cryptohash_free(pg_cryptohash_ctx *ctx)
 	if (ctx == NULL)
 		return;
 
-	switch (ctx->type)
-	{
-		case PG_MD5:
-			explicit_bzero(ctx->data, sizeof(pg_md5_ctx));
-			break;
-		case PG_SHA224:
-			explicit_bzero(ctx->data, sizeof(pg_sha224_ctx));
-			break;
-		case PG_SHA256:
-			explicit_bzero(ctx->data, sizeof(pg_sha256_ctx));
-			break;
-		case PG_SHA384:
-			explicit_bzero(ctx->data, sizeof(pg_sha384_ctx));
-			break;
-		case PG_SHA512:
-			explicit_bzero(ctx->data, sizeof(pg_sha512_ctx));
-			break;
-	}
-
-	FREE(ctx->data);
 	explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
 	FREE(ctx);
 }
diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c
index 0fd6622576..551ec392b6 100644
--- a/src/common/cryptohash_openssl.c
+++ b/src/common/cryptohash_openssl.c
@@ -31,11 +31,12 @@
 #endif
 
 /*
- * In backend, use palloc/pfree to ease the error handling.  In frontend,
- * use malloc to be able to return a failure status back to the caller.
+ * In the backend, use an allocation in TopMemoryContext to count for
+ * resowner cleanup handling.  In the frontend, use malloc to be able
+ * to return a failure status back to the caller.
  */
 #ifndef FRONTEND
-#define ALLOC(size) palloc(size)
+#define ALLOC(size) MemoryContextAlloc(TopMemoryContext, size)
 #define FREE(ptr) pfree(ptr)
 #else
 #define ALLOC(size) malloc(size)
@@ -43,19 +44,21 @@
 #endif
 
 /*
- * Internal structure for pg_cryptohash_ctx->data.
+ * Internal pg_cryptohash_ctx structure.
  *
  * This tracks the resource owner associated to each EVP context data
  * for the backend.
  */
-typedef struct pg_cryptohash_state
+struct pg_cryptohash_ctx
 {
+	pg_cryptohash_type type;
+
 	EVP_MD_CTX *evpctx;
 
 #ifndef FRONTEND
 	ResourceOwner resowner;
 #endif
-} pg_cryptohash_state;
+};
 
 /*
  * pg_cryptohash_create
@@ -67,49 +70,42 @@ pg_cryptohash_ctx *
 pg_cryptohash_create(pg_cryptohash_type type)
 {
 	pg_cryptohash_ctx *ctx;
-	pg_cryptohash_state *state;
-
-	ctx = ALLOC(sizeof(pg_cryptohash_ctx));
-	if (ctx == NULL)
-		return NULL;
-
-	state = ALLOC(sizeof(pg_cryptohash_state));
-	if (state == NULL)
-	{
-		explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
-		FREE(ctx);
-		return NULL;
-	}
-
-	ctx->data = state;
-	ctx->type = type;
 
+	/*
+	 * Make sure that the resource owner has space to remember this reference.
+	 * This can error out with "out of memory", so do this before any other
+	 * allocation to avoid leaking.
+	 */
 #ifndef FRONTEND
 	ResourceOwnerEnlargeCryptoHash(CurrentResourceOwner);
 #endif
 
+	ctx = ALLOC(sizeof(pg_cryptohash_ctx));
+	if (ctx == NULL)
+		return NULL;
+	memset(ctx, 0, sizeof(pg_cryptohash_ctx));
+	ctx->type = type;
+
 	/*
 	 * Initialization takes care of assigning the correct type for OpenSSL.
 	 */
-	state->evpctx = EVP_MD_CTX_create();
+	ctx->evpctx = EVP_MD_CTX_create();
 
-	if (state->evpctx == NULL)
+	if (ctx->evpctx == NULL)
 	{
-		explicit_bzero(state, sizeof(pg_cryptohash_state));
 		explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
+		FREE(ctx);
 #ifndef FRONTEND
 		ereport(ERROR,
 				(errcode(ERRCODE_OUT_OF_MEMORY),
 				 errmsg("out of memory")));
 #else
-		FREE(state);
-		FREE(ctx);
 		return NULL;
 #endif
 	}
 
 #ifndef FRONTEND
-	state->resowner = CurrentResourceOwner;
+	ctx->resowner = CurrentResourceOwner;
 	ResourceOwnerRememberCryptoHash(CurrentResourceOwner,
 									PointerGetDatum(ctx));
 #endif
@@ -126,29 +122,26 @@ int
 pg_cryptohash_init(pg_cryptohash_ctx *ctx)
 {
 	int			status = 0;
-	pg_cryptohash_state *state;
 
 	if (ctx == NULL)
-		return 0;
-
-	state = (pg_cryptohash_state *) ctx->data;
+		return -1;
 
 	switch (ctx->type)
 	{
 		case PG_MD5:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_md5(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_md5(), NULL);
 			break;
 		case PG_SHA224:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_sha224(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_sha224(), NULL);
 			break;
 		case PG_SHA256:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_sha256(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_sha256(), NULL);
 			break;
 		case PG_SHA384:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_sha384(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_sha384(), NULL);
 			break;
 		case PG_SHA512:
-			status = EVP_DigestInit_ex(state->evpctx, EVP_sha512(), NULL);
+			status = EVP_DigestInit_ex(ctx->evpctx, EVP_sha512(), NULL);
 			break;
 	}
 
@@ -167,13 +160,11 @@ int
 pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
 {
 	int			status = 0;
-	pg_cryptohash_state *state;
 
 	if (ctx == NULL)
-		return 0;
+		return -1;
 
-	state = (pg_cryptohash_state *) ctx->data;
-	status = EVP_DigestUpdate(state->evpctx, data, len);
+	status = EVP_DigestUpdate(ctx->evpctx, data, len);
 
 	/* OpenSSL internals return 1 on success, 0 on failure */
 	if (status <= 0)
@@ -190,13 +181,11 @@ int
 pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
 {
 	int			status = 0;
-	pg_cryptohash_state *state;
 
 	if (ctx == NULL)
-		return 0;
+		return -1;
 
-	state = (pg_cryptohash_state *) ctx->data;
-	status = EVP_DigestFinal_ex(state->evpctx, dest, 0);
+	status = EVP_DigestFinal_ex(ctx->evpctx, dest, 0);
 
 	/* OpenSSL internals return 1 on success, 0 on failure */
 	if (status <= 0)
@@ -212,21 +201,16 @@ pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
 void
 pg_cryptohash_free(pg_cryptohash_ctx *ctx)
 {
-	pg_cryptohash_state *state;
-
 	if (ctx == NULL)
 		return;
 
-	state = (pg_cryptohash_state *) ctx->data;
-	EVP_MD_CTX_destroy(state->evpctx);
+	EVP_MD_CTX_destroy(ctx->evpctx);
 
 #ifndef FRONTEND
-	ResourceOwnerForgetCryptoHash(state->resowner,
+	ResourceOwnerForgetCryptoHash(ctx->resowner,
 								  PointerGetDatum(ctx));
 #endif
 
-	explicit_bzero(state, sizeof(pg_cryptohash_state));
 	explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
-	FREE(state);
 	FREE(ctx);
 }
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 9cd047ba25..f3957bad6c 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3196,7 +3196,6 @@ pg_conv_map
 pg_crc32
 pg_crc32c
 pg_cryptohash_ctx
-pg_cryptohash_state
 pg_cryptohash_type
 pg_ctype_cache
 pg_enc

#13

Heikki Linnakangas

hlinnaka@iki.fi

almost 5 years ago

In reply to: Michael Paquier (#12)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On 06/01/2021 13:42, Michael Paquier wrote:

On Sat, Dec 19, 2020 at 03:13:50PM +0900, Michael Paquier wrote:

At the same time, I have taken care of your comment from upthread to
return a failure if the caller passes NULL for the context, and
adjusted some comments. What do you think of the attached?

I have looked again at this thread with a fresher mind and I did not
see a problem with the previous patch, except some indentation
issues. So if there are no objections, I'd like to commit the
attached.

Looks fine to me.

- Heikki

#14

Heikki Linnakangas

hlinnaka@iki.fi

almost 5 years ago

In reply to: Michael Paquier (#11)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On 25/12/2020 02:57, Michael Paquier wrote:

On Mon, Dec 21, 2020 at 04:28:26PM -0500, Robert Haas wrote:

TBH, I think there's no point in return an error here at all, because
it's totally non-specific. You have no idea what failed, just that
something failed. Blech. If we want to check that ctx is non-NULL, we
should do that with an Assert(). Complicating the code with error
checks that have to be added in multiple places that are far removed
from where the actual problem was detected stinks.

You could technically do that, but only for the backend at the cost of
painting the code of src/common/ with more #ifdef FRONTEND. Even if
we do that, enforcing an error in the backend could be a problem when
it comes to some code paths.

Yeah, you would still need to remember to check for the error in
frontend code. Maybe it would still be a good idea, not sure. It would
be a nice backstop, if you forget to check for the error.

I had a quick look at the callers:

contrib/pgcrypto/internal-sha2.c and
src/backend/utils/adt/cryptohashfuncs.c: the call to
pg_cryptohash_create() is missing check for NULL result. With your
latest patch, that's OK because the subsequent pg_cryptohash_update()
calls will fail if passed a NULL context. But seems sloppy.

contrib/pgcrypto/internal.c: all the calls to pg_cryptohash_* functions
are missing checks for error return codes.

contrib/uuid-ossp/uuid-ossp.c: uses pg_cryptohash for MD5, but borrows
the built-in implementation of SHA1 on some platforms. Should we add
support for SHA1 in pg_cryptohash and use that for consistency?

src/backend/libpq/auth-scram.c: SHA256 is used in the mock
authentication. If the pg_cryptohash functions fail, we throw a distinct
error "could not encode salt" that reveals that it was a mock
authentication. I don't think this is a big deal in practice, it would
be hard for an attacker to induce the SHA256 computation to fail, and
there are probably better ways to distinguish mock authentication from
real, like timing attacks. But still.

src/include/common/checksum_helper.h: in pg_checksum_raw_context, do we
still need separate fields for the different sha variants.

- Heikki

#15

Michael Paquier

michael@paquier.xyz

almost 5 years ago

In reply to: Heikki Linnakangas (#13)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On Wed, Jan 06, 2021 at 03:27:03PM +0200, Heikki Linnakangas wrote:

Looks fine to me.

Thanks, I have been able to get this part done as of 55fe26a.
--
Michael

#16

Michael Paquier

michael@paquier.xyz

almost 5 years ago

In reply to: Heikki Linnakangas (#14)

1 attachment(s)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On Wed, Jan 06, 2021 at 03:58:22PM +0200, Heikki Linnakangas wrote:

contrib/pgcrypto/internal-sha2.c and
src/backend/utils/adt/cryptohashfuncs.c: the call to pg_cryptohash_create()
is missing check for NULL result. With your latest patch, that's OK because
the subsequent pg_cryptohash_update() calls will fail if passed a NULL
context. But seems sloppy.

Is it? pg_cryptohash_create() will never return NULL for the backend.

contrib/pgcrypto/internal.c: all the calls to pg_cryptohash_* functions are
missing checks for error return codes.

Indeed, these are incorrect, thanks. I'll go fix that separately.

contrib/uuid-ossp/uuid-ossp.c: uses pg_cryptohash for MD5, but borrows the
built-in implementation of SHA1 on some platforms. Should we add support for
SHA1 in pg_cryptohash and use that for consistency?

Yeah, I have sent a separate patch for that:
https://commitfest.postgresql.org/31/2868/
The cleanups produced by this patch are kind of nice.

src/backend/libpq/auth-scram.c: SHA256 is used in the mock authentication.
If the pg_cryptohash functions fail, we throw a distinct error "could not
encode salt" that reveals that it was a mock authentication. I don't think
this is a big deal in practice, it would be hard for an attacker to induce
the SHA256 computation to fail, and there are probably better ways to
distinguish mock authentication from real, like timing attacks. But still.

This maps with the second error in the mock routine, so wouldn't it be
better to change both and back-patch? The last place where this error
message is used is pg_be_scram_build_secret() for the generation of
what's stored in pg_authid. An idea would be to use "out of memory".
That can be faced for any palloc() calls.

src/include/common/checksum_helper.h: in pg_checksum_raw_context, do we
still need separate fields for the different sha variants.

Using separate fields looked cleaner to me if it came to debugging,
and the cleanup was rather minimal except if we use more switch/case
to set up the various variables. What about something like the
attached?
--
Michael

Attachments:

cryptohash-cleanups.patchtext/x-diff; charset=us-asciiDownload

diff --git a/src/include/common/checksum_helper.h b/src/include/common/checksum_helper.h
index ebdf1ccf44..cac7570ea1 100644
--- a/src/include/common/checksum_helper.h
+++ b/src/include/common/checksum_helper.h
@@ -42,10 +42,7 @@ typedef enum pg_checksum_type
 typedef union pg_checksum_raw_context
 {
 	pg_crc32c	c_crc32c;
-	pg_cryptohash_ctx *c_sha224;
-	pg_cryptohash_ctx *c_sha256;
-	pg_cryptohash_ctx *c_sha384;
-	pg_cryptohash_ctx *c_sha512;
+	pg_cryptohash_ctx *c_sha2;
 } pg_checksum_raw_context;
 
 /*
diff --git a/src/common/checksum_helper.c b/src/common/checksum_helper.c
index f6b49de405..2881b2c178 100644
--- a/src/common/checksum_helper.c
+++ b/src/common/checksum_helper.c
@@ -93,42 +93,42 @@ pg_checksum_init(pg_checksum_context *context, pg_checksum_type type)
 			INIT_CRC32C(context->raw_context.c_crc32c);
 			break;
 		case CHECKSUM_TYPE_SHA224:
-			context->raw_context.c_sha224 = pg_cryptohash_create(PG_SHA224);
-			if (context->raw_context.c_sha224 == NULL)
+			context->raw_context.c_sha2 = pg_cryptohash_create(PG_SHA224);
+			if (context->raw_context.c_sha2 == NULL)
 				return -1;
-			if (pg_cryptohash_init(context->raw_context.c_sha224) < 0)
+			if (pg_cryptohash_init(context->raw_context.c_sha2) < 0)
 			{
-				pg_cryptohash_free(context->raw_context.c_sha224);
+				pg_cryptohash_free(context->raw_context.c_sha2);
 				return -1;
 			}
 			break;
 		case CHECKSUM_TYPE_SHA256:
-			context->raw_context.c_sha256 = pg_cryptohash_create(PG_SHA256);
-			if (context->raw_context.c_sha256 == NULL)
+			context->raw_context.c_sha2 = pg_cryptohash_create(PG_SHA256);
+			if (context->raw_context.c_sha2 == NULL)
 				return -1;
-			if (pg_cryptohash_init(context->raw_context.c_sha256) < 0)
+			if (pg_cryptohash_init(context->raw_context.c_sha2) < 0)
 			{
-				pg_cryptohash_free(context->raw_context.c_sha256);
+				pg_cryptohash_free(context->raw_context.c_sha2);
 				return -1;
 			}
 			break;
 		case CHECKSUM_TYPE_SHA384:
-			context->raw_context.c_sha384 = pg_cryptohash_create(PG_SHA384);
-			if (context->raw_context.c_sha384 == NULL)
+			context->raw_context.c_sha2 = pg_cryptohash_create(PG_SHA384);
+			if (context->raw_context.c_sha2 == NULL)
 				return -1;
-			if (pg_cryptohash_init(context->raw_context.c_sha384) < 0)
+			if (pg_cryptohash_init(context->raw_context.c_sha2) < 0)
 			{
-				pg_cryptohash_free(context->raw_context.c_sha384);
+				pg_cryptohash_free(context->raw_context.c_sha2);
 				return -1;
 			}
 			break;
 		case CHECKSUM_TYPE_SHA512:
-			context->raw_context.c_sha512 = pg_cryptohash_create(PG_SHA512);
-			if (context->raw_context.c_sha512 == NULL)
+			context->raw_context.c_sha2 = pg_cryptohash_create(PG_SHA512);
+			if (context->raw_context.c_sha2 == NULL)
 				return -1;
-			if (pg_cryptohash_init(context->raw_context.c_sha512) < 0)
+			if (pg_cryptohash_init(context->raw_context.c_sha2) < 0)
 			{
-				pg_cryptohash_free(context->raw_context.c_sha512);
+				pg_cryptohash_free(context->raw_context.c_sha2);
 				return -1;
 			}
 			break;
@@ -154,19 +154,10 @@ pg_checksum_update(pg_checksum_context *context, const uint8 *input,
 			COMP_CRC32C(context->raw_context.c_crc32c, input, len);
 			break;
 		case CHECKSUM_TYPE_SHA224:
-			if (pg_cryptohash_update(context->raw_context.c_sha224, input, len) < 0)
-				return -1;
-			break;
 		case CHECKSUM_TYPE_SHA256:
-			if (pg_cryptohash_update(context->raw_context.c_sha256, input, len) < 0)
-				return -1;
-			break;
 		case CHECKSUM_TYPE_SHA384:
-			if (pg_cryptohash_update(context->raw_context.c_sha384, input, len) < 0)
-				return -1;
-			break;
 		case CHECKSUM_TYPE_SHA512:
-			if (pg_cryptohash_update(context->raw_context.c_sha512, input, len) < 0)
+			if (pg_cryptohash_update(context->raw_context.c_sha2, input, len) < 0)
 				return -1;
 			break;
 	}
@@ -207,27 +198,27 @@ pg_checksum_final(pg_checksum_context *context, uint8 *output)
 			memcpy(output, &context->raw_context.c_crc32c, retval);
 			break;
 		case CHECKSUM_TYPE_SHA224:
-			if (pg_cryptohash_final(context->raw_context.c_sha224, output) < 0)
+			if (pg_cryptohash_final(context->raw_context.c_sha2, output) < 0)
 				return -1;
-			pg_cryptohash_free(context->raw_context.c_sha224);
+			pg_cryptohash_free(context->raw_context.c_sha2);
 			retval = PG_SHA224_DIGEST_LENGTH;
 			break;
 		case CHECKSUM_TYPE_SHA256:
-			if (pg_cryptohash_final(context->raw_context.c_sha256, output) < 0)
+			if (pg_cryptohash_final(context->raw_context.c_sha2, output) < 0)
 				return -1;
-			pg_cryptohash_free(context->raw_context.c_sha256);
+			pg_cryptohash_free(context->raw_context.c_sha2);
 			retval = PG_SHA224_DIGEST_LENGTH;
 			break;
 		case CHECKSUM_TYPE_SHA384:
-			if (pg_cryptohash_final(context->raw_context.c_sha384, output) < 0)
+			if (pg_cryptohash_final(context->raw_context.c_sha2, output) < 0)
 				return -1;
-			pg_cryptohash_free(context->raw_context.c_sha384);
+			pg_cryptohash_free(context->raw_context.c_sha2);
 			retval = PG_SHA384_DIGEST_LENGTH;
 			break;
 		case CHECKSUM_TYPE_SHA512:
-			if (pg_cryptohash_final(context->raw_context.c_sha512, output) < 0)
+			if (pg_cryptohash_final(context->raw_context.c_sha2, output) < 0)
 				return -1;
-			pg_cryptohash_free(context->raw_context.c_sha512);
+			pg_cryptohash_free(context->raw_context.c_sha2);
 			retval = PG_SHA512_DIGEST_LENGTH;
 			break;
 	}
diff --git a/contrib/pgcrypto/internal.c b/contrib/pgcrypto/internal.c
index ea377bdf83..79ce513599 100644
--- a/contrib/pgcrypto/internal.c
+++ b/contrib/pgcrypto/internal.c
@@ -96,7 +96,8 @@ int_md5_update(PX_MD *h, const uint8 *data, unsigned dlen)
 {
 	pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
 
-	pg_cryptohash_update(ctx, data, dlen);
+	if (pg_cryptohash_update(ctx, data, dlen) < 0)
+		elog(ERROR, "could not update %s context", "MD5");
 }
 
 static void
@@ -104,7 +105,8 @@ int_md5_reset(PX_MD *h)
 {
 	pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
 
-	pg_cryptohash_init(ctx);
+	if (pg_cryptohash_init(ctx) < 0)
+		elog(ERROR, "could not initialize %s context", "MD5");
 }
 
 static void
@@ -112,7 +114,8 @@ int_md5_finish(PX_MD *h, uint8 *dst)
 {
 	pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
 
-	pg_cryptohash_final(ctx, dst);
+	if (pg_cryptohash_final(ctx, dst) < 0)
+		elog(ERROR, "could not finalize %s context", "MD5");
 }
 
 static void

#17

Heikki Linnakangas

hlinnaka@iki.fi

almost 5 years ago

In reply to: Michael Paquier (#16)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On 07/01/2021 06:15, Michael Paquier wrote:

On Wed, Jan 06, 2021 at 03:58:22PM +0200, Heikki Linnakangas wrote:

contrib/pgcrypto/internal-sha2.c and
src/backend/utils/adt/cryptohashfuncs.c: the call to pg_cryptohash_create()
is missing check for NULL result. With your latest patch, that's OK because
the subsequent pg_cryptohash_update() calls will fail if passed a NULL
context. But seems sloppy.

Is it? pg_cryptohash_create() will never return NULL for the backend.

Ah, you're right.

src/backend/libpq/auth-scram.c: SHA256 is used in the mock authentication.
If the pg_cryptohash functions fail, we throw a distinct error "could not
encode salt" that reveals that it was a mock authentication. I don't think
this is a big deal in practice, it would be hard for an attacker to induce
the SHA256 computation to fail, and there are probably better ways to
distinguish mock authentication from real, like timing attacks. But still.

This maps with the second error in the mock routine, so wouldn't it be
better to change both and back-patch? The last place where this error
message is used is pg_be_scram_build_secret() for the generation of
what's stored in pg_authid. An idea would be to use "out of memory".
That can be faced for any palloc() calls.

Hmm. Perhaps it would be best to change all the errors in mock
authentication to COMMERROR. It'd be nice to have an accurate error
message in the log, but no need to send it to the client.

src/include/common/checksum_helper.h: in pg_checksum_raw_context, do we
still need separate fields for the different sha variants.

Using separate fields looked cleaner to me if it came to debugging,
and the cleanup was rather minimal except if we use more switch/case
to set up the various variables. What about something like the
attached?

- Heikki

#18

Michael Paquier

michael@paquier.xyz

almost 5 years ago

In reply to: Heikki Linnakangas (#17)

Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On Thu, Jan 07, 2021 at 09:51:00AM +0200, Heikki Linnakangas wrote:

Hmm. Perhaps it would be best to change all the errors in mock
authentication to COMMERROR. It'd be nice to have an accurate error message
in the log, but no need to send it to the client.

Yeah, we could do that. Still, this mode still requires a hard
failure because COMMERROR is just a log, and if only COMMERROR is done
we still expect a salt to be generated to send a challenge back to the
client, which would require a fallback for the salt if the one
generated from the mock nonce cannot. Need to think more about that.

Using separate fields looked cleaner to me if it came to debugging,
and the cleanup was rather minimal except if we use more switch/case
to set up the various variables. What about something like the
attached?

+1

Thanks, I have committed this part.
--
Michael

Participants

Thread Outline

Incorrect allocation handling for cryptohash functions with OpenSSL

Attachments:

Attachments:

Attachments:

Attachments:

Attachments:

Attachments: