From 2b6a7dfacc10644c1c6c79e59c953ad1537bd161 Mon Sep 17 00:00:00 2001 From: Thanos Doukoudakis 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 #include -#include #include #include #include +#include #include #include #include @@ -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 #include #include + #include +#include 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 +#include #include #include @@ -46,6 +47,13 @@ QtUIFactory::QtUIFactory(SettingsProviderHierachy* settings, QtSettingsProvider* this->tabs = dynamic_cast(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 #include #include +#include // 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:" ":" ":" */ @@ -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(malloc(len)); + std::shared_ptr pinfo(static_cast(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 /* For SECURITY_FLAG_IGNORE_CERT_CN_INVALID */ +#include #include #include @@ -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(&receivedData_[0]); + inBuffers[0].cbBuffer = static_cast(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(&receivedData_[0]); + inBuffers[0].cbBuffer = static_cast(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(bytesToSend); outBuffers[1].BufferType = SECBUFFER_DATA; outBuffers[2].pvBuffer = &sendBuffer[0] + streamSizes_.cbHeader + bytesToSend; @@ -645,6 +653,7 @@ std::vector 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(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