Change the default key generation type for DH and DSA #13228

mattcaswell · 2020-10-23T14:21:15Z

Previously both DH and DSA were using FIPS186-4 for all key generation for DH and DSA in both the default and FIPS providers.

This is a little too restrictive for general purpose use and is a significant breaking change (for example it introduces restrictions on the sizes keys can be). AFAIK there was no OTC/OMC decision to make this breaking change.

This PR changes things so that by default in the default provider, DH keys swap to DH_PARAMGEN_TYPE_GENERATOR (i.e. PKCS3) and DHX keys swap to DH_PARAMGEN_TYPE_FIPS_186_2.

For DSA we swap to DSA_PARAMGEN_TYPE_FIPS_186_2 in the default provider.

The defaults are unchanged for the FIPS provider.

This was discovered by the tests being introduced in #13138 and is required for those tests to pass.

… module The documentation claimed this was already the default but it wasn't. This was causing the dhparam application to change behaviour when compared to 1.1.1

Inside the FIPS module we continue to use FIPS186-4. We prefer FIPS186-2 in the default provider for backwards compatibility reasons.

levitte · 2020-10-23T14:27:54Z

providers/implementations/keymgmt/build.info

 $EC_GOAL=../../libimplementations.a
 $ECX_GOAL=../../libimplementations.a
 $KDF_GOAL=../../libimplementations.a

 IF[{- !$disabled{dh} -}]
-  SOURCE[$DH_GOAL]=dh_kmgmt.c
+  SOURCE[../../libfips.a]=dh_kmgmt.c
+  SOURCE[../../libnonfips.a]=dh_kmgmt.c


The idea of libimplementations.a is going more and more to waste...

providers/implementations/keymgmt/dh_kmgmt.c

…he FIPS module

mattcaswell · 2020-10-23T16:22:52Z

Fixup pushed. Please take another look.

kroeckx · 2020-10-23T16:56:20Z

This PR changes things so that by default in the default provider, DH keys swap to DH_PARAMGEN_TYPE_GENERATOR (i.e. PKCS3) and DHX keys swap to DH_PARAMGEN_TYPE_FIPS_186_2. For DSA we swap to DSA_PARAMGEN_TYPE_FIPS_186_2 in the default provider.

Why would we use the obsolete FIPS 186-2? Is there a reason not to just use 186-4?

mattcaswell · 2020-10-23T18:34:50Z

Why would we use the obsolete FIPS 186-2? Is there a reason not to
just use 186-4?

Because its a breaking change...its way more restrictive about key lengths etc that you are allowed, e.g. see:

openssl/crypto/ffc/ffc_params_generate.c

Lines 67 to 91 in d1fb6b4

    
           static int ffc_validate_LN(size_t L, size_t N, int type, int verify) 
        
           { 
        
               if (type == FFC_PARAM_TYPE_DH) { 
        
                   /* Allow legacy 1024/160 in non fips mode */ 
        
                   if (L == 1024 && N == 160) 
        
                       return 80; 
        
                   /* Valid DH L,N parameters from SP800-56Ar3 5.5.1 Table 1 */ 
        
                   if (L == 2048 && (N == 224 || N == 256)) 
        
                       return 112; 
        
           # ifndef OPENSSL_NO_DH 
        
                   DHerr(0, DH_R_BAD_FFC_PARAMETERS); 
        
           # endif 
        
               } else if (type == FFC_PARAM_TYPE_DSA) { 
        
                   if (L == 1024 && N == 160) 
        
                       return 80; 
        
                   if (L == 2048 && (N == 224 || N == 256)) 
        
                       return 112; 
        
                   if (L == 3072 && N == 256) 
        
                       return 128; 
        
           # ifndef OPENSSL_NO_DSA 
        
                   DSAerr(0, DSA_R_BAD_FFC_PARAMETERS); 
        
           # endif 
        
               } 
        
               return 0; 
        
           }

We could choose to relax those restrictions in the default provider. Or we could choose to have the breaking change - but that requires an OMC vote.

t8m

LGTM

openssl-machine · 2020-10-27T13:00:14Z

This pull request is ready to merge

slontis

Whilst I understand why you are doing this for backwards compatability- I think it is setting a really bad example if it generates params using a dead legacy algorithm (especially for key sizes larger than 2048).
Personally I think it should be using a named group (it if can) based on the size, and only use fips_1862 if that does not work..

mattcaswell · 2020-10-30T09:36:41Z

