fido: do not use CTAP2 in cryptotoken GetAssertion requests

WebAuthn GetAssertion requests originating from the cryptotoken
extension, which implements Chrome's U2F API, currently may result in a
CTAP2 request if the selected authenticator supports CTAP2.

This hasn't really caused issues in the past: Lenient authenticators
accept an appId as the makeCredential request's rp_id parameter and let
us challenge the U2F credential that way. Or if the authenticator
indicates over CTAP2 that the credential doesn't exist, GetAssertionTask
would then automatically retry over the U2F interface based on the
presence of the appId extension. Responses for both cases are
equivalent.

But sending CTAP2 GetAssertion requests in order to respond to a request
to the U2F API is unexpected and inefficient. It may also cause issues
with future authenticators that decide to treat user verification as
non-optional for all requests arriving on the CTAP2 interface.

Instead, change GetAssertionTask to never try its CTAP2 path for these
requests.

Bug: 1099782
Change-Id: Ice54122bf3b9f63814d594074a39b9b46279ded4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2298541
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#788798}

content/browser/webauth/authenticator_impl_unittest.cc

@@ -963,38 +963,148 @@ TEST_F(AuthenticatorImplTest, CryptotokenBypass) {
   }
 }
 
-// Requests originating from cryptotoken should only target U2F devices.
-TEST_F(AuthenticatorImplTest, CryptoTokenU2fOnly) {
-  SimulateNavigation(GURL(kTestOrigin1));
+// MakeCredential requests from cryptotoken to a U2F authenticator should
+// succeed.
+TEST_F(AuthenticatorImplTest, CryptoTokenMakeCredentialU2fDevice) {
+  virtual_device_factory_->SetSupportedProtocol(device::ProtocolVersion::kU2f);
+
+  SimulateNavigation(GURL(kCryptotokenOrigin));
+  auto authenticator = ConnectToAuthenticator();
+  PublicKeyCredentialCreationOptionsPtr options =
+      GetTestPublicKeyCredentialCreationOptions();
+  options->relying_party.id = kTestOrigin1;
+  TestMakeCredentialCallback callback_receiver;
+  authenticator->MakeCredential(std::move(options),
+                                callback_receiver.callback());
+
+  callback_receiver.WaitForCallback();
+
+  EXPECT_EQ(AuthenticatorStatus::SUCCESS, callback_receiver.status());
+}
+
+// MakeCredential requests from cryptotoken to an authentictor that does not
+// support U2F should fail.
+TEST_F(AuthenticatorImplTest, CryptoTokenMakeCredentialCtap2Device) {
+  virtual_device_factory_->SetSupportedProtocol(
+      device::ProtocolVersion::kCtap2);
+
+  SimulateNavigation(GURL(kCryptotokenOrigin));
   auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
       base::Time::Now(), base::TimeTicks::Now());
   auto authenticator = ConstructAuthenticatorWithTimer(task_runner);
+  PublicKeyCredentialCreationOptionsPtr options =
+      GetTestPublicKeyCredentialCreationOptions();
+  options->relying_party.id = kTestOrigin1;
+  TestMakeCredentialCallback callback_receiver;
+  authenticator->MakeCredential(std::move(options),
+                                callback_receiver.callback());
 
-  // TODO(martinkr): VirtualFidoDeviceFactory does not offer devices that
-  // support both U2F and CTAP yet; we should test those.
-  for (const bool u2f_authenticator : {true, false}) {
-    SCOPED_TRACE(u2f_authenticator ? "U2F" : "CTAP");
-    OverrideLastCommittedOrigin(main_rfh(),
-                                url::Origin::Create(GURL(kCryptotokenOrigin)));
+  base::RunLoop().RunUntilIdle();
+  task_runner->FastForwardBy(base::TimeDelta::FromMinutes(1));
+  callback_receiver.WaitForCallback();
 
-    virtual_device_factory_->SetSupportedProtocol(
-        u2f_authenticator ? device::ProtocolVersion::kU2f
-                          : device::ProtocolVersion::kCtap2);
+  EXPECT_EQ(AuthenticatorStatus::NOT_ALLOWED_ERROR, callback_receiver.status());
+}
 
-    PublicKeyCredentialCreationOptionsPtr options =
-        GetTestPublicKeyCredentialCreationOptions();
-    TestMakeCredentialCallback callback_receiver;
-    authenticator->MakeCredential(std::move(options),
-                                  callback_receiver.callback());
+// GetAssertion requests from cryptotoken to a U2F/CTAP authenticator should
+// use the U2F interface.
+TEST_F(AuthenticatorImplTest, CryptoTokenMakeCredentialDualProtocolDevice) {
+  virtual_device_factory_->SetSupportedProtocol(
+      device::ProtocolVersion::kCtap2);
+  device::VirtualCtap2Device::Config config;
+  config.u2f_support = true;
+  virtual_device_factory_->SetCtap2Config(config);
 
-    base::RunLoop().RunUntilIdle();
-    task_runner->FastForwardBy(base::TimeDelta::FromMinutes(1));
-    callback_receiver.WaitForCallback();
+  SimulateNavigation(GURL(kCryptotokenOrigin));
+  auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
+      base::Time::Now(), base::TimeTicks::Now());
+  auto authenticator = ConstructAuthenticatorWithTimer(task_runner);
+  PublicKeyCredentialCreationOptionsPtr options =
+      GetTestPublicKeyCredentialCreationOptions();
+  options->relying_party.id = kTestOrigin1;
+  TestMakeCredentialCallback callback_receiver;
+  authenticator->MakeCredential(std::move(options),
+                                callback_receiver.callback());
 
-    EXPECT_EQ((u2f_authenticator ? AuthenticatorStatus::SUCCESS
-                                 : AuthenticatorStatus::NOT_ALLOWED_ERROR),
-              callback_receiver.status());
-  }
+  callback_receiver.WaitForCallback();
+
+  EXPECT_EQ(AuthenticatorStatus::SUCCESS, callback_receiver.status());
+  ASSERT_EQ(virtual_device_factory_->mutable_state()->registrations.size(), 1u);
+  EXPECT_TRUE(virtual_device_factory_->mutable_state()
+                  ->registrations.begin()
+                  ->second.is_u2f);
+}
+
+// GetAssertion requests from cryptotoken to a U2F authenticator should
+// succeed.
+TEST_F(AuthenticatorImplTest, CryptoTokenGetAssertionU2fDevice) {
+  SimulateNavigation(GURL(kCryptotokenOrigin));
+  virtual_device_factory_->SetSupportedProtocol(device::ProtocolVersion::kU2f);
+
+  PublicKeyCredentialRequestOptionsPtr options =
+      GetTestPublicKeyCredentialRequestOptions();
+  ASSERT_TRUE(virtual_device_factory_->mutable_state()->InjectRegistration(
+      options->allow_credentials[0].id(), kTestOrigin1));
+  options->appid = kTestOrigin1;
+
+  TestGetAssertionCallback callback_receiver;
+  auto authenticator = ConnectToAuthenticator();
+  authenticator->GetAssertion(std::move(options), callback_receiver.callback());
+
+  callback_receiver.WaitForCallback();
+
+  EXPECT_EQ(AuthenticatorStatus::SUCCESS, callback_receiver.status());
+}
+
+// GetAssertion requests from cryptotoken to an authentictor that does not
+// support U2F should fail.
+TEST_F(AuthenticatorImplTest, CryptoTokenGetAssertionCtap2Device) {
+  SimulateNavigation(GURL(kCryptotokenOrigin));
+  virtual_device_factory_->SetSupportedProtocol(
+      device::ProtocolVersion::kCtap2);
+
+  PublicKeyCredentialRequestOptionsPtr options =
+      GetTestPublicKeyCredentialRequestOptions();
+  ASSERT_TRUE(virtual_device_factory_->mutable_state()->InjectRegistration(
+      options->allow_credentials[0].id(), kTestOrigin1));
+  options->appid = kTestOrigin1;
+
+  TestGetAssertionCallback callback_receiver;
+  auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
+      base::Time::Now(), base::TimeTicks::Now());
+  auto authenticator = ConstructAuthenticatorWithTimer(task_runner);
+  authenticator->GetAssertion(std::move(options), callback_receiver.callback());
+
+  base::RunLoop().RunUntilIdle();
+  task_runner->FastForwardBy(base::TimeDelta::FromMinutes(1));
+  callback_receiver.WaitForCallback();
+
+  EXPECT_EQ(AuthenticatorStatus::NOT_ALLOWED_ERROR, callback_receiver.status());
+}
+
+// GetAssertion requests from cryptotoken should challenge credential on a
+// U2F/CTAP authenticator via the U2F interface.
+TEST_F(AuthenticatorImplTest, CryptoTokenGetAssertionDualProtocolDevice) {
+  SimulateNavigation(GURL(kCryptotokenOrigin));
+  virtual_device_factory_->SetSupportedProtocol(
+      device::ProtocolVersion::kCtap2);
+  device::VirtualCtap2Device::Config config;
+  config.u2f_support = true;
+  config.ignore_u2f_credentials = true;
+  virtual_device_factory_->SetCtap2Config(config);
+
+  PublicKeyCredentialRequestOptionsPtr options =
+      GetTestPublicKeyCredentialRequestOptions();
+  ASSERT_TRUE(virtual_device_factory_->mutable_state()->InjectRegistration(
+      options->allow_credentials[0].id(), kTestOrigin1));
+  options->appid = kTestOrigin1;
+
+  TestGetAssertionCallback callback_receiver;
+  auto authenticator = ConnectToAuthenticator();
+  authenticator->GetAssertion(std::move(options), callback_receiver.callback());
+  callback_receiver.WaitForCallback();
+
+  EXPECT_EQ(AuthenticatorStatus::SUCCESS, callback_receiver.status());
 }
 
 // Test that Cryptotoken requests should only be dispatched to USB
