From 915d1d82c0fed8c656d1104b71728c1cf9747ede Mon Sep 17 00:00:00 2001 From: Paul Kehrer Date: Wed, 29 Nov 2017 18:20:33 +0800 Subject: [PATCH 1/3] fix a memory leak and a potential UAF and also #722 --- src/OpenSSL/SSL.py | 6 ++++-- src/OpenSSL/crypto.py | 7 +++---- tests/test_ssl.py | 25 +++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/OpenSSL/SSL.py b/src/OpenSSL/SSL.py index 32c038ab0..ec7c36385 100644 --- a/src/OpenSSL/SSL.py +++ b/src/OpenSSL/SSL.py @@ -309,8 +309,10 @@ 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) + res = _lib.X509_up_ref(x509) + _openssl_assert(res == 1) + 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 From cef7f41f4c8e2815d01313d908c72dd6b71f3cbb Mon Sep 17 00:00:00 2001 From: Paul Kehrer Date: Wed, 29 Nov 2017 18:51:59 +0800 Subject: [PATCH 2/3] sanity check --- src/OpenSSL/SSL.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/OpenSSL/SSL.py b/src/OpenSSL/SSL.py index ec7c36385..ec338145e 100644 --- a/src/OpenSSL/SSL.py +++ b/src/OpenSSL/SSL.py @@ -310,8 +310,7 @@ def __init__(self, callback): @wraps(callback) def wrapper(ok, store_ctx): x509 = _lib.X509_STORE_CTX_get_current_cert(store_ctx) - res = _lib.X509_up_ref(x509) - _openssl_assert(res == 1) + _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) From 4f0b2fa18e8dc77721c3cc11c058855e9e585b8a Mon Sep 17 00:00:00 2001 From: Paul Kehrer Date: Thu, 30 Nov 2017 10:36:28 +0800 Subject: [PATCH 3/3] bump cryptography minimum version, add changelog --- CHANGELOG.rst | 6 +++--- setup.py | 2 +- tox.ini | 2 +- 3 files changed, 5 insertions(+), 5 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 `_ ---- 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/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.