Closed Bug 461861 Opened 16 years ago Closed 15 years ago

Set aside the frame chain when firing synchronous events

Categories

(Core :: XPConnect, defect, P2)

x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: mrbkap, Assigned: smaug)

References

Details

(Keywords: fixed1.9.1, verified1.9.0.12, Whiteboard: [sg:critical][needs 1.8 patch diff before review])

Attachments

(8 files, 5 obsolete files)

6.89 KB, patch
Details | Diff | Splinter Review
1.21 KB, patch
Details | Diff | Splinter Review
7.28 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
7.28 KB, patch
Details | Diff | Splinter Review
7.12 KB, patch
Details | Diff | Splinter Review
9.33 KB, patch
mrbkap
: review+
mrbkap
: superreview+
Details | Diff | Splinter Review
10.75 KB, patch
Details | Diff | Splinter Review
15.32 KB, text/plain
Details
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.
Attached patch So like this (obsolete) — Splinter Review
This independently fixes bug 461743.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #345021 - Flags: superreview?(bzbarsky)
Attachment #345021 - Flags: review?(jst)
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x+
Flags: blocking1.9.1?
Flags: blocking1.9.0.5+
Flags: blocking1.8.1.19+
Whiteboard: [sg:critical]
So the reason we have the != e.cx check is to fix bug 390488...
So perhaps we can censor the stack frame if the current principals don't match the principals of the pushed context's JS object?
Current principals?
In other words, if the return value of GetSubjectPrincipal doesn't match the principal of the context's global object.
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?
Whiteboard: [sg:critical] → [sg:critical] needs reviews (jst/bz)
Yeah. We'd need to do something smarter than calling GetObjectPrincipal and GetSubjectPrincipal...
Not blocking 1.9.1 on this as there's no known exploits for this.
Flags: wanted1.9.1+
Flags: wanted-next+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Priority: -- → P1
Whiteboard: [sg:critical] needs reviews (jst/bz) → [sg:critical]
Whiteboard: [sg:critical] → [sg:critical][needs r/sr jst/bz - needs new patch?]
Attachment #345021 - Attachment is obsolete: true
Attachment #345021 - Flags: superreview?(bzbarsky)
Attachment #345021 - Flags: review?(jst)
Whiteboard: [sg:critical][needs r/sr jst/bz - needs new patch?] → [sg:critical][needs new patch]
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?
Moving this out to 1.9.0.6 and 1.8.1.next (since it affects Thunderbird).
Flags: blocking1.9.0.6+
Flags: blocking1.9.0.5+
Flags: blocking1.8.1.next+
Flags: blocking1.8.1.19-
Flags: blocking1.8.1.19+
Flags: blocking1.9.0.6+
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)?
Flags: blocking1.9.0.8?
Flags: blocking1.8.1.next+
Flags: blocking1.9.0.8?
Adding this to the Top Security Bugs list.  Please make this a priority.
Component: DOM: Events → XPConnect
OS: Linux → All
QA Contact: events → xpconnect
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.
Flags: blocking1.9.1- → blocking1.9.1?
Blocking, smaug's on this.
Assignee: mrbkap → Olli.Pettay
Flags: blocking1.9.1? → blocking1.9.1+
Priority: P1 → P2
Target Milestone: --- → mozilla1.9.1
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.
Blake, do you happen to have any testcases for this?
... because even after backing out bug 461743/bug 464620, 
I can't reproduce bug 461743.
Ah, no. That was my testcase. I wonder why it fails now...
Attached patch slow patch (obsolete) — Splinter Review
So we want something like this, except that this slows down the
testcase (with DOM depth 1) up to 10%.
Attached file 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.
Thanks!
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.
Attached patch still not good.Splinter Review
Attachment #373733 - Attachment is obsolete: true
Attachment #373842 - Attachment is obsolete: true
Blake, you meant something like this? Unfortunately this regress
Bug 390488. Though, perhaps I'm doing something wrong here.
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.
Attachment #375445 - Flags: superreview?(jst)
Attachment #375445 - Flags: review?(mrbkap)
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?
(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.
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.
> 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.
(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().
(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 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).
Attachment #375445 - Flags: review?(mrbkap) → review+
Attachment #375445 - Flags: superreview?(jst) → superreview+
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
Whiteboard: [sg:critical][needs new patch] → [sg:critical]
Attached patch nits fixedSplinter Review
PConnect uses strange coding style.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attached patch for 1.9.1 (obsolete) — Splinter Review
Attachment #377652 - Attachment is obsolete: true
Attached patch for 1.9.1 (obsolete) — Splinter Review
Uh, this one I hope :)
Attachment #377657 - Attachment is patch: true
Attachment #377657 - Attachment mime type: application/octet-stream → text/plain
Attachment #377657 - Attachment is obsolete: true
Keywords: fixed1.9.1
Depends on: 493366
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.
Flags: blocking1.9.0.12?
testcase affects 1.9.0 as well.
Flags: blocking1.9.0.12? → blocking1.9.0.12+
Flags: blocking1.8.1.next?
Attached patch for 1.9.0Splinter Review
Fixes also the regression.
The difference to 1.9.1 is in the RePush. 1.9.0 doesn't have GetContextForEventHandlers helper.
Attachment #382130 - Flags: superreview?(mrbkap)
Attachment #382130 - Flags: review?(mrbkap)
Attachment #382130 - Flags: superreview?(mrbkap)
Attachment #382130 - Flags: superreview+
Attachment #382130 - Flags: review?(mrbkap)
Attachment #382130 - Flags: review+
Attachment #382130 - Flags: approval1.9.0.12?
Comment on attachment 382130 [details] [diff] [review]
for 1.9.0

Approved for 1.9.0.12, a=dveditz for release-drivers
Attachment #382130 - Flags: approval1.9.0.12? → approval1.9.0.12+
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
Keywords: fixed1.9.0.12
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.
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
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.
Attached patch 1.8.0 versionSplinter Review
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]
Attachment #388212 - Flags: review?(mrbkap)
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.
Group: core-security
Ping?
Whiteboard: [sg:critical] → [sg:critical][needs 1.8 patch diff before review]
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Attachment #388212 - Flags: review?(mrbkap)
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.
You need to log in before you can comment on or make changes to this bug.