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

diff --git a/contrib/pgcrypto/pgp-decrypt.c b/contrib/pgcrypto/pgp-decrypt.c
index 1fd7cf3976714e65cf763a9b1c4b0f725a5861b6..2c744b73a3018393b95321e7ec824335d9bf647d 100644 (file)
--- 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: