From 9294fd5f6a460faed3d909209cbee3b16219935c Mon Sep 17 00:00:00 2001 From: Timothy Loh Date: Thu, 4 Feb 2021 03:49:37 +0000 Subject: [PATCH] Explicitly disallow USB mass storage devices in WebUSB In crrev.com/c/2548705, we disabled access to USB mass storage devices to all clients aside from VMs in the USB service. However, as these were formerly accessible from chrome.usb when allowed in policy, we want to push this logic up into the consumers of the USB service. While Blink already disallows claiming mass storage interfaces, this CL ensures we keep the intended behaviour by having the browser process hide mass storage devices from the renderer process entirely. Bug: 1169558 Change-Id: I36a41fda581870bb4131234ae82ca5bedea360d5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2670379 Reviewed-by: Reilly Grant Reviewed-by: Andrey Kosyakov Commit-Queue: Timothy Loh Cr-Commit-Position: refs/heads/master@{#850442} --- .../device/usb/android_usb_browsertest.cc | 3 +- chrome/browser/usb/usb_chooser_context.cc | 36 ++++++++++++++++++- .../usb/usb_chooser_context_unittest.cc | 34 ++++++++++++++++++ .../public/cpp/test/fake_usb_device_info.cc | 22 ++++++++++++ .../public/cpp/test/fake_usb_device_info.h | 8 +++++ 5 files changed, 101 insertions(+), 2 deletions(-) diff --git a/chrome/browser/devtools/device/usb/android_usb_browsertest.cc b/chrome/browser/devtools/device/usb/android_usb_browsertest.cc index 639dd034c51a98..e920973dc3de15 100644 --- a/chrome/browser/devtools/device/usb/android_usb_browsertest.cc +++ b/chrome/browser/devtools/device/usb/android_usb_browsertest.cc @@ -141,7 +141,8 @@ class FakeAndroidUsbDeviceInfo : public FakeUsbDeviceInfo { /*product_id=*/0, kDeviceManufacturer, kDeviceModel, - kDeviceSerial), + kDeviceSerial, + std::vector()), broken_traits_(is_broken) {} bool broken_traits() const { return broken_traits_; } diff --git a/chrome/browser/usb/usb_chooser_context.cc b/chrome/browser/usb/usb_chooser_context.cc index 81a7e7fd443a47..ffb3f5f274074a 100644 --- a/chrome/browser/usb/usb_chooser_context.cc +++ b/chrome/browser/usb/usb_chooser_context.cc @@ -34,6 +34,7 @@ constexpr char kProductIdKey[] = "product-id"; constexpr char kSerialNumberKey[] = "serial-number"; constexpr char kVendorIdKey[] = "vendor-id"; constexpr int kDeviceIdWildcard = -1; +constexpr int kUsbClassMassStorage = 0x08; // Reasons a permission may be closed. These are used in histograms so do not // remove/reorder entries. Only add at the end just before @@ -115,6 +116,28 @@ base::Value DeviceIdsToValue(int vendor_id, int product_id) { return device_value; } +bool IsMassStorageInterface(const device::mojom::UsbInterfaceInfo& interface) { + for (auto& alternate : interface.alternates) { + if (alternate->class_code == kUsbClassMassStorage) + return true; + } + return false; +} + +bool ShouldExposeDevice(const device::mojom::UsbDeviceInfo& device_info) { + // blink::USBDevice::claimInterface() disallows claiming mass storage + // interfaces, but explicitly prevent access in the browser process as + // ChromeOS would allow these interfaces to be claimed. + + for (auto& configuration : device_info.configurations) { + for (auto& interface : configuration->interfaces) { + if (!IsMassStorageInterface(*interface)) + return true; + } + } + return false; +} + } // namespace void UsbChooserContext::DeviceObserver::OnDeviceAdded( @@ -158,7 +181,10 @@ void UsbChooserContext::InitDeviceList( std::vector devices) { for (auto& device_info : devices) { DCHECK(device_info); - devices_.insert(std::make_pair(device_info->guid, std::move(device_info))); + if (ShouldExposeDevice(*device_info)) { + devices_.insert( + std::make_pair(device_info->guid, std::move(device_info))); + } } is_initialized_ = true; @@ -542,6 +568,8 @@ void UsbChooserContext::OnDeviceAdded( DCHECK(device_info); // Update the device list. DCHECK(!base::Contains(devices_, device_info->guid)); + if (!ShouldExposeDevice(*device_info)) + return; devices_.insert(std::make_pair(device_info->guid, device_info->Clone())); // Notify all observers. @@ -552,6 +580,12 @@ void UsbChooserContext::OnDeviceAdded( void UsbChooserContext::OnDeviceRemoved( device::mojom::UsbDeviceInfoPtr device_info) { DCHECK(device_info); + + if (!ShouldExposeDevice(*device_info)) { + DCHECK(!base::Contains(devices_, device_info->guid)); + return; + } + // Update the device list. DCHECK(base::Contains(devices_, device_info->guid)); devices_.erase(device_info->guid); diff --git a/chrome/browser/usb/usb_chooser_context_unittest.cc b/chrome/browser/usb/usb_chooser_context_unittest.cc index 01fc46ca7528b7..a70f56784117f4 100644 --- a/chrome/browser/usb/usb_chooser_context_unittest.cc +++ b/chrome/browser/usb/usb_chooser_context_unittest.cc @@ -9,6 +9,7 @@ #include "base/no_destructor.h" #include "base/run_loop.h" #include "base/strings/utf_string_conversions.h" +#include "base/test/bind.h" #include "base/values.h" #include "build/chromeos_buildflags.h" #include "chrome/browser/content_settings/host_content_settings_map_factory.h" @@ -1272,3 +1273,36 @@ TEST_F(UsbChooserContextTest, /*product_id=*/1357, /*name=*/"Unknown product 0x054D from vendor 0x18D2"); } + +TEST_F(UsbChooserContextTest, MassStorageHidden) { + GURL url("https://www.google.com"); + const auto origin = url::Origin::Create(url); + + // Mass storage devices should be hidden. + std::vector storage_configs; + storage_configs.push_back( + device::FakeUsbDeviceInfo::CreateConfiguration(0x08, 0x06, 0x50)); + UsbDeviceInfoPtr storage_device_info = device_manager_.CreateAndAddDevice( + 0, 0, "vendor1", "storage", "123ABC", std::move(storage_configs)); + + // Composite devices with both mass storage and allowed interfaces should be + // shown. + std::vector complex_configs; + complex_configs.push_back( + device::FakeUsbDeviceInfo::CreateConfiguration(0x08, 0x06, 0x50, 1)); + complex_configs.push_back( + device::FakeUsbDeviceInfo::CreateConfiguration(0xff, 0x42, 0x1, 2)); + UsbDeviceInfoPtr complex_device_info = device_manager_.CreateAndAddDevice( + 0, 0, "vendor2", "complex", "456DEF", std::move(complex_configs)); + + UsbChooserContext* chooser_context = GetChooserContext(profile()); + + base::RunLoop loop; + chooser_context->GetDevices( + base::BindLambdaForTesting([&](std::vector devices) { + EXPECT_EQ(1u, devices.size()); + EXPECT_EQ(complex_device_info->product_name, devices[0]->product_name); + loop.Quit(); + })); + loop.Run(); +} diff --git a/services/device/public/cpp/test/fake_usb_device_info.cc b/services/device/public/cpp/test/fake_usb_device_info.cc index 950e30b3113b55..82a7fa4be334f1 100644 --- a/services/device/public/cpp/test/fake_usb_device_info.cc +++ b/services/device/public/cpp/test/fake_usb_device_info.cc @@ -44,6 +44,7 @@ FakeUsbDeviceInfo::FakeUsbDeviceInfo(uint16_t usb_version, device_info_.manufacturer_name = base::UTF8ToUTF16(manufacturer_string); device_info_.product_name = base::UTF8ToUTF16(product_string); device_info_.serial_number = base::UTF8ToUTF16(serial_number); + AddConfig(CreateConfiguration(0xFF, 0x00, 0x00)); } FakeUsbDeviceInfo::FakeUsbDeviceInfo(uint16_t vendor_id, @@ -135,4 +136,25 @@ void FakeUsbDeviceInfo::NotifyDeviceRemoved() { observer.OnDeviceRemoved(this); } +mojom::UsbConfigurationInfoPtr FakeUsbDeviceInfo::CreateConfiguration( + uint8_t device_class, + uint8_t subclass_code, + uint8_t protocol_code, + uint8_t configuration_value) { + auto alternate = device::mojom::UsbAlternateInterfaceInfo::New(); + alternate->alternate_setting = 0; + alternate->class_code = device_class; + alternate->subclass_code = subclass_code; + alternate->protocol_code = protocol_code; + + auto interface = device::mojom::UsbInterfaceInfo::New(); + interface->interface_number = 0; + interface->alternates.push_back(std::move(alternate)); + + auto config = device::mojom::UsbConfigurationInfo::New(); + config->configuration_value = configuration_value; + config->interfaces.push_back(std::move(interface)); + return config; +} + } // namespace device diff --git a/services/device/public/cpp/test/fake_usb_device_info.h b/services/device/public/cpp/test/fake_usb_device_info.h index 9dcc9731967ee5..e737268a9b6d9c 100644 --- a/services/device/public/cpp/test/fake_usb_device_info.h +++ b/services/device/public/cpp/test/fake_usb_device_info.h @@ -22,6 +22,8 @@ namespace device { // This class acts like device::UsbDevice and provides mojom::UsbDeviceInfo. // It should be used together with FakeUsbDeviceManager just for testing. +// For convenience, a default configuration will be set if the chosen +// constructor does not explicitly set these. class FakeUsbDeviceInfo : public base::RefCounted { public: class Observer : public base::CheckedObserver { @@ -77,6 +79,12 @@ class FakeUsbDeviceInfo : public base::RefCounted { void SetMockDevice(MockUsbMojoDevice* device) { mock_device_ = device; } MockUsbMojoDevice* mock_device() const { return mock_device_; } + static mojom::UsbConfigurationInfoPtr CreateConfiguration( + uint8_t device_class, + uint8_t subclass_code, + uint8_t protocol_code, + uint8_t configuration_value = 1); + protected: friend class RefCounted; virtual ~FakeUsbDeviceInfo();