Last Comment Bug 605991 - Drag-and-drop may be used to steal content across domains
: Drag-and-drop may be used to steal content across domains
Status: RESOLVED FIXED
[sg:moderate]
:
Product: Core
Classification: Components
Component: Drag and Drop (show other bugs)
: Trunk
: All All
: -- normal with 7 votes (vote)
: mozilla14
Assigned To: Neil Deakin (away until Oct 3)
:
Mentors:
Depends on: 775110 845194 851674
Blocks: 639796
  Show dependency treegraph
 
Reported: 2010-10-20 15:14 PDT by Michal Zalewski
Modified: 2015-05-05 23:01 PDT (History)
29 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+


Attachments
patch (5.48 KB, patch)
2011-11-22 17:05 PST, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
Patch 2 (18.12 KB, patch)
2012-02-10 06:19 PST, Neil Deakin (away until Oct 3)
bugs: review-
Details | Diff | Splinter Review
updated patch 3 (16.29 KB, patch)
2012-03-13 13:19 PDT, Neil Deakin (away until Oct 3)
bugs: review+
Ms2ger: feedback-
Details | Diff | Splinter Review

Description Michal Zalewski 2010-10-20 15:14:24 PDT
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 2010-10-20 15:23:03 PDT
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) 2010-10-20 15:47:02 PDT
(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 :Ehsan Akhgari 2010-11-02 13:16:03 PDT
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] (TPAC) 2010-11-02 14:56:15 PDT
Yeah, I think we could do that pretty easily.
Neil would know better, though.
Comment 5 Neil Deakin (away until Oct 3) 2010-11-02 16:54:16 PDT
(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 2010-11-02 17:22:29 PDT
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 :Ehsan Akhgari 2010-11-03 08:18:48 PDT
(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 2010-11-03 10:34:18 PDT
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 2010-11-12 10:31:49 PST
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) 2010-12-12 07:11:11 PST
Blocking on this until it's figured out.
Comment 11 Neil Deakin (away until Oct 3) 2011-11-22 17:05:20 PST
Created attachment 576355 [details] [diff] [review]
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 (away until Oct 3) 2011-11-22 17:07:42 PST
I wrote a manual test for this which I can attach to the bug if desired.
Comment 13 Neil Deakin (away until Oct 3) 2011-11-23 06:53:56 PST
Test builds are at https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/neil@mozilla.com-4cfbdd77a436/
Comment 14 Brandon Sterne (:bsterne) 2011-12-20 16:04:09 PST
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 (away until Oct 3) 2011-12-20 17:18:36 PST
Can you provide the testcase?
Comment 16 Masatoshi Kimura [:emk] 2011-12-20 17:39:15 PST
> - data urls are excepted from this. The webkit patch seems to have this check.
What about blob urls?
Comment 17 Brandon Sterne (:bsterne) 2011-12-22 14:33:02 PST
(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 (away until Oct 3) 2011-12-23 07:27:25 PST
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 2012-01-10 13:45:02 PST
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 [:bz] (TPAC) 2012-01-10 13:49:33 PST
What does view-source have to do with this?  That's the first mention of view-source in this bug....
Comment 21 loftwing 2012-01-10 13:54:54 PST
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 2012-01-17 13:39:57 PST
Updates or thoughts on the view-source approach or the initial fix? Thanks.
Comment 23 loftwing 2012-02-08 17:29:12 PST
Ping Boris, Neil, Michal - Thanks
Comment 24 Boris Zbarsky [:bz] (TPAC) 2012-02-08 18:50:25 PST
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 (away until Oct 3) 2012-02-10 06:19:46 PST
Created attachment 596035 [details] [diff] [review]
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 (away until Oct 3) 2012-02-10 06:22:16 PST
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 2012-02-10 08:42:25 PST
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] (TPAC) 2012-02-20 06:55:34 PST
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 (away until Oct 3) 2012-02-23 08:09:45 PST
(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 2012-03-13 12:55:00 PDT
Hey Neil, any chance we can get the latest diff up for review/commit?
Comment 31 Neil Deakin (away until Oct 3) 2012-03-13 13:19:49 PDT
Created attachment 605513 [details] [diff] [review]
updated patch 3
Comment 32 Olli Pettay [:smaug] (TPAC) 2012-03-16 12:19:22 PDT
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) 2012-03-21 03:53:14 PDT
https://hg.mozilla.org/mozilla-central/rev/3b64dbdc8d39
Comment 34 :Ms2ger (⌚ UTC+1/+2) 2012-03-21 04:07:37 PDT
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 2012-04-20 11:23:18 PDT
Ping - Any updates? Thanks, Facebook
Comment 36 loftwing 2012-04-20 11:24:23 PDT
NVM, mozilla14

Note You need to log in before you can comment on or make changes to this bug.