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

Em qui., 11 de fev. de 2021 às 09:47, Michael Paquier <michael(at)paquier(dot)xyz>
escreveu:

> On Wed, Feb 10, 2021 at 09:14:46AM -0300, Ranier Vilela wrote:
> > It is necessary to correct the interfaces. To caller, inform the size of
> > the buffer it created.
>
> Well, Coverity likes nannyism, so each one of its reports is to take
> with a pinch of salt, so there is no point to change something that
> does not make sense just to please a static analyzer. The context
> of the code matters.
>
I do not agree. Coverity is a valuable tool that points to bad design
functions.
As demonstrated in the first email, it allows the user of the functions to
corrupt memory.
So it makes perfect sense, fixing the interface to prevent and prevent
future modifications, simply breaking cryptohash api.

>
> Now, the patch you sent has no need to be that complicated, and it
> partially works while not actually solving at all the problem you are
> trying to solve (nothing done for MD5 or OpenSSL). Attached is an
> example of what I finish with while poking at this issue. There is IMO
> no point to touch the internals of SCRAM that all rely on the same
> digest lengths for the proof generation with SHA256.
>
Too fast. I spent 30 minutes doing the patch.

>
> > I think it should be error-out, because the buffer can be malloc.
>
> I don't understand what you mean here, but cryptohash[_openssl].c
> should not issue an error directly, just return a status code that the
> caller can consume to generate an error.
>
I meant that it is not a case of assertion, as suggested by Kyotaro,
because someone might want to create a dynamic buffer per malloc, to store
the digest.
Anyway, the buffer creator needs to tell the functions what the actual
buffer size is, so they can decide what to do.

regards,
Ranier Vilela