From 2b6a7dfacc10644c1c6c79e59c953ad1537bd161 Mon Sep 17 00:00:00 2001
From: Thanos Doukoudakis <thanos.doukoudakis@isode.com>
Date: Mon, 14 Aug 2017 17:08:44 +0100
Subject: =?UTF-8?q?Improve=20Swift=E2=80=99s=20interactions=20with=20Smart?=
 =?UTF-8?q?=20Cards?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This patch improves logging and refactors SchannelContext and
CAPICertificate classes, to improve logging and how Swift interacts with
smart cards.

Test-Information:

Tested on Windows 10 (Qt 5.7)

Change-Id: Ic4d306beafb9e5d253731769f222e6949995d5e7

diff --git a/Swift/Controllers/Chat/ChatsManager.cpp b/Swift/Controllers/Chat/ChatsManager.cpp
index 95a9f64..a6f7fe0 100644
--- a/Swift/Controllers/Chat/ChatsManager.cpp
+++ b/Swift/Controllers/Chat/ChatsManager.cpp
@@ -33,10 +33,10 @@
 #include <Swiften/StringCodecs/Base64.h>
 #include <Swiften/VCards/VCardManager.h>
 
-#include <Swift/Controllers/Chat/ChatListWindowChatBoostSerialize.h>
 #include <Swift/Controllers/Chat/AutoAcceptMUCInviteDecider.h>
 #include <Swift/Controllers/Chat/ChatController.h>
 #include <Swift/Controllers/Chat/ChatControllerBase.h>
+#include <Swift/Controllers/Chat/ChatListWindowChatBoostSerialize.h>
 #include <Swift/Controllers/Chat/ChatMessageParser.h>
 #include <Swift/Controllers/Chat/MUCController.h>
 #include <Swift/Controllers/Chat/MUCSearchController.h>
