At Thu, 11 Feb 2021 19:55:45 -0300, Ranier Vilela <ranier...@gmail.com> wrote in > Em qui., 11 de fev. de 2021 às 09:47, Michael Paquier <mich...@paquier.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
