fido: make CtapMakeCredentialRequest carry the full UserVerificationRequirement

This adds a UserVerificationRequirement enum field to
CtapMakeCredentialRequest. The existing "effective" user verification
bool is removed and the computation of the effective value (required vs
discouraged) is moved into FidoDeviceAuthenticator, in parallel to how
this is handled for GetAssertion requests.

This change makes the full uv requirement available to the Windows
authenticators, and at the same time fixes a bug for device
authenticators where requests with uv="preferred" would result in
CTAP device requests with uv=false even for authenticators with UV
support.

Also rename CtapMakeCredentialRequest::resident_key_supported() to
resident_key_required().

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

content/browser/webauth/authenticator_impl.cc

@@ -224,19 +224,6 @@ 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/ctap_make_credential_request.cc

@@ -71,16 +71,15 @@ std::vector<uint8_t> CtapMakeCredentialRequest::EncodeAsCBOR() const {
 
   cbor::Value::MapValue option_map;
 
-  // Resident keys are not supported by default.
-  if (resident_key_supported_) {
+  // Resident keys are not required by default.
+  if (resident_key_required_) {
     option_map[cbor::Value(kResidentKeyMapKey)] =
-        cbor::Value(resident_key_supported_);
+        cbor::Value(resident_key_required_);
   }
 
   // User verification is not required by default.
-  if (user_verification_required_) {
-    option_map[cbor::Value(kUserVerificationMapKey)] =
-        cbor::Value(user_verification_required_);
+  if (user_verification_ == UserVerificationRequirement::kRequired) {
+    option_map[cbor::Value(kUserVerificationMapKey)] = cbor::Value(true);
   }
 
   if (!option_map.empty()) {
@@ -97,16 +96,15 @@ std::vector<uint8_t> CtapMakeCredentialRequest::EncodeAsCBOR() const {
   return cbor_request;
 }
 
-CtapMakeCredentialRequest&
-CtapMakeCredentialRequest::SetUserVerificationRequired(
-    bool user_verification_required) {
-  user_verification_required_ = user_verification_required;
+CtapMakeCredentialRequest& CtapMakeCredentialRequest::SetUserVerification(
+    UserVerificationRequirement user_verification) {
+  user_verification_ = user_verification;
   return *this;
 }
 
-CtapMakeCredentialRequest& CtapMakeCredentialRequest::SetResidentKeySupported(
-    bool resident_key_supported) {
-  resident_key_supported_ = resident_key_supported;
+CtapMakeCredentialRequest& CtapMakeCredentialRequest::SetResidentKeyRequired(
+    bool resident_key_required) {
+  resident_key_required_ = resident_key_required;
   return *this;
 }
 

device/fido/ctap_make_credential_request.h

@@ -45,9 +45,9 @@ class COMPONENT_EXPORT(DEVICE_FIDO) CtapMakeCredentialRequest {
   // https://drafts.fidoalliance.org/fido-2/latest/fido-client-to-authenticator-protocol-v2.0-wd-20180305.html#authenticatorMakeCredential
   std::vector<uint8_t> EncodeAsCBOR() const;
 
-  CtapMakeCredentialRequest& SetUserVerificationRequired(
-      bool user_verfication_required);
-  CtapMakeCredentialRequest& SetResidentKeySupported(bool resident_key);
+  CtapMakeCredentialRequest& SetUserVerification(
+      UserVerificationRequirement user_verification);
+  CtapMakeCredentialRequest& SetResidentKeyRequired(bool resident_key);
   CtapMakeCredentialRequest& SetExcludeList(
       std::vector<PublicKeyCredentialDescriptor> exclude_list);
   CtapMakeCredentialRequest& SetPinAuth(std::vector<uint8_t> pin_auth);
@@ -65,10 +65,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) CtapMakeCredentialRequest {
   const PublicKeyCredentialParams& public_key_credential_params() const {
     return public_key_credential_params_;
   }
-  bool user_verification_required() const {
-    return user_verification_required_;
+  UserVerificationRequirement user_verification() const {
+    return user_verification_;
   }
-  bool resident_key_supported() const { return resident_key_supported_; }
+  bool resident_key_required() const { return resident_key_required_; }
   bool is_individual_attestation() const { return is_individual_attestation_; }
   bool hmac_secret() const { return hmac_secret_; }
   const base::Optional<std::vector<PublicKeyCredentialDescriptor>>&
@@ -85,8 +85,9 @@ class COMPONENT_EXPORT(DEVICE_FIDO) CtapMakeCredentialRequest {
   PublicKeyCredentialRpEntity rp_;
   PublicKeyCredentialUserEntity user_;
   PublicKeyCredentialParams public_key_credential_params_;
-  bool user_verification_required_ = false;
-  bool resident_key_supported_ = false;
+  UserVerificationRequirement user_verification_ =
+      UserVerificationRequirement::kPreferred;
+  bool resident_key_required_ = false;
   bool is_individual_attestation_ = false;
   // hmac_secret_ indicates whether the "hmac-secret" extension should be
   // asserted to CTAP2 authenticators.

device/fido/ctap_request_unittest.cc

@@ -30,9 +30,10 @@ TEST(CTAPRequestTest, TestConstructMakeCredentialRequestParam) {
       test_data::kClientDataJson, std::move(rp), std::move(user),
       PublicKeyCredentialParams({{CredentialType::kPublicKey, 7},
                                  {CredentialType::kPublicKey, 257}}));
-  auto serialized_data = make_credential_param.SetResidentKeySupported(true)
-                             .SetUserVerificationRequired(true)
-                             .EncodeAsCBOR();
+  auto serialized_data =
+      make_credential_param.SetResidentKeyRequired(true)
+          .SetUserVerification(UserVerificationRequirement::kRequired)
+          .EncodeAsCBOR();
   EXPECT_THAT(serialized_data, ::testing::ElementsAreArray(
                                    test_data::kCtapMakeCredentialRequest));
 }
@@ -111,8 +112,9 @@ TEST(VirtualCtap2DeviceTest, ParseMakeCredentialRequestForVirtualCtapKey) {
                      .public_key_credential_params()
                      .at(1)
                      .algorithm);
-  EXPECT_TRUE(request.user_verification_required());
-  EXPECT_TRUE(request.resident_key_supported());
+  EXPECT_EQ(UserVerificationRequirement::kRequired,
+            request.user_verification());
+  EXPECT_TRUE(request.resident_key_required());
 }
 
 TEST(VirtualCtap2DeviceTest, ParseGetAssertionRequestForVirtualCtapKey) {

device/fido/fido_device_authenticator.cc

@@ -57,9 +57,18 @@ void FidoDeviceAuthenticator::MakeCredential(CtapMakeCredentialRequest request,
   DCHECK(!task_);
   DCHECK(device_->SupportedProtocolIsInitialized())
       << "InitializeAuthenticator() must be called first.";
-
   DCHECK(Options());
 
+  // Update the request to the "effective" user verification requirement.
+  // https://w3c.github.io/webauthn/#effective-user-verification-requirement-for-credential-creation
+  if (Options()->user_verification_availability() ==
+      AuthenticatorSupportedOptions::UserVerificationAvailability::
+          kSupportedAndConfigured) {
+    request.SetUserVerification(UserVerificationRequirement::kRequired);
+  } else {
+    request.SetUserVerification(UserVerificationRequirement::kDiscouraged);
+  }
+
   // 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>(
@@ -68,12 +77,13 @@ void FidoDeviceAuthenticator::MakeCredential(CtapMakeCredentialRequest request,
 
 void FidoDeviceAuthenticator::GetAssertion(CtapGetAssertionRequest request,
                                            GetAssertionCallback callback) {
+  DCHECK(!task_);
   DCHECK(device_->SupportedProtocolIsInitialized())
       << "InitializeAuthenticator() must be called first.";
+  DCHECK(Options());
 
   // 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) {

device/fido/make_credential_request_handler.cc

@@ -112,6 +112,13 @@ void MakeCredentialRequestHandler::DispatchRequest(
           authenticator, authenticator_selection_criteria_))
     return;
 
+  // Set the rk and uv fields, which were only initialized to default values
+  // up to here.
+  request_parameter_.SetResidentKeyRequired(
+      authenticator_selection_criteria_.require_resident_key());
+  request_parameter_.SetUserVerification(
+      authenticator_selection_criteria_.user_verification_requirement());
+
   authenticator->MakeCredential(
       request_parameter_,
       base::BindOnce(&MakeCredentialRequestHandler::HandleResponse,

device/fido/make_credential_task.cc

@@ -27,7 +27,7 @@ namespace {
 // See: https://crbug.com/870892
 bool IsClientPinOptionCompatible(const FidoDevice* device,
                                  const CtapMakeCredentialRequest& request) {
-  if (request.user_verification_required())
+  if (request.user_verification() == UserVerificationRequirement::kRequired)
     return true;
 
   DCHECK(device && device->device_info());

device/fido/make_credential_task_unittest.cc

@@ -177,7 +177,7 @@ TEST_F(FidoMakeCredentialTaskTest, EnforceClientPinWhenUserVerificationSet) {
       test_data::kClientDataJson, std::move(rp), std::move(user),
       PublicKeyCredentialParams(
           std::vector<PublicKeyCredentialParams::CredentialInfo>(1)));
-  request.SetUserVerificationRequired(true);
+  request.SetUserVerification(UserVerificationRequirement::kRequired);
   const auto task = std::make_unique<MakeCredentialTask>(
       device.get(), std::move(request), callback_receiver_.callback());
 

device/fido/u2f_command_constructor.cc

@@ -15,7 +15,8 @@ namespace device {
 
 bool IsConvertibleToU2fRegisterCommand(
     const CtapMakeCredentialRequest& request) {
-  if (request.user_verification_required() || request.resident_key_supported())
+  if (request.user_verification() == UserVerificationRequirement::kRequired ||
+      request.resident_key_required())
     return false;
 
   const auto& public_key_credential_info =

device/fido/u2f_command_constructor_unittest.cc

@@ -128,14 +128,15 @@ TEST(U2fCommandConstructorTest, TestU2fRegisterCredentialAlgorithmRequirement) {
 
 TEST(U2fCommandConstructorTest, TestU2fRegisterUserVerificationRequirement) {
   auto make_credential_param = ConstructMakeCredentialRequest();
-  make_credential_param.SetUserVerificationRequired(true);
+  make_credential_param.SetUserVerification(
+      UserVerificationRequirement::kRequired);
 
   EXPECT_FALSE(IsConvertibleToU2fRegisterCommand(make_credential_param));
 }
 
 TEST(U2fCommandConstructorTest, TestU2fRegisterResidentKeyRequirement) {
   auto make_credential_param = ConstructMakeCredentialRequest();
-  make_credential_param.SetResidentKeySupported(true);
+  make_credential_param.SetResidentKeyRequired(true);
 
   EXPECT_FALSE(IsConvertibleToU2fRegisterCommand(make_credential_param));
 }

device/fido/virtual_ctap2_device.cc

@@ -52,10 +52,11 @@ void ReturnCtap2Response(
 
 bool AreMakeCredentialOptionsValid(const AuthenticatorSupportedOptions& options,
                                    const CtapMakeCredentialRequest& request) {
-  if (request.resident_key_supported() && !options.supports_resident_key())
+  if (request.resident_key_required() && !options.supports_resident_key())
     return false;
 
-  return !request.user_verification_required() ||
+  return request.user_verification() !=
+             UserVerificationRequirement::kRequired ||
          options.user_verification_availability() ==
              AuthenticatorSupportedOptions::UserVerificationAvailability::
                  kSupportedAndConfigured;
@@ -555,12 +556,15 @@ ParseCtapMakeCredentialRequest(base::span<const uint8_t> request_bytes) {
     const auto resident_key_option =
         option_map.find(cbor::Value(kResidentKeyMapKey));
     if (resident_key_option != option_map.end())
-      request.SetResidentKeySupported(resident_key_option->second.GetBool());
+      request.SetResidentKeyRequired(resident_key_option->second.GetBool());
 
     const auto uv_option =
         option_map.find(cbor::Value(kUserVerificationMapKey));
     if (uv_option != option_map.end())
-      request.SetUserVerificationRequired(uv_option->second.GetBool());
+      request.SetUserVerification(
+          uv_option->second.GetBool()
+              ? UserVerificationRequirement::kRequired
+              : UserVerificationRequirement::kDiscouraged);
   }
 
   const auto pin_auth_it = request_map.find(cbor::Value(8));

device/fido/win/authenticator.cc

@@ -175,15 +175,6 @@ void WinNativeCrossPlatformAuthenticator::MakeCredentialBlocking(
     }
   }
 
-  // TODO(crbug/896522): The Windows API uses the W3C WebAuthn Authenticator
-  // Model abstraction whereas FidoAuthenticator operates at the CTAP2 spec
-  // level (e.g. CtapMakeCredentialRequest). As a result we only have the
-  // boolean "uv" rather than the enum value from the original request.
-  const uint32_t user_verification_requirement =
-      request.user_verification_required()
-          ? WEBAUTHN_USER_VERIFICATION_REQUIREMENT_REQUIRED
-          : WEBAUTHN_USER_VERIFICATION_REQUIREMENT_DISCOURAGED;
-
   std::vector<WEBAUTHN_EXTENSION> extensions;
   if (request.hmac_secret()) {
     static BOOL kHMACSecretTrue = TRUE;
@@ -202,7 +193,8 @@ void WinNativeCrossPlatformAuthenticator::MakeCredentialBlocking(
       // generally displayed first in the Windows UI.
       use_u2f_only_ ? WEBAUTHN_AUTHENTICATOR_ATTACHMENT_CROSS_PLATFORM_U2F_V2
                     : WEBAUTHN_AUTHENTICATOR_ATTACHMENT_CROSS_PLATFORM,
-      request.resident_key_supported(), user_verification_requirement,
+      request.resident_key_required(),
+      ToWinUserVerificationRequirement(request.user_verification()),
       WEBAUTHN_ATTESTATION_CONVEYANCE_PREFERENCE_DIRECT, 0 /* flags */,
       &cancellation_id_};