On 25/12/2020 02:57, Michael Paquier wrote:
> On Mon, Dec 21, 2020 at 04:28:26PM -0500, Robert Haas wrote:
>> TBH, I think there's no point in return an error here at all, because
>> it's totally non-specific. You have no idea what failed, just that
>> something failed. Blech. If we want to check that ctx is non-NULL, we
>> should do that with an Assert(). Complicating the code with error
>> checks that have to be added in multiple places that are far removed
>> from where the actual problem was detected stinks.
>
> You could technically do that, but only for the backend at the cost of
> painting the code of src/common/ with more #ifdef FRONTEND. Even if
> we do that, enforcing an error in the backend could be a problem when
> it comes to some code paths.
Yeah, you would still need to remember to check for the error in
frontend code. Maybe it would still be a good idea, not sure. It would
be a nice backstop, if you forget to check for the error.
I had a quick look at the callers:
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.
contrib/pgcrypto/internal.c: all the calls to pg_cryptohash_* functions
are missing checks for error return codes.
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?
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.
src/include/common/checksum_helper.h: in pg_checksum_raw_context, do we
still need separate fields for the different sha variants.
- Heikki
