From f09e4b4651141e0428bd8c19db757bd3348a473e Mon Sep 17 00:00:00 2001
From: Rob Pitkin <robpitkin@google.com>
Date: Thu, 16 Jul 2026 11:11:37 -0700
Subject: [PATCH] hid: Recursively filter nested collections

WebHID security filters (blocklist and report protection) previously
only evaluated Top-Level Collections (TLCs). If a device nested a
sensitive collection (like a Keyboard or FIDO key) inside a benign TLC
(like a Gamepad), the sensitive reports bypassed the blocklist because
they were evaluated against the benign TLC's usage.

This CL patches the bypass by introducing recursive evaluation:

1. Recursive Blocklisting: Updated HidBlocklist::CheckBlocklistEntry to
recursively evaluate child collections.

2. Recursive Connection Protection: Updated IsReportProtected in
HidConnection to recursively check if a report belongs to a FIDO or
always-protected collection.

3. Recursive Browser-Side Stripping: Refactored RemoveProtectedReports
in HidService to recursively traverse the collection tree and strip
protected reports before exposing the device to the renderer.

4. Side-Effect Prevention: Cloned the HidDeviceInfo at the start of
RemoveProtectedReports to perform all security queries against the
unmutated original state, ensuring that stripping a child collection
does not cause its parent TLC to bypass checks.

5. Feature Flag: Gated changes behind a platform-agnostic
kWebHidRecursiveFiltering feature flag (enabled by default).

6. Tests: Added comprehensive unit tests in content_unittests and
services_unittests verifying the recursive blocking.

Bug: 522791354
Test: content_unittests --gtest_filter="*HidService*"
Test: services_unittests --gtest_filter="*HidConnection*:*HidBlocklist*"
Change-Id: If7849de56a8b9e25267af94792531f7b846a16a2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7987617
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Commit-Queue: Rob Pitkin <robpitkin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1663315}
---

diff --git a/content/browser/BUILD.gn b/content/browser/BUILD.gn
index 70ff8fa..8fa0d5c 100644
--- a/content/browser/BUILD.gn
+++ b/content/browser/BUILD.gn
@@ -274,6 +274,7 @@
     "//services/device/public/cpp/compute_pressure:buildflags",
     "//services/device/public/cpp/generic_sensor",
     "//services/device/public/cpp/geolocation",
+    "//services/device/public/cpp/hid",
     "//services/device/public/mojom",
     "//services/device/public/mojom:device_service",
     "//services/device/public/mojom:generic_sensor",
diff --git a/content/browser/hid/hid_service.cc b/content/browser/hid/hid_service.cc
index 9b5e15f..fe153f8 100644
--- a/content/browser/hid/hid_service.cc
+++ b/content/browser/hid/hid_service.cc
@@ -25,6 +25,7 @@
 #include "mojo/public/cpp/bindings/message.h"
 #include "mojo/public/cpp/bindings/self_owned_receiver.h"
 #include "services/device/public/cpp/device_features.h"
+#include "services/device/public/cpp/hid/hid_report_utils.h"
 #include "services/network/public/mojom/permissions_policy/permissions_policy_feature.mojom.h"
 
 namespace content {
@@ -206,6 +207,100 @@
       std::move(receiver));
 }
 
