From 4f939d4646c8dfa5152183760bf2d2bf9428946c Mon Sep 17 00:00:00 2001
From: Jack Miller <[email protected]>
Date: Mon, 29 Jun 2026 11:11:36 -0700
Subject: [PATCH] Deep copy DVTARGETDEVICE in IDataObject::SetData

When an external application calls `IDataObject::SetData` on Chrome's
data object, it can pass a `FORMATETC` containing a non-null target
device pointer (`ptd`). Since the `ptd` buffer is owned by the temporary
COM stub, it is freed as soon as the `SetData` RPC call returns.

This change updates `FormatEtcEnumerator` and
`DataObjectImpl::StoredDataInfo` to perform a deep copy of the target
device pointer (`ptd`) when receiving a `IDataObject::SetData` call.

Bug: 513819157
Change-Id: Ib52a5fad56effa1a7756eb07dd49105062f6ff85
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7952108
Reviewed-by: Jesse McKenna <[email protected]>
Reviewed-by: David Bienvenu <[email protected]>
Commit-Queue: Jack Miller <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1654205}
---

diff --git a/ui/base/dragdrop/os_exchange_data_provider_win.cc b/ui/base/dragdrop/os_exchange_data_provider_win.cc
index 01c5fa8..64fd562 100644
--- a/ui/base/dragdrop/os_exchange_data_provider_win.cc
+++ b/ui/base/dragdrop/os_exchange_data_provider_win.cc
@@ -92,6 +92,105 @@
 
 }  // namespace
 
+ScopedTargetDevice::ScopedTargetDevice() = default;
+
+ScopedTargetDevice::ScopedTargetDevice(const DVTARGETDEVICE* source) {
+  Reset(source);
+}
+
+ScopedTargetDevice::ScopedTargetDevice(const ScopedTargetDevice& other) {
+  Reset(other.get());
+}
+
+ScopedTargetDevice& ScopedTargetDevice::operator=(
+    const ScopedTargetDevice& other) {
+  if (this != &other) {
+    Reset(other.get());
+  }
+  return *this;
+}
+
+ScopedTargetDevice::ScopedTargetDevice(ScopedTargetDevice&& other) noexcept
+    : device_(std::exchange(other.device_, nullptr)) {}
+
+ScopedTargetDevice& ScopedTargetDevice::operator=(
+    ScopedTargetDevice&& other) noexcept {
+  if (this != &other) {
+    Reset(nullptr);
+    device_ = std::exchange(other.device_, nullptr);
+  }
+  return *this;
+}
+
+ScopedTargetDevice::~ScopedTargetDevice() {
+  Reset(nullptr);
+}
+
+void ScopedTargetDevice::Reset(const DVTARGETDEVICE* source) {
+  if (device_) {
+    ::CoTaskMemFree(device_);
+    device_ = nullptr;
+  }
+  if (source && source->tdSize >= sizeof(DVTARGETDEVICE)) {
+    device_ = static_cast<DVTARGETDEVICE*>(::CoTaskMemAlloc(source->tdSize));
+    if (device_) {
+      auto source_span = base::span<const uint8_t>(
+          reinterpret_cast<const uint8_t*>(source), source->tdSize);
+      auto dest_span = base::span<uint8_t>(reinterpret_cast<uint8_t*>(device_),
+                                           source->tdSize);
+      dest_span.copy_from(source_span);
+    }
+  }
+}
+
+DVTARGETDEVICE* ScopedTargetDevice::release() {
+  return std::exchange(device_, nullptr);
+}
+
+ScopedFormatEtc::ScopedFormatEtc() = default;
+
+ScopedFormatEtc::ScopedFormatEtc(const FORMATETC& source)
+    : format_etc(source), target_device(source.ptd) {
+  // Perform a deep copy of the `FORMATETC` structure. This is required because
+  // the incoming `FORMATETC` may contain a pointer to a target device (`ptd`)
+  // that is owned by the COM stub and will be freed as soon as `SetData`
+  // returns.
+  format_etc.ptd = target_device.get();
+}
+
+ScopedFormatEtc::ScopedFormatEtc(const ScopedFormatEtc& other)
+    : format_etc(other.format_etc), target_device(other.target_device) {
+  format_etc.ptd = target_device.get();
+}
+
+ScopedFormatEtc::ScopedFormatEtc(ScopedFormatEtc&& other) noexcept
+    : format_etc(other.format_etc),
+      target_device(std::move(other.target_device)) {
+  format_etc.ptd = target_device.get();
+  other.format_etc.ptd = nullptr;
+}
+
+ScopedFormatEtc& ScopedFormatEtc::operator=(const ScopedFormatEtc& other) {
+  if (this != &other) {
+    format_etc = other.format_etc;
+    target_device = other.target_device;
+    format_etc.ptd = target_device.get();
+  }
+  return *this;
+}
+
+ScopedFormatEtc& ScopedFormatEtc::operator=(ScopedFormatEtc&& other) noexcept {
+  if (this != &other) {
+    format_etc = other.format_etc;
+    target_device = std::move(other.target_device);
+    format_etc.ptd = target_device.get();
+    other.format_etc.ptd = nullptr;
+  }
+  return *this;
+}
+
+ScopedFormatEtc::~ScopedFormatEtc() = default;
+
 ///////////////////////////////////////////////////////////////////////////////
 // FormatEtcEnumerator
 
