Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix various issues around X509_STORE_CTX reuse #1272

Merged
merged 3 commits into from
Nov 30, 2023
Merged

Fix various issues around X509_STORE_CTX reuse #1272

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c82c5f6
Don't try to access X509_STORE_CTX after _cleanup
davidben Nov 30, 2023
6a1b0a8
Internal variables are internal
davidben Nov 30, 2023
1454cd6
Don't reuse X509_STORE_CTXs
davidben Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 33 additions & 62 deletions src/OpenSSL/crypto.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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(
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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]:
"""
Expand All @@ -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 = []
Expand All @@ -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


Expand Down