scram-sha-256 broken with FIPS and OpenSSL 1.0.2

On 24 Sep 2020, at 04:53, Michael Paquier <michael@paquier.xyz> wrote:

Enabling FIPS with OpenSSL 1.0.2 causes direct calls to the SHAXXX
routines to fail:
"Low level API call to digest SHA256 forbidden in fips mode"

Confirmed by running 1.0.2s-fips with fips_mode=yes in the alg_section in
openssl.cnf.

This got discussed back in 2018, but I never got back to it:
/messages/by-id/20180911030250.GA27115@paquier.xyz

One thing I did not like back in the past patch was that we did not
handle failures if one of OpenSSL's call failed, but this can easily
be handled by using a trick similar to jsonapi.c to fail hard if that
happens.

It is worth noting that the low-level SHA routines are not recommended
for years in OpenSSL, and that these have been officially marked as
deprecated in 3.0.0. So, while the changes in sha2.h don't make this
stuff back-patchable per the ABI breakage it introduces, switching
sha2_openssl.c to use EVP is a better move in the long term,

Agreed. Commit 5ff4a67f63f [0]/messages/by-id/561274F1.1030000@iki.fi moved contrib/pgcrypto from using low-level
functions for pretty much exactly the same reasons: they are advised against
and break FIPS.

With your patch I can run the SSL tests successfully both with and without
FIPS. I'm in favor of moving to the EVP API.

..even if
that means that SCRAM+FIPS would not work with PG 10~13, so the
attached is something for HEAD, even if this would be possible to do
in older releases as the routines used in the attached are available
in versions of OpenSSL older than 1.0.1.

Thats kind of a shame, but given the low volume of reports to -bugs and
-hackers, the combination SCRAM and FIPS might not be all too common. Since
OpenSSL FIPS is EOL it might also not increase until 3.0.0 comes with FIPS
certification?

If we really want to support it (which would require more evidence of it being
a problem IMO), using the non-OpenSSL sha256 code would be one option I guess?

cheers ./daniel

[0]: /messages/by-id/561274F1.1030000@iki.fi

On 24/09/2020 17:21, Daniel Gustafsson wrote:

If we really want to support it (which would require more evidence of it being
a problem IMO), using the non-OpenSSL sha256 code would be one option I guess?

That would technically work, but wouldn't it make the product as whole
not FIPS compliant? I'm not a FIPS lawyer, but as I understand it the
point of FIPS is that all the crypto code is encapsulated in a certified
module. Having your own SHA-256 implementation would defeat that.

- Heikki

On 24 Sep 2020, at 18:21, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 24/09/2020 17:21, Daniel Gustafsson wrote:

If we really want to support it (which would require more evidence of it being
a problem IMO), using the non-OpenSSL sha256 code would be one option I guess?

That would technically work, but wouldn't it make the product as whole not FIPS compliant? I'm not a FIPS lawyer, but as I understand it the point of FIPS is that all the crypto code is encapsulated in a certified module. Having your own SHA-256 implementation would defeat that.

Doh, of course, I blame a lack of caffeine this afternoon. Having a private
local sha256 implementation using the EVP_* API inside scram-common would
maintain FIPS compliance and ABI compatibility, but would also be rather ugly.

cheers ./daniel

On 2020-09-24 18:21, Heikki Linnakangas wrote:

That would technically work, but wouldn't it make the product as whole
not FIPS compliant? I'm not a FIPS lawyer, but as I understand it the
point of FIPS is that all the crypto code is encapsulated in a certified
module. Having your own SHA-256 implementation would defeat that.

Depends on what one considers to be covered by FIPS. The entire rest of
SCRAM is custom code, so running it on top of the world's greatest
SHA-256 implementation isn't going to make the end product any more
trustworthy.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Sep 24, 2020 at 1:57 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

Depends on what one considers to be covered by FIPS. The entire rest of
SCRAM is custom code, so running it on top of the world's greatest
SHA-256 implementation isn't going to make the end product any more
trustworthy.

