From 2ad1938c50f9fe57fe3dd98eb9f4bb711ac52acd Mon Sep 17 00:00:00 2001
From: Tim Costen <tim.costen@isode.com>
Date: Fri, 4 Oct 2019 10:03:59 +0100
Subject: Correct leaks in OpenSSL interface

Remove increment of reference count on first certificate added
to a new SSL context - the call to SSL_CTX_use_certificate does
this internally. When adding extra certificates to the context
via calls to SSL_CTX_add_extra_certificate, the explicit
increment of the reference count is still required to prevent
destruction of the certificates when the SSL context is freed.

In OpenSSLContext::setPrivateKey, make sure the EVP_PKEY returned
by PEM_read_bio_PrivateKey is tidied up, by wrapping it in a
shared_ptr which calls EVP_PKEY_free.

Add a new Unit test which creates an SSL context and inserts a
multi-element certificate chain and a private key.

JIRA: SWIFT-423

Bug:

Release-notes:

Manual:

Change-Id: I82c66139a9dfe7a925eb39f73721200895a689e2
Test-information:
Leak testing performed via ASAN-compiled MLink unit tests -
now no leaks/errors reported associated with TLS Contexts and
Certificates. Swiften unit test runs as expected.

diff --git a/Swiften/QA/TLSTest/CertificateTest.cpp b/Swiften/QA/TLSTest/CertificateTest.cpp
index 21f749c..624d953 100644
--- a/Swiften/QA/TLSTest/CertificateTest.cpp
+++ b/Swiften/QA/TLSTest/CertificateTest.cpp
@@ -15,6 +15,9 @@
 
 #include <Swiften/Base/ByteArray.h>
 #include <Swiften/TLS/CertificateFactory.h>
+#include <Swiften/TLS/TLSContext.h>
+#include <Swiften/TLS/PlatformTLSFactories.h>
+#include <Swiften/TLS/TLSContextFactory.h>
 
 #include <SwifTools/Application/PlatformApplicationPathProvider.h>
 
