Set aside the frame chain when firing synchronous events

RESOLVED FIXED in mozilla1.9.1

Status

()

Core
XPConnect
P2
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: mrbkap, Assigned: smaug)

Tracking

({fixed1.9.1, verified1.9.0.12})

Trunk
mozilla1.9.1
x86
All
fixed1.9.1, verified1.9.0.12
Points:
---
Bug Flags:
wanted-next +
blocking1.9.1 +
wanted1.9.1 +
blocking1.9.0.12 +
wanted1.9.0.x +
blocking1.8.1.19 -
blocking1.8.1.next +
wanted1.8.1.x +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical][needs 1.8 patch diff before review])

Attachments

(8 attachments, 5 obsolete attachments)

(Reporter)

Description

9 years ago
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.
(Reporter)

Comment 1

9 years ago
Created attachment 345021 [details] [diff] [review]
So like this

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...
(Reporter)

Comment 3

9 years ago
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?
(Reporter)

Comment 5

9 years ago
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)
(Reporter)

Comment 7

9 years ago
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?]
(Reporter)

Updated

9 years ago
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.
(Assignee)

Updated

9 years ago
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
(Assignee)

Comment 15

8 years ago
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.
(Assignee)

Comment 16

8 years ago
Blake, do you happen to have any testcases for this?
(Assignee)

Comment 17

8 years ago
... because even after backing out bug 461743/bug 464620, 
I can't reproduce bug 461743.
(Reporter)

Comment 18

8 years ago
Ah, no. That was my testcase. I wonder why it fails now...
(Assignee)

Comment 19

8 years ago
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

8 years ago
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.
(Assignee)

Comment 21

8 years ago
Thanks!
(Assignee)

Comment 22

8 years ago
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.
(Assignee)

Comment 23

8 years ago
Created attachment 374947 [details] [diff] [review]
still not good.
Attachment #373733 - Attachment is obsolete: true
Attachment #373842 - Attachment is obsolete: true
(Assignee)

Comment 24

8 years ago
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.
(Assignee)

Comment 25

8 years ago
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.
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?
(Assignee)

Comment 27

8 years ago
(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.
(Assignee)

Comment 28

8 years ago
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.
(Assignee)

Comment 30

8 years ago
(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().
(Assignee)

Comment 31

8 years ago
(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.
(Reporter)

Comment 32

8 years ago
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+

Updated

8 years ago
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

Updated

8 years ago
Whiteboard: [sg:critical][needs new patch] → [sg:critical]
(Assignee)

Comment 34

8 years ago
Created attachment 377354 [details] [diff] [review]
nits fixed

XPConnect uses strange coding style.
(Assignee)

Comment 35

8 years ago
http://hg.mozilla.org/mozilla-central/rev/ec0e6d5f5bc7
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 36

8 years ago
Created attachment 377652 [details] [diff] [review]
for 1.9.1
(Assignee)

Updated

8 years ago
Attachment #377652 - Attachment is obsolete: true
(Assignee)

Comment 37

8 years ago
Created attachment 377657 [details] [diff] [review]
for 1.9.1

Uh, this one I hope :)
(Assignee)

Updated

8 years ago
Attachment #377657 - Attachment is patch: true
Attachment #377657 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

8 years ago
Attachment #377657 - Attachment is obsolete: true
(Assignee)

Comment 38

8 years ago
Created attachment 377840 [details] [diff] [review]
for 1.9.1

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

Updated

8 years ago
Keywords: fixed1.9.1

Updated

8 years ago
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?
(Assignee)

Comment 41

8 years ago
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.
Attachment #382130 - Flags: superreview?(mrbkap)
Attachment #382130 - Flags: review?(mrbkap)
(Assignee)

Comment 42

8 years ago
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=
(Reporter)

Updated

8 years ago
Attachment #382130 - Flags: superreview?(mrbkap)
Attachment #382130 - Flags: superreview+
Attachment #382130 - Flags: review?(mrbkap)
Attachment #382130 - Flags: review+
(Assignee)

Updated

8 years ago
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+
(Assignee)

Comment 44

8 years ago
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
(Assignee)

Updated

8 years ago
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.
Keywords: fixed1.9.0.12 → verified1.9.0.12

Comment 46

8 years ago
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

8 years ago
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

8 years ago
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]
Attachment #388212 - Flags: review?(mrbkap)
(Reporter)

Comment 49

8 years ago
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
(Reporter)

Comment 50

8 years ago
Ping?
Whiteboard: [sg:critical] → [sg:critical][needs 1.8 patch diff before review]
Flags: blocking1.8.1.next? → blocking1.8.1.next+
(Reporter)

Updated

8 years ago
Attachment #388212 - Flags: review?(mrbkap)

Comment 51

8 years ago
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.
You need to log in before you can comment on or make changes to this bug.