461861 - Set aside the frame chain when firing synchronous events

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Blake Kaplan (:mrbkap) (inactive)]

This is a spin-off of bug 461743 comment 0. When we fire synchronous events, we don't set aside the frame chain, so security checks will find any chrome code running as a result of setTimeout, even if the code is properly using XPC*Wrappers.

It seems to me that we should always set aside the frame pointer when pushing a context onto the context stack.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: So like this

This independently fixes bug 461743.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So the reason we have the != e.cx check is to fix bug 390488...

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

So perhaps we can censor the stack frame if the current principals don't match the principals of the pushed context's JS object?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Current principals?

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

In other words, if the return value of GetSubjectPrincipal doesn't match the principal of the context's global object.

Comment 6

[Boris Zbarsky [:bzbarsky]]

That is, the principal of the global of the new context we're pushing?

That could work, yeah....  We push/pop a fair amount, though, don't we?  Are there any performance concerns here?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Yeah. We'd need to do something smarter than calling GetObjectPrincipal and GetSubjectPrincipal...

Comment 8

[Johnny Stenback (:jst)]

Not blocking 1.9.1 on this as there's no known exploits for this.

Comment 9

[Daniel Veditz [:dveditz]]

Are you saying it is not exploitable and [sg:critical] is wrong, or are you saying we're going to wait for an exploit before we fix it?

Comment 10

[Samuel Sidler (old account; do not CC)]

Moving this out to 1.9.0.6 and 1.8.1.next (since it affects Thunderbird).

Comment 11

[Samuel Sidler (old account; do not CC)]

Note: This doesn't reasonably blocking 1.8.1.21 (Thunderbird 2.0.0.21) since it didn't land for 1.9.0.7. We'll get it in the next 1.8 release if Blake's done with it then.

Blake, any update on this bug (and an answer to comment 9)?

Comment 12

[Brandon Sterne (:bsterne)]

Adding this to the Top Security Bugs list.  Please make this a priority.

Comment 13

[Mike Shaver (:shaver emeritus)]

Renominating, to get an answer to dan's question in comment 9.  jst: do you think that this isn't exploitable?  It seems from the original description that it would be straightforward to craft one, especially given the test cases in bug 461743.

Comment 14

[Johnny Stenback (:jst)]

Blocking, smaug's on this.

Comment 15

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

So pushing null or just one context to stack are the most common cases.
But for example when an event handler dispatches a new event and there
is a handler for that event too, there are several contexts in the stack.
I'll check how much event handling slows down if some principal check is added.
http://mozilla.pettay.fi/moztests/events/event_speed_3.html is pretty
much the worst possible case.

Comment 16

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

Blake, do you happen to have any testcases for this?

Comment 17

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

... because even after backing out bug 461743/bug 464620, 
I can't reproduce bug 461743.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Ah, no. That was my testcase. I wonder why it fails now...

Comment 19

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

Attachment: slow patch

So we want something like this, except that this slows down the
testcase (with DOM depth 1) up to 10%.

Comment 20

[moz_bug_r_a4]

Attachment: testcase

This is an arbitrary code execution testcase, which works with the Error
Console.  I can reproduce this on fx-3.6a1pre-2009-04-20-04.

Comment 21

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

Thanks!

Comment 22

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

Attachment: previous patch + reduced AddRef/Release

This is sort of a test to try to use the previous approach but get
performance at least pretty close to what we have now.
AddRef/Release is *slow* when it is done to ccollectable or threadsafe objects.

Comment 23

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

Attachment: still not good.

Comment 24

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

Attachment: Regress Bug 390488

Blake, you meant something like this? Unfortunately this regress
Bug 390488. Though, perhaps I'm doing something wrong here.

Comment 25

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

Attachment: principal check + nsCxPusher optimization for ELM

This speeds up event handling when there are many listeners, but slows it down
a bit (2-3%) if only one very simple listener.
Fixes the testcase and doesn't regress Bug 460001 nor the mochitest for bug 390488.

Comment 26

[Boris Zbarsky [:bzbarsky]]

For what it's worth, Blake and I talked about this a bit on Friday night.  We were thinking that it might make sense to either:

1)  Pass the desired subject principal to Push(); if cx is the same as the 
    existing cx, but the desired subject principal differs from the current
    subject principal of the running code, set aside the frame chain.
2)  Stop depending on this "push cx to get the right subject principal" thing,
    and have API to directly push the desired fallback subject principal
    (presumably on the security manager).  Then change all calls that currently
    push cx to push both.  I guess that doesn't help with this bug, though,
    since the callstack will still be there?

That last change implements approach (1), or tries to, right?

Comment 27

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

(In reply to comment #26)
> 1)  Pass the desired subject principal to Push(); if cx is the same as the 
>     existing cx, but the desired subject principal differs from the current
>     subject principal of the running code, set aside the frame chain.
This doesn't change anything comparing to the patch. The subject principal
would be taken from nsPIDOMEventTarget, so probably NodePrincipal(), which
in practice should be the same as the principal from global object.
(well, at least in most common cases)

> 2)  Stop depending on this "push cx to get the right subject principal" thing,
>     and have API to directly push the desired fallback subject principal
>     (presumably on the security manager).  Then change all calls that currently
>     push cx to push both.
So in practice we'd push the context and the principal we get from context's global object (although we might get that principal via NodePrincipal()).

> That last change implements approach (1), or tries to, right?
Well sort-of. It compares the principal of the cx's global to the current subject principal. The problem is that getting the right principals is slow.
So the optimization to ELM/nsCxPusher reduces push calls when ELM has
many listeners to some event.

Comment 28

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

Especially getting the global object from cx and then principal from that is slow.
Too many QIs, Addrefs and Releases and nsJSContext::GetGlobalObject()
isn't too fast either.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> The subject principal would be taken from nsPIDOMEventTarget, so probably
> NodePrincipal(), which in practice should be the same as the principal from
> global object

Not at all.  The global object is the outer window, which might well have a different principal from the node.

And getting the principal from the node directly might help the performance issue too, fwiw.

Comment 30

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