@@ -31,6 +34,7 @@ class CertificateTest : public CppUnit::TestFixture {
         CPPUNIT_TEST(testGetDNSNames);
         CPPUNIT_TEST(testGetXMPPAddresses);
         CPPUNIT_TEST(testCreateCertificateChain);
+        CPPUNIT_TEST(testCreateTlsContext);
         CPPUNIT_TEST_SUITE_END();
 
     public:
@@ -38,7 +42,11 @@ class CertificateTest : public CppUnit::TestFixture {
             pathProvider = std::make_unique<PlatformApplicationPathProvider>("FileReadBytestreamTest");
             readByteArrayFromFile(certificateData, (pathProvider->getExecutableDir() / "jabber_org.crt"));
             readByteArrayFromFile(chainData, (pathProvider->getExecutableDir() / "certificateChain.pem"));
+            readByteArrayFromFile(keyData, (pathProvider->getExecutableDir() / "privateKey.pem"));
             certificateFactory = std::unique_ptr<CertificateFactory>(new CERTIFICATE_FACTORY());
+
+            PlatformTLSFactories* tlsFactories_ = new PlatformTLSFactories();
+            tlsContextFactory_ = tlsFactories_->getTLSContextFactory();
         }
 
         void testConstructFromDER() {
@@ -106,11 +114,29 @@ class CertificateTest : public CppUnit::TestFixture {
             CPPUNIT_ASSERT_EQUAL(std::string("New Messaging CA"), chain[1]->getCommonNames()[0]);
         }
 
+        void testCreateTlsContext() {
+            // Create 2-certificate chain as in previous test
+            std::vector<std::shared_ptr<Certificate>> chain = certificateFactory->createCertificateChain(chainData);
+            CPPUNIT_ASSERT_EQUAL(2,static_cast<int>(chain.size()));
+
+            // Load private key from string
+            PrivateKey::ref key = certificateFactory->createPrivateKey(Swift::createSafeByteArray(keyData));
+            CPPUNIT_ASSERT(key);
+
+            const TLSOptions options;
+            auto context = tlsContextFactory_->createTLSContext(options, TLSContext::Mode::Server);
+            CPPUNIT_ASSERT(context);
+
+            context->setCertificateChain(chain);
+            context->setPrivateKey(key);
+        }
     private:
         std::unique_ptr<PlatformApplicationPathProvider> pathProvider;
         ByteArray certificateData;
         ByteArray chainData;
+        ByteArray keyData;
         std::unique_ptr<CertificateFactory> certificateFactory;
+        TLSContextFactory* tlsContextFactory_;
 };
 
 #ifdef HAVE_OPENSSL
diff --git a/Swiften/QA/TLSTest/privateKey.pem b/Swiften/QA/TLSTest/privateKey.pem
new file mode 100644
index 0000000..5769000
--- /dev/null
+++ b/Swiften/QA/TLSTest/privateKey.pem
@@ -0,0 +1,40 @@
+-----BEGIN PRIVATE KEY-----
+MIIG/wIBADANBgkqhkiG9w0BAQEFAASCBukwggblAgEAAoIBgQDu1QdEBrcWj+D/
+rkmh++QSu2f0qlJ6Re8dEBtbqpxLiyYZ5IeaLts9szXabfSTchdJr/d0IyHfHQFS
+MGXDphKRaNnV5r//XuojUtorPyGe0DnZR2mp8S9adD7WxNjQLSQabr8PPPW8jrTx
+eJyIvYToLs9lx1IIDcr/3ZTuhBe2FK3Q173M5GF36Jb4yKWLPIfJ6auZjO5G9LZF
+3o2vVWxfc7ESnXvf3sAcWQPR08/ud0vLa3W3A8dC0XGk4BbE32cxvSuzWPHZd257
+HiHIW5pKLZXSMTu7fVNzBzDlo8BYQ5kad1ic+hhyIHBwTUO0Hz3EYP+9FG3TNG84
+65K9EeN3/Qw2P9468GHWAXqlzKFiIGikxYHGwvGd0CdegHtZ/TwIgVvpIDp6XB6U
+ez/TYPEiMCqX8TCIQi8FU3obEouMoPMHKM8vzQdSItZtPn6gD16M4xKdMm6fbvCD
+1okdcrWQnZo72pp8cfpS87KhP5z1ec9B/Wqysh4nrO35v8LXH60CAwEAAQKCAYEA
+jPDUJ9XaqAriWaBtvZTbpB5KG72DjLrGgB0oN/E36PDF3FPbniZ2pTOj3TI0OesD
+SS351uSAsZz5UZpUA6B2pq78llllBnvpqkzTiN/ppEH3UXzuIya8riGZj758wGVT
+P/II+CIeVlbU+wcVQTCuRSKSq9pzU2NoX5RQtmznXUFYzbzzOf2wc0WkCk7GOqPO
+8l3eMXBUkTUKd7L9Y/ICUVYBsh2To6pdLp1tPp9DvtNRvEq/HfCx34GgEg9YAHhg
+1rcPhh71M+TLYHznl5r/Jm1kIVrP3zyr1Bm5DDgZLE3GTN/oFumgXQyFCPyslup1
+gdZzS6W+fbeKxoPzjPOhzHVUxVZ/yqJH1xa1gs4ECQ4QXxdnr7yY1H5k5S8dabO5
+bEvXP+tH95HcAtAbvoRt+NC+xIJ39d6X7X2c4TPLoMIxDxmbEOCi9sg+4Ws+7E2s
+a/01fTZFT+lzuGBdp9Zz/tltDrwfYD0V+Q7qO0o/nJPINI+alAWlqQia00ZyZr4V
+AoHBAPxwCls99/LUzY7IJc0TV3ukk3sFi3rt58u8BE4+RaCtmgPMDj1l+EnaY9RW
+IOj91ECZ8+a23elNPZOkXKuYuJmJIpjOogOMM8r+Q4WF87xoRcdcjPh+PBat66HZ
++8mbm0VQ98cjxs0/kTRRayzz7UG9Onf1PhFfnw55sbMGItVssRDi9lRZJdSRU+CC
+qyAt8TUEH0lo+8AKbRn7xW4VHiD0hmLKDi4F713QLCPgmNlPQ/C60FTIRYS18gzK
+ARhuzwKBwQDyM9YiiFFQ3irGKtbj9W3bDHNmMl9YOHMYVXJAvh83Zcp80qRsShtw
+n3mV3vcVI+KNeZtKFUrJIYNTspNBP/w8U4lGGW+7tAt0dd0WY9m3ygnZg0GOHoaC
+uUusGicZR7FgbYlJzCiRhFhWcFyh0VOrm/k7OjznAvwfWbRKrlLvQdrWrLj7dyN3
+8n9lArq9ZxXJLpBXDUJ1R+F+hPIIIRKeYF2ULUFNE0U9Pj7SVTT7L7jPMWKnrVJh
+U4/hVAEHyMMCgcB4hCTtmpAdZmscl4E0ft9tMA0Y1nTYo2veYEzN7fzf0QGOfoTt
+2xjGaXTvko7zrPsAPH+szfDzyOR08Cst4SOAaXAS89N1TiIL74fc3y6V7FIj85N5
+rwqQ6UdtZdxHS/q9BQLGF9Z5drej+proQywqDmUzj+mp8bTF/GNRzMQkkFeYcEKZ
+0lW1PgyFStzX6BcX8HffXDeUX2Xm2cRP4dUYdqUR1NUgM8UrTI9GMZvHY4hUDVwY
+neRSj2qXoHkVaRECgcEArilkM9S+VF5Nd85aU/WqFzeuy7AxK2j8KmVXEQMlw1oo
+7vUxUsU/Ug77CTAZkFQLlxv49J629kZo/wiMJwFxyZdwQL4NwHXJPud6IZ2Pcz+P
+MZ/WxfFhXCMOLSVpNB5/iA18CVsLWQhH1XBay+mQNvijkVlhbeSRk6GXqZQNAwrh
+6Divk/Opx5jSzrnVulikK9SV6mMYhOk5VxcWS44sq0I0SFb6fAf9Y/qchfbLcExy
+olqqzFQvxtilv6v+SbCtAoHBAOXPUQ7VVuQZo4HA+CaQRYgQjGMxo4jeGiqrUAaX
+b+MpUjU7VxiSrfH3wFxCuMfW7dfQJKp7BAG8PCNzP1eW3+LhPkRSPAT0nwk/bQ5E
+N/n6NBqwsJFoTqueS0qDVdPichwKGvnIrraHSVeMeHZNv+TQdMjmTJ5AfBNCal9b
+7EPTFQO0Tj4GAB77fVRzewyVB+qXccoD2Gts9aWbY9FVGyhkvRenL7CcbgrzLZvt
+php/1crfbWtZ/3Nwz6L8LEdZHA==
+-----END PRIVATE KEY-----
diff --git a/Swiften/TLS/OpenSSL/OpenSSLContext.cpp b/Swiften/TLS/OpenSSL/OpenSSLContext.cpp
index 32d6470..d9560de 100644
--- a/Swiften/TLS/OpenSSL/OpenSSLContext.cpp
+++ b/Swiften/TLS/OpenSSL/OpenSSLContext.cpp
@@ -579,13 +579,11 @@ bool OpenSSLContext::setCertificateChain(const std::vector<std::shared_ptr<Certi
         return false;
     }
 
+    // This increments the reference count on the X509 certificate automatically
     if (SSL_CTX_use_certificate(context_.get(), openSSLCert->getInternalX509().get()) != 1) {
         return false;
     }
 
-    // Increment reference count on certificate so that it does not get freed when the SSL context is destroyed
-    openSSLCert->incrementReferenceCount();
-
     if (certificateChain.size() > 1) {
         for (auto certificate = certificateChain.begin() + 1; certificate != certificateChain.end(); ++certificate) {
             auto openSSLCert = dynamic_cast<OpenSSLCertificate*>(certificate->get());
@@ -597,7 +595,7 @@ bool OpenSSLContext::setCertificateChain(const std::vector<std::shared_ptr<Certi
                 SWIFT_LOG(warning) << "Trying to load empty certificate chain." << std::endl;
                 return false;
             }
-
+            // Have to manually increment reference count as SSL_CTX_add_extra_chain_cert does not do so
             openSSLCert->incrementReferenceCount();
         }
     }
@@ -644,16 +642,17 @@ bool OpenSSLContext::setPrivateKey(const PrivateKey::ref& privateKey) {
         safePassword.push_back(0);
         password = safePassword.data();
     }
-    auto resultKey = PEM_read_bio_PrivateKey(bio.get(), nullptr, empty_or_preset_password_cb, password);
+    // Make sure resultKey is tidied up by wrapping it in a shared_ptr
+    auto resultKey = std::shared_ptr<EVP_PKEY>(PEM_read_bio_PrivateKey(bio.get(), nullptr, empty_or_preset_password_cb, password), EVP_PKEY_free);
     if (resultKey) {
         if (handle_) {
-            auto result = SSL_use_PrivateKey(handle_.get(), resultKey);;
+            auto result = SSL_use_PrivateKey(handle_.get(), resultKey.get());
             if (result != 1) {
                 return false;
             }
         }
         else {
-            auto result = SSL_CTX_use_PrivateKey(context_.get(), resultKey);
+            auto result = SSL_CTX_use_PrivateKey(context_.get(), resultKey.get());
             if (result != 1) {
                 return false;
             }
-- 
cgit v0.10.2-6-g49f6