On Fri, Feb 12, 2021 at 03:21:40PM +0900, Kyotaro Horiguchi wrote: > The v3 drops the changes of the uuid_ossp contrib. I'm not sure the > change of scram_HMAC_final is needed.
Meaning that v3 would fail to compile uuid-ossp. v3 also produces compilation warnings in auth-scram.c. > 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.) Right. These should just use h->result_size(h), and not h->block_size(h). -extern int scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx); +extern int scram_HMAC_final(scram_HMAC_ctx *ctx, uint8 *result); There is no point in this change. You just make back-patching harder while doing nothing about the problem at hand. - if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf) < 0) + if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf, + PG_SHA256_DIGEST_LENGTH) < 0) Here this could just use sizeof(checksumbuf)? This pattern could be used elsewhere as well, like in md5_common.c. > 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. The checksum stuff just relies on PG_CHECKSUM_MAX_LENGTH and there are already static assertions used as sanity checks, so I see little point in adding a new argument that would be just PG_CHECKSUM_MAX_LENGTH. This backup checksum code is already very specific, and it is not intended for uses as generic as the cryptohash functions. With such a change, my guess is that it becomes really easy to miss that pg_checksum_final() has to return the size of the digest result, and not the maximum buffer size allocation. Perhaps one thing this part could do is just to save the digest length in a variable and use it for retval and the third argument of pg_cryptohash_final(), but the impact looks limited. > 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().. I guess that you mean px_hmac_finish() for the second one. The first one is tied to passing down result_size() and down to the cryptohash functoins, meaning that there is no need to take about it more than that IMO. The second one would be tied to the HMAC refactoring. This would be valuable in the case of pgcrypto when building with OpenSSL, meaning that the code would go through the defenses put in place at the PG level. -- Michael
signature.asc
Description: PGP signature
