Re: scram-sha-256 broken with FIPS and OpenSSL 1.0.2 - Mailing list pgsql-hackers

On Thu, Nov 19, 2020 at 02:05:35PM +0100, Daniel Gustafsson wrote:
> 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:
>
> [... code ...]

We use this style in other places of the code.  Slitting each part can
also be done, if that's easier for the eye.  Personal taste likely ;)

> 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, and OpenSSL visibly want to keep the right
to do extra allocations if necessary within what's set in the context.
This is based on my reads of the upstream code as of
crypto/evp/digest.c and crypto/evp/evp.h.  evp.h includes
env_md_ctx_st for OpenSSL <= 1.0.2, but this has been moved to
evp_local.h, header not installed, in newer versions which refers only
to the internals of the thing, with ossl_typ.h still making EVP_MD_CTX
point to env_md_ctx_st, but it becomes an incomplete type.  On top of
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.
If you want, it is easy to test that with stuff like that in
cryptohashes.c:
+   EVP_MD_CTX  *ctx;
[...]
+   ctx = palloc(sizeof(EVP_MD_CTX));
+
+   EVP_MD_CTX_init(ctx);
+
+   EVP_DigestInit_ex(ctx, EVP_sha256(), NULL);
+   EVP_DigestUpdate(ctx, data, len);
+   EVP_DigestFinal_ex(ctx, buf, 0);

FWIW, this thread has dealt with the problem for pgcrypto:
https://www.postgresql.org/message-id/20160701094159.GA17882@msg.df7cb.de

> +#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_.

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

Indeed, let's do that.

> +/* 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.
--
Michael