From 27211ac2ca11c6ac259bc09bb81a7ed297a9d07d Mon Sep 17 00:00:00 2001 From: Tobias Markmann Date: Mon, 8 Feb 2016 16:06:54 +0100 Subject: Treat cert verify errors as non-fatal in OS X TLS backend Our TLS backends need to tread TLS verification errors, e.g. outdated certificate, untrusted CA, non-matching host, etc., as non-fatal, so the application can apply custom key pinning verification or similar. This patch changes the OS X SecureTransport backend to behave accordingly and adjusts the CertificateErrorTest to mirror this behavior. This commit also fixes a double-free in SecureTransportCertificate. Test-Information: Connected to a host with an untrusted CA and non-matching domain in the certificate and was prompted with the Swift certificate trust dialog on OS X 10.11.3. Swiften/QA/TLSTest run successfully on OS X 10.11.3. Change-Id: I4c8ce2178540d79a5f328e2e0558d4deb4295134 diff --git a/Swiften/QA/TLSTest/CertificateErrorTest.cpp b/Swiften/QA/TLSTest/CertificateErrorTest.cpp index e69af0b..3b33e8e 100644 --- a/Swiften/QA/TLSTest/CertificateErrorTest.cpp +++ b/Swiften/QA/TLSTest/CertificateErrorTest.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015 Isode Limited. + * Copyright (c) 2015-2016 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ @@ -13,8 +13,8 @@ #include #include -#include #include +#include #include #include #include @@ -122,7 +122,7 @@ class CertificateErrorTest : public CppUnit::TestFixture { connectToServer(connection, "test5.tls-o-matic.com", 405); - CPPUNIT_ASSERT_EQUAL(true, connectFinishedWithError_); + CPPUNIT_ASSERT_EQUAL(false, connectFinishedWithError_); CPPUNIT_ASSERT(context->getPeerCertificateVerificationError()); CPPUNIT_ASSERT_EQUAL(CertificateVerificationError::NotYetValid, context->getPeerCertificateVerificationError()->getType()); } @@ -133,7 +133,7 @@ class CertificateErrorTest : public CppUnit::TestFixture { connectToServer(connection, "test6.tls-o-matic.com", 406); - CPPUNIT_ASSERT_EQUAL(true, connectFinishedWithError_); + CPPUNIT_ASSERT_EQUAL(false, connectFinishedWithError_); CPPUNIT_ASSERT(context->getPeerCertificateVerificationError()); CPPUNIT_ASSERT_EQUAL(CertificateVerificationError::Expired, context->getPeerCertificateVerificationError()->getType()); } @@ -144,7 +144,7 @@ class CertificateErrorTest : public CppUnit::TestFixture { connectToServer(connection, "test7.tls-o-matic.com", 407); - CPPUNIT_ASSERT_EQUAL(true, connectFinishedWithError_); + CPPUNIT_ASSERT_EQUAL(false, connectFinishedWithError_); CPPUNIT_ASSERT(context->getPeerCertificateVerificationError()); CPPUNIT_ASSERT_EQUAL(CertificateVerificationError::Untrusted, context->getPeerCertificateVerificationError()->getType()); } @@ -156,7 +156,7 @@ class CertificateErrorTest : public CppUnit::TestFixture { connectToServer(connection, "test14.tls-o-matic.com", 414); - CPPUNIT_ASSERT_EQUAL(true, connectFinishedWithError_); + CPPUNIT_ASSERT_EQUAL(false, connectFinishedWithError_); CPPUNIT_ASSERT(context->getPeerCertificateVerificationError()); CPPUNIT_ASSERT_EQUAL(CertificateVerificationError::InvalidPurpose, context->getPeerCertificateVerificationError()->getType()); } @@ -179,7 +179,7 @@ class CertificateErrorTest : public CppUnit::TestFixture { connectToServer(connection, "revoked.grc.com", 443); - CPPUNIT_ASSERT_EQUAL(true, connectFinishedWithError_); + CPPUNIT_ASSERT_EQUAL(false, connectFinishedWithError_); CPPUNIT_ASSERT(context->getPeerCertificateVerificationError()); CPPUNIT_ASSERT_EQUAL(CertificateVerificationError::Revoked, context->getPeerCertificateVerificationError()->getType()); } diff --git a/Swiften/TLS/SecureTransport/SecureTransportCertificate.mm b/Swiften/TLS/SecureTransport/SecureTransportCertificate.mm index 4270a6f..ed409bd 100644 --- a/Swiften/TLS/SecureTransport/SecureTransportCertificate.mm +++ b/Swiften/TLS/SecureTransport/SecureTransportCertificate.mm @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015 Isode Limited. + * Copyright (c) 2015-2016 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ @@ -36,9 +36,9 @@ SecureTransportCertificate::SecureTransportCertificate(SecCertificateRef certifi SecureTransportCertificate::SecureTransportCertificate(const ByteArray& der) { - CFDataRef derData = CFDataCreateWithBytesNoCopy(NULL, der.data(), static_cast(der.size()), NULL); + CFDataRef derData = CFDataCreateWithBytesNoCopy(NULL, der.data(), static_cast(der.size()), NULL); + // certificate will take ownership of derData and free it on its release. SecCertificateRef certificate = SecCertificateCreateWithData(NULL, derData); - CFRelease(derData); if (certificate) { certificateHandle_ = boost::shared_ptr(certificate, CFRelease); parse(); diff --git a/Swiften/TLS/SecureTransport/SecureTransportContext.mm b/Swiften/TLS/SecureTransport/SecureTransportContext.mm index 2357579..ca6c5bb 100644 --- a/Swiften/TLS/SecureTransport/SecureTransportContext.mm +++ b/Swiften/TLS/SecureTransport/SecureTransportContext.mm @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015 Isode Limited. + * Copyright (c) 2015-2016 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ @@ -270,16 +270,11 @@ void SecureTransportContext::verifyServerCertificate() { break; } - if (verificationError_) { - setState(Error); - SSLClose(sslContext_.get()); - sslContext_.reset(); - onError(boost::make_shared()); - } - else { - // proceed with handshake - processHandshake(); - } + // We proceed with the TLS handshake here to give the application an opportunity + // to apply custom validation and trust management. The application is responsible + // to call \ref getPeerCertificateVerificationError directly after the \ref onConnected + // signal is called and before any application data is send to the context. + processHandshake(); } #pragma clang diagnostic pop -- cgit v0.10.2-6-g49f6