Skip to content

Commit b83dcf7

Browse files
michaelpqmichaelpq
committed
Add result size as argument of pg_cryptohash_final() for overflow checks
With its current design, a careless use of pg_cryptohash_final() could would result in an out-of-bound write in memory as the size of the destination buffer to store the result digest is not known to the cryptohash internals, without the caller knowing about that. This commit adds a new argument to pg_cryptohash_final() to allow such sanity checks, and implements such defenses. The internals of SCRAM for HMAC could be tightened a bit more, but as everything is based on SCRAM_KEY_LEN with uses particular to this code there is no need to complicate its interface more than necessary, and this comes back to the refactoring of HMAC in core. Except that, this minimizes the uses of the existing DIGEST_LENGTH variables, relying instead on sizeof() for the result sizes. In ossp-uuid, this also makes the code more defensive, as it already relied on dce_uuid_t being at least the size of a MD5 digest. This is in philosophy similar to cfc40d3 for base64.c and aef8948 for hex.c. Reported-by: Ranier Vilela Author: Michael Paquier, Ranier Vilela Reviewed-by: Kyotaro Horiguchi Discussion: https://postgr.es/m/CAEudQAoqEGmcff3J4sTSV-R_16Monuz-UpJFbf_dnVH=APr02Q@mail.gmail.com
1 parent 2dd6733 commit b83dcf7

File tree

13 files changed

+82
-31
lines changed

13 files changed

+82
-31
lines changed

‎contrib/pgcrypto/internal-sha2.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ int_sha2_finish(PX_MD *h, uint8 *dst)
118118
{
119119
pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
120120

121-
if (pg_cryptohash_final(ctx, dst) < 0)
121+
if (pg_cryptohash_final(ctx, dst, h->result_size(h)) < 0)
122122
elog(ERROR, "could not finalize %s context", "SHA2");
123123
}
124124

‎contrib/pgcrypto/internal.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ int_md5_finish(PX_MD *h, uint8 *dst)
106106
{
107107
pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
108108

109-
if (pg_cryptohash_final(ctx, dst) < 0)
109+
if (pg_cryptohash_final(ctx, dst, h->result_size(h)) < 0)
110110
elog(ERROR, "could not finalize %s context", "MD5");
111111
}
112112

@@ -156,7 +156,7 @@ int_sha1_finish(PX_MD *h, uint8 *dst)
156156
{
157157
pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) h->p.ptr;
158158

159-
if (pg_cryptohash_final(ctx, dst) < 0)
159+
if (pg_cryptohash_final(ctx, dst, h->result_size(h)) < 0)
160160
elog(ERROR, "could not finalize %s context", "SHA1");
161161
}
162162

‎contrib/uuid-ossp/uuid-ossp.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -324,7 +324,8 @@ uuid_generate_internal(int v, unsigned char *ns, const char *ptr, int len)
324324
pg_cryptohash_update(ctx, (unsigned char *) ptr, len) < 0)
325325
elog(ERROR, "could not update %s context", "MD5");
326326
/* we assume sizeof MD5 result is 16, same as UUID size */
327-
if (pg_cryptohash_final(ctx, (unsigned char *) &uu) < 0)
327+
if (pg_cryptohash_final(ctx, (unsigned char *) &uu,
328+
sizeof(uu)) < 0)
328329
elog(ERROR, "could not finalize %s context", "MD5");
329330
pg_cryptohash_free(ctx);
330331
}
@@ -338,7 +339,7 @@ uuid_generate_internal(int v, unsigned char *ns, const char *ptr, int len)
338339
if (pg_cryptohash_update(ctx, ns, sizeof(uu)) < 0 ||
339340
pg_cryptohash_update(ctx, (unsigned char *) ptr, len) < 0)
340341
elog(ERROR, "could not update %s context", "SHA1");
341-
if (pg_cryptohash_final(ctx, sha1result) < 0)
342+
if (pg_cryptohash_final(ctx, sha1result, sizeof(sha1result)) < 0)
342343
elog(ERROR, "could not finalize %s context", "SHA1");
343344
pg_cryptohash_free(ctx);
344345

