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

> On 13 Nov 2020, at 04:14, Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> I got to think more about this stuff and attached is a new patch set
> that redesigns the generic interface used for the crypto hash
> functions

I've spent some time on this, and the gist of the patchset is clearly something
we want to do: move to the EVP API and ensure that we don't drop any contexts.

IIUC, this patchset moves the allocation of the context inside the API rather
than having the caller be responsible for providing the memory (and thus be
able to use the stack). This in turn changes the API to returning int rather
than being void.

As an effect of this we get the below types of statements that aren't easy on
the eyes:

+ /*
+ * Calculate ClientSignature. Note that we don't log directly a failure
+ * here even when processing the calculations as this could involve a mock
+ * authentication.
+ */
+ if (scram_HMAC_init(&ctx, state->StoredKey, SCRAM_KEY_LEN) < 0 ||
+ scram_HMAC_update(&ctx,
+ state->client_first_message_bare,
+ strlen(state->client_first_message_bare)) < 0 ||
+ scram_HMAC_update(&ctx, ",", 1) < 0 ||
+ scram_HMAC_update(&ctx,
+ state->server_first_message,
+ strlen(state->server_first_message)) < 0 ||
+ scram_HMAC_update(&ctx, ",", 1) < 0 ||
+ scram_HMAC_update(&ctx,
+ state->client_final_message_without_proof,
+ strlen(state->client_final_message_without_proof)) < 0 ||
+ scram_HMAC_final(ClientSignature, &ctx) < 0)
+ elog(ERROR, "could not calculate client signature");

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.

Are there other error cases and/or errorpaths you're considering that I'm
missing here? If it's just OOM on allocation it seems a high price to pay.

+#ifndef _PG_CRYPTOHASH_H_
+#define _PG_CRYPTOHASH_H_
This should probably be CRYPTOHASH_H to be consistent?

+ status = EVP_DigestInit_ex((EVP_MD_CTX *) ctx->data,
+ EVP_sha224(), NULL);
Since we always use the default implementation and never select an ENGINE, why
not use EVP_DigestInit instead?

+/* 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?

> I think that we could also rename the existing cryptohashes.c to
> crypohashfuncs.c to be more consistent, but I have left that out for now.

+1. That can be done as a separate patch submission though as it's unrelated.

cheers ./daniel