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 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.rst
Expand Up @@ -23,8 +23,9 @@ Deprecations:
Changes:
^^^^^^^^

*none*

- Corrected a use-after-free with some uses of the ``X509`` API.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a sentence here to state that referencing a previously obtained issuer/subject after a subsequent set call will now raise an exception?

`#709 <https://github.com/pyca/pyopenssl/pull/709>`_

----

Expand Down
7 changes: 2 additions & 5 deletions src/OpenSSL/SSL.py
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
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