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

At Tue, 9 Feb 2021 22:01:45 -0300, Ranier Vilela <ranier(dot)vf(at)gmail(dot)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