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

At Thu, 11 Feb 2021 19:55:45 -0300, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote in
> 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.
> >
> > 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.
> >
> Ok, I take a look at your patch and I have comments:
>
> 1. Looks missed contrib/pgcrypto.
> 2. scram_HMAC_final function still have a exchanged parameters,
> which in the future may impair maintenance.

The v3 drops the changes of the uuid_ossp contrib. I'm not sure the
change of scram_HMAC_final is needed.

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

Although I don't oppose to make things defensive, I think the derived
interfaces should be defensive in the same extent if we do. Especially
the calls to the function in checksum_helper.c is just nullifying the
protection.

For now, we can actually protect from too-short buffers in the
following places. pg_cryptohash_final receives the buffer length
irrelevant to the actual length in other places.

0/3 places in pgcrypto.
2/2 places in uuid-ossp.
1/1 place in auth-scram.c
1/1 place in backup_manifest.c
1/1 place in cryptohashfuncs.c
1/1 place in parse_manifest.c
0/4 places in checksum_helper.c
1/2 place in md5_common.c
2/4 places in scram-common.c (The two places are claimed not to need the protection.)

Total 9/19 places. I think at least pg_checksum_final() should take
the buffer length. I'm not sure about px_md_finish() and
hmac_md_finish()..

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center