On Wed, Feb 10, 2021 at 09:14:46AM -0300, Ranier Vilela wrote: > It is necessary to correct the interfaces. To caller, inform the size of > the buffer it created.
Well, Coverity likes nannyism, so each one of its reports is to take with a pinch of salt, so there is no point to change something that does not make sense just to please a static analyzer. The context of the code matters. Now, the patch you sent has no need to be that complicated, and it partially works while not actually solving at all the problem you are trying to solve (nothing done for MD5 or OpenSSL). Attached is an example of what I finish with while poking at this issue. There is IMO no point to touch the internals of SCRAM that all rely on the same digest lengths for the proof generation with SHA256. > I think it should be error-out, because the buffer can be malloc. I don't understand what you mean here, but cryptohash[_openssl].c should not issue an error directly, just return a status code that the caller can consume to generate an error. -- Michael
diff --git a/src/include/common/cryptohash.h b/src/include/common/cryptohash.h
index 32d7784ca5..541dc844c8 100644
--- a/src/include/common/cryptohash.h
+++ b/src/include/common/cryptohash.h
@@ -32,7 +32,7 @@ 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);
extern int pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len);
-extern int pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest);
+extern int pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest, size_t len);
extern void pg_cryptohash_free(pg_cryptohash_ctx *ctx);
#endif /* PG_CRYPTOHASH_H */
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 8d857f39df..49386131c1 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -1429,7 +1429,7 @@ scram_mock_salt(const char *username)
if (pg_cryptohash_init(ctx) < 0 ||
pg_cryptohash_update(ctx, (uint8 *) username, strlen(username)) < 0 ||
pg_cryptohash_update(ctx, (uint8 *) mock_auth_nonce, MOCK_AUTH_NONCE_LEN) < 0 ||
- pg_cryptohash_final(ctx, sha_digest) < 0)
+ pg_cryptohash_final(ctx, sha_digest, PG_SHA256_DIGEST_LENGTH) < 0)
{
pg_cryptohash_free(ctx);
return NULL;
diff --git a/src/backend/replication/backup_manifest.c b/src/backend/replication/backup_manifest.c
index 0cefd181b5..18c7bca8d3 100644
--- a/src/backend/replication/backup_manifest.c
+++ b/src/backend/replication/backup_manifest.c
@@ -330,7 +330,8 @@ SendBackupManifest(backup_manifest_info *manifest)
* twice.
*/
manifest->still_checksumming = false;
- if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf) < 0)
+ if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf,
+ PG_SHA256_DIGEST_LENGTH) < 0)
elog(ERROR, "failed to finalize checksum of backup manifest");
AppendStringToManifest(manifest, "\"Manifest-Checksum\": \"");
dstlen = pg_hex_enc_len(PG_SHA256_DIGEST_LENGTH);
diff --git a/src/backend/utils/adt/cryptohashfuncs.c b/src/backend/utils/adt/cryptohashfuncs.c
index 152adcbfb4..6a0f0258e6 100644
--- a/src/backend/utils/adt/cryptohashfuncs.c
+++ b/src/backend/utils/adt/cryptohashfuncs.c
@@ -114,7 +114,8 @@ cryptohash_internal(pg_cryptohash_type type, bytea *input)
elog(ERROR, "could not initialize %s context", typestr);
if (pg_cryptohash_update(ctx, data, len) < 0)
elog(ERROR, "could not update %s context", typestr);
- if (pg_cryptohash_final(ctx, (unsigned char *) VARDATA(result)) < 0)
+ if (pg_cryptohash_final(ctx, (unsigned char *) VARDATA(result),
+ digest_len) < 0)
elog(ERROR, "could not finalize %s context", typestr);
pg_cryptohash_free(ctx);
diff --git a/src/common/checksum_helper.c b/src/common/checksum_helper.c
index a895e2e285..54b684d5e8 100644
--- a/src/common/checksum_helper.c
+++ b/src/common/checksum_helper.c
@@ -198,25 +198,29 @@ 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_sha2, output) < 0)
+ if (pg_cryptohash_final(context->raw_context.c_sha2,
+ output, PG_SHA224_DIGEST_LENGTH) < 0)
return -1;
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_sha2, output) < 0)
+ if (pg_cryptohash_final(context->raw_context.c_sha2,
+ output, PG_SHA256_DIGEST_LENGTH) < 0)
return -1;
pg_cryptohash_free(context->raw_context.c_sha2);
retval = PG_SHA256_DIGEST_LENGTH;
break;
case CHECKSUM_TYPE_SHA384:
- if (pg_cryptohash_final(context->raw_context.c_sha2, output) < 0)
+ if (pg_cryptohash_final(context->raw_context.c_sha2,
+ output, PG_SHA384_DIGEST_LENGTH) < 0)
return -1;
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_sha2, output) < 0)
+ if (pg_cryptohash_final(context->raw_context.c_sha2,
+ output, PG_SHA512_DIGEST_LENGTH) < 0)
return -1;
pg_cryptohash_free(context->raw_context.c_sha2);
retval = PG_SHA512_DIGEST_LENGTH;
diff --git a/src/common/cryptohash.c b/src/common/cryptohash.c
index 5b2c050d79..9ab36003c9 100644
--- a/src/common/cryptohash.c
+++ b/src/common/cryptohash.c
@@ -161,11 +161,11 @@ 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 except if the caller has
- * given a NULL context.
+ * to never fail, but note that this would fail if the destination buffer
+ * is not large enough.
*/
int
-pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
+pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest, size_t len)
{
if (ctx == NULL)
return -1;
@@ -173,21 +173,33 @@ pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
switch (ctx->type)
{
case PG_MD5:
+ if (len < MD5_DIGEST_LENGTH)
+ return -1;
pg_md5_final(&ctx->data.md5, dest);
break;
case PG_SHA1:
+ if (len < SHA1_DIGEST_LENGTH)
+ return -1;
pg_sha1_final(&ctx->data.sha1, dest);
break;
case PG_SHA224:
+ if (len < PG_SHA224_DIGEST_LENGTH)
+ return -1;
pg_sha224_final(&ctx->data.sha224, dest);
break;
case PG_SHA256:
+ if (len < PG_SHA256_DIGEST_LENGTH)
+ return -1;
pg_sha256_final(&ctx->data.sha256, dest);
break;
case PG_SHA384:
+ if (len < PG_SHA384_DIGEST_LENGTH)
+ return -1;
pg_sha384_final(&ctx->data.sha384, dest);
break;
case PG_SHA512:
+ if (len < PG_SHA512_DIGEST_LENGTH)
+ return -1;
pg_sha512_final(&ctx->data.sha512, dest);
break;
}
diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c
index 006e867403..643cc7aea2 100644
--- a/src/common/cryptohash_openssl.c
+++ b/src/common/cryptohash_openssl.c
@@ -24,6 +24,9 @@
#include <openssl/evp.h>
#include "common/cryptohash.h"
+#include "common/md5.h"
+#include "common/sha1.h"
+#include "common/sha2.h"
#ifndef FRONTEND
#include "utils/memutils.h"
#include "utils/resowner.h"
@@ -181,13 +184,41 @@ pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
* Finalize a hash context. Returns 0 on success, and -1 on failure.
*/
int
-pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
+pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest, size_t len)
{
int status = 0;
if (ctx == NULL)
return -1;
+ switch (ctx->type)
+ {
+ case PG_MD5:
+ if (len < MD5_DIGEST_LENGTH)
+ return -1;
+ break;
+ case PG_SHA1:
+ if (len < SHA1_DIGEST_LENGTH)
+ return -1;
+ break;
+ case PG_SHA224:
+ if (len < PG_SHA224_DIGEST_LENGTH)
+ return -1;
+ break;
+ case PG_SHA256:
+ if (len < PG_SHA256_DIGEST_LENGTH)
+ return -1;
+ break;
+ case PG_SHA384:
+ if (len < PG_SHA384_DIGEST_LENGTH)
+ return -1;
+ break;
+ case PG_SHA512:
+ if (len < PG_SHA512_DIGEST_LENGTH)
+ return -1;
+ break;
+ }
+
status = EVP_DigestFinal_ex(ctx->evpctx, dest, 0);
/* OpenSSL internals return 1 on success, 0 on failure */
diff --git a/src/common/md5_common.c b/src/common/md5_common.c
index b01c95ebb6..77e8708ffd 100644
--- a/src/common/md5_common.c
+++ b/src/common/md5_common.c
@@ -78,7 +78,7 @@ pg_md5_hash(const void *buff, size_t len, char *hexsum)
if (pg_cryptohash_init(ctx) < 0 ||
pg_cryptohash_update(ctx, buff, len) < 0 ||
- pg_cryptohash_final(ctx, sum) < 0)
+ pg_cryptohash_final(ctx, sum, MD5_DIGEST_LENGTH) < 0)
{
pg_cryptohash_free(ctx);
return false;
@@ -100,7 +100,7 @@ pg_md5_binary(const void *buff, size_t len, void *outbuf)
if (pg_cryptohash_init(ctx) < 0 ||
pg_cryptohash_update(ctx, buff, len) < 0 ||
- pg_cryptohash_final(ctx, outbuf) < 0)
+ pg_cryptohash_final(ctx, outbuf, MD5_DIGEST_LENGTH) < 0)
{
pg_cryptohash_free(ctx);
return false;
diff --git a/src/common/scram-common.c b/src/common/scram-common.c
index 3f406d4e4d..67fc94c0a8 100644
--- a/src/common/scram-common.c
+++ b/src/common/scram-common.c
@@ -51,7 +51,7 @@ scram_HMAC_init(scram_HMAC_ctx *ctx, const uint8 *key, int keylen)
return -1;
if (pg_cryptohash_init(sha256_ctx) < 0 ||
pg_cryptohash_update(sha256_ctx, key, keylen) < 0 ||
- pg_cryptohash_final(sha256_ctx, keybuf) < 0)
+ pg_cryptohash_final(sha256_ctx, keybuf, SCRAM_KEY_LEN) < 0)
{
pg_cryptohash_free(sha256_ctx);
return -1;
@@ -112,7 +112,7 @@ scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx)
Assert(ctx->sha256ctx != NULL);
- if (pg_cryptohash_final(ctx->sha256ctx, h) < 0)
+ if (pg_cryptohash_final(ctx->sha256ctx, h, SCRAM_KEY_LEN) < 0)
{
pg_cryptohash_free(ctx->sha256ctx);
return -1;
@@ -122,7 +122,7 @@ scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx)
if (pg_cryptohash_init(ctx->sha256ctx) < 0 ||
pg_cryptohash_update(ctx->sha256ctx, ctx->k_opad, SHA256_HMAC_B) < 0 ||
pg_cryptohash_update(ctx->sha256ctx, h, SCRAM_KEY_LEN) < 0 ||
- pg_cryptohash_final(ctx->sha256ctx, result) < 0)
+ pg_cryptohash_final(ctx->sha256ctx, result, SCRAM_KEY_LEN) < 0)
{
pg_cryptohash_free(ctx->sha256ctx);
return -1;
@@ -202,7 +202,7 @@ scram_H(const uint8 *input, int len, uint8 *result)
if (pg_cryptohash_init(ctx) < 0 ||
pg_cryptohash_update(ctx, input, len) < 0 ||
- pg_cryptohash_final(ctx, result) < 0)
+ pg_cryptohash_final(ctx, result, SCRAM_KEY_LEN) < 0)
{
pg_cryptohash_free(ctx);
return -1;
diff --git a/src/bin/pg_verifybackup/parse_manifest.c b/src/bin/pg_verifybackup/parse_manifest.c
index db2fa90cfe..28c1bfbe5c 100644
--- a/src/bin/pg_verifybackup/parse_manifest.c
+++ b/src/bin/pg_verifybackup/parse_manifest.c
@@ -659,7 +659,8 @@ verify_manifest_checksum(JsonManifestParseState *parse, char *buffer,
context->error_cb(context, "could not initialize checksum of manifest");
if (pg_cryptohash_update(manifest_ctx, (uint8 *) buffer, penultimate_newline + 1) < 0)
context->error_cb(context, "could not update checksum of manifest");
- if (pg_cryptohash_final(manifest_ctx, manifest_checksum_actual) < 0)
+ if (pg_cryptohash_final(manifest_ctx, manifest_checksum_actual,
+ PG_SHA256_DIGEST_LENGTH) < 0)
context->error_cb(context, "could not finalize checksum of manifest");
/* Now verify it. */
diff --git a/contrib/pgcrypto/internal-sha2.c b/contrib/pgcrypto/internal-sha2.c
index 0fe53e15af..a0b7bff516 100644
--- a/contrib/pgcrypto/internal-sha2.c
+++ b/contrib/pgcrypto/internal-sha2.c
@@ -118,7 +118,7 @@ int_sha2_finish(PX_MD *h, uint8 *dst)
{
pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
- if (pg_cryptohash_final(ctx, dst) < 0)
+ if (pg_cryptohash_final(ctx, dst, h->block_size(h)) < 0)
elog(ERROR, "could not finalize %s context", "SHA2");
}
diff --git a/contrib/pgcrypto/internal.c b/contrib/pgcrypto/internal.c
index ef6ce2fb1e..b9e2963b85 100644
--- a/contrib/pgcrypto/internal.c
+++ b/contrib/pgcrypto/internal.c
@@ -106,7 +106,7 @@ int_md5_finish(PX_MD *h, uint8 *dst)
{
pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
- if (pg_cryptohash_final(ctx, dst) < 0)
+ if (pg_cryptohash_final(ctx, dst, h->block_size(h)) < 0)
elog(ERROR, "could not finalize %s context", "MD5");
}
@@ -156,7 +156,7 @@ int_sha1_finish(PX_MD *h, uint8 *dst)
{
pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
- if (pg_cryptohash_final(ctx, dst) < 0)
+ if (pg_cryptohash_final(ctx, dst, h->block_size(h)) < 0)
elog(ERROR, "could not finalize %s context", "SHA1");
}
diff --git a/contrib/uuid-ossp/uuid-ossp.c b/contrib/uuid-ossp/uuid-ossp.c
index 49a4a59264..b887c56455 100644
--- a/contrib/uuid-ossp/uuid-ossp.c
+++ b/contrib/uuid-ossp/uuid-ossp.c
@@ -15,6 +15,7 @@
#include "fmgr.h"
#include "common/cryptohash.h"
+#include "common/md5.h"
#include "common/sha1.h"
#include "port/pg_bswap.h"
#include "utils/builtins.h"
@@ -324,7 +325,8 @@ uuid_generate_internal(int v, unsigned char *ns, const char *ptr, int len)
pg_cryptohash_update(ctx, (unsigned char *) ptr, len) < 0)
elog(ERROR, "could not update %s context", "MD5");
/* we assume sizeof MD5 result is 16, same as UUID size */
- if (pg_cryptohash_final(ctx, (unsigned char *) &uu) < 0)
+ if (pg_cryptohash_final(ctx, (unsigned char *) &uu,
+ MD5_DIGEST_LENGTH) < 0)
elog(ERROR, "could not finalize %s context", "MD5");
pg_cryptohash_free(ctx);
}
@@ -338,7 +340,8 @@ uuid_generate_internal(int v, unsigned char *ns, const char *ptr, int len)
if (pg_cryptohash_update(ctx, ns, sizeof(uu)) < 0 ||
pg_cryptohash_update(ctx, (unsigned char *) ptr, len) < 0)
elog(ERROR, "could not update %s context", "SHA1");
- if (pg_cryptohash_final(ctx, sha1result) < 0)
+ if (pg_cryptohash_final(ctx, sha1result,
+ SHA1_DIGEST_LENGTH) < 0)
elog(ERROR, "could not finalize %s context", "SHA1");
pg_cryptohash_free(ctx);
signature.asc
Description: PGP signature
