Bug 1820628 - don't use server certificates to partition remembered client auth certificate decisions r=jschanck

Previously, remembered client authentication certificate decisions would be
partitioned by server certificate (as well as domain, obviously). This led to
unexpected behavior whereby a user could connect to the same domain multiple
times and be asked each time to choose a client certificate (given that a
single domain could be backed by multiple servers each with a different
certificate).

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

security/manager/ssl/TLSClientAuthCertSelection.cpp

@@ -575,9 +575,8 @@ void SelectClientAuthCertificate::DoSelectClientAuthCertificate() {
   if (cars) {
     nsCString rememberedDBKey;
     bool found;
-    nsCOMPtr<nsIX509Cert> cert(new nsNSSCertificate(mServerCert.get()));
     nsresult rv = cars->HasRememberedDecision(
-        hostname, mInfo.OriginAttributesRef(), cert, rememberedDBKey, &found);
+        hostname, mInfo.OriginAttributesRef(), rememberedDBKey, &found);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return;
     }
@@ -661,9 +660,8 @@ void SelectClientAuthCertificate::DoSelectClientAuthCertificate() {
   }
 
   if (cars && wantRemember) {
-    nsCOMPtr<nsIX509Cert> serverCert(new nsNSSCertificate(mServerCert.get()));
     rv = cars->RememberDecision(hostname, mInfo.OriginAttributesRef(),
-                                serverCert, selectedCert);
+                                selectedCert);
     Unused << NS_WARN_IF(NS_FAILED(rv));
   }
 }

security/manager/ssl/nsCertOverrideService.cpp

@@ -31,7 +31,6 @@
 #endif
 #include "nsISafeOutputStream.h"
 #include "nsIX509Cert.h"
-#include "nsNSSCertHelper.h"
 #include "nsNSSCertificate.h"
 #include "nsNSSComponent.h"
 #include "nsNetUtil.h"
@@ -389,6 +388,16 @@ nsresult nsCertOverrideService::Write(const MutexAutoLock& aProofOfLock) {
   return NS_OK;
 }
 
