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 From ea2caa8ce983f069eeeba0957b9db6b20baf82b2 Mon Sep 17 00:00:00 2001 From: Shane Kearns Date: Wed, 10 Nov 2010 16:43:20 +0000 Subject: Doc: update symbian capabilities regarding SSL Testing shows some CA certs require ReadUserData depending which part of the database they are stored in. Updated docs to reflect this. Reviewed-by: Markus Goetz --- doc/src/platforms/platform-notes.qdoc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/src/platforms/platform-notes.qdoc b/doc/src/platforms/platform-notes.qdoc index dbedc1d..9c5f3c8 100644 --- a/doc/src/platforms/platform-notes.qdoc +++ b/doc/src/platforms/platform-notes.qdoc @@ -739,6 +739,9 @@ \o \c AllFiles when \l{http://developer.symbian.org/wiki/index.php/Capabilities_%28Symbian_Signed%29/AllFiles_Capability}{accessing specific areas.} \row \o QtNetwork \o \c NetworkServices is basically always required for this module. + \o \c ReadUserData is required to include all the phone's SSL certificates in the system's default CA certificate list + (for example those added by the user or stored in the SIM card), + without this capability only the CA certs built into the phone are used. \row \o QtMultiMedia \o \c UserEnvironment if QAudioInput is used. \endtable -- cgit v0.12 From 644df27056fcd697713fe2338e6743e0fd067011 Mon Sep 17 00:00:00 2001 From: Shane Kearns Date: Wed, 10 Nov 2010 16:45:29 +0000 Subject: SSL: Fix for capabilities in QSslCertificate auto test They were in wrong section of the file due to editing error and therefore ignored. Reviewed-by: Markus Goetz --- tests/auto/qsslcertificate/qsslcertificate.pro | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/auto/qsslcertificate/qsslcertificate.pro b/tests/auto/qsslcertificate/qsslcertificate.pro index d7671ea..b04dde3 100644 --- a/tests/auto/qsslcertificate/qsslcertificate.pro +++ b/tests/auto/qsslcertificate/qsslcertificate.pro @@ -24,5 +24,6 @@ wince*: { DEFINES += SRCDIR=\\\".\\\" } else:!symbian { DEFINES += SRCDIR=\\\"$$PWD/\\\" - TARGET.CAPABILITY = NetworkServices } + +symbian:TARGET.CAPABILITY = NetworkServices ReadUserData -- cgit v0.12