fido: disable CrOS u2fd virtual HID device during WebAuthn requests

This adds the ability to selectively ignore HID FIDO devices by VID/PID
during discovery in FidoHidDiscovery and ignores the ChromeOS u2fd
virtual HID device during WebAuthn requests that don't originate from
cryptoken if the ChromeOS platform authenticator is flag-enabled.

The virtual HID device and the platform authenticator are both
implemented in u2fd and both bring their own UI, so having a single
WebAuthn request target both would cause all kinds of trouble.

The platform authenticator will be able to challenge credentials
registered by the virtual HID device via the appID extension. So
eventually we might want to make the platform authenticator feature flag
disable the virtual HID device during cryptotoken requests as well, and
advise users to switch to the WebAuthn API instead.

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

content/browser/webauth/authenticator_common.cc

@@ -528,7 +528,8 @@ base::flat_set<device::FidoTransportProtocol> GetAvailableTransports(
 // given frame.
 std::unique_ptr<device::FidoDiscoveryFactory> MakeDiscoveryFactory(
     RenderFrameHost* render_frame_host,
-    AuthenticatorRequestClientDelegate* request_delegate) {
+    AuthenticatorRequestClientDelegate* request_delegate,
+    bool is_u2f_api_request) {
   VirtualAuthenticatorManagerImpl* virtual_authenticator_manager =
       AuthenticatorEnvironmentImpl::GetInstance()
           ->MaybeGetVirtualAuthenticatorManager(
@@ -551,6 +552,17 @@ std::unique_ptr<device::FidoDiscoveryFactory> MakeDiscoveryFactory(
   }
 #endif  // defined(OS_WIN)
 
+#if defined(OS_CHROMEOS)
+  // Ignore the ChromeOS u2fd virtual U2F HID device for WebAuthn requests so
+  // that it doesn't collide with the ChromeOS platform authenticator, also
+  // implemented in u2fd.
+  if (base::FeatureList::IsEnabled(device::kWebAuthCrosPlatformAuthenticator) &&
+      !is_u2f_api_request) {
+    constexpr device::VidPid kChromeOsU2fdVidPid{0x18d1, 0x502c};
+    discovery_factory->set_hid_ignore_list({kChromeOsU2fdVidPid});
+  }
+#endif  // defined(OS_CHROMEOS)
+
   return discovery_factory;
 }
 
@@ -1096,7 +1108,8 @@ void AuthenticatorCommon::IsUserVerifyingPlatformAuthenticatorAvailable(
                         : maybe_request_delegate.get();
 
   std::unique_ptr<device::FidoDiscoveryFactory> discovery_factory =
-      MakeDiscoveryFactory(render_frame_host_, request_delegate_ptr);
+      MakeDiscoveryFactory(render_frame_host_, request_delegate_ptr,
+                           /*is_u2f_api_request=*/false);
   device::FidoDiscoveryFactory* discovery_factory_testing_override =
       AuthenticatorEnvironmentImpl::GetInstance()
           ->MaybeGetDiscoveryFactoryTestOverride();
@@ -1608,8 +1621,11 @@ device::FidoDiscoveryFactory* AuthenticatorCommon::discovery_factory() {
 
 void AuthenticatorCommon::InitDiscoveryFactory() {
   DCHECK(!discovery_factory_ && !discovery_factory_testing_override_);
-  discovery_factory_ =
-      MakeDiscoveryFactory(render_frame_host_, request_delegate_.get());
+  const bool is_u2f_api_request =
+      WebAuthRequestSecurityChecker::OriginIsCryptoTokenExtension(
+          caller_origin_);
+  discovery_factory_ = MakeDiscoveryFactory(
+      render_frame_host_, request_delegate_.get(), is_u2f_api_request);
   // TODO(martinkr): |discovery_factory_testing_override_| is a long-lived
   // VirtualFidoDeviceDiscovery so that tests can maintain and alter virtual
   // authenticator state in between requests. We should extract a longer-lived

device/fido/fido_discovery_factory.cc

@@ -32,19 +32,6 @@
 
 namespace device {
 
-namespace {
-
-std::unique_ptr<FidoDiscoveryBase> CreateUsbFidoDiscovery() {
-#if defined(OS_ANDROID)
-  NOTREACHED() << "USB HID not supported on Android.";
-  return nullptr;
-#else
-  return std::make_unique<FidoHidDiscovery>();
-#endif  // !defined(OS_ANDROID)
-}
-
-}  // namespace
-
 FidoDiscoveryFactory::FidoDiscoveryFactory() = default;
 FidoDiscoveryFactory::~FidoDiscoveryFactory() = default;
 
@@ -52,7 +39,7 @@ std::unique_ptr<FidoDiscoveryBase> FidoDiscoveryFactory::Create(
     FidoTransportProtocol transport) {
   switch (transport) {
     case FidoTransportProtocol::kUsbHumanInterfaceDevice:
-      return CreateUsbFidoDiscovery();
+      return std::make_unique<FidoHidDiscovery>(hid_ignore_list_);
     case FidoTransportProtocol::kBluetoothLowEnergy:
       return nullptr;
     case FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy:
@@ -106,6 +93,11 @@ void FidoDiscoveryFactory::set_cable_pairing_callback(
   cable_pairing_callback_.emplace(std::move(pairing_callback));
 }
 
+void FidoDiscoveryFactory::set_hid_ignore_list(
+    base::flat_set<VidPid> hid_ignore_list) {
+  hid_ignore_list_ = std::move(hid_ignore_list);
+}
+
 #if defined(OS_WIN)
 void FidoDiscoveryFactory::set_win_webauthn_api(WinWebAuthnApi* api) {
   win_webauthn_api_ = api;

device/fido/fido_discovery_factory.h

@@ -16,6 +16,7 @@
 #include "device/fido/fido_discovery_base.h"
 #include "device/fido/fido_request_handler_base.h"
 #include "device/fido/fido_transport_protocol.h"
+#include "device/fido/hid/fido_hid_discovery.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "services/device/public/mojom/usb_manager.mojom.h"
 
@@ -58,6 +59,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscoveryFactory {
   void set_cable_pairing_callback(
       base::RepeatingCallback<void(std::unique_ptr<CableDiscoveryData>)>);
 
+  void set_hid_ignore_list(base::flat_set<VidPid> hid_ignore_list);
+
 #if defined(OS_MAC)
   // Configures the Touch ID authenticator. Set to base::nullopt to disable it.
   void set_mac_touch_id_info(
@@ -96,6 +99,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscoveryFactory {
 #if defined(OS_WIN)
   WinWebAuthnApi* win_webauthn_api_ = nullptr;
 #endif  // defined(OS_WIN)
+  base::flat_set<VidPid> hid_ignore_list_;
 };
 
 }  // namespace device

device/fido/hid/fido_hid_discovery.cc

@@ -8,12 +8,24 @@
 
 #include "base/bind.h"
 #include "base/no_destructor.h"
+#include "base/stl_util.h"
+#include "components/device_event_log/device_event_log.h"
 #include "device/fido/hid/fido_hid_device.h"
 
 namespace device {
 
 namespace {
 
+// Checks that the supported report sizes of |device| are sufficient for at
+// least one byte of non-header data per report and not larger than our maximum
+// size.
+bool ReportSizesSufficient(const device::mojom::HidDeviceInfo& device) {
+  return device.max_input_report_size > kHidInitPacketHeaderSize &&
+         device.max_input_report_size <= kHidMaxPacketSize &&
+         device.max_output_report_size > kHidInitPacketHeaderSize &&
+         device.max_output_report_size <= kHidMaxPacketSize;
+}
+
 FidoHidDiscovery::HidManagerBinder& GetHidManagerBinder() {
   static base::NoDestructor<FidoHidDiscovery::HidManagerBinder> binder;
   return *binder;
@@ -21,10 +33,19 @@ FidoHidDiscovery::HidManagerBinder& GetHidManagerBinder() {
 
 }  // namespace
 
-FidoHidDiscovery::FidoHidDiscovery()
-    : FidoDeviceDiscovery(FidoTransportProtocol::kUsbHumanInterfaceDevice) {
-  // TODO(piperc@): Give this constant a name.
-  filter_.SetUsagePage(0xf1d0);
+bool operator==(const VidPid& lhs, const VidPid& rhs) {
+  return lhs.vid == rhs.vid && lhs.pid == rhs.pid;
+}
+
+bool operator<(const VidPid& lhs, const VidPid& rhs) {
+  return lhs.vid < rhs.vid || (lhs.vid == rhs.vid && lhs.pid < rhs.pid);
+}
+
+FidoHidDiscovery::FidoHidDiscovery(base::flat_set<VidPid> ignore_list)
+    : FidoDeviceDiscovery(FidoTransportProtocol::kUsbHumanInterfaceDevice),
+      ignore_list_(std::move(ignore_list)) {
+  constexpr uint16_t kFidoUsagePage = 0xf1d0;
+  filter_.SetUsagePage(kFidoUsagePage);
 }
 
 FidoHidDiscovery::~FidoHidDiscovery() = default;
@@ -54,18 +75,19 @@ void FidoHidDiscovery::DeviceAdded(
       kHidInitPacketHeaderSize >= kHidContinuationPacketHeaderSize,
       "init header is expected to be larger than continuation header");
 
-  // Ignore non-U2F devices.
-  if (filter_.Matches(*device_info) &&
-      // Check that the supported report sizes are sufficient for at least one
-      // byte of non-header data per report and not larger than our maximum
-      // size.
-      device_info->max_input_report_size > kHidInitPacketHeaderSize &&
-      device_info->max_input_report_size <= kHidMaxPacketSize &&
-      device_info->max_output_report_size > kHidInitPacketHeaderSize &&
-      device_info->max_output_report_size <= kHidMaxPacketSize) {
-    AddDevice(std::make_unique<FidoHidDevice>(std::move(device_info),
-                                              hid_manager_.get()));
+  if (!filter_.Matches(*device_info) || !ReportSizesSufficient(*device_info)) {
+    return;
+  }
+
+  const VidPid vid_pid{device_info->vendor_id, device_info->product_id};
+  if (base::Contains(ignore_list_, vid_pid)) {
+    FIDO_LOG(EVENT) << "Ignoring HID device " << vid_pid.vid << ":"
+                    << vid_pid.pid;
+    return;
   }
+
+  AddDevice(std::make_unique<FidoHidDevice>(std::move(device_info),
+                                            hid_manager_.get()));
 }
 
 void FidoHidDiscovery::DeviceRemoved(

device/fido/hid/fido_hid_discovery.h

@@ -10,6 +10,7 @@
 
 #include "base/callback.h"
 #include "base/component_export.h"
+#include "base/containers/flat_set.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "device/fido/fido_device_discovery.h"
@@ -21,14 +22,22 @@
 
 namespace device {
 
-// TODO(crbug/769631): Now the U2F is talking to HID via mojo, once the U2F
-// servicification is unblocked, we'll move U2F back to //service/device/.
-// Then it will talk to HID via C++ as part of servicifying U2F.
+// VidPid represents an HID vendor and product ID pair.
+struct COMPONENT_EXPORT(DEVICE_FIDO) VidPid {
+  uint16_t vid;
+  uint16_t pid;
+};
+
+COMPONENT_EXPORT(DEVICE_FIDO)
+bool operator==(const VidPid& lhs, const VidPid& rhs);
+COMPONENT_EXPORT(DEVICE_FIDO)
+bool operator<(const VidPid& lhs, const VidPid& rhs);
+
 class COMPONENT_EXPORT(DEVICE_FIDO) FidoHidDiscovery
     : public FidoDeviceDiscovery,
       device::mojom::HidManagerClient {
  public:
-  FidoHidDiscovery();
+  explicit FidoHidDiscovery(base::flat_set<VidPid> ignore_list = {});
   ~FidoHidDiscovery() override;
 
   // Sets a callback for this class to use when binding a HidManager receiver.
@@ -49,6 +58,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoHidDiscovery
   mojo::Remote<device::mojom::HidManager> hid_manager_;
   mojo::AssociatedReceiver<device::mojom::HidManagerClient> receiver_{this};
   HidDeviceFilter filter_;
+  base::flat_set<VidPid> ignore_list_;
   base::WeakPtrFactory<FidoHidDiscovery> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(FidoHidDiscovery);