Page MenuHome GnuPG

Implement service indicators
Closed, ResolvedPublic

Description

The FIPS 140-3 requires service indicators. There are couple of notes from previous email discussion to keep them in one place:

The last paragraph of ISO 19790:2012 section 7.2.4.2 states:

All services shall [02.24] provide an indicator when the service utilises an approved cryptographic algorithm, security function or process in an approved manner and those services

or processes specified in 7.4.3

This means our libraries need to grow an API or provide some additional information via contexts or similar in order for an application to be able to query this indicator. This can't be just a Security Policy description because ISO 24759:2017 section 6.2.4.2 states:

TE02.24.02: The tester shall execute all services and verify that the indicator provides an unambiguous indication of whether the service utilizes an approved cryptographic algorithm,

security function or process in an approved manner or not.

The indicator can't be just a marker over an algorithm either, because it needs to show different values based on whether the algorithm parameters cause the algorithm to run in approved or non-approved mode (ie keys outside of valid range for RSA means RSA is being used in non-approved mode ...)

More comments from Simo:

[...] in short we have two ways to go about it:

  1. provide a context to every "service" we provide (services can be as simple as "AES encrypt operation" or as complicated as "apply GPG signature to a document") and then make an API available to consult this context and find if all algorithms used to implement the service were FIPS approved. This includes also that key sizes or other parameters are within FIPS allowed parameters. So for example a service that provides RSA encryption, should return an indicator of "approved" if key sizes are > 2048 bit, and "not approved" if key sizes are <2048.

This would require huge changes in libgcrypt so it is probably not a way to go.

  1. Provide an "implicit" indicator. An implicit indicator can be the return code of a function. In this case a successful outcome would require that only approved FIPS algorithms can be used to implement the service. IE any non approved algorithm or key size (or other parameter) would have to be blocked when the library is operating in FIPS mode.

As far as I understand this here, this quite much what is done in libgcrypt now. But it might need to review the algorithms and key sizes to make sure the rules are enforced everywhere.

Event Timeline

werner triaged this task as High priority.Jun 28 2021, 1:27 PM
werner added subscribers: werner, gniibe.

Just FYI, NSS offers following API:

/* FIPS indicator functions. Some operations are physically allowed, but
 * are against the NSS FIPS security policy. This is because sometimes NSS
 * functions are used in non-security contexts. You can call these functions
 * to determine if you are operating inside or outside the the current vendor's
 * FIPS Security Policy for NSS. NOTE: if the current version of NSS is not
 * actually FIPS certified, then these functions will always return PR_FALSE */

/* This function tells if if the last single shot operation on the slot
 * was inside or outside the FIPS security policy */
PRBool PK11_SlotGetLastFIPSStatus(PK11SlotInfo *slot);
/* This tells you if the current operation is within the FIPS security policy. If
 * you have called finalize on the context, it tells you if the last operation
 * was within the FIPS security policy */
PRBool PK11_ContextGetFIPSStatus(PK11Context *context);
/* This tells you if the requested object was created in accordance to the
 * NSS FIPS security policy. */
PRBool PK11_ObjectGetFIPSStatus(PK11ObjectType objType, void *objSpec);

I went through the OpenSSL drafts. The module boundary in OpenSSL will be separate fips.so object and only non-deprecated functions of OpenSSL 3.0 will be FIPS compliant. There is a global state, that will allow only approved algorithms and modes and there will be API to query the FIPS mode status using OSSL_PARAM_get* functions, but we still have some unknowns so I hope we will know more on the next meeting.

Just FYI, see also how GnuTLS has proposed to implement the service indicator:
https://gitlab.com/gnutls/gnutls/-/merge_requests/1465

Tsss, requires to allow JS for Google.

werner lowered the priority of this task from High to Normal.Oct 25 2021, 11:20 AM

We are currently using "implict" service indicators but eventually we may change Libgcrypt to support explicit indicators.

Implicit indicators mean that we need to go through the all algorithms and verify that they work if they have approved key sizes/parameters and do not work when they do not.

I started with RSA and asymmetric crypto. In FIPS we are required not only to generate at least 2k keys, but this check should land also when creating signatures, verifying them, encrypting and decrypting data. Right now, libgcrypt happily does this with 1k keys and smaller, which are used all over the basic tests. FIPS aside, I wanted to introduce similar check also for non-FIPS use to require at least 1k bits for key operations (similarly as in the key generation), but the 1k keys are used everywhere so it would require a lot of changes:

  • Do we want libgcrypt to allow using 1k keys? If so, we might just implement the checks in the new "FISP approved" sign_digest API. If you have concerns of using smaller keys, it might make sense to implement it in the low level API already.
  • The pubkey API does not respect the "disabled" flags of the algorithms, which prevents us disabling algorithms in FIPS mode (similarly as we do with digests and others).
  • A lot of other tests in pkcs1v2 and pubkey still use some 1k keys which should no longer work in FIPS mode -- this needs some more work -- probably bringing in larger keys and checking the 1k keys fail in FIPS mode.

