From 18d49d0c6cd117e82934f65e641c1d830edd20df Mon Sep 17 00:00:00 2001 From: Paul Kehrer Date: Tue, 4 Sep 2018 15:29:34 -0500 Subject: 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 --- src/cryptography/hazmat/backends/openssl/x509.py | 14 ++++++++++-- tests/x509/test_x509.py | 29 ++++++++++++++++++++++++ 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) -- cgit v1.2.3