Add Fips module checks to provider key operations

[slontis]

For key operations the fips module now checks for approved digests, named curves, safe prime groups, and minimum security strength for keys .
Note that for encryption, signing and exchange operations 112 bits of security strength are required, but lower values are allowed for decryption and verifying for legacy purposes.
These checks are not done by the default providers for backwards compatibility reasons.

Checklist

• documentation is added or updated
• tests are added or updated

[slontis]

This PR has been broken up into separate commits to hopefully make it easier to review.

[richsalz]

So doing these tests is not a FIPS requirement. Are we sure that, for example, they are not invoked in any of these paths?

• When used in TLS, certificate signatures are checked
• And signatures are also verified while building a certificate chain
• When doing OCSP stapling
• In the verify command, and other apps

I think the safest thing to do is wrap all those checks in a new ifdef. Yes, I really dislike adding a new ifdef, but we are concerned that these checks, which are now required for Common Criteria, but not required for FIPS, will end up being invoked through some codepath that we haven't analyzed. Especially so close to the BETA freeze.

(Of course the absolute safest is to not put this into OpenSSL, which only promises FIPS not Common Criteria.)

[t8m]

They might not be required by FIPS 140-2, but they will be effectively required by FIPS 140-3. Also they are good idea anyway as they help the application to be FIPS compliant. Once you use such non-approved key sizes you're out of FIPS compliance. If you care about real FIPS compliance and not just paper compliance you need to use approved key sizes.

[richsalz]

As you say, it is not currently required. I am concerned that it will break some of our deployed customers.

[t8m]

So they do not really care about FIPS compliance?

[t8m]

I mean either they care about FIPS compliance and then they cannot use non-approved key sizes. Or they do not, but then why they would be using FIPS provider.

[richsalz]

We care about FIPS 140-2, not 140-3. If the project wants to do 140-3, then presumably it will discuss this among the OMC and the FIPS project sponsors.

[t8m]

I am sorry for not being clear. FIPS 140-2 requires you to use approved key sizes, it just does not require the software to enforce the requirement. The compliance can be achieved by simply not using non-approved key sizes by the means of the system administrator (the FIPS module operator) diligence. So if the sysadmin uses wrong key sizes, the resulting operation is not FIPS compliant. And it can even invalidate keys if you for example use properly FIPS compliant RSA key to create signature with non-approved hash (which is even sha1 now for signatures) it invalidates the compliance of the RSA key, even if you would like to later use it with an approved hash. So the key size checks are actually very much useful even with FIPS 140-2.

[richsalz]

Please see the archives on the fips-sponsors list. 140-2 says nothing about certificates, for example.

[slontis]

Please see the archives on the fips-sponsors list. 140-2 says nothing about certificates, for example.

If you are signing with SHA1 for example - then you are not fips compliant.

Ticking the fips box, and then allowing non secure operations is just sticking your head in the sand.
If you require operations that are not fips then you should be setting up a separate libctx that can handle these operations.

Ideally any cert or key that is not trusted should also be doing a key check also, there are many rules as to what part of the key needs to be validated (especially with key agreement).

https://csrc.nist.gov/projects/cryptographic-module-validation-program/announcements mentions SP800-131A quite a lot.

If you read https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-4.pdf it does set limits on keysizes, and it also mentions key assurances in section 3.3.

[richsalz]

I am going to stand by what Ashit said on that list. This is moving decisions that were in policy, to being enforced in code, and it's not for 140-2 but for Common Criteria and 140-3. This will break some of our customers.

[t-j-h]

Having an option to have technical enforcement of items which are allowed to be handled by policy (i.e. passed to the user as their responsibility) is a useful thing to do - however it should also be able to be disabled and such a disabling should not be in violation of the security policy. There are actually contexts in which things are permitted to be used without breaking FIPS requirements which depend on application knowledge that sits outside of the FIPS module itself.

Yes - I'm agreeing with both @slontis and @richsalz at the same time - with the modification that it should be able to be configured on or off in the configuration file. That enables meeting both objectives which apply to different situations.

[richsalz]

I can acccept @t-j-h's proposal. I'd add that the default should be settable via the fipsinstall program.

[slontis]

