Unify X.509 signature algorithm validation (#5276)HEADmaster

- Use common implementation
- OCSP signing was using different validation
- Check if private key is usable for signing

3 files changed, 72 insertions, 48 deletions

diff --git a/src/cryptography/hazmat/backends/openssl/backend.py b/src/cryptography/hazmat/backends/openssl/backend.py
index 0a6b7bc3..952c0f65 100644
--- a/src/cryptography/hazmat/backends/openssl/backend.py
+++ b/src/cryptography/hazmat/backends/openssl/backend.py
@@ -732,26 +732,33 @@ class Backend(object):
     def create_cmac_ctx(self, algorithm):
         return _CMACContext(self, algorithm)
 
-    def create_x509_csr(self, builder, private_key, algorithm):
-        if not isinstance(builder, x509.CertificateSigningRequestBuilder):
-            raise TypeError('Builder type mismatch.')
-
+    def _x509_check_signature_params(self, private_key, algorithm):
         if isinstance(private_key,
                       (ed25519.Ed25519PrivateKey, ed448.Ed448PrivateKey)):
             if algorithm is not None:
                 raise ValueError(
                     "algorithm must be None when signing via ed25519 or ed448"
                 )
+        elif not isinstance(private_key, (rsa.RSAPrivateKey, dsa.DSAPrivateKey,
+                            ec.EllipticCurvePrivateKey)):
+            raise TypeError(
+                "Key must be rsa, dsa, ec, ed25519 or ed448 private key."
+            )
         elif not isinstance(algorithm, hashes.HashAlgorithm):
-            raise TypeError('Algorithm must be a registered hash algorithm.')
+            raise TypeError("Algorithm must be a registered hash algorithm.")
         elif (
             isinstance(algorithm, hashes.MD5) and not
             isinstance(private_key, rsa.RSAPrivateKey)
         ):
             raise ValueError(
-                "MD5 is not a supported hash algorithm for EC/DSA CSRs"
+                "MD5 hash algorithm is only supported with RSA keys"
             )
 
+    def create_x509_csr(self, builder, private_key, algorithm):
+        if not isinstance(builder, x509.CertificateSigningRequestBuilder):
+            raise TypeError('Builder type mismatch.')
+        self._x509_check_signature_params(private_key, algorithm)
+
         # Resolve the signature algorithm.
         evp_md = self._evp_md_x509_null_if_eddsa(private_key, algorithm)
 
@@ -820,22 +827,7 @@ class Backend(object):
     def create_x509_certificate(self, builder, private_key, algorithm):
         if not isinstance(builder, x509.CertificateBuilder):
             raise TypeError('Builder type mismatch.')
-        if isinstance(private_key,
-                      (ed25519.Ed25519PrivateKey, ed448.Ed448PrivateKey)):
-            if algorithm is not None:
-                raise ValueError(
-                    "algorithm must be None when signing via ed25519 or ed448"
-                )
-        elif not isinstance(algorithm, hashes.HashAlgorithm):
-            raise TypeError('Algorithm must be a registered hash algorithm.')
-
-        if (
-            isinstance(algorithm, hashes.MD5) and not
-            isinstance(private_key, rsa.RSAPrivateKey)
-        ):
-            raise ValueError(
-                "MD5 is only (reluctantly) supported for RSA certificates"
-            )
+        self._x509_check_signature_params(private_key, algorithm)
 
         # Resolve the signature algorithm.
         evp_md = self._evp_md_x509_null_if_eddsa(private_key, algorithm)
@@ -932,22 +924,7 @@ class Backend(object):
     def create_x509_crl(self, builder, private_key, algorithm):
         if not isinstance(builder, x509.CertificateRevocationListBuilder):
             raise TypeError('Builder type mismatch.')
-        if isinstance(private_key,
-                      (ed25519.Ed25519PrivateKey, ed448.Ed448PrivateKey)):
-            if algorithm is not None:
-                raise ValueError(
-                    "algorithm must be None when signing via ed25519 or ed448"
-                )
-        elif not isinstance(algorithm, hashes.HashAlgorithm):
-            raise TypeError('Algorithm must be a registered hash algorithm.')
-
-        if (
-            isinstance(algorithm, hashes.MD5) and not
-            isinstance(private_key, rsa.RSAPrivateKey)
-        ):
-            raise ValueError(
-                "MD5 is not a supported hash algorithm for EC/DSA CRLs"
-            )
+        self._x509_check_signature_params(private_key, algorithm)
 
         evp_md = self._evp_md_x509_null_if_eddsa(private_key, algorithm)
 
@@ -1559,6 +1536,8 @@ class Backend(object):
         return _OCSPRequest(self, ocsp_req)
 
     def _create_ocsp_basic_response(self, builder, private_key, algorithm):
+        self._x509_check_signature_params(private_key, algorithm)
+
         basic = self._lib.OCSP_BASICRESP_new()
         self.openssl_assert(basic != self._ffi.NULL)
         basic = self._ffi.gc(basic, self._lib.OCSP_BASICRESP_free)
diff --git a/src/cryptography/x509/ocsp.py b/src/cryptography/x509/ocsp.py
index 4e0c985a..7db92b90 100644
--- a/src/cryptography/x509/ocsp.py
+++ b/src/cryptography/x509/ocsp.py
@@ -12,7 +12,6 @@ import six
 
 from cryptography import x509
 from cryptography.hazmat.primitives import hashes
-from cryptography.hazmat.primitives.asymmetric import ed25519, ed448
 from cryptography.x509.base import (
     _EARLIEST_UTC_TIME, _convert_to_naive_utc_time, _reject_duplicate_extension
 )
@@ -242,15 +241,6 @@ class OCSPResponseBuilder(object):
         if self._responder_id is None:
             raise ValueError("You must add a responder_id before signing")
 
-        if isinstance(private_key,
-                      (ed25519.Ed25519PrivateKey, ed448.Ed448PrivateKey)):
-            if algorithm is not None:
-                raise ValueError(
-                    "algorithm must be None when signing via ed25519 or ed448"
-                )
-        elif not isinstance(algorithm, hashes.HashAlgorithm):
-            raise TypeError("Algorithm must be a registered hash algorithm.")
-
         return backend.create_ocsp_response(
             OCSPResponseStatus.SUCCESSFUL, self, private_key, algorithm
         )
diff --git a/tests/x509/test_x509.py b/tests/x509/test_x509.py
index 7c45660f..38fe6bf8 100644
--- a/tests/x509/test_x509.py
+++ b/tests/x509/test_x509.py
@@ -4674,6 +4674,61 @@ class TestEd448Certificate(object):
         assert cert.signature_algorithm_oid == SignatureAlgorithmOID.ED448
 
 
+@pytest.mark.requires_backend_interface(interface=X509Backend)
+class TestSignatureRejection(object):
+    """Test if signing rejects DH keys properly.
+    """
+    def load_key(self, backend):
+        data = load_vectors_from_file(
+            os.path.join("asymmetric", "DH", "dhkey.pem"),
+            lambda pemfile: pemfile.read(),
+            mode="rb"
+        )
+        return serialization.load_pem_private_key(data, None, backend)
+
+    def test_crt_signing_check(self, backend):
+        issuer_private_key = self.load_key(backend)
+        public_key = RSA_KEY_2048.private_key(backend).public_key()
+        not_valid_before = datetime.datetime(2020, 1, 1, 1, 1)
+        not_valid_after = datetime.datetime(2050, 12, 31, 8, 30)
+        builder = x509.CertificateBuilder().serial_number(
+            777
+        ).issuer_name(x509.Name([
+            x509.NameAttribute(NameOID.COUNTRY_NAME, u'US'),
+        ])).subject_name(x509.Name([
+            x509.NameAttribute(NameOID.COUNTRY_NAME, u'US'),
+        ])).public_key(
+            public_key
+        ).not_valid_before(
+            not_valid_before
+        ).not_valid_after(
+            not_valid_after
+        )
+
+        with pytest.raises(TypeError):
+            builder.sign(issuer_private_key, hashes.SHA256(), backend)
+
+    def test_csr_signing_check(self, backend):
+        private_key = self.load_key(backend)
+        builder = x509.CertificateSigningRequestBuilder().subject_name(
+            x509.Name([x509.NameAttribute(NameOID.COUNTRY_NAME, u'US')])
+        )
+
+        with pytest.raises(TypeError):
+            builder.sign(private_key, hashes.SHA256(), backend)
+
+    def test_crl_signing_check(self, backend):
+        private_key = self.load_key(backend)
+        last_time = datetime.datetime.utcnow().replace(microsecond=0)
+        next_time = last_time
+        builder = x509.CertificateRevocationListBuilder().issuer_name(
+            x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, u"CA")])
+        ).last_update(last_time).next_update(next_time)
+
+        with pytest.raises(TypeError):
+            builder.sign(private_key, hashes.SHA256(), backend)
+
+
 def test_random_serial_number(monkeypatch):
     sample_data = os.urandom(20)