@@ -1046,40 +1156,6 @@ TEST_F(AuthenticatorImplTest, CryptotokenUsbOnly) {
   }
 }
 
-// Requests originating from cryptotoken should only target U2F devices.
-TEST_F(AuthenticatorImplTest, AttestationPermitted) {
-  SimulateNavigation(GURL(kTestOrigin1));
-  auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
-      base::Time::Now(), base::TimeTicks::Now());
-  auto authenticator = ConstructAuthenticatorWithTimer(task_runner);
-
-  // TODO(martinkr): VirtualFidoDeviceFactory does not offer devices that
-  // support both U2F and CTAP yet; we should test those.
-  for (const bool u2f_authenticator : {true, false}) {
-    SCOPED_TRACE(u2f_authenticator ? "U2F" : "CTAP");
-    OverrideLastCommittedOrigin(main_rfh(),
-                                url::Origin::Create(GURL(kCryptotokenOrigin)));
-
-    virtual_device_factory_->SetSupportedProtocol(
-        u2f_authenticator ? device::ProtocolVersion::kU2f
-                          : device::ProtocolVersion::kCtap2);
-
-    PublicKeyCredentialCreationOptionsPtr options =
-        GetTestPublicKeyCredentialCreationOptions();
-    TestMakeCredentialCallback callback_receiver;
-    authenticator->MakeCredential(std::move(options),
-                                  callback_receiver.callback());
-
-    base::RunLoop().RunUntilIdle();
-    task_runner->FastForwardBy(base::TimeDelta::FromMinutes(1));
-    callback_receiver.WaitForCallback();
-
-    EXPECT_EQ((u2f_authenticator ? AuthenticatorStatus::SUCCESS
-                                 : AuthenticatorStatus::NOT_ALLOWED_ERROR),
-              callback_receiver.status());
-  }
-}
-
 // Verify that a credential registered with U2F can be used via webauthn.
 TEST_F(AuthenticatorImplTest, AppIdExtension) {
   SimulateNavigation(GURL(kTestOrigin1));

device/fido/get_assertion_task.cc

@@ -105,9 +105,16 @@ bool GetAssertionTask::StringFixupPredicate(
 }
 
 void GetAssertionTask::StartTask() {
-  if (device()->supported_protocol() == ProtocolVersion::kCtap2) {
+  if (device()->supported_protocol() == ProtocolVersion::kCtap2 &&
+      !request_.is_u2f_only) {
     GetAssertion();
   } else {
+    // |device_info| should be present iff the device is CTAP2.
+    // |MaybeRevertU2fFallbackAndInvokeCallback| uses this to restore the
+    // protocol of CTAP2 devices once this task is complete.
+    DCHECK_EQ(device()->supported_protocol() == ProtocolVersion::kCtap2,
+              device()->device_info().has_value());
+    device()->set_supported_protocol(ProtocolVersion::kU2f);
     U2fSign();
   }
 }
@@ -188,8 +195,10 @@ void GetAssertionTask::GetAssertion() {
 void GetAssertionTask::U2fSign() {
   DCHECK_EQ(ProtocolVersion::kU2f, device()->supported_protocol());
 
-  sign_operation_ = std::make_unique<U2fSignOperation>(device(), request_,
-                                                       std::move(callback_));
+  sign_operation_ = std::make_unique<U2fSignOperation>(
+      device(), request_,
+      base::BindOnce(&GetAssertionTask::MaybeRevertU2fFallbackAndInvokeCallback,
+                     weak_factory_.GetWeakPtr()));
   sign_operation_->Start();
 }
 
@@ -300,4 +309,16 @@ void GetAssertionTask::HandleDummyMakeCredentialComplete(
                            base::nullopt);
 }
 
+void GetAssertionTask::MaybeRevertU2fFallbackAndInvokeCallback(
+    CtapDeviceResponseCode status,
+    base::Optional<AuthenticatorGetAssertionResponse> response) {
+  DCHECK_EQ(ProtocolVersion::kU2f, device()->supported_protocol());
+  if (device()->device_info()) {
+    // This was actually a CTAP2 device, but the protocol version was set to U2F
+    // in order to execute a sign command.
+    device()->set_supported_protocol(ProtocolVersion::kCtap2);
+  }
+  std::move(callback_).Run(status, std::move(response));
+}
+
 }  // namespace device

device/fido/get_assertion_task.h

@@ -84,6 +84,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) GetAssertionTask : public FidoTask {
       CtapDeviceResponseCode response_code,
       base::Optional<AuthenticatorMakeCredentialResponse> response_data);
 
