Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On Wed, Jan 06, 2021 at 03:58:22PM +0200, Heikki Linnakangas wrote:
> contrib/pgcrypto/internal-sha2.c and
> src/backend/utils/adt/cryptohashfuncs.c: the call to pg_cryptohash_create()
> is missing check for NULL result. With your latest patch, that's OK because
> the subsequent pg_cryptohash_update() calls will fail if passed a NULL
> context. But seems sloppy.

Is it? pg_cryptohash_create() will never return NULL for the backend.

> contrib/pgcrypto/internal.c: all the calls to pg_cryptohash_* functions are
> missing checks for error return codes.

Indeed, these are incorrect, thanks. I'll go fix that separately.

> contrib/uuid-ossp/uuid-ossp.c: uses pg_cryptohash for MD5, but borrows the
> built-in implementation of SHA1 on some platforms. Should we add support for
> SHA1 in pg_cryptohash and use that for consistency?

Yeah, I have sent a separate patch for that:
https://commitfest.postgresql.org/31/2868/
The cleanups produced by this patch are kind of nice.

> src/backend/libpq/auth-scram.c: SHA256 is used in the mock authentication.
> If the pg_cryptohash functions fail, we throw a distinct error "could not
> encode salt" that reveals that it was a mock authentication. I don't think
> this is a big deal in practice, it would be hard for an attacker to induce
> the SHA256 computation to fail, and there are probably better ways to
> distinguish mock authentication from real, like timing attacks. But still.

This maps with the second error in the mock routine, so wouldn't it be
better to change both and back-patch? The last place where this error
message is used is pg_be_scram_build_secret() for the generation of
what's stored in pg_authid. An idea would be to use "out of memory".
That can be faced for any palloc() calls.

> src/include/common/checksum_helper.h: in pg_checksum_raw_context, do we
> still need separate fields for the different sha variants.

Using separate fields looked cleaner to me if it came to debugging,
and the cleanup was rather minimal except if we use more switch/case
to set up the various variables. What about something like the
attached?
--
Michael