+namespace {
+
+// Recursively filters reports from the collection tree. Returns true if the
+// collection is now empty (has no reports and no active children) and should
+// be pruned by its parent.
+bool FilterCollectionReports(
+    device::mojom::HidCollectionInfo& collection,
+    const device::mojom::HidCollectionInfo& original_collection,
+    const std::optional<std::vector<uint8_t>>& protected_input_report_ids,
+    const std::optional<std::vector<uint8_t>>& protected_output_report_ids,
+    const std::optional<std::vector<uint8_t>>& protected_feature_report_ids,
+    bool is_fido_allowed) {
+  // Recursively filter children first.
+  std::vector<device::mojom::HidCollectionInfoPtr> children;
+  CHECK_EQ(collection.children.size(), original_collection.children.size());
+  for (size_t i = 0; i < collection.children.size(); ++i) {
+    if (!FilterCollectionReports(
+            *collection.children[i], *original_collection.children[i],
+            protected_input_report_ids, protected_output_report_ids,
+            protected_feature_report_ids, is_fido_allowed)) {
+      children.push_back(std::move(collection.children[i]));
+    }
+  }
+  collection.children = std::move(children);
+
+  // Filter input reports.
+  std::vector<device::mojom::HidReportDescriptionPtr> input_reports;
+  for (auto& report : collection.input_reports) {
+    const bool is_fido = device::HasReportInCollectionWithUsagePage(
+        original_collection, report->report_id, device::HidReportType::kInput,
+        device::mojom::kPageFido);
+    const bool is_always_protected =
+        device::HasReportInAlwaysProtectedCollection(
+            original_collection, report->report_id,
+            device::HidReportType::kInput);
+    if ((is_fido && is_fido_allowed) ||
+        (!is_always_protected &&
+         (!protected_input_report_ids.has_value() ||
+          !std::ranges::contains(*protected_input_report_ids,
+                                 report->report_id)))) {
+      input_reports.push_back(std::move(report));
+    }
+  }
+  collection.input_reports = std::move(input_reports);
+
+  // Filter output reports.
+  std::vector<device::mojom::HidReportDescriptionPtr> output_reports;
+  for (auto& report : collection.output_reports) {
+    const bool is_fido = device::HasReportInCollectionWithUsagePage(
+        original_collection, report->report_id, device::HidReportType::kOutput,
+        device::mojom::kPageFido);
+    const bool is_always_protected =
+        device::HasReportInAlwaysProtectedCollection(
+            original_collection, report->report_id,
+            device::HidReportType::kOutput);
+    if ((is_fido && is_fido_allowed) ||
+        (!is_always_protected &&
+         (!protected_output_report_ids.has_value() ||
+          !std::ranges::contains(*protected_output_report_ids,
+                                 report->report_id)))) {
+      output_reports.push_back(std::move(report));
+    }
+  }
+  collection.output_reports = std::move(output_reports);
+
+  // Filter feature reports.
+  std::vector<device::mojom::HidReportDescriptionPtr> feature_reports;
+  for (auto& report : collection.feature_reports) {
+    const bool is_fido = device::HasReportInCollectionWithUsagePage(
+        original_collection, report->report_id, device::HidReportType::kFeature,
+        device::mojom::kPageFido);
+    const bool is_always_protected =
+        device::HasReportInAlwaysProtectedCollection(
+            original_collection, report->report_id,
+            device::HidReportType::kFeature);
+    if ((is_fido && is_fido_allowed) ||
+        (!is_always_protected &&
+         (!protected_feature_report_ids.has_value() ||
+          !std::ranges::contains(*protected_feature_report_ids,
+                                 report->report_id)))) {
+      feature_reports.push_back(std::move(report));
+    }
+  }
+  collection.feature_reports = std::move(feature_reports);
+
+  // Return true if this collection is now empty and should be pruned by its
+  // parent.
+  return collection.input_reports.empty() &&
+         collection.output_reports.empty() &&
+         collection.feature_reports.empty() && collection.children.empty();
+}
+
+}  // namespace
+
 // static
 void HidService::RemoveProtectedReports(device::mojom::HidDeviceInfo& device,
                                         bool is_known_security_key,
@@ -219,44 +314,60 @@
     return;
   }
 #endif  // !BUILDFLAG(IS_ANDROID)
+
   std::vector<device::mojom::HidCollectionInfoPtr> collections;
