fido: make FidoAuthenticator::Options optional.

This changes the return type of FidoAuthenticator::Options() from
AuthenticatorSupportedOptions to
base::Optional<AuthenticatorSupportedOptions>.

The FidoAuthenticator subclass for the Windows WebAuthn API can
potentially talk to a number of different authenticators with
different capabilities and hence cannot return a sensible value for
this method. It previously returned a "maximum possible" Options
value; but that lead to a bug where GetAssertionHandler assumed it
could set the user verification requirement of a request from
"preferred" to "required" because the Windows authenticator reported
itself as having a PIN set up (while the actual physical device might
not even support PINs).

By returning an Optional, we make the Windows case more explicit.

The aforementioned bug is fixed by moving the "effective uv
requirement" computation for GetAssertion into
FidoDeviceAuthenticator.

For MakeCredential, we have a similar bug: Because
CtapMakeCredentialRequest only stores the UV requirement as a bool
rather than the full enum, we currently default to uv=false for
authenticators that *do* support UV (even in the non-Windows case),
which is wrong. I'm going to address this in an immediate follow up CL.

Bug: 898718
Change-Id: I3e40296578929c75585e820aa5764bd623e1fe79
Reviewed-on: https://chromium-review.googlesource.com/c/1321852
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Kim Paulhamus <kpaulhamus@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606973}

content/browser/webauth/authenticator_impl.cc

@@ -224,6 +224,19 @@ device::CtapMakeCredentialRequest CreateCtapMakeCredentialRequest(
   make_credential_param.SetExcludeList(std::move(exclude_list));
   make_credential_param.SetIsIndividualAttestation(is_individual_attestation);
   make_credential_param.SetHmacSecret(options->hmac_create_secret);
+  // TODO(martinkr): Rename to (Set)ResidentKeyRequired.
+  make_credential_param.SetResidentKeySupported(
+      options->authenticator_selection &&
+      options->authenticator_selection->require_resident_key);
+  // TODO(martinkr): Change CtapMakeCredentialRequest to store the
+  // enum-valued UV requirement; then compute the "effective" requirement for
+  // CTAP devices in FidoDeviceAuthenticator, as it is already done for
+  // GetAssertion. As it is, this will default to uv=false for kPreferred,
+  // which is wrong if the authenticator actually does support UV.
+  make_credential_param.SetUserVerificationRequired(
+      options->authenticator_selection &&
+      options->authenticator_selection->user_verification ==
+          blink::mojom::UserVerificationRequirement::REQUIRED);
   return make_credential_param;
 }
 

device/fido/authenticator_supported_options.cc

@@ -11,13 +11,14 @@
 namespace device {
 
 AuthenticatorSupportedOptions::AuthenticatorSupportedOptions() = default;
-
+AuthenticatorSupportedOptions::AuthenticatorSupportedOptions(
+    const AuthenticatorSupportedOptions& other) = default;
 AuthenticatorSupportedOptions::AuthenticatorSupportedOptions(
     AuthenticatorSupportedOptions&& other) = default;
-
+AuthenticatorSupportedOptions& AuthenticatorSupportedOptions::operator=(
+    const AuthenticatorSupportedOptions& other) = default;
 AuthenticatorSupportedOptions& AuthenticatorSupportedOptions::operator=(
     AuthenticatorSupportedOptions&& other) = default;
-
 AuthenticatorSupportedOptions::~AuthenticatorSupportedOptions() = default;
 
 AuthenticatorSupportedOptions&

device/fido/authenticator_supported_options.h

@@ -33,7 +33,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) AuthenticatorSupportedOptions {
   };
 
   AuthenticatorSupportedOptions();
+  AuthenticatorSupportedOptions(const AuthenticatorSupportedOptions& other);
   AuthenticatorSupportedOptions(AuthenticatorSupportedOptions&& other);
+  AuthenticatorSupportedOptions& operator=(
+      const AuthenticatorSupportedOptions& other);
   AuthenticatorSupportedOptions& operator=(
       AuthenticatorSupportedOptions&& other);
   ~AuthenticatorSupportedOptions();
@@ -74,8 +77,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) AuthenticatorSupportedOptions {
   // optional if client pin capability is not supported by the authenticator.
   ClientPinAvailability client_pin_availability_ =
       ClientPinAvailability::kNotSupported;
-
-  DISALLOW_COPY_AND_ASSIGN(AuthenticatorSupportedOptions);
 };
 
 COMPONENT_EXPORT(DEVICE_FIDO)