@@ -113,8 +212,6 @@
   FormatEtcEnumerator(const FormatEtcEnumerator&) = delete;
   FormatEtcEnumerator& operator=(const FormatEtcEnumerator&) = delete;
 
-  ~FormatEtcEnumerator();
-
   // IEnumFORMATETC implementation:
   HRESULT __stdcall Next(ULONG count,
                          FORMATETC* elements_array,
@@ -142,7 +239,7 @@
   // IEnumFORMATETC API assumes a deterministic ordering of elements through
   // methods like Next and Skip. This exposes the underlying data structure to
   // the user. Bah.
-  std::vector<std::unique_ptr<FORMATETC>> contents_;
+  std::vector<ScopedFormatEtc> contents_;
 
   // The cursor of the active enumeration - an index into |contents_|.
   size_t cursor_;
@@ -150,30 +247,15 @@
   ULONG ref_count_;
 };
 
-// Safely makes a copy of all of the relevant bits of a FORMATETC object.
-static void CloneFormatEtc(const FORMATETC* source, FORMATETC* clone) {
-  *clone = *source;
-  if (source->ptd) {
-    clone->ptd =
-        static_cast<DVTARGETDEVICE*>(CoTaskMemAlloc(sizeof(DVTARGETDEVICE)));
-    *(clone->ptd) = *(source->ptd);
-  }
-}
-
 FormatEtcEnumerator::FormatEtcEnumerator(
     DataObjectImpl::StoredData::const_iterator start,
     DataObjectImpl::StoredData::const_iterator end)
     : cursor_(0), ref_count_(0) {
-  // Copy FORMATETC data from our source into ourselves.
-  while (start != end) {
-    auto format_etc = std::make_unique<FORMATETC>();
-    CloneFormatEtc(&(*start)->format_etc, format_etc.get());
-    contents_.push_back(std::move(format_etc));
-    ++start;
-  }
-}
-
-FormatEtcEnumerator::~FormatEtcEnumerator() {
+  std::ranges::transform(
+      start, end, std::back_inserter(contents_),
+      [](const std::unique_ptr<DataObjectImpl::StoredDataInfo>& info) {
+        return info->format_etc;
+      });
 }
 
 HRESULT FormatEtcEnumerator::Next(ULONG count,
@@ -186,7 +268,9 @@
   // This method copies count elements into |elements_array|.
   ULONG index = 0;
   while (cursor_ < contents_.size() && index < count) {
-    CloneFormatEtc(contents_[cursor_].get(), &elements_array[index]);
+    ScopedFormatEtc copy = contents_[cursor_];
+    elements_array[index] = copy.format_etc;
+    elements_array[index].ptd = copy.target_device.release();
     ++cursor_;
     ++index;
   }
@@ -246,14 +330,7 @@
 FormatEtcEnumerator* FormatEtcEnumerator::CloneFromOther(

diff --git a/ui/base/dragdrop/os_exchange_data_win_unittest.cc b/ui/base/dragdrop/os_exchange_data_win_unittest.cc
index 3c479d7..3a8710c 100644
--- a/ui/base/dragdrop/os_exchange_data_win_unittest.cc
+++ b/ui/base/dragdrop/os_exchange_data_win_unittest.cc
@@ -17,6 +17,7 @@
 #include "base/files/file_util.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback.h"
+#include "base/functional/callback_helpers.h"
 #include "base/memory/raw_ptr.h"
 #include "base/memory/ref_counted.h"
 #include "base/sequence_checker.h"
@@ -24,6 +25,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/run_until.h"
 #include "base/test/task_environment.h"
+#include "base/threading/thread.h"
 #include "base/win/scoped_com_initializer.h"
 #include "base/win/scoped_hglobal.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -39,6 +41,16 @@
 
 namespace {
 
+// Creates a dummy `STGMEDIUM` containing 4 bytes of data ("AAAA").
+STGMEDIUM CreateHGlobalMedium() {
+  STGMEDIUM stgm = {};
+  stgm.tymed = TYMED_HGLOBAL;
+  stgm.hGlobal = GlobalAlloc(GPTR, 4);
+  base::win::ScopedHGlobal<std::array<char, 4>*> lock(stgm.hGlobal);
+  *lock.data() = {'A', 'A', 'A', 'A'};
+  return stgm;
+}
+
 const std::vector<DWORD> kStorageMediaTypesForVirtualFiles = {
     TYMED_ISTORAGE,
     TYMED_ISTREAM,
@@ -1236,4 +1248,308 @@
   EXPECT_EQ(stream.GetRefCount(), 0u);
 }
 
+// Verifies that calling `DataObjectImpl::SetData` with a `FORMATETC` containing
+// a target device (`ptd`) correctly performs a deep copy of the variable-length
+// `DVTARGETDEVICE` structure. This prevents Use-After-Free of the COM-stub
+// owned `ptd` pointer, and Out-of-Bounds heap read when the data object's
+// formats are later cloned and marshalled back to the caller.
+TEST_F(OSExchangeDataWinTest, SetDataTargetDeviceUAFAndOOB) {
+  base::win::ScopedCOMInitializer com_initializer;
+
+  // Create the data provider on the main thread and marshal the `IDataObject`
+  // pointer into a stream so it can be unmarshalled and accessed by a
+  // background thread.
+  OSExchangeDataProviderWin data_provider;
+  IDataObject* data_object = data_provider.data_object();
+
+  IStream* stream = nullptr;
+  ASSERT_EQ(CoMarshalInterThreadInterfaceInStream(IID_IDataObject, data_object,
+                                                  &stream),
+            S_OK);
+
+  base::RunLoop run_loop;
+
+  // Run a background thread representing the cross-process COM client.
+  base::Thread background_thread("UAFBackgroundThread");
+  background_thread.Start();
+
+  background_thread.task_runner()->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          [](uintptr_t stream_ptr_val, base::OnceClosure quit_closure) {
+            base::ScopedClosureRunner quit_runner(std::move(quit_closure));
+            {
+              base::win::ScopedCOMInitializer com_initializer;
+
+              // Unmarshal the data object proxy.
+              IStream* stream_ptr = reinterpret_cast<IStream*>(stream_ptr_val);
+              Microsoft::WRL::ComPtr<IDataObject> proxy;
+              ASSERT_EQ(CoGetInterfaceAndReleaseStream(stream_ptr,
+                                                       IID_PPV_ARGS(&proxy)),
+                        S_OK);
+
+              // Call `SetData` with a `FORMATETC` containing a target device.
+              const DWORD kTdSize = sizeof(DVTARGETDEVICE);
+              base::win::ScopedCoMem<DVTARGETDEVICE> dv;
+              dv.Reset(static_cast<DVTARGETDEVICE*>(CoTaskMemAlloc(kTdSize)));
+              ASSERT_NE(dv.get(), nullptr);
+
+              dv->tdSize = kTdSize;
+              dv->tdDriverNameOffset = 0;
+              dv->tdDeviceNameOffset = 0;
+              dv->tdPortNameOffset = 0;
+              dv->tdExtDevmodeOffset = 0;
+              dv->tdData[0] = 0;
+
+              FORMATETC fe =
+                  ClipboardFormatType(RegisterClipboardFormatW(L"ChromePtdPoC"),
+                                      -1, TYMED_HGLOBAL)
+                      .ToFormatEtc();
+              fe.ptd = dv.get();
+
+              STGMEDIUM stgm = CreateHGlobalMedium();
+              // `DataObjectImpl` takes ownership of `stgm.hGlobal` when calling
+              // `SetData` with `fRelease=TRUE`.
+              ASSERT_EQ(proxy->SetData(&fe, &stgm, TRUE), S_OK);
+
+              // Free the local target device buffer by resetting the
+              // `ScopedCoMem`. The COM stub on the main thread will also free
+              // its temporary buffer when `SetData` returns.
+              dv.Reset(nullptr);
+
+              // Call `EnumFormatEtc` and retrieve the cloned format. If Chrome
+              // shallow-copied `ptd`, it will read a dangling pointer,
+              // corrupting the `tdSize` value. During marshalling of `Next()`,
+              // COM will crash attempting to read an invalid `tdSize` out of
+              // bounds of the 16-byte header, causing `next_hr` to return
+              // `RPC_E_SERVERFAULT`. If Chrome correctly performs a deep copy
+              // of `ptd` during `SetData` and clones it with the correct size,
+              // `Next()` will succeed and return the duplicated `ptd`.
+              Microsoft::WRL::ComPtr<IEnumFORMATETC> enum_fmt;
+              ASSERT_EQ(proxy->EnumFormatEtc(DATADIR_GET, &enum_fmt), S_OK);
+              FORMATETC out_fe = {};
+              ULONG fetched = 0;
+              EXPECT_EQ(enum_fmt->Next(1, &out_fe, &fetched), S_OK);
+              EXPECT_EQ(fetched, 1ul);
+
+              base::win::ScopedCoMem<DVTARGETDEVICE> out_ptd;
+              out_ptd.Reset(out_fe.ptd);
+              ASSERT_NE(out_ptd.get(), nullptr);
+              EXPECT_EQ(out_ptd->tdSize, kTdSize);
+            }
+          },
+          reinterpret_cast<uintptr_t>(stream), run_loop.QuitClosure()));
+
+  run_loop.Run();
+}
+
+// Verifies that calling `DataObjectImpl::SetData` with a `FORMATETC` containing
+// a `null` target device (`ptd`) is handled safely.
+TEST_F(OSExchangeDataWinTest, SetDataTargetDeviceNull) {
+  base::win::ScopedCOMInitializer com_initializer;
+
+  OSExchangeDataProviderWin data_provider;
+  IDataObject* data_object = data_provider.data_object();
+
+  IStream* stream = nullptr;
+  ASSERT_EQ(CoMarshalInterThreadInterfaceInStream(IID_IDataObject, data_object,
+                                                  &stream),
+            S_OK);
+
+  base::RunLoop run_loop;
+
+  base::Thread background_thread("BackgroundThread");
+  background_thread.Start();
+
+  background_thread.task_runner()->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          [](uintptr_t stream_ptr_val, base::OnceClosure quit_closure) {
+            base::ScopedClosureRunner quit_runner(std::move(quit_closure));
+            {
+              base::win::ScopedCOMInitializer com_initializer;
+
+              IStream* stream_ptr = reinterpret_cast<IStream*>(stream_ptr_val);
+              Microsoft::WRL::ComPtr<IDataObject> proxy;
+              ASSERT_EQ(CoGetInterfaceAndReleaseStream(stream_ptr,
+                                                       IID_PPV_ARGS(&proxy)),
+                        S_OK);
+
+              FORMATETC fe = ClipboardFormatType(
+                                 RegisterClipboardFormatW(L"ChromePtdNull"), -1,
+                                 TYMED_HGLOBAL)
+                                 .ToFormatEtc();
+              fe.ptd = nullptr;
+
+              STGMEDIUM stgm = CreateHGlobalMedium();
+              // `DataObjectImpl` takes ownership of `stgm.hGlobal` when calling
+              // `SetData` with `fRelease=TRUE`.
+              ASSERT_EQ(proxy->SetData(&fe, &stgm, TRUE), S_OK);
+
+              Microsoft::WRL::ComPtr<IEnumFORMATETC> enum_fmt;
+              ASSERT_EQ(proxy->EnumFormatEtc(DATADIR_GET, &enum_fmt), S_OK);
+              FORMATETC out_fe = {};
+              ULONG fetched = 0;
+              EXPECT_EQ(enum_fmt->Next(1, &out_fe, &fetched), S_OK);
+              EXPECT_EQ(fetched, 1ul);
+              EXPECT_EQ(out_fe.ptd, nullptr);
+            }
+          },
+          reinterpret_cast<uintptr_t>(stream), run_loop.QuitClosure()));
+
+  run_loop.Run();
+}
+
+// Verifies that calling `DataObjectImpl::SetData` directly (in-process) with a
+// `FORMATETC` containing a target device of an abnormally small `tdSize`
+// (smaller than `sizeof(DVTARGETDEVICE)`) is handled safely without
+// out-of-bounds access.
+TEST_F(OSExchangeDataWinTest, SetDataTargetDeviceSmallSize) {
+  OSExchangeDataProviderWin data_provider;
+  IDataObject* data_object = data_provider.data_object();
+
+  // Call `SetData` directly on the main thread with a target device size of
+  // only 4 bytes (enough for `tdSize` itself, but smaller than
+  // `sizeof(DVTARGETDEVICE)`).
+  const DWORD kTdSize = 4;
+  base::win::ScopedCoMem<DVTARGETDEVICE> dv;
+  dv.Reset(static_cast<DVTARGETDEVICE*>(CoTaskMemAlloc(kTdSize)));
+  ASSERT_NE(dv.get(), nullptr);
+  dv->tdSize = kTdSize;
+
+  FORMATETC fe =
+      ClipboardFormatType(RegisterClipboardFormatW(L"ChromePtdSmall"), -1,
+                          TYMED_HGLOBAL)
+          .ToFormatEtc();
+  fe.ptd = dv.get();
+
+  STGMEDIUM stgm = CreateHGlobalMedium();
+  // `DataObjectImpl` takes ownership of `stgm.hGlobal` when calling `SetData`
+  // with `fRelease=TRUE`.
+  ASSERT_EQ(data_object->SetData(&fe, &stgm, TRUE), S_OK);
+  dv.Reset(nullptr);
+
+  Microsoft::WRL::ComPtr<IEnumFORMATETC> enum_fmt;
+  ASSERT_EQ(data_object->EnumFormatEtc(DATADIR_GET, &enum_fmt), S_OK);
+  FORMATETC out_fe = {};
+  ULONG fetched = 0;
+  EXPECT_EQ(enum_fmt->Next(1, &out_fe, &fetched), S_OK);
+  EXPECT_EQ(fetched, 1ul);
+
+  EXPECT_EQ(out_fe.ptd, nullptr);
+}
+
+TEST_F(OSExchangeDataWinTest, ScopedTargetDeviceLifecycle) {
+  ScopedTargetDevice empty;
+  EXPECT_EQ(empty.get(), nullptr);
+
+  const DWORD kTdSize = 64;
+  base::win::ScopedCoMem<DVTARGETDEVICE> raw;
+  raw.Reset(static_cast<DVTARGETDEVICE*>(CoTaskMemAlloc(kTdSize)));
+  ASSERT_NE(raw.get(), nullptr);
+  raw->tdSize = kTdSize;
+  raw->tdDriverNameOffset = 0;
+  raw->tdDeviceNameOffset = 0;
+  raw->tdPortNameOffset = 0;
+  raw->tdExtDevmodeOffset = 0;
+
+  ScopedTargetDevice device(raw.get());
+  EXPECT_NE(device.get(), nullptr);
+  EXPECT_NE(device.get(), raw.get());
+  EXPECT_EQ(device.get()->tdSize, kTdSize);
+
+  ScopedTargetDevice copy(device);
+  EXPECT_NE(copy.get(), nullptr);
+  EXPECT_NE(copy.get(), device.get());
+  EXPECT_EQ(copy.get()->tdSize, kTdSize);
+
+  DVTARGETDEVICE* copy_raw = copy.get();
+  ScopedTargetDevice moved(std::move(copy));
+  EXPECT_EQ(copy.get(), nullptr);
+  EXPECT_EQ(moved.get(), copy_raw);
+
+  ScopedTargetDevice assigned;
+  assigned = device;
+  EXPECT_NE(assigned.get(), nullptr);
+  EXPECT_NE(assigned.get(), device.get());
+  EXPECT_EQ(assigned.get()->tdSize, kTdSize);
+
+  ScopedTargetDevice moved_assigned;
+  DVTARGETDEVICE* moved_raw = moved.get();
+  moved_assigned = std::move(moved);
+  EXPECT_EQ(moved.get(), nullptr);
+  EXPECT_EQ(moved_assigned.get(), moved_raw);
+
+  base::win::ScopedCoMem<DVTARGETDEVICE> released;
+  released.Reset(moved_assigned.release());
+  EXPECT_EQ(moved_assigned.get(), nullptr);
+  EXPECT_EQ(released.get(), moved_raw);
+
+  // Test rejection of abnormally small target devices.
+  {
+    const DWORD kSmallTdSize = sizeof(DVTARGETDEVICE) - 1;
+    base::win::ScopedCoMem<DVTARGETDEVICE> small_raw;
+    small_raw.Reset(static_cast<DVTARGETDEVICE*>(CoTaskMemAlloc(kSmallTdSize)));
+    ASSERT_NE(small_raw.get(), nullptr);
+    small_raw->tdSize = kSmallTdSize;
+
+    ScopedTargetDevice small_device(small_raw.get());
+    EXPECT_EQ(small_device.get(), nullptr);
+  }
+}
+
+TEST_F(OSExchangeDataWinTest, ScopedFormatEtcLifecycle) {
+  ScopedFormatEtc empty;
+  EXPECT_EQ(empty.format_etc.ptd, nullptr);
+
+  const DWORD kTdSize = 64;
+  base::win::ScopedCoMem<DVTARGETDEVICE> raw_td;
+  raw_td.Reset(static_cast<DVTARGETDEVICE*>(CoTaskMemAlloc(kTdSize)));
+  ASSERT_NE(raw_td.get(), nullptr);
+  raw_td->tdSize = kTdSize;
... (truncated)