From a97dfdfd9043449e67aeefe8248891b43b37eed8 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Fri, 30 Jan 2015 13:05:04 -0500
Subject: [PATCH] Fix Coverity warning about contrib/pgcrypto's mdc_finish().
Coverity points out that mdc_finish returns a pointer to a local buffer
(which of course is gone as soon as the function returns), leaving open
a risk of misbehaviors possibly as bad as a stack overwrite.
In reality, the only possible call site is in process_data_packets()
which does not examine the returned pointer at all. So there's no
live bug, but nonetheless the code is confusing and risky. Refactor
to avoid the issue by letting process_data_packets() call mdc_finish()
directly instead of going through the pullf_read() API.
Although this is only cosmetic, it seems good to back-patch so that
the logic in pgp-decrypt.c stays in sync across all branches.
Marko Kreen
---
contrib/pgcrypto/pgp-decrypt.c | 49 +++++++++++++---------------------
1 file changed, 19 insertions(+), 30 deletions(-)
diff --git a/contrib/pgcrypto/pgp-decrypt.c b/contrib/pgcrypto/pgp-decrypt.c
index 1fd7cf39767..2c744b73a30 100644
--- a/contrib/pgcrypto/pgp-decrypt.c
+++ b/contrib/pgcrypto/pgp-decrypt.c
@@ -351,37 +351,33 @@ mdc_free(void *priv)
}
static int
-mdc_finish(PGP_Context *ctx, PullFilter *src,
- int len, uint8 **data_p)
+mdc_finish(PGP_Context *ctx, PullFilter *src, int len)
{
int res;
uint8 hash[20];
- uint8 tmpbuf[22];
+ uint8 tmpbuf[20];
+ uint8 *data;
- if (len + 1 > sizeof(tmpbuf))
+ /* should not happen */
+ if (ctx->use_mdcbuf_filter)
return PXE_BUG;
+ /* It's SHA1 */
+ if (len != 20)
+ return PXE_PGP_CORRUPT_DATA;
+
+ /* mdc_read should not call md_update */
+ ctx->in_mdc_pkt = 1;
+
/* read data */
- res = pullf_read_max(src, len + 1, data_p, tmpbuf);
+ res = pullf_read_max(src, len, &data, tmpbuf);
if (res < 0)
return res;
if (res == 0)
{
- if (ctx->mdc_checked == 0)
- {
- px_debug("no mdc");
- return PXE_PGP_CORRUPT_DATA;
- }
- return 0;
- }
-
- /* safety check */
- if (ctx->in_mdc_pkt > 1)
- {
- px_debug("mdc_finish: several times here?");
+ px_debug("no mdc");
return PXE_PGP_CORRUPT_DATA;
}
- ctx->in_mdc_pkt++;
/* is the packet sane? */
if (res != 20)
@@ -394,7 +390,7 @@ mdc_finish(PGP_Context *ctx, PullFilter *src,
* ok, we got the hash, now check
*/
px_md_finish(ctx->mdc_ctx, hash);
- res = memcmp(hash, *data_p, 20);
+ res = memcmp(hash, data, 20);
px_memset(hash, 0, 20);
px_memset(tmpbuf, 0, sizeof(tmpbuf));
if (res != 0)
@@ -403,7 +399,7 @@ mdc_finish(PGP_Context *ctx, PullFilter *src,
return PXE_PGP_CORRUPT_DATA;
}
ctx->mdc_checked = 1;
- return len;
+ return 0;
}
static int
@@ -414,12 +410,9 @@ mdc_read(void *priv, PullFilter *src, int len,
PGP_Context *ctx = priv;
/* skip this filter? */
- if (ctx->use_mdcbuf_filter)
+ if (ctx->use_mdcbuf_filter || ctx->in_mdc_pkt)
return pullf_read(src, len, data_p);
- if (ctx->in_mdc_pkt)
- return mdc_finish(ctx, src, len, data_p);
-
res = pullf_read(src, len, data_p);
if (res < 0)
return res;
@@ -878,7 +871,6 @@ process_data_packets(PGP_Context *ctx, MBuf *dst, PullFilter *src,
int got_data = 0;
int got_mdc = 0;
PullFilter *pkt = NULL;
- uint8 *tmp;
while (1)
{
@@ -937,11 +929,8 @@ process_data_packets(PGP_Context *ctx, MBuf *dst, PullFilter *src,
break;
}
- /* notify mdc_filter */
- ctx->in_mdc_pkt = 1;
-
- res = pullf_read(pkt, 8192, &tmp);
- if (res > 0)
+ res = mdc_finish(ctx, pkt, len);
+ if (res >= 0)
got_mdc = 1;
break;
default:
--
GitLab