bug 1554152 - use the auto-clearing TLS context for connections with origin attributes from private contexts r=KevinJacobs

PSM has two instances of TLS bookkeeping structures ("SharedSSLState"): a
"public" one for most connections and a "private" one that automatically clears
its state when the last private browsing context (usually a window) closes.
Since we moved to separating connections by origin attributes, the latter is
largely redundant because keying by origin attributes already separates
connections from different contexts, even when using the "public" shared TLS
state structure. However, it still has the advantage of clearing its state when
the last private browsing context closes. This patch updates the decision of
which SharedSSLState to use by taking into account origin attributes. That is,
if the origin attributes of the connection has a private browsing ID that isn't
the default (unset), we'll use the auto-clearing SharedSSLState. This has the
effect of auto-clearing cached client auth certificate state for private
contexts when the last private browsing window closes. It also clears
accumulated TLS intolerance state in the private context, but that isn't as
relevant any more since we don't do TLS fallback by default.

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

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

security/manager/ssl/nsNSSIOLayer.cpp

@@ -2511,9 +2511,10 @@ nsresult nsSSLIOLayerAddToSocket(int32_t family, const char* host, int32_t port,
     allocatedState = new SharedSSLState(providerTlsFlags);
     sharedState = allocatedState.get();
   } else {
-    sharedState = (providerFlags & nsISocketProvider::NO_PERMANENT_STORAGE)
-                      ? PrivateSSLState()
-                      : PublicSSLState();
+    bool isPrivate = providerFlags & nsISocketProvider::NO_PERMANENT_STORAGE ||
+                     originAttributes.mPrivateBrowsingId !=
+                         OriginAttributes().mPrivateBrowsingId;
+    sharedState = isPrivate ? PrivateSSLState() : PublicSSLState();
   }
 
   nsNSSSocketInfo* infoObject =

security/manager/ssl/tests/mochitest/browser/browser_clientAuth_connection.js

