Re: scram-sha-256 broken with FIPS and OpenSSL 1.0.2

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: scram-sha-256 broken with FIPS and OpenSSL 1.0.2
Date: 2020-11-20 22:17:44
Message-ID: E820988A-0E44-42C5-90B2-72B11439551C@yesql.se
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On 20 Nov 2020, at 01:33, Michael Paquier <michael(at)paquier(dot)xyz> wrote:

>> This seems like a complication which brings little benefit since the only real
>> errorcase is OOM in creating the context? The built-in crypto support is
>> designed to never fail, and reading the OpenSSL code the only failure cases are
>> ENGINE initialization (which we don't do) and memory allocation. Did you
>> consider using EVP_MD_CTX_init instead which place the memory allocation
>> responsibility with the caller? Keeping this a void API and leaving the caller
>> to decide on memory allocation would make the API a lot simpler IMHO.
>
> Yes. That's actually the main take and why EVP callers *have* to let
> OpenSSL do the allocation: we cannot know the size of EVP_MD_CTX in
> advance in newer versions,

Yeah, there is that.

> knowing that we still need to deal with the OOM failure handling
> and pass down the error to the callers playing with SHA2, it feels
> like the most consistent API to me for the frontend and the backend.

For the backend I'd prefer an API where the allocation worked like palloc, if
not then it's a straight exit through the giftshop. But if we want an API for
both the frontend and backend, I guess this is what we'll have to do.

>> +#ifndef _PG_CRYPTOHASH_H_
>> +#define _PG_CRYPTOHASH_H_
>> This should probably be CRYPTOHASH_H to be consistent?
>
> cryptohash.h sounds like a header file we could find somewhere else,
> hence the extra PG_.

Ok, then at least I think we should use PG_CRYPTOHASH_H to be more consistent
with the tree, and since leading underscores in C are problematic spec-wise.

>> +/* Include internals of each implementation here */
>> +#include "sha2.c"
>> Do we really want to implement this by including a .c file? Are there no other
>> options you see?
>
> That was the least intrusive option I could figure out. Two other
> options I have thought about:
> - Compile those fallback implementations independently and publish the
> internals in a separate header, but nobody is going to need that if we
> have a generic entry point.
> - Include the internals of each implementation in cryptohash.c, but
> this bloats the file with more implementations added (HMAC and MD5
> still need to be added on top of SHA2), and it messes up with the
> existing copyright entries.
> So splitting and just including them sounds like the cleanest option
> of the set.

Personally I think the first option of using an internal header seems cleaner,
but MMV so I'll leave it to others to weigh in too.

cheers ./daniel

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2020-11-20 22:20:43 Re: [HACKERS] Custom compression methods
Previous Message Robert Haas 2020-11-20 22:16:55 Re: xid wraparound danger due to INDEX_CLEANUP false