bug 1485087 - remove the option to use the TLS session cache from nsITLSServerSocket r=jryans,mayhemer

As initially implemented, nsITLSServerSocket by default enabled the use of the
TLS session cache provided by NSS. However, no consumers of nsITLSServerSocket
actually used it. Because it was an option, though, PSM had to jump through some
hoops to a) make it work in the first place and b) not have NSS panic on
shutdown. Furthermore, it meant increased memory usage for every user of Firefox
(and again, nothing actually used the feature, so this was for naught).

In bug 1479918, we discovered that if PSM shut down before Necko, NSS could
attempt to acquire a lock on the session cache that had been deleted, causing a
shutdown hang. We probably should make it less easy to make this mistake in NSS,
but in the meantime bug 1479918 needs uplifting and this workaround is the
safest, most straight-forward way to achieve this.

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

--HG--
extra : moz-landing-system : lando

browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js

@@ -81,7 +81,6 @@ function startServer(cert) {
     }
   };
 
-  tlsServer.setSessionCache(false);
   tlsServer.setSessionTickets(false);
   tlsServer.setRequestClientCertificate(Ci.nsITLSServerSocket.REQUEST_ALWAYS);
 

devtools/shared/security/socket.js

@@ -485,7 +485,6 @@ SocketListener.prototype = {
   async _setAdditionalSocketOptions() {
     if (this.encryption) {
       this._socket.serverCert = await cert.local.getOrCreate();
-      this._socket.setSessionCache(false);
       this._socket.setSessionTickets(false);
       const requestCert = Ci.nsITLSServerSocket.REQUEST_NEVER;
       this._socket.setRequestClientCertificate(requestCert);

dom/presentation/provider/PresentationControlService.js

@@ -90,7 +90,6 @@ PresentationControlService.prototype = {
 
       if (aCert) {
         this._serverSocket.serverCert = aCert;
-        this._serverSocket.setSessionCache(false);
         this._serverSocket.setSessionTickets(false);
         let requestCert = Ci.nsITLSServerSocket.REQUEST_NEVER;
         this._serverSocket.setRequestClientCertificate(requestCert);

netwerk/base/TLSServerSocket.cpp

@@ -49,12 +49,12 @@ TLSServerSocket::SetSocketDefaults()
   SSL_OptionSet(mFD, SSL_SECURITY, true);
   SSL_OptionSet(mFD, SSL_HANDSHAKE_AS_CLIENT, false);
   SSL_OptionSet(mFD, SSL_HANDSHAKE_AS_SERVER, true);
+  SSL_OptionSet(mFD, SSL_NO_CACHE, true);
 
   // We don't currently notify the server API consumer of renegotiation events
   // (to revalidate peer certs, etc.), so disable it for now.
   SSL_OptionSet(mFD, SSL_ENABLE_RENEGOTIATION, SSL_RENEGOTIATE_NEVER);
 
-  SetSessionCache(true);
   SetSessionTickets(true);
   SetRequestClientCertificate(REQUEST_NEVER);
 
@@ -168,18 +168,6 @@ TLSServerSocket::SetServerCert(nsIX509Cert* aCert)
   return NS_OK;
 }
 
-NS_IMETHODIMP
-TLSServerSocket::SetSessionCache(bool aEnabled)
-{
-  // If AsyncListen was already called (and set mListener), it's too late to set
-  // this.
-  if (NS_WARN_IF(mListener)) {
-    return NS_ERROR_IN_PROGRESS;
-  }
-  SSL_OptionSet(mFD, SSL_NO_CACHE, !aEnabled);
-  return NS_OK;
-}
-
 NS_IMETHODIMP
 TLSServerSocket::SetSessionTickets(bool aEnabled)
 {

netwerk/base/nsITLSServerSocket.idl

@@ -19,15 +19,6 @@ interface nsITLSServerSocket : nsIServerSocket
    */
   attribute nsIX509Cert serverCert;
 
-  /**
-   * setSessionCache
-   *
-   * Whether the server should use a session cache.  Defaults to true.  This
-   * should be set before calling |asyncListen| if you wish to change the
-   * default.
-   */
-  void setSessionCache(in boolean aSessionCache);
-
   /**
    * setSessionTickets
    *

netwerk/test/unit/test_be_conservative.js

@@ -139,7 +139,6 @@ function startServer(cert, minServerVersion, maxServerVersion) {
   tlsServer.init(-1, true, -1);
   tlsServer.serverCert = cert;
   tlsServer.setVersionRange(minServerVersion, maxServerVersion);
-  tlsServer.setSessionCache(false);
   tlsServer.setSessionTickets(false);
   tlsServer.asyncListen(new ServerSocketListener());
   return tlsServer;

netwerk/test/unit/test_be_conservative_error_handling.js

@@ -139,7 +139,6 @@ function startServer(cert, minServerVersion, maxServerVersion) {
   tlsServer.init(-1, true, -1);
   tlsServer.serverCert = cert;
   tlsServer.setVersionRange(minServerVersion, maxServerVersion);
-  tlsServer.setSessionCache(false);
   tlsServer.setSessionTickets(false);
   tlsServer.asyncListen(new ServerSocketListener());
   return tlsServer;

netwerk/test/unit/test_tls_flags.js

@@ -118,7 +118,6 @@ function startServer(cert, minServerVersion, maxServerVersion, expectedVersion)
   tlsServer.init(-1, true, -1);
   tlsServer.serverCert = cert;
   tlsServer.setVersionRange(minServerVersion, maxServerVersion);
-  tlsServer.setSessionCache(false);
   tlsServer.setSessionTickets(false);
 
   let listener = {

netwerk/test/unit/test_tls_server.js

@@ -93,7 +93,6 @@ function startServer(cert, expectingPeerCert, clientCertificateConfig,
     }
   };
 
-  tlsServer.setSessionCache(false);
   tlsServer.setSessionTickets(false);
   tlsServer.setRequestClientCertificate(clientCertificateConfig);
 

netwerk/test/unit/test_tls_server_multiple_clients.js

@@ -66,7 +66,6 @@ function startServer(cert) {
     onStopListening: function() {}
   };
 
-  tlsServer.setSessionCache(true);
   tlsServer.setSessionTickets(false);
 
   tlsServer.asyncListen(listener);

security/manager/ssl/nsNSSComponent.cpp

@@ -203,7 +203,7 @@ nsNSSComponent::nsNSSComponent()
   , mLoadableRootsLoadedResult(NS_ERROR_FAILURE)
   , mMutex("nsNSSComponent.mMutex")
   , mMitmDetecionEnabled(false)
-  , mNonIdempotentCleanupMustHappen(false)
+  , mLoadLoadableRootsTaskDispatched(false)
 {
   MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("nsNSSComponent::ctor\n"));
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
@@ -1945,17 +1945,6 @@ nsNSSComponent::InitializeNSS()
       MOZ_ALWAYS_SUCCEEDS(handle->TrustLoaded3rdPartyRoots());
     }));
 
-    // TLSServerSocket may be run with the session cache enabled. It is
-    // necessary to call this once before that can happen. This specifies a
-    // maximum of 1000 cache entries (the default number of cache entries is
-    // 10000, which seems a little excessive as there probably won't be that
-    // many clients connecting to any TLSServerSockets the browser runs.) Note
-    // that this must occur before any calls to SSL_ClearSessionCache (otherwise
-    // memory will leak).
-    if (SSL_ConfigServerSessionIDCache(1000, 0, 0, nullptr) != SECSuccess) {
-      return NS_ERROR_FAILURE;
-    }
-
     // Set dynamic options from prefs. This has to run after
     // SSL_ConfigServerSessionIDCache.
     setValidationOptions(true, lock);
@@ -1967,7 +1956,7 @@ nsNSSComponent::InitializeNSS()
       return rv;
     }
 
-    mNonIdempotentCleanupMustHappen = true;
+    mLoadLoadableRootsTaskDispatched = true;
     return NS_OK;
   }
 }
@@ -1986,16 +1975,9 @@ nsNSSComponent::ShutdownNSS()
   // 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 (mNonIdempotentCleanupMustHappen) {
+  if (mLoadLoadableRootsTaskDispatched) {
     Unused << BlockUntilLoadableRootsLoaded();
-
-    // We can only run SSL_ShutdownServerSessionIDCache once (the rest of
-    // these operations are idempotent).
-    SSL_ClearSessionCache();
-    // TLSServerSocket may be run with the session cache enabled. This ensures
-    // those resources are cleaned up.
-    Unused << SSL_ShutdownServerSessionIDCache();
-    mNonIdempotentCleanupMustHappen = false;
+    mLoadLoadableRootsTaskDispatched = false;
   }
 
   ::mozilla::psm::UnloadLoadableRoots();

security/manager/ssl/nsNSSComponent.h

@@ -118,21 +118,13 @@ private:
 
   // The following members are accessed only on the main thread:
   static int mInstanceCount;
-  // If initialization (i.e. InitializeNSS) succeeds, we have called
-  // SSL_ConfigServerSessionIDCache. To clean this up, we must call
-  // SSL_ClearSessionCache and SSL_ShutdownServerSessionIDCache exactly once
-  // each (and if we haven't called SSL_ConfigServerSessionIDCache - for example
-  // if initialization failed - then we mustn't call the cleanup functions
-  // ever). There are multiple events that can cause us to enter our cleanup
-  // function (ShutdownNSS) and so we keep track of if we need to call these
-  // non-idempotent functions in the following boolean.
-  // Similarly, if InitializeNSS succeeds, then we have dispatched an event to
-  // load the loadable roots module on a background thread. We must wait for it
-  // to complete before attempting to unload the module again in ShutdownNSS. If
-  // we never dispatched the event, then we can't wait for it to complete
-  // (because it will never complete) so we again use this boolean to keep track
-  // of if we should wait.
-  bool mNonIdempotentCleanupMustHappen;
+  // If InitializeNSS succeeds, then we have dispatched an event to load the
+  // loadable roots module on a background thread. We must wait for it to
+  // complete before attempting to unload the module again in ShutdownNSS. If we
+  // never dispatched the event, then we can't wait for it to complete (because
+  // it will never complete) so we use this boolean to keep track of if we
+  // should wait.
+  bool mLoadLoadableRootsTaskDispatched;
 };
 
 inline nsresult

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

@@ -138,7 +138,6 @@ function getStartedServer(cert) {
                     .createInstance(Ci.nsITLSServerSocket);
   tlsServer.init(-1, true, -1);
   tlsServer.serverCert = cert;
-  tlsServer.setSessionCache(false);
   tlsServer.setSessionTickets(false);
   tlsServer.asyncListen(new ServerSocketListener());
   return tlsServer;

xpcom/build/XPCOMInit.cpp

@@ -103,6 +103,7 @@ extern nsresult nsStringInputStreamConstructor(nsISupports*, REFNSIID, void**);
 #include "nsSecurityConsoleMessage.h"
 #include "nsMessageLoop.h"
 #include "nss.h"
+#include "ssl.h"
 
 #include <locale.h>
 #include "mozilla/Services.h"
@@ -1037,6 +1038,7 @@ ShutdownXPCOM(nsIServiceManager* aServMgr)
   // down, any remaining objects that could be holding NSS resources (should)
   // have been released, so we can safely shut down NSS.
   if (NSS_IsInitialized()) {
+    SSL_ClearSessionCache();
     if (NSS_Shutdown() != SECSuccess) {
       // If you're seeing this crash and/or warning, some NSS resources are
       // still in use (see bugs 1417680 and 1230312).