Swiftdiff options
| author | Thanos Doukoudakis <thanos.doukoudakis@isode.com> | 2017-08-14 16:08:44 (GMT) |
|---|---|---|
| committer | Thanos Doukoudakis <thanos.doukoudakis@isode.com> | 2017-08-15 08:48:42 (GMT) |
| commit | 2b6a7dfacc10644c1c6c79e59c953ad1537bd161 (patch) | |
| tree | 9cb5170b99d3db0fd91bc9c170edb5375ca1ddda | |
| parent | c5aa27ee869cc4859e494bde8ddf7da60e177f40 (diff) | |
| download | swift-2b6a7dfacc10644c1c6c79e59c953ad1537bd161.zip swift-2b6a7dfacc10644c1c6c79e59c953ad1537bd161.tar.bz2 | |
Improve Swift’s interactions with Smart Cards
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
| -rw-r--r-- | Swift/Controllers/Chat/ChatsManager.cpp | 3 | ||||
| -rw-r--r-- | Swift/QtUI/CAPICertificateSelector.cpp | 23 | ||||
| -rw-r--r-- | Swift/QtUI/QtUIFactory.cpp | 8 | ||||
| -rw-r--r-- | Swift/QtUI/QtUIFactory.h | 2 | ||||
| -rw-r--r-- | Swiften/TLS/CAPICertificate.cpp | 49 | ||||
| -rw-r--r-- | Swiften/TLS/Schannel/SchannelContext.cpp | 27 |
6 files changed, 63 insertions, 49 deletions
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 @@ -35,3 +35,2 @@ -#include <Swift/Controllers/Chat/ChatListWindowChatBoostSerialize.h> #include <Swift/Controllers/Chat/AutoAcceptMUCInviteDecider.h> @@ -39,2 +38,3 @@ #include <Swift/Controllers/Chat/ChatControllerBase.h> +#include <Swift/Controllers/Chat/ChatListWindowChatBoostSerialize.h> #include <Swift/Controllers/Chat/ChatMessageParser.h> @@ -166,2 +166,3 @@ ChatsManager::~ChatsManager() { delete joinMUCWindow_; + SWIFT_LOG(debug) << "Destroying ChatsManager, containing " << chatControllers_.size() << " chats and " << mucControllers_.size() << " MUCs" << std::endl; for (JIDChatControllerPair controllerPair : chatControllers_) { 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 @@ -20,3 +20,5 @@ #include <Swift/QtUI/QtSwiftUtil.h> + #include <Swiften/Base/Log.h> +#include <Swiften/TLS/Schannel/SchannelUtil.h> @@ -89,6 +91,10 @@ 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); + } + } @@ -98,10 +104,5 @@ std::string selectCAPICertificate() { 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; + } } 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 @@ -12,2 +12,3 @@ +#include <Swiften/Base/Log.h> #include <Swiften/Whiteboard/WhiteboardSession.h> @@ -48,2 +49,9 @@ QtUIFactory::QtUIFactory(SettingsProviderHierachy* settings, QtSettingsProvider* +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() { 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 @@ -38,3 +38,3 @@ namespace Swift { 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(); 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 @@ -16,2 +16,3 @@ #include <Swiften/StringCodecs/Hexify.h> +#include <Swiften/TLS/Schannel/SchannelUtil.h> @@ -45,2 +46,3 @@ CAPICertificate::CAPICertificate(const std::string& capiUri, TimerFactory* timer CAPICertificate::~CAPICertificate() { + SWIFT_LOG(debug) << "Destroying the CAPICertificate" << std::endl; if (smartCardTimer_) { @@ -52,3 +54,5 @@ CAPICertificate::~CAPICertificate() { if (certStoreHandle_) { - CertCloseStore(certStoreHandle_, 0); + if (CertCloseStore(certStoreHandle_, 0) == FALSE) { + SWIFT_LOG(debug) << "Failed to close the certificate store handle" << std::endl; + } } @@ -82,3 +86,3 @@ const std::string& CAPICertificate::getSmartCardReaderName() const { -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:")) { @@ -107,4 +111,3 @@ PCCERT_CONTEXT findCertificateInStore (HCERTSTORE certStoreHandle, const std::st - -void CAPICertificate::setUri (const std::string& capiUri) { +void CAPICertificate::setUri(const std::string& capiUri) { valid_ = false; @@ -120,3 +123,3 @@ void CAPICertificate::setUri (const std::string& capiUri) { std::string newCertStoreName; - size_t pos = capiIdentity.find_first_of (':'); + size_t pos = capiIdentity.find_first_of(':'); @@ -148,4 +151,3 @@ void CAPICertificate::setUri (const std::string& capiUri) { - PCCERT_CONTEXT certContext = findCertificateInStore (certStoreHandle_, certName_); - + ScopedCertContext certContext(findCertificateInStore(certStoreHandle_, certName_)); if (!certContext) { @@ -154,3 +156,2 @@ void CAPICertificate::setUri (const std::string& capiUri) { - /* Now verify that we can have access to the corresponding private key */ @@ -158,6 +159,2 @@ void CAPICertificate::setUri (const std::string& capiUri) { DWORD len; - CRYPT_KEY_PROV_INFO *pinfo; - HCRYPTPROV hprov; - HCRYPTKEY key; - if (!CertGetCertificateContextProperty(certContext, @@ -166,3 +163,3 @@ void CAPICertificate::setUri (const std::string& capiUri) { &len)) { - CertFreeCertificateContext(certContext); + SWIFT_LOG(error) << "Error while retrieving context properties" << std::endl; return; @@ -170,5 +167,4 @@ void CAPICertificate::setUri (const std::string& capiUri) { - 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; @@ -176,13 +172,10 @@ void CAPICertificate::setUri (const std::string& capiUri) { - 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; @@ -190,3 +183,2 @@ void CAPICertificate::setUri (const std::string& capiUri) { - char smartCardReader[1024]; @@ -207,3 +199,4 @@ void CAPICertificate::setUri (const std::string& capiUri) { else { - ///Need to handle an error here + CryptReleaseContext(hprov, 0); + return; } @@ -211,5 +204,5 @@ void CAPICertificate::setUri (const std::string& capiUri) { + HCRYPTKEY key; if (!CryptGetUserKey(hprov, pinfo->dwKeySpec, &key)) { CryptReleaseContext(hprov, 0); - free(pinfo); return; @@ -219,3 +212,2 @@ void CAPICertificate::setUri (const std::string& capiUri) { CryptReleaseContext(hprov, 0); - free(pinfo); @@ -229,3 +221,3 @@ void CAPICertificate::setUri (const std::string& capiUri) { -static void smartcard_check_status (SCARDCONTEXT hContext, +static void smartcard_check_status(SCARDCONTEXT hContext, const char* pReader, @@ -290,3 +282,3 @@ static void smartcard_check_status (SCARDCONTEXT hContext, -bool CAPICertificate::checkIfSmartCardPresent () { +bool CAPICertificate::checkIfSmartCardPresent() { if (!smartCardReaderName_.empty()) { @@ -319,4 +311,2 @@ bool CAPICertificate::checkIfSmartCardPresent () { - - switch (dwState) { @@ -344,2 +334,3 @@ void CAPICertificate::handleSmartCardTimerTick() { if (lastPollingResult_ && !poll) { + SWIFT_LOG(debug) << "CAPI Certificate detected that the certificate card was removed" << std::endl; onCertificateCardRemoved(); 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 @@ -18,2 +18,3 @@ +#include <Swiften/Base/Log.h> #include <Swiften/TLS/CAPICertificate.h> @@ -41,3 +42,8 @@ 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; + } + } } @@ -47,3 +53,5 @@ SchannelContext::~SchannelContext() { 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; + } } @@ -269,4 +277,4 @@ void SchannelContext::continueHandshake(const SafeByteArray& data) { // 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; @@ -485,4 +493,4 @@ void SchannelContext::decryptAndProcessData(const SafeByteArray& 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_DATA; @@ -580,3 +588,3 @@ void SchannelContext::encryptAndSendData(const SafeByteArray& data) { 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; @@ -647,2 +655,3 @@ std::vector<Certificate::ref> SchannelContext::getPeerCertificateChain() const { if (status != SEC_E_OK) { + SWIFT_LOG(debug) << "Error while Querying the Certificate Chain" << std::endl; return certificateChain; @@ -680,2 +689,6 @@ ByteArray SchannelContext::getFinishMessage() const { } + else { + SWIFT_LOG(debug) << "Error while retrieving Finish Message" << std::endl; + } + return ByteArray(); |