Last Comment Bug 572129 - (CVE-2011-2984) Arbitrary code execution via browser's tab element that was dropped on the content area
(CVE-2011-2984)
: Arbitrary code execution via browser's tab element that was dropped on the co...
Status: RESOLVED FIXED
[sg:critical][stale][ccbr][partial fi...
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: unspecified
: x86 Windows XP
-- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug] (pto-ish for couple of days)
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
Depends on: 661970 572647 611485
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-15 08:32 PDT by moz_bug_r_a4
Modified: 2011-11-09 01:17 PST (History)
21 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.20+
.20-fixed
needed
wanted


Attachments
Possible fix (1.18 KB, patch)
2011-02-14 19:32 PST, Blake Kaplan (:mrbkap)
enndeakin: review-
Details | Diff | Splinter Review
Possible fix v2 (2.58 KB, patch)
2011-02-22 16:57 PST, Blake Kaplan (:mrbkap)
enndeakin: review-
Details | Diff | Splinter Review
Be very strict when handling common DOM objects as transfer data (6.71 KB, patch)
2011-02-23 11:37 PST, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
v2 (2.72 KB, patch)
2011-02-23 12:44 PST, Olli Pettay [:smaug] (pto-ish for couple of days)
enndeakin: review+
Details | Diff | Splinter Review
a bit simpler per bz's comments on IRC (2.32 KB, patch)
2011-02-24 09:47 PST, Olli Pettay [:smaug] (pto-ish for couple of days)
bzbarsky: review+
dveditz: approval1.9.2.18-
dveditz: approval1.9.1.20-
Details | Diff | Splinter Review
GetObjectPrincipal method from trunk inline (2.45 KB, patch)
2011-08-01 15:04 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
dveditz: approval1.9.2.20+
Details | Diff | Splinter Review

Description User image moz_bug_r_a4 2010-06-15 08:32:25 PDT
If a user drag a browser's tab element and drop it on the content area, a
content-registered drop event listener can access the tab element and its
properties including privileged functions.

Only drop event listeners can access privileged objects without the security
check.
http://hg.mozilla.org/mozilla-central/file/2348611d0230/content/events/src/nsDOMDataTransfer.cpp#l455
Comment 1 User image moz_bug_r_a4 2010-06-15 08:34:09 PDT
Created attachment 451291 [details]
testcase - Arbitrary code execution

This works on Firefox trunk, 3.6.* and 3.5.*.

(SeaMonkey does not store a tab element in a dataTransfer when a tab is
dragged.)
Comment 2 User image Blake Kaplan (:mrbkap) 2010-06-16 20:36:36 PDT
There are two bugs here:
  * XPCConvert::NativeInterface2JSObject does not create COWs automatically. IMO we should fix this in another bug.
  * nsDOMDataTransfer::MozGetDataAt doesn't do anything to censor chrome objects from content getting at them. While having COWs would fix this bug, IMO it would be better to avoid exposing chrome objects to content at all from this API. Does that sound reasonable to people?
