Remove DH generator size constraint #3364

ria4 · 2017-01-23T10:21:33Z

I've been working on a cryptography based TLS module for Scapy (see PR secdev/scapy#476).

During test handshakes with openssl, I came across this limitation on the generator size of DH groups. Default DH group parameters with openssl are currently 2048-bit order with a 2048-bit generator. While we could debate on the impact of choosing a long generator over a value of 2 or 5, it does not seem appropriate to keep this constraint in the library.

mention-bot · 2017-01-23T10:21:34Z

@mtury, thanks for your PR! By analyzing the history of the files in this pull request, we identified @public, @palaviv and @elitest to be potential reviewers.

reaperhulk · 2017-01-23T11:13:01Z

To my knowledge the only reason we have this constraint is that we don't have tests for anything other than generator values of 2 or 5. Are you aware of anywhere we can get tests to cover larger generator sizes, or, failing that, a reason why we shouldn't care?

It seems clear it's desirable to remove it so I'd like to be confident that our test suite can handle the case properly in a future where we add a new backend that has a bug with larger generator sizes.

alex · 2017-01-23T13:43:24Z

Jenkins, ok to test.

ria4 · 2017-01-23T13:50:19Z

NIST provides test vectors with 512-bit generators here.

Unfortunately the keys cannot be directly imported because of a call to openssl's DH_check through cryptography/hazmat/backends/openssl/backend.py. On the tests I ran, it returned the codes for DH_NOT_SUITABLE_GENERATOR (which is to be expected for any non-{2,3,5} generator) but also DH_CHECK_P_NOT_SAFE_PRIME.

However, when I bypassed this, I could check that the computed shared secret was the expected one.

What do you think? Should the backend check be made unreceptive to those two codes (I did not come across any safe prime on 5 different groups)? Or should we rather not test this?

palaviv · 2017-01-23T22:18:46Z

@mtury you can pass DH_check by setting the appropriate q value. I assume that in all the cases that you have with such generator provide it.
I was planning to add support for prime-order subgroup size but didn't had the time yet.

ria4 · 2017-01-24T17:16:21Z

Indeed q is provided with the test vectors. But supporting the subgroup order with cryptography requires some work, and it is somewhat distinct from this PR.

Rather than weakening the backend check, I'd favor not testing long generators for now. The generator size distinction in openssl only intervenes in dh_check.c (and dh_gen.c). If anything should go wrong with long generators, we can expect the existing tests with small generators to fail anyway.

New vectors could be added as soon as subgroup orders are supported.

palaviv · 2017-01-24T20:49:40Z

First I will start working on adding the subgroup order and hopefully will have a PR ready by next week. In any case I think that this is O.K to remove the DH_check and instead encourage people to use dh_parameters_supported instead.

markrwilliams · 2017-01-29T21:32:50Z

@palaviv DH_check also fails when the generator is 2 and p mod 24 = 23. This prevents the use of the 2048-bit MODP Group from RFC-3526 and thus the implementation of the diffie-hellman-group14-sha256 SSH key exchange algorithm.

While it's possible to clear the DH_NOT_SUITABLE_GENERATOR bit from DH_check's error codes iff g = 2 and p mod 24 = 23, it's probably not worth it. I'd like to get Twisted's SSH implementation using this DH implementation - would it be helpful to do a PR that removed the DH_check in favor of dh_parameters_supported ?

palaviv · 2017-01-29T21:39:39Z

I think that will be the best solution @markrwilliams. I now have problems with DH_check in #3369 and that change will make it easier.

markrwilliams · 2017-01-29T22:40:06Z

@palaviv Hm, looking at the code it appears that dh_parameters_supported is implemented in terms of DH_check. Do you have a different approach for this?

reaperhulk · 2017-01-30T11:23:18Z

(We talked about this a bit in IRC and @markrwilliams is going to look at pulling in a local copy of DH_check as Cryptography_DH_check)

palaviv · 2017-01-30T18:24:13Z

I looked at the logs of the IRC talk (hope I did not miss anything). I don't think that a local copy of the new DH_check will help in this problem. @reaperhulk have you meant to #3369 that will be a great solution to the problem there (Would you like me to do it there @markrwilliams?).
The problem here will remain with the new code as without a q we only accept 2,5 as generators in DH_check.

@palaviv Hm, looking at the code it appears that dh_parameters_supported is implemented in terms of DH_check. Do you have a different approach for this?

I think you misunderstood me @markrwilliams. My suggestion is to remove the DH_check in load_dh_private_numbers and in the documentation encourage the user to use dh_parameters_supported.

It seems like there are a lot of problems with DH_check. We could try DH_check first and then use our own function if that fails. I am not a mathematics expert but finding if a number is a primitive root modulo is very slow. We could make it faster by adding a list of known p,g pairs from the RFC.

markrwilliams · 2017-01-30T19:38:25Z

My suggestion is to remove the DH_check in load_dh_private_numbers and in the documentation encourage the user to use dh_parameters_supported.

