fido/mac: Add |CountWebAuthnCredentials| method.

CountWebAuthnCredentials counts the number of Touch ID WebAuthn
credentials stored in the keychain for a given profile with a creation
date in a given interval. This will be used to implement a counter label
for these credentials in the browsing data deletion dialog.

Bug: 879548
Change-Id: Iacac0ee3c13f69a0b7f9ea1c1f2e1b10f0ce949e
Reviewed-on: https://chromium-review.googlesource.com/1239537
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593785}

device/fido/mac/browsing_data_deletion.h

@@ -15,9 +15,9 @@ namespace fido {
 namespace mac {
 
 // DeleteWebAuthnCredentiuals deletes Touch ID authenticator credentials from
-// the macOS keychain that were created within the time interval `[begin, end)`
-// and with the given metadata secret (which is tied to a browser profile).
-// The |keychain_access_group| parameter is an identifier tied to Chrome's code
+// the macOS keychain that were created within the given time interval and with
+// the given metadata secret (which is tied to a browser profile). The
+// |keychain_access_group| parameter is an identifier tied to Chrome's code
 // signing identity that identifies the set of all keychain items associated
 // with the Touch ID WebAuthentication authenticator.
 //
@@ -30,8 +30,16 @@ namespace mac {
 bool COMPONENT_EXPORT(DEVICE_FIDO)
     DeleteWebAuthnCredentials(const std::string& keychain_access_group,
                               const std::string& profile_metadata_secret,
-                              base::Time begin,
-                              base::Time end);
+                              base::Time created_not_before,
+                              base::Time created_not_after);
+
+// CountWebAuthnCredentials returns the number of credentials that would get
+// deleted by a call to |DeleteWebAuthnCredentials| with identical arguments.
+size_t COMPONENT_EXPORT(DEVICE_FIDO)
+    CountWebAuthnCredentials(const std::string& keychain_access_group,
+                             const std::string& profile_metadata_secret,
+                             base::Time created_not_before,
+                             base::Time created_not_after);
 
 }  // namespace mac
 }  // namespace fido

device/fido/mac/browsing_data_deletion.mm

@@ -15,6 +15,7 @@
 #include "base/mac/mac_logging.h"
 #include "base/mac/scoped_cftyperef.h"
 #include "base/optional.h"