device/fido/fido_authenticator.h

@@ -15,11 +15,11 @@
 #include "base/strings/string16.h"
 #include "device/fido/authenticator_get_assertion_response.h"
 #include "device/fido/authenticator_make_credential_response.h"
+#include "device/fido/authenticator_supported_options.h"
 #include "device/fido/fido_transport_protocol.h"
 
 namespace device {
 
-class AuthenticatorSupportedOptions;
 class CtapGetAssertionRequest;
 class CtapMakeCredentialRequest;
 
@@ -42,15 +42,15 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoAuthenticator {
   // call is received, |callback| is invoked. Below MakeCredential() and
   // GetAssertion() must only called after |callback| is invoked.
   virtual void InitializeAuthenticator(base::OnceClosure callback) = 0;
-  virtual void MakeCredential(
-      CtapMakeCredentialRequest request,
-      MakeCredentialCallback callback) = 0;
+  virtual void MakeCredential(CtapMakeCredentialRequest request,
+                              MakeCredentialCallback callback) = 0;
   virtual void GetAssertion(CtapGetAssertionRequest request,
                             GetAssertionCallback callback) = 0;
   virtual void Cancel() = 0;
   virtual std::string GetId() const = 0;
   virtual base::string16 GetDisplayName() const = 0;
-  virtual const AuthenticatorSupportedOptions& Options() const = 0;
+  virtual const base::Optional<AuthenticatorSupportedOptions>& Options()
+      const = 0;
   virtual FidoTransportProtocol AuthenticatorTransport() const = 0;
   virtual bool IsInPairingMode() const = 0;
   virtual base::WeakPtr<FidoAuthenticator> GetWeakPtr() = 0;

device/fido/fido_device_authenticator.cc

@@ -27,8 +27,29 @@ void FidoDeviceAuthenticator::InitializeAuthenticator(
     base::OnceClosure callback) {
   base::SequencedTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
-      base::BindOnce(&FidoDevice::DiscoverSupportedProtocolAndDeviceInfo,
-                     device()->GetWeakPtr(), std::move(callback)));
+      base::BindOnce(
+          &FidoDevice::DiscoverSupportedProtocolAndDeviceInfo,
+          device()->GetWeakPtr(),
+          base::BindOnce(&FidoDeviceAuthenticator::InitializeAuthenticatorDone,
+                         weak_factory_.GetWeakPtr(), std::move(callback))));
+}
+
+void FidoDeviceAuthenticator::InitializeAuthenticatorDone(
+    base::OnceClosure callback) {
+  DCHECK(!options_);
+  switch (device_->supported_protocol()) {
+    case ProtocolVersion::kU2f:
+      options_ = AuthenticatorSupportedOptions();
+      break;
+    case ProtocolVersion::kCtap:
+      DCHECK(device_->device_info()) << "uninitialized device";
+      options_ = device_->device_info()->options();
+      break;
+    case ProtocolVersion::kUnknown:
+      NOTREACHED() << "uninitialized device";
+      options_ = AuthenticatorSupportedOptions();
+  }
+  std::move(callback).Run();
 }
 
 void FidoDeviceAuthenticator::MakeCredential(CtapMakeCredentialRequest request,
@@ -36,6 +57,9 @@ void FidoDeviceAuthenticator::MakeCredential(CtapMakeCredentialRequest request,
   DCHECK(!task_);
   DCHECK(device_->SupportedProtocolIsInitialized())
       << "InitializeAuthenticator() must be called first.";
+
+  DCHECK(Options());
+
   // TODO(martinkr): Change FidoTasks to take all request parameters by const
   // reference, so we can avoid copying these from the RequestHandler.
   task_ = std::make_unique<MakeCredentialTask>(
@@ -46,6 +70,18 @@ void FidoDeviceAuthenticator::GetAssertion(CtapGetAssertionRequest request,
                                            GetAssertionCallback callback) {
   DCHECK(device_->SupportedProtocolIsInitialized())
       << "InitializeAuthenticator() must be called first.";
+
+  // Update the request to the "effective" user verification requirement.
+  // https://w3c.github.io/webauthn/#effective-user-verification-requirement-for-assertion
+  DCHECK(Options());
+  if (Options()->user_verification_availability() ==
+      AuthenticatorSupportedOptions::UserVerificationAvailability::
+          kSupportedAndConfigured) {
+    request.SetUserVerification(UserVerificationRequirement::kRequired);
+  } else {
+    request.SetUserVerification(UserVerificationRequirement::kDiscouraged);
+  }
+
   task_ = std::make_unique<GetAssertionTask>(device_.get(), std::move(request),
                                              std::move(callback));
 }
@@ -65,19 +101,9 @@ base::string16 FidoDeviceAuthenticator::GetDisplayName() const {
   return device_->GetDisplayName();
 }
 
-const AuthenticatorSupportedOptions& FidoDeviceAuthenticator::Options() const {
-  static const AuthenticatorSupportedOptions default_options;
-  switch (device_->supported_protocol()) {
-    case ProtocolVersion::kU2f:
-      return default_options;
-    case ProtocolVersion::kCtap:
-      DCHECK(device_->device_info()) << "uninitialized device";
-      return device_->device_info()->options();
-    case ProtocolVersion::kUnknown:
-      NOTREACHED() << "uninitialized device";
-  }
-  NOTREACHED();
-  return default_options;
+const base::Optional<AuthenticatorSupportedOptions>&
+FidoDeviceAuthenticator::Options() const {
+  return options_;
 }
 
 FidoTransportProtocol FidoDeviceAuthenticator::AuthenticatorTransport() const {

device/fido/fido_device_authenticator.h

@@ -41,7 +41,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDeviceAuthenticator
   void Cancel() override;
   std::string GetId() const override;
   base::string16 GetDisplayName() const override;
-  const AuthenticatorSupportedOptions& Options() const override;
+  const base::Optional<AuthenticatorSupportedOptions>& Options() const override;
   FidoTransportProtocol AuthenticatorTransport() const override;
   bool IsInPairingMode() const override;
   base::WeakPtr<FidoAuthenticator> GetWeakPtr() override;
@@ -59,7 +59,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDeviceAuthenticator
   void SetTaskForTesting(std::unique_ptr<FidoTask> task);
 
  private:
+  void InitializeAuthenticatorDone(base::OnceClosure callback);
+
   const std::unique_ptr<FidoDevice> device_;
+  base::Optional<AuthenticatorSupportedOptions> options_;
   std::unique_ptr<FidoTask> task_;
   base::WeakPtrFactory<FidoDeviceAuthenticator> weak_factory_;
 

device/fido/get_assertion_request_handler.cc

@@ -71,7 +71,8 @@ bool CheckResponseCredentialIdMatchesRequestAllowList(
   const auto& allow_list = request.allow_list();
   if (!allow_list || allow_list->empty()) {
     // Allow list can't be empty for authenticators w/o resident key support.
-    return authenticator.Options().supports_resident_key();
+    return !authenticator.Options() ||
+           authenticator.Options()->supports_resident_key();
   }
   // Credential ID may be omitted if allow list has size 1. Otherwise, it needs
   // to match.
@@ -98,36 +99,22 @@ void SetCredentialIdForResponseWithEmptyCredential(
 }
 
 // Checks UserVerificationRequirement enum passed from the relying party is
-// compatible with the authenticator, and updates the request to the
-// "effective" user verification requirement.
-// https://w3c.github.io/webauthn/#effective-user-verification-requirement-for-assertion
+// compatible with the authenticator.
 bool CheckUserVerificationCompatible(FidoAuthenticator* authenticator,
-                                     CtapGetAssertionRequest* request) {
-  const auto uv_availability =
-      authenticator->Options().user_verification_availability();
+                                     const CtapGetAssertionRequest& request) {
+  const auto& opt_options = authenticator->Options();
+  if (!opt_options) {
+    // This authenticator doesn't know its capabilities yet, so we need
+    // to assume it can handle the request. This is the case for Windows,
+    // where we proxy the request to the native API.
+    return true;
+  }
 
-  switch (request->user_verification()) {
-    case UserVerificationRequirement::kRequired:
-      return uv_availability ==
+  return request.user_verification() !=
+             UserVerificationRequirement::kRequired ||
+         opt_options->user_verification_availability() ==
              AuthenticatorSupportedOptions::UserVerificationAvailability::
                  kSupportedAndConfigured;
-
-    case UserVerificationRequirement::kDiscouraged:
-      return true;
-
-    case UserVerificationRequirement::kPreferred:
-      if (uv_availability ==
-          AuthenticatorSupportedOptions::UserVerificationAvailability::
-              kSupportedAndConfigured) {
-        request->SetUserVerification(UserVerificationRequirement::kRequired);
-      } else {
-        request->SetUserVerification(UserVerificationRequirement::kDiscouraged);
-      }
-      return true;
-  }
-
-  NOTREACHED();
-  return false;
 }
 
 base::flat_set<FidoTransportProtocol> GetTransportsAllowedByRP(
@@ -201,17 +188,12 @@ GetAssertionRequestHandler::~GetAssertionRequestHandler() = default;
 
 void GetAssertionRequestHandler::DispatchRequest(
     FidoAuthenticator* authenticator) {
-  // The user verification field of the request may be adjusted to the
-  // authenticator, so we need to make a copy.
-  CtapGetAssertionRequest request_copy = request_;
-  if (!CheckUserVerificationCompatible(authenticator, &request_copy)) {
+  if (!CheckUserVerificationCompatible(authenticator, request_))
     return;
-  }
 
   authenticator->GetAssertion(
-      std::move(request_copy),
-      base::BindOnce(&GetAssertionRequestHandler::HandleResponse,
-                     weak_factory_.GetWeakPtr(), authenticator));
+      request_, base::BindOnce(&GetAssertionRequestHandler::HandleResponse,
+                               weak_factory_.GetWeakPtr(), authenticator));
 }
 
 void GetAssertionRequestHandler::HandleResponse(

device/fido/mac/authenticator.h

@@ -58,7 +58,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) TouchIdAuthenticator
   void Cancel() override;
   std::string GetId() const override;
   base::string16 GetDisplayName() const override;
-  const AuthenticatorSupportedOptions& Options() const override;
+  const base::Optional<AuthenticatorSupportedOptions>& Options() const override;
   FidoTransportProtocol AuthenticatorTransport() const override;
   bool IsInPairingMode() const override;
   base::WeakPtr<FidoAuthenticator> GetWeakPtr() override;

device/fido/mac/authenticator.mm

@@ -155,8 +155,9 @@ AuthenticatorSupportedOptions TouchIdAuthenticatorOptions() {
 
 }  // namespace
 
-const AuthenticatorSupportedOptions& TouchIdAuthenticator::Options() const {
-  static const AuthenticatorSupportedOptions options =
+const base::Optional<AuthenticatorSupportedOptions>&
+TouchIdAuthenticator::Options() const {
+  static const base::Optional<AuthenticatorSupportedOptions> options =
       TouchIdAuthenticatorOptions();
   return options;
 }

device/fido/make_credential_request_handler.cc

@@ -22,39 +22,35 @@ namespace {
 
 bool CheckIfAuthenticatorSelectionCriteriaAreSatisfied(
     FidoAuthenticator* authenticator,
-    const AuthenticatorSelectionCriteria& authenticator_selection_criteria,
-    CtapMakeCredentialRequest* request) {
+    const AuthenticatorSelectionCriteria& authenticator_selection_criteria) {
   using AuthenticatorAttachment =
       AuthenticatorSelectionCriteria::AuthenticatorAttachment;
   using UvAvailability =
       AuthenticatorSupportedOptions::UserVerificationAvailability;
 
-  const auto& options = authenticator->Options();
+  const auto& opt_options = authenticator->Options();
+  if (!opt_options) {
+    // This authenticator doesn't know its capabilities yet, so we need
+    // to assume it can handle the request. This is the case for Windows,
+    // where we proxy the request to the native API.
+    return true;
+  }
+
   if ((authenticator_selection_criteria.authenticator_attachement() ==
            AuthenticatorAttachment::kPlatform &&
-       !options.is_platform_device()) ||
+       !opt_options->is_platform_device()) ||
       (authenticator_selection_criteria.authenticator_attachement() ==
            AuthenticatorAttachment::kCrossPlatform &&
-       options.is_platform_device())) {
+       opt_options->is_platform_device()))
     return false;
-  }
 
   if (authenticator_selection_criteria.require_resident_key() &&
-      !options.supports_resident_key()) {
+      !opt_options->supports_resident_key())
     return false;
-  }
-  request->SetResidentKeySupported(
-      authenticator_selection_criteria.require_resident_key());
-
-  const auto& user_verification_requirement =
-      authenticator_selection_criteria.user_verification_requirement();
-  if (user_verification_requirement == UserVerificationRequirement::kRequired) {
-    request->SetUserVerificationRequired(true);
-  }
 
-  return user_verification_requirement !=
+  return authenticator_selection_criteria.user_verification_requirement() !=
              UserVerificationRequirement::kRequired ||
-         options.user_verification_availability() ==
+         opt_options->user_verification_availability() ==
              UvAvailability::kSupportedAndConfigured;
 }
 
@@ -112,16 +108,12 @@ MakeCredentialRequestHandler::~MakeCredentialRequestHandler() = default;
 
 void MakeCredentialRequestHandler::DispatchRequest(
     FidoAuthenticator* authenticator) {
-  // The user verification field of the request may be adjusted to the
-  // authenticator, so we need to make a copy.
-  CtapMakeCredentialRequest request_copy = request_parameter_;
   if (!CheckIfAuthenticatorSelectionCriteriaAreSatisfied(
-          authenticator, authenticator_selection_criteria_, &request_copy)) {
+          authenticator, authenticator_selection_criteria_))
     return;
-  }
 
   authenticator->MakeCredential(
-      std::move(request_copy),
+      request_parameter_,
       base::BindOnce(&MakeCredentialRequestHandler::HandleResponse,
                      weak_factory_.GetWeakPtr(), authenticator));
 }

device/fido/win/authenticator.cc

@@ -35,25 +35,6 @@ base::string16 OptionalGURLToUTF16(const base::Optional<GURL>& in) {
   return in ? base::UTF8ToUTF16(in->spec()) : base::string16();
 }
 
-AuthenticatorSupportedOptions WinNativeCrossPlatformAuthenticatorOptions() {
-  AuthenticatorSupportedOptions options;
-  // Both MakeCredential and GetAssertion force CROSS_PLATFORM (in CTAP- and in
-  // U2F mode).
-  options.SetIsPlatformDevice(false);
-  // We don't know whether any of the connected USB devices support certain
-  // features such as resident keys, client PINs or UV/UP; but they might, and
-  // the platform supports them, so we set the "maximum" feature set here.
-  options.SetSupportsResidentKey(true);
-  options.SetClientPinAvailability(
-      AuthenticatorSupportedOptions::ClientPinAvailability::
-          kSupportedButPinNotSet);
-  options.SetUserVerificationAvailability(
-      AuthenticatorSupportedOptions::UserVerificationAvailability::
-          kSupportedAndConfigured);
-  options.SetUserPresenceRequired(true);
-  return options;
-}
-
 }  // namespace
 
 WinNativeCrossPlatformAuthenticator::WinNativeCrossPlatformAuthenticator(
@@ -389,11 +370,14 @@ WinNativeCrossPlatformAuthenticator::AuthenticatorTransport() const {
   return FidoTransportProtocol::kUsbHumanInterfaceDevice;
 }
 
-const AuthenticatorSupportedOptions&
+const base::Optional<AuthenticatorSupportedOptions>&
 WinNativeCrossPlatformAuthenticator::Options() const {
-  static const AuthenticatorSupportedOptions options =
-      WinNativeCrossPlatformAuthenticatorOptions();
-  return options;
+  // The request can potentially be fulfilled by any device that Windows
+  // communicates with, so returning AuthenticatorSupportedOptions really
+  // doesn't make much sense.
+  static const base::Optional<AuthenticatorSupportedOptions> no_options =
+      base::nullopt;
+  return no_options;
 }
 
 base::WeakPtr<FidoAuthenticator>

device/fido/win/authenticator.h

@@ -53,7 +53,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) WinNativeCrossPlatformAuthenticator
   std::string GetId() const override;
   base::string16 GetDisplayName() const override;
   bool IsInPairingMode() const override;
-  const AuthenticatorSupportedOptions& Options() const override;
+  const base::Optional<AuthenticatorSupportedOptions>& Options() const override;
   FidoTransportProtocol AuthenticatorTransport() const override;
   base::WeakPtr<FidoAuthenticator> GetWeakPtr() override;