pg_cryptohash_final possible out-of-bounds access (per Coverity)
Hi Hackers,
Per Coverity.
Coverity complaints about pg_cryptohash_final function.
And I agree with Coverity, it's a bad design.
Its allows this:
#define MY_RESULT_LENGTH 32
function pgtest(char * buffer, char * text) {
pg_cryptohash_ctx *ctx;
uint8 digest[MY_RESULT_LENGTH];
ctx = pg_cryptohash_create(PG_SHA512);
pg_cryptohash_init(ctx);
pg_cryptohash_update(ctx, (uint8 *) buffer, text);
pg_cryptohash_final(ctx, digest); // <-- CID 1446240 (#1 of 1):
Out-of-bounds access (OVERRUN)
pg_cryptohash_free(ctx);
return
}
Attached has a patch with suggestions to make things better.
regards,
Ranier Vilela
Attachments:
pg_cryptohash.patchapplication/octet-stream; name=pg_cryptohash.patchDownload
diff --git a/contrib/pgcrypto/internal-sha2.c b/contrib/pgcrypto/internal-sha2.c
index 0fe53e15af..012203eea4 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, PG_SHA256_DIGEST_LENGTH) < 0)
elog(ERROR, "could not finalize %s context", "SHA2");
}
diff --git a/contrib/pgcrypto/internal.c b/contrib/pgcrypto/internal.c
index ef6ce2fb1e..955501b4d4 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, MD5_DIGEST_LENGTH) < 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, SHA1_DIGEST_LENGTH) < 0)
elog(ERROR, "could not finalize %s context", "SHA1");
}
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 8d857f39df..736b1191e2 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -1120,7 +1120,7 @@ verify_client_proof(scram_state *state)
scram_HMAC_update(&ctx,
state->client_final_message_without_proof,
strlen(state->client_final_message_without_proof)) < 0 ||
- scram_HMAC_final(ClientSignature, &ctx) < 0)
+ scram_HMAC_final(&ctx, ClientSignature, SCRAM_KEY_LEN) < 0)
{
elog(ERROR, "could not calculate client signature");
}
@@ -1130,7 +1130,7 @@ verify_client_proof(scram_state *state)
ClientKey[i] = state->ClientProof[i] ^ ClientSignature[i];
/* Hash it one more time, and compare with StoredKey */
- if (scram_H(ClientKey, SCRAM_KEY_LEN, client_StoredKey) < 0)
+ if (scram_H(ClientKey, SCRAM_KEY_LEN, client_StoredKey, SCRAM_KEY_LEN) < 0)
elog(ERROR, "could not hash stored key");
if (memcmp(client_StoredKey, state->StoredKey, SCRAM_KEY_LEN) != 0)
@@ -1374,7 +1374,7 @@ build_server_final_message(scram_state *state)
scram_HMAC_update(&ctx,
state->client_final_message_without_proof,
strlen(state->client_final_message_without_proof)) < 0 ||
- scram_HMAC_final(ServerSignature, &ctx) < 0)
+ scram_HMAC_final(&ctx, ServerSignature, SCRAM_KEY_LEN) < 0)
{
elog(ERROR, "could not calculate server signature");
}
@@ -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..b57e93ddbd 100644
--- a/src/backend/replication/backup_manifest.c
+++ b/src/backend/replication/backup_manifest.c
@@ -180,7 +180,7 @@ AddFileToBackupManifest(backup_manifest_info *manifest, const char *spcoid,
int checksumlen;
uint64 dstlen;
- checksumlen = pg_checksum_final(checksum_ctx, checksumbuf);
+ checksumlen = pg_checksum_final(checksum_ctx, checksumbuf, PG_CHECKSUM_MAX_LENGTH);
if (checksumlen < 0)
elog(ERROR, "could not finalize checksum of file \"%s\"",
pathname);
@@ -330,7 +330,7 @@ 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..c4915435ab 100644
--- a/src/backend/utils/adt/cryptohashfuncs.c
+++ b/src/backend/utils/adt/cryptohashfuncs.c
@@ -114,7 +114,7 @@ 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/bin/pg_verifybackup/parse_manifest.c b/src/bin/pg_verifybackup/parse_manifest.c
index db2fa90cfe..c138991be4 100644
--- a/src/bin/pg_verifybackup/parse_manifest.c
+++ b/src/bin/pg_verifybackup/parse_manifest.c
@@ -659,7 +659,7 @@ 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/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c
index bb3f2783d0..cec932b9fa 100644
--- a/src/bin/pg_verifybackup/pg_verifybackup.c
+++ b/src/bin/pg_verifybackup/pg_verifybackup.c
@@ -780,7 +780,7 @@ verify_file_checksum(verifier_context *context, manifest_file *m,
}
/* Get the final checksum. */
- checksumlen = pg_checksum_final(&checksum_ctx, checksumbuf);
+ checksumlen = pg_checksum_final(&checksum_ctx, checksumbuf, PG_CHECKSUM_MAX_LENGTH);
if (checksumlen < 0)
{
report_backup_error(context,
diff --git a/src/common/checksum_helper.c b/src/common/checksum_helper.c
index 2881b2c178..6062c88321 100644
--- a/src/common/checksum_helper.c
+++ b/src/common/checksum_helper.c
@@ -173,7 +173,7 @@ pg_checksum_update(pg_checksum_context *context, const uint8 *input,
* or -1 for a failure.
*/
int
-pg_checksum_final(pg_checksum_context *context, uint8 *output)
+pg_checksum_final(pg_checksum_context *context, uint8 *output, size_t len)
{
int retval = 0;
@@ -198,25 +198,25 @@ 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, len) < 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, len) < 0)
return -1;
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_sha2, output) < 0)
+ if (pg_cryptohash_final(context->raw_context.c_sha2, output, len) < 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, len) < 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..94e6a396e0 100644
--- a/src/common/cryptohash.c
+++ b/src/common/cryptohash.c
@@ -165,7 +165,7 @@ pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
* given a NULL context.
*/
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;
@@ -176,19 +176,19 @@ pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
pg_md5_final(&ctx->data.md5, dest);
break;
case PG_SHA1:
- pg_sha1_final(&ctx->data.sha1, dest);
+ pg_sha1_final(&ctx->data.sha1, dest, len);
break;
case PG_SHA224:
- pg_sha224_final(&ctx->data.sha224, dest);
+ pg_sha224_final(&ctx->data.sha224, dest, len);
break;
case PG_SHA256:
- pg_sha256_final(&ctx->data.sha256, dest);
+ pg_sha256_final(&ctx->data.sha256, dest, len);
break;
case PG_SHA384:
- pg_sha384_final(&ctx->data.sha384, dest);
+ pg_sha384_final(&ctx->data.sha384, dest, len);
break;
case PG_SHA512:
- pg_sha512_final(&ctx->data.sha512, dest);
+ pg_sha512_final(&ctx->data.sha512, dest, len);
break;
}
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..8f10c94137 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;
@@ -106,13 +106,13 @@ scram_HMAC_update(scram_HMAC_ctx *ctx, const char *str, int slen)
* The hash function used is SHA-256. Returns 0 on success, -1 on failure.
*/
int
-scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx)
+scram_HMAC_final(scram_HMAC_ctx *ctx, uint8 *result, int rlen)
{
uint8 h[SCRAM_KEY_LEN];
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, rlen) < 0)
{
pg_cryptohash_free(ctx->sha256ctx);
return -1;
@@ -161,7 +161,7 @@ scram_SaltedPassword(const char *password,
if (scram_HMAC_init(&hmac_ctx, (uint8 *) password, password_len) < 0 ||
scram_HMAC_update(&hmac_ctx, salt, saltlen) < 0 ||
scram_HMAC_update(&hmac_ctx, (char *) &one, sizeof(uint32)) < 0 ||
- scram_HMAC_final(Ui_prev, &hmac_ctx) < 0)
+ scram_HMAC_final(&hmac_ctx, Ui_prev, SCRAM_KEY_LEN) < 0)
{
return -1;
}
@@ -173,7 +173,7 @@ scram_SaltedPassword(const char *password,
{
if (scram_HMAC_init(&hmac_ctx, (uint8 *) password, password_len) < 0 ||
scram_HMAC_update(&hmac_ctx, (const char *) Ui_prev, SCRAM_KEY_LEN) < 0 ||
- scram_HMAC_final(Ui, &hmac_ctx) < 0)
+ scram_HMAC_final(&hmac_ctx, Ui, SCRAM_KEY_LEN) < 0)
{
return -1;
}
@@ -192,7 +192,7 @@ scram_SaltedPassword(const char *password,
* not included in the hash). Returns 0 on success, -1 on failure.
*/
int
-scram_H(const uint8 *input, int len, uint8 *result)
+scram_H(const uint8 *input, int len, uint8 *result, int rlen)
{
pg_cryptohash_ctx *ctx;
@@ -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, rlen) < 0)
{
pg_cryptohash_free(ctx);
return -1;
@@ -216,13 +216,13 @@ scram_H(const uint8 *input, int len, uint8 *result)
* Calculate ClientKey. Returns 0 on success, -1 on failure.
*/
int
-scram_ClientKey(const uint8 *salted_password, uint8 *result)
+scram_ClientKey(const uint8 *salted_password, uint8 *result, int rlen)
{
scram_HMAC_ctx ctx;
if (scram_HMAC_init(&ctx, salted_password, SCRAM_KEY_LEN) < 0 ||
scram_HMAC_update(&ctx, "Client Key", strlen("Client Key")) < 0 ||
- scram_HMAC_final(result, &ctx) < 0)
+ scram_HMAC_final(&ctx, result, rlen) < 0)
{
return -1;
}
@@ -240,7 +240,7 @@ scram_ServerKey(const uint8 *salted_password, uint8 *result)
if (scram_HMAC_init(&ctx, salted_password, SCRAM_KEY_LEN) < 0 ||
scram_HMAC_update(&ctx, "Server Key", strlen("Server Key")) < 0 ||
- scram_HMAC_final(result, &ctx) < 0)
+ scram_HMAC_final(&ctx, result, SCRAM_KEY_LEN) < 0)
{
return -1;
}
@@ -278,9 +278,9 @@ scram_build_secret(const char *salt, int saltlen, int iterations,
/* Calculate StoredKey and ServerKey */
if (scram_SaltedPassword(password, salt, saltlen, iterations,
salted_password) < 0 ||
- scram_ClientKey(salted_password, stored_key) < 0 ||
- scram_H(stored_key, SCRAM_KEY_LEN, stored_key) < 0 ||
- scram_ServerKey(salted_password, server_key) < 0)
+ scram_ClientKey(salted_password, stored_key, SCRAM_KEY_LEN) < 0 ||
+ scram_H(stored_key, SCRAM_KEY_LEN, stored_key, SCRAM_KEY_LEN) < 0 ||
+ scram_ServerKey(salted_password, server_key, SCRAM_KEY_LEN) < 0)
{
#ifdef FRONTEND
return NULL;
diff --git a/src/common/sha2.c b/src/common/sha2.c
index ae4936b1a8..4654a5f71b 100644
--- a/src/common/sha2.c
+++ b/src/common/sha2.c
@@ -574,7 +574,7 @@ SHA256_Last(pg_sha256_ctx *context)
}
void
-pg_sha256_final(pg_sha256_ctx *context, uint8 *digest)
+pg_sha256_final(pg_sha256_ctx *context, uint8 *digest, size_t len)
{
/* If no digest buffer is passed, we don't bother doing this: */
if (digest != NULL)
@@ -592,7 +592,7 @@ pg_sha256_final(pg_sha256_ctx *context, uint8 *digest)
}
}
#endif
- memcpy(digest, context->state, PG_SHA256_DIGEST_LENGTH);
+ memcpy(digest, context->state, len);
}
/* Clean up state data: */
@@ -902,7 +902,7 @@ SHA512_Last(pg_sha512_ctx *context)
}
void
-pg_sha512_final(pg_sha512_ctx *context, uint8 *digest)
+pg_sha512_final(pg_sha512_ctx *context, uint8 *digest, size_t len)
{
/* If no digest buffer is passed, we don't bother doing this: */
if (digest != NULL)
@@ -921,7 +921,7 @@ pg_sha512_final(pg_sha512_ctx *context, uint8 *digest)
}
}
#endif
- memcpy(digest, context->state, PG_SHA512_DIGEST_LENGTH);
+ memcpy(digest, context->state, len);
}
/* Zero out state data */
@@ -947,7 +947,7 @@ pg_sha384_update(pg_sha384_ctx *context, const uint8 *data, size_t len)
}
void
-pg_sha384_final(pg_sha384_ctx *context, uint8 *digest)
+pg_sha384_final(pg_sha384_ctx *context, uint8 *digest, size_t len)
{
/* If no digest buffer is passed, we don't bother doing this: */
if (digest != NULL)
@@ -966,7 +966,7 @@ pg_sha384_final(pg_sha384_ctx *context, uint8 *digest)
}
}
#endif
- memcpy(digest, context->state, PG_SHA384_DIGEST_LENGTH);
+ memcpy(digest, context->state, len);
}
/* Zero out state data */
@@ -991,7 +991,7 @@ pg_sha224_update(pg_sha224_ctx *context, const uint8 *data, size_t len)
}
void
-pg_sha224_final(pg_sha224_ctx *context, uint8 *digest)
+pg_sha224_final(pg_sha224_ctx *context, uint8 *digest, size_t len)
{
/* If no digest buffer is passed, we don't bother doing this: */
if (digest != NULL)
@@ -1009,7 +1009,7 @@ pg_sha224_final(pg_sha224_ctx *context, uint8 *digest)
}
}
#endif
- memcpy(digest, context->state, PG_SHA224_DIGEST_LENGTH);
+ memcpy(digest, context->state, len);
}
/* Clean up state data: */
diff --git a/src/include/common/checksum_helper.h b/src/include/common/checksum_helper.h
index cac7570ea1..20d939b4bc 100644
--- a/src/include/common/checksum_helper.h
+++ b/src/include/common/checksum_helper.h
@@ -67,6 +67,6 @@ extern char *pg_checksum_type_name(pg_checksum_type);
extern int pg_checksum_init(pg_checksum_context *, pg_checksum_type);
extern int pg_checksum_update(pg_checksum_context *, const uint8 *input,
size_t len);
-extern int pg_checksum_final(pg_checksum_context *, uint8 *output);
+extern int pg_checksum_final(pg_checksum_context *, uint8 *output, size_t len);
#endif
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/include/common/scram-common.h b/src/include/common/scram-common.h
index 9d684b41e8..d9f1537243 100644
--- a/src/include/common/scram-common.h
+++ b/src/include/common/scram-common.h
@@ -21,7 +21,7 @@
#define SCRAM_SHA_256_PLUS_NAME "SCRAM-SHA-256-PLUS" /* with channel binding */
/* Length of SCRAM keys (client and server) */
-#define SCRAM_KEY_LEN PG_SHA256_DIGEST_LENGTH
+#define SCRAM_KEY_LEN PG_SHA512_DIGEST_LENGTH /* make sure that have enough space */
/* length of HMAC */
#define SHA256_HMAC_B PG_SHA256_BLOCK_LENGTH
@@ -57,11 +57,11 @@ typedef struct
extern int scram_HMAC_init(scram_HMAC_ctx *ctx, const uint8 *key, int keylen);
extern int scram_HMAC_update(scram_HMAC_ctx *ctx, const char *str, int slen);
-extern int scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx);
+extern int scram_HMAC_final(scram_HMAC_ctx *ctx, uint8 *result, int rlen);
extern int scram_SaltedPassword(const char *password, const char *salt,
int saltlen, int iterations, uint8 *result);
-extern int scram_H(const uint8 *str, int len, uint8 *result);
+extern int scram_H(const uint8 *str, int len, uint8 *result, int rlen);
extern int scram_ClientKey(const uint8 *salted_password, uint8 *result);
extern int scram_ServerKey(const uint8 *salted_password, uint8 *result);
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index 002469540a..38a5741a80 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -785,7 +785,7 @@ calculate_client_proof(fe_scram_state *state,
if (scram_SaltedPassword(state->password, state->salt, state->saltlen,
state->iterations, state->SaltedPassword) < 0 ||
scram_ClientKey(state->SaltedPassword, ClientKey) < 0 ||
- scram_H(ClientKey, SCRAM_KEY_LEN, StoredKey) < 0 ||
+ scram_H(ClientKey, SCRAM_KEY_LEN, StoredKey, SCRAM_KEY_LEN) < 0 ||
scram_HMAC_init(&ctx, StoredKey, SCRAM_KEY_LEN) < 0 ||
scram_HMAC_update(&ctx,
state->client_first_message_bare,
@@ -798,7 +798,7 @@ calculate_client_proof(fe_scram_state *state,
scram_HMAC_update(&ctx,
client_final_message_without_proof,
strlen(client_final_message_without_proof)) < 0 ||
- scram_HMAC_final(ClientSignature, &ctx) < 0)
+ scram_HMAC_final(&ctx, ClientSignature, SCRAM_KEY_LEN) < 0)
{
return false;
}
@@ -836,7 +836,7 @@ verify_server_signature(fe_scram_state *state, bool *match)
scram_HMAC_update(&ctx,
state->client_final_message_without_proof,
strlen(state->client_final_message_without_proof)) < 0 ||
- scram_HMAC_final(expected_ServerSignature, &ctx) < 0)
+ scram_HMAC_final(&ctx, expected_ServerSignature, SCRAM_KEY_LEN) < 0)
{
return false;
}
At Tue, 9 Feb 2021 22:01:45 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
Hi Hackers,
Per Coverity.
Coverity complaints about pg_cryptohash_final function.
And I agree with Coverity, it's a bad design.
Its allows this:#define MY_RESULT_LENGTH 32
function pgtest(char * buffer, char * text) {
pg_cryptohash_ctx *ctx;
uint8 digest[MY_RESULT_LENGTH];ctx = pg_cryptohash_create(PG_SHA512);
pg_cryptohash_init(ctx);
pg_cryptohash_update(ctx, (uint8 *) buffer, text);
pg_cryptohash_final(ctx, digest); // <-- CID 1446240 (#1 of 1):
Out-of-bounds access (OVERRUN)
pg_cryptohash_free(ctx);
return
}Attached has a patch with suggestions to make things better.
I'm not sure about the details, but it looks like broken.
make complains for inconsistent prototypes abd cryptohahs.c and sha1.c
doesn't seem to agree on its interface.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
At Wed, 10 Feb 2021 12:13:44 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
At Tue, 9 Feb 2021 22:01:45 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
Hi Hackers,
Per Coverity.
Coverity complaints about pg_cryptohash_final function.
And I agree with Coverity, it's a bad design.
Its allows this:#define MY_RESULT_LENGTH 32
function pgtest(char * buffer, char * text) {
pg_cryptohash_ctx *ctx;
uint8 digest[MY_RESULT_LENGTH];ctx = pg_cryptohash_create(PG_SHA512);
pg_cryptohash_init(ctx);
pg_cryptohash_update(ctx, (uint8 *) buffer, text);
pg_cryptohash_final(ctx, digest); // <-- CID 1446240 (#1 of 1):
Out-of-bounds access (OVERRUN)
pg_cryptohash_free(ctx);
return
}Attached has a patch with suggestions to make things better.
I'm not sure about the details, but it looks like broken.
make complains for inconsistent prototypes abd cryptohahs.c and sha1.c
doesn't seem to agree on its interface.
Sorry, my messages was broken.
make complains for inconsistent prototypes, and cryptohahs.c and
sha1.c don't seem to agree on the interface of pg_sha1_final.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
At Tue, 9 Feb 2021 22:01:45 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
Hi Hackers,
Per Coverity.
Coverity complaints about pg_cryptohash_final function.
And I agree with Coverity, it's a bad design.
Its allows this:#define MY_RESULT_LENGTH 32
function pgtest(char * buffer, char * text) {
pg_cryptohash_ctx *ctx;
uint8 digest[MY_RESULT_LENGTH];ctx = pg_cryptohash_create(PG_SHA512);
pg_cryptohash_init(ctx);
pg_cryptohash_update(ctx, (uint8 *) buffer, text);
pg_cryptohash_final(ctx, digest); // <-- CID 1446240 (#1 of 1):
Out-of-bounds access (OVERRUN)
pg_cryptohash_free(ctx);
return
}
It seems to me that the above just means the caller must provide a
digest buffer that fits the use. In the above example digest just must
be 64 byte. If Coverity complains so, what should do for the
complaint is to fix the caller to provide a digest buffer of the
correct size.
Could you show the detailed context where Coverity complained?
Attached has a patch with suggestions to make things better.
So it doesn't seem to me the right direction. Even if we are going to
make pg_cryptohash_final to take the buffer length, it should
error-out or assert-out if the length is too small rather than copy a
part of the digest bytes. (In short, it would only be assertion-use.)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Wed, Feb 10, 2021 at 01:44:12PM +0900, Kyotaro Horiguchi wrote:
It seems to me that the above just means the caller must provide a
digest buffer that fits the use. In the above example digest just must
be 64 byte. If Coverity complains so, what should do for the
complaint is to fix the caller to provide a digest buffer of the
correct size.Could you show the detailed context where Coverity complained?
FWIW, the community Coverity instance is not complaining here, so
I have no idea what kind of configuration it uses to generate this
report. Saying that, this is just the same idea as cfc40d3 for
base64.c and aef8948 for hex.c where we provide the length of the
result buffer to be able to control any overflow. So that's a safety
belt to avoid a caller to do stupid things where he/she would
overwrite some memory with a buffer allocation with a size lower than
the size of the digest expected in the result generated.
So it doesn't seem to me the right direction. Even if we are going to
make pg_cryptohash_final to take the buffer length, it should
error-out or assert-out if the length is too small rather than copy a
part of the digest bytes. (In short, it would only be assertion-use.)
Yes, we could be more defensive here, and considering libpq I think
that this had better be an error rather than an assertion to remain on
the safe side. The patch proposed is incomplete on several points:
- cryptohash_openssl.c is not touched, so this patch will fail to
compile with --with-ssl=openssl (or --with-openssl if you want).
- There is nothing actually checked in the final function. As we
already know the size of the result digest, we just need to make sure
that the size of the output is at least the size of the digest, so we
can just add a check based on MD5_DIGEST_LENGTH and such. There is no
need to touch the internal functions of MD5/SHA1/SHA2 for the
non-OpenSSL case. For the OpenSSL case, and looking at digest.c in
the upstream code, we would need a similar check, as
EVP_DigestFinal_ex() would happily overwrite the area if the caller is
not careful (note that the third argument of the function reports the
number of bytes written, *after* the fact).
I don't see much the point to complicate scram_HMAC_final() and
scram_H() here, as well as the manipulations done for SCRAM_KEY_LEN in
scram-common.h.
--
Michael
Em qua., 10 de fev. de 2021 às 01:44, Kyotaro Horiguchi <
horikyota.ntt@gmail.com> escreveu:
At Tue, 9 Feb 2021 22:01:45 -0300, Ranier Vilela <ranier.vf@gmail.com>
wrote inHi Hackers,
Per Coverity.
Coverity complaints about pg_cryptohash_final function.
And I agree with Coverity, it's a bad design.
Its allows this:#define MY_RESULT_LENGTH 32
function pgtest(char * buffer, char * text) {
pg_cryptohash_ctx *ctx;
uint8 digest[MY_RESULT_LENGTH];ctx = pg_cryptohash_create(PG_SHA512);
pg_cryptohash_init(ctx);
pg_cryptohash_update(ctx, (uint8 *) buffer, text);
pg_cryptohash_final(ctx, digest); // <-- CID 1446240 (#1 of 1):
Out-of-bounds access (OVERRUN)
pg_cryptohash_free(ctx);
return
}It seems to me that the above just means the caller must provide a
digest buffer that fits the use. In the above example digest just must
be 64 byte. If Coverity complains so, what should do for the
complaint is to fix the caller to provide a digest buffer of the
correct size.
Exactly.
Could you show the detailed context where Coverity complained?
Coverity complains about call memcpy with fixed size, in a context with
buffer variable size supplied by the caller.
Attached has a patch with suggestions to make things better.
So it doesn't seem to me the right direction. Even if we are going to
make pg_cryptohash_final to take the buffer length, it should
error-out or assert-out if the length is too small rather than copy a
part of the digest bytes. (In short, it would only be assertion-use.)
It is necessary to correct the interfaces. To caller, inform the size of
the buffer it created.
I think it should be error-out, because the buffer can be malloc.
regards,
Ranier Vilela
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
Attachments:
pg_cryptohash_v2.patchtext/x-diff; charset=us-asciiDownload
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);
Em qui., 11 de fev. de 2021 às 09:47, Michael Paquier <michael@paquier.xyz>
escreveu:
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.
I do not agree. Coverity is a valuable tool that points to bad design
functions.
As demonstrated in the first email, it allows the user of the functions to
corrupt memory.
So it makes perfect sense, fixing the interface to prevent and prevent
future modifications, simply breaking cryptohash api.
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.
Too fast. I spent 30 minutes doing the patch.
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.
I meant that it is not a case of assertion, as suggested by Kyotaro,
because someone might want to create a dynamic buffer per malloc, to store
the digest.
Anyway, the buffer creator needs to tell the functions what the actual
buffer size is, so they can decide what to do.
regards,
Ranier Vilela
Em qui., 11 de fev. de 2021 às 09:47, Michael Paquier <michael@paquier.xyz>
escreveu:
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.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.
Ok, I take a look at your patch and I have comments:
1. Looks missed contrib/pgcrypto.
2. scram_HMAC_final function still have a exchanged parameters,
which in the future may impair maintenance.
Attached the v3 same patch.
regards,
Ranier Vilela
Attachments:
pg_cryptohash_v3.patchapplication/octet-stream; name=pg_cryptohash_v3.patchDownload
diff --git a/contrib/pgcrypto/internal-sha2.c b/contrib/pgcrypto/internal-sha2.c
index 0fe53e15af..c0d9eea731 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, PG_SHA224_DIGEST_LENGTH) < 0)
elog(ERROR, "could not finalize %s context", "SHA2");
}
diff --git a/contrib/pgcrypto/internal.c b/contrib/pgcrypto/internal.c
index ef6ce2fb1e..955501b4d4 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, MD5_DIGEST_LENGTH) < 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, SHA1_DIGEST_LENGTH) < 0)
elog(ERROR, "could not finalize %s context", "SHA1");
}
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/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/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..e58709041d 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;
@@ -106,13 +106,13 @@ scram_HMAC_update(scram_HMAC_ctx *ctx, const char *str, int slen)
* The hash function used is SHA-256. Returns 0 on success, -1 on failure.
*/
int
-scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx)
+scram_HMAC_final(scram_HMAC_ctx *ctx, uint8 *result)
{
uint8 h[SCRAM_KEY_LEN];
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;
@@ -161,7 +161,7 @@ scram_SaltedPassword(const char *password,
if (scram_HMAC_init(&hmac_ctx, (uint8 *) password, password_len) < 0 ||
scram_HMAC_update(&hmac_ctx, salt, saltlen) < 0 ||
scram_HMAC_update(&hmac_ctx, (char *) &one, sizeof(uint32)) < 0 ||
- scram_HMAC_final(Ui_prev, &hmac_ctx) < 0)
+ scram_HMAC_final(&hmac_ctx, Ui_prev) < 0)
{
return -1;
}
@@ -173,7 +173,7 @@ scram_SaltedPassword(const char *password,
{
if (scram_HMAC_init(&hmac_ctx, (uint8 *) password, password_len) < 0 ||
scram_HMAC_update(&hmac_ctx, (const char *) Ui_prev, SCRAM_KEY_LEN) < 0 ||
- scram_HMAC_final(Ui, &hmac_ctx) < 0)
+ scram_HMAC_final(&hmac_ctx, Ui) < 0)
{
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;
@@ -222,7 +222,7 @@ scram_ClientKey(const uint8 *salted_password, uint8 *result)
if (scram_HMAC_init(&ctx, salted_password, SCRAM_KEY_LEN) < 0 ||
scram_HMAC_update(&ctx, "Client Key", strlen("Client Key")) < 0 ||
- scram_HMAC_final(result, &ctx) < 0)
+ scram_HMAC_final(&ctx, result) < 0)
{
return -1;
}
@@ -240,7 +240,7 @@ scram_ServerKey(const uint8 *salted_password, uint8 *result)
if (scram_HMAC_init(&ctx, salted_password, SCRAM_KEY_LEN) < 0 ||
scram_HMAC_update(&ctx, "Server Key", strlen("Server Key")) < 0 ||
- scram_HMAC_final(result, &ctx) < 0)
+ scram_HMAC_final(&ctx, result) < 0)
{
return -1;
}
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/include/common/scram-common.h b/src/include/common/scram-common.h
index 9d684b41e8..9d7d1171b5 100644
--- a/src/include/common/scram-common.h
+++ b/src/include/common/scram-common.h
@@ -57,7 +57,7 @@ typedef struct
extern int scram_HMAC_init(scram_HMAC_ctx *ctx, const uint8 *key, int keylen);
extern int scram_HMAC_update(scram_HMAC_ctx *ctx, const char *str, int slen);
-extern int scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx);
+extern int scram_HMAC_final(scram_HMAC_ctx *ctx, uint8 *result);
extern int scram_SaltedPassword(const char *password, const char *salt,
int saltlen, int iterations, uint8 *result);
At Thu, 11 Feb 2021 19:55:45 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
Em qui., 11 de fev. de 2021 às 09:47, Michael Paquier <michael@paquier.xyz>
escreveu: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.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.Ok, I take a look at your patch and I have comments:
1. Looks missed contrib/pgcrypto.
2. scram_HMAC_final function still have a exchanged parameters,
which in the future may impair maintenance.
The v3 drops the changes of the uuid_ossp contrib. I'm not sure the
change of scram_HMAC_final is needed.
In v2, int_md5_finish() calls pg_cryptohash_final() with
h->block_size(h) (64) but it should be h->result_size(h)
(16). int_sha1_finish() is wrong the same way. (and, v3 seems fixing
them in the wrong way.)
Although I don't oppose to make things defensive, I think the derived
interfaces should be defensive in the same extent if we do. Especially
the calls to the function in checksum_helper.c is just nullifying the
protection.
For now, we can actually protect from too-short buffers in the
following places. pg_cryptohash_final receives the buffer length
irrelevant to the actual length in other places.
0/3 places in pgcrypto.
2/2 places in uuid-ossp.
1/1 place in auth-scram.c
1/1 place in backup_manifest.c
1/1 place in cryptohashfuncs.c
1/1 place in parse_manifest.c
0/4 places in checksum_helper.c
1/2 place in md5_common.c
2/4 places in scram-common.c (The two places are claimed not to need the protection.)
Total 9/19 places. I think at least pg_checksum_final() should take
the buffer length. I'm not sure about px_md_finish() and
hmac_md_finish()..
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Fri, Feb 12, 2021 at 03:21:40PM +0900, Kyotaro Horiguchi wrote:
The v3 drops the changes of the uuid_ossp contrib. I'm not sure the
change of scram_HMAC_final is needed.
Meaning that v3 would fail to compile uuid-ossp. v3 also produces
compilation warnings in auth-scram.c.
In v2, int_md5_finish() calls pg_cryptohash_final() with
h->block_size(h) (64) but it should be h->result_size(h)
(16). int_sha1_finish() is wrong the same way. (and, v3 seems fixing
them in the wrong way.)
Right. These should just use h->result_size(h), and not
h->block_size(h).
-extern int scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx);
+extern int scram_HMAC_final(scram_HMAC_ctx *ctx, uint8 *result);
There is no point in this change. You just make back-patching harder
while doing nothing about the problem at hand.- if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf) < 0)
+ if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf,
+ PG_SHA256_DIGEST_LENGTH) < 0)
Here this could just use sizeof(checksumbuf)? This pattern could be
used elsewhere as well, like in md5_common.c.Although I don't oppose to make things defensive, I think the derived
interfaces should be defensive in the same extent if we do. Especially
the calls to the function in checksum_helper.c is just nullifying the
protection.
The checksum stuff just relies on PG_CHECKSUM_MAX_LENGTH and there are
already static assertions used as sanity checks, so I see little point
in adding a new argument that would be just PG_CHECKSUM_MAX_LENGTH.
This backup checksum code is already very specific, and it is not
intended for uses as generic as the cryptohash functions. With such a
change, my guess is that it becomes really easy to miss that
pg_checksum_final() has to return the size of the digest result, and
not the maximum buffer size allocation. Perhaps one thing this part
could do is just to save the digest length in a variable and use it
for retval and the third argument of pg_cryptohash_final(), but the
impact looks limited.
Total 9/19 places. I think at least pg_checksum_final() should take
the buffer length. I'm not sure about px_md_finish() and
hmac_md_finish()..
I guess that you mean px_hmac_finish() for the second one. The first
one is tied to passing down result_size() and down to the cryptohash
functoins, meaning that there is no need to take about it more than
that IMO. The second one would be tied to the HMAC refactoring. This
would be valuable in the case of pgcrypto when building with OpenSSL,
meaning that the code would go through the defenses put in place at
the PG level.
--
Michael
Em sex., 12 de fev. de 2021 às 22:47, Michael Paquier <michael@paquier.xyz>
escreveu:
On Fri, Feb 12, 2021 at 03:21:40PM +0900, Kyotaro Horiguchi wrote:
The v3 drops the changes of the uuid_ossp contrib. I'm not sure the
change of scram_HMAC_final is needed.Meaning that v3 would fail to compile uuid-ossp. v3 also produces
compilation warnings in auth-scram.c.In v2, int_md5_finish() calls pg_cryptohash_final() with
h->block_size(h) (64) but it should be h->result_size(h)
(16). int_sha1_finish() is wrong the same way. (and, v3 seems fixing
them in the wrong way.)Right. These should just use h->result_size(h), and not
h->block_size(h).-extern int scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx); +extern int scram_HMAC_final(scram_HMAC_ctx *ctx, uint8 *result); There is no point in this change. You just make back-patching harder while doing nothing about the problem at hand.
IMO there is no necessity in back-patching.
- if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf) < 0) + if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf, + PG_SHA256_DIGEST_LENGTH) < 0) Here this could just use sizeof(checksumbuf)? This pattern could be used elsewhere as well, like in md5_common.c.
Done.
Attached a v4 of patch.
regards,
Ranier Vilela
Attachments:
pg_cryptohash_v4.patchapplication/octet-stream; name=pg_cryptohash_v4.patchDownload
diff --git a/contrib/pgcrypto/internal-sha2.c b/contrib/pgcrypto/internal-sha2.c
index 0fe53e15af..c0d9eea731 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, PG_SHA224_DIGEST_LENGTH) < 0)
elog(ERROR, "could not finalize %s context", "SHA2");
}
diff --git a/contrib/pgcrypto/internal.c b/contrib/pgcrypto/internal.c
index ef6ce2fb1e..955501b4d4 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, MD5_DIGEST_LENGTH) < 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, SHA1_DIGEST_LENGTH) < 0)
elog(ERROR, "could not finalize %s context", "SHA1");
}
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 8d857f39df..284b03c50e 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -1120,7 +1120,7 @@ verify_client_proof(scram_state *state)
scram_HMAC_update(&ctx,
state->client_final_message_without_proof,
strlen(state->client_final_message_without_proof)) < 0 ||
- scram_HMAC_final(ClientSignature, &ctx) < 0)
+ scram_HMAC_final(&ctx, ClientSignature) < 0)
{
elog(ERROR, "could not calculate client signature");
}
@@ -1374,7 +1374,7 @@ build_server_final_message(scram_state *state)
scram_HMAC_update(&ctx,
state->client_final_message_without_proof,
strlen(state->client_final_message_without_proof)) < 0 ||
- scram_HMAC_final(ServerSignature, &ctx) < 0)
+ scram_HMAC_final(&ctx, ServerSignature) < 0)
{
elog(ERROR, "could not calculate server signature");
}
@@ -1429,7 +1429,8 @@ 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,
+ sizeof(sha_digest) - 1) < 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..5516ed4948 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,
+ sizeof(checksumbuf) - 1) < 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/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/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..6a7f57031e 100644
--- a/src/common/md5_common.c
+++ b/src/common/md5_common.c
@@ -78,7 +78,8 @@ 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,
+ sizeof(sum) - 1) < 0)
{
pg_cryptohash_free(ctx);
return false;
@@ -100,7 +101,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..7ac5810cf6 100644
--- a/src/common/scram-common.c
+++ b/src/common/scram-common.c
@@ -51,7 +51,8 @@ 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,
+ sizeof(keybuf) - 1) < 0)
{
pg_cryptohash_free(sha256_ctx);
return -1;
@@ -106,13 +107,14 @@ scram_HMAC_update(scram_HMAC_ctx *ctx, const char *str, int slen)
* The hash function used is SHA-256. Returns 0 on success, -1 on failure.
*/
int
-scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx)
+scram_HMAC_final(scram_HMAC_ctx *ctx, uint8 *result)
{
uint8 h[SCRAM_KEY_LEN];
Assert(ctx->sha256ctx != NULL);
- if (pg_cryptohash_final(ctx->sha256ctx, h) < 0)
+ if (pg_cryptohash_final(ctx->sha256ctx, h,
+ sizeof(h) - 1) < 0)
{
pg_cryptohash_free(ctx->sha256ctx);
return -1;
@@ -122,7 +124,8 @@ 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,
+ sizeof(result) - 1) < 0)
{
pg_cryptohash_free(ctx->sha256ctx);
return -1;
@@ -161,7 +164,7 @@ scram_SaltedPassword(const char *password,
if (scram_HMAC_init(&hmac_ctx, (uint8 *) password, password_len) < 0 ||
scram_HMAC_update(&hmac_ctx, salt, saltlen) < 0 ||
scram_HMAC_update(&hmac_ctx, (char *) &one, sizeof(uint32)) < 0 ||
- scram_HMAC_final(Ui_prev, &hmac_ctx) < 0)
+ scram_HMAC_final(&hmac_ctx, Ui_prev) < 0)
{
return -1;
}
@@ -173,7 +176,7 @@ scram_SaltedPassword(const char *password,
{
if (scram_HMAC_init(&hmac_ctx, (uint8 *) password, password_len) < 0 ||
scram_HMAC_update(&hmac_ctx, (const char *) Ui_prev, SCRAM_KEY_LEN) < 0 ||
- scram_HMAC_final(Ui, &hmac_ctx) < 0)
+ scram_HMAC_final(&hmac_ctx, Ui) < 0)
{
return -1;
}
@@ -202,7 +205,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;
@@ -222,7 +225,7 @@ scram_ClientKey(const uint8 *salted_password, uint8 *result)
if (scram_HMAC_init(&ctx, salted_password, SCRAM_KEY_LEN) < 0 ||
scram_HMAC_update(&ctx, "Client Key", strlen("Client Key")) < 0 ||
- scram_HMAC_final(result, &ctx) < 0)
+ scram_HMAC_final(&ctx, result) < 0)
{
return -1;
}
@@ -240,7 +243,7 @@ scram_ServerKey(const uint8 *salted_password, uint8 *result)
if (scram_HMAC_init(&ctx, salted_password, SCRAM_KEY_LEN) < 0 ||
scram_HMAC_update(&ctx, "Server Key", strlen("Server Key")) < 0 ||
- scram_HMAC_final(result, &ctx) < 0)
+ scram_HMAC_final(&ctx, result) < 0)
{
return -1;
}
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/include/common/scram-common.h b/src/include/common/scram-common.h
index 9d684b41e8..9d7d1171b5 100644
--- a/src/include/common/scram-common.h
+++ b/src/include/common/scram-common.h
@@ -57,7 +57,7 @@ typedef struct
extern int scram_HMAC_init(scram_HMAC_ctx *ctx, const uint8 *key, int keylen);
extern int scram_HMAC_update(scram_HMAC_ctx *ctx, const char *str, int slen);
-extern int scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx);
+extern int scram_HMAC_final(scram_HMAC_ctx *ctx, uint8 *result);
extern int scram_SaltedPassword(const char *password, const char *salt,
int saltlen, int iterations, uint8 *result);
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index 002469540a..f425140170 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -798,7 +798,7 @@ calculate_client_proof(fe_scram_state *state,
scram_HMAC_update(&ctx,
client_final_message_without_proof,
strlen(client_final_message_without_proof)) < 0 ||
- scram_HMAC_final(ClientSignature, &ctx) < 0)
+ scram_HMAC_final(&ctx, ClientSignature) < 0)
{
return false;
}
@@ -836,7 +836,7 @@ verify_server_signature(fe_scram_state *state, bool *match)
scram_HMAC_update(&ctx,
state->client_final_message_without_proof,
strlen(state->client_final_message_without_proof)) < 0 ||
- scram_HMAC_final(expected_ServerSignature, &ctx) < 0)
+ scram_HMAC_final(&ctx, expected_ServerSignature) < 0)
{
return false;
}
On Sat, Feb 13, 2021 at 05:37:32PM -0300, Ranier Vilela wrote:
IMO there is no necessity in back-patching.
You are missing the point here. What you are proposing here would not
be backpatched. However, reusing the same words as upthread, this has
a cost in terms of *future* maintenance. In short, any *future*
potential bug fix that would require to be backpatched in need of
using this function or touching its area would result in a conflict.
This changes makes no sense.
--
Michael
Em sáb., 13 de fev. de 2021 às 20:32, Michael Paquier <michael@paquier.xyz>
escreveu:
On Sat, Feb 13, 2021 at 05:37:32PM -0300, Ranier Vilela wrote:
IMO there is no necessity in back-patching.
You are missing the point here. What you are proposing here would not
be backpatched. However, reusing the same words as upthread, this has
a cost in terms of *future* maintenance. In short, any *future*
potential bug fix that would require to be backpatched in need of
using this function or touching its area would result in a conflict.
Ok. +1 for back-patching.
Any future maintenance, or use of that functions, need to consult the api.
scram_HMAC_init(scram_HMAC_ctx *ctx, const uint8 *key, int keylen);
scram_HMAC_update(scram_HMAC_ctx *ctx, const char *str, int slen);
scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx);
See both "result" and "ctx" are pointers.
Someone can use like this:
scram_HMAC_init(&ctx, key, keylen);
scram_HMAC_update(&ctx, str, slen);
scram_HMAC_final(&ctx, result); // parameters wrong order
And many compilers won't complain.
regards,
Ranier Vilela
On Sat, Feb 13, 2021 at 09:33:48PM -0300, Ranier Vilela wrote:
Em sáb., 13 de fev. de 2021 às 20:32, Michael Paquier <michael@paquier.xyz>
escreveu:You are missing the point here. What you are proposing here would not
be backpatched. However, reusing the same words as upthread, this has
a cost in terms of *future* maintenance. In short, any *future*
potential bug fix that would require to be backpatched in need of
using this function or touching its area would result in a conflict.Ok. +1 for back-patching.
Please take the time to read again my previous email.
And also, please take the time to actually test patches you send,
because the list of things getting broken is impressive. At least
you make sure that the internals of cryptohash.c generate an error as
they should because of those incorrect sizes :)
git diff --check complains, in various places.
@@ -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,
+ sizeof(checksumbuf) - 1) < 0)
elog(ERROR, "failed to finalize checksum of backup manifest");
This breaks backup manifests, due to an incorrect calculation.@@ -78,7 +78,8 @@ 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,
+ sizeof(sum) - 1) < 0)
This one breaks MD5 hashing, due to an incorrect size calculation,
again.@@ -51,7 +51,8 @@ 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,
+ sizeof(keybuf) - 1) < 0)
[...]
- if (pg_cryptohash_final(ctx->sha256ctx, h) < 0)
+ if (pg_cryptohash_final(ctx->sha256ctx, h,
+ sizeof(h) - 1) < 0)
This breaks SCRAM authentication, for the same reason. In three
places.I think that in pg_checksum_final() we had better save the digest
length in "retval" before calling pg_cryptohash_final(), and use it
for the size passed down.
contrib/uuid-ossp/ fails to compile.
contrib/pgcrypto/ fix is incorrect, requiring h->result_size(h) in
three places.
I think that as a whole we should try to minimize the number of times
we use any DIGEST_LENGTH variable, relying a maximum on sizeof().
This v4 is a mixed bad of that. Once you switch to that, there is an
interesting result with uuid-ossp, where you can notice that there is
a match between the size of dce_uuid_t and MD5_DIGEST_LENGTH.
No need to send a new patch, the attached taking care of those
issues, and it is correctly indented. I'll just look at that again
tomorrow, it is already late here.
--
Michael
Attachments:
pg_cryptohash_v5.patchtext/x-diff; charset=us-asciiDownload
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..b9b6d464a0 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, sizeof(sha_digest)) < 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..32bb0efb3d 100644
--- a/src/backend/replication/backup_manifest.c
+++ b/src/backend/replication/backup_manifest.c
@@ -330,12 +330,13 @@ 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,
+ sizeof(checksumbuf)) < 0)
elog(ERROR, "failed to finalize checksum of backup manifest");
AppendStringToManifest(manifest, "\"Manifest-Checksum\": \"");
- dstlen = pg_hex_enc_len(PG_SHA256_DIGEST_LENGTH);
+ dstlen = pg_hex_enc_len(sizeof(checksumbuf));
checksumstringbuf = palloc0(dstlen + 1); /* includes \0 */
- pg_hex_encode((char *) checksumbuf, sizeof checksumbuf,
+ pg_hex_encode((char *) checksumbuf, sizeof(checksumbuf),
checksumstringbuf, dstlen);
checksumstringbuf[dstlen] = '\0';
AppendStringToManifest(manifest, checksumstringbuf);
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..431e247d59 100644
--- a/src/common/checksum_helper.c
+++ b/src/common/checksum_helper.c
@@ -198,28 +198,32 @@ 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)
+ retval = PG_SHA224_DIGEST_LENGTH;
+ if (pg_cryptohash_final(context->raw_context.c_sha2,
+ output, retval) < 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)
+ retval = PG_SHA256_DIGEST_LENGTH;
+ if (pg_cryptohash_final(context->raw_context.c_sha2,
+ output, retval) < 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)
+ retval = PG_SHA384_DIGEST_LENGTH;
+ if (pg_cryptohash_final(context->raw_context.c_sha2,
+ output, retval) < 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)
+ retval = PG_SHA512_DIGEST_LENGTH;
+ if (pg_cryptohash_final(context->raw_context.c_sha2,
+ output, retval) < 0)
return -1;
pg_cryptohash_free(context->raw_context.c_sha2);
- retval = PG_SHA512_DIGEST_LENGTH;
break;
}
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..2114890eff 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, sizeof(sum)) < 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..0b9557376e 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, sizeof(keybuf)) < 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, sizeof(h)) < 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..3b13ae5b84 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,
+ sizeof(manifest_checksum_actual)) < 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..ecf3004e95 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->result_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..dd45fee7ed 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->result_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->result_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..f9335f2863 100644
--- a/contrib/uuid-ossp/uuid-ossp.c
+++ b/contrib/uuid-ossp/uuid-ossp.c
@@ -324,7 +324,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,
+ sizeof(uu)) < 0)
elog(ERROR, "could not finalize %s context", "MD5");
pg_cryptohash_free(ctx);
}
@@ -338,7 +339,7 @@ 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, sizeof(sha1result)) < 0)
elog(ERROR, "could not finalize %s context", "SHA1");
pg_cryptohash_free(ctx);
Em dom., 14 de fev. de 2021 às 08:22, Michael Paquier <michael@paquier.xyz>
escreveu:
On Sat, Feb 13, 2021 at 09:33:48PM -0300, Ranier Vilela wrote:
Em sáb., 13 de fev. de 2021 às 20:32, Michael Paquier <
michael@paquier.xyz>
escreveu:
You are missing the point here. What you are proposing here would not
be backpatched. However, reusing the same words as upthread, this has
a cost in terms of *future* maintenance. In short, any *future*
potential bug fix that would require to be backpatched in need of
using this function or touching its area would result in a conflict.Ok. +1 for back-patching.
Please take the time to read again my previous email.
And also, please take the time to actually test patches you send,
because the list of things getting broken is impressive. At least
you make sure that the internals of cryptohash.c generate an error as
they should because of those incorrect sizes :)git diff --check complains, in various places.
@@ -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, + sizeof(checksumbuf) - 1) < 0) elog(ERROR, "failed to finalize checksum of backup manifest"); This breaks backup manifests, due to an incorrect calculation.
Bad habits.
sizeof - 1, I use with strings.
I think that in pg_checksum_final() we had better save the digest
length in "retval" before calling pg_cryptohash_final(), and use it
for the size passed down.
pg_checksum_final I would like to see it like this:
case CHECKSUM_TYPE_SHA224:
retval = PG_SHA224_DIGEST_LENGTH;
break;
case CHECKSUM_TYPE_SHA256:
retval = PG_SHA256_DIGEST_LENGTH;
break;
case CHECKSUM_TYPE_SHA384:
retval = PG_SHA384_DIGEST_LENGTH;
break;
case CHECKSUM_TYPE_SHA512:
retval = PG_SHA512_DIGEST_LENGTH;
break;
default:
return -1;
}
if (pg_cryptohash_final(context->raw_context.c_sha2,
output, retval) < 0)
return -1;
pg_cryptohash_free(context->raw_context.c_sha2);
What do you think?
regards,
Ranier Vilela
On Sun, Feb 14, 2021 at 11:39:47AM -0300, Ranier Vilela wrote:
What do you think?
That's not a good idea for two reasons:
1) There is CRC32 to worry about, which relies on a different logic.
2) It would become easier to miss the new option as compilation would
not warn anymore if a new checksum type is added.
I have reviewed my patch this morning, tweaked a comment, and applied
it.
--
Michael
Em dom., 14 de fev. de 2021 às 22:28, Michael Paquier <michael@paquier.xyz>
escreveu:
On Sun, Feb 14, 2021 at 11:39:47AM -0300, Ranier Vilela wrote:
What do you think?
That's not a good idea for two reasons:
1) There is CRC32 to worry about, which relies on a different logic.
2) It would become easier to miss the new option as compilation would
not warn anymore if a new checksum type is added.I have reviewed my patch this morning, tweaked a comment, and applied
it.
Thanks for the commit.
regards,
Ranier Vilela