diff --git a/chrome/browser/metrics/authenticator_utility.cc b/chrome/browser/metrics/authenticator_utility.cc index 5c3426ca10854a..0b697c1515f892 100644 --- a/chrome/browser/metrics/authenticator_utility.cc +++ b/chrome/browser/metrics/authenticator_utility.cc @@ -38,8 +38,12 @@ void ReportAvailability(bool available) { #if defined(OS_MAC) void ReportUVPlatformAuthenticatorAvailabilityWithConfig( base::Optional config) { - ReportAvailability(config && - content::IsUVPlatformAuthenticatorAvailable(*config)); + if (!config) { + ReportAvailability(false); + return; + } + content::IsUVPlatformAuthenticatorAvailable( + *config, base::BindOnce(&ReportAvailability)); } void ReportUVPlatformAuthenticatorAvailabilityMainThreadMac() { @@ -91,13 +95,9 @@ void ReportUVPlatformAuthenticatorAvailability() { base::BindOnce( &ReportUVPlatformAuthenticatorAvailabilityMainThreadMac)); #elif defined(OS_WIN) - std::unique_ptr client_delegate = - std::make_unique(); - device::WinWebAuthnApi* win_webauthn_api = - device::WinWebAuthnApi::GetDefault(); - ReportAvailability( - win_webauthn_api && - content::IsUVPlatformAuthenticatorAvailable(win_webauthn_api)); + content::IsUVPlatformAuthenticatorAvailable( + device::WinWebAuthnApi::GetDefault(), + base::BindOnce(&ReportAvailability)); #elif BUILDFLAG(IS_CHROMEOS_ASH) // TODO(crbug.com/1181426): Reenable the IsUVPAA() startup metric on CrOS. #endif diff --git a/chromeos/dbus/u2f/fake_u2f_client.cc b/chromeos/dbus/u2f/fake_u2f_client.cc index 7f528269a65b37..c293d30f157cec 100644 --- a/chromeos/dbus/u2f/fake_u2f_client.cc +++ b/chromeos/dbus/u2f/fake_u2f_client.cc @@ -22,11 +22,12 @@ void FakeU2FClient::WaitForServiceToBeAvailable( FROM_HERE, base::BindOnce(std::move(callback), true)); } -base::Optional FakeU2FClient::IsUvpaaBlocking( - const u2f::IsUvpaaRequest& request) { +void FakeU2FClient::IsUvpaa(const u2f::IsUvpaaRequest& request, + DBusMethodCallback callback) { u2f::IsUvpaaResponse response; response.set_available(false); - return response; + base::ThreadTaskRunnerHandle::Get()->PostTask( + FROM_HERE, base::BindOnce(std::move(callback), std::move(response))); } void FakeU2FClient::IsU2FEnabled( diff --git a/chromeos/dbus/u2f/fake_u2f_client.h b/chromeos/dbus/u2f/fake_u2f_client.h index f06ae52657f18c..4bb5096db2a166 100644 --- a/chromeos/dbus/u2f/fake_u2f_client.h +++ b/chromeos/dbus/u2f/fake_u2f_client.h @@ -24,8 +24,8 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS_U2F) FakeU2FClient : public U2FClient { // U2FClient: void WaitForServiceToBeAvailable( WaitForServiceToBeAvailableCallback callback) override; - base::Optional IsUvpaaBlocking( - const u2f::IsUvpaaRequest& request) override; + void IsUvpaa(const u2f::IsUvpaaRequest& request, + DBusMethodCallback callback) override; void IsU2FEnabled(const u2f::IsUvpaaRequest& request, DBusMethodCallback callback) override; void MakeCredential( diff --git a/chromeos/dbus/u2f/u2f_client.cc b/chromeos/dbus/u2f/u2f_client.cc index ac9ad9002dd429..e524f0abcc5d7b 100644 --- a/chromeos/dbus/u2f/u2f_client.cc +++ b/chromeos/dbus/u2f/u2f_client.cc @@ -60,8 +60,8 @@ class U2FClientImpl : public U2FClient { // U2FClient: void WaitForServiceToBeAvailable( WaitForServiceToBeAvailableCallback callback) override; - base::Optional IsUvpaaBlocking( - const u2f::IsUvpaaRequest& request) override; + void IsUvpaa(const u2f::IsUvpaaRequest& request, + DBusMethodCallback callback) override; void IsU2FEnabled(const u2f::IsUvpaaRequest& request, DBusMethodCallback callback) override; void MakeCredential( @@ -106,26 +106,15 @@ void U2FClientImpl::WaitForServiceToBeAvailable( proxy_->WaitForServiceToBeAvailable(std::move(callback)); } -base::Optional U2FClientImpl::IsUvpaaBlocking( - const u2f::IsUvpaaRequest& request) { +void U2FClientImpl::IsUvpaa(const u2f::IsUvpaaRequest& request, + DBusMethodCallback callback) { dbus::MethodCall method_call(u2f::kU2FInterface, u2f::kU2FIsUvpaa); dbus::MessageWriter writer(&method_call); writer.AppendProtoAsArrayOfBytes(request); - - std::unique_ptr dbus_response = - proxy_->CallMethodAndBlock(&method_call, kU2FShortTimeout); - - if (!dbus_response) { - return base::nullopt; - } - - dbus::MessageReader reader(dbus_response.get()); - u2f::IsUvpaaResponse response; - if (!reader.PopArrayOfBytesAsProto(&response)) { - return base::nullopt; - } - - return response; + proxy_->CallMethod( + &method_call, kU2FShortTimeout, + base::BindOnce(&U2FClientImpl::HandleResponse, + weak_factory_.GetWeakPtr(), std::move(callback))); } void U2FClientImpl::IsU2FEnabled( diff --git a/chromeos/dbus/u2f/u2f_client.h b/chromeos/dbus/u2f/u2f_client.h index b775df8677ed7a..b5699ff972f8d3 100644 --- a/chromeos/dbus/u2f/u2f_client.h +++ b/chromeos/dbus/u2f/u2f_client.h @@ -44,9 +44,8 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS_U2F) U2FClient { // or fingerprint is enrolled). The name is short for // IsUserVerifyingPlatformAuthenticatorAvailable(), which is a method defined // in the WebAuthn spec. - // TODO(martinkr): Change to take a callback. - virtual base::Optional IsUvpaaBlocking( - const u2f::IsUvpaaRequest& request) = 0; + virtual void IsUvpaa(const u2f::IsUvpaaRequest& request, + DBusMethodCallback callback) = 0; // Returns whether the legacy enterprise policy to enable a U2F authenticator // that requires a power button press to register or sign with a credential is diff --git a/content/browser/webauth/authenticator_common.cc b/content/browser/webauth/authenticator_common.cc index 83022a3c55b8ee..f6b6bf87f078a8 100644 --- a/content/browser/webauth/authenticator_common.cc +++ b/content/browser/webauth/authenticator_common.cc @@ -19,8 +19,6 @@ #include "base/rand_util.h" #include "base/strings/string_piece.h" #include "base/strings/utf_string_conversion_utils.h" -#include "base/task/task_traits.h" -#include "base/task/thread_pool.h" #include "base/timer/timer.h" #include "build/build_config.h" #include "build/chromeos_buildflags.h" @@ -512,9 +510,11 @@ void IsUserVerifyingPlatformAuthenticatorAvailableImpl( #if defined(OS_MAC) const base::Optional config = delegate->GetTouchIdAuthenticatorConfig(); - std::move(callback).Run(config && - IsUVPlatformAuthenticatorAvailable(*config)); - return; + if (!config) { + std::move(callback).Run(false); + return; + } + IsUVPlatformAuthenticatorAvailable(*config, std::move(callback)); #elif defined(OS_WIN) // TODO(crbug.com/908622): Enable platform authenticators in Incognito on // Windows once the API allows triggering an adequate warning dialog. @@ -523,19 +523,12 @@ void IsUserVerifyingPlatformAuthenticatorAvailableImpl( return; } - std::move(callback).Run(IsUVPlatformAuthenticatorAvailable( - discovery_factory->win_webauthn_api())); - return; + IsUVPlatformAuthenticatorAvailable(discovery_factory->win_webauthn_api(), + std::move(callback)); #elif BUILDFLAG(IS_CHROMEOS_ASH) - // ChromeOS needs to do a dbus call to determine platform authenticator - // availability. The call is fast in practice, but nonetheless may - // theoretically block. - base::ThreadPool::PostTaskAndReplyWithResult( - FROM_HERE, {base::TaskPriority::USER_BLOCKING, base::MayBlock()}, - base::BindOnce(&IsUVPlatformAuthenticatorAvailable), std::move(callback)); + IsUVPlatformAuthenticatorAvailable(std::move(callback)); #else std::move(callback).Run(false); - return; #endif } diff --git a/content/browser/webauth/authenticator_impl_unittest.cc b/content/browser/webauth/authenticator_impl_unittest.cc index 4df6ff42c9209c..69103434db00b1 100644 --- a/content/browser/webauth/authenticator_impl_unittest.cc +++ b/content/browser/webauth/authenticator_impl_unittest.cc @@ -857,11 +857,10 @@ TEST(ClientDataSerializationTest, Sign) { } TEST_F(AuthenticatorImplTest, TestMakeCredentialTimeout) { - // The VirtualFidoAuthenticator simulates a tap immediately after it gets the - // request. Replace by the real discovery that will wait until timeout. - AuthenticatorEnvironmentImpl::GetInstance() - ->ReplaceDefaultDiscoveryFactoryForTesting( - std::make_unique()); + // Don't provide an authenticator tap so the request times out. + virtual_device_factory_->mutable_state()->simulate_press_callback = + base::BindLambdaForTesting( + [&](device::VirtualFidoDevice* device) { return false; }); NavigateAndCommit(GURL(kTestOrigin1)); PublicKeyCredentialCreationOptionsPtr options = diff --git a/content/browser/webauth/is_uvpaa.cc b/content/browser/webauth/is_uvpaa.cc index 3083b6e2c9717d..deb58ec5362c7f 100644 --- a/content/browser/webauth/is_uvpaa.cc +++ b/content/browser/webauth/is_uvpaa.cc @@ -25,31 +25,39 @@ namespace content { #if defined(OS_MAC) -bool IsUVPlatformAuthenticatorAvailable( +void IsUVPlatformAuthenticatorAvailable( const content::AuthenticatorRequestClientDelegate:: - TouchIdAuthenticatorConfig& config) { - return device::fido::mac::TouchIdAuthenticator::IsAvailable(config); + TouchIdAuthenticatorConfig& config, + IsUVPlatformAuthenticatorAvailableCallback callback) { + device::fido::mac::TouchIdAuthenticator::IsAvailable(config, + std::move(callback)); } #elif defined(OS_WIN) -bool IsUVPlatformAuthenticatorAvailable( - device::WinWebAuthnApi* win_webauthn_api) { - return base::FeatureList::IsEnabled(device::kWebAuthUseNativeWinApi) && - device::WinWebAuthnApiAuthenticator:: - IsUserVerifyingPlatformAuthenticatorAvailable(win_webauthn_api); +void IsUVPlatformAuthenticatorAvailable( + device::WinWebAuthnApi* win_webauthn_api, + IsUVPlatformAuthenticatorAvailableCallback callback) { + device::WinWebAuthnApiAuthenticator:: + IsUserVerifyingPlatformAuthenticatorAvailable(win_webauthn_api, + std::move(callback)); } #elif BUILDFLAG(IS_CHROMEOS_ASH) -bool IsUVPlatformAuthenticatorAvailable() { - return base::FeatureList::IsEnabled( - device::kWebAuthCrosPlatformAuthenticator) && - device::ChromeOSAuthenticator:: - IsUVPlatformAuthenticatorAvailableBlocking(); +void IsUVPlatformAuthenticatorAvailable( + IsUVPlatformAuthenticatorAvailableCallback callback) { + if (!base::FeatureList::IsEnabled( + device::kWebAuthCrosPlatformAuthenticator)) { + std::move(callback).Run(false); + return; + } + device::ChromeOSAuthenticator::IsUVPlatformAuthenticatorAvailable( + std::move(callback)); } #else -bool IsUVPlatformAuthenticatorAvailable() { - return false; +void IsUVPlatformAuthenticatorAvailable( + IsUVPlatformAuthenticatorAvailableCallback callback) { + std::move(callback).Run(false); } #endif diff --git a/content/public/browser/is_uvpaa.h b/content/public/browser/is_uvpaa.h index f7587f684d0ac6..aec3712dbad594 100644 --- a/content/public/browser/is_uvpaa.h +++ b/content/public/browser/is_uvpaa.h @@ -5,6 +5,7 @@ #ifndef CONTENT_PUBLIC_BROWSER_IS_UVPAA_H_ #define CONTENT_PUBLIC_BROWSER_IS_UVPAA_H_ +#include "base/callback.h" #include "build/build_config.h" #include "build/chromeos_buildflags.h" #include "content/common/content_export.h" @@ -23,23 +24,31 @@ namespace content { // OS. This is exposed through the content public API for the purpose of // reporting startup metrics. +using IsUVPlatformAuthenticatorAvailableCallback = + base::OnceCallback; + #if defined(OS_MAC) -CONTENT_EXPORT bool IsUVPlatformAuthenticatorAvailable( +CONTENT_EXPORT void IsUVPlatformAuthenticatorAvailable( const content::AuthenticatorRequestClientDelegate:: - TouchIdAuthenticatorConfig&); + TouchIdAuthenticatorConfig&, + IsUVPlatformAuthenticatorAvailableCallback); #elif defined(OS_WIN) -CONTENT_EXPORT bool IsUVPlatformAuthenticatorAvailable(device::WinWebAuthnApi*); +CONTENT_EXPORT void IsUVPlatformAuthenticatorAvailable( + device::WinWebAuthnApi*, + IsUVPlatformAuthenticatorAvailableCallback); #elif BUILDFLAG(IS_CHROMEOS_ASH) -CONTENT_EXPORT bool IsUVPlatformAuthenticatorAvailable(); +CONTENT_EXPORT void IsUVPlatformAuthenticatorAvailable( + IsUVPlatformAuthenticatorAvailableCallback); #else // Always returns false. On Android IsUVPlatformAuthenticatorAvailable() is // called on GMSCore from Java and is not proxied from here because there could // be performance costs to calling it outside of an actual WebAuthn API // invocation. -CONTENT_EXPORT bool IsUVPlatformAuthenticatorAvailable(); +CONTENT_EXPORT void IsUVPlatformAuthenticatorAvailable( + IsUVPlatformAuthenticatorAvailableCallback); #endif } // namespace content diff --git a/device/fido/cros/authenticator.cc b/device/fido/cros/authenticator.cc index ab80417ce7e593..3cb8ebf97da0dd 100644 --- a/device/fido/cros/authenticator.cc +++ b/device/fido/cros/authenticator.cc @@ -331,14 +331,18 @@ void ChromeOSAuthenticator::OnCancelResponse( } } -// static -bool ChromeOSAuthenticator::IsUVPlatformAuthenticatorAvailableBlocking() { - base::Optional response = - chromeos::U2FClient::Get()->IsUvpaaBlocking(u2f::IsUvpaaRequest()); - return response && response->available(); +void ChromeOSAuthenticator::IsUVPlatformAuthenticatorAvailable( + base::OnceCallback callback) { + chromeos::U2FClient::Get()->IsUvpaa( + u2f::IsUvpaaRequest(), + base::BindOnce( + [](base::OnceCallback callback, + base::Optional response) { + std::move(callback).Run(response && response->available()); + }, + std::move(callback))); } -// static void ChromeOSAuthenticator::IsPowerButtonModeEnabled( base::OnceCallback callback) { chromeos::U2FClient::Get()->IsU2FEnabled( diff --git a/device/fido/cros/authenticator.h b/device/fido/cros/authenticator.h index 6db0561edf7ab0..595f190b82bf03 100644 --- a/device/fido/cros/authenticator.h +++ b/device/fido/cros/authenticator.h @@ -36,14 +36,11 @@ class COMPONENT_EXPORT(DEVICE_FIDO) ChromeOSAuthenticator const CtapGetAssertionRequest& request, base::OnceCallback callback); - // Returns whether the platform authenticator is available, which is true if - // the current user has a PIN set up or biometrics enrolled. - // - // Since this call makes a (quick) dbus call, it is potentially blocking and - // should not run on the main thread/sequence. - // - // TODO(crbug.com/1154063): Refactor IsUVPAA() to be async. - static bool IsUVPlatformAuthenticatorAvailableBlocking(); + // Invokes |callback| with a bool indicating whether the platform + // authenticator is available, which is true if the current user has a PIN set + // up or biometrics enrolled. + static void IsUVPlatformAuthenticatorAvailable( + base::OnceCallback callback); // Invokes |callback| with a bool indicating whether the legacy U2F // authenticator, which uses the power button for user presence checking, is diff --git a/device/fido/cros/discovery.cc b/device/fido/cros/discovery.cc index 360572445eaef4..5b6e457bf615f3 100644 --- a/device/fido/cros/discovery.cc +++ b/device/fido/cros/discovery.cc @@ -5,8 +5,7 @@ #include "device/fido/cros/discovery.h" #include "base/bind.h" -#include "base/task/task_traits.h" -#include "base/task/thread_pool.h" +#include "base/logging.h" #include "chromeos/dbus/u2f/u2f_client.h" namespace device { @@ -57,14 +56,7 @@ void FidoChromeOSDiscovery::OnU2FServiceAvailable(bool u2f_service_available) { return; } - AddAuthenticatorIfIsUVPAA(); -} - -void FidoChromeOSDiscovery::AddAuthenticatorIfIsUVPAA() { - base::ThreadPool::PostTaskAndReplyWithResult( - FROM_HERE, {base::TaskPriority::USER_BLOCKING, base::MayBlock()}, - base::BindOnce( - &ChromeOSAuthenticator::IsUVPlatformAuthenticatorAvailableBlocking), + ChromeOSAuthenticator::IsUVPlatformAuthenticatorAvailable( base::BindOnce(&FidoChromeOSDiscovery::MaybeAddAuthenticator, weak_factory_.GetWeakPtr())); } @@ -82,7 +74,9 @@ void FidoChromeOSDiscovery::MaybeAddAuthenticator(bool is_available) { void FidoChromeOSDiscovery::OnHasLegacyU2fCredential(bool has_credential) { DCHECK(!authenticator_); if (!has_credential) { - AddAuthenticatorIfIsUVPAA(); + ChromeOSAuthenticator::IsUVPlatformAuthenticatorAvailable( + base::BindOnce(&FidoChromeOSDiscovery::MaybeAddAuthenticator, + weak_factory_.GetWeakPtr())); return; } diff --git a/device/fido/cros/discovery.h b/device/fido/cros/discovery.h index 2791cec936e6d8..bd0ddf9685da7c 100644 --- a/device/fido/cros/discovery.h +++ b/device/fido/cros/discovery.h @@ -7,9 +7,12 @@ #include +#include "base/callback.h" #include "base/component_export.h" #include "base/memory/weak_ptr.h" +#include "base/optional.h" #include "device/fido/cros/authenticator.h" +#include "device/fido/ctap_get_assertion_request.h" #include "device/fido/fido_discovery_base.h" namespace device { @@ -29,7 +32,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoChromeOSDiscovery private: void OnU2FServiceAvailable(bool u2f_service_available); - void AddAuthenticatorIfIsUVPAA(); void MaybeAddAuthenticator(bool is_available); void OnHasLegacyU2fCredential(bool has_credential); diff --git a/device/fido/mac/authenticator.h b/device/fido/mac/authenticator.h index fd59dfcc504d91..54e16fe333697b 100644 --- a/device/fido/mac/authenticator.h +++ b/device/fido/mac/authenticator.h @@ -8,6 +8,7 @@ #include #include +#include "base/callback.h" #include "base/component_export.h" #include "base/macros.h" #include "base/memory/weak_ptr.h" @@ -24,20 +25,21 @@ namespace mac { struct AuthenticatorConfig; +// TouchIdAuthenticator is a platform authenticator on macOS. It persists Secure +// Enclave backed credentials along with metadata in the macOS keychain. Each +// Chrome profile maintains its own set of credentials. +// +// Despite the name, any local login factor can be used for User Verification +// (e.g. Touch ID, password, Apple Watch). class COMPONENT_EXPORT(DEVICE_FIDO) TouchIdAuthenticator : public FidoAuthenticator { public: - // 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(const AuthenticatorConfig& config); + // IsAvailable runs |callback| with a bool incidating whether the + // authenticator is available, i.e. whether the device has a Secure Enclave + // and the current binary carries a keychain-access-groups entitlement that + // matches the one set in |config|. + static void IsAvailable(AuthenticatorConfig config, + base::OnceCallback callback); // CreateIfAvailable returns a TouchIdAuthenticator. Callers must check // IsAvailable() first. diff --git a/device/fido/mac/authenticator.mm b/device/fido/mac/authenticator.mm index 3de090c026b2b7..f4bec38c4ff28c 100644 --- a/device/fido/mac/authenticator.mm +++ b/device/fido/mac/authenticator.mm @@ -32,17 +32,19 @@ namespace mac { // static -bool TouchIdAuthenticator::IsAvailable(const AuthenticatorConfig& config) { +void TouchIdAuthenticator::IsAvailable( + AuthenticatorConfig config, + base::OnceCallback callback) { if (__builtin_available(macOS 10.12.2, *)) { - return TouchIdContext::TouchIdAvailable(config); + TouchIdContext::TouchIdAvailable(std::move(config), std::move(callback)); + return; } - return false; + std::move(callback).Run(false); } // static std::unique_ptr TouchIdAuthenticator::Create( AuthenticatorConfig config) { - DCHECK(IsAvailable(config)); return base::WrapUnique( new TouchIdAuthenticator(std::move(config.keychain_access_group), std::move(config.metadata_secret))); diff --git a/device/fido/mac/discovery.cc b/device/fido/mac/discovery.cc index 80e4fce5012bc8..0c7101f4fe307a 100644 --- a/device/fido/mac/discovery.cc +++ b/device/fido/mac/discovery.cc @@ -26,17 +26,14 @@ void FidoTouchIdDiscovery::Start() { return; } - // Start() is currently invoked synchronously in the - // FidoRequestHandler ctor. Invoke AddAuthenticator() asynchronously - // to avoid hairpinning FidoRequestHandler::AuthenticatorAdded() - // before the request handler has an observer. - base::SequencedTaskRunnerHandle::Get()->PostTask( - FROM_HERE, base::BindOnce(&FidoTouchIdDiscovery::AddAuthenticator, - weak_factory_.GetWeakPtr())); + TouchIdAuthenticator::IsAvailable( + authenticator_config_, + base::BindOnce(&FidoTouchIdDiscovery::OnAuthenticatorAvailable, + weak_factory_.GetWeakPtr())); } -void FidoTouchIdDiscovery::AddAuthenticator() { - if (!TouchIdAuthenticator::IsAvailable(authenticator_config_)) { +void FidoTouchIdDiscovery::OnAuthenticatorAvailable(bool is_available) { + if (!is_available) { observer()->DiscoveryStarted(this, /*success=*/false); return; } diff --git a/device/fido/mac/discovery.h b/device/fido/mac/discovery.h index 96a340213b4b1c..8e376bed9e2690 100644 --- a/device/fido/mac/discovery.h +++ b/device/fido/mac/discovery.h @@ -29,7 +29,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoTouchIdDiscovery void Start() override; private: - void AddAuthenticator(); + void OnAuthenticatorAvailable(bool is_available); AuthenticatorConfig authenticator_config_; std::unique_ptr authenticator_; diff --git a/device/fido/mac/scoped_touch_id_test_environment.h b/device/fido/mac/scoped_touch_id_test_environment.h index 61c881ea10500d..6512d5c78b1a3b 100644 --- a/device/fido/mac/scoped_touch_id_test_environment.h +++ b/device/fido/mac/scoped_touch_id_test_environment.h @@ -48,10 +48,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) private: static std::unique_ptr ForwardCreate(); - static bool ForwardTouchIdAvailable(const AuthenticatorConfig& config); + static bool ForwardTouchIdAvailable(AuthenticatorConfig); std::unique_ptr CreateTouchIdContext(); - bool TouchIdAvailable(const AuthenticatorConfig&); + bool TouchIdAvailable(AuthenticatorConfig); using CreateFuncPtr = decltype(&ForwardCreate); CreateFuncPtr touch_id_context_create_ptr_; diff --git a/device/fido/mac/scoped_touch_id_test_environment.mm b/device/fido/mac/scoped_touch_id_test_environment.mm index 6d53fa09f2fd19..33e0ec874bbff5 100644 --- a/device/fido/mac/scoped_touch_id_test_environment.mm +++ b/device/fido/mac/scoped_touch_id_test_environment.mm @@ -54,8 +54,8 @@ static API_AVAILABLE(macosx(10.12.2)) // static bool ScopedTouchIdTestEnvironment::ForwardTouchIdAvailable( - const AuthenticatorConfig& config) { - return g_current_environment->TouchIdAvailable(config); + AuthenticatorConfig config) { + return g_current_environment->TouchIdAvailable(std::move(config)); } bool ScopedTouchIdTestEnvironment::SetTouchIdAvailable(bool available) { @@ -63,7 +63,7 @@ static API_AVAILABLE(macosx(10.12.2)) } bool ScopedTouchIdTestEnvironment::TouchIdAvailable( - const AuthenticatorConfig& config) { + AuthenticatorConfig config) { return touch_id_available_; } diff --git a/device/fido/mac/touch_id_context.h b/device/fido/mac/touch_id_context.h index 9d135e235b3d21..a96a80d0ef7154 100644 --- a/device/fido/mac/touch_id_context.h +++ b/device/fido/mac/touch_id_context.h @@ -46,7 +46,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) // Returns whether the device has a secure enclave and can authenticate the // local user, and whether the current binary carries a // keychain-access-groups entitlement that matches the one set in |config|. - static bool TouchIdAvailable(const AuthenticatorConfig& config); + static void TouchIdAvailable(AuthenticatorConfig config, + base::OnceCallback); virtual ~TouchIdContext(); @@ -69,13 +70,13 @@ class COMPONENT_EXPORT(DEVICE_FIDO) private: using CreateFuncPtr = decltype(&Create); - using TouchIdAvailableFuncPtr = decltype(&TouchIdAvailable); - static CreateFuncPtr g_create_; + + static bool TouchIdAvailableImplBlocking(AuthenticatorConfig config); + using TouchIdAvailableFuncPtr = decltype(&TouchIdAvailableImplBlocking); static TouchIdAvailableFuncPtr g_touch_id_available_; static std::unique_ptr CreateImpl(); - static bool TouchIdAvailableImpl(const AuthenticatorConfig& config); void RunCallback(bool success); diff --git a/device/fido/mac/touch_id_context.mm b/device/fido/mac/touch_id_context.mm index 82ee3860776dd2..26f14382f77a97 100644 --- a/device/fido/mac/touch_id_context.mm +++ b/device/fido/mac/touch_id_context.mm @@ -14,6 +14,9 @@ #include "base/memory/ptr_util.h" #include "base/sequenced_task_runner.h" #include "base/strings/sys_string_conversions.h" +#include "base/task/task_traits.h" +#include "base/task/thread_pool.h" +#include "base/threading/scoped_blocking_call.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" @@ -24,6 +27,7 @@ namespace mac { namespace { + API_AVAILABLE(macosx(10.12.2)) base::ScopedCFTypeRef DefaultAccessControl() { // The default access control policy used for WebAuthn credentials stored by @@ -39,8 +43,12 @@ // 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( +bool BinaryHasKeychainAccessGroupEntitlementBlocking( const std::string& keychain_access_group) { + // This method makes call into the macOS Security Framework, which may block. + base::ScopedBlockingCall scoped_blocking_call(FROM_HERE, + base::BlockingType::MAY_BLOCK); + base::ScopedCFTypeRef code; if (SecCodeCopySelf(kSecCSDefaultFlags, code.InitializeInto()) != errSecSuccess) { @@ -74,10 +82,13 @@ bool BinaryHasKeychainAccessGroupEntitlement( } API_AVAILABLE(macosx(10.12.2)) -bool CanCreateSecureEnclaveKeyPair() { +bool CanCreateSecureEnclaveKeyPairBlocking() { // CryptoKit offers SecureEnclave.isAvailable but does not have Swift // bindings. Instead, attempt to create an ephemeral key pair in the secure // enclave. + base::ScopedBlockingCall scoped_blocking_call(FROM_HERE, + base::BlockingType::MAY_BLOCK); + base::ScopedCFTypeRef params( CFDictionaryCreateMutable(kCFAllocatorDefault, 0, &kCFTypeDictionaryKeyCallBacks, @@ -117,13 +128,18 @@ bool CanCreateSecureEnclaveKeyPair() { } // static -bool TouchIdContext::TouchIdAvailableImpl(const AuthenticatorConfig& config) { +bool TouchIdContext::TouchIdAvailableImplBlocking(AuthenticatorConfig config) { // Ensure that the binary is signed with the keychain-access-group // entitlement that is configured by the embedder; that user authentication // with biometry, watch, or device passcode possible; and that the device has // a secure enclave. + // TODO(martinkr): The calls into the Security Framework in these methods + // have been observed to hang or crash. We have since moved them off the UI + // thread to keep the browser responsive during hangs at least. But we should + // find a way to eliminate them or perhaps cache the result. - if (!BinaryHasKeychainAccessGroupEntitlement(config.keychain_access_group)) { + if (!BinaryHasKeychainAccessGroupEntitlementBlocking( + config.keychain_access_group)) { FIDO_LOG(ERROR) << "Touch ID authenticator unavailable because keychain-access-group " "entitlement is missing or incorrect"; @@ -138,7 +154,7 @@ bool CanCreateSecureEnclaveKeyPair() { return false; } - if (!CanCreateSecureEnclaveKeyPair()) { + if (!CanCreateSecureEnclaveKeyPairBlocking()) { FIDO_LOG(DEBUG) << "No secure enclave"; return false; } @@ -146,14 +162,20 @@ bool CanCreateSecureEnclaveKeyPair() { return true; } -// static +// Testing seam to allow faking Touch ID in tests. TouchIdContext::TouchIdAvailableFuncPtr TouchIdContext::g_touch_id_available_ = - &TouchIdContext::TouchIdAvailableImpl; + &TouchIdContext::TouchIdAvailableImplBlocking; // static -bool TouchIdContext::TouchIdAvailable(const AuthenticatorConfig& config) { - // Testing seam to allow faking Touch ID in tests. - return (*g_touch_id_available_)(config); +void TouchIdContext::TouchIdAvailable( + AuthenticatorConfig config, + base::OnceCallback callback) { + base::ThreadPool::PostTaskAndReplyWithResult( + FROM_HERE, + {base::TaskPriority::USER_VISIBLE, base::MayBlock(), + base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}, + base::BindOnce(g_touch_id_available_, std::move(config)), + std::move(callback)); } TouchIdContext::TouchIdContext() diff --git a/device/fido/win/authenticator.cc b/device/fido/win/authenticator.cc index c6f56843a67336..bc47142633e8f0 100644 --- a/device/fido/win/authenticator.cc +++ b/device/fido/win/authenticator.cc @@ -18,6 +18,7 @@ #include "base/strings/string16.h" #include "base/strings/utf_string_conversions.h" #include "base/task/post_task.h" +#include "base/task/task_traits.h" #include "base/task/thread_pool.h" #include "device/fido/authenticator_supported_options.h" #include "device/fido/ctap_get_assertion_request.h" @@ -30,13 +31,23 @@ namespace device { -// static -bool WinWebAuthnApiAuthenticator::IsUserVerifyingPlatformAuthenticatorAvailable( - WinWebAuthnApi* api) { - BOOL result; - return api && api->IsAvailable() && - api->IsUserVerifyingPlatformAuthenticatorAvailable(&result) == S_OK && - result == TRUE; +void WinWebAuthnApiAuthenticator::IsUserVerifyingPlatformAuthenticatorAvailable( + WinWebAuthnApi* api, + base::OnceCallback callback) { + base::ThreadPool::PostTaskAndReplyWithResult( + FROM_HERE, + {base::TaskPriority::USER_VISIBLE, base::MayBlock(), + base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}, + base::BindOnce( + [](WinWebAuthnApi* api) { + BOOL result; + return api && api->IsAvailable() && + api->IsUserVerifyingPlatformAuthenticatorAvailable( + &result) == S_OK && + result == TRUE; + }, + api), + std::move(callback)); } WinWebAuthnApiAuthenticator::WinWebAuthnApiAuthenticator( diff --git a/device/fido/win/authenticator.h b/device/fido/win/authenticator.h index 93cb2d8ba104d8..b0c2e4df74a5a6 100644 --- a/device/fido/win/authenticator.h +++ b/device/fido/win/authenticator.h @@ -9,6 +9,7 @@ #include #include +#include "base/callback.h" #include "base/component_export.h" #include "base/macros.h" #include "base/memory/weak_ptr.h" @@ -29,10 +30,11 @@ class WinWebAuthnApi; class COMPONENT_EXPORT(DEVICE_FIDO) WinWebAuthnApiAuthenticator : public FidoAuthenticator { public: - // This method is safe to call without checking - // WinWebAuthnApi::IsAvailable(). - static bool IsUserVerifyingPlatformAuthenticatorAvailable( - WinWebAuthnApi* api); + // This method is safe to call without checking WinWebAuthnApi::IsAvailable(). + // Returns false if |api| is nullptr. + static void IsUserVerifyingPlatformAuthenticatorAvailable( + WinWebAuthnApi* api, + base::OnceCallback); // Instantiates an authenticator that uses the default WinWebAuthnApi. // diff --git a/device/fido/win/webauthn_api.cc b/device/fido/win/webauthn_api.cc index fc658b4dfa795c..313174bd00f4e8 100644 --- a/device/fido/win/webauthn_api.cc +++ b/device/fido/win/webauthn_api.cc @@ -8,6 +8,7 @@ #include #include "base/bind.h" +#include "base/feature_list.h" #include "base/logging.h" #include "base/no_destructor.h" #include "base/optional.h" @@ -15,8 +16,10 @@ #include "base/strings/string_piece_forward.h" #include "base/strings/string_util_win.h" #include "base/strings/utf_string_conversions.h" +#include "base/threading/scoped_blocking_call.h" #include "base/threading/scoped_thread_priority.h" #include "components/device_event_log/device_event_log.h" +#include "device/fido/features.h" #include "device/fido/win/logging.h" #include "device/fido/win/type_conversions.h" @@ -36,6 +39,10 @@ constexpr uint32_t kWinWebAuthnTimeoutMilliseconds = 1000 * 60 * 5; class WinWebAuthnApiImpl : public WinWebAuthnApi { public: WinWebAuthnApiImpl() : WinWebAuthnApi(), is_bound_(false) { + if (base::FeatureList::IsEnabled(device::kWebAuthUseNativeWinApi)) { + FIDO_LOG(DEBUG) << "Windows WebAuthn API deactivated via feature flag"; + return; + } { // Mitigate the issues caused by loading DLLs on a background thread // (http://crbug/973868). @@ -44,6 +51,7 @@ class WinWebAuthnApiImpl : public WinWebAuthnApi { LoadLibraryExA("webauthn.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32); } if (!webauthn_dll_) { + FIDO_LOG(ERROR) << "Windows WebAuthn API failed to load"; return; } @@ -99,6 +107,8 @@ class WinWebAuthnApiImpl : public WinWebAuthnApi { // Mitigate the issues caused by loading DLLs on a background thread // (http://crbug/973868). SCOPED_MAY_LOAD_LIBRARY_AT_BACKGROUND_PRIORITY(); + base::ScopedBlockingCall scoped_blocking_call( + FROM_HERE, base::BlockingType::MAY_BLOCK); return is_user_verifying_platform_authenticator_available_(available); } @@ -111,6 +121,8 @@ class WinWebAuthnApiImpl : public WinWebAuthnApi { PCWEBAUTHN_AUTHENTICATOR_MAKE_CREDENTIAL_OPTIONS options, PWEBAUTHN_CREDENTIAL_ATTESTATION* credential_attestation_ptr) override { DCHECK(is_bound_); + base::ScopedBlockingCall scoped_blocking_call( + FROM_HERE, base::BlockingType::MAY_BLOCK); return authenticator_make_credential_( h_wnd, rp, user, cose_credential_parameters, client_data, options, credential_attestation_ptr); @@ -123,6 +135,8 @@ class WinWebAuthnApiImpl : public WinWebAuthnApi { PCWEBAUTHN_AUTHENTICATOR_GET_ASSERTION_OPTIONS options, PWEBAUTHN_ASSERTION* assertion_ptr) override { DCHECK(is_bound_); + base::ScopedBlockingCall scoped_blocking_call( + FROM_HERE, base::BlockingType::MAY_BLOCK); return authenticator_get_assertion_(h_wnd, rp_id, client_data, options, assertion_ptr); }