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
signature.asc
Description: PGP signature
