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

Em qua., 10 de fev. de 2021 às 01:44, Kyotaro Horiguchi <
horikyota(dot)ntt(at)gmail(dot)com> escreveu:

> 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.
>
Exactly.

> Could you show the detailed context where Coverity complained?
>
Coverity complains about call memcpy with fixed size, in a context with
buffer variable size supplied by the caller.

>
> > 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.)
>
It is necessary to correct the interfaces. To caller, inform the size of
the buffer it created.
I think it should be error-out, because the buffer can be malloc.

regards,
Ranier Vilela