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-30 12:43:24
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

> On 24 Nov 2020, at 11:52, Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> I got to look at your suggestion, and finished with the attached which
> is pretty close my previous set, except that MSVC scripts as well as
> the header includes needed a slight refresh.

Looks good, nothing major sticks out. I'm not excited about all the
allocations needed here now, but it seems the only optipn.

> Please note that the OpenSSL docs tell that EVP_DigestInit() is
> obsolete and that applications should just use EVP_DigestInit_ex(), so
> I have kept the original:

Fair enough.

> What do you think?

A few comments in no particular order:

+ if (scram_HMAC_init(&ctx, state->ServerKey, SCRAM_KEY_LEN) < 0 ||
+ scram_HMAC_update(&ctx,
+ state->client_first_message_bare,
+ scram_HMAC_final(ServerSignature, &ctx) < 0)
+ {
+ elog(ERROR, "could not calculate server signature");
+ }
Some of these long if-statements omit the {} around the elog, while some (like
this one) don't. I think it makes sense from a readability POV to use the

+ * pg_cryptohash_create
+ *
+ * Allocate a hash context. Returns NULL on failure.
This comment should perhaps be amended to specify that it returns NULL on
failure in the frontend, in the backend it won't return on error.

+ case PG_SHA224:
+ pg_sha224_update((pg_sha224_ctx *) ctx->data, data, len);
+ break;
+ case PG_SHA256:
+ pg_sha256_update((pg_sha256_ctx *) ctx->data, data, len);
+ break;
Would codepaths like these become more readable if pg_cryptohash_ctx used a
union with shaxxx_ctx's rather than a void pointer for the data part?

+ /* Ditto for EVP contexts */
+ while (ResourceArrayGetAny(&(owner->evparr), &foundres))
+ {
+ if (isCommit)
+ PrintEVPLeakWarning(foundres);
+ EVP_MD_CTX_destroy((EVP_MD_CTX *) DatumGetPointer(foundres));
+ ResourceOwnerForgetEVP(owner, foundres);
+ }
Since the cryptohash support is now generalized behind an abstraction layer,
wouldn't it make sense to roll the resource ownership there as well kind of
like how JIT is handled? It would make it easier to implement TLS backend
support, and we won't have to inject OpenSSL headers here.

cheers ./daniel

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Fabien COELHO 2020-11-30 12:45:16 Re: pgbench - test whether a variable exists
Previous Message Fabien COELHO 2020-11-30 12:40:23 Re: pgbench - test whether a variable exists