fido: disable Touch ID if keychain-access-groups entitlement is missing

This changes TouchIdAuthenticator::IsAvailable to check whether the
current executable is signed with a keychain-access-groups entitlement
matching the value that the authenticator is instantiated with. Without
it, calls to the keychain API to access credentials will fail.

Some embedders seem to provide a TouchIdAuthenticatorConfig that
attempts to configure Touch ID but don't entitle their binary as
required. This results in Touch ID being available but
MakeCredential/GetAssertion calls hanging until they time out. With this
change, Touch ID will simply be unvavailable instead.

Bug: 898577
Change-Id: I7b653a492661f36c921ab4fcd8785d90c92612ac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1628734
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Reviewed-by: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664837}

content/browser/webauth/authenticator_common.cc

@@ -975,7 +975,9 @@ bool AuthenticatorCommon::IsUserVerifyingPlatformAuthenticatorAvailableImpl(
            ->IsWebAuthenticationTouchIdAuthenticatorSupported())
     return false;
 
-  return device::fido::mac::TouchIdAuthenticator::IsAvailable();
+  auto opt_config = request_delegate->GetTouchIdAuthenticatorConfig();
+  return opt_config &&
+         device::fido::mac::TouchIdAuthenticator::IsAvailable(*opt_config);
 #elif defined(OS_WIN)
   return base::FeatureList::IsEnabled(device::kWebAuthUseNativeWinApi) &&
          device::WinWebAuthnApiAuthenticator::
@@ -1447,8 +1449,7 @@ CreateTouchIdAuthenticatorIfAvailable(
     return nullptr;
   }
   return device::fido::mac::TouchIdAuthenticator::CreateIfAvailable(
-      std::move(opt_authenticator_config->keychain_access_group),
-      std::move(opt_authenticator_config->metadata_secret));
+      std::move(*opt_authenticator_config));
 }
 }  // namespace
 #endif

content/browser/webauth/authenticator_impl_unittest.cc

@@ -59,6 +59,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if defined(OS_MACOSX)
+#include "device/fido/mac/authenticator_config.h"
 #include "device/fido/mac/scoped_touch_id_test_environment.h"
 #endif
 
@@ -1523,14 +1524,6 @@ class TestAuthenticatorContentBrowserClient : public ContentBrowserClient {
         individual_attestation, attestation_consent, is_focused);
   }
 
-#if defined(OS_MACOSX)
-  bool IsWebAuthenticationTouchIdAuthenticatorSupported() override {
-    return supports_touch_id;
-  }
-
-  bool supports_touch_id = true;
-#endif
-
   // If set, this closure will be called when the subsequently constructed
   // delegate is informed that the request has started.
   base::OnceClosure action_callbacks_registered_callback;
@@ -2170,42 +2163,6 @@ TEST_F(AuthenticatorContentBrowserClientTest,
   }
 }
 
