Re: Incorrect allocation handling for cryptohash functions with OpenSSL

On 07/01/2021 06:15, Michael Paquier wrote:
> 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.

Ah, you're right.

>> 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.

Hmm. Perhaps it would be best to change all the errors in mock
authentication to COMMERROR. It'd be nice to have an accurate error
message in the log, but no need to send it to the client.

>> 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?

+1

- Heikki