Check for detached ArrayBuffers in WebUSB transfer functions

This change adds checks to verify that the ArrayBuffers passed to WebUSB
functions controlTransferOut(), isochronousTransferOut(), and
transferOut() have not been detached. If so then an InvalidStateError
is thrown.

While the previous behavior was safe from use-after-frees it silently
sent the USB device an empty buffer which would be difficult to debug.

Change-Id: I94249dd0097991b2b4933093ac85f50f0ccda7c3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1598048Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#657350}

third_party/blink/renderer/modules/webusb/usb_device.cc

@@ -38,6 +38,7 @@ namespace blink {
 
 namespace {
 
+const char kDetachedBuffer[] = "The data buffer has been detached.";
 const char kDeviceStateChangeInProgress[] =
     "An operation that changes the device state is in progress.";
 const char kDeviceDisconnected[] = "The device was disconnected.";
@@ -91,19 +92,32 @@ String ConvertTransferStatus(const UsbTransferStatus& status) {
   }
 }
 
-Vector<uint8_t> ConvertBufferSource(
-    const ArrayBufferOrArrayBufferView& buffer) {
-  DCHECK(!buffer.IsNull());
-  Vector<uint8_t> vector;
-  if (buffer.IsArrayBuffer()) {
-    vector.Append(static_cast<uint8_t*>(buffer.GetAsArrayBuffer()->Data()),
-                  buffer.GetAsArrayBuffer()->ByteLength());
+bool ConvertBufferSource(const ArrayBufferOrArrayBufferView& buffer_source,
+                         Vector<uint8_t>* vector,
+                         ScriptPromiseResolver* resolver) {
+  DCHECK(!buffer_source.IsNull());
+  if (buffer_source.IsArrayBuffer()) {
+    ArrayBuffer* array_buffer = buffer_source.GetAsArrayBuffer()->Buffer();
+    if (array_buffer->IsNeutered()) {
+      resolver->Reject(DOMException::Create(
+          DOMExceptionCode::kInvalidStateError, kDetachedBuffer));
+      return false;
+    }
+
+    vector->Append(static_cast<uint8_t*>(array_buffer->Data()),
+                   array_buffer->ByteLength());
   } else {
-    vector.Append(static_cast<uint8_t*>(
-                      buffer.GetAsArrayBufferView().View()->BaseAddress()),
-                  buffer.GetAsArrayBufferView().View()->byteLength());
+    ArrayBufferView* view = buffer_source.GetAsArrayBufferView().View()->View();
+    if (!view->Buffer() || view->Buffer()->IsNeutered()) {
+      resolver->Reject(DOMException::Create(
+          DOMExceptionCode::kInvalidStateError, kDetachedBuffer));
+      return false;
+    }
+
+    vector->Append(static_cast<uint8_t*>(view->BaseAddress()),
+                   view->ByteLength());
   }
-  return vector;
+  return true;
 }
 
 }  // namespace
@@ -367,18 +381,23 @@ ScriptPromise USBDevice::controlTransferOut(
     const ArrayBufferOrArrayBufferView& data) {
   auto* resolver = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
   ScriptPromise promise = resolver->Promise();
-  if (EnsureDeviceConfigured(resolver)) {
-    auto parameters = ConvertControlTransferParameters(setup, resolver);
-    if (parameters) {
-      Vector<uint8_t> buffer = ConvertBufferSource(data);
-      unsigned transfer_length = buffer.size();
-      device_requests_.insert(resolver);
-      device_->ControlTransferOut(
-          std::move(parameters), buffer, 0,
-          WTF::Bind(&USBDevice::AsyncControlTransferOut, WrapPersistent(this),
-                    transfer_length, WrapPersistent(resolver)));
-    }
-  }
+  if (!EnsureDeviceConfigured(resolver))
+    return promise;
+
+  auto parameters = ConvertControlTransferParameters(setup, resolver);
+  if (!parameters)
+    return promise;
+
+  Vector<uint8_t> buffer;
+  if (!ConvertBufferSource(data, &buffer, resolver))
+    return promise;
+
+  unsigned transfer_length = buffer.size();
+  device_requests_.insert(resolver);
+  device_->ControlTransferOut(
+      std::move(parameters), buffer, 0,
+      WTF::Bind(&USBDevice::AsyncControlTransferOut, WrapPersistent(this),
+                transfer_length, WrapPersistent(resolver)));
   return promise;
 }
 
@@ -416,15 +435,19 @@ ScriptPromise USBDevice::transferOut(ScriptState* script_state,
                                      const ArrayBufferOrArrayBufferView& data) {
   auto* resolver = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
   ScriptPromise promise = resolver->Promise();
-  if (EnsureEndpointAvailable(false /* out */, endpoint_number, resolver)) {
-    Vector<uint8_t> buffer = ConvertBufferSource(data);
-    unsigned transfer_length = buffer.size();
-    device_requests_.insert(resolver);
-    device_->GenericTransferOut(
-        endpoint_number, buffer, 0,
-        WTF::Bind(&USBDevice::AsyncTransferOut, WrapPersistent(this),
-                  transfer_length, WrapPersistent(resolver)));
-  }
+  if (!EnsureEndpointAvailable(false /* out */, endpoint_number, resolver))
+    return promise;
+
+  Vector<uint8_t> buffer;
+  if (!ConvertBufferSource(data, &buffer, resolver))
+    return promise;
+
+  unsigned transfer_length = buffer.size();
+  device_requests_.insert(resolver);
+  device_->GenericTransferOut(
+      endpoint_number, buffer, 0,
+      WTF::Bind(&USBDevice::AsyncTransferOut, WrapPersistent(this),
+                transfer_length, WrapPersistent(resolver)));
   return promise;
 }
 
@@ -451,13 +474,18 @@ ScriptPromise USBDevice::isochronousTransferOut(
     Vector<unsigned> packet_lengths) {
   auto* resolver = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
   ScriptPromise promise = resolver->Promise();
-  if (EnsureEndpointAvailable(false /* out */, endpoint_number, resolver)) {
-    device_requests_.insert(resolver);
-    device_->IsochronousTransferOut(
-        endpoint_number, ConvertBufferSource(data), packet_lengths, 0,
-        WTF::Bind(&USBDevice::AsyncIsochronousTransferOut, WrapPersistent(this),
-                  WrapPersistent(resolver)));
-  }
+  if (!EnsureEndpointAvailable(false /* out */, endpoint_number, resolver))
+    return promise;
+
+  Vector<uint8_t> buffer;
+  if (!ConvertBufferSource(data, &buffer, resolver))
+    return promise;
+
+  device_requests_.insert(resolver);
+  device_->IsochronousTransferOut(
+      endpoint_number, buffer, packet_lengths, 0,
+      WTF::Bind(&USBDevice::AsyncIsochronousTransferOut, WrapPersistent(this),
+                WrapPersistent(resolver)));
   return promise;
 }
 

third_party/blink/web_tests/external/wpt/webusb/usbDevice.https.any.js

@@ -34,6 +34,13 @@ function assertRejectsWithInterfaceStateChangeInProgressError(promise) {
     'An operation that changes interface state is in progress.');
 }
 
+function detachBuffer(buffer) {
+  if (self.GLOBAL.isWindow())
+    window.postMessage('', '*', [buffer]);
+  else
+    self.postMessage('', [buffer]);
+}
+
 usb_test(() => {
   return getFakeDevice().then(({ device, fakeDevice }) => {
     return waitForDisconnect(fakeDevice)
@@ -635,6 +642,47 @@ usb_test(() => {
   });
 }, 'requests to interfaces and endpoint require an interface claim');
 
+usb_test(async () => {
+  const { device } = await getFakeDevice();
+  await device.open();
+  await device.selectConfiguration(1);
+  await device.claimInterface(0);
+
+  const transfer_params = {
+      requestType: 'vendor',
+      recipient: 'device',
+      request: 0,
+      value: 0,
+      index: 0
+  };
+
+  try {
+    const array_buffer = new ArrayBuffer(64 * 8);
+    const result =
+        await device.controlTransferOut(transfer_params, array_buffer);
+    assert_equals(result.status, 'ok');
+
+    detachBuffer(array_buffer);
+    await device.controlTransferOut(transfer_params, array_buffer);
+    assert_unreached();
+  } catch (e) {
+    assert_equals(e.code, DOMException.INVALID_STATE_ERR);
+  }
+
+  try {
+    const typed_array = new Uint8Array(64 * 8);
+    const result =
+        await device.controlTransferOut(transfer_params, typed_array);
+    assert_equals(result.status, 'ok');
+
+    detachBuffer(typed_array.buffer);
+    await device.controlTransferOut(transfer_params, typed_array);
+    assert_unreached();
+  } catch (e) {
+    assert_equals(e.code, DOMException.INVALID_STATE_ERR);
+  }
+}, 'controlTransferOut rejects if called with a detached buffer');
+
 usb_test(() => {
   return getFakeDevice().then(({ device }) => {
     return device.open()
@@ -763,6 +811,38 @@ usb_test(() => {
   });
 }, 'transferOut rejects if called on a disconnected device');
 
+usb_test(async () => {
+  const { device } = await getFakeDevice();
+  await device.open();
+  await device.selectConfiguration(1);
+  await device.claimInterface(1);
+
+
+  try {
+    const array_buffer = new ArrayBuffer(64 * 8);
+    const result = await device.transferOut(2, array_buffer);
+    assert_equals(result.status, 'ok');
+
+    detachBuffer(array_buffer);
+    await device.transferOut(2, array_buffer);
+    assert_unreached();
+  } catch (e) {
+    assert_equals(e.code, DOMException.INVALID_STATE_ERR);
+  }
+
+  try {
+    const typed_array = new Uint8Array(64 * 8);
+    const result = await device.transferOut(2, typed_array);
+    assert_equals(result.status, 'ok');
+
+    detachBuffer(typed_array.buffer);
+    await device.transferOut(2, typed_array);
+    assert_unreached();
+  } catch (e) {
+    assert_equals(e.code, DOMException.INVALID_STATE_ERR);
+  }
+}, 'transferOut rejects if called with a detached buffer');
+
 usb_test(() => {
   return getFakeDevice().then(({ device }) => {
     return device.open()
@@ -854,6 +934,45 @@ usb_test(() => {
   });
 }, 'isochronousTransferOut rejects when called on a disconnected device');
 
+usb_test(async () => {
+  const { device } = await getFakeDevice();
+  await device.open();
+  await device.selectConfiguration(2);
+  await device.claimInterface(0);
+  await device.selectAlternateInterface(0, 1);
+
+
+  try {
+    const array_buffer = new ArrayBuffer(64 * 8);
+    const result = await device.isochronousTransferOut(
+        1, array_buffer, [64, 64, 64, 64, 64, 64, 64, 64]);
+    for (let i = 0; i < result.packets.length; ++i)
+      assert_equals(result.packets[i].status, 'ok');
+
+    detachBuffer(array_buffer);
+    await device.isochronousTransferOut(
+        1, array_buffer, [64, 64, 64, 64, 64, 64, 64, 64]);
+    assert_unreached();
+  } catch (e) {
+    assert_equals(e.code, DOMException.INVALID_STATE_ERR);
+  }
+
+  try {
+    const typed_array = new Uint8Array(64 * 8);
+    const result = await device.isochronousTransferOut(
+        1, typed_array, [64, 64, 64, 64, 64, 64, 64, 64]);
+    for (let i = 0; i < result.packets.length; ++i)
+      assert_equals(result.packets[i].status, 'ok');
+
+    detachBuffer(typed_array.buffer);
+    await device.isochronousTransferOut(
+        1, typed_array, [64, 64, 64, 64, 64, 64, 64, 64]);
+    assert_unreached();
+  } catch (e) {
+    assert_equals(e.code, DOMException.INVALID_STATE_ERR);
+  }
+}, 'isochronousTransferOut rejects when called with a detached buffer');
+
 usb_test(() => {
   return getFakeDevice().then(({ device }) => {
     return device.open().then(() => device.reset()).then(() => device.close());