Closed Bug 605991 Opened 14 years ago Closed 12 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14
Tracking Status
blocking2.0 --- .x+

People

(Reporter: lcamtuf, Assigned: enndeakin)

References

(Depends on 2 open bugs)

Details

(Whiteboard: [sg:moderate])

Attachments

(1 file, 2 obsolete files)

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
I'd like to know how https://developer.mozilla.org/en/Security/CSP/Default_CSP_restrictions in Firefox 4 mitigates this, if at all.
Whiteboard: [sg:moderate]
blocking2.0: --- → ?
(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.
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?
Yeah, I think we could do that pretty easily.
Neil would know better, though.
(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)?
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.
(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.
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...
You may be interested to know that a similar change is now committed in WebKit:

http://trac.webkit.org/changeset/71925
Blocking on this until it's figured out.
blocking2.0: ? → final+
blocking2.0: final+ → .x
Component: Security → Drag and Drop
Product: Firefox → Core
QA Contact: firefox → drag-drop
Version: 3.6 Branch → unspecified
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
I wrote a manual test for this which I can attach to the bug if desired.
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.
Can you provide the testcase?
> - data urls are excepted from this. The webkit patch seems to have this check.
What about blob urls?
(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.
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?
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)?
What does view-source have to do with this?  That's the first mention of view-source in this bug....
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.
Updates or thoughts on the view-source approach or the initial fix? Thanks.
Ping Boris, Neil, Michal - Thanks
I think blocking cross-domain drags makes more sense than blocking view-source in subframes, personally....  it may also be simpler to implement.
Attached patch Patch 2 (obsolete) — Splinter Review
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.
Attachment #596035 - Flags: review?(bugs)
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.
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 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?
Attachment #596035 - Flags: review?(bugs) → review-
(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.
Hey Neil, any chance we can get the latest diff up for review/commit?
Attached patch updated patch 3Splinter Review
Attachment #576355 - Attachment is obsolete: true
Attachment #596035 - Attachment is obsolete: true
Attachment #605513 - Flags: review?(bugs)
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;
}
Attachment #605513 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/3b64dbdc8d39
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Version: unspecified → Trunk
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
Attachment #605513 - Flags: feedback-
Ping - Any updates? Thanks, Facebook
NVM, mozilla14
Depends on: 775110
Depends on: 845194
Depends on: 851674
See Also: → 1352840
You need to log in before you can comment on or make changes to this bug.