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.

Sign up for GitHub

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

Remove DH generator size constraint #3364

Merged
merged 2 commits into from
Jul 17, 2017
Merged

Remove DH generator size constraint #3364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
95e993c
Remove DH generator size constraint
Jan 23, 2017
1d999b8
Check that g > 1
Jul 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/cryptography/hazmat/primitives/asymmetric/dh.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ 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")
Copy link
Author

Choose a reason for hiding this comment

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

Is this not redundant with DH_check?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

if g < 2:
raise ValueError("DH generator must be 2 or greater")

self._p = p
self._g = g
Expand Down
2 changes: 1 addition & 1 deletion tests/hazmat/primitives/test_dh.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def test_dh_parameternumbers():

with pytest.raises(ValueError):
dh.DHParameterNumbers(
65537, 7
65537, 1
)

params = dh.DHParameterNumbers(
Expand Down