Do not connect to Cable devices on FidoBleDiscovery

Make sure that FidoBleDiscovery stores list of all Cable devices it
found and never attempt to connect to Cable devices.

Bug: 880295
Change-Id: I3799103da15b9314a8f8bdb57e0f1d42f7a99a28
Reviewed-on: https://chromium-review.googlesource.com/1214182
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590536}

device/bluetooth/test/bluetooth_test.cc

@@ -4,6 +4,7 @@
 
 #include "device/bluetooth/test/bluetooth_test.h"
 
+#include <iterator>
 #include <memory>
 
 #include "base/bind.h"
@@ -28,6 +29,7 @@ const char BluetoothTestBase::kTestAdapterAddress[] = "A1:B2:C3:D4:E5:F6";
 const char BluetoothTestBase::kTestDeviceName[] = "FakeBluetoothDevice";
 const char BluetoothTestBase::kTestDeviceNameEmpty[] = "";
 const char BluetoothTestBase::kTestDeviceNameU2f[] = "U2F FakeDevice";
+const char BluetoothTestBase::kTestDeviceNameCable[] = "Cable FakeDevice";
 
 const char BluetoothTestBase::kTestDeviceAddress1[] = "01:00:00:90:1E:BE";
 const char BluetoothTestBase::kTestDeviceAddress2[] = "02:00:00:8B:74:63";
@@ -66,8 +68,15 @@ const char BluetoothTestBase::kTestUUIDServerCharacteristicConfiguration[] =
     "00002903-0000-1000-8000-00805f9b34fb";
 const char BluetoothTestBase::kTestUUIDCharacteristicPresentationFormat[] =
     "00002904-0000-1000-8000-00805f9b34fb";
+const char BluetoothTestBase::kTestUUIDCableAdvertisement[] =
+    "0000fde2-0000-1000-8000-00805f9b34fb";
 // Manufacturer kTestAdapterAddress
 const uint16_t BluetoothTestBase::kTestManufacturerId = 0x00E0;
+const uint8_t BluetoothTestBase::kTestCableEid[] = {
+    0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+    0x08, 0x09, 0x10, 0x11, 0x12, 0x13, 0x14, 0x15};
+const char BluetoothTestBase::kTestUuidFormattedClientEid[] =
+    "00010203-0405-0607-0809-101112131415";
 
 BluetoothTestBase::BluetoothTestBase() : weak_factory_(this) {}
 
@@ -546,6 +555,25 @@ BluetoothTestBase::GetLowEnergyDeviceData(int device_ordinal) const {
       device_data.service_data = {
           {BluetoothUUID(kTestUUIDU2fControlPointLength), {0, 20}}};
       break;
+    case 8:
+      device_data.name = kTestDeviceNameCable;
+      device_data.address = kTestDeviceAddress1;
+      device_data.flags = 0x07;
+      device_data.rssi = static_cast<int>(TestRSSI::LOWEST);
+      device_data.service_data = {
+          {BluetoothUUID(kTestUUIDCableAdvertisement),
+           std::vector<uint8_t>(std::begin(kTestCableEid),
+                                std::end(kTestCableEid))}};
+      break;
+    case 9:
+      device_data.name = kTestDeviceNameCable;
+      device_data.address = kTestDeviceAddress2;
+      device_data.flags = 0x07;
+      device_data.rssi = static_cast<int>(TestRSSI::LOWEST);
+      device_data.advertised_uuids = {
+          BluetoothUUID(kTestUUIDCableAdvertisement),
+          BluetoothUUID(kTestUuidFormattedClientEid)};
+      break;
     default:
       NOTREACHED();
   }

device/bluetooth/test/bluetooth_test.h

