device/fido: get touch if no credentials are recognised.

Some CTAP2 devices will return immediately if they don't recognise
credentials. This will result in the authenticators not flashing for
a request.

This change triggers a dummy make-credential operation in the event that
a get-assertion operation returns immediately because no credentials were
recognised. This makes CTAP2 behave like U2F.

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

content/browser/webauth/authenticator_impl_unittest.cc

@@ -1281,32 +1281,36 @@ TEST_F(AuthenticatorImplTest, InvalidResponse) {
   }
 }
 
-TEST_F(AuthenticatorImplTest, Ctap2AssertionWithInvalidCredentialError) {
-  device::test::ScopedVirtualFidoDevice scoped_virtual_device;
-  device::VirtualCtap2Device::Config config;
-  config.return_immediate_invalid_credential_error = true;
-  scoped_virtual_device.SetCtap2Config(config);
-
+TEST_F(AuthenticatorImplTest, Ctap2AssertionWithUnknownCredential) {
   TestServiceManagerContext service_manager_context;
   SimulateNavigation(GURL(kTestOrigin1));
-  auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
-      base::Time::Now(), base::TimeTicks::Now());
 
-  bool pressed = false;
-  scoped_virtual_device.mutable_state()->simulate_press_callback =
-      base::BindRepeating([](bool* flag) { *flag = true; }, &pressed);
+  for (bool return_immediate_invalid_credential_error : {false, true}) {
+    SCOPED_TRACE(::testing::Message()
+                 << "return_immediate_invalid_credential_error="
+                 << return_immediate_invalid_credential_error);
 
-  TestGetAssertionCallback callback_receiver;
-  auto authenticator = ConstructAuthenticatorWithTimer(task_runner);
-  authenticator->GetAssertion(GetTestPublicKeyCredentialRequestOptions(),
-                              callback_receiver.callback());
-  base::RunLoop().RunUntilIdle();
-  task_runner->FastForwardBy(base::TimeDelta::FromMinutes(1));
-  callback_receiver.WaitForCallback();
-  // TODO(agl): this shouldn't timeout, rather it should return
-  // CREDENTIAL_NOT_RECOGNIZED after a press.
-  EXPECT_EQ(AuthenticatorStatus::NOT_ALLOWED_ERROR, callback_receiver.status());
-  EXPECT_FALSE(pressed);
+    device::test::ScopedVirtualFidoDevice scoped_virtual_device;
+    device::VirtualCtap2Device::Config config;
+    config.return_immediate_invalid_credential_error =
+        return_immediate_invalid_credential_error;
+    scoped_virtual_device.SetCtap2Config(config);
+
+    bool pressed = false;
+    scoped_virtual_device.mutable_state()->simulate_press_callback =
+        base::BindRepeating([](bool* flag) { *flag = true; }, &pressed);
+
+    TestGetAssertionCallback callback_receiver;
+    AuthenticatorPtr authenticator = ConnectToAuthenticator();
+    authenticator->GetAssertion(GetTestPublicKeyCredentialRequestOptions(),
+                                callback_receiver.callback());
+    callback_receiver.WaitForCallback();
+    EXPECT_EQ(AuthenticatorStatus::CREDENTIAL_NOT_RECOGNIZED,
+              callback_receiver.status());
+    // The user must have pressed the authenticator for the operation to
+    // resolve.
+    EXPECT_TRUE(pressed);
+  }
 }
 
 enum class IndividualAttestation {

device/fido/fido_device_authenticator.cc

@@ -18,7 +18,6 @@
 #include "device/fido/get_assertion_task.h"
 #include "device/fido/make_credential_task.h"
 #include "device/fido/pin.h"
-#include "device/fido/u2f_command_constructor.h"
 
 namespace device {
 
@@ -73,29 +72,8 @@ void FidoDeviceAuthenticator::GetAssertion(CtapGetAssertionRequest request,
 }
 
 void FidoDeviceAuthenticator::GetTouch(base::OnceCallback<void()> callback) {
-  // We want to flash and wait for a touch. Newer versions of the CTAP2 spec
-  // include a provision for blocking for a touch when an empty pinAuth is
-  // specified, but devices exist that predate this part of the spec and also
-  // the spec says that devices need only do that if they implement PIN support.
-  // Therefore, in order to portably wait for a touch, a dummy credential is
-  // created. This does assume that the device supports ECDSA P-256, however.
-  PublicKeyCredentialUserEntity user({1} /* user ID */);
-  // The user name is incorrectly marked as optional in the CTAP2 spec.
-  user.SetUserName("dummy");
-  CtapMakeCredentialRequest req(
-      "" /* client_data_json */, PublicKeyCredentialRpEntity(".dummy"),
-      std::move(user),
-      PublicKeyCredentialParams(
-          {{CredentialType::kPublicKey,
-            base::strict_cast<int>(CoseAlgorithmIdentifier::kCoseEs256)}}));
-  req.SetExcludeList({});
-  // Set an empty pinAuth in case the device is a newer CTAP2 and understands
-  // this to mean to block for touch.
-  req.SetPinAuth({});
-  DCHECK(IsConvertibleToU2fRegisterCommand(req));
-
   MakeCredential(
-      std::move(req),
+      MakeCredentialTask::GetTouchRequest(device()),
       base::BindOnce(
           [](base::OnceCallback<void()> callback, CtapDeviceResponseCode status,
              base::Optional<AuthenticatorMakeCredentialResponse>) {

device/fido/get_assertion_task.cc

@@ -10,6 +10,7 @@
 #include "device/base/features.h"
 #include "device/fido/authenticator_get_assertion_response.h"
 #include "device/fido/ctap2_device_operation.h"
+#include "device/fido/make_credential_task.h"
 #include "device/fido/u2f_sign_operation.h"
 
 namespace device {
@@ -81,7 +82,9 @@ void GetAssertionTask::GetAssertion() {
   sign_operation_ =
       std::make_unique<Ctap2DeviceOperation<CtapGetAssertionRequest,
                                             AuthenticatorGetAssertionResponse>>(
-          device(), request_, std::move(callback_),
+          device(), request_,
+          base::BindOnce(&GetAssertionTask::HandleResponse,
+                         weak_factory_.GetWeakPtr()),
           base::BindOnce(&ReadCTAPGetAssertionResponse));
   sign_operation_->Start();
 }
@@ -94,6 +97,28 @@ void GetAssertionTask::U2fSign() {
   sign_operation_->Start();
 }
 
+void GetAssertionTask::HandleResponse(
+    CtapDeviceResponseCode response_code,
+    base::Optional<AuthenticatorGetAssertionResponse> response_data) {
+  // Some authenticators will return this error before waiting for a touch if
+  // they don't recognise a credential. In other cases the result can be
+  // returned immediately.
+  if (response_code != CtapDeviceResponseCode::kCtap2ErrInvalidCredential) {
+    std::move(callback_).Run(response_code, std::move(response_data));
+    return;
+  }
+
+  // The request failed in a way that didn't request a touch. Simulate it.
+  dummy_register_operation_ = std::make_unique<Ctap2DeviceOperation<
+      CtapMakeCredentialRequest, AuthenticatorMakeCredentialResponse>>(
+      device(), MakeCredentialTask::GetTouchRequest(device()),
+      base::BindOnce(&GetAssertionTask::HandleDummyMakeCredentialComplete,
+                     weak_factory_.GetWeakPtr()),
+      base::BindOnce(&ReadCTAPMakeCredentialResponse,
+                     device()->DeviceTransport()));
+  dummy_register_operation_->Start();
+}
+
 void GetAssertionTask::HandleResponseToSilentRequest(
     UserVerificationRequirement original_uv_configuration,
     CtapDeviceResponseCode response_code,
@@ -123,9 +148,18 @@ void GetAssertionTask::HandleResponseToSilentRequest(
   sign_operation_ =
       std::make_unique<Ctap2DeviceOperation<CtapGetAssertionRequest,
                                             AuthenticatorGetAssertionResponse>>(
-          device(), request_, std::move(callback_),
+          device(), request_,
+          base::BindOnce(&GetAssertionTask::HandleResponse,
+                         weak_factory_.GetWeakPtr()),
           base::BindOnce(&ReadCTAPGetAssertionResponse));
   sign_operation_->Start();
 }
 
+void GetAssertionTask::HandleDummyMakeCredentialComplete(
+    CtapDeviceResponseCode response_code,
+    base::Optional<AuthenticatorMakeCredentialResponse> response_data) {
+  std::move(callback_).Run(CtapDeviceResponseCode::kCtap2ErrNoCredentials,
+                           base::nullopt);
+}
+
 }  // namespace device

device/fido/get_assertion_task.h

@@ -16,6 +16,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "device/fido/ctap_get_assertion_request.h"
+#include "device/fido/ctap_make_credential_request.h"
 #include "device/fido/device_operation.h"
 #include "device/fido/fido_constants.h"
 #include "device/fido/fido_task.h"
@@ -23,6 +24,7 @@
 namespace device {
 
 class AuthenticatorGetAssertionResponse;
+class AuthenticatorMakeCredentialResponse;
 
 // Represents per device sign operation on CTAP1/CTAP2 devices.
 // https://fidoalliance.org/specs/fido-v2.0-rd-20161004/fido-client-to-authenticator-protocol-v2.0-rd-20161004.html#authenticatorgetassertion
@@ -33,6 +35,9 @@ class COMPONENT_EXPORT(DEVICE_FIDO) GetAssertionTask : public FidoTask {
       base::Optional<AuthenticatorGetAssertionResponse>)>;
   using SignOperation = DeviceOperation<CtapGetAssertionRequest,
                                         AuthenticatorGetAssertionResponse>;
+  using RegisterOperation =
+      DeviceOperation<CtapMakeCredentialRequest,
+                      AuthenticatorMakeCredentialResponse>;
 
   GetAssertionTask(FidoDevice* device,
                    CtapGetAssertionRequest request,
@@ -46,6 +51,12 @@ class COMPONENT_EXPORT(DEVICE_FIDO) GetAssertionTask : public FidoTask {
   void GetAssertion();
   void U2fSign();
 
+  // HandleResponse is the callback to a CTAP2 assertion request that requested
+  // user-presence.
+  void HandleResponse(
+      CtapDeviceResponseCode response_code,
+      base::Optional<AuthenticatorGetAssertionResponse> response_data);
+
   // HandleResponseToSilentRequest is a callback to a request without user-
   // presence requested used when this assertion request may require falling
   // back to U2F.
@@ -54,8 +65,15 @@ class COMPONENT_EXPORT(DEVICE_FIDO) GetAssertionTask : public FidoTask {
       CtapDeviceResponseCode response_code,
       base::Optional<AuthenticatorGetAssertionResponse> response_data);
 
+  // HandleDummyMakeCredentialComplete is the callback for the dummy credential
+  // creation request that will be triggered, if needed, to get a touch.
+  void HandleDummyMakeCredentialComplete(
+      CtapDeviceResponseCode response_code,
+      base::Optional<AuthenticatorMakeCredentialResponse> response_data);
+
   CtapGetAssertionRequest request_;
   std::unique_ptr<SignOperation> sign_operation_;
+  std::unique_ptr<RegisterOperation> dummy_register_operation_;
   GetAssertionTaskCallback callback_;
   base::WeakPtrFactory<GetAssertionTask> weak_factory_;
 

device/fido/make_credential_task.cc

@@ -58,6 +58,44 @@ MakeCredentialTask::MakeCredentialTask(
 
 MakeCredentialTask::~MakeCredentialTask() = default;
 
+// static
+CtapMakeCredentialRequest MakeCredentialTask::GetTouchRequest(
+    const FidoDevice* device) {
+  // We want to flash and wait for a touch. Newer versions of the CTAP2 spec
+  // include a provision for blocking for a touch when an empty pinAuth is
+  // specified, but devices exist that predate this part of the spec and also
+  // the spec says that devices need only do that if they implement PIN support.
+  // Therefore, in order to portably wait for a touch, a dummy credential is
+  // created. This does assume that the device supports ECDSA P-256, however.
+  PublicKeyCredentialUserEntity user({1} /* user ID */);
+  // The user name is incorrectly marked as optional in the CTAP2 spec.
+  user.SetUserName("dummy");
+  CtapMakeCredentialRequest req(
+      "" /* client_data_json */, PublicKeyCredentialRpEntity(".dummy"),
+      std::move(user),
+      PublicKeyCredentialParams(
+          {{CredentialType::kPublicKey,
+            base::strict_cast<int>(CoseAlgorithmIdentifier::kCoseEs256)}}));
+  req.SetExcludeList({});
+
+  // If a device supports CTAP2 and has PIN support then setting an empty
+  // pinAuth should trigger just a touch[1]. Our U2F code also understands
+  // this convention.
+  // [1]
+  // https://fidoalliance.org/specs/fido-v2.0-ps-20190130/fido-client-to-authenticator-protocol-v2.0-ps-20190130.html#using-pinToken-in-authenticatorGetAssertion
+  if (device->supported_protocol() == ProtocolVersion::kU2f ||
+      (device->device_info() &&
+       device->device_info()->options().client_pin_availability !=
+           AuthenticatorSupportedOptions::ClientPinAvailability::
+               kNotSupported)) {
+    req.SetPinAuth({});
+  }
+
+  DCHECK(IsConvertibleToU2fRegisterCommand(req));
+
+  return req;
+}
+
 void MakeCredentialTask::StartTask() {
   if (device()->supported_protocol() == ProtocolVersion::kCtap &&
       !request_parameter_.is_u2f_only() &&

device/fido/make_credential_task.h

@@ -39,6 +39,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) MakeCredentialTask : public FidoTask {
                      MakeCredentialTaskCallback callback);
   ~MakeCredentialTask() override;
 
+  // GetTouchRequest returns a request that will cause a device to flash and
+  // wait for a touch.
+  static CtapMakeCredentialRequest GetTouchRequest(const FidoDevice* device);
+
  private:
   // FidoTask:
   void StartTask() final;