don't sort the serial numbers in a parsed CRL (#4457)

* don't sort the serial numbers in a parsed CRL

OpenSSL sorts them in place and this breaks the signature and more.
fixes #4456

* cache the sorted CRL (but create it lazily)

* use the cache decorator

2 files changed, 41 insertions, 2 deletions

diff --git a/src/cryptography/hazmat/backends/openssl/x509.py b/src/cryptography/hazmat/backends/openssl/x509.py
index a7a2c70d..ad838b7f 100644
--- a/src/cryptography/hazmat/backends/openssl/x509.py
+++ b/src/cryptography/hazmat/backends/openssl/x509.py
@@ -238,11 +238,21 @@ class _CertificateRevocationList(object):
         h.update(der)
         return h.finalize()
 
+    @utils.cached_property
+    def _sorted_crl(self):
+        # X509_CRL_get0_by_serial sorts in place, which breaks a variety of
+        # things we don't want to break (like iteration and the signature).
+        # Let's dupe it and sort that instead.
+        dup = self._backend._lib.X509_CRL_dup(self._x509_crl)
+        self._backend.openssl_assert(dup != self._backend._ffi.NULL)
+        dup = self._backend._ffi.gc(dup, self._backend._lib.X509_CRL_free)
+        return dup
+
     def get_revoked_certificate_by_serial_number(self, serial_number):
         revoked = self._backend._ffi.new("X509_REVOKED **")
         asn1_int = _encode_asn1_int_gc(self._backend, serial_number)
         res = self._backend._lib.X509_CRL_get0_by_serial(
-            self._x509_crl, revoked, asn1_int
+            self._sorted_crl, revoked, asn1_int
         )
         if res == 0:
             return None
@@ -251,7 +261,7 @@ class _CertificateRevocationList(object):
                 revoked[0] != self._backend._ffi.NULL
             )
             return _RevokedCertificate(
-                self._backend, self._x509_crl, revoked[0]
+                self._backend, self._sorted_crl, revoked[0]
             )
 
     @property
diff --git a/tests/x509/test_x509.py b/tests/x509/test_x509.py
index a866d813..4ce4d958 100644
--- a/tests/x509/test_x509.py
+++ b/tests/x509/test_x509.py
@@ -550,6 +550,35 @@ class TestRevokedCertificate(object):
         assert crl[2:4][0].serial_number == crl[2].serial_number
         assert crl[2:4][1].serial_number == crl[3].serial_number
 
+    def test_get_revoked_certificate_doesnt_reorder(self, backend):
+        private_key = RSA_KEY_2048.private_key(backend)
+        last_update = datetime.datetime(2002, 1, 1, 12, 1)
+        next_update = datetime.datetime(2030, 1, 1, 12, 1)
+        builder = x509.CertificateRevocationListBuilder().issuer_name(
+            x509.Name([
+                x509.NameAttribute(NameOID.COMMON_NAME, u"cryptography.io CA")
+            ])
+        ).last_update(
+            last_update
+        ).next_update(
+            next_update
+        )
+        for i in [2, 500, 3, 49, 7, 1]:
+            revoked_cert = x509.RevokedCertificateBuilder().serial_number(
+                i
+            ).revocation_date(
+                datetime.datetime(2012, 1, 1, 1, 1)
+            ).build(backend)
+            builder = builder.add_revoked_certificate(revoked_cert)
+        crl = builder.sign(private_key, hashes.SHA256(), backend)
+        assert crl[0].serial_number == 2
+        assert crl[2].serial_number == 3
+        # make sure get_revoked_certificate_by_serial_number doesn't affect
+        # ordering after being invoked
+        crl.get_revoked_certificate_by_serial_number(500)
+        assert crl[0].serial_number == 2
+        assert crl[2].serial_number == 3
+
 
 @pytest.mark.requires_backend_interface(interface=RSABackend)
 @pytest.mark.requires_backend_interface(interface=X509Backend)