‎src/backend/libpq/auth-scram.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1429,7 +1429,7 @@ scram_mock_salt(const char *username)
14291429
if (pg_cryptohash_init(ctx) < 0 ||
14301430
pg_cryptohash_update(ctx, (uint8 *) username, strlen(username)) < 0 ||
14311431
pg_cryptohash_update(ctx, (uint8 *) mock_auth_nonce, MOCK_AUTH_NONCE_LEN) < 0 ||
1432-
pg_cryptohash_final(ctx, sha_digest) < 0)
1432+
pg_cryptohash_final(ctx, sha_digest, sizeof(sha_digest)) < 0)
14331433
{
14341434
pg_cryptohash_free(ctx);
14351435
return NULL;

‎src/backend/replication/backup_manifest.c

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -330,12 +330,13 @@ SendBackupManifest(backup_manifest_info *manifest)
330330
* twice.
331331
*/
332332
manifest->still_checksumming = false;
333-
if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf) < 0)
333+
if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf,
334+
sizeof(checksumbuf)) < 0)
334335
elog(ERROR, "failed to finalize checksum of backup manifest");
335336
AppendStringToManifest(manifest, "\"Manifest-Checksum\": \"");
336-
dstlen = pg_hex_enc_len(PG_SHA256_DIGEST_LENGTH);
337+
dstlen = pg_hex_enc_len(sizeof(checksumbuf));
337338
checksumstringbuf = palloc0(dstlen + 1); /* includes \0 */
338-
pg_hex_encode((char *) checksumbuf, sizeof checksumbuf,
339+
pg_hex_encode((char *) checksumbuf, sizeof(checksumbuf),
339340
checksumstringbuf, dstlen);
340341
checksumstringbuf[dstlen] = '\0';
341342
AppendStringToManifest(manifest, checksumstringbuf);

‎src/backend/utils/adt/cryptohashfuncs.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,8 @@ cryptohash_internal(pg_cryptohash_type type, bytea *input)
114114
elog(ERROR, "could not initialize %s context", typestr);
115115
if (pg_cryptohash_update(ctx, data, len) < 0)
116116
elog(ERROR, "could not update %s context", typestr);
117-
if (pg_cryptohash_final(ctx, (unsigned char *) VARDATA(result)) < 0)
117+
if (pg_cryptohash_final(ctx, (unsigned char *) VARDATA(result),
118+
digest_len) < 0)
118119
elog(ERROR, "could not finalize %s context", typestr);
119120
pg_cryptohash_free(ctx);
120121

‎src/bin/pg_verifybackup/parse_manifest.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -659,7 +659,8 @@ verify_manifest_checksum(JsonManifestParseState *parse, char *buffer,
659659
context->error_cb(context, "could not initialize checksum of manifest");
660660
if (pg_cryptohash_update(manifest_ctx, (uint8 *) buffer, penultimate_newline + 1) < 0)
661661
context->error_cb(context, "could not update checksum of manifest");
662-
if (pg_cryptohash_final(manifest_ctx, manifest_checksum_actual) < 0)
662+
if (pg_cryptohash_final(manifest_ctx, manifest_checksum_actual,
663+
sizeof(manifest_checksum_actual)) < 0)
663664
context->error_cb(context, "could not finalize checksum of manifest");
664665

665666
/* Now verify it. */

‎src/common/checksum_helper.c

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -198,28 +198,32 @@ pg_checksum_final(pg_checksum_context *context, uint8 *output)
198198
memcpy(output, &context->raw_context.c_crc32c, retval);
199199
break;
200200
case CHECKSUM_TYPE_SHA224:
201-
if (pg_cryptohash_final(context->raw_context.c_sha2, output) < 0)
201+
retval = PG_SHA224_DIGEST_LENGTH;
202+
if (pg_cryptohash_final(context->raw_context.c_sha2,
203+
output, retval) < 0)
202204
return -1;
203205
pg_cryptohash_free(context->raw_context.c_sha2);
204-
retval = PG_SHA224_DIGEST_LENGTH;
205206
break;
206207
case CHECKSUM_TYPE_SHA256:
207-
if (pg_cryptohash_final(context->raw_context.c_sha2, output) < 0)
208+
retval = PG_SHA256_DIGEST_LENGTH;
209+
if (pg_cryptohash_final(context->raw_context.c_sha2,
210+
output, retval) < 0)
208211
return -1;
209212
pg_cryptohash_free(context->raw_context.c_sha2);
210-
retval = PG_SHA256_DIGEST_LENGTH;
211213
break;
212214
case CHECKSUM_TYPE_SHA384:
213-
if (pg_cryptohash_final(context->raw_context.c_sha2, output) < 0)
215+
retval = PG_SHA384_DIGEST_LENGTH;
216+
if (pg_cryptohash_final(context->raw_context.c_sha2,
217+
output, retval) < 0)
214218
return -1;
215219
pg_cryptohash_free(context->raw_context.c_sha2);
216-
retval = PG_SHA384_DIGEST_LENGTH;
217220
break;
218221
case CHECKSUM_TYPE_SHA512:
219-
if (pg_cryptohash_final(context->raw_context.c_sha2, output) < 0)
222+
retval = PG_SHA512_DIGEST_LENGTH;
223+
if (pg_cryptohash_final(context->raw_context.c_sha2,
224+
output, retval) < 0)
220225
return -1;
221226
pg_cryptohash_free(context->raw_context.c_sha2);
222-
retval = PG_SHA512_DIGEST_LENGTH;
223227
break;
224228
}
225229

‎src/common/cryptohash.c

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -160,34 +160,46 @@ pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
160160
/*
161161
* pg_cryptohash_final
162162
*
163-
* Finalize a hash context. Note that this implementation is designed
164-
* to never fail, so this always returns 0 except if the caller has
165-
* given a NULL context.
163+
* Finalize a hash context. Note that this implementation is designed to
164+
* never fail, so this always returns 0 except if the destination buffer
165+
* is not large enough.
166166
*/
167167
int
168-
pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
168+
pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest, size_t len)
169169
{
170170
if (ctx == NULL)
171171
return -1;
172172

173173
switch (ctx->type)
174174
{
175175
case PG_MD5:
176+
if (len < MD5_DIGEST_LENGTH)
177+
return -1;
176178
pg_md5_final(&ctx->data.md5, dest);
177179
break;
178180
case PG_SHA1:
181+
if (len < SHA1_DIGEST_LENGTH)
182+
return -1;
179183
pg_sha1_final(&ctx->data.sha1, dest);
180184
break;
181185
case PG_SHA224:
186+
if (len < PG_SHA224_DIGEST_LENGTH)
187+
return -1;
182188
pg_sha224_final(&ctx->data.sha224, dest);
183189
break;
184190
case PG_SHA256:
191+
if (len < PG_SHA256_DIGEST_LENGTH)
192+
return -1;
185193
pg_sha256_final(&ctx->data.sha256, dest);
186194
break;
187195
case PG_SHA384:
196+
if (len < PG_SHA384_DIGEST_LENGTH)
197+
return -1;
188198
pg_sha384_final(&ctx->data.sha384, dest);
189199
break;
190200
case PG_SHA512:
201+
if (len < PG_SHA512_DIGEST_LENGTH)
202+
return -1;
191203
pg_sha512_final(&ctx->data.sha512, dest);
192204
break;
193205
}

‎src/common/cryptohash_openssl.c

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@
2424
#include <openssl/evp.h>
2525

2626
#include "common/cryptohash.h"
27+
#include "common/md5.h"
28+
#include "common/sha1.h"
29+
#include "common/sha2.h"
2730
#ifndef FRONTEND
2831
#include "utils/memutils.h"
2932
#include "utils/resowner.h"
@@ -181,13 +184,41 @@ pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
181184
* Finalize a hash context. Returns 0 on success, and -1 on failure.
182185
*/
183186
int
184-
pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
187+
pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest, size_t len)
185188
{
186189
int status = 0;
187190

188191
if (ctx == NULL)
189192
return -1;
190193

194+
switch (ctx->type)
195+
{
196+
case PG_MD5:
197+
if (len < MD5_DIGEST_LENGTH)
198+
return -1;
199+
break;
200+
case PG_SHA1:
201+
if (len < SHA1_DIGEST_LENGTH)
202+
return -1;
203+
break;
204+
case PG_SHA224:
205+
if (len < PG_SHA224_DIGEST_LENGTH)
206+
return -1;
207+
break;
208+
case PG_SHA256:
209+
if (len < PG_SHA256_DIGEST_LENGTH)
210+
return -1;
211+
break;
212+
case PG_SHA384:
213+
if (len < PG_SHA384_DIGEST_LENGTH)
214+
return -1;
215+
break;
216+
case PG_SHA512:
217+
if (len < PG_SHA512_DIGEST_LENGTH)
218+
return -1;
219+
break;
220+
}
221+
191222
status = EVP_DigestFinal_ex(ctx->evpctx, dest, 0);
192223

193224
/* OpenSSL internals return 1 on success, 0 on failure */

‎src/common/md5_common.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ pg_md5_hash(const void *buff, size_t len, char *hexsum)
7878

7979
if (pg_cryptohash_init(ctx) < 0 ||
8080
pg_cryptohash_update(ctx, buff, len) < 0 ||
81-
pg_cryptohash_final(ctx, sum) < 0)
81+
pg_cryptohash_final(ctx, sum, sizeof(sum)) < 0)
8282
{
8383
pg_cryptohash_free(ctx);
8484
return false;
@@ -100,7 +100,7 @@ pg_md5_binary(const void *buff, size_t len, void *outbuf)
100100

101101
if (pg_cryptohash_init(ctx) < 0 ||
102102
pg_cryptohash_update(ctx, buff, len) < 0 ||
103-
pg_cryptohash_final(ctx, outbuf) < 0)
103+
pg_cryptohash_final(ctx, outbuf, MD5_DIGEST_LENGTH) < 0)
104104
{
105105
pg_cryptohash_free(ctx);
106106
return false;

‎src/common/scram-common.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ scram_HMAC_init(scram_HMAC_ctx *ctx, const uint8 *key, int keylen)
5151
return -1;
5252
if (pg_cryptohash_init(sha256_ctx) < 0 ||
5353
pg_cryptohash_update(sha256_ctx, key, keylen) < 0 ||
54-
pg_cryptohash_final(sha256_ctx, keybuf) < 0)
54+
pg_cryptohash_final(sha256_ctx, keybuf, sizeof(keybuf)) < 0)
5555
{
5656
pg_cryptohash_free(sha256_ctx);
5757
return -1;
@@ -112,7 +112,7 @@ scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx)
112112