+#include "base/stl_util.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
@@ -28,16 +29,41 @@ namespace mac {
 
 namespace {
 
-bool DoDeleteWebAuthnCredentials(const std::string& keychain_access_group,
-                                 const std::string& profile_metadata_secret,
-                                 base::Time begin,
-                                 base::Time end)
+// Erase all keychain items with a creation date that is not within [not_before,
+// not_after).
+void FilterKeychainItemsByCreationDate(
+    std::vector<base::ScopedCFTypeRef<CFDictionaryRef>>* keychain_items,
+    base::Time not_before,
+    base::Time not_after) {
+  base::EraseIf(
+      *keychain_items,
+      [not_before, not_after](const CFDictionaryRef& attributes) -> bool {
+        // If the creation date is missing for some obscure reason, treat as if
+        // the date is inside the interval, i.e. keep it in the list.
+        CFDateRef creation_date_cf =
+            base::mac::GetValueFromDictionary<CFDateRef>(attributes,
+                                                         kSecAttrCreationDate);
+        if (!creation_date_cf) {
+          return false;
+        }
+        base::Time creation_date = base::Time::FromCFAbsoluteTime(
+            CFDateGetAbsoluteTime(creation_date_cf));
+        return creation_date < not_before || creation_date >= not_after;
+      });
+}
+base::Optional<std::vector<base::ScopedCFTypeRef<CFDictionaryRef>>>
+QueryKeychainItemsForProfile(const std::string& keychain_access_group,
+                             const std::string& profile_metadata_secret,
+                             base::Time created_not_before,
+                             base::Time created_not_after)
     API_AVAILABLE(macosx(10.12.2)) {
   // Query the keychain for all items tagged with the given access group, which
   // should in theory yield all WebAuthentication credentials (for all
   // profiles). Sadly, the kSecAttrAccessGroup filter doesn't quite work, and
   // so we also get results from the legacy keychain that are tagged with no
   // keychain access group.
+  std::vector<base::ScopedCFTypeRef<CFDictionaryRef>> result;
+
   base::ScopedCFTypeRef<CFMutableDictionaryRef> query(
       CFDictionaryCreateMutable(kCFAllocatorDefault, 0, nullptr, nullptr));
   CFDictionarySetValue(query, kSecClass, kSecClassKey);
@@ -54,19 +80,22 @@ bool DoDeleteWebAuthnCredentials(const std::string& keychain_access_group,
         query, reinterpret_cast<CFTypeRef*>(keychain_items.InitializeInto()));
     if (status == errSecItemNotFound) {
       DVLOG(1) << "no credentials found";
-      return true;
+      return base::nullopt;
     }
     if (status != errSecSuccess) {
       OSSTATUS_DLOG(ERROR, status) << "SecItemCopyMatching failed";
-      return false;
+      return base::nullopt;
     }
   }
 
-  bool result = true;
-  // Determine which items from the result need to be deleted.
   for (CFIndex i = 0; i < CFArrayGetCount(keychain_items); ++i) {
     CFDictionaryRef attributes = base::mac::CFCast<CFDictionaryRef>(
         CFArrayGetValueAtIndex(keychain_items, i));
+    if (!attributes) {
+      DLOG(ERROR) << "unexpected result type";
+      return base::nullopt;
+    }
+
     // Skip items that don't belong to the correct keychain access group
     // because the kSecAttrAccessGroup filter is broken.
     CFStringRef attr_access_group =
@@ -77,6 +106,7 @@ bool DoDeleteWebAuthnCredentials(const std::string& keychain_access_group,
       DVLOG(1) << "missing/invalid access group";
       continue;
     }
+
     // If the RP ID, stored encrypted in the item's label, cannot be decrypted
     // with the given metadata secret, then the credential belongs to a
     // different profile and must be ignored.
@@ -93,30 +123,38 @@ bool DoDeleteWebAuthnCredentials(const std::string& keychain_access_group,
       continue;
     }
 
-    // If the creation date is out of range, the credential must be ignored.
-    // If the creation date is missing for some obscure reason, we delete.
-    CFDateRef creation_date_cf = base::mac::GetValueFromDictionary<CFDateRef>(
-        attributes, kSecAttrCreationDate);
-    if (!creation_date_cf) {
-      DVLOG(1) << "missing creation date; deleting anyway";
-    } else {
-      base::Time creation_date = base::Time::FromCFAbsoluteTime(
-          CFDateGetAbsoluteTime(creation_date_cf));
-      // Browsing data deletion API uses [begin, end) intervals.
-      if (creation_date < begin || creation_date >= end) {
-        DVLOG(1) << "creation date out of range";
-        continue;
-      }
-    }
+    result.push_back(base::ScopedCFTypeRef<CFDictionaryRef>(
+        attributes, base::scoped_policy::RETAIN));
+  }
+
+  FilterKeychainItemsByCreationDate(&result, created_not_before,
+                                    created_not_after);
+  return result;
+}
 
-    // The sane way to delete this item would be to build a query that has the
-    // kSecMatchItemList field set to a list of SecKeyRef objects that need
-    // deleting. Sadly, on macOS that appears to work only if you also set
-    // kSecAttrNoLegacy (which is an internal symbol); otherwise it appears to
-    // only search the "legacy" keychain and return errSecItemNotFound. What
-    // does work however, is lookup by the (unique) kSecAttrApplicationLabel
-    // (which stores the credential id). So we clumsily do this for each item
-    // instead.
+bool DoDeleteWebAuthnCredentials(const std::string& keychain_access_group,
+                                 const std::string& profile_metadata_secret,
+                                 base::Time created_not_before,
+                                 base::Time created_not_after)
+    API_AVAILABLE(macosx(10.12.2)) {
+  bool result = true;
+  base::Optional<std::vector<base::ScopedCFTypeRef<CFDictionaryRef>>>
+      keychain_items = QueryKeychainItemsForProfile(
+          keychain_access_group, profile_metadata_secret, created_not_before,
+          created_not_after);
+  if (!keychain_items) {
+    return false;
+  }
+
+  // The sane way to delete this item would be to build a query that has the
+  // kSecMatchItemList field set to a list of SecKeyRef objects that need
+  // deleting. Sadly, on macOS that appears to work only if you also set
+  // kSecAttrNoLegacy (which is an internal symbol); otherwise it appears to
+  // only search the "legacy" keychain and return errSecItemNotFound. What
+  // does work however, is to look up and delete by the (unique)
+  // kSecAttrApplicationLabel (which stores the credential id). So we clumsily
+  // do this for each item instead.
+  for (const CFDictionaryRef& attributes : *keychain_items) {
     CFDataRef sec_attr_app_label = base::mac::GetValueFromDictionary<CFDataRef>(
         attributes, kSecAttrApplicationLabel);
     if (!sec_attr_app_label) {
@@ -138,25 +176,58 @@ bool DoDeleteWebAuthnCredentials(const std::string& keychain_access_group,
   return result;
 }
 
+size_t DoCountWebAuthnCredentials(const std::string& keychain_access_group,
+                                  const std::string& profile_metadata_secret,
+                                  base::Time created_not_before,
+                                  base::Time created_not_after)
+    API_AVAILABLE(macosx(10.12.2)) {
+  base::Optional<std::vector<base::ScopedCFTypeRef<CFDictionaryRef>>>
+      keychain_items = QueryKeychainItemsForProfile(
+          keychain_access_group, profile_metadata_secret, created_not_before,
+          created_not_after);
+  if (!keychain_items) {
+    DLOG(ERROR) << "Failed to query credentials in keychain";
+    return 0;
+  }
+
+  return keychain_items->size();
+}
 }  // namespace
 
 bool DeleteWebAuthnCredentials(const std::string& keychain_access_group,
                                const std::string& profile_metadata_secret,
-                               base::Time begin,
-                               base::Time end) {
+                               base::Time created_not_before,
+                               base::Time created_not_after) {
 #if defined(OS_MACOSX)
   if (base::FeatureList::IsEnabled(device::kWebAuthTouchId)) {
     // Touch ID uses macOS APIs available in 10.12.2 or newer. No need to check
     // for credentials in lower OS versions.
     if (__builtin_available(macos 10.12.2, *)) {
       return DoDeleteWebAuthnCredentials(keychain_access_group,
-                                         profile_metadata_secret, begin, end);
+                                         profile_metadata_secret,
+                                         created_not_before, created_not_after);
     }
   }
 #endif  // defined(OS_MACOSX)
   return true;
 }
 
+size_t CountWebAuthnCredentials(const std::string& keychain_access_group,
+                                const std::string& profile_metadata_secret,
+                                base::Time created_not_before,
+                                base::Time created_not_after) {
+#if defined(OS_MACOSX)
+  if (base::FeatureList::IsEnabled(device::kWebAuthTouchId)) {
+    if (__builtin_available(macos 10.12.2, *)) {
+      return DoCountWebAuthnCredentials(keychain_access_group,
+                                        profile_metadata_secret,
+                                        created_not_before, created_not_after);
+    }
+  }
+#endif  // defined(OS_MACOSX)
+  return 0;
+}
+
 }  // namespace mac
 }  // namespace fido
 }  // namespace device

