From e73818600065821d588af475b024f4eb518c3509 Mon Sep 17 00:00:00 2001
From: Paul Kehrer <paul.l.kehrer@gmail.com>
Date: Thu, 30 Nov 2017 20:55:25 +0800
Subject: [PATCH] fix a memory leak and a potential UAF and also #722 (#723)

* fix a memory leak and a potential UAF and also #722

* sanity check

* bump cryptography minimum version, add changelog
---
 CHANGELOG.rst         |  6 +++---
 setup.py              |  2 +-
 src/OpenSSL/SSL.py    |  5 +++--
 src/OpenSSL/crypto.py |  7 +++----
 tests/test_ssl.py     | 25 +++++++++++++++++++++++++
 tox.ini               |  2 +-
 6 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index abf3efa1e..3f5590fce 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -11,7 +11,7 @@ The third digit is only for regressions.
 Backward-incompatible changes:
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-*none*
+* The minimum ``cryptography`` version is now 2.1.4.
 
 
 Deprecations:
@@ -23,8 +23,8 @@ Deprecations:
 Changes:
 ^^^^^^^^
 
-
-*none*
+- Fixed a potential use-after-free in the verify callback and resolved a memory leak when loading PKCS12 files with ``cacerts``.
+  `#723 <https://github.com/pyca/pyopenssl/pull/723>`_
 
 ----
 
diff --git a/setup.py b/setup.py
index 937d556ce..85252e947 100755
--- a/setup.py
+++ b/setup.py
@@ -95,7 +95,7 @@ def find_meta(meta):
         package_dir={"": "src"},
         install_requires=[
             # Fix cryptographyMinimum in tox.ini when changing this!
-            "cryptography>=1.9",
+            "cryptography>=2.1.4",
             "six>=1.5.2"
         ],
         extras_require={
diff --git a/src/OpenSSL/SSL.py b/src/OpenSSL/SSL.py
index 32c038ab0..ec338145e 100644
--- a/src/OpenSSL/SSL.py
+++ b/src/OpenSSL/SSL.py
@@ -309,8 +309,9 @@ def __init__(self, callback):
 
         @wraps(callback)
         def wrapper(ok, store_ctx):
-            cert = X509.__new__(X509)
-            cert._x509 = _lib.X509_STORE_CTX_get_current_cert(store_ctx)
+            x509 = _lib.X509_STORE_CTX_get_current_cert(store_ctx)
+            _lib.X509_up_ref(x509)
+            cert = X509._from_raw_x509_ptr(x509)
             error_number = _lib.X509_STORE_CTX_get_error(store_ctx)
             error_depth = _lib.X509_STORE_CTX_get_error_depth(store_ctx)
 
diff --git a/src/OpenSSL/crypto.py b/src/OpenSSL/crypto.py
index ecd055e15..12b4db0db 100644
--- a/src/OpenSSL/crypto.py
+++ b/src/OpenSSL/crypto.py
@@ -3058,8 +3058,7 @@ def load_pkcs12(buffer, passphrase=None):
         pycert = None
         friendlyname = None
     else:
-        pycert = X509.__new__(X509)
-        pycert._x509 = _ffi.gc(cert[0], _lib.X509_free)
+        pycert = X509._from_raw_x509_ptr(cert[0])
 
         friendlyname_length = _ffi.new("int*")
         friendlyname_buffer = _lib.X509_alias_get0(
@@ -3073,8 +3072,8 @@ def load_pkcs12(buffer, passphrase=None):
 
     pycacerts = []
     for i in range(_lib.sk_X509_num(cacerts)):
-        pycacert = X509.__new__(X509)
-        pycacert._x509 = _lib.sk_X509_value(cacerts, i)
+        x509 = _lib.sk_X509_value(cacerts, i)
+        pycacert = X509._from_raw_x509_ptr(x509)
         pycacerts.append(pycacert)
     if not pycacerts:
         pycacerts = None
diff --git a/tests/test_ssl.py b/tests/test_ssl.py
index 03f9abdae..76d8c4d9e 100644
--- a/tests/test_ssl.py
+++ b/tests/test_ssl.py
@@ -1279,6 +1279,31 @@ def callback(self, connection, *args):
 
         assert verify.connection is clientConnection
 
+    def test_x509_in_verify_works(self):
+        """
+        We had a bug where the X509 cert instantiated in the callback wrapper
+        didn't __init__ so it was missing objects needed when calling
+        get_subject. This test sets up a handshake where we call get_subject
+        on the cert provided to the verify callback.
+        """
+        serverContext = Context(TLSv1_METHOD)
+        serverContext.use_privatekey(
+            load_privatekey(FILETYPE_PEM, cleartextPrivateKeyPEM))
+        serverContext.use_certificate(
+            load_certificate(FILETYPE_PEM, cleartextCertificatePEM))
+        serverConnection = Connection(serverContext, None)
+
+        def verify_cb_get_subject(conn, cert, errnum, depth, ok):
+            assert cert.get_subject()
+            return 1
+
+        clientContext = Context(TLSv1_METHOD)
+        clientContext.set_verify(VERIFY_PEER, verify_cb_get_subject)
+        clientConnection = Connection(clientContext, None)
+        clientConnection.set_connect_state()
+
+        handshake_in_memory(clientConnection, serverConnection)
+
     def test_set_verify_callback_exception(self):
         """
         If the verify callback passed to `Context.set_verify` raises an
diff --git a/tox.ini b/tox.ini
index a16e69015..bcb2f480c 100644
--- a/tox.ini
+++ b/tox.ini
@@ -10,7 +10,7 @@ extras =
 deps =
     coverage>=4.2
     cryptographyMaster: git+https://github.com/pyca/cryptography.git
-    cryptographyMinimum: cryptography<=1.9
+    cryptographyMinimum: cryptography==2.1.4
 setenv =
     # Do not allow the executing environment to pollute the test environment
     # with extra packages.