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

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