From 8937df0ad72b3f83d806a10f498857da2ac5477d Mon Sep 17 00:00:00 2001 From: Paul Kehrer Date: Sat, 8 Jul 2017 16:16:22 -0500 Subject: [PATCH 1/4] allow p % 24 == 23 when generator == 2 in DH_check --- src/_cffi_src/openssl/dh.py | 2 ++ .../hazmat/backends/openssl/backend.py | 14 +++++++++++-- tests/hazmat/primitives/test_dh.py | 21 ++++++++++++++++++- 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/_cffi_src/openssl/dh.py b/src/_cffi_src/openssl/dh.py index be761b97bae9..7ab06ae9c8fb 100644 --- a/src/_cffi_src/openssl/dh.py +++ b/src/_cffi_src/openssl/dh.py @@ -10,6 +10,8 @@ TYPES = """ typedef ... DH; + +const long DH_NOT_SUITABLE_GENERATOR; """ FUNCTIONS = """ diff --git a/src/cryptography/hazmat/backends/openssl/backend.py b/src/cryptography/hazmat/backends/openssl/backend.py index 28760aa85951..edbf3d7a34ce 100644 --- a/src/cryptography/hazmat/backends/openssl/backend.py +++ b/src/cryptography/hazmat/backends/openssl/backend.py @@ -1776,8 +1776,18 @@ def load_dh_private_numbers(self, numbers): res = self._lib.Cryptography_DH_check(dh_cdata, codes) self.openssl_assert(res == 1) - if codes[0] != 0: - raise ValueError("DH private numbers did not pass safety checks.") + # 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- + # parameter-check-when-g-2-must-p-mod-24-11 + if codes[0] != 0 and not ( + parameter_numbers.g == 2 and + codes[0] ^ self._lib.DH_NOT_SUITABLE_GENERATOR == 0 + ): + raise ValueError( + "DH private numbers did not pass safety checks." + ) evp_pkey = self._dh_cdata_to_evp_pkey(dh_cdata) diff --git a/tests/hazmat/primitives/test_dh.py b/tests/hazmat/primitives/test_dh.py index 1fdabe57be80..7abb85efe81f 100644 --- a/tests/hazmat/primitives/test_dh.py +++ b/tests/hazmat/primitives/test_dh.py @@ -4,6 +4,7 @@ from __future__ import absolute_import, division, print_function +import binascii import os import pytest @@ -158,6 +159,24 @@ def test_dh_parameters_supported(self, backend): assert backend.dh_parameters_supported(23, 5) assert not backend.dh_parameters_supported(23, 18) + @pytest.mark.parametrize( + "vector", + load_vectors_from_file( + os.path.join("asymmetric", "DH", "rfc3526.txt"), + load_nist_vectors + ) + ) + def test_dh_parameters_allows_rfc3526_groups(self, backend, vector): + p = int_from_bytes(binascii.unhexlify(vector["p"]), 'big') + params = dh.DHParameterNumbers(p, int(vector["g"])) + param = params.parameters(backend) + key = param.generate_private_key() + # This confirms that a key generated with this group + # will pass DH_check when we serialize and de-serialize it via + # the Numbers path. + roundtripped_key = key.private_numbers().private_key(backend) + assert key.private_numbers() == roundtripped_key.private_numbers() + @pytest.mark.parametrize( "vector", load_vectors_from_file( @@ -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) public = dh.DHPublicNumbers(1, params) private = dh.DHPrivateNumbers(2, public) From 3290796c95e58855fa099b6d4667732d17bbc344 Mon Sep 17 00:00:00 2001 From: Paul Kehrer Date: Sat, 8 Jul 2017 19:43:35 -0500 Subject: [PATCH 2/4] short url --- src/cryptography/hazmat/backends/openssl/backend.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cryptography/hazmat/backends/openssl/backend.py b/src/cryptography/hazmat/backends/openssl/backend.py index edbf3d7a34ce..919d8dba1c4b 100644 --- a/src/cryptography/hazmat/backends/openssl/backend.py +++ b/src/cryptography/hazmat/backends/openssl/backend.py @@ -1779,8 +1779,7 @@ def load_dh_private_numbers(self, numbers): # 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- - # parameter-check-when-g-2-must-p-mod-24-11 + # https://crypto.stackexchange.com/questions/12961 if codes[0] != 0 and not ( parameter_numbers.g == 2 and codes[0] ^ self._lib.DH_NOT_SUITABLE_GENERATOR == 0 From 507932a7323b1c4a43e3ae58a00389463159da8f Mon Sep 17 00:00:00 2001 From: Paul Kehrer Date: Sun, 9 Jul 2017 09:42:03 -0500 Subject: [PATCH 3/4] update and expand comments --- src/cryptography/hazmat/backends/openssl/backend.py | 10 +++++++--- tests/hazmat/primitives/test_dh.py | 5 +++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/cryptography/hazmat/backends/openssl/backend.py b/src/cryptography/hazmat/backends/openssl/backend.py index 919d8dba1c4b..5c5d01d9efd9 100644 --- a/src/cryptography/hazmat/backends/openssl/backend.py +++ b/src/cryptography/hazmat/backends/openssl/backend.py @@ -1777,9 +1777,13 @@ def load_dh_private_numbers(self, numbers): self.openssl_assert(res == 1) # 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 + # 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 + # Diffie-Hellman this means it can only generate half the possible + # values. That sounds bad, but quadratic nonresidues leak a bit of + # the key to the attacker in exchange for having the full key space + # available. See: https://crypto.stackexchange.com/questions/12961 if codes[0] != 0 and not ( parameter_numbers.g == 2 and codes[0] ^ self._lib.DH_NOT_SUITABLE_GENERATOR == 0 diff --git a/tests/hazmat/primitives/test_dh.py b/tests/hazmat/primitives/test_dh.py index 7abb85efe81f..fa658ae5ed66 100644 --- a/tests/hazmat/primitives/test_dh.py +++ b/tests/hazmat/primitives/test_dh.py @@ -221,6 +221,11 @@ def test_convert_to_numbers(self, backend, with_q): dh.DHPrivateKeyWithSerialization) def test_numbers_unsupported_parameters(self, backend): + # p is set to 21 because when calling private_key we want it to + # fail the DH_check call OpenSSL does. Originally this was 23, but + # we are allowing p % 24 to == 23 with this PR (see #3768 for more) + # By setting it to 21 it fails later in DH_check in a primality check + # which triggers the code path we want to test params = dh.DHParameterNumbers(21, 2) public = dh.DHPublicNumbers(1, params) private = dh.DHPrivateNumbers(2, public) From c6bb128c160980e36cb77f60db13df1444aa3140 Mon Sep 17 00:00:00 2001 From: Paul Kehrer Date: Sun, 9 Jul 2017 11:14:12 -0500 Subject: [PATCH 4/4] even better language! --- src/cryptography/hazmat/backends/openssl/backend.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cryptography/hazmat/backends/openssl/backend.py b/src/cryptography/hazmat/backends/openssl/backend.py index 5c5d01d9efd9..17e6735426f6 100644 --- a/src/cryptography/hazmat/backends/openssl/backend.py +++ b/src/cryptography/hazmat/backends/openssl/backend.py @@ -1779,7 +1779,7 @@ def load_dh_private_numbers(self, numbers): # 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 + # Specifically, g is then a quadratic residue. Within the context of # Diffie-Hellman this means it can only generate half the possible # values. That sounds bad, but quadratic nonresidues leak a bit of # the key to the attacker in exchange for having the full key space