I mean, the issue here, as is so often the case, is not what is
actually more secure, but what meets the terms of some security
standard. At least in the US, FIPS 140-2 compliance is a reasonably
common need, so if we can make it easier for people who have that need
to be compliant, they are more likely to use PostgreSQL, which seems
like something that we should want. Our opinions about that standard
do not matter to the users who are legally required to comply with it;
the opinions of their lawyers and auditors do.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 24 Sep 2020, at 21:22, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Sep 24, 2020 at 1:57 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

Depends on what one considers to be covered by FIPS. The entire rest of
SCRAM is custom code, so running it on top of the world's greatest
SHA-256 implementation isn't going to make the end product any more
trustworthy.

I mean, the issue here, as is so often the case, is not what is
actually more secure, but what meets the terms of some security
standard.

Correct, IIUC in order to be FIPS compliant all cryptographic modules used must
be FIPS certified.

At least in the US, FIPS 140-2 compliance is a reasonably
common need, so if we can make it easier for people who have that need
to be compliant, they are more likely to use PostgreSQL, which seems
like something that we should want.

The proposed patch makes SCRAM+FIPS work for 14, question is if we need/want to
try and address v10-13.

cheers ./daniel

On Thu, Sep 24, 2020 at 06:28:25PM +0200, Daniel Gustafsson wrote:

Doh, of course, I blame a lack of caffeine this afternoon. Having a private
local sha256 implementation using the EVP_* API inside scram-common would
maintain FIPS compliance and ABI compatibility, but would also be rather ugly.

Even if we'd try to force our internal implementation of SHA256 on
already-released branches instead of the one of OpenSSL, this would be
an ABI break for compiled modules expected to work on this released
branch as OpenSSL's internal SHA structures don't exactly match with
our own implementation (think just about sizeof() or such).
--
Michael

On Thu, Sep 24, 2020 at 09:44:57PM +0200, Daniel Gustafsson wrote:

On 24 Sep 2020, at 21:22, Robert Haas <robertmhaas@gmail.com> wrote:

I mean, the issue here, as is so often the case, is not what is
actually more secure, but what meets the terms of some security
standard.

Correct, IIUC in order to be FIPS compliant all cryptographic modules used must
be FIPS certified.

/me whitles, thinking about not using src/common/md5.c when building
with OpenSSL to actually get a complain if FIPS gets used.

At least in the US, FIPS 140-2 compliance is a reasonably
common need, so if we can make it easier for people who have that need
to be compliant, they are more likely to use PostgreSQL, which seems
like something that we should want.

The proposed patch makes SCRAM+FIPS work for 14, question is if we need/want to
try and address v10-13.

Unfortunately, I don't have a good answer for that, except for the
answers involving an ABI breakage. FWIW, the only users of those APIs
I can find in the open wild are pgpool, which actually bundles a copy
of the code in src/common/ so it does not matter, and pgbouncer, that
uses a different compatibility layer to make the code compilable:
https://sources.debian.org/src/pgbouncer/1.14.0-1/include/common/postgres_compat.h/?hl=26#L26

But it is not really possible to say if there is any closed code
relying on that, so I'd like to fix that just on HEAD, about which I
guess there would be no objections?
--
Michael

On Fri, Sep 25, 2020 at 12:19:44PM +0900, Michael Paquier wrote:

Even if we'd try to force our internal implementation of SHA256 on
already-released branches instead of the one of OpenSSL, this would be
an ABI break for compiled modules expected to work on this released
branch as OpenSSL's internal SHA structures don't exactly match with
our own implementation (think just about sizeof() or such).

Well, we could as well add one extra SHA API layer pointing to the EVP
structures and APIs with new names, leaving the original ones in
place, and then have SCRAM use the new ones, but I'd rather not go
down that road for the back-branches.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Fri, Sep 25, 2020 at 12:19:44PM +0900, Michael Paquier wrote:

Even if we'd try to force our internal implementation of SHA256 on
already-released branches instead of the one of OpenSSL, this would be
an ABI break for compiled modules expected to work on this released
branch as OpenSSL's internal SHA structures don't exactly match with
our own implementation (think just about sizeof() or such).

