Merge 259353 "Mark drags starting in web content as tainted to a..."

> Mark drags starting in web content as tainted to avoid file path forgery
> 
> This patch takes the simplest possible approach and simply clears any
> filename data when the browser-side dragenter handler notices that a
> drag originated from a Chrome renderer. This breaks file:// URL dragging
> within Chrome, but it turns out this is already mostly broken anyway.
> Dragging file:// URLs is filtered out by FilterURL, since we don't
> GrantRequestSpecificFileURL to the renderer, so it generally ends up
> loading about:blank anyway.
> 
> The ChromeOS bits are left unimplemented for the moment. The specific
> security issues fixed by this patch don't presently affect Aura because
> it doesn't implement the DownloadURL protocol at all, and it doesn't
> get confused between URLs and filenames like Linux. While it would be
> nice to implement this for ChromeOS, doing so breaks drags from the
> File Manager app.
> 
> BUG=346135
> R=creis@chromium.org, erg@chromium.org, sky@chromium.org, tony@chromium.org, tsepez@chromium.org
> 
> Review URL: https://codereview.chromium.org/207013003

TBR=dcheng@chromium.org

Review URL: https://codereview.chromium.org/212693004

git-svn-id: svn://svn.chromium.org/chrome/branches/1847/src@260001 0039d316-1c4b-4281-b951-d872f2087c98

18 files changed, 119 insertions, 8 deletions

