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
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
signature.asc
Description: PGP signature
