Bug 1720118 - store certificate error override and failed certificate chain information in the TLS token cache r=kershaw,jschanck,necko-reviewers

Differential Revision: https://phabricator.services.mozilla.com/D158793

netwerk/base/SSLTokensCache.cpp

@@ -46,6 +46,14 @@ SessionCacheInfo SessionCacheInfo::Clone() const {
                 *mSucceededCertChainBytes,
                 [](const auto& element) { return element.Clone(); }))
           : Nothing();
+  result.mIsBuiltCertChainRootBuiltInRoot = mIsBuiltCertChainRootBuiltInRoot;
+  result.mOverridableErrorCategory = mOverridableErrorCategory;
+  result.mFailedCertChainBytes =
+      mFailedCertChainBytes
+          ? Some(TransformIntoNewArray(
+                *mFailedCertChainBytes,
+                [](const auto& element) { return element.Clone(); }))
+          : Nothing();
   return result;
 }
 
@@ -64,12 +72,19 @@ SSLTokensCache::TokenCacheRecord::~TokenCacheRecord() {
 uint32_t SSLTokensCache::TokenCacheRecord::Size() const {
   uint32_t size = mToken.Length() + sizeof(mSessionCacheInfo.mEVStatus) +
                   sizeof(mSessionCacheInfo.mCertificateTransparencyStatus) +
-                  mSessionCacheInfo.mServerCertBytes.Length();
+                  mSessionCacheInfo.mServerCertBytes.Length() +
+                  sizeof(mSessionCacheInfo.mIsBuiltCertChainRootBuiltInRoot) +
+                  sizeof(mSessionCacheInfo.mOverridableErrorCategory);
   if (mSessionCacheInfo.mSucceededCertChainBytes) {
     for (const auto& cert : mSessionCacheInfo.mSucceededCertChainBytes.ref()) {
       size += cert.Length();
     }
   }
+  if (mSessionCacheInfo.mFailedCertChainBytes) {
+    for (const auto& cert : mSessionCacheInfo.mFailedCertChainBytes.ref()) {
+      size += cert.Length();
+    }
+  }
   return size;
 }
 
@@ -81,6 +96,10 @@ void SSLTokensCache::TokenCacheRecord::Reset() {
       nsITransportSecurityInfo::CERTIFICATE_TRANSPARENCY_NOT_APPLICABLE;
   mSessionCacheInfo.mServerCertBytes.Clear();
   mSessionCacheInfo.mSucceededCertChainBytes.reset();
+  mSessionCacheInfo.mIsBuiltCertChainRootBuiltInRoot.reset();
+  mSessionCacheInfo.mOverridableErrorCategory =
+      nsITransportSecurityInfo::OverridableErrorCategory::ERROR_UNSET;
+  mSessionCacheInfo.mFailedCertChainBytes.reset();
 }
 
 uint32_t SSLTokensCache::TokenCacheEntry::Size() const {
@@ -260,6 +279,30 @@ nsresult SSLTokensCache::Put(const nsACString& aKey, const uint8_t* aToken,
     return rv;
   }
 
+  nsITransportSecurityInfo::OverridableErrorCategory overridableErrorCategory;
+  rv = aSecInfo->GetOverridableErrorCategory(&overridableErrorCategory);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  Maybe<nsTArray<nsTArray<uint8_t>>> failedCertChainBytes;
+  nsTArray<RefPtr<nsIX509Cert>> failedCertArray;
+  rv = aSecInfo->GetFailedCertChain(failedCertArray);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  if (!failedCertArray.IsEmpty()) {
+    failedCertChainBytes.emplace();
+    for (const auto& cert : failedCertArray) {
+      nsTArray<uint8_t> rawCert;
+      nsresult rv = cert->GetRawDER(rawCert);
+      if (NS_FAILED(rv)) {
+        return rv;
+      }
+      failedCertChainBytes->AppendElement(std::move(rawCert));
+    }
+  }
+
   auto makeUniqueRecord = [&]() {
     auto rec = MakeUnique<TokenCacheRecord>();
     rec->mKey = aKey;
@@ -268,23 +311,18 @@ nsresult SSLTokensCache::Put(const nsACString& aKey, const uint8_t* aToken,
     rec->mToken.AppendElements(aToken, aTokenLen);
     rec->mId = ++sRecordId;
     rec->mSessionCacheInfo.mServerCertBytes = std::move(certBytes);
-
     rec->mSessionCacheInfo.mSucceededCertChainBytes =
-        succeededCertChainBytes
-            ? Some(TransformIntoNewArray(
-                  *succeededCertChainBytes,
-                  [](auto& element) { return nsTArray(std::move(element)); }))
-            : Nothing();
-
+        std::move(succeededCertChainBytes);
     if (isEV) {
       rec->mSessionCacheInfo.mEVStatus = psm::EVStatus::EV;
     }
-
     rec->mSessionCacheInfo.mCertificateTransparencyStatus =
         certificateTransparencyStatus;
-
     rec->mSessionCacheInfo.mIsBuiltCertChainRootBuiltInRoot =
         std::move(isBuiltCertChainRootBuiltInRoot);
+    rec->mSessionCacheInfo.mOverridableErrorCategory = overridableErrorCategory;
+    rec->mSessionCacheInfo.mFailedCertChainBytes =
+        std::move(failedCertChainBytes);
     return rec;
   };
 

netwerk/base/SSLTokensCache.h

@@ -5,16 +5,17 @@
 #ifndef SSLTokensCache_h_
 #define SSLTokensCache_h_
 
-#include "nsIMemoryReporter.h"
-#include "nsClassHashtable.h"
-#include "nsTArray.h"
+#include "CertVerifier.h"  // For EVStatus
+#include "TransportSecurityInfo.h"
 #include "mozilla/Maybe.h"
 #include "mozilla/StaticMutex.h"
 #include "mozilla/StaticPrefs_network.h"
 #include "mozilla/StaticPtr.h"
+#include "nsClassHashtable.h"
+#include "nsIMemoryReporter.h"
+#include "nsTArray.h"
+#include "nsTHashMap.h"
 #include "nsXULAppAPI.h"
-#include "TransportSecurityInfo.h"
-#include "CertVerifier.h"  // For EVStatus
 
 namespace mozilla {
 namespace net {
@@ -28,6 +29,8 @@ struct SessionCacheInfo {
   nsTArray<uint8_t> mServerCertBytes;
   Maybe<nsTArray<nsTArray<uint8_t>>> mSucceededCertChainBytes;
   Maybe<bool> mIsBuiltCertChainRootBuiltInRoot;
+  nsITransportSecurityInfo::OverridableErrorCategory mOverridableErrorCategory;
+  Maybe<nsTArray<nsTArray<uint8_t>>> mFailedCertChainBytes;
 };
 
 class SSLTokensCache : public nsIMemoryReporter {

netwerk/protocol/http/QuicSocketControl.cpp

@@ -69,8 +69,6 @@ void QuicSocketControl::CallAuthenticated() {
 }
 
 void QuicSocketControl::HandshakeCompleted() {
-  psm::RememberCertErrorsTable::GetInstance().LookupCertErrorBits(this);
-
   uint32_t state = nsIWebProgressListener::STATE_IS_SECURE;
 
   MutexAutoLock lock(mMutex);

security/manager/ssl/CommonSocketControl.cpp

@@ -235,7 +235,11 @@ void CommonSocketControl::RebuildCertificateInfoFromSSLTokenCache() {
   mozilla::net::SessionCacheInfo& info = *mSessionCacheInfo;
   nsCOMPtr<nsIX509Cert> cert(
       new nsNSSCertificate(std::move(info.mServerCertBytes)));
-  SetServerCert(cert, info.mEVStatus);
+  if (info.mOverridableErrorCategory == OverridableErrorCategory::ERROR_UNSET) {
+    SetServerCert(cert, info.mEVStatus);
+  } else {
+    SetStatusErrorBits(cert, info.mOverridableErrorCategory);
+  }
   SetCertificateTransparencyStatus(info.mCertificateTransparencyStatus);
   if (info.mSucceededCertChainBytes) {
     SetSucceededCertChain(std::move(*info.mSucceededCertChainBytes));
@@ -244,6 +248,10 @@ void CommonSocketControl::RebuildCertificateInfoFromSSLTokenCache() {
   if (info.mIsBuiltCertChainRootBuiltInRoot) {
     SetIsBuiltCertChainRootBuiltInRoot(*info.mIsBuiltCertChainRootBuiltInRoot);
   }
+
+  if (info.mFailedCertChainBytes) {
+    SetFailedCertChain(std::move(*info.mFailedCertChainBytes));
+  }
 }
 
 NS_IMETHODIMP

security/manager/ssl/SSLServerCertVerification.cpp

@@ -104,7 +104,7 @@
 #include "ScopedNSSTypes.h"
 #include "SharedCertVerifier.h"
 #include "SharedSSLState.h"
-#include "TransportSecurityInfo.h"  // For RememberCertErrorsTable
+#include "TransportSecurityInfo.h"
 #include "VerifySSLServerCertChild.h"
 #include "cert.h"
 #include "mozilla/Assertions.h"
@@ -1094,11 +1094,6 @@ SSLServerCertVerificationResult::Run() {
   mInfoObject->SetMadeOCSPRequest(mMadeOCSPRequests);
 
   if (mSucceeded) {
-    // Certificate verification succeeded. Delete any potential record of
-    // certificate error bits.
-    RememberCertErrorsTable::GetInstance().RememberCertHasError(mInfoObject,
-                                                                SECSuccess);
-
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
             ("SSLServerCertVerificationResult::Run setting NEW cert"));
     nsTArray<uint8_t> certBytes(mBuiltChain.ElementAt(0).Clone());

security/manager/ssl/TransportSecurityInfo.cpp

@@ -938,85 +938,6 @@ TransportSecurityInfo::GetClassIDNoAlloc(nsCID* aClassIDNoAlloc) {
   return NS_OK;
 }
 
-// RememberCertErrorsTable
-
-/*static*/
-RememberCertErrorsTable* RememberCertErrorsTable::sInstance = nullptr;
-
-RememberCertErrorsTable::RememberCertErrorsTable()
-    : mErrorHosts(), mMutex("RememberCertErrorsTable::mMutex") {}
-
-static nsresult GetHostPortKey(TransportSecurityInfo* infoObject,
-                               /*out*/ nsCString& result) {
-  MOZ_ASSERT(infoObject);
-  NS_ENSURE_ARG(infoObject);
-
-  result.Truncate();
-
-  result.Assign(infoObject->GetHostName());
-  result.Append(':');
-  result.AppendInt(infoObject->GetPort());
-
-  return NS_OK;
-}
-
-void RememberCertErrorsTable::RememberCertHasError(
-    TransportSecurityInfo* infoObject, SECStatus certVerificationResult) {
-  nsresult rv;
-
-  nsAutoCString hostPortKey;
-  rv = GetHostPortKey(infoObject, hostPortKey);
-  if (NS_FAILED(rv)) {
-    return;
-  }
-
-  if (certVerificationResult != SECSuccess) {
-    MOZ_ASSERT(infoObject->mHaveCertErrorBits,
-               "Must have error bits when remembering flags");
-    if (!infoObject->mHaveCertErrorBits) {
-      return;
-    }
-
-    MutexAutoLock lock(mMutex);
-    mErrorHosts.InsertOrUpdate(hostPortKey,
-                               infoObject->mOverridableErrorCategory);
-  } else {
-    MutexAutoLock lock(mMutex);
-    mErrorHosts.Remove(hostPortKey);
-  }
-}
-
-void RememberCertErrorsTable::LookupCertErrorBits(
-    TransportSecurityInfo* infoObject) {
-  // Get remembered error bits from our cache, because of SSL session caching
-  // the NSS library potentially hasn't notified us for this socket.
-  if (infoObject->mHaveCertErrorBits) {
-    // Rather do not modify bits if already set earlier
-    return;
-  }
-
-  nsresult rv;
-
-  nsAutoCString hostPortKey;
-  rv = GetHostPortKey(infoObject, hostPortKey);
-  if (NS_FAILED(rv)) {
-    return;
-  }
-
-  nsITransportSecurityInfo::OverridableErrorCategory overridableErrorCategory;
-  {
-    MutexAutoLock lock(mMutex);
-    if (!mErrorHosts.Get(hostPortKey, &overridableErrorCategory)) {
-      // No record was found, this host had no cert errors
-      return;
-    }
-  }
-
-  // This host had cert errors, update the bits correctly
-  infoObject->mHaveCertErrorBits = true;
-  infoObject->mOverridableErrorCategory = overridableErrorCategory;
-}
-
 void TransportSecurityInfo::SetStatusErrorBits(
     const nsCOMPtr<nsIX509Cert>& cert,
     OverridableErrorCategory overridableErrorCategory) {
@@ -1024,8 +945,6 @@ void TransportSecurityInfo::SetStatusErrorBits(
 
   mHaveCertErrorBits = true;
   mOverridableErrorCategory = overridableErrorCategory;
-
-  RememberCertErrorsTable::GetInstance().RememberCertHasError(this, SECFailure);
 }
 
 NS_IMETHODIMP

security/manager/ssl/TransportSecurityInfo.h

@@ -16,7 +16,6 @@
 #include "mozilla/RefPtr.h"
 #include "mozilla/ipc/TransportSecurityInfoUtils.h"
 #include "mozpkix/pkixtypes.h"
-#include "nsTHashMap.h"
 #include "nsIClassInfo.h"
 #include "nsIObjectInputStream.h"
 #include "nsIInterfaceRequestor.h"
@@ -219,37 +218,6 @@ class TransportSecurityInfo : public nsITransportSecurityInfo,
                                       MutexAutoLock& aProofOfLock);
 };
 
-class RememberCertErrorsTable {
- private:
-  RememberCertErrorsTable();
-
-  nsTHashMap<nsCStringHashKey,
-             nsITransportSecurityInfo::OverridableErrorCategory>
-      mErrorHosts MOZ_GUARDED_BY(mMutex);
-
- public:
-  void RememberCertHasError(TransportSecurityInfo* infoObject,
-                            SECStatus certVerificationResult);
-  void LookupCertErrorBits(TransportSecurityInfo* infoObject);
-
-  static void Init() { sInstance = new RememberCertErrorsTable(); }
-
-  static RememberCertErrorsTable& GetInstance() {
-    MOZ_ASSERT(sInstance);
-    return *sInstance;
-  }
-
-  static void Cleanup() {
-    delete sInstance;
-    sInstance = nullptr;
-  }
-
- private:
-  Mutex mMutex;
-
-  static RememberCertErrorsTable* sInstance;
-};
-
 }  // namespace psm
 }  // namespace mozilla
 

security/manager/ssl/nsNSSCallbacks.cpp

@@ -1125,7 +1125,6 @@ void HandshakeCallback(PRFileDesc* fd, void* client_data) {
 
   bool deprecatedTlsVer =
       (channelInfo.protocolVersion < SSL_LIBRARY_VERSION_TLS_1_2);
-  RememberCertErrorsTable::GetInstance().LookupCertErrorBits(infoObject);
 
   uint32_t state;
   if (renegotiationUnsafe || deprecatedTlsVer) {

security/manager/ssl/nsNSSComponent.cpp

@@ -304,7 +304,6 @@ nsNSSComponent::~nsNSSComponent() {
 
   ShutdownNSS();
   SharedSSLState::GlobalCleanup();
-  RememberCertErrorsTable::Cleanup();
   --mInstanceCount;
 
   MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("nsNSSComponent::dtor finished\n"));
@@ -1198,10 +1197,7 @@ nsresult CommonInit() {
   DisableMD5();
 
   mozilla::pkix::RegisterErrorTable();
-
   SharedSSLState::GlobalInit();
-  RememberCertErrorsTable::Init();
-
   SetValidationOptionsCommon();
 
   return NS_OK;
@@ -1210,7 +1206,6 @@ nsresult CommonInit() {
 void NSSShutdownForSocketProcess() {
   MOZ_ASSERT(XRE_IsSocketProcess());
   SharedSSLState::GlobalCleanup();
-  RememberCertErrorsTable::Cleanup();
 }
 
 bool HandleTLSPrefChange(const nsCString& prefName) {

security/manager/ssl/tests/unit/test_session_resumption.js

@@ -40,6 +40,7 @@ function add_resume_non_ev_with_override_test() {
     PRErrorCodeSuccess,
     null,
     transportSecurityInfo => {
+      ok(transportSecurityInfo.resumed, "connection should be resumed");
       ok(
         transportSecurityInfo.securityState &
           Ci.nsIWebProgressListener.STATE_CERT_USER_OVERRIDDEN,
@@ -48,7 +49,12 @@ function add_resume_non_ev_with_override_test() {
       equal(
         transportSecurityInfo.succeededCertChain.length,
         0,
-        "ev-test.example.com should not have succeededCertChain set"
+        "expired.example.com should not have succeededCertChain set"
+      );
+      equal(
+        transportSecurityInfo.failedCertChain.length,
+        2,
+        "expired.example.com should have failedCertChain set"
       );
       equal(
         transportSecurityInfo.overridableErrorCategory,
@@ -76,12 +82,17 @@ function add_resume_non_ev_with_override_test() {
 // verifies that it validates as EV (or not, if we're running a non-debug
 // build). This assumes that an appropriate OCSP responder is running or that
 // good responses are cached.
-function add_one_ev_test() {
+function add_one_ev_test(resumed) {
   add_connection_test(
     "ev-test.example.com",
     PRErrorCodeSuccess,
     null,
     transportSecurityInfo => {
+      equal(
+        transportSecurityInfo.resumed,
+        resumed,
+        "connection should be resumed or not resumed as expected"
+      );
       ok(
         !(
           transportSecurityInfo.securityState &
@@ -89,10 +100,16 @@ function add_one_ev_test() {
         ),
         "ev-test.example.com should not have STATE_CERT_USER_OVERRIDDEN flag"
       );
-      ok(
-        transportSecurityInfo.succeededCertChain,
+      equal(
+        transportSecurityInfo.succeededCertChain.length,
+        3,
         "ev-test.example.com should have succeededCertChain set"
       );
+      equal(
+        transportSecurityInfo.failedCertChain.length,
+        0,
+        "ev-test.example.com should not have failedCertChain set"
+      );
       equal(
         transportSecurityInfo.overridableErrorCategory,
         Ci.nsITransportSecurityInfo.ERROR_UNSET,
@@ -133,11 +150,11 @@ function add_resume_ev_test() {
   });
   // We should be able to connect and verify the certificate as EV (in debug
   // builds).
-  add_one_ev_test();
+  add_one_ev_test(false);
   // We should be able to connect again (using session resumption). In debug
   // builds, the certificate should be noted as EV. Again, it's important that
   // nothing clears the TLS cache in between these two operations.
-  add_one_ev_test();
+  add_one_ev_test(true);
 
   add_test(() => {
     ocspResponder.stop(run_next_test);