From 7675c9965682a9d83de5aff379ab78531204238d Mon Sep 17 00:00:00 2001
From: Daniel Cheng <[email protected]>
Date: Fri, 28 Mar 2025 13:30:27 -0700
Subject: [PATCH] Don't allow text -> URL conversion when dropping to bypass URL filtering

When starting a drag from a renderer, the browser process filters out
URLs that the initiating renderer process should not be able to navigate
to, e.g. a random http/https page should not be able to specify
chrome://settings/ as URL to navigate to when dropped.

However, when dropping, Chrome is clever and tries to interpret text as
URLs when needed. To prevent this from bypassing the URL filtering, only
allow this conversion if:
- the drag data does not originate from the renderer
- or the text to URL conversion results in a HTTP or HTTPS url

Bug: 404000989
Change-Id: I28baf7e6385b440af7e76b08471588299e24e247
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6400553
Reviewed-by: Avi Drissman <[email protected]>
Commit-Queue: Daniel Cheng <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1439671}
---

diff --git a/chrome/browser/ui/views/frame/browser_root_view.cc b/chrome/browser/ui/views/frame/browser_root_view.cc
index d38c38b..abf409a 100644
--- a/chrome/browser/ui/views/frame/browser_root_view.cc
+++ b/chrome/browser/ui/views/frame/browser_root_view.cc
@@ -13,6 +13,7 @@
 
 #include "base/check_op.h"
 #include "base/containers/adapters.h"
+#include "base/feature_list.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback_helpers.h"
 #include "base/metrics/user_metrics.h"
@@ -50,6 +51,7 @@
 #include "ui/base/dragdrop/os_exchange_data.h"
 #include "ui/base/hit_test.h"
 #include "ui/base/metadata/metadata_impl_macros.h"
+#include "ui/base/ui_base_features.h"
 #include "ui/color/color_provider.h"
 #include "ui/compositor/layer_tree_owner.h"
 #include "ui/compositor/paint_recorder.h"
@@ -555,6 +557,17 @@
     return std::nullopt;
   }
 
+  // `OSExchangeData` already tries to do best-effort conversion of strings
+  // to URLs, but the browser also does this coercion using slightly different
+  // logic. To avoid this coercion from bypassing URL filtering, only allow this
+  // coercion from http or https URLs if the drag data is renderer tainted.
+  if (base::FeatureList::IsEnabled(
+          features::kDragDropOnlySynthesizeHttpOrHttpsUrlsFromText) &&
+      data.IsRendererTainted() &&
+      !match.destination_url.SchemeIsHTTPOrHTTPS()) {
+    return std::nullopt;
+  }
+
   return match.destination_url;
 }
 
diff --git a/chrome/browser/ui/views/frame/browser_root_view_browsertest.cc b/chrome/browser/ui/views/frame/browser_root_view_browsertest.cc
index 5660bbe9..aa7385f 100644
--- a/chrome/browser/ui/views/frame/browser_root_view_browsertest.cc
+++ b/chrome/browser/ui/views/frame/browser_root_view_browsertest.cc
@@ -56,23 +56,26 @@
     browser_root_view()->OnMouseWheel(wheel_event);
   }
 
+  void WaitForDragStart(const ui::DropTargetEvent& event) {
+    base::RunLoop run_loop;
+    BrowserRootView* const root_view = browser_root_view();
+    root_view->SetOnFilteringCompleteClosureForTesting(run_loop.QuitClosure());
+    root_view->OnDragEntered(event);
+    run_loop.Run();
+  }
+
   void StartAndFinishDrag(const ui::OSExchangeData& data,
                           ui::mojom::DragOperation& out_drag_op) {
     ui::DropTargetEvent event(data, gfx::PointF(), gfx::PointF(),
                               ui::DragDropTypes::DRAG_COPY);
-    BrowserRootView* root_view = browser_root_view();
-
-    base::RunLoop run_loop;
-    root_view->SetOnFilteringCompleteClosureForTesting(run_loop.QuitClosure());
-    root_view->OnDragEntered(event);
+    WaitForDragStart(event);
 
     // At this point, the drag information will have been set, and a background
     // task will have been posted to process the dragged URLs
     // (`GetURLMimeTypes()` -> `FilterURLs()`). Ensure that all background
     // processing is complete before checking the drag operation or invoking the
     // drag callback.
-    run_loop.Run();
-
+    BrowserRootView* const root_view = browser_root_view();
     EXPECT_NE(ui::DragDropTypes::DRAG_NONE, root_view->OnDragUpdated(event));
 
     auto drop_cb = root_view->GetDropCallback(event);
@@ -471,3 +474,32 @@
   EXPECT_TRUE(observer.last_initiator_origin().has_value());
   EXPECT_EQ(initiator_origin, observer.last_initiator_origin().value());
 }
