From 5feca34327eb661cc84e1d6f79052dcdbc1a09c4 Mon Sep 17 00:00:00 2001 From: Shane Kearns Date: Wed, 10 Nov 2010 15:19:14 +0000 Subject: SSL: Fix crashes/hangs when retrieving CA certificates Added error handling to the certificate retrieval thread Made the certificate retrieval thread process critical (so if it crashes the process will crash instead of hang) Filter the certificate list to only fetch CA certificates which are in X.509 format (symbian also allows WAP formats, but Qt does not support these). Put the TPtr8 for asynch function parameter in the class data so it does not go out of scope while the function is in progress. Previously it was on the stack so it could be corrupted before the certificate server had finished using it. Task-number: QTBUG-15005 Task-number: QTBUG-15126 Reviewed-by: Markus Goetz --- src/network/ssl/qsslsocket_openssl.cpp | 75 ++++++++++++++++++++++++++-------- src/network/ssl/qsslsocket_openssl_p.h | 1 + 2 files changed, 60 insertions(+), 16 deletions(-) diff --git a/src/network/ssl/qsslsocket_openssl.cpp b/src/network/ssl/qsslsocket_openssl.cpp index 2910538..426b07a 100644 --- a/src/network/ssl/qsslsocket_openssl.cpp +++ b/src/network/ssl/qsslsocket_openssl.cpp @@ -51,6 +51,7 @@ #include #include #include +#include #include #include #include @@ -574,7 +575,7 @@ void QSslSocketPrivate::resetDefaultCiphers() #if defined(Q_OS_SYMBIAN) CSymbianCertificateRetriever::CSymbianCertificateRetriever() : CActive(CActive::EPriorityStandard), - iSequenceError(KErrNone) + iCertificatePtr(0,0,0), iSequenceError(KErrNone) { } @@ -618,6 +619,11 @@ void CSymbianCertificateRetriever::doThreadEntryL() iCertStore = CUnifiedCertStore::NewLC(qt_s60GetRFs(), EFalse); iCertFilter = CCertAttributeFilter::NewLC(); + // only interested in CA certs + iCertFilter->SetOwnerType(ECACertificate); + // only interested in X.509 format (we don't support WAP formats) + iCertFilter->SetFormat(EX509Certificate); + // Kick off the sequence by initializing the cert store iState = Initializing; iCertStore->Initialize(iStatus); @@ -637,6 +643,7 @@ void CSymbianCertificateRetriever::doThreadEntryL() TInt CSymbianCertificateRetriever::ThreadEntryPoint(TAny* aParams) { + User::SetCritical(User::EProcessCritical); CTrapCleanup* cleanupStack = CTrapCleanup::New(); CSymbianCertificateRetriever* self = (CSymbianCertificateRetriever*) aParams; @@ -658,7 +665,17 @@ void CSymbianCertificateRetriever::ConstructL() void CSymbianCertificateRetriever::DoCancel() { - // We never cancel the sequence + switch(iState) { + case Initializing: + iCertStore->CancelInitialize(); + break; + case Listing: + iCertStore->CancelList(); + break; + case RetrievingCertificates: + iCertStore->CancelGetCert(); + break; + } } TInt CSymbianCertificateRetriever::RunError(TInt aError) @@ -671,37 +688,51 @@ TInt CSymbianCertificateRetriever::RunError(TInt aError) void CSymbianCertificateRetriever::GetCertificateL() { - CCTCertInfo* certInfo = iCertInfos[iCurrentCertIndex]; - iCertificateData.resize(certInfo->Size()); - TPtr8 des((TUint8*)iCertificateData.data(), 0, iCertificateData.size()); - iCertStore->Retrieve(*certInfo, des, iStatus); - iState = RetrievingCertificates; - SetActive(); + if (iCurrentCertIndex < iCertInfos.Count()) { + CCTCertInfo* certInfo = iCertInfos[iCurrentCertIndex++]; + iCertificateData = QByteArray(); + QT_TRYCATCH_LEAVING(iCertificateData.resize(certInfo->Size())); + iCertificatePtr.Set((TUint8*)iCertificateData.data(), 0, iCertificateData.size()); +#ifdef QSSLSOCKET_DEBUG + qDebug() << "getting " << qt_TDesC2QString(certInfo->Label()) << " size=" << certInfo->Size(); + qDebug() << "format=" << certInfo->CertificateFormat(); + qDebug() << "ownertype=" << certInfo->CertificateOwnerType(); + qDebug() << "type=" << hex << certInfo->Type().iUid; +#endif + iCertStore->Retrieve(*certInfo, iCertificatePtr, iStatus); + iState = RetrievingCertificates; + SetActive(); + } else { + //reached end of list + CActiveScheduler::Stop(); + } } void CSymbianCertificateRetriever::RunL() { +#ifdef QSSLSOCKET_DEBUG + qDebug() << "CSymbianCertificateRetriever::RunL status " << iStatus.Int() << " count " << iCertInfos.Count() << " index " << iCurrentCertIndex; +#endif switch (iState) { case Initializing: + User::LeaveIfError(iStatus.Int()); // initialise fail means pointless to continue iState = Listing; iCertStore->List(iCertInfos, *iCertFilter, iStatus); SetActive(); break; case Listing: + User::LeaveIfError(iStatus.Int()); // listing fail means pointless to continue iCurrentCertIndex = 0; GetCertificateL(); break; case RetrievingCertificates: - iCertificates->append(iCertificateData); - iCertificateData = QByteArray(); - iCurrentCertIndex++; - if (iCurrentCertIndex < iCertInfos.Count()) - GetCertificateL(); + if (iStatus.Int() == KErrNone) + iCertificates->append(iCertificateData); else - // Stop the scheduler to return to the thread entry function - CActiveScheduler::Stop(); + qWarning() << "CSymbianCertificateRetriever: failed to retreive a certificate, error " << iStatus.Int(); + GetCertificateL(); break; } } @@ -710,6 +741,10 @@ void CSymbianCertificateRetriever::RunL() QList QSslSocketPrivate::systemCaCertificates() { ensureInitialized(); +#ifdef QSSLSOCKET_DEBUG + QElapsedTimer timer; + timer.start(); +#endif QList systemCerts; #if defined(Q_OS_MAC) CFArrayRef cfCerts; @@ -808,10 +843,18 @@ QList QSslSocketPrivate::systemCaCertificates() retriever->GetCertificates(certs); foreach (const QByteArray &encodedCert, certs) { QSslCertificate cert(encodedCert, QSsl::Der); - if (!cert.isNull()) + if (!cert.isNull()) { +#ifdef QSSLSOCKET_DEBUG + qDebug() << "imported certificate: " << cert.issuerInfo(QSslCertificate::CommonName); +#endif systemCerts.append(cert); + } } #endif +#ifdef QSSLSOCKET_DEBUG + qDebug() << "systemCaCertificates retrieval time " << timer.elapsed() << "ms"; + qDebug() << "imported " << systemCerts.count() << " certificates"; +#endif return systemCerts; } diff --git a/src/network/ssl/qsslsocket_openssl_p.h b/src/network/ssl/qsslsocket_openssl_p.h index dec98ae..b59a6c9 100644 --- a/src/network/ssl/qsslsocket_openssl_p.h +++ b/src/network/ssl/qsslsocket_openssl_p.h @@ -165,6 +165,7 @@ private: CCertAttributeFilter* iCertFilter; TInt iCurrentCertIndex; QByteArray iCertificateData; + TPtr8 iCertificatePtr; QList* iCertificates; TInt iSequenceError; }; -- cgit v0.12