@@ -88,6 +88,7 @@ class BluetoothTestBase : public testing::Test {
   static const char kTestDeviceName[];
   static const char kTestDeviceNameEmpty[];
   static const char kTestDeviceNameU2f[];
+  static const char kTestDeviceNameCable[];
 
   static const char kTestDeviceAddress1[];
   static const char kTestDeviceAddress2[];
@@ -129,8 +130,12 @@ class BluetoothTestBase : public testing::Test {
   static const char kTestUUIDClientCharacteristicConfiguration[];
   static const char kTestUUIDServerCharacteristicConfiguration[];
   static const char kTestUUIDCharacteristicPresentationFormat[];
+  static const char kTestUUIDCableAdvertisement[];
   // Manufacturer data
   static const uint16_t kTestManufacturerId;
+  // Test ephemeral ID for BLE devices that support cloud-assisted BLE protocol.
+  static const uint8_t kTestCableEid[];
+  static const char kTestUuidFormattedClientEid[];
 
   BluetoothTestBase();
   ~BluetoothTestBase() override;
@@ -249,6 +254,23 @@ class BluetoothTestBase : public testing::Test {
   //      Service Data:     {kTestUUIDU2fControlPointLength: [0, 20]}
   //      No Manufacturer Data
   //      No Tx Power
+  //   8: Name: kTestDeviceNameCable;
+  //      Address: kTestDeviceAddress1;
+  //      Flags: 0x07;
+  //      RSSI: static_cast<int>(TestRSSI::LOWEST);
+  //      Advertised UUIDs: {BluetoothUUID(kTestUUIDU2f)};
+  //      Service Data: {
+  //                      {BluetoothUUID(kTestUUIDCableAdvertisement128),
+  //                       std::vector<uint8_t>(std::begin(kTestCableEid),
+  //                                            std::end(kTestCableEid))}};
+  //   9: Name: kTestDeviceNameCable;
+  //      Address: kTestDeviceAddress2;
+  //      Flags: = 0x07;
+  //      RSSI: static_cast<int>(TestRSSI::LOWEST);
+  //      Advertised UUIDs: {
+  //                          BluetoothUUID(kTestUUIDCableAdvertisement16),
+  //                          BluetoothUUID(kTestUuidFormattedClientEid)};
+
   virtual BluetoothDevice* SimulateLowEnergyDevice(int device_ordinal);
 
   // Simulates a connected low energy device. Used before starting a low energy

device/fido/ble/fido_ble_discovery.cc

@@ -34,7 +34,8 @@ void FidoBleDiscovery::OnSetPowered() {
   VLOG(2) << "Adapter " << adapter()->GetAddress() << " is powered on.";
 
   for (BluetoothDevice* device : adapter()->GetDevices()) {
-    if (base::ContainsKey(device->GetUUIDs(), FidoServiceUUID())) {
+    if (!CheckForExcludedDeviceAndCacheAddress(device) &&
+        base::ContainsKey(device->GetUUIDs(), FidoServiceUUID())) {
       VLOG(2) << "U2F BLE device: " << device->GetAddress();
       AddDevice(
           std::make_unique<FidoBleDevice>(adapter(), device->GetAddress()));
@@ -57,7 +58,8 @@ void FidoBleDiscovery::OnSetPowered() {
 
 void FidoBleDiscovery::DeviceAdded(BluetoothAdapter* adapter,
                                    BluetoothDevice* device) {
-  if (base::ContainsKey(device->GetUUIDs(), FidoServiceUUID())) {
+  if (!CheckForExcludedDeviceAndCacheAddress(device) &&
+      base::ContainsKey(device->GetUUIDs(), FidoServiceUUID())) {
     VLOG(2) << "Discovered U2F BLE device: " << device->GetAddress();
     AddDevice(std::make_unique<FidoBleDevice>(adapter, device->GetAddress()));
   }
@@ -65,7 +67,8 @@ void FidoBleDiscovery::DeviceAdded(BluetoothAdapter* adapter,
 
 void FidoBleDiscovery::DeviceChanged(BluetoothAdapter* adapter,
                                      BluetoothDevice* device) {
-  if (base::ContainsKey(device->GetUUIDs(), FidoServiceUUID()) &&
+  if (!CheckForExcludedDeviceAndCacheAddress(device) &&
+      base::ContainsKey(device->GetUUIDs(), FidoServiceUUID()) &&
       !GetDevice(FidoBleDevice::GetId(device->GetAddress()))) {
     VLOG(2) << "Discovered U2F service on existing BLE device: "
             << device->GetAddress();
@@ -90,4 +93,26 @@ void FidoBleDiscovery::AdapterPoweredChanged(BluetoothAdapter* adapter,
     OnSetPowered();
 }
 
+bool FidoBleDiscovery::CheckForExcludedDeviceAndCacheAddress(
+    const BluetoothDevice* device) {
+  std::string device_address = device->GetAddress();
+  auto address_position =
+      excluded_cable_device_addresses_.lower_bound(device_address);
+  if (address_position != excluded_cable_device_addresses_.end() &&
+      *address_position == device_address) {
+    return true;
+  }
+
+  // IsCableDevice() is not stable, and can change throughout the lifetime. As
+  // so, cache device address for known Cable devices so that we do not attempt
+  // to connect to these devices.
+  if (IsCableDevice(device)) {
+    excluded_cable_device_addresses_.insert(address_position,
+                                            std::move(device_address));
+    return true;
+  }
+
+  return false;
+}
+
 }  // namespace device

device/fido/ble/fido_ble_discovery.h

@@ -6,6 +6,8 @@
 #define DEVICE_FIDO_BLE_FIDO_BLE_DISCOVERY_H_
 
 #include <memory>
+#include <set>
+#include <string>
 
 #include "base/component_export.h"
 #include "base/macros.h"
@@ -37,6 +39,11 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoBleDiscovery
                      BluetoothDevice* device) override;
   void AdapterPoweredChanged(BluetoothAdapter* adapter, bool powered) override;
 
+  // Returns true if |device| is a Cable device. If so, add address of |device|
+  // to |blacklisted_cable_device_addresses_|.
+  bool CheckForExcludedDeviceAndCacheAddress(const BluetoothDevice* device);
+
+  std::set<std::string> excluded_cable_device_addresses_;
   base::WeakPtrFactory<FidoBleDiscovery> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(FidoBleDiscovery);

device/fido/ble/fido_ble_discovery_base.cc

@@ -9,11 +9,13 @@
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/location.h"
+#include "base/no_destructor.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "device/bluetooth/bluetooth_adapter_factory.h"
 #include "device/bluetooth/bluetooth_common.h"
 #include "device/bluetooth/bluetooth_discovery_session.h"
+#include "device/fido/ble/fido_ble_uuids.h"
 
 namespace device {
 
@@ -27,6 +29,13 @@ FidoBleDiscoveryBase::~FidoBleDiscoveryBase() {
   // Destroying |discovery_session_| will best-effort-stop discovering.
 }
 
+// static
+const BluetoothUUID& FidoBleDiscoveryBase::CableAdvertisementUUID() {
+  static const base::NoDestructor<BluetoothUUID> service_uuid(
+      kCableAdvertisementUUID128);
+  return *service_uuid;
+}
+
 void FidoBleDiscoveryBase::OnStartDiscoverySessionWithFilter(
     std::unique_ptr<BluetoothDiscoverySession> session) {
   SetDiscoverySession(std::move(session));
@@ -49,6 +58,12 @@ void FidoBleDiscoveryBase::SetDiscoverySession(
   discovery_session_ = std::move(discovery_session);
 }
 
+bool FidoBleDiscoveryBase::IsCableDevice(const BluetoothDevice* device) const {
+  const auto& uuid = CableAdvertisementUUID();
+  return base::ContainsKey(device->GetServiceData(), uuid) ||
+         base::ContainsKey(device->GetUUIDs(), uuid);
+}
+
 void FidoBleDiscoveryBase::OnGetAdapter(
     scoped_refptr<BluetoothAdapter> adapter) {
   if (!adapter->IsPresent()) {

device/fido/ble/fido_ble_discovery_base.h

@@ -12,6 +12,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "device/bluetooth/bluetooth_adapter.h"
+#include "device/bluetooth/bluetooth_uuid.h"
 #include "device/fido/fido_discovery.h"
 
 namespace device {
@@ -26,6 +27,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoBleDiscoveryBase
   ~FidoBleDiscoveryBase() override;
 
  protected:
+  static const BluetoothUUID& CableAdvertisementUUID();
+
   virtual void OnSetPowered() = 0;
   virtual void OnStartDiscoverySessionWithFilter(
       std::unique_ptr<BluetoothDiscoverySession>);
@@ -34,6 +37,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoBleDiscoveryBase
   void OnStartDiscoverySessionError();
   void SetDiscoverySession(
       std::unique_ptr<BluetoothDiscoverySession> discovery_session);
+  bool IsCableDevice(const BluetoothDevice* device) const;
 
   BluetoothAdapter* adapter() { return adapter_.get(); }
 

device/fido/ble/fido_ble_discovery_unittest.cc

@@ -198,4 +198,38 @@ TEST_F(BluetoothTest, FidoBleDiscoveryFindsUpdatedDevice) {
   }
 }
 
+TEST_F(BluetoothTest, FidoBleDiscoveryRejectsCableDevice) {
+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+  InitWithFakeAdapter();
+
+  FidoBleDiscovery discovery;
+  MockFidoDiscoveryObserver observer;
+  discovery.set_observer(&observer);
+
+  {
+    base::RunLoop run_loop;
+    auto quit = run_loop.QuitClosure();
+    EXPECT_CALL(observer, DiscoveryStarted(&discovery, true))
+        .WillOnce(ReturnFromAsyncCall(quit));
+
+    discovery.Start();
+    run_loop.Run();
+  }
+
+  EXPECT_CALL(observer, DeviceAdded(&discovery, _)).Times(0);
+
+  // Simulates a discovery of two Cable devices one of which is an Android Cable
+  // authenticator and other is IOS Cable authenticator.
+  SimulateLowEnergyDevice(8);
+  SimulateLowEnergyDevice(9);
+
+  // Simulates a device change update received from the BluetoothAdapter. As the
+  // updated device has an address that we know is an Cable device, this should
+  // not trigger DeviceAdded().
+  SimulateLowEnergyDevice(7);
+}
+
 }  // namespace device

device/fido/cable/fido_cable_discovery.cc

@@ -27,22 +27,6 @@ namespace device {
 
 namespace {
 
-const BluetoothUUID& CableAdvertisementUUID128() {
-  static const BluetoothUUID service_uuid(kCableAdvertisementUUID128);
-  return service_uuid;
-}
-
-const BluetoothUUID& CableAdvertisementUUID16() {
-  static const BluetoothUUID service_uuid(kCableAdvertisementUUID16);
-  return service_uuid;
-}
-
-bool IsCableDevice(const BluetoothDevice* device) {
-  return base::ContainsKey(device->GetServiceData(),
-                           CableAdvertisementUUID128()) ||
-         base::ContainsKey(device->GetUUIDs(), CableAdvertisementUUID16());
-}
-
 // Construct advertisement data with different formats depending on client's
 // operating system. Ideally, we advertise EIDs as part of Service Data, but
 // this isn't available on all platforms. On Windows we use Manufacturer Data
@@ -364,7 +348,7 @@ const CableDiscoveryData*
 FidoCableDiscovery::GetFoundCableDiscoveryDataFromServiceData(
     const BluetoothDevice* device) const {
   const auto* service_data =
-      device->GetServiceDataForUUID(CableAdvertisementUUID128());
+      device->GetServiceDataForUUID(CableAdvertisementUUID());
   if (!service_data) {
     return nullptr;
   }
@@ -397,10 +381,9 @@ FidoCableDiscovery::GetFoundCableDiscoveryDataFromServiceUUIDs(
     const BluetoothDevice* device) const {
   const auto service_uuids = device->GetUUIDs();
   for (const auto& uuid : service_uuids) {
-    if (uuid == CableAdvertisementUUID128() ||
-        uuid == CableAdvertisementUUID16()) {
+    if (uuid == CableAdvertisementUUID())
       continue;
-    }
+
     auto discovery_data_iterator = std::find_if(
         discovery_data_.begin(), discovery_data_.end(),
         [&uuid](const auto& data) {