Hm, so then users would call dh_parameters_supported on the assumption that they have the right q value for their g and p? I like the flexibility of this but don't like that users could forget to call it.

If we did take that approach, we'd still have to backport a DH_check that knows about q for OpenSSLs < 1.0.1, right?

It seems like there are a lot of problems with DH_check. We could try DH_check first and then use our own function if that fails. I am not a mathematics expert but finding if a number is a primitive root modulo is very slow. We could make it faster by adding a list of known p,g pairs from the RFC.

I'm definitely not even a mathematics amateur :( OpenSSH seems to just check if the public key passes very basic sanity checks. The most nuanced thing there is the bit checking. Maybe that's good enough, and we can do this in our own replacement for DH_check?

@reaperhulk what do you think?

palaviv · 2017-01-30T21:24:42Z

If we did take that approach, we'd still have to backport a DH_check that knows about q for OpenSSLs < 1.0.1, right?

Yes. I would suggest doing that in #3369 and not in the PR. Would you like me to do that or would you like to open a PR to my branch?

Doing what OpenSSH does sounds good as a fallback. I looked in mbedtls and they do the same.

Hm, so then users would call dh_parameters_supported on the assumption that they have the right q value for their g and p? I like the flexibility of this but don't like that users could forget to call it.

How about adding a check parameter that will default to True?

markrwilliams · 2017-01-30T22:17:52Z

Would you like me to do that or would you like to open a PR to my branch?

I'll be happy to do a PR against your branch tonight :)

How about adding a check parameter that will default to True?

I like this. Since this is a hazmat primitive, a secure default seems sufficient to me.

reaperhulk · 2017-01-31T00:21:59Z

Okay, so if I understand correctly, the order here is:

Port the DH_check from 1.1.0 and use that if the detected openssl version < 1.1.0d (please make this a separate PR, not a part of 3369. It will ease review)
Support DH subgroup order (DH subgroup order (q) #3369).
Remove the DH size constraint while adding the 512-bit generator tests that @mtury found.

markrwilliams · 2017-01-31T03:59:48Z

@reaperhulk Yes, with some additions:

Before DH subgroup order (q) #3369 lands, running DH_check should be made controllable by an argument (probably to DHParameters.generate_private_key and DHPublicNumbers.public_key) with a default that ensure it runs. That way if you don't have q handy but know that the prime is legal, you can still use it. This seems OK because it's a safe default for a potentially dangerous option inside hazmat. Thoughts?
After DH subgroup order (q) #3369 lands, we should allow checking the validity of the public key as mbedtls and OpenSSH do. I imagine this would also be an argument to generate_private_key/public_key.

The two together make me think we should have an enum representing these possible checks:

private_numbers = DHPrivateNumbers(...)
private_key = private_numbers.parameters().private_key(DHCheck.CHECK_PARAMETERS)
# -or-
private_key = private_numbers.parameters().private_key(DHCheck.CHECK_PUBLIC_KEY)

But if that happens, it would be happen after the PR to backport DH_check!

markrwilliams · 2017-01-31T05:39:10Z

#3375 is ready to go, but is hard to test without the addition of q, which will come in #3369

reaperhulk · 2017-03-09T22:04:19Z

We're close here thanks to a bunch of DH subgroup order work landing, but this will be in 1.9 rather than 1.8.

palaviv · 2017-03-23T18:41:15Z

@mtury, @markrwilliams any of you planning on working on this or would you like me to do this?

ria4 · 2017-04-14T14:40:23Z

I was away for a few weeks, sorry. Unfortunately I won't have time to spend on the subject before the summer. Considering your involvement on the DH implementation, it would probably be better if you handled this.

palaviv · 2017-06-24T15:45:50Z

I am starting to work on this but I am having a little hard time understanding the use cases. As much as I can understand @mtury would like to remove the generator size constraint and @markrwilliams would like to remove (or replace) the DH_check from load_dh_private_numbers.
@mtury is this needed after the subgroup support?
@markrwilliams would you mind posting a small use case so I will be clear on what you would like to do? Do you use previously calculated private and public key?

reaperhulk · 2017-07-06T03:00:27Z

#3755 might also be relevant since removing DH_check would make it so you could pass alternate generators without complaint.

DH_check turns out to block some bad values, but not all. It also blocks some *good* values. This causes problems. Removing this check will fix pyca#3755 and pyca#3364

ria4 · 2017-07-07T14:01:25Z

@palaviv Unfortunately the subgroup support is virtually of no use to TLS, as the protocol does not provide a way to send subgroup orders. I discussed this briefly in #3414.

What prevents Scapy from using the library without any hack is basically a pre-DH_check test in DHParameterNumbers __init__. If DH_check ends up being removed, I believe this test should be removed too.

ria4 · 2017-07-07T14:14:07Z

src/cryptography/hazmat/primitives/asymmetric/dh.py

@@ -87,9 +87,6 @@ def __init__(self, p, g, q=None):
        if q is not None and not isinstance(q, six.integer_types):
            raise TypeError("q must be integer or None")

-        if q is None and g not in (2, 5):
-            raise ValueError("DH generator must be 2 or 5")


Is this not redundant with DH_check?

It is, in fact, worse than DH_check since DH_check allows 2, 3, and 5. Presumably scapy wants to convert Numbers into an instance of the object though, correct? So we need to potentially remove this check and also either remove or modify DH_check?

Actually once these two lines are removed it works fine. Indeed Scapy uses instances of DHParameterNumbers, but this never calls DH_check.

As far as I can tell, for now DH_check is called only in asymmetric/dh.py for DHPrivateNumbers through load_dh_private_numbers (or in test_dh.py through dh_parameters_supported). So apart from the library tests it is not called when dealing with DHParameterNumbers or DHPublicNumbers.

Do you convert the DHParameterNumbers to a DHParameters via parameters? I guess we don't validate anything on that path so you can make a parameters and then call generate_private_key.

Indeed we do not use DHParameterNumbers unless for immediate conversion to DHParameters. We'd be fine with the workaround you suggest. Do you see anything else to add to the PR?

I'm mostly trying to reconcile the existence of DH_check with the fact that we don't (and can't) validate here, but g=1 would still be a very bad thing to put here, so I think g > 1 is probably what the modified check should be if we're going to merge this.

reaperhulk · 2017-07-17T09:01:13Z

LGTM, let's see what CI has to say :)

(I hate FFDH)

ria4 · 2017-07-17T09:04:38Z

Now an error is raised when g < 2.
I did not keep the q is None check because we never want g to be less than 2 anyway.

reaperhulk · 2017-07-17T09:21:52Z

tests/hazmat/primitives/test_dh.py

@@ -51,9 +51,9 @@ def test_dh_parameternumbers():
            None, None
        )

-    with pytest.raises(ValueError):
+    with pytest.raises(TypeError):


This still needs to be ValueError

reaperhulk · 2017-07-17T12:29:21Z

Thanks for sticking around for so long @mtury. Could I prevail on you to submit one more quick PR to update the docs to note the new restriction on DHParameterNumbers?

reaperhulk · 2017-07-17T12:33:50Z

Nevermind! I went ahead and put it in as #3786

ria4 · 2017-07-19T10:02:27Z

Congrats for closing on the whole FFDH saga. :)

