Mark drags starting in web content as tainted to avoid file path forgery

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

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259353

[dcheng, 2014-03-21 22:48:08]

creis: security perspective and content OWNER.
tony: overall patch
erg: *aura*, *gtk*, and *x11* bits. I've made some minor changes to avoid 
crashing in OnSelectionRequest if someone does try to request the data
associated with the type.

I'm not 100% sure this is the right approach. In particular, I'm concerned about
CrOS because I noticed the CrOS file manager writes data to the clipboard, and I
don't see a reason why web content can't forge those entries as well:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

An alternative approach is to just add a renderer_taint field to
content::DropData and skip all the privilege granting in
RenderViewHostImpl::DragTargetDragEnter when tainted. But we need to some way to
avoid breaking the CrOS file manager in that case...

[tony, 2014-03-21 23:08:59]

What are your test plans?  If we can't do an automated test, can we check in a
manual test?

https://codereview.chromium.org/207013003/diff/80002/ui/base/clipboard/clipbo...
File ui/base/clipboard/clipboard_aurax11.cc (right):

https://codereview.chromium.org/207013003/diff/80002/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_aurax11.cc:769:
base::RefCountedString::TakeString(&empty)));
This doesn't seem related to the bug fix.  If you want to include it, mention it
in the change description.

Why do you need to pass the empty string?

https://codereview.chromium.org/207013003/diff/80002/ui/base/dragdrop/os_exch...
File ui/base/dragdrop/os_exchange_data.h (right):

https://codereview.chromium.org/207013003/diff/80002/ui/base/dragdrop/os_exch...
ui/base/dragdrop/os_exchange_data.h:106: virtual bool IsRendererTainted() const
= 0;
Nit: I would name these something more explicit.  E.g.,
MarkOriginateFromRenderer(), DoesOriginateFromRenderer() or something.

https://codereview.chromium.org/207013003/diff/80002/ui/base/dragdrop/os_exch...
File ui/base/dragdrop/os_exchange_data_provider_aura.cc (right):

https://codereview.chromium.org/207013003/diff/80002/ui/base/dragdrop/os_exch...
ui/base/dragdrop/os_exchange_data_provider_aura.cc:38: // URL and filename
metadata, and does not implement the DownloadURL protocol.
I worry that someone will implement DownloadURL and not know to implement this.

https://codereview.chromium.org/207013003/diff/80002/ui/base/dragdrop/os_exch...
File ui/base/dragdrop/os_exchange_data_provider_aurax11.cc (right):

https://codereview.chromium.org/207013003/diff/80002/ui/base/dragdrop/os_exch...
ui/base/dragdrop/os_exchange_data_provider_aurax11.cc:122: format_map_.end();
Nit: Is this the correct indent?  I thought it was always 4 spaces.

[Charlie Reis, 2014-03-21 23:30:03]

Hmm, I'm not sure I'm a good reviewer for this.  Certainly not for the security
perspective, since I don't know enough about drag and drop.  At most I can give
an owner's rubber stamp once it's been reviewed.

[dcheng, 2014-03-21 23:57:06]

creis: Unfortunately, there's not a lot of people familiar with this portion of
the code. I've added tsepez@ as a second pair of security eyes to review this,
and I'll ask you for an OWNERS stamp when it's time =)

tsepez: Do you mind doing an overall security review? I'm still researching the
File Manager case for CrOS... hopefully I'll figure something out.

https://codereview.chromium.org/207013003/diff/80002/ui/base/clipboard/clipbo...
File ui/base/clipboard/clipboard_aurax11.cc (right):

https://codereview.chromium.org/207013003/diff/80002/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_aurax11.cc:769:
base::RefCountedString::TakeString(&empty)));
On 2014/03/21 23:09:00, tony wrote:
> This doesn't seem related to the bug fix.  If you want to include it, mention
it
> in the change description.
> 
> Why do you need to pass the empty string?

It's not directly related. I can pull it out--I did this for consistency, since
this was the code I originally copied in the Aura X11 OSExchangeData::Provider
implementation.

You need to pass something or we unconditionally try to deref the
RefCountedMemory in OnSelectionRequest, which crashes if the pointer is null
like here.

https://codereview.chromium.org/207013003/diff/80002/ui/base/dragdrop/os_exch...
File ui/base/dragdrop/os_exchange_data.h (right):

https://codereview.chromium.org/207013003/diff/80002/ui/base/dragdrop/os_exch...
ui/base/dragdrop/os_exchange_data.h:106: virtual bool IsRendererTainted() const
= 0;
On 2014/03/21 23:09:00, tony wrote:
> Nit: I would name these something more explicit.  E.g.,
> MarkOriginateFromRenderer(), DoesOriginateFromRenderer() or something.

Done.

https://codereview.chromium.org/207013003/diff/80002/ui/base/dragdrop/os_exch...
File ui/base/dragdrop/os_exchange_data_provider_aura.cc (right):