113113
Assert(ctx->sha256ctx != NULL);
114114

115-
if (pg_cryptohash_final(ctx->sha256ctx, h) < 0)
115+
if (pg_cryptohash_final(ctx->sha256ctx, h, sizeof(h)) < 0)
116116
{
117117
pg_cryptohash_free(ctx->sha256ctx);
118118
return -1;
@@ -122,7 +122,7 @@ scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx)
122122
if (pg_cryptohash_init(ctx->sha256ctx) < 0 ||
123123
pg_cryptohash_update(ctx->sha256ctx, ctx->k_opad, SHA256_HMAC_B) < 0 ||
124124
pg_cryptohash_update(ctx->sha256ctx, h, SCRAM_KEY_LEN) < 0 ||
125-
pg_cryptohash_final(ctx->sha256ctx, result) < 0)
125+
pg_cryptohash_final(ctx->sha256ctx, result, SCRAM_KEY_LEN) < 0)
126126
{
127127
pg_cryptohash_free(ctx->sha256ctx);
128128
return -1;
@@ -202,7 +202,7 @@ scram_H(const uint8 *input, int len, uint8 *result)
202202

203203
if (pg_cryptohash_init(ctx) < 0 ||
204204
pg_cryptohash_update(ctx, input, len) < 0 ||
205-
pg_cryptohash_final(ctx, result) < 0)
205+
pg_cryptohash_final(ctx, result, SCRAM_KEY_LEN) < 0)
206206
{
207207
pg_cryptohash_free(ctx);
208208
return -1;

‎src/include/common/cryptohash.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ typedef struct pg_cryptohash_ctx pg_cryptohash_ctx;
3232
extern pg_cryptohash_ctx *pg_cryptohash_create(pg_cryptohash_type type);
3333
extern int pg_cryptohash_init(pg_cryptohash_ctx *ctx);
3434
extern int pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len);
35-
extern int pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest);
35+
extern int pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest, size_t len);
3636
extern void pg_cryptohash_free(pg_cryptohash_ctx *ctx);
3737

3838
#endif /* PG_CRYPTOHASH_H */

0 commit comments

Comments
 (0)