Track allowed interfaces for sharing USB devices to guests.

Some interfaces on multi-interface USB devices should be blocked from
sharing with guests. The mechanism for this is a mask parameter sent to
PermissionBroker for use when it opens the raw USB fd for the guest to use.
The mask is a "drop privileges" mask that can only be used to narrow the
privileges given to the fd.

Change-Id: Ia37726384666a1b24e336d1e8b4ec66117b94384
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2196885
Commit-Queue: Nicholas Verne <nverne@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Fergus Dall <sidereal@google.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: David Munro <davidmunro@google.com>
Cr-Commit-Position: refs/heads/master@{#772143}

chrome/browser/chromeos/usb/cros_usb_detector.cc

@@ -33,6 +33,8 @@ namespace chromeos {
 
 namespace {
 
+constexpr uint32_t kAllInterfacesMask = ~0U;
+
 // Not owned locally.
 static CrosUsbDetector* g_cros_usb_detector = nullptr;
 
@@ -58,6 +60,68 @@ base::string16 ProductLabelFromDevice(
   return product_label;
 }
 
+uint32_t ClearMatchingInterfaces(
+    uint32_t in_mask,
+    const device::mojom::UsbDeviceFilter& filter,
+    const device::mojom::UsbDeviceInfo& device_info) {
+  uint32_t mask = in_mask;
+
+  for (auto& config : device_info.configurations) {
+    for (auto& iface : config->interfaces) {
+      for (auto& alternate_info : iface->alternates) {
+        if (filter.has_class_code &&
+            alternate_info->class_code != filter.class_code) {
+          continue;
+        }
+        if (filter.has_subclass_code &&
+            alternate_info->subclass_code != filter.subclass_code) {
+          continue;
+        }
+        if (filter.has_protocol_code &&
+            alternate_info->protocol_code != filter.protocol_code) {
+          continue;
+        }
+        if (iface->interface_number >= 32) {
+          LOG(ERROR) << "Interface number too high in USB descriptor";
+          continue;
+        }
+        mask &= ~(1U << iface->interface_number);
+      }
+    }
+  }
+
+  return mask;
+}
+
+uint32_t GetUsbInterfaceBaseMask(
+    const device::mojom::UsbDeviceInfo& device_info) {
+  if (device_info.configurations.empty()) {
+    // No specific interfaces to clear.
+    return kAllInterfacesMask;
+  }
+  uint32_t mask = 0;
+  for (auto& config : device_info.configurations) {
+    for (auto& iface : config->interfaces) {
+      if (iface->interface_number >= 32) {
+        LOG(ERROR) << "Interface number too high in USB descriptor.";
+        continue;
+      }
+      mask |= (1U << iface->interface_number);
+    }
+  }
+  return mask;
+}
+
+uint32_t GetFilteredInterfacesMask(
+    const std::vector<device::mojom::UsbDeviceFilterPtr>& filters,
+    const device::mojom::UsbDeviceInfo& device_info) {
+  uint32_t mask = GetUsbInterfaceBaseMask(device_info);
+  for (const auto& filter : filters) {
+    mask = ClearMatchingInterfaces(mask, *filter, device_info);
+  }
+  return mask;
+}
+
 Profile* profile() {
   return ProfileManager::GetActiveUserProfile();
 }
@@ -301,10 +365,15 @@ bool CrosUsbDetector::ShouldShowNotification(
   }
   if (device::UsbDeviceFilterMatches(*adb_device_filter_, device_info) ||
       device::UsbDeviceFilterMatches(*fastboot_device_filter_, device_info)) {
+    VLOG(1) << "Adb or fastboot device found";
+    return true;
+  }
+  if (!device::UsbDeviceFilterMatchesAny(guest_os_classes_without_notif_,
+                                         device_info)) {
+    VLOG(1) << "Only notifiable interfaces found for device";
     return true;
   }
-  return !device::UsbDeviceFilterMatchesAny(guest_os_classes_without_notif_,
-                                            device_info);
+  return false;
 }
 
 void CrosUsbDetector::OnDeviceChecked(
@@ -324,11 +393,13 @@ void CrosUsbDetector::OnDeviceChecked(
   const bool has_supported_interface =
       device::UsbDeviceFilterMatches(*adb_device_filter_, *device_info) ||
       device::UsbDeviceFilterMatches(*fastboot_device_filter_, *device_info);
-  const bool has_blocked_interface = device::UsbDeviceFilterMatchesAny(
-      guest_os_classes_blocked_, *device_info);
+
+  new_device.allowed_interfaces_mask =
+      GetFilteredInterfacesMask(guest_os_classes_blocked_, *device_info);
+
   new_device.sharable_with_crostini =
       has_supported_interface ||
-      (!has_blocked_interface &&
+      (new_device.allowed_interfaces_mask != 0 &&
        base::FeatureList::IsEnabled(features::kCrostiniUsbAllowUnsupported));
 
   usb_devices_.push_back(new_device);
@@ -338,6 +409,7 @@ void CrosUsbDetector::OnDeviceChecked(
   // Some devices should not trigger the notification.
   if (!new_device.sharable_with_crostini || hide_notification ||
       !ShouldShowNotification(*device_info)) {
+    VLOG(1) << "Not showing USB notification for " << new_device.label;
     return;
   }
   ShowNotificationForDevice(std::move(device_info));
@@ -401,6 +473,7 @@ void CrosUsbDetector::AttachUsbDeviceToVm(
     const std::string& vm_name,
     const std::string& guid,
     base::OnceCallback<void(bool success)> callback) {
+  uint32_t allowed_interfaces_mask = 0;
   for (auto& device : usb_devices_) {
     if (device.guid == guid) {
       // Mark the USB device shared so that we know to reattach it on VM
@@ -408,6 +481,7 @@ void CrosUsbDetector::AttachUsbDeviceToVm(
       // Setting this flag early also allows the UI not to flicker because of
       // the notification resulting from the default VM detach below.
       device.vm_sharing_info[vm_name].shared = true;
+      allowed_interfaces_mask = device.allowed_interfaces_mask;
       // The guest port will be set on completion.
       break;
     }
@@ -421,11 +495,15 @@ void CrosUsbDetector::AttachUsbDeviceToVm(
   // Close any associated notifications (the user isn't using them).
   SystemNotificationHelper::GetInstance()->Close(
       CrosUsbDetector::MakeNotificationId(guid));
+
+  VLOG(1) << "Opening " << std::hex << guid << " with mask "
+          << allowed_interfaces_mask;
   // Open a file descriptor to pass to CrostiniManager & Concierge.
   device_manager_->OpenFileDescriptor(
-      guid, base::BindOnce(&CrosUsbDetector::OnAttachUsbDeviceOpened,
-                           weak_ptr_factory_.GetWeakPtr(), vm_name,
-                           device_info.Clone(), std::move(callback)));
+      guid, allowed_interfaces_mask,
+      base::BindOnce(&CrosUsbDetector::OnAttachUsbDeviceOpened,
+                     weak_ptr_factory_.GetWeakPtr(), vm_name,
+                     device_info.Clone(), std::move(callback)));
 }
 
 void CrosUsbDetector::DetachUsbDeviceFromVm(

chrome/browser/chromeos/usb/cros_usb_detector.h

@@ -66,8 +66,10 @@ struct CrosUsbDeviceInfo {
   base::flat_map<std::string, VmSharingInfo> vm_sharing_info;
   std::string guid;
   base::string16 label;
-  // Whether the device can be shared with Crostini.
+  // Whether the device can be shared with guest OSes.
   bool sharable_with_crostini = false;
+  // Interfaces shareable with guest OSes
+  uint32_t allowed_interfaces_mask = 0;
   // TODO(nverne): Add current state and errors etc.
 };
 

chrome/browser/chromeos/usb/cros_usb_detector_unittest.cc

@@ -769,3 +769,25 @@ TEST_F(CrosUsbDetectorTest, SharedDevicesGetAttachedOnStartup) {
   EXPECT_EQ(0, usb_device_observer_.notify_count());
   EXPECT_TRUE(IsSharedWithCrostini(GetSingleDeviceInfo()));
 }
+
+TEST_F(CrosUsbDetectorTest, DeviceAllowedInterfacesMaskSetCorrectly) {
+  ConnectToDeviceManager();
+  base::RunLoop().RunUntilIdle();
+
+  const int kAdbClass = 0xff;
+  const int kAdbSubclass = 0x42;
+  const int kAdbProtocol = 0x1;
+
+  // Adb interface as well as a forbidden interface and allowed interface.
+  scoped_refptr<device::FakeUsbDeviceInfo> device = CreateTestDeviceFromCodes(
+      /* USB_CLASS_HID */ 0x03,
+      {InterfaceCodes(0x03, 0xff, 0xff),
+       InterfaceCodes(kAdbClass, kAdbSubclass, kAdbProtocol),
+       InterfaceCodes(/*USB_CLASS_AUDIO*/ 0x01, 0xff, 0xff)});
+
+  device_manager_.AddDevice(device);
+  base::RunLoop().RunUntilIdle();
+  auto device_info = GetSingleDeviceInfo();
+
+  EXPECT_EQ(0x00000006U, device_info.allowed_interfaces_mask);
+}

chromeos/dbus/permission_broker/fake_permission_broker_client.cc

@@ -84,6 +84,14 @@ void FakePermissionBrokerClient::OpenPath(const std::string& path,
                      base::ThreadTaskRunnerHandle::Get()));
 }
 
+void FakePermissionBrokerClient::OpenPathWithDroppedPrivileges(
+    const std::string& path,
+    uint32_t allowed_interfaces_mask,
+    OpenPathCallback callback,
+    ErrorCallback error_callback) {
+  OpenPath(path, std::move(callback), std::move(error_callback));
+}
+
 void FakePermissionBrokerClient::RequestTcpPortAccess(
     uint16_t port,
     const std::string& interface,

chromeos/dbus/permission_broker/fake_permission_broker_client.h

@@ -31,6 +31,10 @@ class COMPONENT_EXPORT(PERMISSION_BROKER) FakePermissionBrokerClient
   void OpenPath(const std::string& path,
                 OpenPathCallback callback,
                 ErrorCallback error_callback) override;
+  void OpenPathWithDroppedPrivileges(const std::string& path,
+                                     uint32_t allowed_interfaces_mask,
+                                     OpenPathCallback callback,
+                                     ErrorCallback error_callback) override;
   void RequestTcpPortAccess(uint16_t port,
                             const std::string& interface,
                             int lifeline_fd,

chromeos/dbus/permission_broker/permission_broker_client.cc

@@ -18,6 +18,7 @@
 
 using permission_broker::kCheckPathAccess;
 using permission_broker::kOpenPath;
+using permission_broker::kOpenPathWithDroppedPrivileges;
 using permission_broker::kPermissionBrokerInterface;
 using permission_broker::kPermissionBrokerServiceName;
 using permission_broker::kPermissionBrokerServicePath;
@@ -71,6 +72,24 @@ class PermissionBrokerClientImpl : public PermissionBrokerClient {
                        std::move(error_callback)));
   }
 
+  void OpenPathWithDroppedPrivileges(const std::string& path,
+                                     uint32_t allowed_interfaces_mask,
+                                     OpenPathCallback callback,
+                                     ErrorCallback error_callback) override {
+    dbus::MethodCall method_call(kPermissionBrokerInterface,
+                                 kOpenPathWithDroppedPrivileges);
+    dbus::MessageWriter writer(&method_call);
+    writer.AppendString(path);
+    writer.AppendUint32(allowed_interfaces_mask);
+    proxy_->CallMethodWithErrorCallback(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::BindOnce(&PermissionBrokerClientImpl::OnOpenPathResponse,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)),
+        base::BindOnce(&PermissionBrokerClientImpl::OnError,
+                       weak_ptr_factory_.GetWeakPtr(),
+                       std::move(error_callback)));
+  }
+
   void RequestTcpPortAccess(uint16_t port,
                             const std::string& interface,
                             int lifeline_fd,

chromeos/dbus/permission_broker/permission_broker_client.h

@@ -67,6 +67,19 @@ class COMPONENT_EXPORT(PERMISSION_BROKER) PermissionBrokerClient {
                         OpenPathCallback callback,
                         ErrorCallback error_callback) = 0;
 
+  // OpenPathWithDroppedPrivileges requests that the permission broker open
+  // the device node identified by |path| and set of USB interfaces that can be
+  // claimed |allowed_interfaces_mask|, returning the resulting file descriptor.
+  // The interface number 0 corresponds to the LSB of |allowed_interfaces_mask|.
+  // A device which has an ADB interface and other interfaces for Camera or
+  // Storage may be opened purely as an ADB device using a mask that zeros out
+  // the Camera and Storage interface number bit positions.
+  // One of |callback| or |error_callback| is called.
+  virtual void OpenPathWithDroppedPrivileges(const std::string& path,
+                                             uint32_t allowed_interfaces_mask,
+                                             OpenPathCallback callback,
+                                             ErrorCallback error_callback) = 0;
+
   // Requests the |port| be opened on the firewall for incoming TCP/IP
   // connections received on |interface| (an empty string indicates all
   // interfaces). One end of an open pipe must be passed as |lifeline_fd| so

services/device/public/cpp/test/fake_usb_device_manager.cc

@@ -82,6 +82,7 @@ void FakeUsbDeviceManager::CheckAccess(const std::string& guid,
 
 void FakeUsbDeviceManager::OpenFileDescriptor(
     const std::string& guid,
+    uint32_t drop_privileges_mask,
     OpenFileDescriptorCallback callback) {
   std::move(callback).Run(base::File(
       base::FilePath(FILE_PATH_LITERAL("/dev/null")),

services/device/public/cpp/test/fake_usb_device_manager.h

@@ -85,6 +85,7 @@ class FakeUsbDeviceManager : public mojom::UsbDeviceManager {
                    CheckAccessCallback callback) override;
 
   void OpenFileDescriptor(const std::string& guid,
+                          uint32_t drop_privileges_mask,
                           OpenFileDescriptorCallback callback) override;
 #endif  // defined(OS_CHROMEOS)
 

services/device/public/mojom/usb_manager.mojom

@@ -37,9 +37,11 @@ interface UsbDeviceManager {
   CheckAccess(string guid) => (bool success);
 
   // Attempt to open a USB device using permission_broker and return
-  // a file descriptor.
+  // a file descriptor. Allow access only to interfaces specified in
+  // |allowed_interfaces_mask|.
   [EnableIf=is_chromeos]
-  OpenFileDescriptor(string guid) => (mojo_base.mojom.File? fd);
+  OpenFileDescriptor(string guid, uint32 allowed_interfaces_mask)
+      => (mojo_base.mojom.File? fd);
 
   // Sets the client for this DeviceManager service. The service will notify its
   // client of device events such as connection and disconnection.

services/device/usb/mojo/device_manager_impl.cc

@@ -123,6 +123,7 @@ void DeviceManagerImpl::CheckAccess(const std::string& guid,
 
 void DeviceManagerImpl::OpenFileDescriptor(
     const std::string& guid,
+    uint32_t drop_privileges_mask,
     OpenFileDescriptorCallback callback) {
   scoped_refptr<UsbDevice> device = usb_service_->GetDevice(guid);
   if (!device) {
@@ -133,8 +134,8 @@ void DeviceManagerImpl::OpenFileDescriptor(
         base::AdaptCallbackForRepeating(std::move(callback));
     auto devpath =
         static_cast<device::UsbDeviceLinux*>(device.get())->device_path();
-    chromeos::PermissionBrokerClient::Get()->OpenPath(
-        devpath,
+    chromeos::PermissionBrokerClient::Get()->OpenPathWithDroppedPrivileges(
+        devpath, drop_privileges_mask,
         base::BindOnce(&DeviceManagerImpl::OnOpenFileDescriptor,
                        weak_factory_.GetWeakPtr(), copyable_callback),
         base::BindOnce(&DeviceManagerImpl::OnOpenFileDescriptorError,

services/device/usb/mojo/device_manager_impl.h

@@ -70,6 +70,7 @@ class DeviceManagerImpl : public mojom::UsbDeviceManager,
                    CheckAccessCallback callback) override;
 
   void OpenFileDescriptor(const std::string& guid,
+                          uint32_t drop_privileges_mask,
                           OpenFileDescriptorCallback callback) override;
 
   void OnOpenFileDescriptor(OpenFileDescriptorCallback callback,