605991 - Drag-and-drop may be used to steal content across domains

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Michal Zalewski]

Hi folks,

As discussed by several researchers in the past, it is fairly easy to use CSS overlays and onmousemove tracking to trick the user into selecting text in an invisible IFRAME pointing to trusted-application.com; and then have him drag-and-drop it into a container on the top-level, malicious page.

The canonical example of a method to exploit this is interacting with faux scrollbars; or performing drag-and-drop-like operations in a web game, or a drawing application. I discuss the specifics here:

http://lcamtuf.blogspot.com/2010/10/attack-of-monster-frames-mini.html

(Start from "Since then, the situation with framed content"...)

I have seen privately shared proof-of-concepts exploits for this, and have been shown at least one in-the-wild rogue Facebook application that operated in a similar way - although I don't have any pointers handy at this point.

The risk is weakly mitigated by X-Frame-Options, but only for certain classes of web apps, and only on an opt-in basis. This is probably insufficient. We're currently siding with a view that this is best prevented by implementing one of the following options:

1) Preventing drag-and-drop if source frame is non-same-origin with destination window / frame (cross-domain drag-and-drop between top-level windows is probably OK). I believe this is unlikely to break legitimate uses of the browser.

2) If this is not feasible or too risky, requiring a secondary gesture to complete such drag-and-drop. For example, when the content hovers over non-same-origin destination, we could display a hovering tooltip saying "Hold shift to drop selection here".

Cheers,
/mz

Comment 1

[Mardeg]

I'd like to know how https://developer.mozilla.org/en/Security/CSP/Default_CSP_restrictions in Firefox 4 mitigates this, if at all.

Comment 2

[Brandon Sterne (:bsterne)]

(In reply to comment #0)
> the following options:
> 
> 1) Preventing drag-and-drop if source frame is non-same-origin with destination
> window / frame (cross-domain drag-and-drop between top-level windows is
> probably OK). I believe this is unlikely to break legitimate uses of the
> browser.

I think this is entirely reasonable, for what it's worth.