https://codereview.chromium.org/207013003/diff/80002/ui/base/dragdrop/os_exch...
ui/base/dragdrop/os_exchange_data_provider_aura.cc:38: // URL and filename
metadata, and does not implement the DownloadURL protocol.
On 2014/03/21 23:09:00, tony wrote:
> I worry that someone will implement DownloadURL and not know to implement
this.

Implementing this breaks dragging and dropping files from the file manager.
Unfortunately, it looks like all the engineers familiar with this bit of the
code are in Tokyo. I'm looking for if there's someway the browser process can
skip the tainting for the file manager (it looks like it's an app), but I'm not
sure what a secure way to do this is yet--presumably, the render process hosting
it can get shared...

https://codereview.chromium.org/207013003/diff/80002/ui/base/dragdrop/os_exch...
File ui/base/dragdrop/os_exchange_data_provider_aurax11.cc (right):

https://codereview.chromium.org/207013003/diff/80002/ui/base/dragdrop/os_exch...
ui/base/dragdrop/os_exchange_data_provider_aurax11.cc:122: format_map_.end();
On 2014/03/21 23:09:00, tony wrote:
> Nit: Is this the correct indent?  I thought it was always 4 spaces.

This is what clang-format said. I didn't clang format the entire CL though, as
it results in some more radical changes that would be better to avoid for ease
of merges.

[Tom Sepez, 2014-03-22 00:07:49]

I'll take a more thorough look on monday.  I didn't see any obvious problem with
a first cursory inspection.

[tony, 2014-03-22 00:15:05]

LGTM with tom's review and with some testing plan (at a minimum "see bug for
test case", but something checked in or automated would be better).

https://codereview.chromium.org/207013003/diff/80002/ui/base/clipboard/clipbo...
File ui/base/clipboard/clipboard_aurax11.cc (right):

https://codereview.chromium.org/207013003/diff/80002/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_aurax11.cc:769:
base::RefCountedString::TakeString(&empty)));
On 2014/03/21 23:57:07, dcheng wrote:
> You need to pass something or we unconditionally try to deref the
> RefCountedMemory in OnSelectionRequest, which crashes if the pointer is null
> like here.

This is fine, but please mention this in the change description.

[Elliot Glaysher, 2014-03-22 00:29:59]

lgtm. this looks fairly sane.

You mentioned over IM that this caused some issues with drags of files on Linux.
Could you please document that somewhere?

[dcheng, 2014-03-22 00:32:46]

On 2014/03/22 00:29:59, Elliot Glaysher wrote:
> lgtm. this looks fairly sane.
> 
> You mentioned over IM that this caused some issues with drags of files on
Linux.
> Could you please document that somewhere?

Actually, it's not just Linux, it's all platforms. If you go to
file://path/to/homedir and try to drag a link there to any other browser window
that's not already at a file:// URL, we'll silently redirect it to about:blank.
I'm making a few more adjustments to this patch and I'll update the description
as appropriate.

[Charlie Reis, 2014-03-22 00:36:11]

On 2014/03/22 00:32:46, dcheng wrote:
> On 2014/03/22 00:29:59, Elliot Glaysher wrote:
> > lgtm. this looks fairly sane.
> > 
> > You mentioned over IM that this caused some issues with drags of files on
> Linux.
> > Could you please document that somewhere?
> 
> Actually, it's not just Linux, it's all platforms. If you go to
> file://path/to/homedir and try to drag a link there to any other browser
window
> that's not already at a file:// URL, we'll silently redirect it to
about:blank.
> I'm making a few more adjustments to this patch and I'll update the
description
> as appropriate.

That sounds like FilterURL is catching it and rewriting it to about:blank.  See
https://code.google.com/p/chromium/issues/detail?id=243679.

[dcheng, 2014-03-24 18:01:29]

I experimented with some different approaches for ChromeOS, but I was unable to
find a good approach that I was happy with.

The fundamental issue is I have no way of identifying drags from the file
manager and avoid the filename filtering for those drags. I tried implementing a
heuristic of "if the renderer already has access to all these files, then don't
flag it as renderer originated". I didn't like that approach because hostile web
content could probably cause things that should be flagged to be skipped.

[Tom Sepez, 2014-03-24 20:55:19]

lgtm

[dcheng, 2014-03-24 21:09:49]

On 2014/03/24 20:55:19, Tom Sepez wrote:
> lgtm

I'm going to land this as is for now since it needs to be merged to a release
branch. I'll continue hacking on tests outside of this patch.

[dcheng, 2014-03-24 21:10:04]

+sky for OWNERS approval for ui/

[Charlie Reis, 2014-03-24 21:19:09]

Rubber stamp LGTM for content/.

https://codereview.chromium.org/207013003/diff/90022/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/207013003/diff/90022/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:752: if
(drop_data.did_originate_from_renderer) {
nit: No need for braces here.

[sky, 2014-03-25 16:31:01]

LGTM

[dcheng, 2014-03-25 18:11:15]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-03-25 18:11:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/207013003/90022

[commit-bot: I haz the power, 2014-03-25 18:19:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 18:19:32]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[dcheng, 2014-03-25 22:04:11]

Committed patchset #10 manually as r259353 (presubmit successful).