Well, we could as well add one extra SHA API layer pointing to the EVP
structures and APIs with new names, leaving the original ones in
place, and then have SCRAM use the new ones, but I'd rather not go
down that road for the back-branches.

Given the tiny number of complaints to date, it seems sufficient to me
to deal with this in HEAD.

regards, tom lane

On 2020-09-24 21:44, Daniel Gustafsson wrote:

On 24 Sep 2020, at 21:22, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Sep 24, 2020 at 1:57 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

Depends on what one considers to be covered by FIPS. The entire rest of
SCRAM is custom code, so running it on top of the world's greatest
SHA-256 implementation isn't going to make the end product any more
trustworthy.

I mean, the issue here, as is so often the case, is not what is
actually more secure, but what meets the terms of some security
standard.

Correct, IIUC in order to be FIPS compliant all cryptographic modules used must
be FIPS certified.

As I read FIPS 140-2, it just specifies what must be true of
cryptographic modules that claim to follow that standard, it doesn't say
that all cryptographic activity in an application or platform must only
use such modules. (Notably, it doesn't even seem to define
"cryptographic".) The latter may well be a requirement of a user or
customer on top of the actual standard. However, again, the SCRAM
implementation would already appear to fail that requirement because it
uses a custom HMAC implementation, and HMAC is listed in FIPS 140-2 as a
covered algorithm.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 2020-09-24 21:44, Daniel Gustafsson wrote:

Correct, IIUC in order to be FIPS compliant all cryptographic modules used must
be FIPS certified.

As I read FIPS 140-2, it just specifies what must be true of
cryptographic modules that claim to follow that standard, it doesn't say
that all cryptographic activity in an application or platform must only
use such modules. (Notably, it doesn't even seem to define
"cryptographic".) The latter may well be a requirement of a user or
customer on top of the actual standard.

Hm ... AFAICT, the intent of the "FIPS mode" in Red Hat's implementation,
and probably other Linux distros, is exactly that thou shalt not use
any non-FIPS-approved crypto code. By your reading above, there would
be no need at all for a kernel-level enforcement switch.

However, again, the SCRAM
implementation would already appear to fail that requirement because it
uses a custom HMAC implementation, and HMAC is listed in FIPS 140-2 as a
covered algorithm.

Ugh. But is there any available FIPS-approved library code that could be
used instead?

regards, tom lane

On Fri, Sep 25, 2020 at 01:36:44AM -0400, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

However, again, the SCRAM
implementation would already appear to fail that requirement because it
uses a custom HMAC implementation, and HMAC is listed in FIPS 140-2 as a
covered algorithm.

Ugh. But is there any available FIPS-approved library code that could be
used instead?

That's a good point, and I think that this falls down to use OpenSSL's
HMAC_* interface for this job when building with OpenSSL:
https://www.openssl.org/docs/man1.1.1/man3/HMAC.html

Worth noting that these have been deprecated in 3.0.0 as per the
rather-recent commit dbde472, where they recommend the use of
EVP_MAC_*() instead.
--
Michael

On Fri, Sep 25, 2020 at 03:56:53PM +0900, Michael Paquier wrote:

On Fri, Sep 25, 2020 at 01:36:44AM -0400, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

However, again, the SCRAM
implementation would already appear to fail that requirement because it
uses a custom HMAC implementation, and HMAC is listed in FIPS 140-2 as a
covered algorithm.

Ugh. But is there any available FIPS-approved library code that could be
used instead?

That's a good point, and I think that this falls down to use OpenSSL's
HMAC_* interface for this job when building with OpenSSL:
https://www.openssl.org/docs/man1.1.1/man3/HMAC.html

Worth noting that these have been deprecated in 3.0.0 as per the
rather-recent commit dbde472, where they recommend the use of
EVP_MAC_*() instead.

Would a FIPS server only be able to talk to a FIPS client, or would our
internal code produce the same output?

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

Bruce,