+
+IN_PROC_BROWSER_TEST_F(BrowserRootViewBrowserTest, NavigateToUrlFromText) {
+  ASSERT_TRUE(AddTabAtIndex(0, GURL("about:blank"), ui::PAGE_TRANSITION_LINK));
+  using BrowserRootView::DropIndex::RelativeToIndex::kReplaceIndex;
+
+  ui::OSExchangeData data;
+  data.SetString(u"chrome://settings/");
+  ui::DropTargetEvent event(data, gfx::PointF(), gfx::PointF(),
+                            ui::DragDropTypes::DRAG_COPY);
+  WaitForDragStart(event);
+
+  EXPECT_EQ(ui::DragDropTypes::DRAG_COPY,
+            browser_root_view()->OnDragUpdated(event));
+}
+IN_PROC_BROWSER_TEST_F(BrowserRootViewBrowserTest,
+                       DoesNotNavigateToUrlFromRendererTaintedText) {
+  ASSERT_TRUE(AddTabAtIndex(0, GURL("about:blank"), ui::PAGE_TRANSITION_LINK));
+  using BrowserRootView::DropIndex::RelativeToIndex::kReplaceIndex;
+
+  ui::OSExchangeData data;
+  data.SetString(u"chrome://settings/");
+  data.MarkRendererTaintedFromOrigin(url::Origin());
+  ui::DropTargetEvent event(data, gfx::PointF(), gfx::PointF(),
+                            ui::DragDropTypes::DRAG_COPY);
+  WaitForDragStart(event);
+
+  EXPECT_EQ(ui::DragDropTypes::DRAG_NONE,
+            browser_root_view()->OnDragUpdated(event));
+}
diff --git a/ui/base/clipboard/clipboard_util_mac.mm b/ui/base/clipboard/clipboard_util_mac.mm
index 3a196911..2a8365a 100644
--- a/ui/base/clipboard/clipboard_util_mac.mm
+++ b/ui/base/clipboard/clipboard_util_mac.mm
@@ -17,6 +17,7 @@
 #include "ui/base/clipboard/clipboard_constants.h"
 #include "ui/base/clipboard/file_info.h"
 #include "ui/base/clipboard/url_file_parser.h"
+#include "ui/base/ui_base_features.h"
 #include "url/gurl.h"
 
 @interface URLAndTitle ()
@@ -228,7 +229,8 @@
 
 // Returns a URL and title if a string on the pasteboard item is formatted as a
 // URL but doesn't actually have the URL type.
