From 2e194f690b247c776d07fc79f0b32aca9b7b0d99 Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Wed, 15 Nov 2017 19:20:43 -0500 Subject: [PATCH 1/3] Don't use things after they're freed...duh --- src/OpenSSL/SSL.py | 7 ++----- src/OpenSSL/crypto.py | 45 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/src/OpenSSL/SSL.py b/src/OpenSSL/SSL.py index 75d080acd..a32c10b32 100644 --- a/src/OpenSSL/SSL.py +++ b/src/OpenSSL/SSL.py @@ -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): @@ -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 diff --git a/src/OpenSSL/crypto.py b/src/OpenSSL/crypto.py index 72c04b0f2..ee422cbaa 100644 --- a/src/OpenSSL/crypto.py +++ b/src/OpenSSL/crypto.py @@ -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. @@ -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. @@ -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): """ @@ -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): """ @@ -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): """ @@ -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): """ @@ -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): @@ -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): From f4fa8ec772075bdef3ae681483c3c81210252aea Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Mon, 20 Nov 2017 07:28:21 -0500 Subject: [PATCH 2/3] changelog --- CHANGELOG.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 5025cc864..c05ebe888 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -23,8 +23,9 @@ Deprecations: Changes: ^^^^^^^^ -*none* +- Corrected a use-after-free with some uses of the ``X509`` API. + `#709 `_ ---- From 08f0af720a51c0b1d1e170431ff8315d072980a6 Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Mon, 20 Nov 2017 08:47:45 -0500 Subject: [PATCH 3/3] more details --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index c05ebe888..0eb7f815a 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -24,7 +24,7 @@ Changes: ^^^^^^^^ -- Corrected a use-after-free with some uses of the ``X509`` API. +- Corrected a use-after-free when reusing an issuer or subject from an ``X509`` object after the underlying object has been mutated. `#709 `_ ----