I was thinking the same things in terms of it being optional.. The default should be that the checks are on, My personal opinion is that the security policy would need to state that you are not fips compliant if you disable it - i.e you are allowing non secure operations (which may be required by P11 devices for example) Whilst I understand that it has the potential to break things, I have also seen many instances in production environments where developers think they are fips compliant, and have no idea that they are not even close to being secure…. Another thing to note is that a lot of these restrictions are many years old, which means people remain unaware that they are insecure and will remain so whilst we continue to control this via policy. I am unsure if this option should be in the fips config file that is produced from fipsinstall - to me that is encouraging people to disable the checks. Tim do you have an opinion on this? Cheers, Shane

…

On 3 Sep 2020, at 8:52 am, Tim Hudson ***@***.***> wrote: Having an option to have technical enforcement of items which are allowed to be handled by policy (i.e. passed to the user as their responsibility) is a useful thing to do - however it should also be able to be disabled and such a disabling should not be in violation of the security policy. There are actually contexts in which things are permitted to be used without breaking FIPS requirements which depend on application knowledge that sits outside of the FIPS module itself. Yes - I'm agreeing with both @slontis <https://urldefense.com/v3/__https://github.com/slontis__;!!GqivPVa7Brio!Nj72opLpDJojCRLE9i4KYu9B1kLFxIKap7vvUN6NhavlADNE4K06SkgBzrw_uOivUg$> and @richsalz <https://urldefense.com/v3/__https://github.com/richsalz__;!!GqivPVa7Brio!Nj72opLpDJojCRLE9i4KYu9B1kLFxIKap7vvUN6NhavlADNE4K06SkgBzrz5Q-GjGQ$> at the same time - with the modification that it should be able to be configured on or off in the configuration file. That enables meeting both objectives which apply to different situations. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://urldefense.com/v3/__https://github.com/openssl/openssl/pull/12745*issuecomment-686069302__;Iw!!GqivPVa7Brio!Nj72opLpDJojCRLE9i4KYu9B1kLFxIKap7vvUN6NhavlADNE4K06SkgBzrzhCJ-rjg$>, or unsubscribe <https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AJUSG6S7VJKKKHSHZPNAO53SD3EELANCNFSM4QOXNNCA__;!!GqivPVa7Brio!Nj72opLpDJojCRLE9i4KYu9B1kLFxIKap7vvUN6NhavlADNE4K06SkgBzrxFO9XBEA$>.

[richsalz]

My personal opinion is that the security policy would need to state that you are not fips compliant if you disable it - i.e you are allowing non secure operations (which may be required by P11 devices for example)

I disagree. It should state that compliance is determined via administrative guidance. Just like other guidances we're going to have.

I am unsure if this option should be in the fips config file that is produced from fipsinstall -

Without that, how does someone change from admin-style guidance to code-enforcement? Rewrite their app? Install a differently-built module?

[slontis]

It will be up to whoever writes the security policy to determine what to do with this.
I will make it optional - if people chose to abuse it then that is their choice.
The consequence will be that it will be disabled by people because something doesn't work instead of actually addressing the problem.

[t-j-h]

I see nothing wrong with on-by-default for the checks.

However it defeats the point if you basically add a requirement in the security policy for this setting to always be on - FIPS allows for non-technical enforcement - if it didn't then all modules would be required to enforce.

There are good reasons why this is the case for FIPS but that is a longer conversation that needed for this threat.

Simple summary:
a) add checks
b) checks on by default
c) can be disabled in config file or compile time or fipsinstall time
d) no mandated specific setting of the config file mechanism in the security policy or other referenced documents in the security policy

[richsalz]

@t-j-h's comments in #12745 (comment) meet our needs and also seem to meet the FIPS requirements as well.

I can help with the config file stuff, and wiring it through to the checks if you want.

[slontis]

I can help with the config file stuff, and wiring it through to the checks if you want.

Yes that would be helpful if you could..
I am re-jigging the code now to handle it.
I just need to separate out the dependant evp_tests somehow so they dont fall over if the option is disabled.

[slontis]

@richsalz - I have added a config option 'fips-securitychecks'..

If you look in security_check_fips.c you will see a TODO(3.0) item where the check should be placed..
evp_test.c would also need the check where I put the #ifdef for this also in this PR.

[richsalz]

I mailed you a patch.

[slontis]

I mailed you a patch.

Thanks for that.. I did not quite know how to put the global variable in either, I was interested to see what you would do :)
I have modified it a bit.

[slontis]

rebased to fix commit message..

[t8m]

LGTM, good work!

[openssl-machine]

This pull request is ready to merge

[mattcaswell]

Pushed.