@@ -24,6 +24,8 @@ let sdr = Cc["@mozilla.org/security/sdr;1"].getService(Ci.nsISecretDecoderRing);
 // Mock implementation of nsIClientAuthDialogs.
 const gClientAuthDialogs = {
   _state: DialogState.ASSERT_NOT_CALLED,
+  _rememberClientAuthCertificate: false,
+  _chooseCertificateCalled: false,
 
   set state(newState) {
     info(`old state: ${this._state}`);
@@ -35,8 +37,25 @@ const gClientAuthDialogs = {
     return this._state;
   },
 
+  set rememberClientAuthCertificate(value) {
+    this._rememberClientAuthCertificate = value;
+  },
+
+  get rememberClientAuthCertificate() {
+    return this._rememberClientAuthCertificate;
+  },
+
+  get chooseCertificateCalled() {
+    return this._chooseCertificateCalled;
+  },
+
+  set chooseCertificateCalled(value) {
+    this._chooseCertificateCalled = value;
+  },
+
   chooseCertificate(ctx, hostname, port, organization, issuerOrg, certList,
                     selectedIndex) {
+    this.chooseCertificateCalled = true;
     Assert.notEqual(this.state, DialogState.ASSERT_NOT_CALLED,
                     "chooseCertificate() should be called only when expected");
 
@@ -44,7 +63,7 @@ const gClientAuthDialogs = {
     Assert.notEqual(caud, null,
                     "nsIClientAuthUserDecision should be queryable from the " +
                     "given context");
-    caud.rememberClientAuthCertificate = false;
+    caud.rememberClientAuthCertificate = this.rememberClientAuthCertificate;
 
     Assert.equal(hostname, "requireclientcert.example.com",
                  "Hostname should be 'requireclientcert.example.com'");
@@ -91,33 +110,47 @@ add_task(async function setup() {
  *        If the connection is expected to load successfully, the URL that
  *        should load. If the connection is expected to fail and result in an
  *        error page, |undefined|.
+ * @param {Object} options
+ *        Optional options object to pass on to the window that gets opened.
  */
-async function testHelper(prefValue, expectedURL) {
+async function testHelper(prefValue, expectedURL, options = undefined) {
+  gClientAuthDialogs.chooseCertificateCalled = false;
   await SpecialPowers.pushPrefEnv({"set": [
     ["security.default_personal_cert", prefValue],
   ]});
 
-  await BrowserTestUtils.loadURI(gBrowser.selectedBrowser,
+  let win = await BrowserTestUtils.openNewBrowserWindow(options);
+
+  await BrowserTestUtils.loadURI(win.gBrowser.selectedBrowser,
                                  "https://requireclientcert.example.com:443");
 
   // |loadedURL| will be a string URL if browserLoaded() wins the race, or
   // |undefined| if waitForErrorPage() wins the race.
   let loadedURL = await Promise.race([
-    BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser),
-    BrowserTestUtils.waitForErrorPage(gBrowser.selectedBrowser),
+    BrowserTestUtils.browserLoaded(win.gBrowser.selectedBrowser),
+    BrowserTestUtils.waitForErrorPage(win.gBrowser.selectedBrowser),
   ]);
   Assert.equal(expectedURL, loadedURL, "Expected and actual URLs should match");
+  Assert.equal(gClientAuthDialogs.chooseCertificateCalled,
+               prefValue == "Ask Every Time",
+               "chooseCertificate should have been called if we were expecting it to be called");
 
-  // Ensure previously successful connections don't influence future tests.
-  sdr.logoutAndTeardown();
+  await win.close();
+
+  // This clears the TLS session cache so we don't use a previously-established
+  // ticket to connect and bypass selecting a client auth certificate in
+  // subsequent tests.
+  sdr.logout();
 }
 
 // Test that if a certificate is chosen automatically the connection succeeds,
 // and that nsIClientAuthDialogs.chooseCertificate() is never called.
 add_task(async function testCertChosenAutomatically() {
   gClientAuthDialogs.state = DialogState.ASSERT_NOT_CALLED;
-  await testHelper("Select Automatically",
-                    "https://requireclientcert.example.com/");
+  await testHelper("Select Automatically", "https://requireclientcert.example.com/");
+  // This clears all saved client auth certificate state so we don't influence
+  // subsequent tests.
+  sdr.logoutAndTeardown();
 });
 
 // Test that if the user doesn't choose a certificate, the connection fails and
@@ -125,11 +158,33 @@ add_task(async function testCertChosenAutomatically() {
 add_task(async function testCertNotChosenByUser() {
   gClientAuthDialogs.state = DialogState.RETURN_CERT_NOT_SELECTED;
   await testHelper("Ask Every Time", undefined);
+  sdr.logoutAndTeardown();
 });
 
 // Test that if the user chooses a certificate the connection suceeeds.
 add_task(async function testCertChosenByUser() {
   gClientAuthDialogs.state = DialogState.RETURN_CERT_SELECTED;
-  await testHelper("Ask Every Time",
-                    "https://requireclientcert.example.com/");
+  await testHelper("Ask Every Time", "https://requireclientcert.example.com/");
+  sdr.logoutAndTeardown();
+});
+
+// Test that if the user chooses a certificate in a private browsing window,
+// configures Firefox to remember this certificate for the duration of the
+// session, closes that window (and thus all private windows), reopens a private
+// window, and visits that site again, they are re-asked for a certificate (i.e.
+// any state from the previous private session should be gone). Similarly, after
+// closing that private window, if the user opens a non-private window, they
+// again should be asked to choose a certificate (i.e. private state should not
+// be remembered/used in non-private contexts).
+add_task(async function testClearPrivateBrowsingState() {
+  gClientAuthDialogs.rememberClientAuthCertificate = true;
+  gClientAuthDialogs.state = DialogState.RETURN_CERT_SELECTED;
+  await testHelper("Ask Every Time", "https://requireclientcert.example.com/", {private: true});
+  await testHelper("Ask Every Time", "https://requireclientcert.example.com/", {private: true});
+  await testHelper("Ask Every Time", "https://requireclientcert.example.com/");
+  // NB: we don't `sdr.logoutAndTeardown()` in between the two calls to
+  // `testHelper` because that would clear all client auth certificate state and
+  // obscure what we're testing (that Firefox properly clears the relevant state
+  // when the last private window closes).
+  sdr.logoutAndTeardown();
 });