diff --git a/content/browser/renderer_host/render_view_host_impl.cc b/content/browser/renderer_host/render_view_host_impl.cc
index 889925f..a303029 100644
--- a/content/browser/renderer_host/render_view_host_impl.cc
+++ b/content/browser/renderer_host/render_view_host_impl.cc
@@ -833,6 +833,9 @@ void RenderViewHostImpl::DragTargetDragEnter(
   // and can't be interpreted as a capability.
   DropData filtered_data(drop_data);
   GetProcess()->FilterURL(true, &filtered_data.url);
+  if (drop_data.did_originate_from_renderer) {
+    filtered_data.filenames.clear();
+  }
 
   // The filenames vector, on the other hand, does represent a capability to
   // access the given files.
diff --git a/content/browser/web_contents/web_contents_view_aura.cc b/content/browser/web_contents/web_contents_view_aura.cc
index aa3be21..c11bf80 100644
--- a/content/browser/web_contents/web_contents_view_aura.cc
+++ b/content/browser/web_contents/web_contents_view_aura.cc
@@ -326,6 +326,7 @@ void PrepareDragForDownload(
 void PrepareDragData(const DropData& drop_data,
                      ui::OSExchangeData::Provider* provider,
                      WebContentsImpl* web_contents) {
+  provider->MarkOriginatedFromRenderer();
 #if defined(OS_WIN)
   // Put download before file contents to prefer the download of a image over
   // its thumbnail link.
@@ -366,6 +367,8 @@ void PrepareDragData(const DropData& drop_data,
 
 // Utility to fill a DropData object from ui::OSExchangeData.
 void PrepareDropData(DropData* drop_data, const ui::OSExchangeData& data) {
+  drop_data->did_originate_from_renderer = data.DidOriginateFromRenderer();
+
   base::string16 plain_text;
   data.GetString(&plain_text);
   if (!plain_text.empty())
diff --git a/content/browser/web_contents/web_drag_dest_gtk.cc b/content/browser/web_contents/web_drag_dest_gtk.cc
index d3b9ff8..0594a2e 100644
--- a/content/browser/web_contents/web_drag_dest_gtk.cc
+++ b/content/browser/web_contents/web_drag_dest_gtk.cc
@@ -121,7 +121,10 @@ gboolean WebDragDestGtk::OnDragMotion(GtkWidget* sender,
     // text/plain with file URLs when dragging files, we want to handle
     // text/uri-list after text/plain so that the plain text can be cleared if
     // it's a file drag.
+    // Similarly, renderer taint must occur before anything else so we can
+    // ignore potentially forged filenames when handling text/uri-list.
     static int supported_targets[] = {
+      ui::RENDERER_TAINT,
       ui::TEXT_PLAIN,
       ui::TEXT_URI_LIST,
       ui::TEXT_HTML,
@@ -182,7 +185,9 @@ void WebDragDestGtk::OnDragDataReceived(
   if (raw_data && data_length > 0) {
     // If the source can't provide us with valid data for a requested target,
     // raw_data will be NULL.
-    if (target == ui::GetAtomForTarget(ui::TEXT_PLAIN)) {
+    if (target == ui::GetAtomForTarget(ui::RENDERER_TAINT)) {
+      drop_data_->did_originate_from_renderer = true;
+    } else if (target == ui::GetAtomForTarget(ui::TEXT_PLAIN)) {
       guchar* text = gtk_selection_data_get_text(data);
       if (text) {
         drop_data_->text = base::NullableString16(
diff --git a/content/browser/web_contents/web_drag_dest_mac.mm b/content/browser/web_contents/web_drag_dest_mac.mm
index 8768bed..20a26f3 100644
--- a/content/browser/web_contents/web_drag_dest_mac.mm
+++ b/content/browser/web_contents/web_drag_dest_mac.mm
@@ -253,6 +253,9 @@ int GetModifierFlags() {
   DCHECK(pboard);
   NSArray* types = [pboard types];
 
+  data->did_originate_from_renderer =
+      [types containsObject:ui::kChromeDragDummyPboardType];
+
   // Get URL if possible. To avoid exposing file system paths to web content,
   // filenames in the drag are not converted to file URLs.
   ui::PopulateURLAndTitleFromPasteboard(&data->url,
diff --git a/content/browser/web_contents/web_drag_source_gtk.cc b/content/browser/web_contents/web_drag_source_gtk.cc
index a515e23..68a79f9 100644
--- a/content/browser/web_contents/web_drag_source_gtk.cc
+++ b/content/browser/web_contents/web_drag_source_gtk.cc
@@ -79,7 +79,7 @@ bool WebDragSourceGtk::StartDragging(const DropData& drop_data,
     return false;
   }
 
-  int targets_mask = 0;
+  int targets_mask = ui::RENDERER_TAINT;
 
   if (!drop_data.text.string().empty())
     targets_mask |= ui::TEXT_PLAIN;
@@ -285,6 +285,17 @@ void WebDragSourceGtk::OnDragDataGet(GtkWidget* sender,
       break;
     }
 
+    case ui::RENDERER_TAINT: {
+      static const char kPlaceholder[] = "x";
+      gtk_selection_data_set(
+          selection_data,
+          ui::GetAtomForTarget(ui::RENDERER_TAINT),
+          kBitsPerByte,
+          reinterpret_cast<const guchar*>(kPlaceholder),
+          strlen(kPlaceholder));
+      break;
+    }
+
     default:
       NOTREACHED();
   }
diff --git a/content/public/common/drop_data.cc b/content/public/common/drop_data.cc
index 3536efb..713751b 100644
--- a/content/public/common/drop_data.cc
+++ b/content/public/common/drop_data.cc
@@ -16,8 +16,8 @@ DropData::FileInfo::FileInfo(const base::string16& path,
 }
 
 DropData::DropData()
-    : referrer_policy(blink::WebReferrerPolicyDefault) {
-}
+    : did_originate_from_renderer(false),
+      referrer_policy(blink::WebReferrerPolicyDefault) {}
 
 DropData::~DropData() {
 }
diff --git a/content/public/common/drop_data.h b/content/public/common/drop_data.h
index 74ad79d..423be52 100644
--- a/content/public/common/drop_data.h
+++ b/content/public/common/drop_data.h
@@ -35,6 +35,9 @@ struct CONTENT_EXPORT DropData {
   DropData();
   ~DropData();
 
+  // Whether this drag originated from a renderer.
+  bool did_originate_from_renderer;
+
   // User is dragging a link into the webview.
   GURL url;
   base::string16 url_title;  // The title associated with |url|.
@@ -46,7 +49,9 @@ struct CONTENT_EXPORT DropData {
   // a download.
   blink::WebReferrerPolicy referrer_policy;
 
-  // User is dropping one or more files on the webview.
+  // User is dropping one or more files on the webview. This field is only
+  // populated if the drag is not renderer tainted, as this allows File access
+  // from web content.
   std::vector<FileInfo> filenames;
 
   // Isolated filesystem ID for the files being dragged on the webview.
diff --git a/ui/base/clipboard/clipboard_aurax11.cc b/ui/base/clipboard/clipboard_aurax11.cc
index 4588cc9..95b1cfd 100644
--- a/ui/base/clipboard/clipboard_aurax11.cc
+++ b/ui/base/clipboard/clipboard_aurax11.cc
@@ -762,8 +762,11 @@ void Clipboard::WriteBookmark(const char* title_data,
 // Write an extra flavor that signifies WebKit was the last to modify the
 // pasteboard. This flavor has no data.
 void Clipboard::WriteWebSmartPaste() {
-  aurax11_details_->InsertMapping(kMimeTypeWebkitSmartPaste,
-                                  scoped_refptr<base::RefCountedMemory>());
+  std::string empty;
+  aurax11_details_->InsertMapping(
+      kMimeTypeWebkitSmartPaste,
+      scoped_refptr<base::RefCountedMemory>(
+          base::RefCountedString::TakeString(&empty)));
 }
 
 void Clipboard::WriteBitmap(const SkBitmap& bitmap) {
diff --git a/ui/base/dragdrop/gtk_dnd_util.cc b/ui/base/dragdrop/gtk_dnd_util.cc
index 163dce4..1282723 100644
--- a/ui/base/dragdrop/gtk_dnd_util.cc
+++ b/ui/base/dragdrop/gtk_dnd_util.cc
@@ -55,6 +55,11 @@ void AddTargetToList(GtkTargetList* targets, int target_code) {
           ui::GetAtomForTarget(ui::CUSTOM_DATA), 0, ui::CUSTOM_DATA);
       break;
 
+    case ui::RENDERER_TAINT:
+      gtk_target_list_add(targets,
+          ui::GetAtomForTarget(ui::RENDERER_TAINT), 0, ui::RENDERER_TAINT);
+      break;
+
     default:
       NOTREACHED() << " Unexpected target code: " << target_code;
   }
@@ -114,6 +119,11 @@ GdkAtom GetAtomForTarget(int target) {
           kMimeTypeWebCustomData, false);
       return kCustomData;
 
+    case RENDERER_TAINT:
+      static const GdkAtom kRendererTaint = gdk_atom_intern(
+          "chromium/x-renderer-taint", false);
+      return kRendererTaint;
+
     default:
       NOTREACHED();
   }
diff --git a/ui/base/dragdrop/gtk_dnd_util.h b/ui/base/dragdrop/gtk_dnd_util.h
index b07a86d..33ee837 100644
--- a/ui/base/dragdrop/gtk_dnd_util.h
+++ b/ui/base/dragdrop/gtk_dnd_util.h
@@ -39,7 +39,11 @@ enum {
 
   // Custom data for web drag/drop.
   CUSTOM_DATA = 1 << 10,
-  INVALID_TARGET = 1 << 11,
+
+  // Tracks if the drag originated from the renderer.
+  RENDERER_TAINT = 1 << 11,
+
+  INVALID_TARGET = 1 << 12,
 };
 
 // Get the atom for a given target (of the above enum type). Will return NULL
diff --git a/ui/base/dragdrop/os_exchange_data.cc b/ui/base/dragdrop/os_exchange_data.cc
index 1b13daf..9cb0ffb 100644
--- a/ui/base/dragdrop/os_exchange_data.cc
+++ b/ui/base/dragdrop/os_exchange_data.cc
@@ -36,6 +36,14 @@ OSExchangeData::OSExchangeData(Provider* provider) : provider_(provider) {
 OSExchangeData::~OSExchangeData() {
 }
 
+void OSExchangeData::MarkOriginatedFromRenderer() {
+  provider_->MarkOriginatedFromRenderer();
+}
+
+bool OSExchangeData::DidOriginateFromRenderer() const {
+  return provider_->DidOriginateFromRenderer();
+}
+
 void OSExchangeData::SetString(const base::string16& data) {
   provider_->SetString(data);
 }
diff --git a/ui/base/dragdrop/os_exchange_data.h b/ui/base/dragdrop/os_exchange_data.h
index 7a968fa..3425ed2 100644
--- a/ui/base/dragdrop/os_exchange_data.h
+++ b/ui/base/dragdrop/os_exchange_data.h
@@ -102,6 +102,9 @@ class UI_BASE_EXPORT OSExchangeData {
 
     virtual Provider* Clone() const = 0;
 
+    virtual void MarkOriginatedFromRenderer() = 0;
+    virtual bool DidOriginateFromRenderer() const = 0;
+
     virtual void SetString(const base::string16& data) = 0;
     virtual void SetURL(const GURL& url, const base::string16& title) = 0;
     virtual void SetFilename(const base::FilePath& path) = 0;
@@ -163,6 +166,12 @@ class UI_BASE_EXPORT OSExchangeData {
   const Provider& provider() const { return *provider_; }
   Provider& provider() { return *provider_; }
 
+  // Marks drag data as tainted if it originates from the renderer. This is used
+  // to avoid granting privileges to a renderer when dragging in tainted data,
+  // since it could allow potential escalation of privileges.
+  void MarkOriginatedFromRenderer();
+  bool DidOriginateFromRenderer() const;
+
   // These functions add data to the OSExchangeData object of various Chrome
   // types. The OSExchangeData object takes care of translating the data into
   // a format suitable for exchange with the OS.
diff --git a/ui/base/dragdrop/os_exchange_data_provider_aura.cc b/ui/base/dragdrop/os_exchange_data_provider_aura.cc
index c629964..039d94f 100644
--- a/ui/base/dragdrop/os_exchange_data_provider_aura.cc
+++ b/ui/base/dragdrop/os_exchange_data_provider_aura.cc
@@ -33,6 +33,15 @@ OSExchangeData::Provider* OSExchangeDataProviderAura::Clone() const {
   return ret;
 }
 
+void OSExchangeDataProviderAura::MarkOriginatedFromRenderer() {
+  // TODO(dcheng): Currently unneeded because ChromeOS Aura correctly separates
+  // URL and filename metadata, and does not implement the DownloadURL protocol.
+}
+
+bool OSExchangeDataProviderAura::DidOriginateFromRenderer() const {
+  return false;
+}
+
 void OSExchangeDataProviderAura::SetString(const base::string16& data) {
   string_ = data;
   formats_ |= OSExchangeData::STRING;
diff --git a/ui/base/dragdrop/os_exchange_data_provider_aura.h b/ui/base/dragdrop/os_exchange_data_provider_aura.h
index 6ef93ca..c656c01 100644
--- a/ui/base/dragdrop/os_exchange_data_provider_aura.h
+++ b/ui/base/dragdrop/os_exchange_data_provider_aura.h
@@ -27,6 +27,8 @@ class UI_BASE_EXPORT OSExchangeDataProviderAura
 
   // Overridden from OSExchangeData::Provider:
   virtual Provider* Clone() const OVERRIDE;
+  virtual void MarkOriginatedFromRenderer() OVERRIDE;
+  virtual bool DidOriginateFromRenderer() const OVERRIDE;
   virtual void SetString(const base::string16& data) OVERRIDE;
   virtual void SetURL(const GURL& url, const base::string16& title) OVERRIDE;
   virtual void SetFilename(const base::FilePath& path) OVERRIDE;
diff --git a/ui/base/dragdrop/os_exchange_data_provider_aurax11.cc b/ui/base/dragdrop/os_exchange_data_provider_aurax11.cc
index ba477f6..a71aa11 100644
--- a/ui/base/dragdrop/os_exchange_data_provider_aurax11.cc
+++ b/ui/base/dragdrop/os_exchange_data_provider_aurax11.cc
@@ -25,6 +25,7 @@ namespace ui {
 namespace {
 
 const char kDndSelection[] = "XdndSelection";
+const char kRendererTaint[] = "chromium/x-renderer-taint";
 
 const char* kAtomsToCache[] = {
   kString,
@@ -34,6 +35,7 @@ const char* kAtomsToCache[] = {
   Clipboard::kMimeTypeURIList,
   kMimeTypeMozillaURL,
   Clipboard::kMimeTypeText,
+  kRendererTaint,
   NULL
 };
 
@@ -108,6 +110,18 @@ OSExchangeData::Provider* OSExchangeDataProviderAuraX11::Clone() const {
   return ret;
 }
 
+void OSExchangeDataProviderAuraX11::MarkOriginatedFromRenderer() {
+  std::string empty;
+  format_map_.Insert(atom_cache_.GetAtom(kRendererTaint),
+                     scoped_refptr<base::RefCountedMemory>(
+                         base::RefCountedString::TakeString(&empty)));
+}
+
+bool OSExchangeDataProviderAuraX11::DidOriginateFromRenderer() const {
+  return format_map_.find(atom_cache_.GetAtom(kRendererTaint)) !=
+         format_map_.end();
+}
+
 void OSExchangeDataProviderAuraX11::SetString(const base::string16& text_data) {
   std::string utf8 = base::UTF16ToUTF8(text_data);
   scoped_refptr<base::RefCountedMemory> mem(
diff --git a/ui/base/dragdrop/os_exchange_data_provider_aurax11.h b/ui/base/dragdrop/os_exchange_data_provider_aurax11.h
index 5107c43..6f99dc1 100644
--- a/ui/base/dragdrop/os_exchange_data_provider_aurax11.h
+++ b/ui/base/dragdrop/os_exchange_data_provider_aurax11.h
@@ -58,6 +58,8 @@ class UI_BASE_EXPORT OSExchangeDataProviderAuraX11
 
   // Overridden from OSExchangeData::Provider:
   virtual Provider* Clone() const OVERRIDE;
+  virtual void MarkOriginatedFromRenderer() OVERRIDE;
+  virtual bool DidOriginateFromRenderer() const OVERRIDE;
   virtual void SetString(const base::string16& data) OVERRIDE;
   virtual void SetURL(const GURL& url, const base::string16& title) OVERRIDE;
   virtual void SetFilename(const base::FilePath& path) OVERRIDE;
diff --git a/ui/base/dragdrop/os_exchange_data_provider_win.cc b/ui/base/dragdrop/os_exchange_data_provider_win.cc
index ac0776e..0e441d0 100644
--- a/ui/base/dragdrop/os_exchange_data_provider_win.cc
+++ b/ui/base/dragdrop/os_exchange_data_provider_win.cc
@@ -23,6 +23,14 @@
 
 namespace ui {
 
+static const OSExchangeData::CustomFormat& GetRendererTaintCustomType() {
+  CR_DEFINE_STATIC_LOCAL(
+      ui::OSExchangeData::CustomFormat,
+      format,
+      (ui::Clipboard::GetFormatType("chromium/x-renderer-taint")));
+  return format;
+}
+
 // Creates a new STGMEDIUM object to hold the specified text. The caller
 // owns the resulting object. The "Bytes" version does not NULL terminate, the
 // string version does.
@@ -269,6 +277,16 @@ OSExchangeData::Provider* OSExchangeDataProviderWin::Clone() const {
   return new OSExchangeDataProviderWin(data_object());
 }
 
+void OSExchangeDataProviderWin::MarkOriginatedFromRenderer() {
+  STGMEDIUM* storage = GetStorageForString(std::string());
+  data_->contents_.push_back(new DataObjectImpl::StoredDataInfo(
+      GetRendererTaintCustomType().ToFormatEtc(), storage));
+}
+
+bool OSExchangeDataProviderWin::DidOriginateFromRenderer() const {
+  return HasCustomFormat(GetRendererTaintCustomType());
+}
+
 void OSExchangeDataProviderWin::SetString(const base::string16& data) {
   STGMEDIUM* storage = GetStorageForString(data);
   data_->contents_.push_back(new DataObjectImpl::StoredDataInfo(
diff --git a/ui/base/dragdrop/os_exchange_data_provider_win.h b/ui/base/dragdrop/os_exchange_data_provider_win.h
index 79a8fdb..98988b6 100644
--- a/ui/base/dragdrop/os_exchange_data_provider_win.h
+++ b/ui/base/dragdrop/os_exchange_data_provider_win.h
@@ -149,6 +149,8 @@ class UI_BASE_EXPORT OSExchangeDataProviderWin
 
   // OSExchangeData::Provider methods.
   virtual Provider* Clone() const;
+  virtual void MarkOriginatedFromRenderer();
+  virtual bool DidOriginateFromRenderer() const;
   virtual void SetString(const base::string16& data);
   virtual void SetURL(const GURL& url, const base::string16& title);
   virtual void SetFilename(const base::FilePath& path);