Last Comment Bug 461861 - Set aside the frame chain when firing synchronous events
: Set aside the frame chain when firing synchronous events
Status: RESOLVED FIXED
[sg:critical][needs 1.8 patch diff be...
: fixed1.9.1, verified1.9.0.12
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86 All
: P2 normal (vote)
: mozilla1.9.1
Assigned To: Olli Pettay [:smaug] (vacation Aug 25-28)
:
Mentors:
Depends on: 493366
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-27 15:28 PDT by Blake Kaplan (:mrbkap)
Modified: 2010-03-11 13:47 PST (History)
12 users (show)
jst: wanted‑next+
jst: blocking1.9.1+
jst: wanted1.9.1+
dveditz: blocking1.9.0.12+
dveditz: wanted1.9.0.x+
samuel.sidler+old: blocking1.8.1.19-
dveditz: blocking1.8.1.next+
dveditz: wanted1.8.1.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
So like this (864 bytes, patch)
2008-10-27 16:44 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
slow patch (2.01 KB, patch)
2009-04-20 14:10 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
previous patch + reduced AddRef/Release (27.23 KB, patch)
2009-04-21 08:00 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
still not good. (6.89 KB, patch)
2009-04-28 14:24 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
Regress Bug 390488 (1.21 KB, patch)
2009-05-01 18:33 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
principal check + nsCxPusher optimization for ELM (7.28 KB, patch)
2009-05-01 22:27 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
mrbkap: review+
jst: superreview+
Details | Diff | Splinter Review
nits fixed (7.28 KB, patch)
2009-05-13 23:42 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
for 1.9.1 (7.12 KB, patch)
2009-05-15 02:41 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
for 1.9.1 (7.49 KB, patch)
2009-05-15 03:06 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
for 1.9.1 (7.12 KB, patch)
2009-05-16 03:06 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
for 1.9.0 (9.33 KB, patch)
2009-06-08 08:12 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
mrbkap: review+
mrbkap: superreview+
dveditz: approval1.9.0.12+
Details | Diff | Splinter Review
1.8.0 version (10.75 KB, patch)
2009-07-13 04:34 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
diff between 1.9.0 and 1.8.0 patch (15.32 KB, text/plain)
2010-03-11 13:47 PST, Martin Stránský
no flags Details

Description Blake Kaplan (:mrbkap) 2008-10-27 15:28:35 PDT
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) 2008-10-27 16:44:38 PDT
Created attachment 345021 [details] [diff] [review]
So like this

