Page MenuHome GnuPG

Fix tests in FIPS mode
Closed, ResolvedPublic

Description

At this moment, libgcrypt tests fail in FIPS mode because they are not updated to match FIPS behavior (mostly secmem and SHA1).

The attached patch resolves the issue and makes the tests passing in FIPS mode.

This is not an issue of high priority, but having tests working in FIPS mode gives more confidence in the functionality of libgcrypt in FIPS mode.

Event Timeline

Thank you for your report.

I'll fix master so that it doesn't fail when FIPS mode is enabled.

Firstly, I'm going to fix the first part (tests/basic.c for checking with gcry_md_test_algo),
then, more fixes for tests/basic.c, and proceed to other parts.

I applied rC297d31294333: tests: Fix messages to STDERR when FIPS mode is enabled.. Please note that your intention to change check_digests is right, but your patch actually didn't; When a MD algo is not supported, gcry_md_test_algo returns != 0 (an error code), and it "continues" to next entry (before the change).

That reminds me that we we should replace libgcrypt's internal debug functions by those from gpgrt. We have a dependency for gpgrt anyway and thus we should avoid code duplication. Sure we will keep the existsing public functions but that is easy given that gpgrt comes with gpgrt_logv since 1.28 which we can make mandatory (currently libgcrypt requires 1.27 (from 2017, with 1.28 is from 2018)

Thank you for checking and for revised patch. I tested your patch and it works fine for the basic test up until this failure with the crcalgo:

basic: algo 316, crcalgo: 3, gcry_md_open failed: Invalid digest algorithm
basic: algo 317, crcalgo: 3, gcry_md_open failed: Invalid digest algorithm

These are GCRY_MD_SHAKE128 and GCRY_MD_SHAKE256, but the md used here is actually GCRY_MD_RMD160 which is hardcoded and not allowed in FIPS.

I used the following patch to workaround this issue:

diff --git a/tests/basic.c b/tests/basic.c
index a2c25766..07bb14aa 100644
--- a/tests/basic.c
+++ b/tests/basic.c
@@ -10503,7 +10503,7 @@ check_one_md (int algo, const char *data, int len, const char *expect, int elen,
 
       if (*data == '!' && !data[1])
 	{
-	  int crcalgo = GCRY_MD_RMD160;
+	  int crcalgo = GCRY_MD_SHA256;
 	  gcry_md_hd_t crc1, crc2;
 	  size_t startlen;
 	  size_t piecelen;

Not sure if it is the only or best solution, but it passes and as far as I see, the test is not dependent on this particular digest algorithm.

That crcalgo can be any digest algorithm and SHA256 seems best option to me.

Update: still ./basic --fips fails (for me), because of GCM (18 errors).
Need to fix T4873: Enable AES GCM in FIPS mode.

With `/etc/gcrypt/fips_enabled/', make check fails by:

FAIL: t-secmem
FAIL: t-mpi-point
FAIL: curves
FAIL: basic
FAIL: keygen
FAIL: pubkey
FAIL: keygrip
FAIL: random
FAIL: dsa-rfc6979
FAIL: t-ed25519
FAIL: t-cv25519
FAIL: t-x448
FAIL: t-ed448
FAIL: basic-disable-all-hwf
FAIL: benchmark
FAIL: bench-slope

I was so far testing with changes on top of our patches.

One of the fixes for the t-secmem was submitted in the initial comment. There are couple of more in Fedora repository disabling the gcm/poly1305 tests in fips mode:

https://src.fedoraproject.org/rpms/libgcrypt/blob/rawhide/f/libgcrypt-1.8.4-tests-fipsmode.patch

There is also the changeset for dsa-rfc6979 and benchmark, which will require some better handling in upstream, because the NIST P-192 is not allowed in FIPS mode:

https://src.fedoraproject.org/rpms/libgcrypt/blob/rawhide/f/libgcrypt-1.7.3-ecc-test-fix.patch

The t-x448 should be skipped in FIPS mode too.

Let me know if you want me to have a look into these patches and propose a upstreamable patch or you want to take care of that.

I went through the patches above + what I suggested in previous comments, tested everything against both upstream and libgcrypt in Fedora in FIPS mode. There were slight differences, some cases were already fixed in master, some needed to upstream some of our changes, but the result is 10 patches working in both FIPS and non-fips mode, hopefully enough annotated. If not, please, ask for clarifications.

Where I am not completely sure is the commit cipher: Do not use of non-approved digests in FIPS mode, which disables non-FIPS approved MD mechanisms (and prevents messing up FIPS state if something tries to use it in non-enforced mode). Having a better look, it might need similar logic as in cipher.c already. The _gcry_md_init() already disables the non-FIPS digests, but md_open does not consult this flags.disabled anywhere, but just blindly calls md_enable(), which jumps into this "trap". It might be enough to check for the flags.disabled in here, but I am really not sure what all consequences is this going to have.

As a start, I applied your patches.

Tested the master on (faked) FIPS and non-FIPS Fedora and I created couple of more changes for master to work in FIPS mode:

FYI I used the following steps to emulate FIPS mode:

echo "# userspace fips" > /etc/system-fips
touch /etc/gcrypt/fips_enabled
make check

but still, I am getting errors in non-fips mode like:

t-secmem: allocation did not fail as expected
FAIL: t-secmem

Reverting 347817438990b7adf22dc71e4fb581e3232f03a7 makes the tests pass for me (in non-FIPS mode), but it was completely not clear to me what was the purpose of that change. Can you check Gniibe?

Sorry, I didn't test for non-FIPS mode when I committed rC347817438990: fips: Fix tests in fips mode..
Tweaking the value for memory allocation is needed for FIPS mode, because it uses some secure memory by DRBG.

I pushed my further change.
Also, applied and pushed your changes.

For tests with FIPS mode enabled, I manually create the file .libgcrypt.so.20.hmac under src/.libs.

There is an entry commented out in src/Makefile.am for the procedure.

looks good to me. Tested now with master 47e425e07995454573e28c13c08229d2f8a75642 and all tests pass for me in and out of FIPS mode as well as in the "soft" one.

I have one more patch set to improve FIPS testing in test/curves.c. In the past, it was basically skipped altogether in FIPS mode. This implements more fine-grained selection of what is being tested. This is the first part.

Additionally, for legal reasons, we have been hobbling brainpool curves from the tarballs before packaging for ages and even though the patents should be expired by now, we were said that their use is still not approved in Fedora [1]. We should be able to go around this by being able to build-time disable brainpool curves, which is the last patch here.

I will probably have to re-confirm with legal, that this approach is fine, but I also wanted to propose the change in here in meantime.

[1] https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/thread/WUQNAB4EPWSJMMVECL2TZGKB5KIDESII/

disable-brainpool.patch is a text of list of patches.
I think the first two could be applied.
@Jakuje Could you please upload them?

Oh, my bad. I probably used wrong git command. Uploaded now the patches themselves:

If a configure switch to disable Brainpool curves will be added, we also need to add a switch to disable NIST curves.

Two third patches are applied to master. (@werner those parts are typo fix and tests improvement, which we agreed to push.)

For unsupported curve removal, if really needed, I think that creating new ticket is better. The removal patch should be kept locally by distro

Thanks. I think we are good here. If we will decide to pursuate the brainpool switch, I will open a new issue.

gniibe removed a project: Restricted Project.