-URLAndTitle* ExtractURLFromStringValue(NSPasteboardItem* item) {
+URLAndTitle* ExtractURLFromStringValue(NSPasteboardItem* item,
+                                       bool is_renderer_tainted) {
   NSString* string = [item stringForType:NSPasteboardTypeString];
   if (!string) {
     return nil;
@@ -250,6 +252,12 @@
     return nil;
   }
 
+  if (base::FeatureList::IsEnabled(
+          features::kDragDropOnlySynthesizeHttpOrHttpsUrlsFromText) &&
+      is_renderer_tainted && !url.SchemeIsHTTPOrHTTPS()) {
+    return nil;
+  }
+
   // The hostname is the best that can be done for the title.
   return [URLAndTitle URLAndTitleWithURL:string
                                    title:base::SysUTF8ToNSString(url.host())];
@@ -275,6 +283,8 @@
                                               bool include_files) {
   NSMutableArray<URLAndTitle*>* result = [NSMutableArray array];
 
+  const bool is_renderer_tainted =
+      [pboard.types containsObject:kUTTypeChromiumRendererInitiatedDrag];
   for (NSPasteboardItem* item in pboard.pasteboardItems) {
     // Try each of several ways of getting URLs from the pasteboard item and
     // stop with the first one that works.
@@ -286,7 +296,7 @@
     }
 
     if (!url_and_title) {
-      url_and_title = ExtractURLFromStringValue(item);
+      url_and_title = ExtractURLFromStringValue(item, is_renderer_tainted);
     }
 
     if (!url_and_title && include_files) {
diff --git a/ui/base/clipboard/clipboard_util_mac_unittest.mm b/ui/base/clipboard/clipboard_util_mac_unittest.mm
index 15ab2f4..f8bef91 100644
--- a/ui/base/clipboard/clipboard_util_mac_unittest.mm
+++ b/ui/base/clipboard/clipboard_util_mac_unittest.mm
@@ -50,22 +50,81 @@
   EXPECT_FALSE([items[1].types containsObject:kUTTypeWebKitWebURLsWithTitles]);
 }
 
-TEST_F(ClipboardUtilMacTest, PasteboardItemsFromString) {
-  NSString* url_string = @"    https://www.google.com/   ";
+TEST_F(ClipboardUtilMacTest, PasteboardUrlsFromString) {
+  {
+    NSString* url_string = @"    https://www.google.com/   ";
 

diff --git a/chrome/browser/ui/views/frame/browser_root_view_browsertest.cc b/chrome/browser/ui/views/frame/browser_root_view_browsertest.cc
index 5660bbe9..aa7385f 100644
--- a/chrome/browser/ui/views/frame/browser_root_view_browsertest.cc
+++ b/chrome/browser/ui/views/frame/browser_root_view_browsertest.cc
@@ -56,23 +56,26 @@
     browser_root_view()->OnMouseWheel(wheel_event);
   }
 
+  void WaitForDragStart(const ui::DropTargetEvent& event) {
+    base::RunLoop run_loop;
+    BrowserRootView* const root_view = browser_root_view();
+    root_view->SetOnFilteringCompleteClosureForTesting(run_loop.QuitClosure());
+    root_view->OnDragEntered(event);
+    run_loop.Run();
+  }
+
   void StartAndFinishDrag(const ui::OSExchangeData& data,
                           ui::mojom::DragOperation& out_drag_op) {
     ui::DropTargetEvent event(data, gfx::PointF(), gfx::PointF(),
                               ui::DragDropTypes::DRAG_COPY);
-    BrowserRootView* root_view = browser_root_view();
-
-    base::RunLoop run_loop;
-    root_view->SetOnFilteringCompleteClosureForTesting(run_loop.QuitClosure());
-    root_view->OnDragEntered(event);
+    WaitForDragStart(event);
 
     // At this point, the drag information will have been set, and a background
     // task will have been posted to process the dragged URLs
     // (`GetURLMimeTypes()` -> `FilterURLs()`). Ensure that all background
     // processing is complete before checking the drag operation or invoking the
     // drag callback.
-    run_loop.Run();
-
+    BrowserRootView* const root_view = browser_root_view();
     EXPECT_NE(ui::DragDropTypes::DRAG_NONE, root_view->OnDragUpdated(event));
 
     auto drop_cb = root_view->GetDropCallback(event);
@@ -471,3 +474,32 @@
   EXPECT_TRUE(observer.last_initiator_origin().has_value());
   EXPECT_EQ(initiator_origin, observer.last_initiator_origin().value());
 }
+
+IN_PROC_BROWSER_TEST_F(BrowserRootViewBrowserTest, NavigateToUrlFromText) {
+  ASSERT_TRUE(AddTabAtIndex(0, GURL("about:blank"), ui::PAGE_TRANSITION_LINK));
+  using BrowserRootView::DropIndex::RelativeToIndex::kReplaceIndex;
+
+  ui::OSExchangeData data;
+  data.SetString(u"chrome://settings/");
+  ui::DropTargetEvent event(data, gfx::PointF(), gfx::PointF(),
+                            ui::DragDropTypes::DRAG_COPY);
+  WaitForDragStart(event);
+
+  EXPECT_EQ(ui::DragDropTypes::DRAG_COPY,
+            browser_root_view()->OnDragUpdated(event));
+}
+IN_PROC_BROWSER_TEST_F(BrowserRootViewBrowserTest,
+                       DoesNotNavigateToUrlFromRendererTaintedText) {
+  ASSERT_TRUE(AddTabAtIndex(0, GURL("about:blank"), ui::PAGE_TRANSITION_LINK));
+  using BrowserRootView::DropIndex::RelativeToIndex::kReplaceIndex;
+
+  ui::OSExchangeData data;
+  data.SetString(u"chrome://settings/");
+  data.MarkRendererTaintedFromOrigin(url::Origin());
+  ui::DropTargetEvent event(data, gfx::PointF(), gfx::PointF(),
+                            ui::DragDropTypes::DRAG_COPY);
+  WaitForDragStart(event);
+
+  EXPECT_EQ(ui::DragDropTypes::DRAG_NONE,
+            browser_root_view()->OnDragUpdated(event));
+}
diff --git a/ui/base/clipboard/clipboard_util_mac_unittest.mm b/ui/base/clipboard/clipboard_util_mac_unittest.mm
index 15ab2f4..f8bef91 100644
--- a/ui/base/clipboard/clipboard_util_mac_unittest.mm
+++ b/ui/base/clipboard/clipboard_util_mac_unittest.mm
@@ -50,22 +50,81 @@
   EXPECT_FALSE([items[1].types containsObject:kUTTypeWebKitWebURLsWithTitles]);
 }
 
-TEST_F(ClipboardUtilMacTest, PasteboardItemsFromString) {
-  NSString* url_string = @"    https://www.google.com/   ";
+TEST_F(ClipboardUtilMacTest, PasteboardUrlsFromString) {
+  {
+    NSString* url_string = @"    https://www.google.com/   ";
 
-  scoped_refptr<UniquePasteboard> pasteboard = new UniquePasteboard;
-  [pasteboard->get() writeObjects:@[ url_string ]];
+    scoped_refptr<UniquePasteboard> pasteboard = new UniquePasteboard;
+    [pasteboard->get() writeObjects:@[ url_string ]];
 
-  NSArray<URLAndTitle*>* urls_and_titles =
-      clipboard_util::URLsAndTitlesFromPasteboard(pasteboard->get(),
-                                                  /*include_files=*/false);
+    NSArray<URLAndTitle*>* urls_and_titles =
+        clipboard_util::URLsAndTitlesFromPasteboard(pasteboard->get(),
+                                                    /*include_files=*/false);
 
-  ASSERT_EQ(1u, urls_and_titles.count);
-  EXPECT_NSEQ(@"https://www.google.com/", urls_and_titles[0].URL);
-  EXPECT_NSEQ(@"www.google.com", urls_and_titles[0].title);
+    ASSERT_EQ(1u, urls_and_titles.count);
+    EXPECT_NSEQ(@"https://www.google.com/", urls_and_titles[0].URL);
+    EXPECT_NSEQ(@"www.google.com", urls_and_titles[0].title);
+  }
+
+  // Even when renderer-tainted, HTTPS URLs should be synthesized from
+  // NSPasteboard's text content.
+  {
+    NSString* url_string = @"    https://www.google.com/   ";
+    NSPasteboardItem* item = [[NSPasteboardItem alloc] init];
+    [item setString:url_string forType:NSPasteboardTypeString];
+    [item setString:@"https://www.google.com/"
+            forType:kUTTypeChromiumRendererInitiatedDrag];
+
+    scoped_refptr<UniquePasteboard> pasteboard = new UniquePasteboard;
+    [pasteboard->get() writeObjects:@[ item ]];
+
+    NSArray<URLAndTitle*>* urls_and_titles =
+        clipboard_util::URLsAndTitlesFromPasteboard(pasteboard->get(),
+                                                    /*include_files=*/false);
+
+    ASSERT_EQ(1u, urls_and_titles.count);
+    EXPECT_NSEQ(@"https://www.google.com/", urls_and_titles[0].URL);
+    EXPECT_NSEQ(@"www.google.com", urls_and_titles[0].title);
+  }
 }
 
-TEST_F(ClipboardUtilMacTest, PasteboardItemWithFilePath) {
+TEST_F(ClipboardUtilMacTest, PasteboardUrlsFromNonHttpAndNonHttpsUrlString) {
+  {
+    NSString* url_string = @"chrome://settings/";
+
+    scoped_refptr<UniquePasteboard> pasteboard = new UniquePasteboard;
+    [pasteboard->get() writeObjects:@[ url_string ]];
+
+    NSArray<URLAndTitle*>* urls_and_titles =
+        clipboard_util::URLsAndTitlesFromPasteboard(pasteboard->get(),
+                                                    /*include_files=*/false);
+
+    ASSERT_EQ(1u, urls_and_titles.count);
+    EXPECT_NSEQ(@"chrome://settings/", urls_and_titles[0].URL);
+    EXPECT_NSEQ(@"settings", urls_and_titles[0].title);
+  }
+
+  // A non-HTTP / non-HTTPS URL should not be synthesized from a
+  // renderer-tainted NSPasteboard's text content.
+  {
+    NSString* url_string = @"chrome://settings/";
+    NSPasteboardItem* item = [[NSPasteboardItem alloc] init];
+    [item setString:url_string forType:NSPasteboardTypeString];
+    [item setString:@"chrome://settings/"
+            forType:kUTTypeChromiumRendererInitiatedDrag];
+
+    scoped_refptr<UniquePasteboard> pasteboard = new UniquePasteboard;
+    [pasteboard->get() writeObjects:@[ item ]];
+
+    NSArray<URLAndTitle*>* urls_and_titles =
+        clipboard_util::URLsAndTitlesFromPasteboard(pasteboard->get(),
+                                                    /*include_files=*/false);
+
+    ASSERT_EQ(0u, urls_and_titles.count);
+  }
+}
+
+TEST_F(ClipboardUtilMacTest, PasteboardUrlsWithFilePath) {
   NSURL* url = [NSURL fileURLWithPath:NSTemporaryDirectory() isDirectory:YES];
   ASSERT_TRUE(url);
   NSString* url_string = url.absoluteString;
diff --git a/ui/base/dragdrop/os_exchange_data_unittest.cc b/ui/base/dragdrop/os_exchange_data_unittest.cc
index 81f4003..91b980c 100644
--- a/ui/base/dragdrop/os_exchange_data_unittest.cc
+++ b/ui/base/dragdrop/os_exchange_data_unittest.cc
@@ -73,6 +73,104 @@
   EXPECT_EQ(u"https://www.google.com/", string);
 }
 
+// TODO(crbug.com/406978702): string -> URL conversion is only implemented on
+// some platforms—and it is not consistently implemented across all platforms.
+// Maybe this will be fixed one day...
+#if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)
+static constexpr bool kSupportsStringToUrlCoercion = true;
+static constexpr bool kStringToUrlCoercionPopulatesTitle = true;
+#elif BUILDFLAG(IS_OZONE) && defined(USE_AURA)
+static constexpr bool kSupportsStringToUrlCoercion = true;
+static constexpr bool kStringToUrlCoercionPopulatesTitle = false;
+#else
+static constexpr bool kSupportsStringToUrlCoercion = false;
+static constexpr bool kStringToUrlCoercionPopulatesTitle = false;
+#endif
+
+TEST_F(OSExchangeDataTest, URLFromString) {
+  const OSExchangeData copy([] {
+    OSExchangeData data;
+    data.SetString(u"https://www.google.com/");
+    return data.provider().Clone();
+  }());
+
+  EXPECT_TRUE(copy.HasURL(FilenameToURLPolicy::DO_NOT_CONVERT_FILENAMES));
+  std::optional<OSExchangeData::UrlInfo> url_info =
+      copy.GetURLAndTitle(FilenameToURLPolicy::DO_NOT_CONVERT_FILENAMES);
+  if constexpr (kSupportsStringToUrlCoercion) {
+    EXPECT_TRUE(url_info.has_value());
+    EXPECT_EQ("https://www.google.com/", url_info->url.spec());
+    if constexpr (kStringToUrlCoercionPopulatesTitle) {
+      EXPECT_EQ(u"www.google.com", url_info->title);
+    } else {
+      EXPECT_EQ(u"", url_info->title);
+    }
+  } else {
+    EXPECT_FALSE(url_info.has_value());
+  }
+}
+
+TEST_F(OSExchangeDataTest, URLFromRendererTaintedString) {
+  const OSExchangeData copy([] {
+    OSExchangeData data;
+    data.SetString(u"https://www.google.com/");
+    data.MarkRendererTaintedFromOrigin(url::Origin());
+    return data.provider().Clone();
+  }());
+
+  EXPECT_TRUE(copy.HasURL(FilenameToURLPolicy::DO_NOT_CONVERT_FILENAMES));
+  std::optional<OSExchangeData::UrlInfo> url_info =
+      copy.GetURLAndTitle(FilenameToURLPolicy::DO_NOT_CONVERT_FILENAMES);
+  if constexpr (kSupportsStringToUrlCoercion) {
+    EXPECT_TRUE(url_info.has_value());
+    EXPECT_EQ("https://www.google.com/", url_info->url.spec());
+    if constexpr (kStringToUrlCoercionPopulatesTitle) {
+      EXPECT_EQ(u"www.google.com", url_info->title);
+    } else {
+      EXPECT_EQ(u"", url_info->title);
+    }
+  } else {
+    EXPECT_FALSE(url_info.has_value());
+  }
+}
+
+TEST_F(OSExchangeDataTest, NonHttpAndNonHttpsURLFromString) {
+  const OSExchangeData copy([] {
+    OSExchangeData data;
+    data.SetString(u"chrome://settings/");
+    return data.provider().Clone();
+  }());
+
+  EXPECT_TRUE(copy.HasURL(FilenameToURLPolicy::DO_NOT_CONVERT_FILENAMES));
+  std::optional<OSExchangeData::UrlInfo> url_info =
+      copy.GetURLAndTitle(FilenameToURLPolicy::DO_NOT_CONVERT_FILENAMES);
+  if constexpr (kSupportsStringToUrlCoercion) {
+    EXPECT_TRUE(url_info.has_value());
+    EXPECT_EQ("chrome://settings/", url_info->url.spec());
+    if constexpr (kStringToUrlCoercionPopulatesTitle) {
+      EXPECT_EQ(u"settings", url_info->title);
+    } else {
+      EXPECT_EQ(u"", url_info->title);
+    }
+  } else {
+    EXPECT_FALSE(url_info.has_value());
+  }
+}
+
+TEST_F(OSExchangeDataTest, NonHttpAndNonHttpsURLFromRendererTaintedString) {
+  const OSExchangeData copy([] {
+    OSExchangeData data;
+    data.SetString(u"chrome://settings/");
+    data.MarkRendererTaintedFromOrigin(url::Origin());
+    return data.provider().Clone();
+  }());
+
+  EXPECT_FALSE(copy.HasURL(FilenameToURLPolicy::DO_NOT_CONVERT_FILENAMES));
+  std::optional<OSExchangeData::UrlInfo> url_info =
+      copy.GetURLAndTitle(FilenameToURLPolicy::DO_NOT_CONVERT_FILENAMES);
+  EXPECT_FALSE(url_info.has_value());
+}
+
 // Test that setting the URL does not overwrite a previously set custom string
 // and that the synthesized URL shortcut file is ignored by GetFileContents().
 TEST_F(OSExchangeDataTest, URLStringFileContents) {