+  void MaybeRevertU2fFallbackAndInvokeCallback(
+      CtapDeviceResponseCode status,
+      base::Optional<AuthenticatorGetAssertionResponse> response);
+
   CtapGetAssertionRequest request_;
   std::vector<std::vector<PublicKeyCredentialDescriptor>> allow_list_batches_;
   size_t current_allow_list_batch_ = 0;

device/fido/virtual_ctap2_device.cc

@@ -1098,28 +1098,30 @@ base::Optional<CtapDeviceResponseCode> VirtualCtap2Device::OnGetAssertion(
     return CtapDeviceResponseCode::kCtap2ErrLimitExceeded;
   }
 
-  // An empty allow_list could be considered to be a resident-key request, but
-  // some authenticators in practice don't take it that way. Thus this code
-  // mirrors that to better reflect reality. CTAP 2.0 leaves it as undefined
-  // behaviour.
   for (const auto& allowed_credential : request.allow_list) {
     if (0 < config_.max_credential_id_length &&
         config_.max_credential_id_length < allowed_credential.id().size()) {
       return CtapDeviceResponseCode::kCtap2ErrLimitExceeded;
     }
-    RegistrationData* found =
+    RegistrationData* registration =
         FindRegistrationData(allowed_credential.id(), rp_id_hash);
-    if (found) {
-      found_registrations.emplace_back(allowed_credential.id(), found);
+    if (registration &&
+        !(registration->is_u2f && config_.ignore_u2f_credentials)) {
+      found_registrations.emplace_back(allowed_credential.id(), registration);
       break;
     }
   }
-  const auto allow_list_it = request_map.find(cbor::Value(3));
-  if (allow_list_it == request_map.end()) {
+
+  // CTAP 2.1 prohibits an empty (but present) allow_list. In CTAP 2.0, it is
+  // technically permissible to send an empty allow_list when asking for
+  // discoverable credentials, but some authenticators in practice don't take it
+  // that way. Thus this code mirrors that to better reflect reality.
+  if (request_map.find(cbor::Value(3)) == request_map.end()) {
     DCHECK(config_.resident_key_support);
     for (auto& registration : mutable_state()->registrations) {
       if (registration.second.is_resident &&
           registration.second.application_parameter == rp_id_hash) {
+        DCHECK(!registration.second.is_u2f);
         found_registrations.emplace_back(registration.first,
                                          &registration.second);
       }

device/fido/virtual_ctap2_device.h

@@ -154,6 +154,11 @@ class COMPONENT_EXPORT(DEVICE_FIDO) VirtualCtap2Device
     // |CoseAlgorithmIdentifier::kInvalidForTesting| public-key algorithm to be
     // advertised and supported to aid testing of unknown public-key types.
     bool support_invalid_for_testing_algorithm = false;
+
+    // ignore_u2f_credentials causes credentials created over the
+    // authenticator's U2F interface not to be available over CTAP2 for
+    // assertions.
+    bool ignore_u2f_credentials = false;
   };
 
   VirtualCtap2Device();