Comment 3 User image Blake Kaplan (:mrbkap) 2010-06-16 20:37:16 PDT
(and the first reason there means that bug 572130 can't save us here.)
Comment 4 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-06-17 03:33:26 PDT
Sounds very reasonable, and that is what I was thinking too; we shouldn't 
expose chrome objects to content. I'm not quite sure how to filter out
chrome objects.

The first reason is the most important, though.
Comment 5 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-06-17 06:15:55 PDT
Ah, we have principal for the TransferItems. We could use those.
And content shouldn't even know if there are items which it can't access.
Comment 6 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-06-17 06:19:03 PDT
Though, perhaps there are cases like string data drop when content should
be able to access the data even if the principal is system principal.
...need to investigate.
Comment 7 User image Neil Deakin 2010-06-17 07:03:03 PDT
>   * nsDOMDataTransfer::MozGetDataAt doesn't do anything to censor chrome
> objects from content getting at them. While having COWs would fix this bug, IMO
> it would be better to avoid exposing chrome objects to content at all from this
> API. Does that sound reasonable to people?

What are 'chrome objects'?

The only thing that should be prevented is dom objects that aren't accessible from the page the drop listener is in. I assumed that was already happening. Is that not the case?
Comment 8 User image Blake Kaplan (:mrbkap) 2010-06-17 12:10:04 PDT
(In reply to comment #7)
> What are 'chrome objects'?

Objects that are implicitly associated with a chrome "context." In the testcase here, the chrome object is the XUL tab element that is being dropped.

> The only thing that should be prevented is dom objects that aren't accessible
> from the page the drop listener is in. I assumed that was already happening. Is
> that not the case?

It is not, unfortunately. Bug 572647 will help, though.
Comment 9 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-06-17 12:16:52 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > What are 'chrome objects'?
> 
> Objects that are implicitly associated with a chrome "context."
But what objects are associated with chrome context?
All nsPIDOMEventTargets, sure, but what else?
All the JS implemented (implemented in chrome JS) objects?
Comment 10 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-06-23 00:59:08 PDT
So it is not clear to me what to do with this.
Bug 572647 would fix the security problem.
Comment 11 User image christian 2010-06-24 16:21:22 PDT
Is it more likely to get a fix for bug 572647 than this bug? If so we'll track the other one for the branches
Comment 12 User image christian 2010-06-29 13:37:44 PDT
We're punting this out of .7/.11 for now, please let me know if it should be pulled back (or if bug 572647 should be on the branches instead)
Comment 13 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-09-17 03:59:19 PDT
So the testcase doesn't seem to work on trunk, but does on branches.
Comment 14 User image Damon Sicore (:damons) 2010-10-05 13:16:12 PDT
Marcia, can you confirm comment 13?  If it's no longer repro on trunk, let's remove blocking status.
Comment 15 User image moz_bug_r_a4 2010-10-06 06:35:57 PDT
I can reproduce the testcase on trunk fx-4.0b7pre-2010-10-06-04.
Comment 16 User image Marcia Knous [:marcia - use ni] 2010-10-06 14:24:20 PDT
Yes, I can repro as well using  Mozilla/5.0 (Windows NT 5.1; rv:2.0b7pre) Gecko/20101006 Firefox/4.0b7pre.
Comment 17 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-10-12 13:22:43 PDT
If compartments fixes this then it should be fixed on tracemonkey by now. Need to verify that before we mark this one FIXED.

However we still need a different fix for branches.
Comment 18 User image Blake Kaplan (:mrbkap) 2010-10-12 17:54:58 PDT
Compartments might not have fixed it due to default-unsafe exposedProps...
Comment 19 User image moz_bug_r_a4 2010-10-13 08:54:43 PDT
The testcase works on tracemonkey 2010-10-13-04.
Comment 20 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-10-13 11:02:34 PDT
What do you mean by "works"? Do you mean that you can still successfully get elevated privileges? Or do you mean that the browser successfully stops you?
Comment 21 User image moz_bug_r_a4 2010-10-13 12:43:56 PDT
The testcase can still successfully get elevated privileges.
Comment 22 User image Johnny Stenback (:jst, jst@mozilla.com) 2011-01-27 15:05:21 PST
This will actually be fixed by mrbkap's fix in bug 611485 (fixed in tracemonkey). Bug 572647 has been fixed for some time now, but it, as filed, wasn't quite enough to fix this issue as well.
Comment 23 User image moz_bug_r_a4 2011-02-01 21:47:25 PST
The old testcase was fixed.  But, this bug's problem is not fixed.  Content
code can still access a tab element and abuse its functions.
Comment 24 User image moz_bug_r_a4 2011-02-01 21:48:48 PST
Created attachment 509020 [details]
testcase 2 - Arbitrary code execution
Comment 25 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-02-02 01:38:45 PST
I can certainly reproduce the problem using testcase 2.
Comment 26 User image Johnny Stenback (:jst, jst@mozilla.com) 2011-02-10 14:24:12 PST
Blake, can we do a spot fix for this issue until COW's are default safe?
Comment 27 User image Blake Kaplan (:mrbkap) 2011-02-10 15:04:34 PST
Not for FF4. The best I could come up with would be to try to flip "default safe" based on how content code got its hands on the chrome object in question.
Comment 28 User image Blake Kaplan (:mrbkap) 2011-02-10 15:07:30 PST
Although, I guess we could try to avoid handing out chrome-compartment stuff to content from the drag/drop API... Not sure how hard that would be though or if it'd have other effects.
Comment 29 User image Blake Kaplan (:mrbkap) 2011-02-14 19:32:58 PST
Created attachment 512386 [details] [diff] [review]
Possible fix

Neil, do you remember why this check was here? It seems to date all the way back to the original implementation.
Comment 30 User image Neil Deakin 2011-02-15 06:16:57 PST
Comment on attachment 512386 [details] [diff] [review]
Possible fix

The line is there so that you can drop something from one source to another. Drag and drop wouldn't be particularly useful without this.
Comment 31 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-02-21 12:43:15 PST
Should we have some way to mark chrome only DataTransfer data items?
Or perhaps other way, if chrome wants to allow content to access the data,
it should mark the data somehow.
This all at least with "uncommon" data transfer formats.
Comment 32 User image Johnny Stenback (:jst, jst@mozilla.com) 2011-02-21 22:37:25 PST
Blake said he's got a patch in progress for limiting where chrome objects are handed out from drag n drop code. Blake, please provide an ETA for the patch.
Comment 33 User image Blake Kaplan (:mrbkap) 2011-02-22 16:57:43 PST
Created attachment 514366 [details] [diff] [review]
Possible fix v2

So, the bug here is really that chrome objects don't behave according to the same origin model. This patch preserves the original behavior for content objects, but denies access to chrome objects that have been dropped.

A second thought: would it make sense in the future to return null instead of throwing here? This seems like a case where the API really means "give me what I asked for if I have access to it, otherwise I don't want to know."
Comment 34 User image Neil Deakin 2011-02-22 18:37:42 PST
Comment on attachment 514366 [details] [diff] [review]
Possible fix v2

But this patch has the same problem.

1. Open a page which listens for the drop event:
data:text/html,<body ondragover="return false" ondrop="event.preventDefault(); alert(event.dataTransfer.getData('text/plain'));">Drop here</body>

2. Drag text from the url field or another application onto it.
Actual: Security Exception
Expected: An alert with the dropped text.

Note that the mPrincipal isn't the principal of the thing being dragged, it's the principal of the caller that put it in the dataTransfer (although a native caller could theoretically use any principal).

You actually want to be checking the thing being dragged (stored in mData) to check if it can be accessed. For this specific bug about dropping tabs, that thing will be a <tab> element from chrome.
Comment 35 User image Blake Kaplan (:mrbkap) 2011-02-23 01:31:02 PST
I'm not really interested in playing whack-a-mole like that. I give up.
Comment 36 User image Johnathan Nightingale [:johnath] 2011-02-23 06:30:00 PST
(In reply to comment #35)
> I'm not really interested in playing whack-a-mole like that. I give up.

Where does that put us then, gents?
Comment 37 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-02-23 09:43:39 PST
So if we end up blacklisting, do we know what all should be blacklisted.

Or, hmm, could we whilelist things which can be dropped using
system principal?
Comment 38 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-02-23 09:48:40 PST
What would happen if the drag started in the content page and
script there sets some data using application/x-moz-tabbrowser-tab?
Comment 39 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-02-23 10:15:04 PST
Enn, Gavin, do you happen to know what all drag formats chrome uses?
Comment 40 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-02-23 11:37:09 PST
Created attachment 514555 [details] [diff] [review]
Be very strict when handling common DOM objects as transfer data

This is not yet tested properly, but at least dropping files from
desktop to content works, d&d tabs works, d&d text from location bar to content page works etc.
Comment 41 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-02-23 12:00:13 PST
So the patch prevents dragging EventTarget objects from content to chrome
and vise versa. Are there cases when we would want to enable that?
Comment 42 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-02-23 12:06:06 PST
Comment on attachment 514555 [details] [diff] [review]
Be very strict when handling common DOM objects as transfer data

I really need feedback from those who know Firefox UI code.
Comment 43 User image Neil Deakin 2011-02-23 12:26:27 PST
Why is the mExplicitlySet flag needed?

> So the patch prevents dragging EventTarget objects from content to chrome
> and vise versa. Are there cases when we would want to enable that?

I don't think there is currently.

Don't we want to prevent access if the caller cannot access the node no matter whether it is chrome or not? For instance, a drag from one page to another?
Comment 44 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-02-23 12:35:24 PST
(In reply to comment #43)
> Don't we want to prevent access if the caller cannot access the node no matter
> whether it is chrome or not? For instance, a drag from one page to another?
Ah, probably. So not only currentIsSystem == dataIsSystem check, but also
principal subsumes check.

and ok, mExplicitlySet isn't probably needed after all...
Comment 45 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-02-23 12:44:13 PST
Created attachment 514576 [details] [diff] [review]
v2
Comment 46 User image Johnny Stenback (:jst, jst@mozilla.com) 2011-02-23 17:58:02 PST
Over to Olli who's working on the alternate fix here!
Comment 47 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-02-24 09:47:24 PST
Created attachment 514819 [details] [diff] [review]
a bit simpler per bz's comments on IRC
Comment 48 User image Boris Zbarsky [:bz] (still a bit busy) 2011-02-24 09:49:30 PST
Comment on attachment 514819 [details] [diff] [review]
a bit simpler per bz's comments on IRC

r=me
Comment 49 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-02-24 12:48:06 PST
http://hg.mozilla.org/mozilla-central/rev/bc901af701c9
Comment 50 User image Daniel Veditz [:dveditz] 2011-02-25 10:27:20 PST
Comment on attachment 514819 [details] [diff] [review]
a bit simpler per bz's comments on IRC

Approved for 1.9.2.15 and 1.9.1.18, a=dveditz for release-drivers
Comment 51 User image Daniel Veditz [:dveditz] 2011-04-15 10:45:58 PDT
Comment on attachment 514819 [details] [diff] [review]
a bit simpler per bz's comments on IRC

Not sure why this didn't make it, but moving approvals forward since it missed.
Comment 52 User image Daniel Veditz [:dveditz] 2011-06-09 01:14:08 PDT
Comment on attachment 514819 [details] [diff] [review]
a bit simpler per bz's comments on IRC

Doesn't compile on 1.9.2, will need a new patch.

  error: ‘class nsIScriptContext’ has no member named ‘GetObjectPrincipal’
Comment 53 User image Daniel Veditz [:dveditz] 2011-06-10 10:57:15 PDT
Olli: still need that new patch.
Comment 54 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-08-01 15:04:07 PDT
Created attachment 549940 [details] [diff] [review]
GetObjectPrincipal method from trunk inline
Comment 55 User image Daniel Veditz [:dveditz] 2011-08-01 15:07:53 PDT
Comment on attachment 549940 [details] [diff] [review]
GetObjectPrincipal method from trunk inline

Approved for 1.9.2.20, a=dveditz
Comment 56 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-08-01 15:22:56 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ab26f1cefe55

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