Re: Incorrect allocation handling for cryptohash functions with OpenSSL

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