+nsresult GetCertSha256Fingerprint(nsIX509Cert* aCert, nsCString& aResult) {
+  nsAutoString fpStrUTF16;
+  nsresult rv = aCert->GetSha256Fingerprint(fpStrUTF16);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  aResult.Assign(NS_ConvertUTF16toUTF8(fpStrUTF16));
+  return NS_OK;
+}
+
 NS_IMETHODIMP
 nsCertOverrideService::RememberValidityOverride(
     const nsACString& aHostName, int32_t aPort,

security/manager/ssl/nsClientAuthRemember.cpp

@@ -13,7 +13,6 @@
 #include "nsINSSComponent.h"
 #include "nsPrintfCString.h"
 #include "nsNSSComponent.h"
-#include "nsNSSCertHelper.h"
 #include "nsIObserverService.h"
 #include "nsNetUtil.h"
 #include "nsPromiseFlatString.h"
@@ -49,12 +48,6 @@ nsClientAuthRemember::GetAsciiHost(/*out*/ nsACString& aAsciiHost) {
   return NS_OK;
 }
 
-NS_IMETHODIMP
-nsClientAuthRemember::GetFingerprint(/*out*/ nsACString& aFingerprint) {
-  aFingerprint = mFingerprint;
-  return NS_OK;
-}
-
 NS_IMETHODIMP
 nsClientAuthRemember::GetDbKey(/*out*/ nsACString& aDBKey) {
   aDBKey = mDBKey;
@@ -63,7 +56,11 @@ nsClientAuthRemember::GetDbKey(/*out*/ nsACString& aDBKey) {
 
 NS_IMETHODIMP
 nsClientAuthRemember::GetEntryKey(/*out*/ nsACString& aEntryKey) {
-  aEntryKey = mEntryKey;
+  aEntryKey.Assign(mAsciiHost);
+  aEntryKey.Append(',');
+  // This used to include the SHA-256 hash of the server certificate.
+  aEntryKey.Append(',');
+  aEntryKey.Append(mOriginAttributesSuffix);
   return NS_OK;
 }
 
@@ -76,7 +73,6 @@ nsresult nsClientAuthRememberService::Init() {
   mClientAuthRememberList =
       mozilla::DataStorage::Get(DataStorageClass::ClientAuthRememberList);
   nsresult rv = mClientAuthRememberList->Init();
-
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
@@ -158,43 +154,34 @@ nsClientAuthRememberService::DeleteDecisionsByHost(
 NS_IMETHODIMP
 nsClientAuthRememberService::RememberDecisionScriptable(
     const nsACString& aHostName, JS::Handle<JS::Value> aOriginAttributes,
-    nsIX509Cert* aServerCert, nsIX509Cert* aClientCert, JSContext* aCx) {
+    nsIX509Cert* aClientCert, JSContext* aCx) {
   OriginAttributes attrs;
   if (!aOriginAttributes.isObject() || !attrs.Init(aCx, aOriginAttributes)) {
     return NS_ERROR_INVALID_ARG;
   }
-  return RememberDecision(aHostName, attrs, aServerCert, aClientCert);
+  return RememberDecision(aHostName, attrs, aClientCert);
 }
 
 NS_IMETHODIMP
 nsClientAuthRememberService::RememberDecision(
     const nsACString& aHostName, const OriginAttributes& aOriginAttributes,
-    nsIX509Cert* aServerCert, nsIX509Cert* aClientCert) {
-  // aClientCert == nullptr means: remember that user does not want to use a
-  // cert
-  NS_ENSURE_ARG_POINTER(aServerCert);
+    nsIX509Cert* aClientCert) {
   if (aHostName.IsEmpty()) {
     return NS_ERROR_INVALID_ARG;
   }
 
-  nsAutoCString fpStr;
-  nsresult rv = GetCertSha256Fingerprint(aServerCert, fpStr);
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-
+  // aClientCert == nullptr means: remember that user does not want to use a
+  // cert
   if (aClientCert) {
     nsAutoCString dbkey;
-    rv = aClientCert->GetDbKey(dbkey);
-    if (NS_SUCCEEDED(rv)) {
-      AddEntryToList(aHostName, aOriginAttributes, fpStr, dbkey);
+    nsresult rv = aClientCert->GetDbKey(dbkey);
+    if (NS_FAILED(rv)) {
+      return rv;
     }
-  } else {
-    AddEntryToList(aHostName, aOriginAttributes, fpStr,
-                   nsClientAuthRemember::SentinelValue);
+    return AddEntryToList(aHostName, aOriginAttributes, dbkey);
   }
-
-  return NS_OK;
+  return AddEntryToList(aHostName, aOriginAttributes,
+                        nsClientAuthRemember::SentinelValue);
 }
 
 #ifdef XP_MACOSX
@@ -247,25 +234,57 @@ nsresult CheckForPreferredCertificate(const nsACString& aHostName,
 }
 #endif
 
+void nsClientAuthRememberService::Migrate() {
+  MOZ_ASSERT(NS_IsMainThread());
+  static bool migrated = false;
+  if (migrated) {
+    return;
+  }
+  nsTArray<DataStorageItem> decisions;
+  mClientAuthRememberList->GetAll(&decisions);
+  for (const auto& decision : decisions) {
+    if (decision.type != DataStorage_Persistent) {
+      continue;
+    }
+    RefPtr<nsClientAuthRemember> entry(
+        new nsClientAuthRemember(decision.key, decision.value));
+    nsAutoCString newKey;
+    if (NS_FAILED(entry->GetEntryKey(newKey))) {
+      continue;
+    }
+    if (newKey != decision.key) {
+      mClientAuthRememberList->Remove(decision.key, DataStorage_Persistent);
+      (void)mClientAuthRememberList->Put(newKey, decision.value,
+                                         DataStorage_Persistent);
+    }
+  }
+  migrated = true;
+}
+
 NS_IMETHODIMP
 nsClientAuthRememberService::HasRememberedDecision(
     const nsACString& aHostName, const OriginAttributes& aOriginAttributes,
-    nsIX509Cert* aCert, nsACString& aCertDBKey, bool* aRetVal) {
-  if (aHostName.IsEmpty()) return NS_ERROR_INVALID_ARG;
-
-  NS_ENSURE_ARG_POINTER(aCert);
+    nsACString& aCertDBKey, bool* aRetVal) {
   NS_ENSURE_ARG_POINTER(aRetVal);
+  if (aHostName.IsEmpty()) {
+    return NS_ERROR_INVALID_ARG;
+  }
+  if (!NS_IsMainThread()) {
+    return NS_ERROR_NOT_SAME_THREAD;
+  }
+
   *aRetVal = false;
   aCertDBKey.Truncate();
 
-  nsAutoCString fpStr;
-  nsresult rv = GetCertSha256Fingerprint(aCert, fpStr);
+  Migrate();
+
+  nsAutoCString entryKey;
+  RefPtr<nsClientAuthRemember> entry(
+      new nsClientAuthRemember(aHostName, aOriginAttributes));
+  nsresult rv = entry->GetEntryKey(entryKey);
   if (NS_FAILED(rv)) {
     return rv;
   }
-
-  nsAutoCString entryKey;
-  GetEntryKey(aHostName, aOriginAttributes, fpStr, entryKey);
   DataStorageType storageType = GetDataStorageType(aOriginAttributes);
 
   nsCString listEntry = mClientAuthRememberList->Get(entryKey, storageType);
@@ -294,23 +313,28 @@ nsClientAuthRememberService::HasRememberedDecision(
 NS_IMETHODIMP
 nsClientAuthRememberService::HasRememberedDecisionScriptable(
     const nsACString& aHostName, JS::Handle<JS::Value> aOriginAttributes,
-    nsIX509Cert* aCert, nsACString& aCertDBKey, JSContext* aCx, bool* aRetVal) {
+    nsACString& aCertDBKey, JSContext* aCx, bool* aRetVal) {
   OriginAttributes attrs;
   if (!aOriginAttributes.isObject() || !attrs.Init(aCx, aOriginAttributes)) {
     return NS_ERROR_INVALID_ARG;
   }
-  return HasRememberedDecision(aHostName, attrs, aCert, aCertDBKey, aRetVal);
+  return HasRememberedDecision(aHostName, attrs, aCertDBKey, aRetVal);
 }
 
 nsresult nsClientAuthRememberService::AddEntryToList(
     const nsACString& aHostName, const OriginAttributes& aOriginAttributes,
-    const nsACString& aFingerprint, const nsACString& aDBKey) {
+    const nsACString& aDBKey) {
   nsAutoCString entryKey;
-  GetEntryKey(aHostName, aOriginAttributes, aFingerprint, entryKey);
+  RefPtr<nsClientAuthRemember> entry(
+      new nsClientAuthRemember(aHostName, aOriginAttributes));
+  nsresult rv = entry->GetEntryKey(entryKey);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
   DataStorageType storageType = GetDataStorageType(aOriginAttributes);
 
   nsCString tmpDbKey(aDBKey);
-  nsresult rv = mClientAuthRememberList->Put(entryKey, tmpDbKey, storageType);
+  rv = mClientAuthRememberList->Put(entryKey, tmpDbKey, storageType);
   if (NS_FAILED(rv)) {
     return rv;
   }
@@ -318,21 +342,6 @@ nsresult nsClientAuthRememberService::AddEntryToList(
   return NS_OK;
 }
 
-void nsClientAuthRememberService::GetEntryKey(
-    const nsACString& aHostName, const OriginAttributes& aOriginAttributes,
-    const nsACString& aFingerprint, nsACString& aEntryKey) {
-  nsAutoCString hostCert(aHostName);
-  hostCert.Append(',');
-  hostCert.Append(aFingerprint);
-  hostCert.Append(',');
-
-  nsAutoCString suffix;
-  aOriginAttributes.CreateSuffix(suffix);
-  hostCert.Append(suffix);
-
-  aEntryKey.Assign(hostCert);
-}
-
 bool nsClientAuthRememberService::IsPrivateBrowsingKey(
     const nsCString& entryKey) {
   const int32_t separator = entryKey.Find(":");

security/manager/ssl/nsClientAuthRemember.h

@@ -31,27 +31,38 @@ class nsClientAuthRemember final : public nsIClientAuthRememberRecord {
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSICLIENTAUTHREMEMBERRECORD
 
+  nsClientAuthRemember(const nsACString& aHostName,
+                       const OriginAttributes& aOriginAttributes) {
+    mAsciiHost.Assign(aHostName);
+    aOriginAttributes.CreateSuffix(mOriginAttributesSuffix);
+  }
+
   nsClientAuthRemember(const nsCString& aEntryKey, const nsCString& aDBKey) {
-    mEntryKey = aEntryKey;
     if (!aDBKey.Equals(nsClientAuthRemember::SentinelValue)) {
       mDBKey = aDBKey;
     }
 
-    nsTArray<nsCString*> fields = {&mAsciiHost, &mFingerprint};
-
-    auto fieldsIter = fields.begin();
-    auto splitter = aEntryKey.Split(',');
-    auto splitterIter = splitter.begin();
-    for (; fieldsIter != fields.end() && splitterIter != splitter.end();
-         ++fieldsIter, ++splitterIter) {
-      (*fieldsIter)->Assign(*splitterIter);
+    size_t field_index = 0;
+    for (const auto& field : aEntryKey.Split(',')) {
+      switch (field_index) {
+        case 0:
+          mAsciiHost.Assign(field);
+          break;
+        case 1:
+          break;
+        case 2:
+          mOriginAttributesSuffix.Assign(field);
+          break;
+        default:
+          break;
+      }
+      field_index++;
     }
   }
 
   nsCString mAsciiHost;
-  nsCString mFingerprint;
+  nsCString mOriginAttributesSuffix;
   nsCString mDBKey;
-  nsCString mEntryKey;
   static const nsCString SentinelValue;
 
  protected:
@@ -67,11 +78,6 @@ class nsClientAuthRememberService final : public nsIClientAuthRememberService {
 
   nsresult Init();
 
-  static void GetEntryKey(const nsACString& aHostName,
-                          const OriginAttributes& aOriginAttributes,
-                          const nsACString& aFingerprint,
-                          /*out*/ nsACString& aEntryKey);
-
   static bool IsPrivateBrowsingKey(const nsCString& entryKey);
 
  protected:
@@ -84,8 +90,9 @@ class nsClientAuthRememberService final : public nsIClientAuthRememberService {
 
   nsresult AddEntryToList(const nsACString& aHost,
                           const OriginAttributes& aOriginAttributes,
-                          const nsACString& aServerFingerprint,
                           const nsACString& aDBKey);
+
+  void Migrate();
 };
 
 #endif

security/manager/ssl/nsIClientAuthRememberService.idl

@@ -19,18 +19,11 @@ interface nsIX509Cert;
 [scriptable, uuid(e92825af-7e81-4b5c-b412-8e1dd36d14fe)]
 interface nsIClientAuthRememberRecord : nsISupports
 {
-
   readonly attribute ACString asciiHost;
-
-  readonly attribute ACString fingerprint;
-
   readonly attribute ACString dbKey;
-
   readonly attribute ACString entryKey;
-
 };
 
-
 [scriptable, uuid(1dbc6eb6-0972-4bdb-9dc4-acd0abf72369)]
 interface nsIClientAuthRememberService : nsISupports
 {
@@ -46,25 +39,21 @@ interface nsIClientAuthRememberService : nsISupports
   [must_use, noscript]
   void rememberDecision(in ACString aHostName,
                         in const_OriginAttributesRef aOriginAttributes,
-                        in nsIX509Cert aServerCert,
                         in nsIX509Cert aClientCert);
 
   [implicit_jscontext]
   void rememberDecisionScriptable(in ACString aHostName,
                                   in jsval originAttributes,
-                                  in nsIX509Cert aServerCert,
                                   in nsIX509Cert aClientCert);
 
   [must_use, noscript]
   bool hasRememberedDecision(in ACString aHostName,
                              in const_OriginAttributesRef aOriginAttributes,
-                             in nsIX509Cert aServerCert,
                              out ACString aCertDBKey);
 
   [implicit_jscontext]
   bool hasRememberedDecisionScriptable(in ACString aHostName,
                                        in jsval originAttributes,
-                                       in nsIX509Cert aServerCert,
                                        out ACString aCertDBKey);
 
   [must_use]

security/manager/ssl/nsNSSCertHelper.cpp

@@ -97,13 +97,3 @@ void LossyUTF8ToUTF16(const char* str, uint32_t len,
     CopyASCIItoUTF16(span, result);
   }
 }
-
-nsresult GetCertSha256Fingerprint(nsIX509Cert* aCert, nsCString& aResult) {
-  nsAutoString fpStrUTF16;
-  nsresult rv = aCert->GetSha256Fingerprint(fpStrUTF16);
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-  aResult.Assign(NS_ConvertUTF16toUTF8(fpStrUTF16));
-  return NS_OK;
-}

security/manager/ssl/nsNSSCertHelper.h

@@ -28,6 +28,4 @@ nsresult PIPBundleFormatStringFromName(const char* stringName,
                                        const nsTArray<nsString>& params,
                                        nsAString& result);
 
-nsresult GetCertSha256Fingerprint(nsIX509Cert* aCert, nsCString& aResult);
-
 #endif  // nsNSSCertHelper_h

security/manager/ssl/tests/unit/test_client_auth_remember_service/ClientAuthRememberList.txt

@@ -0,0 +1,3 @@
+example.com,C9:65:33:89:EE:DC:4D:05:DA:16:3D:D0:12:61:BC:61:21:51:AF:2B:CC:C6:E1:72:B3:78:23:0F:13:B1:C7:4D,	0	19486	AAAA
+example.com,C9:65:33:89:EE:DC:4D:05:DA:16:3D:D0:12:61:BC:61:21:51:AF:2B:CC:C6:E1:72:B3:78:23:0F:13:B1:C7:4D,^partitionKey=%28https%2Cexample.com%29	0	19486	BBBB
+example.test,,	0	19486	CCCC

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

@@ -0,0 +1,64 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+"use strict";
+
+// This tests that the nsIClientAuthRememberService correctly reads its backing
+// state file.
+
+function run_test() {
+  let profile = do_get_profile();
+  let clientAuthRememberFile = do_get_file(
+    `test_client_auth_remember_service/${CLIENT_AUTH_FILE_NAME}`
+  );
+  clientAuthRememberFile.copyTo(profile, CLIENT_AUTH_FILE_NAME);
+
+  let clientAuthRememberService = Cc[
+    "@mozilla.org/security/clientAuthRememberService;1"
+  ].getService(Ci.nsIClientAuthRememberService);
+
+  let dbKey = {};
+  ok(
+    clientAuthRememberService.hasRememberedDecisionScriptable(
+      "example.com",
+      {},
+      dbKey
+    )
+  );
+  equal(dbKey.value, "AAAA");
+
+  dbKey = {};
+  ok(
+    clientAuthRememberService.hasRememberedDecisionScriptable(
+      "example.com",
+      { partitionKey: "(https,example.com)" },
+      dbKey
+    )
+  );
+  equal(dbKey.value, "BBBB");
+
+  ok(
+    !clientAuthRememberService.hasRememberedDecisionScriptable(
+      "example.org",
+      {},
+      {}
+    )
+  );
+  ok(
+    !clientAuthRememberService.hasRememberedDecisionScriptable(
+      "example.com",
+      { partitionKey: "(https,example.org)" },
+      {}
+    )
+  );
+
+  dbKey = {};
+  ok(
+    clientAuthRememberService.hasRememberedDecisionScriptable(
+      "example.test",
+      {},
+      dbKey
+    )
+  );
+  equal(dbKey.value, "CCCC");
+}

security/manager/ssl/tests/unit/xpcshell.ini

@@ -23,6 +23,7 @@ support-files =
   test_cert_utf8/**
   test_cert_version/**
   test_certDB_import/**
+  test_client_auth_remember_service/**
   test_content_signing/**
   test_crlite_filters/**
   test_crlite_preexisting/**
@@ -98,6 +99,7 @@ skip-if = toolkit == 'android' && processor == 'x86_64'
 [test_certDB_import_with_primary_password.js]
 # nsCertificateDialogs not available in geckoview, bug 1554276
 skip-if = toolkit == 'android' && processor == 'x86_64'
+[test_client_auth_remember_service_read.js]
 [test_constructX509FromBase64.js]
 [test_content_signing.js]
 [test_crlite_filters.js]

toolkit/components/antitracking/test/xpcshell/test_staticPartition_clientAuthRemember.js

@@ -27,11 +27,11 @@ function getOAWithPartitionKey(
 
 // These are not actual server and client certs. The ClientAuthRememberService
 // does not care which certs we store decisions for, as long as they're valid.
-let [serverCert, clientCert] = certDB.getCerts();
+let [clientCert] = certDB.getCerts();
 
 function addSecurityInfo({ host, topLevelBaseDomain, originAttributes = {} }) {
   let attrs = getOAWithPartitionKey({ topLevelBaseDomain }, originAttributes);
-  cars.rememberDecisionScriptable(host, attrs, serverCert, clientCert);
+  cars.rememberDecisionScriptable(host, attrs, clientCert);
 }
 
 function testSecurityInfo({
@@ -47,12 +47,7 @@ function testSecurityInfo({
     messageSuffix += ` partitioned under ${topLevelBaseDomain}`;
   }
 
-  let hasRemembered = cars.hasRememberedDecisionScriptable(
-    host,
-    attrs,
-    serverCert,
-    {}
-  );
+  let hasRemembered = cars.hasRememberedDecisionScriptable(host, attrs, {});
 
   Assert.equal(
     hasRemembered,

toolkit/components/cleardata/tests/unit/test_security_settings.js

@@ -22,7 +22,7 @@ let certDB = Cc["@mozilla.org/security/x509certdb;1"].getService(
 
 // These are not actual server and client certs. The ClientAuthRememberService
 // does not care which certs we store decisions for, as long as they're valid.
-let [serverCert, clientCert] = certDB.getCerts();
+let [clientCert] = certDB.getCerts();
 
 function addSecurityInfo({ host, topLevelBaseDomain, originAttributes = {} }) {
   let attrs = getOAWithPartitionKey({ topLevelBaseDomain }, originAttributes);
@@ -31,7 +31,7 @@ function addSecurityInfo({ host, topLevelBaseDomain, originAttributes = {} }) {
 
   gSSService.processHeader(uri, "max-age=1000;", attrs);
 
-  cars.rememberDecisionScriptable(host, attrs, serverCert, clientCert);
+  cars.rememberDecisionScriptable(host, attrs, clientCert);
 }
 
 function addTestSecurityInfo() {
@@ -82,12 +82,7 @@ function testSecurityInfo({
     `HSTS ${expectedHSTS ? "is set" : "is not set"} ${messageSuffix}`
   );
 
-  let hasRemembered = cars.hasRememberedDecisionScriptable(
-    host,
-    attrs,
-    serverCert,
-    {}
-  );
+  let hasRemembered = cars.hasRememberedDecisionScriptable(host, attrs, {});
   // CARS deleteDecisionsByHost does not include subdomains. That means for some
   // test cases we expect a different remembered state.
   Assert.equal(