The following patch implements first steps for disabling pubkey algorithms (and small RSA key sizes) in FIPS mode:

Applied parts except part 2.
The part 3 are modified version, so that memory can be released correctly.

PART 2:
In the target work of ours, DSA is not supported in FIPS mode, but DSA algorithm itself is supported in FIPS 140-3. So, I don't disable DSA in FIPS mode, and I did something for DSA.
<-- I'm not sure this is good or not.

Thank you for merging the important parts of the patches and implementing similar stuff for DSA. You are right that DSA is supported in the 140-3 specs so it is fine to keep it enabled with the keylength constraints.

(I thought I posted the below patch already on Friday, but it looks like it did not go through. So I rebased it on top of current changes and added one fixup for RSA benchmark)

This is a patch updating the basic test to check (most of) symmetric algorithms against FIPS mode more explicitly. There is no code change in libgcrypt as this looks like working as expected.

I need to figure out how the non-FIPS cipher-modes should behave, which will be a follow-up to this.

Applied and pushed symmetric algo for basic.

I'll fix regressions: failures of pubkey and pkcs1v2.

With just implicit indicators, we would have to block all non-approved cipher modes and kdfs including the OCB mode and skcrypt, which would probably make gnupg2 unusable in FIPS mode, which is not our intention.

This means we will have to implement for these operations explicit service indicators with a new api, for example bool FIPS_service_indicator(const char *service_name);, which FIPS-aware application could call to check if the service is FIPS approved. The service name would be something like AES-OCB or AES-CBC and return value True if the mode is known and approved.

Thinking about the API further, it might also make sense to use existing gcry_control() with a new control and arguments might define algorithms, and modes mode dynamically for example gcry_control (GCRYCTL_FIPS_SERVICE_INDICATOR, GCRY_CIPHER_AES, GCRY_CIPHER_MODE_OCB) (naming not fixed yet). I would like to avoid extending this API to all the ciphers/algorithms, because it would make it harder for applications to claim their compliance as they would have to check for the indicator for each of key operation (which is not needed with implicit indicators).

I also wanted to bring up the topic of DSA and 3DES. They are still approved in the current documents, but 3DES is listed as approved only up to 2023 and DSA is not listed in the latest draft of FIPS 186-5 draft [1]. We do not plan to certify either of these so the question follows if they can be disabled in next libgcrypt release or you are still interested in these algorithms in FIPS mode and we should patch out these in our package.

[1] https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-5-draft.pdf

We could use a new mode #define GCRY_GET_CONFIG_FIPS 1 with gcry_get_config:

/* With a MODE of 0 return a malloced string with configured features.
 * In that case a WHAT of NULL returns everything in the same way
 * GCRYCTL_PRINT_CONFIG would do.  With a specific WHAT string only
 * the requested feature is returned (w/o the trailing LF.  On error
 * NULL is returned.  */