-  for (auto& collection : device.collections) {
-    const bool is_fido =
-        collection->usage->usage_page == device::mojom::kPageFido;
-    std::vector<device::mojom::HidReportDescriptionPtr> input_reports;
-    for (auto& report : collection->input_reports) {
-      if ((is_fido && is_fido_allowed) ||
-          !device.protected_input_report_ids.has_value() ||
-          !std::ranges::contains(*device.protected_input_report_ids,
-                                 report->report_id)) {
-        input_reports.push_back(std::move(report));
+  if (base::FeatureList::IsEnabled(features::kWebHidRecursiveFiltering)) {
+    // Clone to preserve original state for queries.
+    auto original_device = device.Clone();
+    for (size_t i = 0; i < device.collections.size(); ++i) {
+      if (!FilterCollectionReports(
+              *device.collections[i], *original_device->collections[i],
+              original_device->protected_input_report_ids,
+              original_device->protected_output_report_ids,
+              original_device->protected_feature_report_ids, is_fido_allowed)) {
+        collections.push_back(std::move(device.collections[i]));
       }
     }
-    std::vector<device::mojom::HidReportDescriptionPtr> output_reports;
-    for (auto& report : collection->output_reports) {

diff --git a/content/browser/hid/hid_service_unittest.cc b/content/browser/hid/hid_service_unittest.cc
index a3bcd5b..8c2ba32 100644
--- a/content/browser/hid/hid_service_unittest.cc
+++ b/content/browser/hid/hid_service_unittest.cc
@@ -200,6 +200,18 @@
         device::TestReportDescriptors::FidoU2fHid());
   }
 
+  device::mojom::HidDeviceInfoPtr CreateNestedFidoDevice() {
+    return device::CreateDeviceFromReportDescriptor(
+        /*vendor_id=*/0x1234, /*product_id=*/0xabcd,
+        device::TestReportDescriptors::VendorWithNestedFido());
+  }
+
+  device::mojom::HidDeviceInfoPtr CreateNestedKeyboardDevice() {
+    return device::CreateDeviceFromReportDescriptor(
+        /*vendor_id=*/0x1234, /*product_id=*/0xabcd,
+        device::TestReportDescriptors::VendorWithNestedKeyboard());
+  }
+
   device::mojom::HidDeviceInfoPtr CreateTitanFidoDevice() {
     return device::CreateDeviceFromReportDescriptor(
         kVendorGoogle, kProductTitan,
@@ -353,7 +365,8 @@
  public:
   void SetUp() override {
     scoped_feature_list_.InitWithFeatures(
-        /*enabled_features=*/{features::kSecurityKeyHidInterfacesAreFido},
+        /*enabled_features=*/{features::kSecurityKeyHidInterfacesAreFido,
+                              features::kWebHidRecursiveFiltering},
         /*disabled_features=*/{});
   }
 
@@ -1376,6 +1389,147 @@
   }
 }
 
+TEST_P(HidServiceFidoTest, NestedFidoDeviceAllowedWithPrivilegedOrigin) {
+  auto service_creation_type = std::get<0>(GetParam());
+  const auto& service = GetService(service_creation_type);
+  const bool is_fido_allowed = std::get<1>(GetParam());
+
+  url::Origin origin = url::Origin::Create(GURL(kTestUrl));
+  EXPECT_CALL(hid_delegate(), IsFidoAllowedForOrigin(_, origin))
+      .WillRepeatedly(Return(is_fido_allowed));
+  EXPECT_CALL(hid_delegate(), HasDevicePermission).WillRepeatedly(Return(true));
+
+  TestFuture<std::vector<device::mojom::HidDeviceInfoPtr>> get_devices_future;
+  service->GetDevices(get_devices_future.GetCallback());
+  EXPECT_TRUE(get_devices_future.Get().empty());
+
+  auto device_info = CreateNestedFidoDevice();
+  ASSERT_EQ(device_info->collections.size(), 1u);
+  EXPECT_EQ(device_info->collections[0]->usage->usage_page,
+            device::mojom::kPageVendor);
+  ASSERT_EQ(device_info->collections[0]->children.size(), 1u);
+  EXPECT_EQ(device_info->collections[0]->children[0]->usage->usage_page,
+            device::mojom::kPageFido);
+
+  TestFuture<device::mojom::HidDeviceInfoPtr> device_added_future;
+  if (is_fido_allowed) {
+    EXPECT_CALL(hid_manager_client(), DeviceAdded)
+        .WillOnce(InvokeFuture(device_added_future));
+  } else {
+    EXPECT_CALL(hid_manager_client(), DeviceAdded).Times(0);
+  }
+  ConnectDevice(*device_info);
+  if (is_fido_allowed) {
+    const auto& d = *device_added_future.Get();
+    ASSERT_EQ(d.collections.size(), 1u);
+    ASSERT_EQ(d.collections[0]->children.size(), 1u);
+    EXPECT_EQ(d.collections[0]->children[0]->input_reports.size(), 1u);
+    EXPECT_EQ(d.collections[0]->children[0]->output_reports.size(), 1u);
+  } else {
+    FlushHidServicePipe(service_);
+  }
+
+  TestFuture<device::mojom::HidDeviceInfoPtr> device_changed_future;
+  EXPECT_CALL(hid_manager_client(), DeviceChanged)
+      .WillOnce(InvokeFuture(device_changed_future));
+
+  auto joystick = device::mojom::HidCollectionInfo::New();
+  joystick->usage = device::mojom::HidUsageAndPage::New(
+      device::mojom::kGenericDesktopJoystick,
+      device::mojom::kPageGenericDesktop);
+  joystick->collection_type = device::mojom::kHIDCollectionTypeApplication;
+  joystick->feature_reports.push_back(
+      device::mojom::HidReportDescription::New());
+
+  auto updated_device_info = device_info.Clone();
+  updated_device_info->collections.push_back(std::move(joystick));
+  UpdateDevice(*updated_device_info);
+  const auto& changed_d = *device_changed_future.Get();
+  if (is_fido_allowed) {
+    ASSERT_EQ(changed_d.collections.size(), 2u);
+    EXPECT_EQ(changed_d.collections[0]->usage->usage_page,
+              device::mojom::kPageVendor);
+    ASSERT_EQ(changed_d.collections[0]->children.size(), 1u);
+    EXPECT_EQ(changed_d.collections[1]->usage->usage_page,
+              device::mojom::kPageGenericDesktop);
+    EXPECT_EQ(changed_d.collections[1]->usage->usage,
+              device::mojom::kGenericDesktopJoystick);
+  } else {
+    ASSERT_EQ(changed_d.collections.size(), 1u);
+    EXPECT_EQ(changed_d.collections[0]->usage->usage_page,
+              device::mojom::kPageGenericDesktop);
+    EXPECT_EQ(changed_d.collections[0]->usage->usage,
+              device::mojom::kGenericDesktopJoystick);
+  }
+
+  TestFuture<device::mojom::HidDeviceInfoPtr> device_removed_future;
+  EXPECT_CALL(hid_manager_client(), DeviceRemoved)
+      .WillOnce(InvokeFuture(device_removed_future));
+  DisconnectDevice(*updated_device_info);
+  const auto& removed_d = *device_removed_future.Get();
+  if (is_fido_allowed) {
+    EXPECT_EQ(removed_d.collections.size(), 2u);
+  } else {
+    EXPECT_EQ(removed_d.collections.size(), 1u);
+  }
+}
+
+TEST_P(HidServiceTest, NestedKeyboardDeviceBlocked) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kWebHidRecursiveFiltering);
+
+  auto service_creation_type = GetParam();
+  GetService(service_creation_type);
+
+  // Set up global expectations for the delegate.
+  EXPECT_CALL(hid_delegate(), HasDevicePermission).WillRepeatedly(Return(true));
+
+  auto device_info = CreateNestedKeyboardDevice();
+  ASSERT_EQ(device_info->collections.size(), 1u);
+  EXPECT_EQ(device_info->collections[0]->usage->usage_page,
+            device::mojom::kPageVendor);
+  ASSERT_EQ(device_info->collections[0]->children.size(), 1u);
+  EXPECT_EQ(device_info->collections[0]->children[0]->usage->usage_page,
+            device::mojom::kPageGenericDesktop);
+  EXPECT_EQ(device_info->collections[0]->children[0]->usage->usage,
+            device::mojom::kGenericDesktopKeyboard);
+
+  EXPECT_CALL(hid_manager_client(), DeviceAdded).Times(0);
+  ConnectDevice(*device_info);
+  FlushHidServicePipe(service_);
+
+  base::RunLoop device_changed_loop;
+  EXPECT_CALL(hid_manager_client(), DeviceChanged).WillOnce([&](auto d) {
+    EXPECT_EQ(d->collections.size(), 1u);
+    EXPECT_EQ(d->collections[0]->usage->usage_page,
+              device::mojom::kPageGenericDesktop);
+    EXPECT_EQ(d->collections[0]->usage->usage,
+              device::mojom::kGenericDesktopJoystick);
+    device_changed_loop.Quit();
+  });
+
+  auto joystick = device::mojom::HidCollectionInfo::New();
+  joystick->usage = device::mojom::HidUsageAndPage::New(
+      device::mojom::kGenericDesktopJoystick,
+      device::mojom::kPageGenericDesktop);
+  joystick->collection_type = device::mojom::kHIDCollectionTypeApplication;
+  joystick->feature_reports.push_back(
+      device::mojom::HidReportDescription::New());
+
+  auto updated_device_info = device_info.Clone();
+  updated_device_info->collections.push_back(std::move(joystick));
+  UpdateDevice(*updated_device_info);
+  device_changed_loop.Run();
+
+  base::RunLoop device_removed_loop;
+  EXPECT_CALL(hid_manager_client(), DeviceRemoved).WillOnce([&](auto d) {
+    EXPECT_EQ(d->collections.size(), 1u);
+    device_removed_loop.Quit();
+  });
+  DisconnectDevice(*updated_device_info);
+  device_removed_loop.Run();
+}
+
 INSTANTIATE_TEST_SUITE_P(
     HidServiceTests,
     HidServiceTest,
diff --git a/services/device/hid/hid_connection_unittest.cc b/services/device/hid/hid_connection_unittest.cc
index 614ee02..f1ebd3d 100644
--- a/services/device/hid/hid_connection_unittest.cc
+++ b/services/device/hid/hid_connection_unittest.cc
@@ -279,7 +279,10 @@
 class HidConnectionProtectedReportTest : public testing::Test,
                                          HidConnection::Client {
  public:
-  HidConnectionProtectedReportTest() = default;
+  HidConnectionProtectedReportTest() {
+    scoped_feature_list_.InitAndEnableFeature(
+        features::kWebHidRecursiveFiltering);
+  }
   HidConnectionProtectedReportTest(const HidConnectionProtectedReportTest&) =
       delete;
   HidConnectionProtectedReportTest& operator=(
@@ -353,6 +356,7 @@
   scoped_refptr<TestHidConnection> connection_;
   base::test::TestFuture<scoped_refptr<base::RefCountedBytes>, size_t>
       input_report_future_;
+  base::test::ScopedFeatureList scoped_feature_list_;
 };
 
 TEST_F(HidConnectionProtectedReportTest, UnprotectedReadWrite) {
@@ -599,4 +603,91 @@
   EXPECT_TRUE(connection().closed());
 }
 
+TEST_F(HidConnectionProtectedReportTest, FidoReportsInNestedCollectionBlocked) {
+  // Simulate a device with a vendor-defined top-level collection containing a
+  // nested FIDO application collection that defines input and output reports.
+  auto device_info =
+      CreateHidDeviceInfo(TestReportDescriptors::VendorWithNestedFido());
+  ASSERT_TRUE(device_info);
+  ASSERT_EQ(device_info->collections().size(), 1u);
+  EXPECT_EQ(device_info->collections()[0]->usage->usage_page,
+            mojom::kPageVendor);
+  CreateConnection(device_info);
+
+  SetConnectionClient();
+
+  // Simulate an input report from the nested FIDO collection. It should not be
+  // received by the client.
+  auto buffer =
+      base::MakeRefCounted<base::RefCountedBytes>(std::vector<uint8_t>{1});
+  connection().SimulateInputReport(buffer);
+  EXPECT_FALSE(HasNextInputReport());
+
+  // Try to write an output report to the nested FIDO collection. It should be
+  // blocked.
+  TestFuture<bool> write_future;
+  connection().Write(buffer, write_future.GetCallback());
+  EXPECT_FALSE(write_future.Get());
+
+  // Close the connection.
+  connection().Close();
+  EXPECT_TRUE(connection().closed());
+}
+
+TEST_F(HidConnectionProtectedReportTest,
+       AllowFidoReportsAllowsFidoInNestedCollection) {
+  // Simulate a device with a vendor-defined top-level collection containing a
+  // nested FIDO application collection that defines input and output reports.
+  auto device_info =
+      CreateHidDeviceInfo(TestReportDescriptors::VendorWithNestedFido());
+  ASSERT_TRUE(device_info);
+
+  // Simulate a connection from a FIDO-privileged origin.
+  CreateConnection(device_info, /*allow_protected_reports=*/false,
+                   /*allow_fido_reports=*/true);
+
+  // Simulate an input report.
+  TestFuture<bool, scoped_refptr<base::RefCountedBytes>, size_t> read_future;
+  auto buffer =
+      base::MakeRefCounted<base::RefCountedBytes>(std::vector<uint8_t>{1});
+  connection().SimulateInputReport(buffer);
+  connection().Read(read_future.GetCallback());
+  EXPECT_TRUE(read_future.Get<0>());
+
+  // Simulate an output report.
+  TestFuture<bool> write_future;
+  connection().Write(buffer, write_future.GetCallback());
+  EXPECT_TRUE(write_future.Get());
+
+  // Close the connection.
+  connection().Close();
+  EXPECT_TRUE(connection().closed());
+}
+
+TEST_F(HidConnectionProtectedReportTest,
+       KeyboardReportsInNestedCollectionBlocked) {
+  // Simulate a device with a vendor-defined top-level collection containing a
+  // nested keyboard application collection that defines an input report.
+  auto device_info =
+      CreateHidDeviceInfo(TestReportDescriptors::VendorWithNestedKeyboard());
+  ASSERT_TRUE(device_info);
+  ASSERT_EQ(device_info->collections().size(), 1u);
+  EXPECT_EQ(device_info->collections()[0]->usage->usage_page,
+            mojom::kPageVendor);
+  CreateConnection(device_info);
+
+  SetConnectionClient();
+
+  // Simulate an input report from the nested keyboard collection. It should
+  // not be received by the client.
+  auto buffer =
+      base::MakeRefCounted<base::RefCountedBytes>(std::vector<uint8_t>{1});
+  connection().SimulateInputReport(buffer);
+  EXPECT_FALSE(HasNextInputReport());
+
+  // Close the connection.
+  connection().Close();
+  EXPECT_TRUE(connection().closed());
+}
+
 }  // namespace device
diff --git a/services/device/hid/hid_device_info_unittest.cc b/services/device/hid/hid_device_info_unittest.cc
index 05df2c4c..315b7172 100644
--- a/services/device/hid/hid_device_info_unittest.cc
... (truncated)