usb: block attempts to use Android phones as security keys.

In the future, Android phones will likely expose their security key
functionality over USB. This will be accessed by using the AOA[1]
protocol to communicate with a specific app on the phone. A request for
security key functionality is expressed by sending a magic string as the
AOA "version".

We don't wish to allow WebUSB, the USB extension API, or future users of
the USB Mojo interface to be able to exercise this. Thus this change
blocks such control transfers by default, unless the caller explicitly
indicates that they expect to be sending security key messages.

[1] https://source.android.com/devices/accessories/aoa

BUG=1002262

Change-Id: I785610ee58e319327950200b7dfb230099cadf6b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2462379Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816559}

chrome/test/data/webui/usb_internals_browsertest.js

@@ -71,6 +71,7 @@ UsbInternalsTest.prototype = {
         super([
           'enumerateDevicesAndSetClient',
           'getDevice',
+          'getSecurityKeyDevice',
           'getDevices',
           'checkAccess',
           'openFileDescriptor',
@@ -106,6 +107,8 @@ UsbInternalsTest.prototype = {
         deviceRemote.router.$.bindHandle(devicePendingReceiver.handle);
       }
 
+      async getSecurityKeyDevice(guid, devicePendingReceiver, deviceClient) {}
+
       async getDevices() {
         this.methodCalled('getDevices');
         return {results: this.devices};

device/fido/aoa/android_accessory_discovery.cc

@@ -58,9 +58,9 @@ void AndroidAccessoryDiscovery::OnDeviceAdded(
   }
 
   mojo::Remote<device::mojom::UsbDevice> device;
-  device_manager_->GetDevice(device_info->guid,
-                             device.BindNewPipeAndPassReceiver(),
-                             mojo::NullRemote() /* device_client */);
+  device_manager_->GetSecurityKeyDevice(device_info->guid,
+                                        device.BindNewPipeAndPassReceiver(),
+                                        /*device_client=*/mojo::NullRemote());
 
   auto* device_ptr = device.get();
   if (device_info->vendor_id == 0x18d1 &&
@@ -369,7 +369,9 @@ void AndroidAccessoryDiscovery::OnConfigurationStepComplete(
   } else if (step == kNumStrings) {
     device_ptr->ControlTransferOut(
         ControlTransferParams(kSendString, step),
-        VectorFromString(kCableOverAOAVersion), kTimeoutMilliseconds,
+        VectorFromString(
+            device::mojom::UsbControlTransferParams::kSecurityKeyAOAVersion),
+        kTimeoutMilliseconds,
         base::BindOnce(&AndroidAccessoryDiscovery::OnConfigurationStepComplete,
                        weak_factory_.GetWeakPtr(), std::move(device),
                        step + 1));
@@ -404,8 +406,9 @@ void AndroidAccessoryDiscovery::OnGetDevices(
     FIDO_LOG(DEBUG) << "Previously opened accessory device found.";
 
     mojo::Remote<device::mojom::UsbDevice> device;
-    device_manager_->GetDevice(guid, device.BindNewPipeAndPassReceiver(),
-                               mojo::NullRemote() /* device_client */);
+    device_manager_->GetSecurityKeyDevice(guid,
+                                          device.BindNewPipeAndPassReceiver(),
+                                          /*device_client=*/mojo::NullRemote());
 
     HandleAccessoryDevice(std::move(device), std::move(device_info));
   }

device/fido/fido_constants.h

@@ -366,12 +366,6 @@ constexpr uint8_t kP1CheckOnly = 0x07;
 constexpr uint8_t kP1IndividualAttestation = 0x80;
 constexpr size_t kMaxKeyHandleLength = 255;
 
-// kCableOverAOAVersion is a magic value that is sent as the "version" in an
-// Android AOA[1] configuration to identity a security-key request.
-//
-// [1] https://source.android.com/devices/accessories/aoa
-constexpr char kCableOverAOAVersion[] = "12eba9f901039b36";
-
 // kCableWebSocketProtocol is the name of the WebSocket subprotocol used by
 // caBLEv2. See https://tools.ietf.org/html/rfc6455#section-1.9.
 constexpr char kCableWebSocketProtocol[] = "fido.cable";

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

@@ -60,6 +60,13 @@ void FakeUsbDeviceManager::GetDevice(
                         std::move(device_client));
 }
 
+void FakeUsbDeviceManager::GetSecurityKeyDevice(
+    const std::string& guid,
+    mojo::PendingReceiver<device::mojom::UsbDevice> device_receiver,
+    mojo::PendingRemote<mojom::UsbDeviceClient> device_client) {
+  return GetDevice(guid, std::move(device_receiver), std::move(device_client));
+}
+
 #if defined(OS_ANDROID)
 void FakeUsbDeviceManager::RefreshDeviceInfo(
     const std::string& guid,

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

@@ -74,6 +74,10 @@ class FakeUsbDeviceManager : public mojom::UsbDeviceManager {
       const std::string& guid,
       mojo::PendingReceiver<device::mojom::UsbDevice> device_receiver,
       mojo::PendingRemote<mojom::UsbDeviceClient> device_client) override;
+  void GetSecurityKeyDevice(
+      const std::string& guid,
+      mojo::PendingReceiver<device::mojom::UsbDevice> device_receiver,
+      mojo::PendingRemote<mojom::UsbDeviceClient> device_client) override;
 
 #if defined(OS_ANDROID)
   void RefreshDeviceInfo(const std::string& guid,

services/device/public/mojom/usb_device.mojom

@@ -132,6 +132,15 @@ struct UsbControlTransferParams {
   uint8 request;
   uint16 value;
   uint16 index;
+
+  // Unless the USB device was opened with |GetSecurityKeyDevice| then control
+  // transfers to attempt to configure an AOA[1] version with the following
+  // prefix will be rejected. These requests are blocked because they instruct
+  // an Android phone to act as a security key and this should not be exposed
+  // to, e.g., WebUSB.
+  //
+  // [1] https://source.android.com/devices/accessories/aoa
+  const string kSecurityKeyAOAVersion = "12eba9f901039b36";
 };
 
 // This enum is exposed through the chrome.usb extension API so existing values

services/device/public/mojom/usb_manager.mojom

@@ -24,6 +24,13 @@ interface UsbDeviceManager {
   GetDevice(string guid, pending_receiver<UsbDevice> device_receiver,
             pending_remote<UsbDeviceClient>? device_client);
 
+  // Requests a device by guid. The returned device will be permitted to send
+  // control transfers that attempt to configure an AOA[1] version that
+  // indicates a security key request. See comment in
+  // |UsbControlTransferParams|.
+  GetSecurityKeyDevice(string guid, pending_receiver<UsbDevice> device_receiver,
+                       pending_remote<UsbDeviceClient>? device_client);
+
   // Check Android permissions to access the device identified by |guid|. If
   // necessary the user is prompted to grant this access. If access is granted
   // the device information is refreshed to include data not previously

services/device/usb/mojo/device_impl.cc

@@ -84,13 +84,34 @@ void OnIsochronousTransferOut(
   std::move(callback).Run(std::move(packets));
 }
 
+// IsAndroidSecurityKeyRequest returns true if |params| is attempting to
+// configure an Android phone to act as a security key.
+bool IsAndroidSecurityKeyRequest(
+    const mojom::UsbControlTransferParamsPtr& params,
+    const std::vector<uint8_t>& data) {
+  // This matches a request to send an AOA version string:
+  // https://source.android.com/devices/accessories/aoa#attempt-to-start-in-accessory-mode
+  //
+  // The magic version is matched as a prefix because sending trailing NULs etc
+  // would be considered equivalent by Android but would not be caught by an
+  // exact match here. Android is case-sensitive thus a byte-wise match is
+  // suitable.
+  const char* magic = mojom::UsbControlTransferParams::kSecurityKeyAOAVersion;
+  return params->type == mojom::UsbControlTransferType::VENDOR &&
+         params->request == 52 && params->index == 3 &&
+         data.size() >= strlen(magic) &&
+         memcmp(data.data(), magic, strlen(magic)) == 0;
+}
+
 }  // namespace
 
 // static
 void DeviceImpl::Create(scoped_refptr<device::UsbDevice> device,
                         mojo::PendingReceiver<mojom::UsbDevice> receiver,
-                        mojo::PendingRemote<mojom::UsbDeviceClient> client) {
-  auto* device_impl = new DeviceImpl(std::move(device), std::move(client));
+                        mojo::PendingRemote<mojom::UsbDeviceClient> client,
+                        bool allow_security_key_requests) {
+  auto* device_impl = new DeviceImpl(std::move(device), std::move(client),
+                                     allow_security_key_requests);
   device_impl->receiver_ = mojo::MakeSelfOwnedReceiver(
       base::WrapUnique(device_impl), std::move(receiver));
 }
@@ -100,8 +121,12 @@ DeviceImpl::~DeviceImpl() {
 }
 
 DeviceImpl::DeviceImpl(scoped_refptr<device::UsbDevice> device,
-                       mojo::PendingRemote<mojom::UsbDeviceClient> client)
-    : device_(std::move(device)), observer_(this), client_(std::move(client)) {
+                       mojo::PendingRemote<mojom::UsbDeviceClient> client,
+                       bool allow_security_key_requests)
+    : device_(std::move(device)),
+      observer_(this),
+      allow_security_key_requests_(allow_security_key_requests),
+      client_(std::move(client)) {
   DCHECK(device_);
   observer_.Add(device_.get());
 
@@ -313,7 +338,9 @@ void DeviceImpl::ControlTransferOut(UsbControlTransferParamsPtr params,
     return;
   }
 
-  if (HasControlTransferPermission(params->recipient, params->index)) {
+  if (HasControlTransferPermission(params->recipient, params->index) &&
+      (allow_security_key_requests_ ||
+       !IsAndroidSecurityKeyRequest(params, data))) {
     auto buffer = base::MakeRefCounted<base::RefCountedBytes>(data);
     device_handle_->ControlTransfer(
         UsbTransferDirection::OUTBOUND, params->type, params->recipient,

services/device/usb/mojo/device_impl.h

@@ -31,13 +31,15 @@ class DeviceImpl : public mojom::UsbDevice, public device::UsbDevice::Observer {
  public:
   static void Create(scoped_refptr<device::UsbDevice> device,
                      mojo::PendingReceiver<mojom::UsbDevice> receiver,
-                     mojo::PendingRemote<mojom::UsbDeviceClient> client);
+                     mojo::PendingRemote<mojom::UsbDeviceClient> client,
+                     bool allow_security_key_requests);
 
   ~DeviceImpl() override;
 
  private:
   DeviceImpl(scoped_refptr<device::UsbDevice> device,
-             mojo::PendingRemote<mojom::UsbDeviceClient> client);
+             mojo::PendingRemote<mojom::UsbDeviceClient> client,
+             bool allow_security_key_requests);
 
   // Closes the device if it's open. This will always set |device_handle_| to
   // null.
@@ -111,6 +113,7 @@ class DeviceImpl : public mojom::UsbDevice, public device::UsbDevice::Observer {
   bool opening_ = false;
   scoped_refptr<UsbDeviceHandle> device_handle_;
 
+  const bool allow_security_key_requests_;
   mojo::SelfOwnedReceiverRef<mojom::UsbDevice> receiver_;
   mojo::Remote<device::mojom::UsbDeviceClient> client_;
   base::WeakPtrFactory<DeviceImpl> weak_factory_{this};

services/device/usb/mojo/device_impl_unittest.cc

@@ -185,6 +185,7 @@ class USBDeviceImplTest : public testing::Test {
       const std::string& manufacturer,
       const std::string& product,
       const std::string& serial,
+      bool allow_security_key_requests,
       mojo::PendingRemote<mojom::UsbDeviceClient> client) {
     mock_device_ =
         new MockUsbDevice(vendor_id, product_id, manufacturer, product, serial);
@@ -192,7 +193,7 @@ class USBDeviceImplTest : public testing::Test {
 
     mojo::Remote<mojom::UsbDevice> proxy;
     DeviceImpl::Create(mock_device_, proxy.BindNewPipeAndPassReceiver(),
-                       std::move(client));
+                       std::move(client), allow_security_key_requests);
 
     // Set up mock handle calls to respond based on mock device configs
     // established by the test.
@@ -227,9 +228,16 @@ class USBDeviceImplTest : public testing::Test {
   mojo::Remote<mojom::UsbDevice> GetMockDeviceProxy(
       mojo::PendingRemote<mojom::UsbDeviceClient> client) {
     return GetMockDeviceProxy(0x1234, 0x5678, "ACME", "Frobinator", "ABCDEF",
+                              /*allow_security_key_requests=*/false,
                               std::move(client));
   }
 
+  mojo::Remote<mojom::UsbDevice> GetMockSecurityKeyDeviceProxy() {
+    return GetMockDeviceProxy(0x1234, 0x5678, "ACME", "Frobinator", "ABCDEF",
+                              /*allow_security_key_requests=*/true,
+                              mojo::NullRemote());
+  }
+
   mojo::Remote<mojom::UsbDevice> GetMockDeviceProxy() {
     return GetMockDeviceProxy(mojo::NullRemote());
   }
@@ -1020,5 +1028,81 @@ TEST_F(USBDeviceImplTest, IsochronousTransfer) {
   EXPECT_CALL(mock_handle(), Close());
 }
 
+class USBDeviceImplSecurityKeyTest : public USBDeviceImplTest,
+                                     public testing::WithParamInterface<bool> {
+};
+
+TEST_P(USBDeviceImplSecurityKeyTest, SecurityKeyControlTransferBlocked) {
+  const bool allow_security_key_requests = GetParam();
+  mojo::Remote<mojom::UsbDevice> device;
+  if (allow_security_key_requests) {
+    device = GetMockSecurityKeyDeviceProxy();
+  } else {
+    device = GetMockDeviceProxy();
+  }
+
+  EXPECT_CALL(mock_device(), OpenInternal(_));
+
+  {
+    base::RunLoop loop;
+    device->Open(base::BindOnce(
+        &ExpectOpenAndThen, mojom::UsbOpenDeviceError::OK, loop.QuitClosure()));
+    loop.Run();
+  }
+
+  AddMockConfig(ConfigBuilder(1).AddInterface(7, 0, 1, 2, 3).Build());
+  EXPECT_CALL(mock_handle(), SetConfigurationInternal(1, _));
+
+  {
+    base::RunLoop loop;
+    device->SetConfiguration(
+        1, base::BindOnce(&ExpectResultAndThen, true, loop.QuitClosure()));
+    loop.Run();
+  }
+
+  const char* data_str =
+      mojom::UsbControlTransferParams::kSecurityKeyAOAVersion;
+  const std::vector<uint8_t> data(
+      reinterpret_cast<const uint8_t*>(data_str),
+      reinterpret_cast<const uint8_t*>(data_str) + strlen(data_str));
+
+  if (allow_security_key_requests) {
+    AddMockOutboundData(data);
+    EXPECT_CALL(mock_handle(),
+                ControlTransferInternal(UsbTransferDirection::OUTBOUND,
+                                        UsbControlTransferType::VENDOR,
+                                        UsbControlTransferRecipient::DEVICE, 52,
+                                        0, 3, _, 0, _));
+  }
+
+  {
+    // This control transfer should be rejected, unless
+    // |allow_security_key_requests| is true, because it's a request to
+    // trigger security key functionality on Android devices.
+
+    auto params = mojom::UsbControlTransferParams::New();
+    params->type = UsbControlTransferType::VENDOR;
+    params->recipient = UsbControlTransferRecipient::DEVICE;
+    params->request = 52;
+    params->value = 0;
+    params->index = 3;
+    base::RunLoop loop;
+    device->ControlTransferOut(
+        std::move(params), data, 0,
+        base::BindOnce(&ExpectTransferStatusAndThen,
+                       allow_security_key_requests
+                           ? mojom::UsbTransferStatus::COMPLETED
+                           : mojom::UsbTransferStatus::PERMISSION_DENIED,
+                       loop.QuitClosure()));
+    loop.Run();
+  }
+
+  EXPECT_CALL(mock_handle(), Close());
+}
+
+INSTANTIATE_TEST_SUITE_P(USBDeviceImplSecurityKeyTests,
+                         USBDeviceImplSecurityKeyTest,
+                         testing::Values(false, true));
+
 }  // namespace usb
 }  // namespace device

services/device/usb/mojo/device_manager_impl.cc

@@ -67,12 +67,18 @@ void DeviceManagerImpl::GetDevice(
     const std::string& guid,
     mojo::PendingReceiver<mojom::UsbDevice> device_receiver,
     mojo::PendingRemote<mojom::UsbDeviceClient> device_client) {
-  scoped_refptr<UsbDevice> device = usb_service_->GetDevice(guid);
-  if (!device)
-    return;
+  return GetDeviceInternal(guid, std::move(device_receiver),
+                           std::move(device_client),
+                           /*allow_security_key_requests=*/false);
+}
 
-  DeviceImpl::Create(std::move(device), std::move(device_receiver),
-                     std::move(device_client));
+void DeviceManagerImpl::GetSecurityKeyDevice(
+    const std::string& guid,
+    mojo::PendingReceiver<mojom::UsbDevice> device_receiver,
+    mojo::PendingRemote<mojom::UsbDeviceClient> device_client) {
+  return GetDeviceInternal(guid, std::move(device_receiver),
+                           std::move(device_client),
+                           /*allow_security_key_requests=*/true);
 }
 
 #if defined(OS_ANDROID)
@@ -206,5 +212,18 @@ void DeviceManagerImpl::WillDestroyUsbService() {
   clients_.Clear();
 }
 
+void DeviceManagerImpl::GetDeviceInternal(
+    const std::string& guid,
+    mojo::PendingReceiver<mojom::UsbDevice> device_receiver,
+    mojo::PendingRemote<mojom::UsbDeviceClient> device_client,
+    bool allow_security_key_requests) {
+  scoped_refptr<UsbDevice> device = usb_service_->GetDevice(guid);
+  if (!device)
+    return;
+
+  DeviceImpl::Create(std::move(device), std::move(device_receiver),
+                     std::move(device_client), allow_security_key_requests);
+}
+
 }  // namespace usb
 }  // namespace device

services/device/usb/mojo/device_manager_impl.h

@@ -56,6 +56,10 @@ class DeviceManagerImpl : public mojom::UsbDeviceManager,
       const std::string& guid,
       mojo::PendingReceiver<mojom::UsbDevice> device_receiver,
       mojo::PendingRemote<mojom::UsbDeviceClient> device_client) override;
+  void GetSecurityKeyDevice(
+      const std::string& guid,
+      mojo::PendingReceiver<mojom::UsbDevice> device_receiver,
+      mojo::PendingRemote<mojom::UsbDeviceClient> device_client) override;
 
 #if defined(OS_ANDROID)
   void RefreshDeviceInfo(const std::string& guid,
@@ -97,6 +101,11 @@ class DeviceManagerImpl : public mojom::UsbDeviceManager,
   void WillDestroyUsbService() override;
 
   void MaybeRunDeviceChangesCallback();
+  void GetDeviceInternal(
+      const std::string& guid,
+      mojo::PendingReceiver<mojom::UsbDevice> device_receiver,
+      mojo::PendingRemote<mojom::UsbDeviceClient> device_client,
+      bool allow_security_key_requests);
 
   std::unique_ptr<UsbService> usb_service_;
   ScopedObserver<UsbService, UsbService::Observer> observer_;