Fix various issues around X509_STORE_CTX reuse (#1272)

* Don't try to access X509_STORE_CTX after _cleanup

Although OpenSSL happens to leave the errors in there on
X509_STORE_CTX_cleanup, in no other OpenSSL API is accessing a cleaned
up object meaningful. Do it in the other order.

* Internal variables are internal

The underscore-prefixed variables were not intended to be exposed as
public API, so don't bother exposing it in the first place.

* Don't reuse X509_STORE_CTXs

There's a lot of history with X509_STORE_CTX's somewhat messy transition
from a stack-allocated type to a heap-allocated type. (This is why a
double X509_STORE_CTX_init used to leak memory.) We can avoid all this
mess by just making a new X509_STORE_CTX each time.

Author: davidben

src/OpenSSL/crypto.py

@@ -1881,12 +1881,6 @@ class X509StoreContext:
     of a certificate in a described context. For describing such a context, see
     :class:`X509Store`.
 
-    :ivar _store_ctx: The underlying X509_STORE_CTX structure used by this
-        instance.  It is dynamically allocated and automatically garbage
-        collected.
-    :ivar _store: See the ``store`` ``__init__`` parameter.
-    :ivar _cert: See the ``certificate`` ``__init__`` parameter.
-    :ivar _chain: See the ``chain`` ``__init__`` parameter.
     :param X509Store store: The certificates which will be trusted for the
         purposes of any verifications.
     :param X509 certificate: The certificate to be verified.
@@ -1901,15 +1895,9 @@ def __init__(
         certificate: X509,
         chain: Optional[Sequence[X509]] = None,
     ) -> None:
-        store_ctx = _lib.X509_STORE_CTX_new()
-        self._store_ctx = _ffi.gc(store_ctx, _lib.X509_STORE_CTX_free)
         self._store = store
         self._cert = certificate
         self._chain = self._build_certificate_stack(chain)
-        # Make the store context available for use after instantiating this
-        # class by initializing it now. Per testing, subsequent calls to
-        # :meth:`_init` have no adverse affect.
-        self._init()
 
     @staticmethod
     def _build_certificate_stack(
@@ -1941,28 +1929,8 @@ def cleanup(s: Any) -> None:
 
         return stack
 
-    def _init(self) -> None:
-        """
-        Set up the store context for a subsequent verification operation.
-
-        Calling this method more than once without first calling
-        :meth:`_cleanup` will leak memory.
-        """
-        ret = _lib.X509_STORE_CTX_init(
-            self._store_ctx, self._store._store, self._cert._x509, self._chain
-        )
-        if ret <= 0:
-            _raise_current_error()
-
-    def _cleanup(self) -> None:
-        """
-        Internally cleans up the store context.
-
-        The store context can then be reused with a new call to :meth:`_init`.
-        """
-        _lib.X509_STORE_CTX_cleanup(self._store_ctx)
-
-    def _exception_from_context(self) -> X509StoreContextError:
+    @staticmethod
+    def _exception_from_context(store_ctx: Any) -> X509StoreContextError:
         """
         Convert an OpenSSL native context error failure into a Python
         exception.
@@ -1972,21 +1940,45 @@ def _exception_from_context(self) -> X509StoreContextError:
         """
         message = _ffi.string(
             _lib.X509_verify_cert_error_string(
-                _lib.X509_STORE_CTX_get_error(self._store_ctx)
+                _lib.X509_STORE_CTX_get_error(store_ctx)
             )
         ).decode("utf-8")
         errors = [
-            _lib.X509_STORE_CTX_get_error(self._store_ctx),
-            _lib.X509_STORE_CTX_get_error_depth(self._store_ctx),
+            _lib.X509_STORE_CTX_get_error(store_ctx),
+            _lib.X509_STORE_CTX_get_error_depth(store_ctx),
             message,
         ]
         # A context error should always be associated with a certificate, so we
         # expect this call to never return :class:`None`.
-        _x509 = _lib.X509_STORE_CTX_get_current_cert(self._store_ctx)
+        _x509 = _lib.X509_STORE_CTX_get_current_cert(store_ctx)
         _cert = _lib.X509_dup(_x509)
         pycert = X509._from_raw_x509_ptr(_cert)
         return X509StoreContextError(message, errors, pycert)
 
+    def _verify_certificate(self) -> Any:
+        """
+        Verifies the certificate and runs an X509_STORE_CTX containing the
+        results.
+
+        :raises X509StoreContextError: If an error occurred when validating a
+          certificate in the context. Sets ``certificate`` attribute to
+          indicate which certificate caused the error.
+        """
+        store_ctx = _lib.X509_STORE_CTX_new()
+        _openssl_assert(store_ctx != _ffi.NULL)
+        store_ctx = _ffi.gc(store_ctx, _lib.X509_STORE_CTX_free)
+
+        ret = _lib.X509_STORE_CTX_init(
+            store_ctx, self._store._store, self._cert._x509, self._chain
+        )
+        _openssl_assert(ret == 1)
+
+        ret = _lib.X509_verify_cert(store_ctx)
+        if ret <= 0:
+            raise self._exception_from_context(store_ctx)
+
+        return store_ctx
+
     def set_store(self, store: X509Store) -> None:
         """
         Set the context's X.509 store.
@@ -2008,17 +2000,7 @@ def verify_certificate(self) -> None:
           certificate in the context. Sets ``certificate`` attribute to
           indicate which certificate caused the error.
         """
-        # Always re-initialize the store context in case
-        # :meth:`verify_certificate` is called multiple times.
-        #
-        # :meth:`_init` is called in :meth:`__init__` so _cleanup is called
-        # before _init to ensure memory is not leaked.
-        self._cleanup()
-        self._init()
-        ret = _lib.X509_verify_cert(self._store_ctx)
-        self._cleanup()
-        if ret <= 0:
-            raise self._exception_from_context()
+        self._verify_certificate()
 
     def get_verified_chain(self) -> List[X509]:
         """
@@ -2031,20 +2013,10 @@ def get_verified_chain(self) -> List[X509]:
 
         .. versionadded:: 20.0
         """
-        # Always re-initialize the store context in case
-        # :meth:`verify_certificate` is called multiple times.
-        #
-        # :meth:`_init` is called in :meth:`__init__` so _cleanup is called
-        # before _init to ensure memory is not leaked.
-        self._cleanup()
-        self._init()
-        ret = _lib.X509_verify_cert(self._store_ctx)
-        if ret <= 0:
-            self._cleanup()
-            raise self._exception_from_context()
+        store_ctx = self._verify_certificate()
 
         # Note: X509_STORE_CTX_get1_chain returns a deep copy of the chain.
-        cert_stack = _lib.X509_STORE_CTX_get1_chain(self._store_ctx)
+        cert_stack = _lib.X509_STORE_CTX_get1_chain(store_ctx)
         _openssl_assert(cert_stack != _ffi.NULL)
 
         result = []
@@ -2056,7 +2028,6 @@ def get_verified_chain(self) -> List[X509]:
 
         # Free the stack but not the members which are freed by the X509 class.
         _lib.sk_X509_free(cert_stack)
-        self._cleanup()
         return result