device/fido/mac/browsing_data_deletion_unittest.mm

@@ -44,6 +44,7 @@ namespace {
 constexpr char kKeychainAccessGroup[] =
     "EQHXZ8M8AV.com.google.chrome.webauthn.test";
 constexpr char kMetadataSecret[] = "supersecret";
+constexpr char kOtherMetadataSecret[] = "reallynotsosecret";
 
 constexpr std::array<uint8_t, kClientDataHashLength> kClientDataHash = {};
 constexpr char kRpId[] = "rp.example.com";
@@ -89,7 +90,7 @@ base::ScopedCFTypeRef<CFArrayRef> QueryAllCredentials() {
 
 // Returns the number of WebAuthn credentials in the keychain (for all
 // profiles), or -1 if an error occurs.
-ssize_t CredentialCount() {
+ssize_t KeychainItemCount() {
   base::ScopedCFTypeRef<CFArrayRef> items = QueryAllCredentials();
   return items ? CFArrayGetCount(items) : -1;
 }
@@ -147,12 +148,17 @@ class BrowsingDataDeletionTest : public testing::Test {
   }
 
   bool DeleteCredentials() { return DeleteCredentials(kMetadataSecret); }
-
   bool DeleteCredentials(const std::string& metadata_secret) {
     return DeleteWebAuthnCredentials(kKeychainAccessGroup, metadata_secret,
                                      base::Time(), base::Time::Max());
   }
 
+  size_t CountCredentials() { return CountCredentials(kMetadataSecret); }
+  size_t CountCredentials(const std::string& metadata_secret) {
+    return CountWebAuthnCredentials(kKeychainAccessGroup, metadata_secret,
+                                    base::Time(), base::Time::Max());
+  }
+
   base::test::ScopedFeatureList scoped_feature_list_;
   base::test::ScopedTaskEnvironment scoped_task_environment_;
   std::unique_ptr<TouchIdAuthenticator> authenticator_;
@@ -163,31 +169,30 @@ class BrowsingDataDeletionTest : public testing::Test {
 // running macOS 10.12.2 or later, and require user input (Touch ID).
 
 TEST_F(BrowsingDataDeletionTest, DISABLED_Basic) {
-  ASSERT_EQ(0, CredentialCount());
+  ASSERT_EQ(0, KeychainItemCount());
   ASSERT_TRUE(MakeCredential());
-  ASSERT_EQ(1, CredentialCount());
+  ASSERT_EQ(1, KeychainItemCount());
 
   EXPECT_TRUE(DeleteCredentials());
-  EXPECT_EQ(0, CredentialCount());
+  EXPECT_EQ(0, KeychainItemCount());
 }
 
 TEST_F(BrowsingDataDeletionTest, DISABLED_DifferentProfiles) {
   // Create credentials in two different profiles.
-  EXPECT_EQ(0, CredentialCount());
+  EXPECT_EQ(0, KeychainItemCount());
   ASSERT_TRUE(MakeCredential());
-  std::string other_metadata_secret = "reallynotsosecret";
-  auto other_authenticator = MakeAuthenticator(other_metadata_secret);
+  auto other_authenticator = MakeAuthenticator(kOtherMetadataSecret);
   ASSERT_TRUE(MakeCredential(other_authenticator.get()));
-  ASSERT_EQ(2, CredentialCount());
+  ASSERT_EQ(2, KeychainItemCount());
 
   // Delete credential from the first profile.
   EXPECT_TRUE(DeleteCredentials());
-  EXPECT_EQ(1, CredentialCount());
+  EXPECT_EQ(1, KeychainItemCount());
   // Only providing the correct secret removes the second credential.
   EXPECT_TRUE(DeleteCredentials());
-  EXPECT_EQ(1, CredentialCount());
-  EXPECT_TRUE(DeleteCredentials(other_metadata_secret));
-  EXPECT_EQ(0, CredentialCount());
+  EXPECT_EQ(1, KeychainItemCount());
+  EXPECT_TRUE(DeleteCredentials(kOtherMetadataSecret));
+  EXPECT_EQ(0, KeychainItemCount());
 }
 
 TEST_F(BrowsingDataDeletionTest, DISABLED_FeatureFlag) {
@@ -196,12 +201,23 @@ TEST_F(BrowsingDataDeletionTest, DISABLED_FeatureFlag) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndDisableFeature(device::kWebAuthTouchId);
 
-  ASSERT_EQ(0, CredentialCount());
+  ASSERT_EQ(0, KeychainItemCount());
   ASSERT_TRUE(MakeCredential());
 
   // DeleteCredentials() has no effect with the feature flag flipped off.
   EXPECT_TRUE(DeleteCredentials());
-  EXPECT_EQ(1, CredentialCount());
+  EXPECT_EQ(1, KeychainItemCount());
+}
+
+TEST_F(BrowsingDataDeletionTest, DISABLED_Count) {
+  EXPECT_EQ(0u, CountCredentials());
+  EXPECT_EQ(0u, CountCredentials(kOtherMetadataSecret));
+  EXPECT_TRUE(MakeCredential());
+  EXPECT_EQ(1u, CountCredentials());
+  EXPECT_EQ(0u, CountCredentials(kOtherMetadataSecret));
+
+  EXPECT_TRUE(DeleteCredentials());
+  EXPECT_EQ(0u, CountCredentials());
 }
 
 }  // namespace