(In reply to comment #29)
> Not at all.  The global object is the outer window, which might well have a
> different principal from the node.
Well, I guess the most common case when that happens is when we're about
to switch inner window. And in that case event handling isn't really 
supported (if innerWindow1->outerwindow->innerwindow != innerwindow1). 
If global object has a document, global object's principal is taken from
the document.


> And getting the principal from the node directly might help the performance
> issue too, fwiw.
That is true. I think we might want to push NodePrincipal() to
nsPIDOMEventTarget. Maybe call it just Principal().

Comment 31

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

(In reply to comment #30) 
> I think we might want to push NodePrincipal() to
> nsPIDOMEventTarget. Maybe call it just Principal().
But maybe not in this bug.

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 375445 [details] [diff] [review]
principal check + nsCxPusher optimization for ELM

>+static nsIPrincipal*
>+GetPrincipalFromCx(JSContext *cx)
>+{
>+    nsIScriptContext* scriptContext = GetScriptContextFromJSContext(cx);
>+    if (scriptContext)

Nit: Here and below: No space after 'if'.

I still prefer the alternate approach of either wrapping incoming functions (or JS objects), or pushing a principal when we call them, but an attempted implementation today and some thinking revealed that this would leave any code that didn't use XPConnect (or nsJSContext) as exploits waiting to happen. A quick mxr search shows that we do actually use the JSAPI in random places in the tree (and I can't even count extensions or embedders).

So this is clearly the best approach for now, barring making native functions participate in principal computation (which is a difficult path to go down and definitely not for 3.5).

Comment 33

[Johnny Stenback (:jst)]

Comment on attachment 375445 [details] [diff] [review]
principal check + nsCxPusher optimization for ELM

           if (*aDOMEvent) {
             nsRefPtr<nsIDOMEventListener> kungFuDeathGrip = ls->mListener;
             if (useTypeInterface) {
+              pusher.Pop();
               DispatchToInterface(*aDOMEvent, ls->mListener,
                                   dispData->method, *typeData->iid);
-            } else if (useGenericInterface) {
+            } else if (useGenericInterface &&
+                       pusher.RePush(aCurrentTarget)) {

Blake and I looked at this and it looked wrong at first, but after having stared at this for a bit longer I've convinced myself that this is in fact correct, given how RePush() works.

sr=jst

Comment 34

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

Attachment: nits fixed

PConnect uses strange coding style.

Comment 35

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

http://hg.mozilla.org/mozilla-central/rev/ec0e6d5f5bc7

Comment 36

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

Attachment: for 1.9.1

Comment 37

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

Attachment: for 1.9.1

Uh, this one I hope :)

Comment 38

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

Attachment: for 1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c123669c9f38

Comment 39

[Daniel Veditz [:dveditz]]

This looks like it applies to the 1.9.0 branch, would it be safe to take in 1.9.0.12? There was at least one regression (bug 493366) but this might get enough of a beating in the 3.5 rc's and release to be confident.

Comment 40

[Daniel Veditz [:dveditz]]

testcase affects 1.9.0 as well.

Comment 41

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

Attachment: for 1.9.0

Fixes also the regression.
The difference to 1.9.1 is in the RePush. 1.9.0 doesn't have GetContextForEventHandlers helper.

Comment 42

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

I tested also the following bugs:
https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=XSS&long_desc_type=substring&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=&resolution=FIXED&emailassigned_to1=1&emailtype1=exact&email1=Olli.Pettay%40gmail.com&emailassigned_to2=1&emailreporter2=1&emailqa_contact2=1&emailtype2=exact&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=noop&type0-0-0=noop&value0-0-0=

Comment 43

[Daniel Veditz [:dveditz]]

Comment on attachment 382130 [details] [diff] [review]
for 1.9.0

Approved for 1.9.0.12, a=dveditz for release-drivers

Comment 44

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

Checking in content/base/public/nsContentUtils.h;
/cvsroot/mozilla/content/base/public/nsContentUtils.h,v  <--  nsContentUtils.h
new revision: 1.179; previous revision: 1.178
done
Checking in content/base/src/nsContentUtils.cpp;
/cvsroot/mozilla/content/base/src/nsContentUtils.cpp,v  <--  nsContentUtils.cpp
new revision: 1.313; previous revision: 1.312
done
Checking in js/src/xpconnect/src/Makefile.in;
/cvsroot/mozilla/js/src/xpconnect/src/Makefile.in,v  <--  Makefile.in
new revision: 1.89; previous revision: 1.88
done
Checking in js/src/xpconnect/src/xpcthreadcontext.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcthreadcontext.cpp,v  <--  xpcthreadcontext.cpp
new revision: 1.55; previous revision: 1.54
done
Checking in content/events/src/nsEventListenerManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventListenerManager.cpp,v  <--  nsEventListenerManager.cpp
new revision: 1.301; previous revision: 1.300
done

Comment 45

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.12 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.12pre) Gecko/2009063005 GranParadiso/3.0.12pre (.NET CLR 3.5.30729). Bug reproduces in 1.9.0.11 with testcase but gives a security error in the error console with 1.9.0.12pre above.

Comment 46

[Martin Stránský [:stransky] (ni? me)]

Does it really affect 1.8? I can't reproduce it, it gives me:

Security Error: Content at https://bug461861.bugzilla.mozilla.org/attachment.cgi?id=373791&t=t8CVJQJPNy may not load data from https://bugzilla.mozilla.org/attachment.cgi?id=373791.

Error: uncaught exception: Permission denied to get property UnnamedClass.stack

Comment 47

[moz_bug_r_a4]

Ah, with fx2 on linux, you need to evaluate the code in the Error Console twice
in order to get Components.stack alerts.  Sorry, I did not test with fx2 on
linux when I created the testcase.

Comment 48

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8.0 version

Can you please check this one? Seems to work fine, but the first evaluation of the testcase throws this exception:

[Exception... "[JavaScript Error: "out of memory"]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]

Comment 49

[Blake Kaplan (:mrbkap) (inactive)]

Martin, could you attach a regular diff (not unified) between your 1.8.0 version and the version of the diff that landed on trunk?

Thanks.

Comment 50

[Blake Kaplan (:mrbkap) (inactive)]

Ping?

Comment 51

[Martin Stránský [:stransky] (ni? me)]

Attachment: diff between 1.9.0 and 1.8.0 patch

Ahh, sorry, I missed your comment. There is a diff between 1.9 and 1.8 because I derived the 1.8 version from it.