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

@@ -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>`_
 
 ----
 

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={

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)
 

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

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

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.