char *
_gcry_get_config (int mode, const char *what)
{
[...]
  if (mode)
    {
      gpg_err_set_errno (EINVAL);
      return NULL;
    }

I do not like the idea of using the get_config interface for this. It should be easily usable by applications to check for single cipher/mode so int/bool return values would be preferred against the string ones (which are now used in the get_config). I am not sure if getting all the configuration in one string blob would be any use (except for some auditing) either.

As we talked earlier this week, I put together patch to utilize the gcry_control API with few usages of this API in the tests (the patch 4 in the attached series). It might still need some love so comments/suggestions/improvements welcomed.

The rest of the patches are mostly related to DSA and 3DES disabling in FIPS mode (I am still not sure if we agreed on doing so or not) and tests adjustments.

Applied the part 4, the indicator patch.

For other parts, let me evaluate.

Applied the part 3, the 3DES is no-FIPS patch.

Also, applied the part 2, improving basic.c.

For the part 1, I created: T5710: FIPS: disable DSA for FIPS

So, please focus on the service indicator API, here.

It turns out that the asymmetric key operations are not yet properly enforced with the .disabled flag. While the other key crypto usually has some "open" api, where this can be simply captured, the pubkey API has several entry points and the "test_algo" is not enough to check for disabled key types.

The attached patch adds the check for disabled flag into the entry points for public key operations (both new and old) and slightly modified the test to make sure these code paths are properly executed, not stopping on the "test_algo" stage..

Sorry for resurrecting the done task, but I got a message from @pmgdeb who noticed there is mismatch between parenthesis in the --with-fips-module-version help string. The attached patch fixes the issue and add proper help text.

The previous comment should have come to the T5600. Sorry for the noise.

During one last reviews we noticed that the KDFs are not yet handled in the FIPS indicators API nor the non-allowed are blocked, which would most likely be a problem for FIPS. The FIPS allows only PBKDF2 so the others should not be allowed or should be flagged with the explicit indicator similarly as the symmetric keys and modes (to keep gnupg working with least effort).

Even though the enums gcry_kdf_algos and gcry_cipher_algos are not overlapping, I am not sure if it is guaranteed that they will not do in the future so we would probably need an extension of the gcry_control(GCRYCTL_FIPS_SERVICE_INDICATOR) with some additional parameter or introducing different constants for ciphers and kdf, for example GCRYCTL_FIPS_SERVICE_INDICATOR_CIPHER and GCRYCTL_FIPS_SERVICE_INDICATOR_KDF, as well as modification of the internal function _gcry_fips_indicator(). Do you have some preferences or strong opinions for/against either of the suggestions before I will start drafting a patch?

Yes, we should introduce an INDICATOR_KDF thing.

This is my draft for the FIPS indicator KDF. I think we do not need to keep the original GCRYCTL_FIPS_SERVICE_INDICATOR if we replace it also in the tests. This will also need some tests and documentation update.

I went through the documentation related to FIPS and updated some wording to match reality. It will probably require still some more work.

Thank you, applied.
Also, add another change.

I'm not completely sure but it might be convenient to mark HMAC keys with lengths less that 112 as non-approved in FIPS mode for both generation and verification. It could be easily implemented by adding a check using cipher/mac-hmac.c:hmac_get_keylen() or at the algo level. What do you think?

See also table 9 in HMAC.

This is my draft for the FIPS indicator KDF. I think we do not need to keep the original GCRYCTL_FIPS_SERVICE_INDICATOR if we replace it also in the tests. This will also need some tests and documentation update.

Obviously, in this patch it should say:

-  enum gcry_cipher_algos alg = va_arg (arg_ptr, enum gcry_kdf_algos);
+  enum gcry_kdf_algos alg = va_arg (arg_ptr, enum gcry_kdf_algos);

I thought I posted the updated patch, but obviously did not. But this was already saved by @gniibe with d0db6a5abf7b8cc5637de5a080a7ed986e3ff63f

The similar thing for the docs, it still contains the TODOs, which need to be addressed before the release (sorry for not mentioning it explicitly).

@pmgdeb about the HMAC, I think this should be implemented as implicit fips indicator not allowing such small keys in FIPS mode, unless we need them for some good reason. Unfortunately, I am right now sick at home unable to work and test a proper change, but I would be happy to review your proposal.

There is also one chunk living in my tree that I did not manage to post earlier, which fixes up the 3204c3827e9840915af2b6cbf603f3cf51664568 and unbreaks the make dist:

diff --git a/m4/Makefile.am b/m4/Makefile.am
index 9b953174..c33f1009 100644
--- a/m4/Makefile.am
+++ b/m4/Makefile.am
@@ -1,2 +1,2 @@
-EXTRA_DIST = libtool.m4 socklen.m4 sys_socket_h.m4 noexecstack.m4
+EXTRA_DIST = libtool.m4 socklen.m4 noexecstack.m4
 EXTRA_DIST += gpg-error.m4

@pmgdeb : IIUC, what we need is:

diff --git a/cipher/md.c b/cipher/md.c
index 34336b5c..4f4fc9bf 100644
--- a/cipher/md.c
+++ b/cipher/md.c
@@ -903,6 +903,9 @@ prepare_macpads (gcry_md_hd_t a, const unsigned char *key, size_t keylen)
 {
   GcryDigestEntry *r;
 
+  if (fips_mode () && keylen < 14)
+    return GPG_ERR_INV_VALUE;
+
   if (!a->ctx->list)
     return GPG_ERR_DIGEST_ALGO; /* Might happen if no algo is enabled.  */

And we need to fix selftest for shorter keys.

Pushed the change in rC76aad97dd312: fips: Reject shorter key for HMAC in FIPS mode..

KDF tests are also needed to change, because pbkdf2 and scrypt uses HMAC internally.

Hi, @gniibe and @Jakuje. That fulfills the requirement and all the regression tests pass in FIPS mode. Thanks!

AFAICS, the last commit removes some tests. We should never remove a test just because FIPS does not allow it. The old tests need to be run in non-fips mode.

@werner Those removed tests are selftests which are only invoked by FIPS mode for its requirement of selftests.

Sorry, it's my misunderstanding.
_gcry_fips_run_selftest can be run by GCRYCTL_SELFTEST.
I was confused by the function name. Perhaps, it is good to change the name of function to _gcry_run_selftest.

Anyway, I'll fix.

Thanks. Looks good to me.

I am finally better so I did one more review of the documentation and added/updated many details that I missed in my initial review. Not sure how to make sure the syntax is correct in all cases for the texi files though.

Additionally, the 11ade08efbfbc36dbf3571f1026946269950bc40 added new file, which is now missing from the tarball. The patch is also attached.

Thank you, applied both of two patches.

gniibe claimed this task.
gniibe removed a project: Testing.