(In reply to comment #1)
> I'd like to know how
> https://developer.mozilla.org/en/Security/CSP/Default_CSP_restrictions in
> Firefox 4 mitigates this, if at all.

I don't think CSP really helps us here.  CSP restricts where content can load from and prevents inline scripts from running, but it would be the attacker page's script that would do stuff with the dropped content so he wouldn't opt-in to CSP, and for the target page CSP does nothing to prevent text selection.

Comment 3

[(no longer active)]

Hmm, nsContentAreaDragDrop seems to be adding every piece of drag data with the node's associated principal, so we should be able to do a cross-domain check before dispatching drop events to the target node, right?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Yeah, I think we could do that pretty easily.
Neil would know better, though.

Comment 5

[Neil Deakin]

(In reply to comment #0)

> 1) Preventing drag-and-drop if source frame is non-same-origin with destination
> window / frame (cross-domain drag-and-drop between top-level windows is
> probably OK). I believe this is unlikely to break legitimate uses of the
> browser.

Do you mean prevent a drop when the data being dragged was from somewhere specifically loaded in a child frame (as opposed to something not loaded in a frame)?

Comment 6

[Michal Zalewski]

Yes, specifically loaded in a child frame that is not same origin with the receiving context (window / frame).

Drag-and-drop across top-level documents in different domains is probably fine.

Comment 7

[(no longer active)]

(In reply to comment #6)
> Yes, specifically loaded in a child frame that is not same origin with the
> receiving context (window / frame).
> 
> Drag-and-drop across top-level documents in different domains is probably fine.

I don't think so.  Why?  The only difference there is that spoofing won't really work.

Comment 8

[Michal Zalewski]

Well, with drag-and-drop across top-level windows, the user knows what the origin and the destination are. When dragging something out of an IFRAME, this is not evident. That said, if you want to nuke both options, fine by me...

Comment 9

[Michal Zalewski]

You may be interested to know that a similar change is now committed in WebKit:

http://trac.webkit.org/changeset/71925

Comment 10

[Dietrich Ayala (:dietrich)]

Blocking on this until it's figured out.

Comment 11

[Neil Deakin]

Attachment: patch

This patch:

- changes nothing for privileged callers
- if a drop occurs on a page, but the source of the drag is from a descendant frame, a principal comparison is made before allowing access to the dropped data.
- data urls are excepted from this. The webkit patch seems to have this check.
- prevents changing the source drag target to something from a different window.

Let me know if this isn't actually what is expected of this bug. The patch is more complex than I'd like so if a simpler check is possible, that would be good.

Comment 12

[Neil Deakin]

I wrote a manual test for this which I can attach to the bug if desired.

Comment 13

[Neil Deakin]

Test builds are at https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4cfbdd77a436/

Comment 14

[Brandon Sterne (:bsterne)]

Hey Neil, any chance I can get you to push this to Try again?  My contacts at Facebook said the builds you made didn't fix the testcase they were using (a copy of which I now have). I'm happy to dig in a bit if you'd be so kind as to re-push the patch to try.  Thanks.

Comment 15

[Neil Deakin]

Can you provide the testcase?

Comment 16

[Masatoshi Kimura [:emk]]

> - data urls are excepted from this. The webkit patch seems to have this check.
What about blob urls?

Comment 17

[Brandon Sterne (:bsterne)]

(In reply to Neil Deakin from comment #15)
> Can you provide the testcase?

I'll email it to you.  They don't want the testcase to be public.

Comment 18

[Neil Deakin]

OK, so this bug is expecting even privileged callers to be blocked from getting the data?

Right now the patch is using nsContentUtils::GetWindowFromCaller() to get the target window, but that, I assume, may not actually be the window that was dropped on. Is there a way to get the target of the drop event from nsDOMDataTransfer::MozGetDataAt where we don't have access to the event? Or perhaps it would be better to check for subframes earlier when we initialize?

Comment 19

[loftwing]

Any updates on this? You can still drag HTML content from an iframe to the parent page (which is on a different origin). If this fix is proving to be difficult, perhaps you can take the another approach in the mean time, like blocking iframe view-source (Webkit/Chrome/Safari do this)?

Comment 20

[Boris Zbarsky [:bzbarsky]]

What does view-source have to do with this?  That's the first mention of view-source in this bug....

Comment 21

[loftwing]

view-source makes cross-origin drag-n-drop more exploitable, allowing for the theft of CSRF tokens or other sensitive elements in the DOM. Nearly every cross-origin drag-n-drop PoC I have seen makes use of view-source, ex: http://blog.kotowicz.net/2011/07/cross-domain-content-extraction-with.html.

Comment 22

[loftwing]

Updates or thoughts on the view-source approach or the initial fix? Thanks.

Comment 23

[loftwing]

Ping Boris, Neil, Michal - Thanks

Comment 24

[Boris Zbarsky [:bzbarsky]]

I think blocking cross-domain drags makes more sense than blocking view-source in subframes, personally....  it may also be simpler to implement.

Comment 25

[Neil Deakin]

Attachment: Patch 2

This patch does the same as the previous but also prevents privileged callers from retrieving the dropped data being dropped onto a content page. This handles dropping on an editable area. It does not change any behaviour when dropping onto a chrome docshell.

Comment 26

[Neil Deakin]

Note that the editor portions of the patch will need to be changed for 499008 in the future, and the recently checked in bug 707382 will affect the patch slightly on any branches that don't have it.

Comment 27

[Michal Zalewski]

View-source: is convenient, but there are many ways to achieve roughly the same effect (rich text, including forms and secret-bearing links, can be dragged and dropped, too). IIRC, Chrome disallows cross-origin drag and drop for a longer while, with no issues.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 596035 [details] [diff] [review]
Patch 2


>+nsContentUtils::CheckForSubFrameDrop(nsIDragSession* aDragSession, nsDragEvent* aDropEvent)
>+{
>+  nsCOMPtr<nsIDocShellTreeItem> tdsti;
>+  nsCOMPtr<nsIContent> target = do_QueryInterface(aDropEvent->originalTarget);
>+  if (target && target->OwnerDoc()) {
>+    nsCOMPtr<nsIWebNavigation> twebnav = do_GetInterface(target->OwnerDoc()->GetWindow());
>+    tdsti = do_QueryInterface(twebnav);
>+  }
>+
>+  if (!tdsti)
>+    return true;
if (expr) {
  stmt;
}
Same also elsewhere.


>+
>+  PRInt32 type = -1;
>+  if (NS_FAILED(tdsti->GetItemType(&type)))
>+    return true;
>+
>+  // Always allow dropping onto chrome shells.
>+  if (type == nsIDocShellTreeItem::typeChrome)
>+    return false;
>+
>+  // If there is no source node, then this is a drag from another
>+  // application, which should be allowed.
>+  nsCOMPtr<nsIDOMDocument> sourceDocument;
>+  aDragSession->GetSourceDocument(getter_AddRefs(sourceDocument));
>+  nsCOMPtr<nsIDocument> doc = do_QueryInterface(sourceDocument);
>+  if (doc) {
>+    // Allow data uris to be used regardless.
>+    bool isDataURI = false;
>+    nsIURI* sourceURI = doc->GetDocumentURI();
>+    if (sourceURI)
>+      sourceURI->SchemeIs("data", &isDataURI);
>+
>+    if (!isDataURI) {
I don't understand why data url can be skipped.

>+      do {
>+        nsCOMPtr<nsIDocShellTreeItem> parentDsti;
>+        sdsti->GetParent(getter_AddRefs(parentDsti));
>+        sdsti.swap(parentDsti);
>+
>+        if (tdsti == sdsti) {
>+          // The drag is from a child frame.
>+          return true;
>+        }
>+      } while (sdsti);
Perhaps use nsIDocument::GetParentDocument(). It is faster and easier to use.

>+
>+  // Make sure that we only allow setting the element to something from
>+  // the same window.
Why this limitation? Wouldn't a principal check work here?

Comment 29

[Neil Deakin]

(In reply to Olli Pettay [:smaug] from comment #28)
> I don't understand why data url can be skipped.

I think webkit had such a check, but I'll remove this.


> >+  // Make sure that we only allow setting the element to something from
> >+  // the same window.
> Why this limitation? Wouldn't a principal check work here?

I think I added this because calling addElement will change the source document, allowing one to override the frame checks by simply popping up another window and using a node from there.

Comment 30

[loftwing]

Hey Neil, any chance we can get the latest diff up for review/commit?

Comment 31

[Neil Deakin]

Attachment: updated patch 3

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 605513 [details] [diff] [review]
updated patch 3


>+  nsCOMPtr<nsIPrivateDOMEvent> privateEvent(do_QueryInterface(aDropEvent));
>+  nsDragEvent* dragEventInternal = static_cast<nsDragEvent *>(privateEvent->GetInternalNSEvent());
>+  if (nsContentUtils::CheckForSubFrameDrop(dragSession, dragEventInternal))
>+    return NS_OK;
>+

if (expr) {
  stmt;
}

Comment 33

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/3b64dbdc8d39

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 605513 [details] [diff] [review]
updated patch 3

Review of attachment 605513 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsContentUtils.cpp
@@ +5255,5 @@
>  bool
> +nsContentUtils::CheckForSubFrameDrop(nsIDragSession* aDragSession, nsDragEvent* aDropEvent)
> +{
> +  nsCOMPtr<nsIContent> target = do_QueryInterface(aDropEvent->originalTarget);
> +  if (!target && !target->OwnerDoc()) {

Consider what happens if target is indeed null. Also, OwnerDoc never returns null.

::: content/events/src/nsDOMDataTransfer.cpp
@@ +653,4 @@
>    if (mReadOnly)
>      return NS_ERROR_DOM_NO_MODIFICATION_ALLOWED_ERR;
>  
>    mDragTarget = do_QueryInterface(aElement);

This QI isn't necessary

Comment 35

[loftwing]

Ping - Any updates? Thanks, Facebook

Comment 36

[loftwing]

NVM, mozilla14