Re: pg_cryptohash_final possible out-of-bounds access (per Coverity)

Em sex., 12 de fev. de 2021 às 22:47, Michael Paquier <michael(at)paquier(dot)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