Whilst I understand why you are doing this for backwards compatability- I think it is setting a really bad example if it generates params using a dead legacy algorithm (especially for key sizes larger than 2048).
Personally I think it should be using a named group (it if can) based on the size, and only use fips_1862 if that does not work..

I somehow suspected you might feel this way. I think this PR is worthy of an OTC discussion.

mattcaswell · 2020-10-30T13:51:14Z

For input into the OTC discussion, these are the things that I know of that break or change behaviour compared to 1.1.1 by having the default generation type be FIPS 186-4:

EVP impacts:

The only key size that can be generated is 2048 bits
In theory a key size of 1024 would also be ok, but the default q bits size is 224 which is not allowed in conjunction with a p bit size of 1024
The default generator changes from 2 to something random
EVP_PKEY_CTX_set_dh_paramgen_generator() no longer works (it returns success but doesn't do anything)
We no longer end up with a "safe" prime

dhparam app impacts:

The dhparam default generator will change (from 2 to something random)
The "-2" and "-5" options cannot be made to work, unless we change the generation type if they are supplied
Only 2048 and 1024 sizes can be generated, unless we decide to change the generation type if something else is selected
We no longer end up with a safe prime

t8m · 2020-10-30T15:51:42Z

IMO there is nothing wrong with DH_PARAMGEN_TYPE_GENERATOR (except of course that it is not FIPS compliant). However one useful thing would be to have a parameter generator that would simply select one of the known safe primes that has the nearest size to the requested one. That could IMO be even the default for the FIPS module for the DH key type.

kroeckx · 2020-10-30T18:38:17Z

Whilst I understand why you are doing this for backwards compatability- I think it is setting a really bad example if it generates params using a dead legacy algorithm (especially for key sizes larger than 2048). Personally I think it should be using a named group (it if can) based on the size, and only use fips_1862 if that does not work..

Are you suggesting that the "generate" function should return existing known curves by default? It would at least be unexpected to me that it didn't generate new parameters for me. I think we all agree that using a named group is what people should use. But weakdh.org / logjam encouraged people to use there own parameters. There are probably other guides that suggest that too. So I think we should do what we can to encourage people to use the named groups, and the changes in #13233 at least try encourage to use them. I wonder if we should publish some document on what we recommend people to do. There might be people who really wish to generate their own safe prime, and I think we should have support for that. I currently don't see what is wrong with using safe primes, which is what the named groups all use. And I think that should be the default. I have not looked at what fips 186-2 does different to 186-4, but it's been replaced for some reason. So I would like to get rid of 186-2, but maybe 3.0 is not realistic. We have actually marked using dsa parameters as deprecated in the dhparam manpage. Did we mark parts of the API that can be used to generate it also as deprecated? If we did, we can just remove the 186-2 code in 4.0.

slontis · 2020-10-31T00:25:00Z

Another thing to keep in mind..
There is more than one issue with non named groups related to key validation.

key validation - This is really easy to get wrong with generated parameters since they have parameters that are not stored as part of the ASN1 that need to be set before validation. Named groups don't have this problem.
TLS doesnt transmit Q. So if you do need to validate you need to somehow know what Q is. Named Groups solve this problem also.

FIPS 186-2 was still present in FIPS 186-4 for backwards compatability for quite a long time(only the validation bit , not the generation part).
It is now completely removed. (I think it only used SHA1).

…he FIPS module

mattcaswell · 2020-11-06T17:07:36Z

I added a couple of fixups to bring this into line with the proposal currently being voted on by OTC.

mattcaswell · 2020-11-09T11:07:28Z

The travis failure does not seem relevant.

@t8m - can you reconfirm your approval of this?

I'm also still waiting on the conclusion of the OTC vote before I can merge this.

t8m · 2020-11-09T12:09:09Z

Yes, still approved.

mattcaswell · 2020-11-18T14:51:16Z

The vote for this passed, so I'm lifting the OTC hold.

mattcaswell · 2020-11-18T14:52:36Z

Vote results:

https://git.openssl.org/?p=otc.git;a=blob;f=votes.txt;h=b04141fbe1fe6a6ba678c23e4060854d408bd8dc;hb=refs/heads/master#l66

mattcaswell · 2020-11-18T14:53:43Z

The travis failure is not relevant.

mattcaswell · 2020-11-18T15:09:59Z

Pushed. Thanks.

… module The documentation claimed this was already the default but it wasn't. This was causing the dhparam application to change behaviour when compared to 1.1.1 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #13228)

Inside the FIPS module we continue to use FIPS186-4. We prefer FIPS186-2 in the default provider for backwards compatibility reasons. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #13228)

mattcaswell added 2 commits October 23, 2020 14:57

Swap to DH_PARAMGEN_TYPE_GENERATOR as the default outside of the FIPS…

8f54f85

… module The documentation claimed this was already the default but it wasn't. This was causing the dhparam application to change behaviour when compared to 1.1.1

Swap to FIPS186-2 DSA generation outside of the FIPS module

18d92c6

Inside the FIPS module we continue to use FIPS186-4. We prefer FIPS186-2 in the default provider for backwards compatibility reasons.

mattcaswell added the branch: master Merge to master branch label Oct 23, 2020

mattcaswell added this to the 3.0.0 beta1 milestone Oct 23, 2020

levitte reviewed Oct 23, 2020

View reviewed changes

t8m reviewed Oct 23, 2020

View reviewed changes

providers/implementations/keymgmt/dh_kmgmt.c Show resolved Hide resolved

This was referenced Oct 23, 2020

Convert dhparam so that it no longer needs low level APIs #13231

Closed

Deprecate DH harder #13138

Closed

fixup! Swap to DH_PARAMGEN_TYPE_GENERATOR as the default outside of t…

aa49c19

…he FIPS module

t8m approved these changes Oct 26, 2020

View reviewed changes

t8m added the approval: done This pull request has the required number of approvals label Oct 26, 2020

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Oct 27, 2020

mattcaswell mentioned this pull request Oct 29, 2020

dhparam app only allows generation of 2048 bit keys #13271

Closed

slontis reviewed Oct 30, 2020

View reviewed changes

mattcaswell added the hold: need otc decision The OTC needs to make a decision label Oct 30, 2020

mattcaswell added 2 commits November 6, 2020 17:05

fixup! Swap to FIPS186-2 DSA generation outside of the FIPS module

d7a43ba

fixup! Swap to DH_PARAMGEN_TYPE_GENERATOR as the default outside of t…

ed89860

…he FIPS module

mattcaswell added approval: review pending This pull request needs review by a committer and removed approval: ready to merge The 24 hour grace period has passed, ready to merge labels Nov 9, 2020

romen added the triaged: OTC evaluated This issue/pr was triaged by OTC label Nov 10, 2020

mattcaswell added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed hold: need otc decision The OTC needs to make a decision approval: review pending This pull request needs review by a committer labels Nov 18, 2020

mattcaswell closed this Nov 18, 2020

mattcaswell mentioned this pull request Nov 23, 2020

dhparam generates DSA parameters by default #13148

Closed

paulnelsontx added this to Ready to merge in 3.0.0 estimator Dec 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the default key generation type for DH and DSA #13228

Change the default key generation type for DH and DSA #13228

mattcaswell commented Oct 23, 2020

levitte Oct 23, 2020

mattcaswell commented Oct 23, 2020

kroeckx commented Oct 23, 2020 via email

mattcaswell commented Oct 23, 2020

t8m left a comment

openssl-machine commented Oct 27, 2020

slontis left a comment

mattcaswell commented Oct 30, 2020

mattcaswell commented Oct 30, 2020

t8m commented Oct 30, 2020

kroeckx commented Oct 30, 2020 via email

slontis commented Oct 31, 2020

mattcaswell commented Nov 6, 2020

mattcaswell commented Nov 9, 2020

t8m commented Nov 9, 2020

mattcaswell commented Nov 18, 2020

mattcaswell commented Nov 18, 2020

mattcaswell commented Nov 18, 2020

mattcaswell commented Nov 18, 2020

Change the default key generation type for DH and DSA #13228

Change the default key generation type for DH and DSA #13228

Conversation

mattcaswell commented Oct 23, 2020

levitte Oct 23, 2020

Choose a reason for hiding this comment

mattcaswell commented Oct 23, 2020

kroeckx commented Oct 23, 2020 via email

mattcaswell commented Oct 23, 2020

t8m left a comment

Choose a reason for hiding this comment

openssl-machine commented Oct 27, 2020

slontis left a comment

Choose a reason for hiding this comment

mattcaswell commented Oct 30, 2020

mattcaswell commented Oct 30, 2020

t8m commented Oct 30, 2020

kroeckx commented Oct 30, 2020 via email

slontis commented Oct 31, 2020

mattcaswell commented Nov 6, 2020

mattcaswell commented Nov 9, 2020

t8m commented Nov 9, 2020

mattcaswell commented Nov 18, 2020

mattcaswell commented Nov 18, 2020

mattcaswell commented Nov 18, 2020

mattcaswell commented Nov 18, 2020