Merge pull request #2558 from reaperhulk/managing-memory-is-fun

fix a potential memory issue when retaining revoked certs from a CRL

2 files changed, 27 insertions, 2 deletions

diff --git a/src/cryptography/hazmat/backends/openssl/x509.py b/src/cryptography/hazmat/backends/openssl/x509.py
index 7e89ac67..12f5d46f 100644
--- a/src/cryptography/hazmat/backends/openssl/x509.py
+++ b/src/cryptography/hazmat/backends/openssl/x509.py
@@ -747,8 +747,16 @@ def _decode_cert_issuer(backend, ext):
 
 @utils.register_interface(x509.RevokedCertificate)
 class _RevokedCertificate(object):
-    def __init__(self, backend, x509_revoked):
+    def __init__(self, backend, crl, x509_revoked):
         self._backend = backend
+        # The X509_REVOKED_value is a X509_REVOKED * that has
+        # no reference counting. This means when X509_CRL_free is
+        # called then the CRL and all X509_REVOKED * are freed. Since
+        # you can retain a reference to a single revoked certificate
+        # and let the CRL fall out of scope we need to retain a
+        # private reference to the CRL inside the RevokedCertificate
+        # object to prevent the gc from being called inappropriately.
+        self._crl = crl
         self._x509_revoked = x509_revoked
 
     @property
@@ -861,7 +869,10 @@ class _CertificateRevocationList(object):
             for i in range(num):
                 r = self._backend._lib.sk_X509_REVOKED_value(revoked, i)
                 self._backend.openssl_assert(r != self._backend._ffi.NULL)
-                revoked_list.append(_RevokedCertificate(self._backend, r))
+                revoked_certificate = _RevokedCertificate(
+                    self._backend, self, r
+                )
+                revoked_list.append(revoked_certificate)
 
         return revoked_list
 
diff --git a/tests/test_x509.py b/tests/test_x509.py
index ae2746e3..ab4d6660 100644
--- a/tests/test_x509.py
+++ b/tests/test_x509.py
@@ -173,6 +173,20 @@ class TestCertificateRevocationList(object):
         # Check that len() works for CRLs.
         assert len(crl) == 12
 
+    def test_revoked_cert_retrieval_retain_only_revoked(self, backend):
+        """
+        This test attempts to trigger the crash condition described in
+        https://github.com/pyca/cryptography/issues/2557
+        PyPy does gc at its own pace, so it will only be reliable on CPython.
+        """
+        revoked = _load_cert(
+            os.path.join("x509", "custom", "crl_all_reasons.pem"),
+            x509.load_pem_x509_crl,
+            backend
+        )[11]
+        assert revoked.revocation_date == datetime.datetime(2015, 1, 1, 0, 0)
+        assert revoked.serial_number == 11
+
     def test_extensions(self, backend):
         crl = _load_cert(
             os.path.join("x509", "custom", "crl_ian_aia_aki.pem"),