Allow use of system TAs to be disabled via TLSOptions

Add new boolean flag to TLSOptions which when set to true prevents system Trust Anchors being loaded into new TLS contexts created using OpenSSL. Add new test to Swiften QA with appropriate comment. JIRA: SWIFT-425 Test-information: Checked logic of change under debugger while running the tests in CertificateTest.cpp which create TLS contexts. Change-Id: I2d4a8410ce9cc752e6774e1d1cdb84dcd37b01d7
author: Tim Costen <tim.costen@isode.com> 2019-10-28 11:15:57 (GMT)
committer: Tim Costen <tim.costen@isode.com> 2019-10-29 11:35:05 (GMT)
commit: be7632881677da5267eb711c1f2823ac82d43d09 (patch)
tree: 9088f5433f24017a4729becf03a0ca871b71a90f
parent: 8e0a9cd6a608ee2bf83b52c9eb9ac556bf10293f (diff)
download: swift-be7632881677da5267eb711c1f2823ac82d43d09.zip
swift-be7632881677da5267eb711c1f2823ac82d43d09.tar.bz2
3 files changed, 72 insertions, 38 deletions
diff --git a/Swiften/QA/TLSTest/CertificateTest.cpp b/Swiften/QA/TLSTest/CertificateTest.cpp
index 624d953..463ef9e 100644
--- a/Swiften/QA/TLSTest/CertificateTest.cpp
+++ b/Swiften/QA/TLSTest/CertificateTest.cpp
@@ -35,6 +35,7 @@ class CertificateTest : public CppUnit::TestFixture {
        CPPUNIT_TEST(testGetXMPPAddresses);
        CPPUNIT_TEST(testCreateCertificateChain);
        CPPUNIT_TEST(testCreateTlsContext);
+        CPPUNIT_TEST(testCreateTlsContextDisableSystemTAs);
        CPPUNIT_TEST_SUITE_END();
    public:
@@ -130,6 +131,29 @@ class CertificateTest : public CppUnit::TestFixture {
            context->setCertificateChain(chain);
            context->setPrivateKey(key);
        }
+    /**
+     * This test does not actually verify that use of system TAs has been disabled, it just provides
+     * a convenient mechanism for testing via a debugger.
+     **/
+        void testCreateTlsContextDisableSystemTAs() {
+            // 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);
+            // Turn off use of system TAs
+            TLSOptions options;
+            options.ignoreSystemTrustAnchors = true;
+            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;
diff --git a/Swiften/TLS/OpenSSL/OpenSSLContext.cpp b/Swiften/TLS/OpenSSL/OpenSSLContext.cpp
index 490a361..b7cf178 100644
--- a/Swiften/TLS/OpenSSL/OpenSSLContext.cpp
+++ b/Swiften/TLS/OpenSSL/OpenSSLContext.cpp
@@ -121,52 +121,57 @@ OpenSSLContext::OpenSSLContext(const TLSOptions& options, Mode mode) : mode_(mod
    // TODO: implement OCSP support
    // TODO: handle OCSP stapling see https://www.rfc-editor.org/rfc/rfc4366.txt
-    // Load system certs
+    // Default for ignoreSystemTrustAnchors is false, i.e. load System TAs by default,
+    // to preserve previous behaviour
+    if (!options.ignoreSystemTrustAnchors) {
+        // Load system certs
 #if defined(SWIFTEN_PLATFORM_WINDOWS)
-    X509_STORE* store = SSL_CTX_get_cert_store(context_.get());
+        X509_STORE* store = SSL_CTX_get_cert_store(context_.get());
-    HCERTSTORE systemStore = CertOpenSystemStore(0, "ROOT");
+        HCERTSTORE systemStore = CertOpenSystemStore(0, "ROOT");
-    if (systemStore) {
+        if (systemStore) {
-        PCCERT_CONTEXT certContext = nullptr;
+            PCCERT_CONTEXT certContext = nullptr;
-        while (true) {
+            while (true) {
-            certContext = CertFindCertificateInStore(systemStore, X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, 0, CERT_FIND_ANY, nullptr, certContext);
+                certContext = CertFindCertificateInStore(systemStore, X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, 0, CERT_FIND_ANY, nullptr, certContext);
-            if (!certContext) {
+                if (!certContext) {
-                break;
+                    break;
-            }
+                }
-            OpenSSLCertificate cert(createByteArray(certContext->pbCertEncoded, certContext->cbCertEncoded));
+                OpenSSLCertificate cert(createByteArray(certContext->pbCertEncoded, certContext->cbCertEncoded));
-            if (store && cert.getInternalX509()) {
+                if (store && cert.getInternalX509()) {
-                X509_STORE_add_cert(store, cert.getInternalX509().get());
+                    X509_STORE_add_cert(store, cert.getInternalX509().get());
+                }
            }
        }
-    }
 #elif !defined(SWIFTEN_PLATFORM_MACOSX)
-    SSL_CTX_set_default_verify_paths(context_.get());
+        SSL_CTX_set_default_verify_paths(context_.get());
 #elif defined(SWIFTEN_PLATFORM_MACOSX) && !defined(SWIFTEN_PLATFORM_IPHONE)
-    // On Mac OS X 10.5 (OpenSSL < 0.9.8), OpenSSL does not automatically look in the system store.
+        // On Mac OS X 10.5 (OpenSSL < 0.9.8), OpenSSL does not automatically look in the system store.
-    // On Mac OS X 10.6 (OpenSSL >= 0.9.8), OpenSSL *does* look in the system store to determine trust.
+        // On Mac OS X 10.6 (OpenSSL >= 0.9.8), OpenSSL *does* look in the system store to determine trust.
-    // However, if there is a certificate error, it will always emit the "Invalid CA" error if we didn't add
+        // However, if there is a certificate error, it will always emit the "Invalid CA" error if we didn't add
-    // the certificates first. See
+        // the certificates first. See
-    //        http://opensource.apple.com/source/OpenSSL098/OpenSSL098-27/src/crypto/x509/x509_vfy_apple.c
+        //        http://opensource.apple.com/source/OpenSSL098/OpenSSL098-27/src/crypto/x509/x509_vfy_apple.c
-    // to understand why. We therefore add all certs from the system store ourselves.
+        // to understand why. We therefore add all certs from the system store ourselves.
-    X509_STORE* store = SSL_CTX_get_cert_store(context_.get());
+        X509_STORE* store = SSL_CTX_get_cert_store(context_.get());
-    CFArrayRef anchorCertificates;
+        CFArrayRef anchorCertificates;
-    if (SecTrustCopyAnchorCertificates(&anchorCertificates) == 0) {
+        if (SecTrustCopyAnchorCertificates(&anchorCertificates) == 0) {
-        for (int i = 0; i < CFArrayGetCount(anchorCertificates); ++i) {
+            for (int i = 0; i < CFArrayGetCount(anchorCertificates); ++i) {
-            SecCertificateRef cert = reinterpret_cast<SecCertificateRef>(const_cast<void*>(CFArrayGetValueAtIndex(anchorCertificates, i)));
+                SecCertificateRef cert = reinterpret_cast<SecCertificateRef>(const_cast<void*>(CFArrayGetValueAtIndex(anchorCertificates, i)));
-            CSSM_DATA certCSSMData;
+                CSSM_DATA certCSSMData;
-            if (SecCertificateGetData(cert, &certCSSMData) != 0 || certCSSMData.Length == 0) {
+                if (SecCertificateGetData(cert, &certCSSMData) != 0 || certCSSMData.Length == 0) {
-                continue;
+                    continue;
-            }
+                }
-            std::vector<unsigned char> certData;
+                std::vector<unsigned char> certData;
-            certData.resize(certCSSMData.Length);
+                certData.resize(certCSSMData.Length);
-            memcpy(&certData[0], certCSSMData.Data, certCSSMData.Length);
+                memcpy(&certData[0], certCSSMData.Data, certCSSMData.Length);
-            OpenSSLCertificate certificate(certData);
+                OpenSSLCertificate certificate(certData);
-            if (store && certificate.getInternalX509()) {
+                if (store && certificate.getInternalX509()) {
-                X509_STORE_add_cert(store, certificate.getInternalX509().get());
+                    X509_STORE_add_cert(store, certificate.getInternalX509().get());
+                }
            }
+            CFRelease(anchorCertificates);
        }
-        CFRelease(anchorCertificates);
-    }
 #endif
+    }
    configure(options);
 }
diff --git a/Swiften/TLS/TLSOptions.h b/Swiften/TLS/TLSOptions.h
index 4109096..e3faaf9 100644
--- a/Swiften/TLS/TLSOptions.h
+++ b/Swiften/TLS/TLSOptions.h
@@ -68,5 +68,10 @@ namespace Swift {
         * Allows specification of application-specific Trust Anchors
         */
        boost::optional<std::vector<std::shared_ptr<Certificate>>> trustAnchors;
+        /**
+         * Turns off automatic loading of system Trust Anchors
+         */
+        bool ignoreSystemTrustAnchors = false;
    };
 }
author	Tim Costen <tim.costen@isode.com>	2019-10-28 11:15:57 (GMT)
committer	Tim Costen <tim.costen@isode.com>	2019-10-29 11:35:05 (GMT)
commit	be7632881677da5267eb711c1f2823ac82d43d09 (patch)
tree	9088f5433f24017a4729becf03a0ca871b71a90f
parent	8e0a9cd6a608ee2bf83b52c9eb9ac556bf10293f (diff)
download	swift-be7632881677da5267eb711c1f2823ac82d43d09.zip swift-be7632881677da5267eb711c1f2823ac82d43d09.tar.bz2

diff --git a/Swiften/QA/TLSTest/CertificateTest.cpp b/Swiften/QA/TLSTest/CertificateTest.cpp index 624d953..463ef9e 100644 --- a/Swiften/QA/TLSTest/CertificateTest.cpp +++ b/Swiften/QA/TLSTest/CertificateTest.cpp
@@ -35,6 +35,7 @@ class CertificateTest : public CppUnit::TestFixture {
35	CPPUNIT_TEST(testGetXMPPAddresses);	35	CPPUNIT_TEST(testGetXMPPAddresses);
36	CPPUNIT_TEST(testCreateCertificateChain);	36	CPPUNIT_TEST(testCreateCertificateChain);
37	CPPUNIT_TEST(testCreateTlsContext);	37	CPPUNIT_TEST(testCreateTlsContext);
		38	CPPUNIT_TEST(testCreateTlsContextDisableSystemTAs);
38	CPPUNIT_TEST_SUITE_END();	39	CPPUNIT_TEST_SUITE_END();
39		40
40	public:	41	public:
@@ -130,6 +131,29 @@ class CertificateTest : public CppUnit::TestFixture {
130	context->setCertificateChain(chain);	131	context->setCertificateChain(chain);
131	context->setPrivateKey(key);	132	context->setPrivateKey(key);
132	}	133	}
		134
		135	/**
		136	* This test does not actually verify that use of system TAs has been disabled, it just provides
		137	* a convenient mechanism for testing via a debugger.
		138	**/
		139	void testCreateTlsContextDisableSystemTAs() {
		140	// Create 2-certificate chain as in previous test
		141	std::vector<std::shared_ptr<Certificate>> chain = certificateFactory->createCertificateChain(chainData);
		142	CPPUNIT_ASSERT_EQUAL(2,static_cast<int>(chain.size()));
		143
		144	// Load private key from string
		145	PrivateKey::ref key = certificateFactory->createPrivateKey(Swift::createSafeByteArray(keyData));
		146	CPPUNIT_ASSERT(key);
		147
		148	// Turn off use of system TAs
		149	TLSOptions options;
		150	options.ignoreSystemTrustAnchors = true;
		151	auto context = tlsContextFactory_->createTLSContext(options, TLSContext::Mode::Server);
		152	CPPUNIT_ASSERT(context);
		153
		154	context->setCertificateChain(chain);
		155	context->setPrivateKey(key);
		156	}
133	private:	157	private:
134	std::unique_ptr<PlatformApplicationPathProvider> pathProvider;	158	std::unique_ptr<PlatformApplicationPathProvider> pathProvider;
135	ByteArray certificateData;	159	ByteArray certificateData;


diff --git a/Swiften/TLS/OpenSSL/OpenSSLContext.cpp b/Swiften/TLS/OpenSSL/OpenSSLContext.cpp index 490a361..b7cf178 100644 --- a/Swiften/TLS/OpenSSL/OpenSSLContext.cpp +++ b/Swiften/TLS/OpenSSL/OpenSSLContext.cpp
@@ -121,52 +121,57 @@ OpenSSLContext::OpenSSLContext(const TLSOptions& options, Mode mode) : mode_(mod
121		121
122	// TODO: implement OCSP support	122	// TODO: implement OCSP support
123	// TODO: handle OCSP stapling see https://www.rfc-editor.org/rfc/rfc4366.txt	123	// TODO: handle OCSP stapling see https://www.rfc-editor.org/rfc/rfc4366.txt
124	// Load system certs	124
		125	// Default for ignoreSystemTrustAnchors is false, i.e. load System TAs by default,
		126	// to preserve previous behaviour
		127	if (!options.ignoreSystemTrustAnchors) {
		128	// Load system certs
125	#if defined(SWIFTEN_PLATFORM_WINDOWS)	129	#if defined(SWIFTEN_PLATFORM_WINDOWS)
126	X509_STORE* store = SSL_CTX_get_cert_store(context_.get());	130	X509_STORE* store = SSL_CTX_get_cert_store(context_.get());
127	HCERTSTORE systemStore = CertOpenSystemStore(0, "ROOT");	131	HCERTSTORE systemStore = CertOpenSystemStore(0, "ROOT");
128	if (systemStore) {	132	if (systemStore) {
129	PCCERT_CONTEXT certContext = nullptr;	133	PCCERT_CONTEXT certContext = nullptr;
130	while (true) {	134	while (true) {
131	certContext = CertFindCertificateInStore(systemStore, X509_ASN_ENCODING \| PKCS_7_ASN_ENCODING, 0, CERT_FIND_ANY, nullptr, certContext);	135	certContext = CertFindCertificateInStore(systemStore, X509_ASN_ENCODING \| PKCS_7_ASN_ENCODING, 0, CERT_FIND_ANY, nullptr, certContext);
132	if (!certContext) {	136	if (!certContext) {
133	break;	137	break;
134	}	138	}
135	OpenSSLCertificate cert(createByteArray(certContext->pbCertEncoded, certContext->cbCertEncoded));	139	OpenSSLCertificate cert(createByteArray(certContext->pbCertEncoded, certContext->cbCertEncoded));
136	if (store && cert.getInternalX509()) {	140	if (store && cert.getInternalX509()) {
137	X509_STORE_add_cert(store, cert.getInternalX509().get());	141	X509_STORE_add_cert(store, cert.getInternalX509().get());
		142	}
138	}	143	}
139	}	144	}
140	}
141	#elif !defined(SWIFTEN_PLATFORM_MACOSX)	145	#elif !defined(SWIFTEN_PLATFORM_MACOSX)
142	SSL_CTX_set_default_verify_paths(context_.get());	146	SSL_CTX_set_default_verify_paths(context_.get());
143	#elif defined(SWIFTEN_PLATFORM_MACOSX) && !defined(SWIFTEN_PLATFORM_IPHONE)	147	#elif defined(SWIFTEN_PLATFORM_MACOSX) && !defined(SWIFTEN_PLATFORM_IPHONE)
144	// On Mac OS X 10.5 (OpenSSL < 0.9.8), OpenSSL does not automatically look in the system store.	148	// On Mac OS X 10.5 (OpenSSL < 0.9.8), OpenSSL does not automatically look in the system store.
145	// On Mac OS X 10.6 (OpenSSL >= 0.9.8), OpenSSL does look in the system store to determine trust.	149	// On Mac OS X 10.6 (OpenSSL >= 0.9.8), OpenSSL does look in the system store to determine trust.
146	// However, if there is a certificate error, it will always emit the "Invalid CA" error if we didn't add	150	// However, if there is a certificate error, it will always emit the "Invalid CA" error if we didn't add
147	// the certificates first. See	151	// the certificates first. See
148	// http://opensource.apple.com/source/OpenSSL098/OpenSSL098-27/src/crypto/x509/x509_vfy_apple.c	152	// http://opensource.apple.com/source/OpenSSL098/OpenSSL098-27/src/crypto/x509/x509_vfy_apple.c
149	// to understand why. We therefore add all certs from the system store ourselves.	153	// to understand why. We therefore add all certs from the system store ourselves.
150	X509_STORE* store = SSL_CTX_get_cert_store(context_.get());	154	X509_STORE* store = SSL_CTX_get_cert_store(context_.get());
151	CFArrayRef anchorCertificates;	155	CFArrayRef anchorCertificates;
152	if (SecTrustCopyAnchorCertificates(&anchorCertificates) == 0) {	156	if (SecTrustCopyAnchorCertificates(&anchorCertificates) == 0) {
153	for (int i = 0; i < CFArrayGetCount(anchorCertificates); ++i) {	157	for (int i = 0; i < CFArrayGetCount(anchorCertificates); ++i) {
154	SecCertificateRef cert = reinterpret_cast<SecCertificateRef>(const_cast<void*>(CFArrayGetValueAtIndex(anchorCertificates, i)));	158	SecCertificateRef cert = reinterpret_cast<SecCertificateRef>(const_cast<void*>(CFArrayGetValueAtIndex(anchorCertificates, i)));
155	CSSM_DATA certCSSMData;	159	CSSM_DATA certCSSMData;
156	if (SecCertificateGetData(cert, &certCSSMData) != 0 \|\| certCSSMData.Length == 0) {	160	if (SecCertificateGetData(cert, &certCSSMData) != 0 \|\| certCSSMData.Length == 0) {
157	continue;	161	continue;
158	}	162	}
159	std::vector<unsigned char> certData;	163	std::vector<unsigned char> certData;
160	certData.resize(certCSSMData.Length);	164	certData.resize(certCSSMData.Length);
161	memcpy(&certData[0], certCSSMData.Data, certCSSMData.Length);	165	memcpy(&certData[0], certCSSMData.Data, certCSSMData.Length);
162	OpenSSLCertificate certificate(certData);	166	OpenSSLCertificate certificate(certData);
163	if (store && certificate.getInternalX509()) {	167	if (store && certificate.getInternalX509()) {
164	X509_STORE_add_cert(store, certificate.getInternalX509().get());	168	X509_STORE_add_cert(store, certificate.getInternalX509().get());
		169	}
165	}	170	}
		171	CFRelease(anchorCertificates);
166	}	172	}
167	CFRelease(anchorCertificates);
168	}
169	#endif	173	#endif
		174	}
170	configure(options);	175	configure(options);
171	}	176	}
172		177


diff --git a/Swiften/TLS/TLSOptions.h b/Swiften/TLS/TLSOptions.h index 4109096..e3faaf9 100644 --- a/Swiften/TLS/TLSOptions.h +++ b/Swiften/TLS/TLSOptions.h
@@ -68,5 +68,10 @@ namespace Swift {
68	* Allows specification of application-specific Trust Anchors	68	* Allows specification of application-specific Trust Anchors
69	*/	69	*/
70	boost::optional<std::vector<std::shared_ptr<Certificate>>> trustAnchors;	70	boost::optional<std::vector<std::shared_ptr<Certificate>>> trustAnchors;
		71
		72	/**
		73	* Turns off automatic loading of system Trust Anchors
		74	*/
		75	bool ignoreSystemTrustAnchors = false;
71	};	76	};
72	}	77	}