In my experience, any client is permitted to connect to FIPS140-2 compliant server. I set this up when I worked at SSA, at management’s request.
—
Jay

Sent from my iPad

On Fri, Sep 25, 2020 at 03:38:22PM -0400, John Scalia wrote:

Bruce,

In my experience, any client is permitted to connect to FIPS140-2 compliant server. I set this up when I worked at SSA, at management’s request.

My question is whether the hash output would match if using different
code.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

FIPS only specifies which algorithms are approved for use on it. For instance, MD-5 is NOT approved at all under FIPS. I would say any algorithm should produce the same result regardless of where it is run. BTW, on Redhat servers, the first algorithm listed for use with SSH is MD-5. This causes the sshd daemon to abort when FIPS is enabled and that config file has not been edited. So, you can no longer connect with an SSH client as the daemon isn’t running. Ask me how I know this.

Sent from my iPad

On Fri, Sep 25, 2020 at 12:27:03AM -0400, Tom Lane wrote:

Given the tiny number of complaints to date, it seems sufficient to me
to deal with this in HEAD.

Thanks. I have done more tests with the range of OpenSSL versions we
support on HEAD, and applied this one. I have noticed that the
previous patch forgot two fail-and-abort code paths as of
EVP_DigestInit_ex() and EVP_DigestUpdate().
--
Michael

On Mon, Sep 28, 2020 at 12:55:06PM +0900, Michael Paquier wrote:

Thanks. I have done more tests with the range of OpenSSL versions we
support on HEAD, and applied this one. I have noticed that the
previous patch forgot two fail-and-abort code paths as of
EVP_DigestInit_ex() and EVP_DigestUpdate().

As this got reverted with fe0a1dc because of the lack of correct error
reporting in libpq, I have restarted this work from scratch, and
finished with the set of two patches attached.

0001 is a redesign of the APIs we use for the SHA2 implementations.
The origin of the problem is that we cannot have a control of the
memory context used by OpenSSL to allocate any of the EVP-related
data, so we need to add some routines to be able to allocate and free
the SHA2 contexts, basically. We have too many routines to do the
work now, so I reduced the whole to 5, instead of 12 originally (this
number would become 20 if we'd add the free/alloc routines for each
SHA2 part), giving the following structure:
/* Context Structures for SHA224/256/384/512 */
typedef enum
{
PG_SHA224 = 0,
PG_SHA256,
PG_SHA384,
PG_SHA512
} pg_sha2_type;
typedef struct pg_sha2_ctx
{
pg_sha2_type type;
/* private area used by each SHA2 implementation */
void *data;
} pg_sha2_ctx;
extern pg_sha2_ctx *pg_sha2_create(pg_sha2_type type);
extern int pg_sha2_init(pg_sha2_ctx *ctx);
extern int pg_sha2_update(pg_sha2_ctx *ctx, const uint8 *data, size_t len);
extern int pg_sha2_final(pg_sha2_ctx *ctx, uint8 *dest);
extern void pg_sha2_free(pg_sha2_ctx *ctx);

A huge advantage of this approach is that the keep all the details of
the SHA2 implementations within each file, so we have nothing related
to OpenSSL in sha2.h, which is rather clean. All the internal
structures part of the fallback implementations are also moved into
their own file sha2.c. I have made the choice to limit the number of
ifdef FRONTEND in the files of src/common/ for clarity, meaning that
the callers of those routines can handle errors as they want, in the
frontend and the backend. The areas making use of the SHA2
implementations are SCRAM (libpq and backend) and the checksum
manifests, so this has required some rework of the existing interfaces
to pass down errors correctly, but at the end the changes needed in
libpq and pg_verifybackup are straight-forward.

With 0001 in place, switching the SHA2 implementation of OpenSSL to
use EVP is straight-forward, as the only thing that's actually needed
here is to put in place a callback to clean up the EVP contexts
allocated by OpenSSL. This is rather similar to what we do in
pgcrypto in some ways, but that's actually simpler and I made things
so as we only track down the EVP_MD_CTX members to free on abort.

I'll add that to the next CF for review.
--
Michael