This independently fixes bug 461743.
Comment 2 Boris Zbarsky [:bz] 2008-10-28 10:46:30 PDT
So the reason we have the != e.cx check is to fix bug 390488...
Comment 3 Blake Kaplan (:mrbkap) 2008-10-28 12:02:31 PDT
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 [:bz] 2008-10-28 13:33:30 PDT
Current principals?
Comment 5 Blake Kaplan (:mrbkap) 2008-10-28 13:54:08 PDT
In other words, if the return value of GetSubjectPrincipal doesn't match the principal of the context's global object.
Comment 6 Boris Zbarsky [:bz] 2008-10-28 14:21:55 PDT
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) 2008-10-29 16:05:18 PDT
Yeah. We'd need to do something smarter than calling GetObjectPrincipal and GetSubjectPrincipal...
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2008-11-06 16:21:32 PST
Not blocking 1.9.1 on this as there's no known exploits for this.
Comment 9 Daniel Veditz [:dveditz] 2008-11-19 15:38:58 PST
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) 2008-11-19 15:54:36 PST
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) 2009-02-23 07:23:46 PST
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) 2009-03-04 17:15:31 PST
Adding this to the Top Security Bugs list.  Please make this a priority.
Comment 13 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-04-13 09:50:00 PDT
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, jst@mozilla.com) 2009-04-17 09:02:51 PDT
Blocking, smaug's on this.
Comment 15 Olli Pettay [:smaug] (vacation Aug 25-28) 2009-04-20 01:53:19 PDT
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] (vacation Aug 25-28) 2009-04-20 06:47:07 PDT
Blake, do you happen to have any testcases for this?
Comment 17 Olli Pettay [:smaug] (vacation Aug 25-28) 2009-04-20 08:29:11 PDT
... because even after backing out bug 461743/bug 464620, 
I can't reproduce bug 461743.
Comment 18 Blake Kaplan (:mrbkap) 2009-04-20 11:58:16 PDT
Ah, no. That was my testcase. I wonder why it fails now...
Comment 19 Olli Pettay [:smaug] (vacation Aug 25-28) 2009-04-20 14:10:00 PDT
Created attachment 373733 [details] [diff] [review]
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 2009-04-20 21:53:41 PDT
Created attachment 373791 [details]
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] (vacation Aug 25-28) 2009-04-21 00:33:03 PDT
Thanks!
Comment 22 Olli Pettay [:smaug] (vacation Aug 25-28) 2009-04-21 08:00:52 PDT
Created attachment 373842 [details] [diff] [review]
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] (vacation Aug 25-28) 2009-04-28 14:24:36 PDT
Created attachment 374947 [details] [diff] [review]
still not good.
Comment 24 Olli Pettay [:smaug] (vacation Aug 25-28) 2009-05-01 18:33:50 PDT
Created attachment 375432 [details] [diff] [review]
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] (vacation Aug 25-28) 2009-05-01 22:27:51 PDT
Created attachment 375445 [details] [diff] [review]
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 [:bz] 2009-05-03 11:31:17 PDT
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] (vacation Aug 25-28) 2009-05-03 13:13:47 PDT
(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] (vacation Aug 25-28) 2009-05-03 13:16:17 PDT
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 [:bz] 2009-05-03 13:46:00 PDT
> 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] (vacation Aug 25-28) 2009-05-03 13:57:26 PDT
(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] (vacation Aug 25-28) 2009-05-12 10:48:35 PDT
(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) 2009-05-13 19:49:28 PDT
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, jst@mozilla.com) 2009-05-13 20:57:34 PDT
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] (vacation Aug 25-28) 2009-05-13 23:42:26 PDT
Created attachment 377354 [details] [diff] [review]
nits fixed

XPConnect uses strange coding style.
Comment 35 Olli Pettay [:smaug] (vacation Aug 25-28) 2009-05-14 00:07:54 PDT
http://hg.mozilla.org/mozilla-central/rev/ec0e6d5f5bc7
Comment 36 Olli Pettay [:smaug] (vacation Aug 25-28) 2009-05-15 02:41:39 PDT
Created attachment 377652 [details] [diff] [review]
for 1.9.1
Comment 37 Olli Pettay [:smaug] (vacation Aug 25-28) 2009-05-15 03:06:48 PDT
Created attachment 377657 [details] [diff] [review]
for 1.9.1

Uh, this one I hope :)
Comment 38 Olli Pettay [:smaug] (vacation Aug 25-28) 2009-05-16 03:06:11 PDT
Created attachment 377840 [details] [diff] [review]
for 1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c123669c9f38
Comment 39 Daniel Veditz [:dveditz] 2009-05-25 09:51:41 PDT
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] 2009-05-28 10:53:23 PDT
testcase affects 1.9.0 as well.
Comment 41 Olli Pettay [:smaug] (vacation Aug 25-28) 2009-06-08 08:12:00 PDT
Created attachment 382130 [details] [diff] [review]
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 43 Daniel Veditz [:dveditz] 2009-06-12 10:22:52 PDT
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] (vacation Aug 25-28) 2009-06-14 07:32:43 PDT
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] 2009-06-30 17:39:07 PDT
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ý 2009-07-10 02:50:56 PDT
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 2009-07-11 01:21:49 PDT
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ý 2009-07-13 04:34:45 PDT
Created attachment 388212 [details] [diff] [review]
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) 2009-07-14 15:57:30 PDT
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) 2009-11-16 05:51:39 PST
Ping?
Comment 51 Martin Stránský 2010-03-11 13:47:07 PST
Created attachment 431960 [details]
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.

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