From db91fa9d4a628002d9f7fae4e9af4a861c8fdaf4 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Sun, 29 Apr 2012 21:00:29 -0700 Subject: [PATCH] Bug 703834 - Part 2 - Make SSLServerCertVerification use TransportSecurityInfo instead of nsNSSSocketInfo, r=honzab --- .../ssl/src/SSLServerCertVerification.cpp | 147 +++++++++--------- .../ssl/src/SSLServerCertVerification.h | 5 - 2 files changed, 77 insertions(+), 75 deletions(-) diff --git a/security/manager/ssl/src/SSLServerCertVerification.cpp b/security/manager/ssl/src/SSLServerCertVerification.cpp index 3bb1af29c275..6d9f02604f2a 100644 --- a/security/manager/ssl/src/SSLServerCertVerification.cpp +++ b/security/manager/ssl/src/SSLServerCertVerification.cpp @@ -124,10 +124,11 @@ * * SSLServerCertVerificationResult must be dispatched to the socket transport * thread because we must only call SSL_* functions on the socket transport - * thread since they may do I/O, because many parts of nsNSSSocketInfo and - * the PSM NSS I/O layer are not thread-safe, and because we need the event to - * interrupt the PR_Poll that may waiting for I/O on the socket for which we - * are validating the cert. + * thread since they may do I/O, because many parts of nsNSSSocketInfo (the + * subclass of TransportSecurityInfo used when validating certificates during + * an SSL handshake) and the PSM NSS I/O layer are not thread-safe, and because + * we need the event to interrupt the PR_Poll that may waiting for I/O on the + * socket for which we are validating the cert. */ #include "SSLServerCertVerification.h" @@ -216,7 +217,7 @@ void StopSSLServerCertVerificationThreads() namespace { -// Dispatched to the STS thread to notify the socketInfo of the verification +// Dispatched to the STS thread to notify the infoObject of the verification // result. // // This will cause the PR_Poll in the STS thread to return, so things work @@ -227,14 +228,14 @@ class SSLServerCertVerificationResult : public nsRunnable public: NS_DECL_NSIRUNNABLE - SSLServerCertVerificationResult(nsNSSSocketInfo & socketInfo, + SSLServerCertVerificationResult(TransportSecurityInfo * infoObject, PRErrorCode errorCode, SSLErrorMessageType errorMessageType = PlainErrorMessage); void Dispatch(); private: - const nsRefPtr mSocketInfo; + const nsRefPtr mInfoObject; public: const PRErrorCode mErrorCode; const SSLErrorMessageType mErrorMessageType; @@ -245,7 +246,7 @@ class CertErrorRunnable : public SyncRunnableBase public: CertErrorRunnable(const void * fdForLogging, nsIX509Cert * cert, - nsNSSSocketInfo * infoObject, + TransportSecurityInfo * infoObject, PRErrorCode defaultErrorCodeToReport, PRUint32 collectedErrors, PRErrorCode errorCodeTrust, @@ -267,7 +268,7 @@ private: const void * const mFdForLogging; // may become an invalid pointer; do not dereference const nsCOMPtr mCert; - const nsRefPtr mInfoObject; + const nsRefPtr mInfoObject; const PRErrorCode mDefaultErrorCodeToReport; const PRUint32 mCollectedErrors; const PRErrorCode mErrorCodeTrust; @@ -283,7 +284,7 @@ CertErrorRunnable::CheckCertOverrides() if (!NS_IsMainThread()) { NS_ERROR("CertErrorRunnable::CheckCertOverrides called off main thread"); - return new SSLServerCertVerificationResult(*mInfoObject, + return new SSLServerCertVerificationResult(mInfoObject, mDefaultErrorCodeToReport); } @@ -310,7 +311,7 @@ CertErrorRunnable::CheckCertOverrides() &strictTransportSecurityEnabled); } if (NS_FAILED(nsrv)) { - return new SSLServerCertVerificationResult(*mInfoObject, + return new SSLServerCertVerificationResult(mInfoObject, mDefaultErrorCodeToReport); } @@ -343,7 +344,7 @@ CertErrorRunnable::CheckCertOverrides() PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("[%p][%p] All errors covered by override rules\n", mFdForLogging, this)); - return new SSLServerCertVerificationResult(*mInfoObject, 0); + return new SSLServerCertVerificationResult(mInfoObject, 0); } } else { PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, @@ -359,15 +360,20 @@ CertErrorRunnable::CheckCertOverrides() // First, deliver the technical details of the broken SSL status. // Try to get a nsIBadCertListener2 implementation from the socket consumer. - nsCOMPtr cb; - mInfoObject->GetNotificationCallbacks(getter_AddRefs(cb)); - if (cb) { - nsCOMPtr bcl = do_GetInterface(cb); - if (bcl) { - nsIInterfaceRequestor *csi = static_cast(mInfoObject); - bool suppressMessage = false; // obsolete, ignored - nsrv = bcl->NotifyCertProblem(csi, mInfoObject->SSLStatus(), - hostWithPortString, &suppressMessage); + nsCOMPtr sslSocketControl = do_QueryInterface( + NS_ISUPPORTS_CAST(nsITransportSecurityInfo*, mInfoObject)); + if (sslSocketControl) { + nsCOMPtr cb; + sslSocketControl->GetNotificationCallbacks(getter_AddRefs(cb)); + if (cb) { + nsCOMPtr bcl = do_GetInterface(cb); + if (bcl) { + nsIInterfaceRequestor *csi + = static_cast(mInfoObject); + bool suppressMessage = false; // obsolete, ignored + nsrv = bcl->NotifyCertProblem(csi, mInfoObject->SSLStatus(), + hostWithPortString, &suppressMessage); + } } } @@ -386,7 +392,7 @@ CertErrorRunnable::CheckCertOverrides() : mErrorCodeExpired ? mErrorCodeExpired : mDefaultErrorCodeToReport; - return new SSLServerCertVerificationResult(*mInfoObject, errorCodeToReport, + return new SSLServerCertVerificationResult(mInfoObject, errorCodeToReport, OverridableCertErrorMessage); } @@ -404,11 +410,11 @@ CertErrorRunnable::RunOnTargetThread() // the CertErrorRunnable. CertErrorRunnable * CreateCertErrorRunnable(PRErrorCode defaultErrorCodeToReport, - nsNSSSocketInfo * socketInfo, + TransportSecurityInfo * infoObject, CERTCertificate * cert, const void * fdForLogging) { - MOZ_ASSERT(socketInfo); + MOZ_ASSERT(infoObject); MOZ_ASSERT(cert); // cert was revoked, don't do anything else @@ -467,7 +473,7 @@ CreateCertErrorRunnable(PRErrorCode defaultErrorCodeToReport, if (!nsNSSComponent::globalConstFlagUsePKIXVerification) { srv = CERT_VerifyCertificate(CERT_GetDefaultCertDB(), cert, true, certificateUsageSSLServer, - PR_Now(), static_cast(socketInfo), + PR_Now(), static_cast(infoObject), verify_log, NULL); } else { @@ -478,7 +484,7 @@ CreateCertErrorRunnable(PRErrorCode defaultErrorCodeToReport, srv = CERT_PKIXVerifyCert(cert, certificateUsageSSLServer, survivingParams->GetRawPointerForNSS(), - cvout, static_cast(socketInfo)); + cvout, static_cast(infoObject)); } // We ignore the result code of the cert verification. @@ -493,13 +499,13 @@ CreateCertErrorRunnable(PRErrorCode defaultErrorCodeToReport, PRUint32 collected_errors = 0; - if (socketInfo->IsCertIssuerBlacklisted()) { + if (infoObject->IsCertIssuerBlacklisted()) { collected_errors |= nsICertOverrideService::ERROR_UNTRUSTED; errorCodeTrust = defaultErrorCodeToReport; } // Check the name field against the desired hostname. - if (CERT_VerifyCertName(cert, socketInfo->GetHostName()) != SECSuccess) { + if (CERT_VerifyCertName(cert, infoObject->GetHostName()) != SECSuccess) { collected_errors |= nsICertOverrideService::ERROR_MISMATCH; errorCodeMismatch = SSL_ERROR_BAD_CERT_DOMAIN; } @@ -549,11 +555,11 @@ CreateCertErrorRunnable(PRErrorCode defaultErrorCodeToReport, return nsnull; } - socketInfo->SetStatusErrorBits(*nssCert, collected_errors); + infoObject->SetStatusErrorBits(*nssCert, collected_errors); return new CertErrorRunnable(fdForLogging, static_cast(nssCert.get()), - socketInfo, defaultErrorCodeToReport, + infoObject, defaultErrorCodeToReport, collected_errors, errorCodeTrust, errorCodeMismatch, errorCodeExpired); } @@ -562,9 +568,9 @@ CreateCertErrorRunnable(PRErrorCode defaultErrorCodeToReport, // socket transport service thread, which blocks the socket transport // service thread while it waits for the inner CertErrorRunnable to execute // CheckCertOverrides on the main thread. CheckCertOverrides must block events -// on both of these threads because it calls nsNSSSocketInfo::GetInterface(), +// on both of these threads because it calls TransportSecurityInfo::GetInterface(), // which may call nsHttpConnection::GetInterface() through -// nsNSSSocketInfo::mCallbacks. nsHttpConnection::GetInterface must always +// TransportSecurityInfo::mCallbacks. nsHttpConnection::GetInterface must always // execute on the main thread, with the socket transport service thread // blocked. class CertErrorRunnableRunnable : public nsRunnable @@ -594,28 +600,28 @@ class SSLServerCertVerificationJob : public nsRunnable public: // Must be called only on the socket transport thread static SECStatus Dispatch(const void * fdForLogging, - nsNSSSocketInfo * infoObject, + TransportSecurityInfo * infoObject, CERTCertificate * serverCert); private: NS_DECL_NSIRUNNABLE // Must be called only on the socket transport thread SSLServerCertVerificationJob(const void * fdForLogging, - nsNSSSocketInfo & socketInfo, - CERTCertificate & cert); + TransportSecurityInfo * infoObject, + CERTCertificate * cert); ~SSLServerCertVerificationJob(); const void * const mFdForLogging; - const nsRefPtr mSocketInfo; + const nsRefPtr mInfoObject; CERTCertificate * const mCert; }; SSLServerCertVerificationJob::SSLServerCertVerificationJob( - const void * fdForLogging, nsNSSSocketInfo & socketInfo, - CERTCertificate & cert) + const void * fdForLogging, TransportSecurityInfo * infoObject, + CERTCertificate * cert) : mFdForLogging(fdForLogging) - , mSocketInfo(&socketInfo) - , mCert(CERT_DupCertificate(&cert)) + , mInfoObject(infoObject) + , mCert(CERT_DupCertificate(cert)) { } @@ -820,11 +826,8 @@ BlockServerCertChangeForSpdy(nsNSSSocketInfo *infoObject, } SECStatus -AuthCertificate(nsNSSSocketInfo * socketInfo, CERTCertificate * cert) +AuthCertificate(TransportSecurityInfo * infoObject, CERTCertificate * cert) { - if (BlockServerCertChangeForSpdy(socketInfo, cert) != SECSuccess) - return SECFailure; - if (cert->serialNumber.data && cert->issuerName && !strcmp(cert->issuerName, @@ -863,14 +866,14 @@ AuthCertificate(nsNSSSocketInfo * socketInfo, CERTCertificate * cert) } } - SECStatus rv = PSM_SSL_PKIX_AuthCertificate(cert, socketInfo, - socketInfo->GetHostName()); + SECStatus rv = PSM_SSL_PKIX_AuthCertificate(cert, infoObject, + infoObject->GetHostName()); // We want to remember the CA certs in the temp db, so that the application can find the // complete chain at any time it might need it. // But we keep only those CA certs in the temp db, that we didn't already know. - nsRefPtr status = socketInfo->SSLStatus(); + nsRefPtr status = infoObject->SSLStatus(); nsRefPtr nsc; if (!status || !status->mServerCert) { @@ -896,7 +899,7 @@ AuthCertificate(nsNSSSocketInfo * socketInfo, CERTCertificate * cert) } if (blacklistErrorCode != 0) { - socketInfo->SetCertIssuerBlacklisted(); + infoObject->SetCertIssuerBlacklisted(); PORT_SetError(blacklistErrorCode); rv = SECFailure; } @@ -952,19 +955,19 @@ AuthCertificate(nsNSSSocketInfo * socketInfo, CERTCertificate * cert) // to the caller that contains at least the cert and its status. if (!status) { status = new nsSSLStatus(); - socketInfo->SetSSLStatus(status); + infoObject->SetSSLStatus(status); } if (rv == SECSuccess) { // Certificate verification succeeded delete any potential record // of certificate error bits. - RememberCertErrorsTable::GetInstance().RememberCertHasError(socketInfo, + RememberCertErrorsTable::GetInstance().RememberCertHasError(infoObject, nsnull, rv); } else { // Certificate verification failed, update the status' bits. RememberCertErrorsTable::GetInstance().LookupCertErrorBits( - socketInfo, status); + infoObject, status); } if (status && !status->mServerCert) { @@ -979,20 +982,19 @@ AuthCertificate(nsNSSSocketInfo * socketInfo, CERTCertificate * cert) /*static*/ SECStatus SSLServerCertVerificationJob::Dispatch(const void * fdForLogging, - nsNSSSocketInfo * socketInfo, + TransportSecurityInfo * infoObject, CERTCertificate * serverCert) { // Runs on the socket transport thread - if (!socketInfo || !serverCert) { + if (!infoObject || !serverCert) { NS_ERROR("Invalid parameters for SSL server cert validation"); PR_SetError(PR_INVALID_ARGUMENT_ERROR, 0); return SECFailure; } nsRefPtr job - = new SSLServerCertVerificationJob(fdForLogging, *socketInfo, *serverCert); + = new SSLServerCertVerificationJob(fdForLogging, infoObject, serverCert); - socketInfo->SetCertVerificationWaiting(); nsresult nrv; if (!gCertVerificationThreadPool) { nrv = NS_ERROR_NOT_INITIALIZED; @@ -1003,7 +1005,7 @@ SSLServerCertVerificationJob::Dispatch(const void * fdForLogging, // We can't call SetCertVerificationResult here to change // mCertVerificationState because SetCertVerificationResult will call // libssl functions that acquire SSL locks that are already being held at - // this point. socketInfo->mCertVerificationState will be stuck at + // this point. infoObject->mCertVerificationState will be stuck at // waiting_for_cert_verification here, but that is OK because we already // have to be able to handle cases where we encounter non-cert errors while // in that state. @@ -1024,21 +1026,21 @@ SSLServerCertVerificationJob::Run() // Runs on a cert verification thread PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, - ("[%p] SSLServerCertVerificationJob::Run\n", mSocketInfo.get())); + ("[%p] SSLServerCertVerificationJob::Run\n", mInfoObject.get())); PRErrorCode error; nsNSSShutDownPreventionLock nssShutdownPrevention; - if (mSocketInfo->isAlreadyShutDown()) { + if (mInfoObject->isAlreadyShutDown()) { error = SEC_ERROR_USER_CANCELLED; } else { // Reset the error code here so we can detect if AuthCertificate fails to // set the error code if/when it fails. PR_SetError(0, 0); - SECStatus rv = AuthCertificate(mSocketInfo, mCert); + SECStatus rv = AuthCertificate(mInfoObject, mCert); if (rv == SECSuccess) { nsRefPtr restart - = new SSLServerCertVerificationResult(*mSocketInfo, 0); + = new SSLServerCertVerificationResult(mInfoObject, 0); restart->Dispatch(); return NS_OK; } @@ -1046,7 +1048,7 @@ SSLServerCertVerificationJob::Run() error = PR_GetError(); if (error != 0) { nsRefPtr runnable = CreateCertErrorRunnable( - error, mSocketInfo, mCert, mFdForLogging); + error, mInfoObject, mCert, mFdForLogging); if (!runnable) { // CreateCertErrorRunnable set a new error code error = PR_GetError(); @@ -1083,7 +1085,7 @@ SSLServerCertVerificationJob::Run() } nsRefPtr failure - = new SSLServerCertVerificationResult(*mSocketInfo, error); + = new SSLServerCertVerificationResult(mInfoObject, error); failure->Dispatch(); return NS_OK; } @@ -1109,15 +1111,17 @@ AuthCertificateHook(void *arg, PRFileDesc *fd, PRBool checkSig, PRBool isServer) // and many things in PSM assume that we are a client. NS_ASSERTION(!isServer, "AuthCertificateHook: isServer unexpectedly true"); - if (!checkSig || isServer) { + nsNSSSocketInfo *socketInfo = static_cast(arg); + CERTCertificate *serverCert = SSL_PeerCertificate(fd); + CERTCertificateCleaner serverCertCleaner(serverCert); + + if (!checkSig || isServer || !socketInfo || !serverCert) { PR_SetError(PR_INVALID_STATE_ERROR, 0); return SECFailure; } - CERTCertificate *serverCert = SSL_PeerCertificate(fd); - CERTCertificateCleaner serverCertCleaner(serverCert); - - nsNSSSocketInfo *socketInfo = static_cast(arg); + if (BlockServerCertChangeForSpdy(socketInfo, serverCert) != SECSuccess) + return SECFailure; bool onSTSThread; nsresult nrv; @@ -1138,6 +1142,7 @@ AuthCertificateHook(void *arg, PRFileDesc *fd, PRBool checkSig, PRBool isServer) // we need the socket transport thread to be free for our OCSP requests, // and we *want* to do certificate verification on a background thread // because of the performance benefits of doing so. + socketInfo->SetCertVerificationWaiting(); SECStatus rv = SSLServerCertVerificationJob::Dispatch( static_cast(fd), socketInfo, serverCert); return rv; @@ -1203,9 +1208,9 @@ AuthCertificateHook(void *arg, PRFileDesc *fd, PRBool checkSig, PRBool isServer) } SSLServerCertVerificationResult::SSLServerCertVerificationResult( - nsNSSSocketInfo & socketInfo, PRErrorCode errorCode, + TransportSecurityInfo * infoObject, PRErrorCode errorCode, SSLErrorMessageType errorMessageType) - : mSocketInfo(&socketInfo) + : mInfoObject(infoObject) , mErrorCode(errorCode) , mErrorMessageType(errorMessageType) { @@ -1228,7 +1233,9 @@ NS_IMETHODIMP SSLServerCertVerificationResult::Run() { // TODO: Assert that we're on the socket transport thread - mSocketInfo->SetCertVerificationResult(mErrorCode, mErrorMessageType); + // XXX: This cast will be removed by the next patch + ((nsNSSSocketInfo *) mInfoObject.get()) + ->SetCertVerificationResult(mErrorCode, mErrorMessageType); return NS_OK; } diff --git a/security/manager/ssl/src/SSLServerCertVerification.h b/security/manager/ssl/src/SSLServerCertVerification.h index 83b16db43ebc..6cf620ce595e 100644 --- a/security/manager/ssl/src/SSLServerCertVerification.h +++ b/security/manager/ssl/src/SSLServerCertVerification.h @@ -41,11 +41,6 @@ #include "seccomon.h" #include "prio.h" -typedef struct PRFileDesc PRFileDesc; -typedef struct CERTCertificateStr CERTCertificate; -class nsNSSSocketInfo; -class nsNSSShutDownPreventionLock; - namespace mozilla { namespace psm { SECStatus AuthCertificateHook(void *arg, PRFileDesc *fd,