[webauthn] Fix UV with platform authenticators

This patch fixes a bug where platform authenticators are always sent
MakeCredential requests with UV=discouraged regardless of relying party
authenticator selection preference and authenticator support.

Fixed: 1096764, 1091789
Change-Id: I37316f470332be3bae0cbb8e5d22f4c3e3ae7e8a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2253124
Commit-Queue: Nina Satragno <nsatragno@chromium.org>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Auto-Submit: Nina Satragno <nsatragno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781561}

device/fido/fido_authenticator.cc

@@ -37,6 +37,10 @@ void FidoAuthenticator::GetUvRetries(
   NOTREACHED();
 }
 
+bool FidoAuthenticator::CanGetUvToken() {
+  return false;
+}
+
 void FidoAuthenticator::GetUvToken(
     base::Optional<std::string> rp_id,
     FidoAuthenticator::GetTokenCallback callback) {

device/fido/fido_authenticator.h

@@ -105,9 +105,11 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoAuthenticator {
                            const std::vector<pin::Permissions>& permissions,
                            base::Optional<std::string> rp_id,
                            GetTokenCallback callback);
+  // Returns |true| if the authenticator supports GetUvToken.
+  virtual bool CanGetUvToken();
   // GetUvToken uses internal user verification to request a PinUvAuthToken from
-  // an authenticator. It is only valid to call this method if |Options|
-  // indicates that the authenticator supports UV tokens.
+  // an authenticator. It is only valid to call this method if CanGetUvToken()
+  // returns true.
   // |rp_id| must be set if the PinUvAuthToken will be used for MakeCredential
   // or GetAssertion.
   virtual void GetUvToken(base::Optional<std::string> rp_id,

device/fido/fido_device_authenticator.cc

@@ -97,12 +97,36 @@ void FidoDeviceAuthenticator::InitializeAuthenticatorDone(
 
 void FidoDeviceAuthenticator::MakeCredential(CtapMakeCredentialRequest request,
                                              MakeCredentialCallback callback) {
+  // If the authenticator has UV configured then UV will be required in
+  // order to create a credential (as specified by CTAP 2.0), even if
+  // user-verification is "discouraged". However, if the request is U2F-only
+  // then that doesn't apply and UV must be set to discouraged so that the
+  // request can be translated to U2F.
+  if (!request.pin_auth &&
+      options_->user_verification_availability ==
+          AuthenticatorSupportedOptions::UserVerificationAvailability::
+              kSupportedAndConfigured &&
+      !request.is_u2f_only) {
+    request.user_verification = UserVerificationRequirement::kRequired;
+  } else {
+    request.user_verification = UserVerificationRequirement::kDiscouraged;
+  }
+
   RunTask<MakeCredentialTask, AuthenticatorMakeCredentialResponse,
           CtapMakeCredentialRequest>(std::move(request), std::move(callback));
 }
 
 void FidoDeviceAuthenticator::GetAssertion(CtapGetAssertionRequest request,
                                            GetAssertionCallback callback) {
+  if (!request.pin_auth &&
+      options_->user_verification_availability ==
+          AuthenticatorSupportedOptions::UserVerificationAvailability::
+              kSupportedAndConfigured &&
+      request.user_verification != UserVerificationRequirement::kDiscouraged) {
+    request.user_verification = UserVerificationRequirement::kRequired;
+  } else {
+    request.user_verification = UserVerificationRequirement::kDiscouraged;
+  }
   RunTask<GetAssertionTask, AuthenticatorGetAssertionResponse,
           CtapGetAssertionRequest>(std::move(request), std::move(callback));
 }
@@ -776,6 +800,13 @@ void FidoDeviceAuthenticator::GetUvRetries(GetRetriesCallback callback) {
       base::BindOnce(&pin::RetriesResponse::ParseUvRetries));
 }
 
+bool FidoDeviceAuthenticator::CanGetUvToken() {
+  return options_->user_verification_availability ==
+             AuthenticatorSupportedOptions::UserVerificationAvailability::
+                 kSupportedAndConfigured &&
+         options_->supports_pin_uv_auth_token;
+}
+
 void FidoDeviceAuthenticator::GetUvToken(base::Optional<std::string> rp_id,
                                          GetTokenCallback callback) {
   GetEphemeralKey(base::BindOnce(

device/fido/fido_device_authenticator.h

@@ -50,6 +50,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDeviceAuthenticator
                    base::Optional<std::string> rp_id,
                    GetTokenCallback callback) override;
   void GetUvRetries(GetRetriesCallback callback) override;
+  bool CanGetUvToken() override;
   void GetUvToken(base::Optional<std::string> rp_id,
                   GetTokenCallback callback) override;
   void SetPIN(const std::string& pin,

device/fido/get_assertion_request_handler.cc

@@ -334,29 +334,20 @@ void GetAssertionRequestHandler::DispatchRequest(
   }
 
   CtapGetAssertionRequest request(request_);
-  if (authenticator->Options()) {
-    if (authenticator->Options()->user_verification_availability ==
-            AuthenticatorSupportedOptions::UserVerificationAvailability::
-                kSupportedAndConfigured &&
-        request_.user_verification !=
-            UserVerificationRequirement::kDiscouraged) {
-      if (authenticator->Options()->supports_pin_uv_auth_token) {
-        FIDO_LOG(DEBUG) << "Getting UV token from "
-                        << authenticator->GetDisplayName();
-        authenticator->GetUvToken(
-            request_.rp_id,
-            base::BindOnce(&GetAssertionRequestHandler::OnHaveUvToken,
-                           weak_factory_.GetWeakPtr(), authenticator));
-        return;
-      }
-      request.user_verification = UserVerificationRequirement::kRequired;
-    } else {
-      request.user_verification = UserVerificationRequirement::kDiscouraged;
-    }
-    if (android_client_data_ext_ && authenticator->Options() &&
-        authenticator->Options()->supports_android_client_data_ext) {
-      request.android_client_data_ext = *android_client_data_ext_;
-    }
+  if (request.user_verification != UserVerificationRequirement::kDiscouraged &&
+      authenticator->CanGetUvToken()) {
+    FIDO_LOG(DEBUG) << "Getting UV token from "
+                    << authenticator->GetDisplayName();
+    authenticator->GetUvToken(
+        request_.rp_id,
+        base::BindOnce(&GetAssertionRequestHandler::OnHaveUvToken,
+                       weak_factory_.GetWeakPtr(), authenticator));
+    return;
+  }
+
+  if (android_client_data_ext_ && authenticator->Options() &&
+      authenticator->Options()->supports_android_client_data_ext) {
+    request.android_client_data_ext = *android_client_data_ext_;
   }
 
   ReportGetAssertionRequestTransport(authenticator);
@@ -796,8 +787,6 @@ void GetAssertionRequestHandler::DispatchRequestWithToken(
   CtapGetAssertionRequest request(request_);
   request.pin_auth = token.PinAuth(request.client_data_hash);
   request.pin_protocol = pin::kProtocolVersion;
-  // Do not do internal UV again.
-  request.user_verification = UserVerificationRequirement::kDiscouraged;
 
   if (android_client_data_ext_ && authenticator_->Options() &&
       authenticator_->Options()->supports_android_client_data_ext) {

device/fido/make_credential_handler_unittest.cc

@@ -13,6 +13,7 @@
 #include "components/cbor/values.h"
 #include "device/bluetooth/bluetooth_adapter_factory.h"
 #include "device/bluetooth/test/mock_bluetooth_adapter.h"
+#include "device/fido/authenticator_get_info_response.h"
 #include "device/fido/authenticator_make_credential_response.h"
 #include "device/fido/authenticator_selection_criteria.h"
 #include "device/fido/ctap_make_credential_request.h"
@@ -331,6 +332,50 @@ MATCHER(IsResidentKeyRequest, "") {
   return true;
 }
 
+// Matches a CTAP command that is:
+// * A valid make credential request,
+// * if |is_uv| is true,
+//   * with an options map present,
+//   * and options.uv present and true.
+// * if |is_uv_| is false,
+//   * with an options map not present,
+//   * or options.uv not present or false.
+MATCHER_P(IsUvRequest, is_uv, "") {
+  if (arg.empty() ||
+      arg[0] != base::strict_cast<uint8_t>(
+                    CtapRequestCommand::kAuthenticatorMakeCredential)) {
+    *result_listener << "not make credential";
+    return false;
+  }
+
+  base::span<const uint8_t> param_bytes(arg);
+  param_bytes = param_bytes.subspan(1);
+  const auto maybe_map = cbor::Reader::Read(param_bytes);
+  if (!maybe_map || !maybe_map->is_map()) {
+    *result_listener << "not a map";
+    return false;
+  }
+  const auto& map = maybe_map->GetMap();
+
+  const auto options_it = map.find(cbor::Value(7));
+  if (options_it == map.end() || !options_it->second.is_map()) {
+    return is_uv == false;
+  }
+  const auto& options = options_it->second.GetMap();
+
+  const auto uv_it = options.find(cbor::Value("uv"));
+  if (uv_it == options.end()) {
+    return is_uv == false;
+  }
+
+  if (!uv_it->second.is_bool()) {
+    *result_listener << "'uv' is not a boolean";
+    return false;
+  }
+
+  return uv_it->second.GetBool() == is_uv;
+}
+
 ACTION_P(Reply, reply) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
@@ -641,6 +686,34 @@ TEST_F(FidoMakeCredentialHandlerTest,
   EXPECT_EQ(MakeCredentialStatus::kUserConsentDenied, callback().status());
 }
 
+TEST_F(FidoMakeCredentialHandlerTest,
+       TestCrossPlatformAuthenticatorsForceUVWhenSupported) {
+  const auto& test_info_response = test_data::kTestAuthenticatorGetInfoResponse;
+  ASSERT_EQ(ReadCTAPGetInfoResponse(test_info_response)
+                ->options.user_verification_availability,
+            AuthenticatorSupportedOptions::UserVerificationAvailability::
+                kSupportedAndConfigured);
+
+  auto device =
+      MockFidoDevice::MakeCtapWithGetInfoExpectation(test_info_response);
+  device->SetDeviceTransport(FidoTransportProtocol::kUsbHumanInterfaceDevice);
+  device->ExpectCtap2CommandAndRespondWith(
+      CtapRequestCommand::kAuthenticatorMakeCredential,
+      test_data::kTestMakeCredentialResponse, base::TimeDelta(),
+      IsUvRequest(true));
+
+  auto request_handler =
+      CreateMakeCredentialHandlerWithAuthenticatorSelectionCriteria(
+          AuthenticatorSelectionCriteria(
+              AuthenticatorAttachment::kAny, /*require_resident_key=*/false,
+              UserVerificationRequirement::kDiscouraged));
+  discovery()->AddDevice(std::move(device));
+  discovery()->WaitForCallToStartAndSimulateSuccess();
+
+  callback().WaitForCallback();
+  EXPECT_EQ(MakeCredentialStatus::kSuccess, callback().status());
+}
+
 // If a device returns CTAP2_ERR_PIN_AUTH_INVALID, the request should complete
 // with MakeCredentialStatus::kUserConsentDenied.
 TEST_F(FidoMakeCredentialHandlerTest, TestRequestWithPinAuthInvalid) {

device/fido/make_credential_request_handler.cc

@@ -432,34 +432,21 @@ void MakeCredentialRequestHandler::DispatchRequest(
   }
 
   CtapMakeCredentialRequest request(request_);
-  if (authenticator->Options()) {
-    // If the authenticator has UV configured then UV will be required in
-    // order to create a credential (as specified by CTAP 2.0), even if
-    // user-verification is "discouraged". However, if the request is U2F-only
-    // then that doesn't apply and UV must be set to discouraged so that the
-    // request can be translated to U2F. Platform authenticators are exempted
-    // from this UV enforcement.
-    if (authenticator->Options()->user_verification_availability ==
-            AuthenticatorSupportedOptions::UserVerificationAvailability::
-                kSupportedAndConfigured &&
-        !request_.is_u2f_only &&
-        authenticator->AuthenticatorTransport() !=
-            FidoTransportProtocol::kInternal) {
-      if (authenticator->Options()->supports_pin_uv_auth_token) {
-        authenticator->GetUvToken(
-            request_.rp.id,
-            base::BindOnce(&MakeCredentialRequestHandler::OnHaveUvToken,
-                           weak_factory_.GetWeakPtr(), authenticator));
-        return;
-      }
-      request.user_verification = UserVerificationRequirement::kRequired;
-    } else {
-      request.user_verification = UserVerificationRequirement::kDiscouraged;
-    }
 
+  if (authenticator->Options()) {
     SpecializeRequestForAuthenticator(&request, authenticator);
   }
 
+  if (!request_.is_u2f_only &&
+      request.user_verification != UserVerificationRequirement::kDiscouraged &&
+      authenticator->CanGetUvToken()) {
+    authenticator->GetUvToken(
+        request_.rp.id,
+        base::BindOnce(&MakeCredentialRequestHandler::OnHaveUvToken,
+                       weak_factory_.GetWeakPtr(), authenticator));
+    return;
+  }
+
   ReportMakeCredentialRequestTransport(authenticator);
 
   authenticator->MakeCredential(
@@ -917,8 +904,6 @@ void MakeCredentialRequestHandler::DispatchRequestWithToken(
   CtapMakeCredentialRequest request(request_);
   request.pin_auth = token.PinAuth(request.client_data_hash);
   request.pin_protocol = pin::kProtocolVersion;
-  // Do not do internal UV again.
-  request.user_verification = UserVerificationRequirement::kDiscouraged;
   SpecializeRequestForAuthenticator(&request, authenticator_);
 
   ReportMakeCredentialRequestTransport(authenticator_);

device/fido/mock_fido_device.cc

@@ -86,12 +86,12 @@ std::vector<uint8_t> MockFidoDevice::EncodeCBORRequest(
   return request_bytes;
 }
 
-// Matcher to compare the fist byte of the incoming requests.
+// Matcher to compare the first byte of the incoming requests.
 MATCHER_P(IsCtap2Command, expected_command, "") {
   return !arg.empty() && arg[0] == base::strict_cast<uint8_t>(expected_command);
 }
 
-MockFidoDevice::MockFidoDevice() {}
+MockFidoDevice::MockFidoDevice() = default;
 MockFidoDevice::MockFidoDevice(
     ProtocolVersion protocol_version,
     base::Optional<AuthenticatorGetInfoResponse> device_info)
@@ -138,14 +138,17 @@ void MockFidoDevice::StubGetId() {
 void MockFidoDevice::ExpectCtap2CommandAndRespondWith(
     CtapRequestCommand command,
     base::Optional<base::span<const uint8_t>> response,
-    base::TimeDelta delay) {
+    base::TimeDelta delay,
+    testing::Matcher<base::span<const uint8_t>> request_matcher) {
   auto data = fido_parsing_utils::MaterializeOrNull(response);
   auto send_response = [ data(std::move(data)), delay ](DeviceCallback & cb) {
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE, base::BindOnce(std::move(cb), std::move(data)), delay);
   };
 
-  EXPECT_CALL(*this, DeviceTransactPtr(IsCtap2Command(command), ::testing::_))
+  EXPECT_CALL(*this,
+              DeviceTransactPtr(AllOf(IsCtap2Command(command), request_matcher),
+                                ::testing::_))
       .WillOnce(::testing::DoAll(
           ::testing::WithArg<1>(::testing::Invoke(send_response)),
           ::testing::Return(0)));

device/fido/mock_fido_device.h

@@ -85,7 +85,9 @@ class MockFidoDevice : public ::testing::StrictMock<FidoDevice> {
   void ExpectCtap2CommandAndRespondWith(
       CtapRequestCommand command,
       base::Optional<base::span<const uint8_t>> response,
-      base::TimeDelta delay = base::TimeDelta());
+      base::TimeDelta delay = base::TimeDelta(),
+      testing::Matcher<base::span<const uint8_t>> request_matcher =
+          testing::A<base::span<const uint8_t>>());
   void ExpectCtap2CommandAndRespondWithError(
       CtapRequestCommand command,
       CtapDeviceResponseCode response_code,