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
