bug 1487258 - load enterprise roots on a background at startup r=jcj
authorDana Keeler <dkeeler@mozilla.com>
Wed, 05 Sep 2018 17:15:53 +0000
changeset 435012 3a800dfc6ad85fad2d0ef1b73e0345d44218681d
parent 435011 a9fd0a6da9c15dc5cff8bea67c94dc6ca35e9152
child 435013 df1f14eb13efba1cb6c86b7f512c058bb6172f82
push id107530
push userapavel@mozilla.com
push dateThu, 06 Sep 2018 04:44:27 +0000
treeherdermozilla-inbound@5f5d7a3ce332 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjcj
bugs1487258
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1487258 - load enterprise roots on a background at startup r=jcj Loading enterprise roots could potentially take a while, so we certainly shouldn't do it on the main thread at startup. Note that this doesn't address the case where a user enables the feature while Firefox is running. This isn't great but since it's an about:config preference rather than a first-class preference exposed in about:preferences, we can probably get away with it for now. Differential Revision: https://phabricator.services.mozilla.com/D4708
security/manager/ssl/nsINSSComponent.idl
security/manager/ssl/nsNSSComponent.cpp
security/manager/ssl/nsNSSComponent.h
--- a/security/manager/ssl/nsINSSComponent.idl
+++ b/security/manager/ssl/nsINSSComponent.idl
@@ -50,26 +50,16 @@ interface nsINSSComponent : nsISupports 
    * (i.e. root certificates gleaned from the OS certificate store). Returns
    * null otherwise.
    * Currently this is only implemented on Windows, so this function returns
    * null on all other platforms.
    */
   [noscript] nsIX509CertList getEnterpriseRoots();
 
   /**
-   * During initialization, nsINSSComponent collects any 3rd party root
-   * certificates from the OS that may be relevant (e.g. enterprise roots, the
-   * Family Safety root on Windows 8). However, to prevent opening a PKCS#11
-   * login prompt and potentially re-entering initialization, the component
-   * delays trusting these roots until a later event tick. This is the function
-   * that enables that.
-   */
-  [noscript] void trustLoaded3rdPartyRoots();
-
-  /**
    * For performance reasons, the builtin roots module is loaded on a background
    * thread. When any code that depends on the builtin roots module runs, it
    * must first wait for the module to be loaded.
    */
   [noscript] void blockUntilLoadableRootsLoaded();
 
   /**
    * In theory a token on a PKCS#11 module can be inserted or removed at any
--- a/security/manager/ssl/nsNSSComponent.cpp
+++ b/security/manager/ssl/nsNSSComponent.cpp
@@ -661,32 +661,36 @@ nsNSSComponent::MaybeImportEnterpriseRoo
   if (!NS_IsMainThread()) {
     return;
   }
   bool importEnterpriseRoots = Preferences::GetBool(kEnterpriseRootModePref,
                                                     false);
   if (!importEnterpriseRoots) {
     return;
   }
+  ImportEnterpriseRoots();
+}
 
+void
+nsNSSComponent::ImportEnterpriseRoots()
+{
   UniqueCERTCertList roots;
   nsresult rv = GatherEnterpriseRoots(roots);
   if (NS_FAILED(rv)) {
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("failed gathering enterprise roots"));
     return;
   }
 
   {
     MutexAutoLock lock(mMutex);
-    MOZ_ASSERT(!mEnterpriseRoots);
     mEnterpriseRoots = std::move(roots);
   }
 }
 
-NS_IMETHODIMP
+nsresult
 nsNSSComponent::TrustLoaded3rdPartyRoots()
 {
   // We can't call ChangeCertTrustWithPossibleAuthentication while holding
   // mMutex (because it could potentially call back in to nsNSSComponent and
   // attempt to acquire mMutex), so we copy mEnterpriseRoots.
   UniqueCERTCertList enterpriseRoots;
   {
     MutexAutoLock lock(mMutex);
@@ -769,31 +773,34 @@ nsNSSComponent::GetEnterpriseRoots(nsIX5
   }
   enterpriseRootsCertList.forget(enterpriseRoots);
   return NS_OK;
 }
 
 class LoadLoadableRootsTask final : public Runnable
 {
 public:
-  explicit LoadLoadableRootsTask(nsNSSComponent* nssComponent)
+  explicit LoadLoadableRootsTask(nsNSSComponent* nssComponent,
+                                 bool importEnterpriseRoots)
     : Runnable("LoadLoadableRootsTask")
     , mNSSComponent(nssComponent)
+    , mImportEnterpriseRoots(importEnterpriseRoots)
   {
     MOZ_ASSERT(nssComponent);
   }
 
   ~LoadLoadableRootsTask() = default;
 
   nsresult Dispatch();
 
 private:
   NS_IMETHOD Run() override;
   nsresult LoadLoadableRoots();
   RefPtr<nsNSSComponent> mNSSComponent;
+  bool mImportEnterpriseRoots;
   nsCOMPtr<nsIThread> mThread;
 };
 
 nsresult
 LoadLoadableRootsTask::Dispatch()
 {
   // Can't add 'this' as the event to run, since mThread may not be set yet
   nsresult rv = NS_NewNamedThread("LoadRoots", getter_AddRefs(mThread),
@@ -802,19 +809,16 @@ LoadLoadableRootsTask::Dispatch()
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   // Note: event must not null out mThread!
   return mThread->Dispatch(this, NS_DISPATCH_NORMAL);
 }
 
-// NB: If anything in this function can cause an acquisition of
-// nsNSSComponent::mMutex, this can potentially deadlock with
-// nsNSSComponent::Shutdown.
 NS_IMETHODIMP
 LoadLoadableRootsTask::Run()
 {
   // First we Run() on the "LoadRoots" thread, do our work, and then we Run()
   // again on the main thread so we can shut down the thread (since we don't
   // need it any more). We can't shut down the thread while we're *on* the
   // thread, which is why we do the dispatch to the main thread. CryptoTask.cpp
   // (which informs this code) says that we can't null out mThread. This appears
@@ -825,41 +829,54 @@ LoadLoadableRootsTask::Run()
   // which shouldn't be possible).
   if (NS_IsMainThread()) {
     if (mThread) {
       mThread->Shutdown();
     }
     return NS_OK;
   }
 
-  nsresult rv = LoadLoadableRoots();
-  if (NS_FAILED(rv)) {
+  nsresult loadLoadableRootsResult = LoadLoadableRoots();
+  if (NS_WARN_IF(NS_FAILED(loadLoadableRootsResult))) {
     MOZ_LOG(gPIPNSSLog, LogLevel::Error, ("LoadLoadableRoots failed"));
-    // We don't return rv here because then BlockUntilLoadableRootsLoaded will
-    // just wait forever. Instead we'll save its value (below) so we can inform
-    // code that relies on the roots module being present that loading it
-    // failed.
+    // We don't return loadLoadableRootsResult here because then
+    // BlockUntilLoadableRootsLoaded will just wait forever. Instead we'll save
+    // its value (below) so we can inform code that relies on the roots module
+    // being present that loading it failed.
   }
 
-  if (NS_SUCCEEDED(rv)) {
+  // Loading EV information will only succeed if we've successfully loaded the
+  // loadable roots module.
+  if (NS_SUCCEEDED(loadLoadableRootsResult)) {
     if (NS_FAILED(LoadExtendedValidationInfo())) {
       // This isn't a show-stopper in the same way that failing to load the
       // roots module is.
       MOZ_LOG(gPIPNSSLog, LogLevel::Error, ("failed to load EV info"));
     }
   }
+
+  if (mImportEnterpriseRoots) {
+    mNSSComponent->ImportEnterpriseRoots();
+  }
+  nsresult rv = mNSSComponent->TrustLoaded3rdPartyRoots();
+  if (NS_FAILED(rv)) {
+    MOZ_LOG(gPIPNSSLog, LogLevel::Error,
+            ("failed to trust loaded 3rd party roots"));
+  }
+
   {
     MonitorAutoLock rootsLoadedLock(mNSSComponent->mLoadableRootsLoadedMonitor);
     mNSSComponent->mLoadableRootsLoaded = true;
     // Cache the result of LoadLoadableRoots so BlockUntilLoadableRootsLoaded
     // can return it to all callers later.
-    mNSSComponent->mLoadableRootsLoadedResult = rv;
+    mNSSComponent->mLoadableRootsLoadedResult = loadLoadableRootsResult;
     rv = mNSSComponent->mLoadableRootsLoadedMonitor.NotifyAll();
     if (NS_WARN_IF(NS_FAILED(rv))) {
-      return rv;
+      MOZ_LOG(gPIPNSSLog, LogLevel::Error,
+              ("failed to notify loadable roots loaded monitor"));
     }
   }
 
   // Go back to the main thread to clean up this worker thread.
   return NS_DispatchToMainThread(this);
 }
 
 NS_IMETHODIMP
@@ -1845,24 +1862,23 @@ nsNSSComponent::InitializeNSS()
 
   rv = setEnabledTLSVersions();
   if (NS_FAILED(rv)) {
     return NS_ERROR_UNEXPECTED;
   }
 
   DisableMD5();
 
-  // Note that these functions do not change the trust of any loaded 3rd party
+  // Note that this function does not change the trust of any loaded 3rd party
   // roots. Because we're initializing the nsNSSComponent, and because if the
   // user has a master password set on the softoken it could cause the
   // authentication dialog to come up, we could conceivably re-enter
-  // nsNSSComponent initialization, which would be bad. Instead, we schedule an
-  // event to set the trust after the component has been initialized (below).
+  // nsNSSComponent initialization, which would be bad. Instead, we set the
+  // trust when the load loadable roots task finishes (below).
   MaybeEnableFamilySafetyCompatibility();
-  MaybeImportEnterpriseRoots();
 
   ConfigureTLSSessionIdentifiers();
 
   bool requireSafeNegotiation =
     Preferences::GetBool("security.ssl.require_safe_negotiation",
                          REQUIRE_SAFE_NEGOTIATION_DEFAULT);
   SSL_OptionSetDefault(SSL_REQUIRE_SAFE_NEGOTIATION, requireSafeNegotiation);
 
@@ -1934,59 +1950,58 @@ nsNSSComponent::InitializeNSS()
                            mContentSigningRootHash);
 
     mMitmCanaryIssuer.Truncate();
     Preferences::GetString("security.pki.mitm_canary_issuer",
                            mMitmCanaryIssuer);
     mMitmDetecionEnabled =
       Preferences::GetBool("security.pki.mitm_canary_issuer.enabled", true);
 
-    nsCOMPtr<nsINSSComponent> handle(this);
-    NS_DispatchToCurrentThread(NS_NewRunnableFunction("nsNSSComponent::TrustLoaded3rdPartyRoots",
-    [handle]() {
-      MOZ_ALWAYS_SUCCEEDS(handle->TrustLoaded3rdPartyRoots());
-    }));
-
     // Set dynamic options from prefs. This has to run after
     // SSL_ConfigServerSessionIDCache.
     setValidationOptions(true, lock);
 
+    bool importEnterpriseRoots = Preferences::GetBool(kEnterpriseRootModePref,
+                                                      false);
     RefPtr<LoadLoadableRootsTask> loadLoadableRootsTask(
-      new LoadLoadableRootsTask(this));
+      new LoadLoadableRootsTask(this, importEnterpriseRoots));
     rv = loadLoadableRootsTask->Dispatch();
     if (NS_FAILED(rv)) {
       return rv;
     }
 
     mLoadLoadableRootsTaskDispatched = true;
     return NS_OK;
   }
 }
 
 void
 nsNSSComponent::ShutdownNSS()
 {
   MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("nsNSSComponent::ShutdownNSS\n"));
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
-  MutexAutoLock lock(mMutex);
-
+  bool loadLoadableRootsTaskDispatched;
+  {
+    MutexAutoLock lock(mMutex);
+    loadLoadableRootsTaskDispatched = mLoadLoadableRootsTaskDispatched;
+  }
   // We have to block until the load loadable roots task has completed, because
   // otherwise we might try to unload the loadable roots while the loadable
   // roots loading thread is setting up EV information, which can cause
   // it to fail to find the roots it is expecting. However, if initialization
   // failed, we won't have dispatched the load loadable roots background task.
   // In that case, we don't want to block on an event that will never happen.
-  if (mLoadLoadableRootsTaskDispatched) {
+  if (loadLoadableRootsTaskDispatched) {
     Unused << BlockUntilLoadableRootsLoaded();
-    mLoadLoadableRootsTaskDispatched = false;
   }
 
   ::mozilla::psm::UnloadLoadableRoots();
 
+  MutexAutoLock lock(mMutex);
 #ifdef XP_WIN
   mFamilySafetyRoot = nullptr;
   mEnterpriseRoots = nullptr;
 #endif
 
   PK11_SetPasswordFunc((PK11PasswordFunc)nullptr);
 
   Preferences::RemoveObserver(this, "security.");
--- a/security/manager/ssl/nsNSSComponent.h
+++ b/security/manager/ssl/nsNSSComponent.h
@@ -76,21 +76,24 @@ private:
   void ShutdownNSS();
 
   void setValidationOptions(bool isInitialSetting,
                             const mozilla::MutexAutoLock& proofOfLock);
   nsresult setEnabledTLSVersions();
   nsresult RegisterObservers();
 
   void MaybeImportEnterpriseRoots();
+  void ImportEnterpriseRoots();
   void UnloadEnterpriseRoots();
 
   void MaybeEnableFamilySafetyCompatibility();
   void UnloadFamilySafetyRoot();
 
+  nsresult TrustLoaded3rdPartyRoots();
+
 #ifdef XP_WIN
   nsresult MaybeImportFamilySafetyRoot(PCCERT_CONTEXT certificate,
                                        bool& wasFamilySafetyRoot);
   nsresult LoadFamilySafetyRoot();
 #endif // XP_WIN
 
   // mLoadableRootsLoadedMonitor protects mLoadableRootsLoaded.
   mozilla::Monitor mLoadableRootsLoadedMonitor;