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.

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

Don't use things after they're freed...duh #709

Merged
merged 4 commits into from
Nov 20, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@ Deprecations:
Changes:
^^^^^^^^

*none*

- Corrected a use-after-free when reusing an issuer or subject from an ``X509`` object after the underlying object has been mutated.
`#709 <https://github.com/pyca/pyopenssl/pull/709>`_

----

Expand Down
7 changes: 2 additions & 5 deletions src/OpenSSL/SSL.py
Original file line number Diff line number Diff line change
Expand Up @@ -1957,9 +1957,7 @@ def get_peer_certificate(self):
"""
cert = _lib.SSL_get_peer_certificate(self._ssl)
if cert != _ffi.NULL:
pycert = X509.__new__(X509)
pycert._x509 = _ffi.gc(cert, _lib.X509_free)
return pycert
return X509._from_raw_x509_ptr(cert)
return None

def get_peer_cert_chain(self):
Expand All @@ -1977,8 +1975,7 @@ def get_peer_cert_chain(self):
for i in range(_lib.sk_X509_num(cert_stack)):
# TODO could incref instead of dup here
cert = _lib.X509_dup(_lib.sk_X509_value(cert_stack, i))
pycert = X509.__new__(X509)
pycert._x509 = _ffi.gc(cert, _lib.X509_free)
pycert = X509._from_raw_x509_ptr(cert)
result.append(pycert)
return result

Expand Down
45 changes: 36 additions & 9 deletions src/OpenSSL/crypto.py
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,19 @@ def _get_asn1_time(timestamp):
return string_result


class _X509NameInvalidator(object):
def __init__(self):
self._names = []

def add(self, name):
self._names.append(name)

def clear(self):
for name in self._names:
# Breaks the object, but also prevents UAF!
del name._name


class PKey(object):
"""
A class representing an DSA or RSA public key or key pair.
Expand Down Expand Up @@ -1032,6 +1045,17 @@ def __init__(self):
_openssl_assert(x509 != _ffi.NULL)
self._x509 = _ffi.gc(x509, _lib.X509_free)

self._issuer_invalidator = _X509NameInvalidator()
self._subject_invalidator = _X509NameInvalidator()

@classmethod
def _from_raw_x509_ptr(cls, x509):
cert = cls.__new__(cls)
cert._x509 = _ffi.gc(x509, _lib.X509_free)
cert._issuer_invalidator = _X509NameInvalidator()
cert._subject_invalidator = _X509NameInvalidator()
return cert

def to_cryptography(self):
"""
Export as a ``cryptography`` certificate.
Expand Down Expand Up @@ -1382,7 +1406,9 @@ def get_issuer(self):
:return: The issuer of this certificate.
:rtype: :class:`X509Name`
"""
return self._get_name(_lib.X509_get_issuer_name)
name = self._get_name(_lib.X509_get_issuer_name)
self._issuer_invalidator.add(name)
return name

def set_issuer(self, issuer):
"""
Expand All @@ -1393,7 +1419,8 @@ def set_issuer(self, issuer):

:return: ``None``
"""
return self._set_name(_lib.X509_set_issuer_name, issuer)
self._set_name(_lib.X509_set_issuer_name, issuer)
self._issuer_invalidator.clear()

def get_subject(self):
"""
Expand All @@ -1407,7 +1434,9 @@ def get_subject(self):
:return: The subject of this certificate.
:rtype: :class:`X509Name`
"""
return self._get_name(_lib.X509_get_subject_name)
name = self._get_name(_lib.X509_get_subject_name)
self._subject_invalidator.add(name)
return name

def set_subject(self, subject):
"""
Expand All @@ -1418,7 +1447,8 @@ def set_subject(self, subject):

:return: ``None``
"""
return self._set_name(_lib.X509_set_subject_name, subject)
self._set_name(_lib.X509_set_subject_name, subject)
self._subject_invalidator.clear()

def get_extension_count(self):
"""
Expand Down Expand Up @@ -1691,8 +1721,7 @@ def _exception_from_context(self):
# expect this call to never return :class:`None`.
_x509 = _lib.X509_STORE_CTX_get_current_cert(self._store_ctx)
_cert = _lib.X509_dup(_x509)
pycert = X509.__new__(X509)
pycert._x509 = _ffi.gc(_cert, _lib.X509_free)
pycert = X509._from_raw_x509_ptr(_cert)
return X509StoreContextError(errors, pycert)

def set_store(self, store):
Expand Down Expand Up @@ -1755,9 +1784,7 @@ def load_certificate(type, buffer):
if x509 == _ffi.NULL:
_raise_current_error()

cert = X509.__new__(X509)
cert._x509 = _ffi.gc(x509, _lib.X509_free)
return cert
return X509._from_raw_x509_ptr(x509)


def dump_certificate(type, cert):
Expand Down