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

On Wed, Feb 10, 2021 at 01:44:12PM +0900, Kyotaro Horiguchi wrote:
> It seems to me that the above just means the caller must provide a
> digest buffer that fits the use. In the above example digest just must
> be 64 byte. If Coverity complains so, what should do for the
> complaint is to fix the caller to provide a digest buffer of the
> correct size.
>
> Could you show the detailed context where Coverity complained?

FWIW, the community Coverity instance is not complaining here, so
I have no idea what kind of configuration it uses to generate this
report. Saying that, this is just the same idea as cfc40d3 for
base64.c and aef8948 for hex.c where we provide the length of the
result buffer to be able to control any overflow. So that's a safety
belt to avoid a caller to do stupid things where he/she would
overwrite some memory with a buffer allocation with a size lower than
the size of the digest expected in the result generated.

> So it doesn't seem to me the right direction. Even if we are going to
> make pg_cryptohash_final to take the buffer length, it should
> error-out or assert-out if the length is too small rather than copy a
> part of the digest bytes. (In short, it would only be assertion-use.)

Yes, we could be more defensive here, and considering libpq I think
that this had better be an error rather than an assertion to remain on
the safe side. The patch proposed is incomplete on several points:
- cryptohash_openssl.c is not touched, so this patch will fail to
compile with --with-ssl=openssl (or --with-openssl if you want).
- There is nothing actually checked in the final function. As we
already know the size of the result digest, we just need to make sure
that the size of the output is at least the size of the digest, so we
can just add a check based on MD5_DIGEST_LENGTH and such. There is no
need to touch the internal functions of MD5/SHA1/SHA2 for the
non-OpenSSL case. For the OpenSSL case, and looking at digest.c in
the upstream code, we would need a similar check, as
EVP_DigestFinal_ex() would happily overwrite the area if the caller is
not careful (note that the third argument of the function reports the
number of bytes written, *after* the fact).

I don't see much the point to complicate scram_HMAC_final() and
scram_H() here, as well as the manipulations done for SCRAM_KEY_LEN in
scram-common.h.
--
Michael