Swift
summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorTim Costen <tim.costen@isode.com>2019-10-04 09:03:59 (GMT)
committerTim Costen <tim.costen@isode.com>2019-10-04 12:25:33 (GMT)
commit2ad1938c50f9fe57fe3dd98eb9f4bb711ac52acd (patch)
treec18d0317b1f750bad3d413ed5bc6ec40a2e0bfbb
parentdf07a5e1e654c5fe4b513b8b0e41a392e9955cdf (diff)
downloadswift-2ad1938c50f9fe57fe3dd98eb9f4bb711ac52acd.zip
swift-2ad1938c50f9fe57fe3dd98eb9f4bb711ac52acd.tar.bz2
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.
Diffstat
-rw-r--r--Swiften/QA/TLSTest/CertificateTest.cpp26
-rw-r--r--Swiften/QA/TLSTest/privateKey.pem40
-rw-r--r--Swiften/TLS/OpenSSL/OpenSSLContext.cpp13
3 files changed, 72 insertions, 7 deletions
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 @@
15 15
16#include <Swiften/Base/ByteArray.h> 16#include <Swiften/Base/ByteArray.h>
17#include <Swiften/TLS/CertificateFactory.h> 17#include <Swiften/TLS/CertificateFactory.h>
18#include <Swiften/TLS/TLSContext.h>
19#include <Swiften/TLS/PlatformTLSFactories.h>
20#include <Swiften/TLS/TLSContextFactory.h>
18 21
19#include <SwifTools/Application/PlatformApplicationPathProvider.h> 22#include <SwifTools/Application/PlatformApplicationPathProvider.h>
20 23
@@ -31,6 +34,7 @@ class CertificateTest : public CppUnit::TestFixture {
31 CPPUNIT_TEST(testGetDNSNames); 34 CPPUNIT_TEST(testGetDNSNames);
32 CPPUNIT_TEST(testGetXMPPAddresses); 35 CPPUNIT_TEST(testGetXMPPAddresses);
33 CPPUNIT_TEST(testCreateCertificateChain); 36 CPPUNIT_TEST(testCreateCertificateChain);
37 CPPUNIT_TEST(testCreateTlsContext);
34 CPPUNIT_TEST_SUITE_END(); 38 CPPUNIT_TEST_SUITE_END();
35 39
36 public: 40 public:
@@ -38,7 +42,11 @@ class CertificateTest : public CppUnit::TestFixture {
38 pathProvider = std::make_unique<PlatformApplicationPathProvider>("FileReadBytestreamTest"); 42 pathProvider = std::make_unique<PlatformApplicationPathProvider>("FileReadBytestreamTest");
39 readByteArrayFromFile(certificateData, (pathProvider->getExecutableDir() / "jabber_org.crt")); 43 readByteArrayFromFile(certificateData, (pathProvider->getExecutableDir() / "jabber_org.crt"));
40 readByteArrayFromFile(chainData, (pathProvider->getExecutableDir() / "certificateChain.pem")); 44 readByteArrayFromFile(chainData, (pathProvider->getExecutableDir() / "certificateChain.pem"));
45 readByteArrayFromFile(keyData, (pathProvider->getExecutableDir() / "privateKey.pem"));
41 certificateFactory = std::unique_ptr<CertificateFactory>(new CERTIFICATE_FACTORY()); 46 certificateFactory = std::unique_ptr<CertificateFactory>(new CERTIFICATE_FACTORY());
47
48 PlatformTLSFactories* tlsFactories_ = new PlatformTLSFactories();
49 tlsContextFactory_ = tlsFactories_->getTLSContextFactory();
42 } 50 }
43 51
44 void testConstructFromDER() { 52 void testConstructFromDER() {
@@ -106,11 +114,29 @@ class CertificateTest : public CppUnit::TestFixture {
106 CPPUNIT_ASSERT_EQUAL(std::string("New Messaging CA"), chain[1]->getCommonNames()[0]); 114 CPPUNIT_ASSERT_EQUAL(std::string("New Messaging CA"), chain[1]->getCommonNames()[0]);
107 } 115 }
108 116
117 void testCreateTlsContext() {
118 // Create 2-certificate chain as in previous test
119 std::vector<std::shared_ptr<Certificate>> chain = certificateFactory->createCertificateChain(chainData);
120 CPPUNIT_ASSERT_EQUAL(2,static_cast<int>(chain.size()));
121
122 // Load private key from string
123 PrivateKey::ref key = certificateFactory->createPrivateKey(Swift::createSafeByteArray(keyData));
124 CPPUNIT_ASSERT(key);
125
126 const TLSOptions options;
127 auto context = tlsContextFactory_->createTLSContext(options, TLSContext::Mode::Server);
128 CPPUNIT_ASSERT(context);
129
130 context->setCertificateChain(chain);
131 context->setPrivateKey(key);
132 }
109 private: 133 private:
110 std::unique_ptr<PlatformApplicationPathProvider> pathProvider; 134 std::unique_ptr<PlatformApplicationPathProvider> pathProvider;
111 ByteArray certificateData; 135 ByteArray certificateData;
112 ByteArray chainData; 136 ByteArray chainData;
137 ByteArray keyData;
113 std::unique_ptr<CertificateFactory> certificateFactory; 138 std::unique_ptr<CertificateFactory> certificateFactory;
139 TLSContextFactory* tlsContextFactory_;
114}; 140};
115 141
116#ifdef HAVE_OPENSSL 142#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 @@
1-----BEGIN PRIVATE KEY-----
2MIIG/wIBADANBgkqhkiG9w0BAQEFAASCBukwggblAgEAAoIBgQDu1QdEBrcWj+D/
3rkmh++QSu2f0qlJ6Re8dEBtbqpxLiyYZ5IeaLts9szXabfSTchdJr/d0IyHfHQFS
4MGXDphKRaNnV5r//XuojUtorPyGe0DnZR2mp8S9adD7WxNjQLSQabr8PPPW8jrTx
5eJyIvYToLs9lx1IIDcr/3ZTuhBe2FK3Q173M5GF36Jb4yKWLPIfJ6auZjO5G9LZF
63o2vVWxfc7ESnXvf3sAcWQPR08/ud0vLa3W3A8dC0XGk4BbE32cxvSuzWPHZd257
7HiHIW5pKLZXSMTu7fVNzBzDlo8BYQ5kad1ic+hhyIHBwTUO0Hz3EYP+9FG3TNG84
865K9EeN3/Qw2P9468GHWAXqlzKFiIGikxYHGwvGd0CdegHtZ/TwIgVvpIDp6XB6U
9ez/TYPEiMCqX8TCIQi8FU3obEouMoPMHKM8vzQdSItZtPn6gD16M4xKdMm6fbvCD
101okdcrWQnZo72pp8cfpS87KhP5z1ec9B/Wqysh4nrO35v8LXH60CAwEAAQKCAYEA
11jPDUJ9XaqAriWaBtvZTbpB5KG72DjLrGgB0oN/E36PDF3FPbniZ2pTOj3TI0OesD
12SS351uSAsZz5UZpUA6B2pq78llllBnvpqkzTiN/ppEH3UXzuIya8riGZj758wGVT
13P/II+CIeVlbU+wcVQTCuRSKSq9pzU2NoX5RQtmznXUFYzbzzOf2wc0WkCk7GOqPO
148l3eMXBUkTUKd7L9Y/ICUVYBsh2To6pdLp1tPp9DvtNRvEq/HfCx34GgEg9YAHhg
151rcPhh71M+TLYHznl5r/Jm1kIVrP3zyr1Bm5DDgZLE3GTN/oFumgXQyFCPyslup1
16gdZzS6W+fbeKxoPzjPOhzHVUxVZ/yqJH1xa1gs4ECQ4QXxdnr7yY1H5k5S8dabO5
17bEvXP+tH95HcAtAbvoRt+NC+xIJ39d6X7X2c4TPLoMIxDxmbEOCi9sg+4Ws+7E2s
18a/01fTZFT+lzuGBdp9Zz/tltDrwfYD0V+Q7qO0o/nJPINI+alAWlqQia00ZyZr4V
19AoHBAPxwCls99/LUzY7IJc0TV3ukk3sFi3rt58u8BE4+RaCtmgPMDj1l+EnaY9RW
20IOj91ECZ8+a23elNPZOkXKuYuJmJIpjOogOMM8r+Q4WF87xoRcdcjPh+PBat66HZ
21+8mbm0VQ98cjxs0/kTRRayzz7UG9Onf1PhFfnw55sbMGItVssRDi9lRZJdSRU+CC
22qyAt8TUEH0lo+8AKbRn7xW4VHiD0hmLKDi4F713QLCPgmNlPQ/C60FTIRYS18gzK
23ARhuzwKBwQDyM9YiiFFQ3irGKtbj9W3bDHNmMl9YOHMYVXJAvh83Zcp80qRsShtw
24n3mV3vcVI+KNeZtKFUrJIYNTspNBP/w8U4lGGW+7tAt0dd0WY9m3ygnZg0GOHoaC
25uUusGicZR7FgbYlJzCiRhFhWcFyh0VOrm/k7OjznAvwfWbRKrlLvQdrWrLj7dyN3
268n9lArq9ZxXJLpBXDUJ1R+F+hPIIIRKeYF2ULUFNE0U9Pj7SVTT7L7jPMWKnrVJh
27U4/hVAEHyMMCgcB4hCTtmpAdZmscl4E0ft9tMA0Y1nTYo2veYEzN7fzf0QGOfoTt
282xjGaXTvko7zrPsAPH+szfDzyOR08Cst4SOAaXAS89N1TiIL74fc3y6V7FIj85N5
29rwqQ6UdtZdxHS/q9BQLGF9Z5drej+proQywqDmUzj+mp8bTF/GNRzMQkkFeYcEKZ
300lW1PgyFStzX6BcX8HffXDeUX2Xm2cRP4dUYdqUR1NUgM8UrTI9GMZvHY4hUDVwY
31neRSj2qXoHkVaRECgcEArilkM9S+VF5Nd85aU/WqFzeuy7AxK2j8KmVXEQMlw1oo
327vUxUsU/Ug77CTAZkFQLlxv49J629kZo/wiMJwFxyZdwQL4NwHXJPud6IZ2Pcz+P
33MZ/WxfFhXCMOLSVpNB5/iA18CVsLWQhH1XBay+mQNvijkVlhbeSRk6GXqZQNAwrh
346Divk/Opx5jSzrnVulikK9SV6mMYhOk5VxcWS44sq0I0SFb6fAf9Y/qchfbLcExy
35olqqzFQvxtilv6v+SbCtAoHBAOXPUQ7VVuQZo4HA+CaQRYgQjGMxo4jeGiqrUAaX
36b+MpUjU7VxiSrfH3wFxCuMfW7dfQJKp7BAG8PCNzP1eW3+LhPkRSPAT0nwk/bQ5E
37N/n6NBqwsJFoTqueS0qDVdPichwKGvnIrraHSVeMeHZNv+TQdMjmTJ5AfBNCal9b
387EPTFQO0Tj4GAB77fVRzewyVB+qXccoD2Gts9aWbY9FVGyhkvRenL7CcbgrzLZvt
39php/1crfbWtZ/3Nwz6L8LEdZHA==
40-----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
579 return false; 579 return false;
580 } 580 }
581 581
582 // This increments the reference count on the X509 certificate automatically
582 if (SSL_CTX_use_certificate(context_.get(), openSSLCert->getInternalX509().get()) != 1) { 583 if (SSL_CTX_use_certificate(context_.get(), openSSLCert->getInternalX509().get()) != 1) {
583 return false; 584 return false;
584 } 585 }
585 586
586 // Increment reference count on certificate so that it does not get freed when the SSL context is destroyed
587 openSSLCert->incrementReferenceCount();
588
589 if (certificateChain.size() > 1) { 587 if (certificateChain.size() > 1) {
590 for (auto certificate = certificateChain.begin() + 1; certificate != certificateChain.end(); ++certificate) { 588 for (auto certificate = certificateChain.begin() + 1; certificate != certificateChain.end(); ++certificate) {
591 auto openSSLCert = dynamic_cast<OpenSSLCertificate*>(certificate->get()); 589 auto openSSLCert = dynamic_cast<OpenSSLCertificate*>(certificate->get());
@@ -597,7 +595,7 @@ bool OpenSSLContext::setCertificateChain(const std::vector<std::shared_ptr<Certi
597 SWIFT_LOG(warning) << "Trying to load empty certificate chain." << std::endl; 595 SWIFT_LOG(warning) << "Trying to load empty certificate chain." << std::endl;
598 return false; 596 return false;
599 } 597 }
600 598 // Have to manually increment reference count as SSL_CTX_add_extra_chain_cert does not do so
601 openSSLCert->incrementReferenceCount(); 599 openSSLCert->incrementReferenceCount();
602 } 600 }
603 } 601 }
@@ -644,16 +642,17 @@ bool OpenSSLContext::setPrivateKey(const PrivateKey::ref& privateKey) {
644 safePassword.push_back(0); 642 safePassword.push_back(0);
645 password = safePassword.data(); 643 password = safePassword.data();
646 } 644 }
647 auto resultKey = PEM_read_bio_PrivateKey(bio.get(), nullptr, empty_or_preset_password_cb, password); 645 // Make sure resultKey is tidied up by wrapping it in a shared_ptr
646 auto resultKey = std::shared_ptr<EVP_PKEY>(PEM_read_bio_PrivateKey(bio.get(), nullptr, empty_or_preset_password_cb, password), EVP_PKEY_free);
648 if (resultKey) { 647 if (resultKey) {
649 if (handle_) { 648 if (handle_) {
650 auto result = SSL_use_PrivateKey(handle_.get(), resultKey);; 649 auto result = SSL_use_PrivateKey(handle_.get(), resultKey.get());
651 if (result != 1) { 650 if (result != 1) {
652 return false; 651 return false;
653 } 652 }
654 } 653 }
655 else { 654 else {
656 auto result = SSL_CTX_use_PrivateKey(context_.get(), resultKey); 655 auto result = SSL_CTX_use_PrivateKey(context_.get(), resultKey.get());
657 if (result != 1) { 656 if (result != 1) {
658 return false; 657 return false;
659 } 658 }