-#if defined(OS_MACOSX)
-TEST_F(AuthenticatorContentBrowserClientTest,
-       IsUVPAAFalseIfEmbedderDoesNotSupportTouchId) {
-  if (__builtin_available(macOS 10.12.2, *)) {
-    // Touch ID is hardware-supported, but not enabled by the embedder.
-    device::fido::mac::ScopedTouchIdTestEnvironment touch_id_test_environment;
-    touch_id_test_environment.SetTouchIdAvailable(true);
-    test_client_.supports_touch_id = false;
-
-    NavigateAndCommit(GURL(kTestOrigin1));
-    AuthenticatorPtr authenticator = ConnectToAuthenticator();
-
-    TestIsUvpaaCallback cb;
-    authenticator->IsUserVerifyingPlatformAuthenticatorAvailable(cb.callback());
-    cb.WaitForCallback();
-    EXPECT_FALSE(cb.value());
-  }
-}
-
-TEST_F(AuthenticatorContentBrowserClientTest, IsUVPAATrueIfTouchIdAvailable) {
-  if (__builtin_available(macOS 10.12.2, *)) {
-    device::fido::mac::ScopedTouchIdTestEnvironment touch_id_test_environment;
-    touch_id_test_environment.SetTouchIdAvailable(true);
-    test_client_.supports_touch_id = true;
-
-    NavigateAndCommit(GURL(kTestOrigin1));
-    AuthenticatorPtr authenticator = ConnectToAuthenticator();
-
-    TestIsUvpaaCallback cb;
-    authenticator->IsUserVerifyingPlatformAuthenticatorAvailable(cb.callback());
-    cb.WaitForCallback();
-    EXPECT_TRUE(cb.value());
-  }
-}
-#endif  // defined(OS_MACOSX)
-
 #if defined(OS_WIN)
 TEST_F(AuthenticatorContentBrowserClientTest, WinIsUVPAA) {
   for (const bool enable_win_webauthn_api : {false, true}) {
@@ -4028,4 +3985,97 @@ TEST_F(InternalAuthenticatorImplTest, GetAssertionOriginAndRpIds) {
   }
 }
 
+#if defined(OS_MACOSX)
+class TouchIdAuthenticatorRequestDelegate
+    : public AuthenticatorRequestClientDelegate {
+ public:
+  using TouchIdAuthenticatorConfig = ::device::fido::mac::AuthenticatorConfig;
+
+  explicit TouchIdAuthenticatorRequestDelegate(
+      TouchIdAuthenticatorConfig config)
+      : config_(std::move(config)) {}
+
+  base::Optional<TouchIdAuthenticatorConfig> GetTouchIdAuthenticatorConfig()
+      const override {
+    return config_;
+  }
+
+ private:
+  TouchIdAuthenticatorConfig config_;
+  DISALLOW_COPY_AND_ASSIGN(TouchIdAuthenticatorRequestDelegate);
+};
+
+class TouchIdAuthenticatorContentBrowserClient : public ContentBrowserClient {
+ public:
+  using TouchIdAuthenticatorConfig = ::device::fido::mac::AuthenticatorConfig;
+
+  std::unique_ptr<AuthenticatorRequestClientDelegate>
+  GetWebAuthenticationRequestDelegate(
+      RenderFrameHost* render_frame_host,
+      const std::string& relying_party_id) override {
+    return std::make_unique<TouchIdAuthenticatorRequestDelegate>(
+        touch_id_config);
+  }
+
+  bool IsWebAuthenticationTouchIdAuthenticatorSupported() override {
+    return supports_touch_id;
+  }
+
+  bool supports_touch_id = true;
+
+  TouchIdAuthenticatorConfig touch_id_config;
+};
+
+class TouchIdAuthenticatorContentBrowserClientTest
+    : public AuthenticatorContentBrowserClientTest {
+ protected:
+  TouchIdAuthenticatorContentBrowserClientTest() = default;
+
+  void SetUp() override {
+    AuthenticatorImplTest::SetUp();
+    old_client_ = SetBrowserClientForTesting(&test_client_);
+    NavigateAndCommit(GURL(kTestOrigin1));
+  }
+
+  void TearDown() override {
+    SetBrowserClientForTesting(old_client_);
+    AuthenticatorImplTest::TearDown();
+  }
+
+  TouchIdAuthenticatorContentBrowserClient test_client_;
+
+  API_AVAILABLE(macos(10.12.2))
+  device::fido::mac::ScopedTouchIdTestEnvironment touch_id_test_environment_;
+
+ private:
+  ContentBrowserClient* old_client_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(TouchIdAuthenticatorContentBrowserClientTest);
+};
+
+TEST_F(TouchIdAuthenticatorContentBrowserClientTest, IsUVPAA) {
+  if (__builtin_available(macOS 10.12.2, *)) {
+    for (const bool touch_id_enabled_in_browser_client : {false, true}) {
+      SCOPED_TRACE(::testing::Message() << "touch_id_enabled_in_browser_client="
+                                        << touch_id_enabled_in_browser_client);
+      for (const bool touch_id_available : {false, true}) {
+        SCOPED_TRACE(::testing::Message()
+                     << "touch_id_available=" << touch_id_available);
+        touch_id_test_environment_.SetTouchIdAvailable(touch_id_available);
+        test_client_.supports_touch_id = touch_id_enabled_in_browser_client;
+
+        AuthenticatorPtr authenticator = ConnectToAuthenticator();
+
+        TestIsUvpaaCallback cb;
+        authenticator->IsUserVerifyingPlatformAuthenticatorAvailable(
+            cb.callback());
+        cb.WaitForCallback();
+        EXPECT_EQ(cb.value(),
+                  touch_id_enabled_in_browser_client && touch_id_available);
+      }
+    }
+  }
+}
+#endif  // defined(OS_MACOSX)
+
 }  // namespace content

