Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow p % 24 == 23 when generator == 2 in DH_check #3768

Merged
merged 4 commits into from
Jul 10, 2017

Conversation

reaperhulk
Copy link
Member

refs #3755

Depends on #3767

https://crypto.stackexchange.com/questions/12961/diffie-hellman-parameter-check-when-g-2-must-p-mod-24-11

  • conversations with competent cryptographers have convinced me that it's reasonable to do this, but once add rfc 3526 DH groups #3767 merges we can cc lvh for a review :)

@mention-bot
Copy link

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

@alex
Copy link
Member

alex commented Jul 8, 2017

Safe to rebase and bother @lvh :-)

# DH_check will return DH_NOT_SUITABLE_GENERATOR if p % 24 does not
# equal 11 when the generator is 2. We want to ignore that error
# because p % 24 == 23 is also fine. See:
# https://crypto.stackexchange.com/questions/12961/diffie-hellman-
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can drop everything after the /12961/

# DH_check will return DH_NOT_SUITABLE_GENERATOR if p % 24 does not
# equal 11 when the generator is 2. We want to ignore that error
# because p % 24 == 23 is also fine. See:
# https://crypto.stackexchange.com/questions/12961
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be marginally happier if this had the language "quadratic residue" and "quadratic nonresidue", but looking at the SO answer it's actually... really good?

@@ -202,7 +221,7 @@ def test_convert_to_numbers(self, backend, with_q):
dh.DHPrivateKeyWithSerialization)

def test_numbers_unsupported_parameters(self, backend):
params = dh.DHParameterNumbers(23, 2)
params = dh.DHParameterNumbers(21, 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be happier if this had a comment, because someone git-blaming this (heck, me git blaming this) would probably not stop to think about why this is different/better and why 23 suddenly works now.

lvh
lvh previously approved these changes Jul 9, 2017
Copy link
Member

@lvh lvh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to see more comments but that's optional.

@lvh
Copy link
Member

lvh commented Jul 9, 2017

(We discussed this in a backchannel. The TL;DR is: OpenSSL's check is fine for g==2 but makes a tradeoff. Your FFDH public keys either leak a bit of your secret, or you can only get half of the possible values. Other standards might reasonably choose the opposite tradeoff.)

@reaperhulk
Copy link
Member Author

Thanks @lvh! I've expanded the comments to address your requests!

# DH_check will return DH_NOT_SUITABLE_GENERATOR if p % 24 does not
# equal 11 when the generator is 2 (a quadratic nonresidue).
# We want to ignore that error because p % 24 == 23 is also fine.
# Specifically, it is a quadratic residue. Within the context of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: "it" is g here. Rest of the explanation looks fine :)

lvh
lvh previously approved these changes Jul 9, 2017
@reaperhulk
Copy link
Member Author

Okay I think this is now ready. @lvh or @alex could you do the honors? 😄

Thanks for the reviews lvh!

@lvh
Copy link
Member

lvh commented Jul 10, 2017

Since @alex has specifically indicated not wanting to touch this with a ten foot pole...

@lvh lvh merged commit dc6e762 into pyca:master Jul 10, 2017
@reaperhulk reaperhulk deleted the dh-improve-check branch July 10, 2017 05:36
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants