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

* allow p % 24 == 23 when generator == 2 in DH_check

* short url

* update and expand comments

* even better language!

src/_cffi_src/openssl/dh.py

@@ -10,6 +10,8 @@
 
 TYPES = """
 typedef ... DH;
+
+const long DH_NOT_SUITABLE_GENERATOR;
 """
 
 FUNCTIONS = """

src/cryptography/hazmat/backends/openssl/backend.py

@@ -1776,8 +1776,21 @@ 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 (a quadratic nonresidue).
+        # We want to ignore that error because p % 24 == 23 is also fine.
+        # 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
+        # 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
+        ):
+            raise ValueError(
+                "DH private numbers did not pass safety checks."
+            )
 
         evp_pkey = self._dh_cdata_to_evp_pkey(dh_cdata)
 

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,12 @@ def test_convert_to_numbers(self, backend, with_q):
                           dh.DHPrivateKeyWithSerialization)
 
     def test_numbers_unsupported_parameters(self, backend):
-        params = dh.DHParameterNumbers(23, 2)
+        # 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)