@@ -164,6 +164,7 @@ ChatsManager::~ChatsManager() {
     roster_->onRosterCleared.disconnect(boost::bind(&ChatsManager::handleRosterCleared, this));
     ftOverview_->onNewFileTransferController.disconnect(boost::bind(&ChatsManager::handleNewFileTransferController, this, _1));
     delete joinMUCWindow_;
+    SWIFT_LOG(debug) << "Destroying ChatsManager, containing " << chatControllers_.size() << " chats and " << mucControllers_.size() << " MUCs" << std::endl;
     for (JIDChatControllerPair controllerPair : chatControllers_) {
         delete controllerPair.second;
     }
diff --git a/Swift/QtUI/CAPICertificateSelector.cpp b/Swift/QtUI/CAPICertificateSelector.cpp
index 36d8c54..e47121b 100644
--- a/Swift/QtUI/CAPICertificateSelector.cpp
+++ b/Swift/QtUI/CAPICertificateSelector.cpp
@@ -18,7 +18,9 @@
 #include <boost/algorithm/string.hpp>
 #include <Swift/Controllers/Intl.h>
 #include <Swift/QtUI/QtSwiftUtil.h>
+
 #include <Swiften/Base/Log.h>
+#include <Swiften/TLS/Schannel/SchannelUtil.h>
 
 namespace Swift {
 
@@ -87,23 +89,22 @@ std::string selectCAPICertificate() {
         }
     }
 
-
-
+    std::string result;
     /* Call Windows dialog to select a suitable certificate */
-    PCCERT_CONTEXT cert = CryptUIDlgSelectCertificateFromStore(hstore, hwnd, titleChars, promptChars, exclude_columns, 0, NULL);
+    {
+        ScopedCertContext cert(CryptUIDlgSelectCertificateFromStore(hstore, hwnd, titleChars, promptChars, exclude_columns, 0, NULL));
+        if (cert) {
+            result = getCertUri(cert, certStoreName);
+        }
+    }
 
     delete[] titleChars;
     delete[] promptChars;
 
     if (hstore) {
-        CertCloseStore(hstore, 0);
-    }
-
-    std::string result;
-
-    if (cert) {
-        result = getCertUri(cert, certStoreName);
-        CertFreeCertificateContext(cert);
+        if (CertCloseStore(hstore, 0) == FALSE) {
+            SWIFT_LOG(debug) << "Failed to close the certificate store handle." << std::endl;
+        }
     }
 
     return result;
diff --git a/Swift/QtUI/QtUIFactory.cpp b/Swift/QtUI/QtUIFactory.cpp
index ece29ec..d3b30de 100644
--- a/Swift/QtUI/QtUIFactory.cpp
+++ b/Swift/QtUI/QtUIFactory.cpp
@@ -10,6 +10,7 @@
 
 #include <QSplitter>
 
+#include <Swiften/Base/Log.h>
 #include <Swiften/Whiteboard/WhiteboardSession.h>
 
 #include <Swift/Controllers/Settings/SettingsProviderHierachy.h>
@@ -46,6 +47,13 @@ QtUIFactory::QtUIFactory(SettingsProviderHierachy* settings, QtSettingsProvider*
     this->tabs = dynamic_cast<QtChatTabs*>(tabsBase);
 }
 
+QtUIFactory::~QtUIFactory() {
+    SWIFT_LOG(debug) << "Entering QtUIFactory destructor. chatWindows size:" << chatWindows.size() << std::endl;
+    for (auto chat : chatWindows) {
+        SWIFT_LOG_ASSERT(!chat.isNull(), debug) << "QtUIFactory has active chat windows and has not been reset properly" << std::endl;
+    }
+}
+
 XMLConsoleWidget* QtUIFactory::createXMLConsoleWidget() {
     QtXMLConsoleWidget* widget = new QtXMLConsoleWidget();
     tabsBase->addTab(widget);
diff --git a/Swift/QtUI/QtUIFactory.h b/Swift/QtUI/QtUIFactory.h
index 18bf9fe..9989101 100644
--- a/Swift/QtUI/QtUIFactory.h
+++ b/Swift/QtUI/QtUIFactory.h
@@ -36,7 +36,7 @@ namespace Swift {
             Q_OBJECT
         public:
             QtUIFactory(SettingsProviderHierachy* settings, QtSettingsProvider* qtOnlySettings, QtChatTabsBase* tabs, QtSingleWindow* netbookSplitter, QtSystemTray* systemTray, QtChatWindowFactory* chatWindowFactory, TimerFactory* timerFactory, StatusCache* statusCache, AutoUpdater* autoUpdater, bool startMinimized, bool emoticonsExist, bool enableAdHocCommandOnJID);
-
+            ~QtUIFactory();
             virtual XMLConsoleWidget* createXMLConsoleWidget();
             virtual HistoryWindow* createHistoryWindow(UIEventStream*);
             virtual MainWindow* createMainWindow(UIEventStream* eventStream);
diff --git a/Swiften/TLS/CAPICertificate.cpp b/Swiften/TLS/CAPICertificate.cpp
index a46b9f6..f10ad47 100644
--- a/Swiften/TLS/CAPICertificate.cpp
+++ b/Swiften/TLS/CAPICertificate.cpp
@@ -14,6 +14,7 @@
 #include <Swiften/Base/Log.h>
 #include <Swiften/Network/TimerFactory.h>
 #include <Swiften/StringCodecs/Hexify.h>
+#include <Swiften/TLS/Schannel/SchannelUtil.h>
 
 // Size of the SHA1 hash
 #define SHA1_HASH_LEN 20
@@ -43,6 +44,7 @@ CAPICertificate::CAPICertificate(const std::string& capiUri, TimerFactory* timer
 }
 
 CAPICertificate::~CAPICertificate() {
+    SWIFT_LOG(debug) << "Destroying the CAPICertificate" << std::endl;
     if (smartCardTimer_) {
         smartCardTimer_->stop();
         smartCardTimer_->onTick.disconnect(boost::bind(&CAPICertificate::handleSmartCardTimerTick, this));
@@ -50,7 +52,9 @@ CAPICertificate::~CAPICertificate() {
     }
 
     if (certStoreHandle_) {
-        CertCloseStore(certStoreHandle_, 0);
+        if (CertCloseStore(certStoreHandle_, 0) == FALSE) {
+            SWIFT_LOG(debug) << "Failed to close the certificate store handle" << std::endl;
+        }
     }
 
     if (cardHandle_) {
@@ -80,7 +84,7 @@ const std::string& CAPICertificate::getSmartCardReaderName() const {
     return smartCardReaderName_;
 }
 
-PCCERT_CONTEXT findCertificateInStore (HCERTSTORE certStoreHandle, const std::string &certName) {
+PCCERT_CONTEXT findCertificateInStore(HCERTSTORE certStoreHandle, const std::string &certName) {
     if (!boost::iequals(certName.substr(0, 5), "sha1:")) {
 
         // Find client certificate. Note that this sample just searches for a
@@ -105,8 +109,7 @@ PCCERT_CONTEXT findCertificateInStore (HCERTSTORE certStoreHandle, const std::st
 
 }
 
-
-void CAPICertificate::setUri (const std::string& capiUri) {
+void CAPICertificate::setUri(const std::string& capiUri) {
     valid_ = false;
 
     /* Syntax: "certstore:" <cert_store> ":" <hash> ":" <hash_of_cert> */
@@ -118,7 +121,7 @@ void CAPICertificate::setUri (const std::string& capiUri) {
     /* Substring of subject: uses "storename" */
     std::string capiIdentity = capiUri.substr(10);
     std::string newCertStoreName;
-    size_t pos = capiIdentity.find_first_of (':');
+    size_t pos = capiIdentity.find_first_of(':');
 
     if (pos == std::string::npos) {
         /* Using the default certificate store */
@@ -146,49 +149,38 @@ void CAPICertificate::setUri (const std::string& capiUri) {
 
     certStore_ = newCertStoreName;
 
-    PCCERT_CONTEXT certContext = findCertificateInStore (certStoreHandle_, certName_);
-
+    ScopedCertContext certContext(findCertificateInStore(certStoreHandle_, certName_));
     if (!certContext) {
         return;
     }
 
-
     /* Now verify that we can have access to the corresponding private key */
 
     DWORD len;
-    CRYPT_KEY_PROV_INFO *pinfo;
-    HCRYPTPROV hprov;
-    HCRYPTKEY key;
-
     if (!CertGetCertificateContextProperty(certContext,
             CERT_KEY_PROV_INFO_PROP_ID,
             NULL,
             &len)) {
-        CertFreeCertificateContext(certContext);
+        SWIFT_LOG(error) << "Error while retrieving context properties" << std::endl;
         return;
     }
 
-    pinfo = static_cast<CRYPT_KEY_PROV_INFO *>(malloc(len));
+    std::shared_ptr<CRYPT_KEY_PROV_INFO> pinfo(static_cast<CRYPT_KEY_PROV_INFO *>(malloc(len)), free);
     if (!pinfo) {
-        CertFreeCertificateContext(certContext);
         return;
     }
 
-    if (!CertGetCertificateContextProperty(certContext, CERT_KEY_PROV_INFO_PROP_ID, pinfo, &len)) {
-        CertFreeCertificateContext(certContext);
-        free(pinfo);
+    if (!CertGetCertificateContextProperty(certContext, CERT_KEY_PROV_INFO_PROP_ID, pinfo.get(), &len)) {
         return;
     }
+    certContext.FreeContext();
 
-    CertFreeCertificateContext(certContext);
-
+    HCRYPTPROV hprov;
     // Now verify if we have access to the private key
     if (!CryptAcquireContextW(&hprov, pinfo->pwszContainerName, pinfo->pwszProvName, pinfo->dwProvType, 0)) {
-        free(pinfo);
         return;
     }
 
-
     char smartCardReader[1024];
     DWORD bufferLength = sizeof(smartCardReader);
     if (!CryptGetProvParam(hprov, PP_SMARTCARD_READER, (BYTE *)&smartCardReader, &bufferLength, 0)) {
@@ -205,19 +197,19 @@ void CAPICertificate::setUri (const std::string& capiUri) {
             smartCardTimer_ = timerFactory_->createTimer(SMARTCARD_EJECTION_CHECK_FREQUENCY_MILLISECONDS);
         }
         else {
-            ///Need to handle an error here
+            CryptReleaseContext(hprov, 0);
+            return;
         }
     }
 
+    HCRYPTKEY key;
     if (!CryptGetUserKey(hprov, pinfo->dwKeySpec, &key)) {
         CryptReleaseContext(hprov, 0);
-        free(pinfo);
         return;
     }
 
     CryptDestroyKey(key);
     CryptReleaseContext(hprov, 0);
-    free(pinfo);
 
     if (smartCardTimer_) {
         smartCardTimer_->onTick.connect(boost::bind(&CAPICertificate::handleSmartCardTimerTick, this));
@@ -227,7 +219,7 @@ void CAPICertificate::setUri (const std::string& capiUri) {
     valid_ = true;
 }
 
-static void smartcard_check_status (SCARDCONTEXT hContext,
+static void smartcard_check_status(SCARDCONTEXT hContext,
         const char* pReader,
         SCARDHANDLE hCardHandle, /* Can be 0 on the first call */
         SCARDHANDLE* newCardHandle, /* The handle returned */
@@ -288,7 +280,7 @@ static void smartcard_check_status (SCARDCONTEXT hContext,
     }
 }
 
-bool CAPICertificate::checkIfSmartCardPresent () {
+bool CAPICertificate::checkIfSmartCardPresent() {
     if (!smartCardReaderName_.empty()) {
         DWORD dwState;
         smartcard_check_status(scardContext_, smartCardReaderName_.c_str(), cardHandle_, &cardHandle_, &dwState);
@@ -317,8 +309,6 @@ bool CAPICertificate::checkIfSmartCardPresent () {
                 break;
         }
 
-
-
         switch (dwState) {
             case SCARD_ABSENT:
                 return false;
@@ -342,6 +332,7 @@ bool CAPICertificate::checkIfSmartCardPresent () {
 void CAPICertificate::handleSmartCardTimerTick() {
     bool poll = checkIfSmartCardPresent();
     if (lastPollingResult_ && !poll) {
+        SWIFT_LOG(debug) << "CAPI Certificate detected that the certificate card was removed" << std::endl;
         onCertificateCardRemoved();
     }
     lastPollingResult_ = poll;
diff --git a/Swiften/TLS/Schannel/SchannelContext.cpp b/Swiften/TLS/Schannel/SchannelContext.cpp
index 5799157..c07d009 100644
--- a/Swiften/TLS/Schannel/SchannelContext.cpp
+++ b/Swiften/TLS/Schannel/SchannelContext.cpp
@@ -16,6 +16,7 @@
 
 #include <WinHTTP.h> /* For SECURITY_FLAG_IGNORE_CERT_CN_INVALID */
 
+#include <Swiften/Base/Log.h>
 #include <Swiften/TLS/CAPICertificate.h>
 #include <Swiften/TLS/Schannel/SchannelCertificate.h>
 
@@ -39,13 +40,20 @@ SchannelContext::SchannelContext(bool tls1_0Workaround) : state_(Start), secCont
 //------------------------------------------------------------------------
 
 SchannelContext::~SchannelContext() {
-    if (myCertStore_) CertCloseStore(myCertStore_, 0);
+    SWIFT_LOG(debug) << "Destroying SchannelContext" << std::endl;
+    if (myCertStore_) {
+        if (CertCloseStore(myCertStore_, 0) == FALSE) {
+            SWIFT_LOG(debug) << "Failed to close the certificate store" << std::endl;
+        }
+    }
 }
 
 //------------------------------------------------------------------------
 
 void SchannelContext::determineStreamSizes() {
-    QueryContextAttributes(contextHandle_, SECPKG_ATTR_STREAM_SIZES, &streamSizes_);
+    if (QueryContextAttributes(contextHandle_, SECPKG_ATTR_STREAM_SIZES, &streamSizes_) != SEC_E_OK) {
+        SWIFT_LOG(debug) << "QueryContextAttributes failed to determinate the stream size" << std::endl;
+    }
 }
 
 //------------------------------------------------------------------------
@@ -267,8 +275,8 @@ void SchannelContext::continueHandshake(const SafeByteArray& data) {
         SecBuffer inBuffers[2];
 
         // Provide Schannel with the remote host's handshake data
-        inBuffers[0].pvBuffer     = (char*)(&receivedData_[0]);
-        inBuffers[0].cbBuffer     = (unsigned long)receivedData_.size();
+        inBuffers[0].pvBuffer = static_cast<char*>(&receivedData_[0]);
+        inBuffers[0].cbBuffer = static_cast<unsigned long>(receivedData_.size());
         inBuffers[0].BufferType  = SECBUFFER_TOKEN;
 
         inBuffers[1].pvBuffer   = NULL;
@@ -483,8 +491,8 @@ void SchannelContext::decryptAndProcessData(const SafeByteArray& data) {
         //   contexts. Additionally, a second SECBUFFER_TOKEN type buffer that contains a security token
         //   must also be supplied.
         //
-        inBuffers[0].pvBuffer     = (char*)(&receivedData_[0]);
-        inBuffers[0].cbBuffer     = (unsigned long)receivedData_.size();
+        inBuffers[0].pvBuffer = static_cast<char*>(&receivedData_[0]);
+        inBuffers[0].cbBuffer = static_cast<unsigned long>(receivedData_.size());
         inBuffers[0].BufferType  = SECBUFFER_DATA;
 
         inBuffers[1].BufferType  = SECBUFFER_EMPTY;
@@ -578,7 +586,7 @@ void SchannelContext::encryptAndSendData(const SafeByteArray& data) {
         outBuffers[0].BufferType = SECBUFFER_STREAM_HEADER;
 
         outBuffers[1].pvBuffer     = &sendBuffer[0] + streamSizes_.cbHeader;
-        outBuffers[1].cbBuffer     = (unsigned long)bytesToSend;
+        outBuffers[1].cbBuffer     = static_cast<unsigned long>(bytesToSend);
         outBuffers[1].BufferType = SECBUFFER_DATA;
 
         outBuffers[2].pvBuffer     = &sendBuffer[0] + streamSizes_.cbHeader + bytesToSend;
@@ -645,6 +653,7 @@ std::vector<Certificate::ref> SchannelContext::getPeerCertificateChain() const {
     SECURITY_STATUS status = QueryContextAttributes(contextHandle_, SECPKG_ATTR_REMOTE_CERT_CONTEXT, pServerCert.Reset());
 
     if (status != SEC_E_OK) {
+        SWIFT_LOG(debug) << "Error while Querying the Certificate Chain" << std::endl;
         return certificateChain;
     }
     certificateChain.push_back(std::make_shared<SchannelCertificate>(pServerCert));
@@ -678,6 +687,10 @@ ByteArray SchannelContext::getFinishMessage() const {
     if (ret == SEC_E_OK) {
         return createByteArray(((unsigned char*) bindings.Bindings) + bindings.Bindings->dwApplicationDataOffset + 11 /* tls-unique:*/, bindings.Bindings->cbApplicationDataLength - 11);
     }
+    else {
+        SWIFT_LOG(debug) << "Error while retrieving Finish Message" << std::endl;
+    }
+
     return ByteArray();
 }
 
-- 
cgit v0.10.2-6-g49f6