device/fido/mac/authenticator.h

@@ -22,27 +22,30 @@ namespace device {
 namespace fido {
 namespace mac {
 
+struct AuthenticatorConfig;
+
 class COMPONENT_EXPORT(DEVICE_FIDO) TouchIdAuthenticator
     : public FidoAuthenticator {
  public:
-  // IsAvailable returns whether Touch ID is available and enrolled on the
-  // current device.
+  // IsAvailable returns true iff Touch ID is available and
+  // enrolled on the current device and the current binary carries
+  // a keychain-access-groups entitlement that matches the one set
+  // in |config|.
   //
   // Note that this may differ from the result of
-  // AuthenticatorImpl::IsUserVerifyingPlatformAuthenticatorAvailable, which
-  // also checks whether the embedder supports this authenticator, and if the
-  // request occurs from an off-the-record/incognito context.
-  static bool IsAvailable();
+  // AuthenticatorImpl::IsUserVerifyingPlatformAuthenticatorAvailable(),
+  // which also checks whether the embedder supports this
+  // authenticator, and if the request occurs from an
+  // off-the-record/incognito context.
+  static bool IsAvailable(const AuthenticatorConfig& config);
 
   // CreateIfAvailable returns a TouchIdAuthenticator if IsAvailable() returns
   // true and nullptr otherwise.
   static std::unique_ptr<TouchIdAuthenticator> CreateIfAvailable(
-      std::string keychain_access_group,
-      std::string metadata_secret);
+      AuthenticatorConfig config);
 
   static std::unique_ptr<TouchIdAuthenticator> CreateForTesting(
-      std::string keychain_access_group,
-      std::string metadata_secret);
+      AuthenticatorConfig config);
 
   ~TouchIdAuthenticator() override;
 

device/fido/mac/authenticator.mm

@@ -21,6 +21,7 @@
 #include "device/fido/ctap_get_assertion_request.h"
 #include "device/fido/ctap_make_credential_request.h"
 #include "device/fido/fido_constants.h"
+#include "device/fido/mac/authenticator_config.h"
 #include "device/fido/mac/get_assertion_operation.h"
 #include "device/fido/mac/make_credential_operation.h"
 #include "device/fido/mac/util.h"
@@ -30,30 +31,28 @@ namespace fido {
 namespace mac {
 
 // static
-bool TouchIdAuthenticator::IsAvailable() {
+bool TouchIdAuthenticator::IsAvailable(const AuthenticatorConfig& config) {
   if (__builtin_available(macOS 10.12.2, *)) {
-    return TouchIdContext::TouchIdAvailable();
+    return TouchIdContext::TouchIdAvailable(config);
   }
   return false;
 }
 
 // static
 std::unique_ptr<TouchIdAuthenticator> TouchIdAuthenticator::CreateIfAvailable(
-    std::string keychain_access_group,
-    std::string metadata_secret) {
-  // N.B. IsAvailable also checks for the feature flag being set.
-  return IsAvailable() ? base::WrapUnique(new TouchIdAuthenticator(
-                             std::move(keychain_access_group),
-                             std::move(metadata_secret)))
-                       : nullptr;
+    AuthenticatorConfig config) {
+  return IsAvailable(config) ? base::WrapUnique(new TouchIdAuthenticator(
+                                   std::move(config.keychain_access_group),
+                                   std::move(config.metadata_secret)))
+                             : nullptr;
 }
 
 // static
 std::unique_ptr<TouchIdAuthenticator> TouchIdAuthenticator::CreateForTesting(
-    std::string keychain_access_group,
-    std::string metadata_secret) {
-  return base::WrapUnique(new TouchIdAuthenticator(
-      std::move(keychain_access_group), std::move(metadata_secret)));
+    AuthenticatorConfig config) {
+  return base::WrapUnique(
+      new TouchIdAuthenticator(std::move(config.keychain_access_group),
+                               std::move(config.metadata_secret)));
 }
 
 TouchIdAuthenticator::~TouchIdAuthenticator() = default;

device/fido/mac/browsing_data_deletion_unittest.mm

@@ -131,7 +131,7 @@ class BrowsingDataDeletionTest : public testing::Test {
   std::unique_ptr<TouchIdAuthenticator> MakeAuthenticator(
       std::string profile_metadata_secret) {
     return TouchIdAuthenticator::CreateForTesting(
-        kKeychainAccessGroup, std::move(profile_metadata_secret));
+        {kKeychainAccessGroup, std::move(profile_metadata_secret)});
   }
 
   bool MakeCredential() { return MakeCredential(authenticator_.get()); }

device/fido/mac/scoped_touch_id_test_environment.h

@@ -15,6 +15,7 @@ namespace device {
 namespace fido {
 namespace mac {
 
+struct AuthenticatorConfig;
 class FakeKeychain;
 class FakeTouchIdContext;
 class TouchIdContext;
@@ -46,10 +47,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO)
 
  private:
   static std::unique_ptr<TouchIdContext> ForwardCreate();
-  static bool ForwardTouchIdAvailable();
+  static bool ForwardTouchIdAvailable(const AuthenticatorConfig& config);
 
   std::unique_ptr<TouchIdContext> CreateTouchIdContext();
-  bool TouchIdAvailable();
+  bool TouchIdAvailable(const AuthenticatorConfig&);
 
   using CreateFuncPtr = decltype(&ForwardCreate);
   CreateFuncPtr touch_id_context_create_ptr_;

device/fido/mac/scoped_touch_id_test_environment.mm

@@ -8,6 +8,7 @@
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "device/fido/mac/authenticator_config.h"
 #include "device/fido/mac/fake_keychain.h"
 #include "device/fido/mac/fake_touch_id_context.h"
 
@@ -51,15 +52,17 @@ std::unique_ptr<TouchIdContext> ScopedTouchIdTestEnvironment::ForwardCreate() {
 }
 
 // static
-bool ScopedTouchIdTestEnvironment::ForwardTouchIdAvailable() {
-  return g_current_environment->TouchIdAvailable();
+bool ScopedTouchIdTestEnvironment::ForwardTouchIdAvailable(
+    const AuthenticatorConfig& config) {
+  return g_current_environment->TouchIdAvailable(config);
 }
 
 bool ScopedTouchIdTestEnvironment::SetTouchIdAvailable(bool available) {
   return touch_id_available_ = available;
 }
 
-bool ScopedTouchIdTestEnvironment::TouchIdAvailable() {
+bool ScopedTouchIdTestEnvironment::TouchIdAvailable(
+    const AuthenticatorConfig& config) {
   return touch_id_available_;
 }
 

device/fido/mac/touch_id_context.h

@@ -20,6 +20,8 @@ namespace device {
 namespace fido {
 namespace mac {
 
+struct AuthenticatorConfig;
+
 // TouchIdContext wraps a macOS Touch ID consent prompt for signing with a
 // secure enclave key. It is a essentially a simpler facade for the LAContext
 // class from the macOS LocalAuthentication framework (c.f.
@@ -40,9 +42,11 @@ class COMPONENT_EXPORT(DEVICE_FIDO)
   // Factory method for instantiating a TouchIdContext.
   static std::unique_ptr<TouchIdContext> Create();
 
-  // Returns whether Touch ID is supported on the current hardware and set up to
-  // be used.
-  static bool TouchIdAvailable();
+  // Returns whether Touch ID is available and enrolled on the
+  // current device and the current binary carries a
+  // keychain-access-groups entitlement that matches the one set
+  // in |config|.
+  static bool TouchIdAvailable(const AuthenticatorConfig& config);
 
   virtual ~TouchIdContext();
 
@@ -70,7 +74,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO)
   static TouchIdAvailableFuncPtr g_touch_id_available_;
 
   static std::unique_ptr<TouchIdContext> CreateImpl();
-  static bool TouchIdAvailableImpl();
+  static bool TouchIdAvailableImpl(const AuthenticatorConfig& config);
 
   void RunCallback(bool success);
 

device/fido/mac/touch_id_context.mm

@@ -4,15 +4,19 @@
 
 #include "device/fido/mac/touch_id_context.h"
 
-#import <Foundation/Foundation.h>
+#import <CoreFoundation/CoreFoundation.h>
+#import <Security/Security.h>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/mac/foundation_util.h"
+#include "base/mac/scoped_cftyperef.h"
 #include "base/memory/ptr_util.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/threading/sequenced_task_runner_handle.h"
+#include "components/device_event_log/device_event_log.h"
+#include "device/fido/mac/authenticator_config.h"
 
 namespace device {
 namespace fido {
@@ -29,6 +33,44 @@ base::ScopedCFTypeRef<SecAccessControlRef> DefaultAccessControl() {
           kSecAccessControlPrivateKeyUsage | kSecAccessControlUserPresence,
           nullptr));
 }
+
+// Returns whether the current binary is signed with a keychain-access-groups
+// entitlement that contains |keychain_access_group|. This is required for the
+// TouchIdAuthenticator to access key material stored in the Touch ID secure
+// enclave.
+bool BinaryHasKeychainAccessGroupEntitlement(
+    const std::string& keychain_access_group) {
+  base::ScopedCFTypeRef<SecCodeRef> code;
+  if (SecCodeCopySelf(kSecCSDefaultFlags, code.InitializeInto()) !=
+      errSecSuccess) {
+    return false;
+  }
+  base::ScopedCFTypeRef<CFDictionaryRef> signing_info;
+  if (SecCodeCopySigningInformation(code, kSecCSDefaultFlags,
+                                    signing_info.InitializeInto()) !=
+      errSecSuccess) {
+    return false;
+  }
+  CFDictionaryRef entitlements =
+      base::mac::GetValueFromDictionary<CFDictionaryRef>(
+          signing_info, kSecCodeInfoEntitlementsDict);
+  if (!entitlements) {
+    return false;
+  }
+  CFArrayRef keychain_access_groups =
+      base::mac::GetValueFromDictionary<CFArrayRef>(
+          entitlements,
+          base::ScopedCFTypeRef<CFStringRef>(
+              base::SysUTF8ToCFStringRef("keychain-access-groups")));
+  if (!keychain_access_groups) {
+    return false;
+  }
+  return CFArrayContainsValue(
+      keychain_access_groups,
+      CFRangeMake(0, CFArrayGetCount(keychain_access_groups)),
+      base::ScopedCFTypeRef<CFStringRef>(
+          base::SysUTF8ToCFStringRef(keychain_access_group)));
+}
 }  // namespace
 
 // static
@@ -47,7 +89,13 @@ std::unique_ptr<TouchIdContext> TouchIdContext::Create() {
 }
 
 // static
-bool TouchIdContext::TouchIdAvailableImpl() {
+bool TouchIdContext::TouchIdAvailableImpl(const AuthenticatorConfig& config) {
+  if (!BinaryHasKeychainAccessGroupEntitlement(config.keychain_access_group)) {
+    FIDO_LOG(ERROR) << "Touch ID unavailable because keychain-access-group "
+                       "entitlement is missing or incorrect";
+    return false;
+  }
+
   base::scoped_nsobject<LAContext> context([[LAContext alloc] init]);
   return
       [context canEvaluatePolicy:LAPolicyDeviceOwnerAuthenticationWithBiometrics
@@ -59,9 +107,9 @@ TouchIdContext::TouchIdAvailableFuncPtr TouchIdContext::g_touch_id_available_ =
     &TouchIdContext::TouchIdAvailableImpl;
 
 // static
-bool TouchIdContext::TouchIdAvailable() {
+bool TouchIdContext::TouchIdAvailable(const AuthenticatorConfig& config) {
   // Testing seam to allow faking Touch ID in tests.
-  return (*g_touch_id_available_)();
+  return (*g_touch_id_available_)(config);
 }
 
 TouchIdContext::TouchIdContext()