palaviv mentioned this pull request Jan 26, 2017

DH subgroup order (q) #3369

Merged

reaperhulk added this to the Eighteenth Release milestone Feb 7, 2017

palaviv mentioned this pull request Feb 21, 2017

Add ASN1_F_ASN1_CHECK_TLEN #3414

Merged

reaperhulk modified the milestones: Eighteenth Release, Nineteenth Release Mar 9, 2017

alex added the primitives label May 27, 2017

alex modified the milestones: Nineteenth Release, Twentieth release May 29, 2017

alex removed this from the Nineteenth Release milestone May 29, 2017

reaperhulk mentioned this pull request Jul 7, 2017

Remove DH check from load_dh_private_numbers #3758

Closed

ria4 commented Jul 7, 2017

View reviewed changes

Remove DH generator size constraint

95e993c

reaperhulk reviewed Jul 17, 2017

View reviewed changes

Check that g > 1

1d999b8

reaperhulk approved these changes Jul 17, 2017

View reviewed changes

reaperhulk merged commit cc12bea into pyca:master Jul 17, 2017

reaperhulk added a commit to reaperhulk/cryptography that referenced this pull request Jul 17, 2017

update docs to reflect pyca#3364

7ff97d4

alex pushed a commit that referenced this pull request Jul 17, 2017

update docs to reflect #3364 (#3786)

ab4a0b3

github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2020

Remove DH generator size constraint #3364

Remove DH generator size constraint #3364

Conversation

ria4 commented Jan 23, 2017

mention-bot commented Jan 23, 2017

reaperhulk commented Jan 23, 2017

alex commented Jan 23, 2017

ria4 commented Jan 23, 2017

palaviv commented Jan 23, 2017

ria4 commented Jan 24, 2017

palaviv commented Jan 24, 2017

markrwilliams commented Jan 29, 2017

palaviv commented Jan 29, 2017

markrwilliams commented Jan 29, 2017

reaperhulk commented Jan 30, 2017

palaviv commented Jan 30, 2017

markrwilliams commented Jan 30, 2017

palaviv commented Jan 30, 2017

markrwilliams commented Jan 30, 2017

reaperhulk commented Jan 31, 2017

markrwilliams commented Jan 31, 2017

markrwilliams commented Jan 31, 2017

reaperhulk commented Mar 9, 2017

palaviv commented Mar 23, 2017

ria4 commented Apr 14, 2017

palaviv commented Jun 24, 2017

reaperhulk commented Jul 6, 2017

ria4 commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reaperhulk commented Jul 17, 2017

ria4 commented Jul 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reaperhulk commented Jul 17, 2017

reaperhulk commented Jul 17, 2017

ria4 commented Jul 19, 2017