serial: Use USB driver name to disambiguate ports

On systems with both the built-in serial driver and a third-party driver
the same port can be enumerated more than once. This difference is
visible to the user in the device path shown in the chooser but was
previously ignored when constructing a persistent ID for the port.

This patch replaces the generic persistent ID field with the individual
components that are available on each platform. This should make it
easier to modify these rules over time as the components won't be opaque
to SerialChooserContext. This is a breaking change which will cause
previously granted persistent permissions to be ignored.

On macOS the name of the USB driver (the parent of the BSD serial
port) is now included in the set of fields used to identify a device in
order to differentiate between USB devices with otherwise have the same
USB vendor ID, product ID and serial number.

Bug: 1127535
Change-Id: If1b06ab82a975976e1e3b7d2075216f11afda5dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2413176
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: James Hollyer <jameshollyer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811028}

chrome/browser/serial/serial_chooser_context.cc

@@ -10,6 +10,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
+#include "build/build_config.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/serial/serial_chooser_histograms.h"
@@ -19,8 +20,17 @@
 namespace {
 
 constexpr char kPortNameKey[] = "name";
-constexpr char kPersistentIdKey[] = "persistent_id";
 constexpr char kTokenKey[] = "token";
+#if defined(OS_WIN)
+constexpr char kDeviceInstanceIdKey[] = "device_instance_id";
+#else
+constexpr char kVendorIdKey[] = "vendor_id";
+constexpr char kProductIdKey[] = "product_id";
+constexpr char kSerialNumberKey[] = "serial_number";
+#if defined(OS_MAC)
+constexpr char kUsbDriverKey[] = "usb_driver";
+#endif  // defined(OS_MAC)
+#endif  // defined(OS_WIN)
 
 std::string EncodeToken(const base::UnguessableToken& token) {
   const uint64_t data[2] = {token.GetHighForSerialization(),
@@ -49,10 +59,29 @@ base::Value PortInfoToValue(const device::mojom::SerialPortInfo& port) {
     value.SetStringKey(kPortNameKey, *port.display_name);
   else
     value.SetStringKey(kPortNameKey, port.path.LossyDisplayName());
-  if (SerialChooserContext::CanStorePersistentEntry(port))
-    value.SetStringKey(kPersistentIdKey, port.persistent_id.value());
-  else
+
+  if (!SerialChooserContext::CanStorePersistentEntry(port)) {
     value.SetStringKey(kTokenKey, EncodeToken(port.token));
+    return value;
+  }
+
+#if defined(OS_WIN)
+  // Windows provides a handy device identifier which we can rely on to be
+  // sufficiently stable for identifying devices across restarts.
+  value.SetStringKey(kDeviceInstanceIdKey, port.device_instance_id);
+#else
+  DCHECK(port.has_vendor_id);
+  value.SetIntKey(kVendorIdKey, port.vendor_id);
+  DCHECK(port.has_product_id);
+  value.SetIntKey(kProductIdKey, port.product_id);
+  DCHECK(port.serial_number);
+  value.SetStringKey(kSerialNumberKey, *port.serial_number);
+
+#if defined(OS_MAC)
+  DCHECK(port.usb_driver_name && !port.usb_driver_name->empty());
+  value.SetStringKey(kUsbDriverKey, *port.usb_driver_name);
+#endif  // defined(OS_MAC)
+#endif  // defined(OS_WIN)
   return value;
 }
 
@@ -71,11 +100,26 @@ SerialChooserContext::SerialChooserContext(Profile* profile)
 SerialChooserContext::~SerialChooserContext() = default;
 
 bool SerialChooserContext::IsValidObject(const base::Value& object) {
+  if (!object.is_dict() || !object.FindStringKey(kPortNameKey))
+    return false;
+
   const std::string* token = object.FindStringKey(kTokenKey);
-  return object.is_dict() && object.DictSize() == 2 &&
-         object.FindStringKey(kPortNameKey) &&
-         ((token && DecodeToken(*token)) ||
-          object.FindStringKey(kPersistentIdKey));
+  if (token)
+    return object.DictSize() == 2 && DecodeToken(*token);
+
+#if defined(OS_WIN)
+  return object.DictSize() == 2 && object.FindStringKey(kDeviceInstanceIdKey);
+#else
+  if (!object.FindIntKey(kVendorIdKey) || !object.FindIntKey(kProductIdKey) ||
+      !object.FindStringKey(kSerialNumberKey)) {
+    return false;
+  }
+#if defined(OS_MAC)
+  return object.DictSize() == 5 && object.FindStringKey(kUsbDriverKey);
+#else
+  return object.DictSize() == 4;
+#endif  // defined(OS_MAC)
+#endif  // defined(OS_WIN)
 }
 
 base::string16 SerialChooserContext::GetObjectDisplayName(
@@ -193,15 +237,46 @@ bool SerialChooserContext::HasPortPermission(
       return true;
   }
 
+  if (!CanStorePersistentEntry(port)) {
+    return false;
+  }
+
   std::vector<std::unique_ptr<permissions::ChooserContextBase::Object>>
       object_list = GetGrantedObjects(requesting_origin, embedding_origin);
   for (const auto& object : object_list) {
     const base::Value& device = object->value;
+
+    // This check guarantees that the keys referenced below will be found.
     DCHECK(IsValidObject(device));
 
-    const std::string* persistent_id = device.FindStringKey(kPersistentIdKey);
-    if (port.persistent_id == *persistent_id)
+#if defined(OS_WIN)
+    const std::string& device_instance_id =
+        *device.FindStringKey(kDeviceInstanceIdKey);
+    if (port.device_instance_id == device_instance_id)
       return true;
+#else
+    const int vendor_id = *device.FindIntKey(kVendorIdKey);
+    const int product_id = *device.FindIntKey(kProductIdKey);
+    const std::string& serial_number = *device.FindStringKey(kSerialNumberKey);
+
+    // Guaranteed by the CanStorePersistentEntry() check above.
+    DCHECK(port.has_vendor_id);
+    DCHECK(port.has_product_id);
+    DCHECK(port.serial_number && !port.serial_number->empty());
+    if (port.vendor_id != vendor_id || port.product_id != product_id ||
+        port.serial_number != serial_number) {
+      continue;
+    }
+
+#if defined(OS_MAC)
+    const std::string& usb_driver_name = *device.FindStringKey(kUsbDriverKey);
+    if (port.usb_driver_name != usb_driver_name) {
+      continue;
+    }
+#endif  // defined(OS_MAC)
+
+    return true;
+#endif  // defined(OS_WIN)
   }
   return false;
 }
@@ -216,7 +291,28 @@ bool SerialChooserContext::CanStorePersistentEntry(
   if (!port.display_name || port.display_name->empty())
     return false;
 
-  return port.persistent_id && !port.persistent_id->empty();
+#if defined(OS_WIN)
+  return !port.device_instance_id.empty();
+#else
+  if (!port.has_vendor_id || !port.has_product_id || !port.serial_number ||
+      port.serial_number->empty()) {
+    return false;
+  }
+
+#if defined(OS_MAC)
+  // The combination of the standard USB vendor ID, product ID and serial
+  // number properties should be enough to uniquely identify a device
+  // however recent versions of macOS include built-in drivers for common
+  // types of USB-to-serial adapters while their manufacturers still
+  // recommend installing their custom drivers. When both are loaded two
+  // IOSerialBSDClient instances are found for each device. Including the
+  // USB driver name allows us to distinguish between the two.
+  if (!port.usb_driver_name || port.usb_driver_name->empty())
+    return false;
+#endif  // defined(OS_MAC)
+
+  return true;
+#endif  // defined(OS_WIN)
 }
 
 device::mojom::SerialPortManager* SerialChooserContext::GetPortManager() {

chrome/browser/serial/serial_chooser_context_unittest.cc

@@ -8,6 +8,7 @@
 #include "base/run_loop.h"
 #include "base/scoped_observer.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "build/build_config.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/serial/serial_chooser_context_factory.h"
 #include "chrome/browser/serial/serial_chooser_histograms.h"
@@ -36,6 +37,27 @@ class MockPortObserver : public SerialChooserContext::PortObserver {
   MOCK_METHOD0(OnPortManagerConnectionError, void());
 };
 
+device::mojom::SerialPortInfoPtr CreatePersistentPort(
+    base::Optional<std::string> name,
+    const std::string& persistent_id) {
+  auto port = device::mojom::SerialPortInfo::New();
+  port->token = base::UnguessableToken::Create();
+  port->display_name = std::move(name);
+#if defined(OS_WIN)
+  port->device_instance_id = persistent_id;
+#else
+  port->has_vendor_id = true;
+  port->vendor_id = 0;
+  port->has_product_id = true;
+  port->product_id = 0;
+  port->serial_number = persistent_id;
+#if defined(OS_MAC)
+  port->usb_driver_name = "AppleUSBCDC";
+#endif
+#endif  // defined(OS_WIN)
+  return port;
+}
+
 class SerialChooserContextTest : public testing::Test {
  public:
   SerialChooserContextTest() {
@@ -139,10 +161,8 @@ TEST_F(SerialChooserContextTest, GrantAndRevokePersistentPermission) {
 
   const auto origin = url::Origin::Create(GURL("https://google.com"));
 
-  auto port = device::mojom::SerialPortInfo::New();
-  port->token = base::UnguessableToken::Create();
-  port->display_name = "Persistent Port";
-  port->persistent_id = "ABC123";
+  device::mojom::SerialPortInfoPtr port =
+      CreatePersistentPort("Persistent Port", "ABC123");
 
   EXPECT_FALSE(context()->HasPortPermission(origin, origin, *port));
 
@@ -229,10 +249,8 @@ TEST_F(SerialChooserContextTest, EphemeralPermissionRevokedOnDisconnect) {
 TEST_F(SerialChooserContextTest, PersistenceRequiresDisplayName) {
   const auto origin = url::Origin::Create(GURL("https://google.com"));
 
-  auto port = device::mojom::SerialPortInfo::New();
-  port->token = base::UnguessableToken::Create();
-  // port->display_name is left unset.
-  port->persistent_id = "ABC123";
+  device::mojom::SerialPortInfoPtr port =
+      CreatePersistentPort(/*name=*/base::nullopt, "ABC123");
   port_manager().AddPort(port.Clone());
 
   context()->GrantPortPermission(origin, origin, *port);
@@ -269,10 +287,8 @@ TEST_F(SerialChooserContextTest, PersistentPermissionNotRevokedOnDisconnect) {
   const auto origin = url::Origin::Create(GURL("https://google.com"));
   const char persistent_id[] = "ABC123";
 
-  auto port = device::mojom::SerialPortInfo::New();
-  port->token = base::UnguessableToken::Create();
-  port->display_name = "Persistent Port";
-  port->persistent_id = persistent_id;
+  device::mojom::SerialPortInfoPtr port =
+      CreatePersistentPort("Persistent Port", persistent_id);
   port_manager().AddPort(port.Clone());
 
   context()->GrantPortPermission(origin, origin, *port);
@@ -307,9 +323,7 @@ TEST_F(SerialChooserContextTest, PersistentPermissionNotRevokedOnDisconnect) {
 
   // Simulate reconnection of the port. It gets a new token but the same
   // persistent ID. This SerialPortInfo should still match the old permission.
-  port = device::mojom::SerialPortInfo::New();
-  port->token = base::UnguessableToken::Create();
-  port->persistent_id = persistent_id;
+  port = CreatePersistentPort("Persistent Port", persistent_id);
   port_manager().AddPort(port.Clone());
 
   EXPECT_TRUE(context()->HasPortPermission(origin, origin, *port));

services/device/public/mojom/serial.mojom

@@ -9,11 +9,6 @@ import "mojo/public/mojom/base/unguessable_token.mojom";
 
 struct SerialPortInfo {
   mojo_base.mojom.UnguessableToken token;
-
-  // This platform-specific identifier, if present, can be used to identify the
-  // device across restarts of the application and operating system.
-  string? persistent_id;
-
   mojo_base.mojom.FilePath path;
 
   // This member is used to identify whether the SerialPortInfo object is
@@ -26,11 +21,27 @@ struct SerialPortInfo {
   // the dial-in device is the only option its path will be in |path|.
   [EnableIf=is_mac] mojo_base.mojom.FilePath? alternate_path;
 
+  // On macOS a single serial port can be enumerated by multiple drivers. This
+  // field permits disambiguation.
+  [EnableIf=is_mac] string? usb_driver_name;
+
+  // On Windows the "device instance ID" provides a stable identifier that can
+  // be used for device permissions.
+  [EnableIf=is_win] string device_instance_id;
+
+  // The USB device vendor and product IDs.
   uint16 vendor_id;
   bool has_vendor_id = false;
   uint16 product_id;
   bool has_product_id = false;
+
+  // A string suitable for display to the user for describing this device. May
+  // be, for example, the USB device product name string.
   string? display_name;
+
+  // The USB device serial number.
+  // Note: This field is not populated on Windows nor for non-USB devices.
+  string? serial_number;
 };
 
 enum SerialSendError {

services/device/serial/serial_device_enumerator_linux.cc

@@ -165,31 +165,30 @@ void SerialDeviceEnumeratorLinux::CreatePort(ScopedUdevDevicePtr device,
   info->path = base::FilePath(path);
   info->token = token;
 
+  uint32_t int_value;
   const char* vendor_id =
       udev_device_get_property_value(device.get(), "ID_VENDOR_ID");
-  const char* product_id =
-      udev_device_get_property_value(device.get(), "ID_MODEL_ID");
-  const char* product_name_enc =
-      udev_device_get_property_value(device.get(), "ID_MODEL_ENC");
-  const char* serial_number =
-      udev_device_get_property_value(device.get(), "ID_SERIAL_SHORT");
-
-  uint32_t int_value;
   if (vendor_id && base::HexStringToUInt(vendor_id, &int_value)) {
     info->vendor_id = int_value;
     info->has_vendor_id = true;
   }
+
+  const char* product_id =
+      udev_device_get_property_value(device.get(), "ID_MODEL_ID");
   if (product_id && base::HexStringToUInt(product_id, &int_value)) {
     info->product_id = int_value;
     info->has_product_id = true;
   }
+
+  const char* product_name_enc =
+      udev_device_get_property_value(device.get(), "ID_MODEL_ENC");
   if (product_name_enc)
     info->display_name = device::UdevDecodeString(product_name_enc);
 
-  if (info->has_vendor_id && info->has_product_id && serial_number) {
-    info->persistent_id = base::StringPrintf("%04X-%04X-%s", info->vendor_id,
-                                             info->product_id, serial_number);
-  }
+  const char* serial_number =
+      udev_device_get_property_value(device.get(), "ID_SERIAL_SHORT");
+  if (serial_number)
+    info->serial_number = serial_number;
 
   paths_.insert(std::make_pair(syspath, token));
   AddPort(std::move(info));

services/device/serial/serial_device_enumerator_linux_unittest.cc

@@ -67,7 +67,7 @@ TEST_F(SerialDeviceEnumeratorLinuxTest, Enumerate) {
   std::unique_ptr<SerialDeviceEnumeratorLinux> enumerator = CreateEnumerator();
   std::vector<mojom::SerialPortInfoPtr> devices = enumerator->GetDevices();
   ASSERT_EQ(devices.size(), 1u);
-  EXPECT_EQ(devices[0]->persistent_id, "2341-0043-000001");
+  EXPECT_EQ(devices[0]->serial_number, "000001");
   EXPECT_EQ(devices[0]->path, base::FilePath("/dev/ttyACM0"));
   EXPECT_TRUE(devices[0]->has_vendor_id);
   EXPECT_EQ(devices[0]->vendor_id, 0x2341);

services/device/serial/serial_device_enumerator_mac.cc

@@ -93,6 +93,33 @@ base::Optional<uint16_t> GetUInt16Property(io_service_t service,
   return base::nullopt;
 }
 
+// Finds the name of the USB driver for |device| by walking up the
+// IORegistry tree to find the first entry provided by the IOUSBInterface
+// class. For drivers compiled for macOS 10.11 and later this was renamed
+// to IOUSBHostInterface.
+base::Optional<std::string> GetUsbDriverName(
+    base::mac::ScopedIOObject<io_object_t> device) {
+  base::mac::ScopedIOObject<io_iterator_t> iterator;
+  kern_return_t kr = IORegistryEntryCreateIterator(
+      device.get(), kIOServicePlane,
+      kIORegistryIterateRecursively | kIORegistryIterateParents,
+      iterator.InitializeInto());
+  if (kr != KERN_SUCCESS)
+    return base::nullopt;
+
+  base::mac::ScopedIOObject<io_service_t> ancestor;
+  while (ancestor.reset(IOIteratorNext(iterator)), ancestor) {
+    base::Optional<std::string> provider_class =
+        GetStringProperty(ancestor.get(), CFSTR(kIOProviderClassKey));
+    if (provider_class && (*provider_class == "IOUSBInterface" ||
+                           *provider_class == "IOUSBHostInterface")) {
+      return GetStringProperty(ancestor.get(), kCFBundleIdentifierKey);
+    }
+  }
+
+  return base::nullopt;
+}
+
 }  // namespace
 
 SerialDeviceEnumeratorMac::SerialDeviceEnumeratorMac() {
@@ -159,27 +186,27 @@ void SerialDeviceEnumeratorMac::AddDevices() {
     auto info = mojom::SerialPortInfo::New();
     base::Optional<uint16_t> vendor_id =
         GetUInt16Property(device.get(), CFSTR(kUSBVendorID));
+    base::Optional<std::string> vendor_id_str;
     if (vendor_id) {
       info->has_vendor_id = true;
       info->vendor_id = *vendor_id;
+      vendor_id_str = base::StringPrintf("%04X", *vendor_id);
     }
 
     base::Optional<uint16_t> product_id =
         GetUInt16Property(device.get(), CFSTR(kUSBProductID));
+    base::Optional<std::string> product_id_str;
     if (product_id) {
       info->has_product_id = true;
       info->product_id = *product_id;
+      product_id_str = base::StringPrintf("%04X", *product_id);
     }
 
     info->display_name =
         GetStringProperty(device.get(), CFSTR(kUSBProductString));
-
-    base::Optional<std::string> serial_number =
+    info->serial_number =
         GetStringProperty(device.get(), CFSTR(kUSBSerialNumberString));
-    if (vendor_id && product_id && serial_number) {
-      info->persistent_id = base::StringPrintf(
-          "%04X-%04X-%s", *vendor_id, *product_id, serial_number->c_str());
-    }
+    info->usb_driver_name = GetUsbDriverName(device);
 
     // Each serial device has two paths associated with it: a "dialin" path
     // starting with "tty" and a "callout" path starting with "cu". The
@@ -203,6 +230,14 @@ void SerialDeviceEnumeratorMac::AddDevices() {
     auto token = base::UnguessableToken::Create();
     info->token = token;
 
+    VLOG(1) << "Found serial device: dialin="
+            << dialin_device.value_or("(none)")
+            << " callout=" << callout_device.value_or("(none)")
+            << " vid=" << vendor_id_str.value_or("(none)")
+            << " pid=" << product_id_str.value_or("(none)")
+            << " usb_serial=" << info->serial_number.value_or("(none)")
+            << " usb_driver=" << info->usb_driver_name.value_or("(none)");
+
     entries_.insert({entry_id, token});
     AddPort(std::move(info));
   }

services/device/serial/serial_device_enumerator_win.cc

@@ -286,11 +286,17 @@ void SerialDeviceEnumeratorWin::EnumeratePort(HDEVINFO dev_info,
   if (!instance_id)
     return;
 
+  // Some versions of Windows pad this string with a variable number of NUL
+  // bytes for no discernible reason.
+  instance_id = base::TrimString(*instance_id, base::StringPiece("\0", 1),
+                                 base::TRIM_TRAILING)
+                    .as_string();
+
   base::UnguessableToken token = base::UnguessableToken::Create();
   auto info = mojom::SerialPortInfo::New();
   info->token = token;
   info->path = *path;
-  info->persistent_id = instance_id;
+  info->device_instance_id = *instance_id;
 
   // TODO(https://crbug.com/1015074): Read the real USB strings here.
   std::string display_name;
@@ -299,15 +305,23 @@ void SerialDeviceEnumeratorWin::EnumeratePort(HDEVINFO dev_info,
 
   // The instance ID looks like "FTDIBUS\VID_0403+PID_6001+A703X87GA\0000".
   uint32_t vendor_id, product_id;
+  base::Optional<std::string> vendor_id_str, product_id_str;
   if (GetVendorID(*instance_id, &vendor_id)) {
     info->has_vendor_id = true;
     info->vendor_id = vendor_id;
+    vendor_id_str = base::StringPrintf("%04X", vendor_id);
   }
   if (GetProductID(*instance_id, &product_id)) {
     info->has_product_id = true;
     info->product_id = product_id;
+    product_id_str = base::StringPrintf("%04X", product_id);
   }
 
+  VLOG(1) << "Found serial device: path=" << info->path
+          << " instance_id=" << info->device_instance_id
+          << " vid=" << vendor_id_str.value_or("(none)")
+          << " pid=" << product_id_str.value_or("(none)");
+
   paths_.insert(std::make_pair(*path, token));
   AddPort(std::move(info));
 }