Last Comment Bug 754202 - Pull principals directly off the context/object compartment
: Pull principals directly off the context/object compartment
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Security: CAPS (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Bobby Holley (busy)
:
Mentors:
Depends on: 825164 762578 762920 763129 763433 763475 765168 771354 773998 774633 CVE-2012-4195 CVE-2012-4192 801305 812546
Blocks: IonMonkey 766641 framesandbox 625199 754156 CVE-2012-1967 775197 CVE-2012-3991 783957
  Show dependency treegraph
 
Reported: 2012-05-11 01:55 PDT by Bobby Holley (busy)
Modified: 2013-07-22 10:19 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Fix content->chrome postMessage mochitest. v1 (1.67 KB, patch)
2012-05-22 03:04 PDT, Bobby Holley (busy)
bzbarsky: review+
Details | Diff | Review
Part 2 - Pull object principals directly off the compartment, and assert that behavior doesn't change. v2 (6.21 KB, patch)
2012-05-22 03:05 PDT, Bobby Holley (busy)
bzbarsky: review+
Details | Diff | Review
Part 3 - Pull subject principals directly off the compartment. v1 (1.27 KB, patch)
2012-05-22 03:05 PDT, Bobby Holley (busy)
bzbarsky: review-
Details | Diff | Review
Part 3 - Pull subject principals directly off the compartment. v2 (1.32 KB, patch)
2012-06-05 10:00 PDT, Bobby Holley (busy)
no flags Details | Diff | Review
Part 4 - Disallow calling EvaluateString{,WithValue} with a principal that doesn't match the global. v1 (3.53 KB, patch)
2012-06-05 10:00 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review
Part 5 - Remove context pushing/popping API. v1 (25.30 KB, patch)
2012-06-05 10:00 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review
Part 6 - Remove mContextPrincipal usage from within nsScriptSecurityManager. v1 (7.10 KB, patch)
2012-06-05 10:01 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review
Part 7 - Remove NoWaiverWrapper. v1 (5.08 KB, patch)
2012-06-05 10:01 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review
Part 3 - Pull subject principals directly off the compartment. v3 (1.27 KB, patch)
2012-06-05 10:40 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review
Part 2 - Pull object principals directly off the compartment. v4 (5.64 KB, patch)
2012-06-12 08:31 PDT, Bobby Holley (busy)
bzbarsky: review-
Details | Diff | Review
Part 8 - Check principal in IsCapabilityEnabled when there's no code on the stack. v1 (1.20 KB, patch)
2012-06-12 08:31 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review
Part 7.1 - Scope the push of the safe js context such that it doesn't include the call to XHR::Send. v1 (942 bytes, patch)
2012-06-15 03:59 PDT, Bobby Holley (busy)
mrbkap: review+
bent.mozilla: review+
Details | Diff | Review
Part 7.2 - Use the safe JSContext rather than the current JSContext in IndexedDBDatabaseParent::HandleDatabaseEvent. v1 (1.09 KB, patch)
2012-06-28 03:29 PDT, Bobby Holley (busy)
bent.mozilla: review+
Details | Diff | Review
Part 7.3 - Only push the context of the event target if the listener is scripted. v2 (9.10 KB, patch)
2012-06-28 06:08 PDT, Bobby Holley (busy)
bugs: review+
Details | Diff | Review

Description Bobby Holley (busy) 2012-05-11 01:55:29 PDT
This is one of the big things that CPG enables, but we still have to actually do it. This will involve ripping out a lot of crappy code. :-)
Comment 1 Bobby Holley (busy) 2012-05-18 03:06:10 PDT
IonMonkey will break gecko security without this.
Comment 2 Bobby Holley (busy) 2012-05-21 06:45:17 PDT
Try push: https://tbpl.mozilla.org/?tree=Try&rev=83c4e102d93d
Comment 3 :Ms2ger 2012-05-21 08:12:42 PDT
Make old_doGetObjectPrincipal DEBUG-only, please?
Comment 4 Bobby Holley (busy) 2012-05-22 03:04:52 PDT
Created attachment 625948 [details] [diff] [review]
Part 1 - Fix content->chrome postMessage mochitest. v1

This isn't something we actually support, and were using enablePrivilege to do it before.
When we switch this to SpecialPowers wrapping, the sender origin becomes a chrome URL (not
very interesting) and the source window becomes null (which we do explicitly in
nsGlobalWindow for chrome callers). So those tests stop being useful.
Comment 5 Bobby Holley (busy) 2012-05-22 03:05:32 PDT
Created attachment 625950 [details] [diff] [review]
Part 2 - Pull object principals directly off the compartment, and assert that behavior doesn't change. v2
Comment 6 Bobby Holley (busy) 2012-05-22 03:05:49 PDT
Created attachment 625951 [details] [diff] [review]
Part 3 - Pull subject principals directly off the compartment. v1

It would be nice to check these principals against the principals acquired
using the old mechanism. Unfortunately, they often differ. Because CAPS uses
JS stack frames, any time we enter a compartment and do an operation (even
throwing an Access-Denied exception) without running any JS code, we'll end
up with a different principal.

Our security story is pretty darn tied to compartments at this point, so let's
just pull the trigger.
Comment 7 Bobby Holley (busy) 2012-05-22 03:06:58 PDT
Comment on attachment 625948 [details] [diff] [review]
Part 1 - Fix content->chrome postMessage mochitest. v1

I decided midstream to flag bz for these reviews, since I've already got a lot of things in Blake's review queue. Blake already signed off on the idea here.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-22 07:41:03 PDT
Comment on attachment 625948 [details] [diff] [review]
Part 1 - Fix content->chrome postMessage mochitest. v1

>+++ b/dom/tests/mochitest/whatwg/test_postMessage_chrome.html
>-  is(evt.origin, "http://example.org", "content response event has wrong URI");
>-  is(evt.source, window.frames.contentDomain,
>-     "wrong source for same-domain message!");

Uh... why?
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-22 07:46:30 PDT
Comment on attachment 625950 [details] [diff] [review]
Part 2 - Pull object principals directly off the compartment, and assert that behavior doesn't change. v2

So a question.  Do the callers of doGetObjectPrincipal ever have cross-compartment proxies in obj?  And if so, do they want the principals of the proxy or of the underlying object?  I guess the former, since the proxy will do its own security checks?

If so, r=me.  Looking forward to all that code (including the code for the xpconnect short-circuit) going away in a followup!
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-22 07:53:36 PDT
Comment on attachment 625951 [details] [diff] [review]
Part 3 - Pull subject principals directly off the compartment. v1

Can you explain to me why this doesn't break nsScriptSecurityManager::PushContextPrincipal?

Also, falling back to system when not in a compartment is a bit odd; the old code fell back to the results of GetScriptContextPrincipalFromJSContext() when there was nothing on the stack, no?

So at first glance, this doesn't look right....
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-22 08:01:33 PDT
Oh, and one more thing.  Do we update the compartment of the JSContext when we set up a new inner window?  If not, then this code is wrong in that respect too: the old code would get the updated principal off the window directly in the "no script running" situation, so would get the updated principal.
Comment 12 Bobby Holley (busy) 2012-05-22 08:12:30 PDT

(In reply to Boris Zbarsky (:bz) from comment #8)
> Uh... why?

See comment 4.
Comment 13 Bobby Holley (busy) 2012-05-22 08:16:11 PDT
(In reply to Boris Zbarsky (:bz) from comment #9)

> So a question.  Do the callers of doGetObjectPrincipal ever have
> cross-compartment proxies in obj?  And if so, do they want the principals of
> the proxy or of the underlying object?  I guess the former, since the proxy
> will do its own security checks?

If they did, they'd get the principal of the proxy. There's no special wrapper handling in doGetObjectPrincipal AFAICT, which means that we'll just grab the wrapper's parent, which is the global of the wrapper (not the wrappee).

> If so, r=me.  Looking forward to all that code (including the code for the
> xpconnect short-circuit) going away in a followup!

Me too!
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-22 09:42:17 PDT
Hrm.  That seems ... highly suboptimal.  Did you look into why we have this test?  Is anything relying on the old behavior?
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-22 09:42:35 PDT
Comment 14 was in response to comment 12...
Comment 16 Bobby Holley (busy) 2012-05-22 12:20:14 PDT
(In reply to Boris Zbarsky (:bz) from comment #10)

> Can you explain to me why this doesn't break
> nsScriptSecurityManager::PushContextPrincipal?

It does, but we don't need it anymore. Attaching patches to rip it out momentarily.

> Also, falling back to system when not in a compartment is a bit odd; the old
> code fell back to the results of GetScriptContextPrincipalFromJSContext()
> when there was nothing on the stack, no?

Good call. I'll fix that.

> Oh, and one more thing.  Do we update the compartment of the JSContext when
> we set up a new inner window?  If not, then this code is wrong in that
> respect too: the old code would get the updated principal off the window
> directly in the "no script running" situation, so would get the updated
> principal.

I'm not sure. Do you know where that code would be, or what it should look like if it doesn't exist?
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-22 12:30:24 PDT
> Attaching patches to rip it out momentarily.

I hope with some asserts about the principal already being correct!  Please have smaug review the result?

> Do you know where that code would be

It would need to be in nsGlobalWindow::SetNewDocument or some callee.  Probably right around where we move the outer window into a different compartment.  As for what it should look like, I'm not sure; I don't know what APIs jseng exposes to just set the compartment of a JSContext and whatnot.. or even whether it's the right thing to do there.
Comment 18 Bobby Holley (busy) 2012-05-22 12:35:55 PDT
(In reply to Boris Zbarsky (:bz) from comment #17)
> I hope with some asserts about the principal already being correct!  Please
> have smaug review the result?

Yep. :-)

> 
> > Do you know where that code would be
> 
> It would need to be in nsGlobalWindow::SetNewDocument or some callee. 
> Probably right around where we move the outer window into a different
> compartment.  As for what it should look like, I'm not sure; I don't know
> what APIs jseng exposes to just set the compartment of a JSContext and
> whatnot.. or even whether it's the right thing to do there.

Hm, maybe we should ask Blake. Blake, if there's no JS code on the stack, should we return system principal, or fall back to the GetScriptContextPrincipalFromJSContext stuff? If the latter, what do you think about bz's concern in comment 11?
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-22 12:39:35 PDT
1) Note that "no JS code on the stack" is not the same as "null context compartment".  The compartment can be non-null even if there's no JS on the stack.

2) At least event dispatch relies on pushing the JSContext (with no code on it!) to push the right principals, iirc.
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-24 12:56:55 PDT
Comment on attachment 625948 [details] [diff] [review]
Part 1 - Fix content->chrome postMessage mochitest. v1

r=me
Comment 21 Ian Melven :imelven 2012-06-01 18:12:14 PDT
Just a note that I was hitting a crash when doing a postMessage, after the assertion "Assertion failure: principals == JS_GetCompartmentPrincipals((js::GetContextComp
artment(cx))), at c:/Users/imelven/src/mozilla/caps/src/nsScriptSecurityManager.
cpp:208" while working on iframe sandbox (bug 341604) that these patches fixed :)
Comment 22 Bobby Holley (busy) 2012-06-04 03:45:16 PDT
FWIW the newer patches for this are here: https://github.com/bholley/mozilla-central/commits/decaps

Haven't worked on it in a few weeks though, been busy :-(
Comment 23 Bobby Holley (busy) 2012-06-05 10:00:14 PDT
Created attachment 630200 [details] [diff] [review]
Part 3 - Pull subject principals directly off the compartment. v2
Comment 24 Bobby Holley (busy) 2012-06-05 10:00:29 PDT
Created attachment 630201 [details] [diff] [review]
Part 4 - Disallow calling EvaluateString{,WithValue} with a principal that doesn't match the global. v1
Comment 25 Bobby Holley (busy) 2012-06-05 10:00:45 PDT
Created attachment 630202 [details] [diff] [review]
Part 5 - Remove context pushing/popping API. v1

Each one of these uses grabs the principal off of an object for pushing, but also enters the compartment of that object. So we shouldn't need this anymore.

Can I get a 'hell yeah'?
Comment 26 Bobby Holley (busy) 2012-06-05 10:01:00 PDT
Created attachment 630203 [details] [diff] [review]
Part 6 - Remove mContextPrincipal usage from within nsScriptSecurityManager. v1
Comment 27 Bobby Holley (busy) 2012-06-05 10:01:17 PDT
Created attachment 630204 [details] [diff] [review]
Part 7 - Remove NoWaiverWrapper. v1

No more principal pushing!
Comment 28 Luke Wagner [:luke] 2012-06-05 10:11:18 PDT
(In reply to Bobby Holley (:bholley) from comment #25)
\o/ \o/ \o/ \o/ \o/ hell yeah \o/ \o/ \o/ \o/ \o/
Comment 29 Bobby Holley (busy) 2012-06-05 10:40:56 PDT
Created attachment 630223 [details] [diff] [review]
Part 3 - Pull subject principals directly off the compartment. v3
Comment 30 Bobby Holley (busy) 2012-06-05 10:41:42 PDT
mrbkap said on IRC that we should just assert that js::GetContextCompartment never returns null.
Comment 31 Bobby Holley (busy) 2012-06-05 10:49:02 PDT
Pushed the updated stack here to try: https://tbpl.mozilla.org/?tree=Try&rev=4a4414893cb1
Comment 32 Bobby Holley (busy) 2012-06-06 04:40:00 PDT
Looks green :-)
Comment 33 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-06-06 14:51:03 PDT
Comment on attachment 630201 [details] [diff] [review]
Part 4 - Disallow calling EvaluateString{,WithValue} with a principal that doesn't match the global. v1

Review of attachment 630201 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsJSEnvironment.cpp
@@ +1200,5 @@
> +    return NS_ERROR_FAILURE;
> +#ifdef DEBUG
> +  bool equal = false;
> +  principal->Equals(aPrincipal, &equal);
> +  MOZ_ASSERT(equal);

Do we want to assert that aScopeObject's principals is the same as principal here?
Comment 34 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-06-06 14:55:43 PDT
Comment on attachment 630202 [details] [diff] [review]
Part 5 - Remove context pushing/popping API. v1

Review of attachment 630202 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/src/json/nsJSON.cpp
@@ +185,4 @@
>    }
>  
>    nsJSONWriter writer;
>    JSBool ok = JS_Stringify(cx, value, NULL, JSVAL_NULL,

Looks like |ok| can go away now.
Comment 35 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-06-06 15:14:51 PDT
Comment on attachment 630223 [details] [diff] [review]
Part 3 - Pull subject principals directly off the compartment. v3

Review of attachment 630223 [details] [diff] [review]:
-----------------------------------------------------------------

This is *hot*.
Comment 36 Bobby Holley (busy) 2012-06-07 03:36:13 PDT
Addressed review comments. Gave this one last linux-debug-only push to try in case the asserts added from comment 33 fire:

https://tbpl.mozilla.org/?tree=Try&rev=54566a8bd245
Comment 39 Bobby Holley (busy) 2012-06-10 15:36:04 PDT
Backed out patches 3-7 for various regressions:
http://hg.mozilla.org/mozilla-central/rev/90107a2a0c64
Comment 40 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-06-10 17:14:27 PDT
Backout was backed out.

https://hg.mozilla.org/mozilla-central/rev/17a91ff5dfd7

I guess technically this should be turned fixed, but I'll leave it alone for now...
Comment 41 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-10 17:23:53 PDT
I redid the backout, but backing out all patches rather than just patches 3-7:
https://hg.mozilla.org/mozilla-central/rev/8d57c055f951
Comment 42 Bobby Holley (busy) 2012-06-12 08:31:05 PDT
Created attachment 632263 [details] [diff] [review]
Part 2 - Pull object principals directly off the compartment. v4

So, it looks like we have to remove the assert here because of (drumroll) UniversalXPConnect. When UniversalXPConnect is enabled,
sometimes we run into situations where we create chrome objects with a content NodePrincipal. layout/base/tests/test_bug458898.html
is an example of where this happens.
Comment 43 Bobby Holley (busy) 2012-06-12 08:31:54 PDT
Created attachment 632264 [details] [diff] [review]
Part 8 - Check principal in IsCapabilityEnabled when there's no code on the stack. v1

This fixes the regression from bug 762920.
Comment 44 Bobby Holley (busy) 2012-06-12 08:40:59 PDT
Try push: https://tbpl.mozilla.org/?tree=Try&rev=045e28393ec5
Comment 45 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-12 19:05:21 PDT
> When UniversalXPConnect is enabled, sometimes we run into situations where we create
> chrome objects with a content NodePrincipal. 

That sounds incredibly broken.  Which exact objects in that testcase are "chrome objects with a content NodePrincipal()"?

I'm not happy at all with removing that assert, esp, because in this case it is in fact catching what totally looks like a bug to me.
Comment 46 Bobby Holley (busy) 2012-06-13 01:48:27 PDT
(In reply to Boris Zbarsky (:bz) from comment #45)
> > When UniversalXPConnect is enabled, sometimes we run into situations where we create
> > chrome objects with a content NodePrincipal. 
> 
> That sounds incredibly broken.  Which exact objects in that testcase are
> "chrome objects with a content NodePrincipal()"?
> 
> I'm not happy at all with removing that assert, esp, because in this case it
> is in fact catching what totally looks like a bug to me.

The document here: https://mxr.mozilla.org/mozilla-central/source/layout/base/tests/test_bug458898.html?force=1#23
Comment 47 Bobby Holley (busy) 2012-06-13 02:41:38 PDT
(In reply to Bobby Holley (:bholley) from comment #46)
> The document here:
> https://mxr.mozilla.org/mozilla-central/source/layout/base/tests/
> test_bug458898.html?force=1#23

Where does the principal come from for data urls?
Comment 48 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-13 08:12:16 PDT
> Where does the principal come from for data urls?

It depends.  Typically it's inherited from somewhere, but in some cases we cut off inheritance and give them a nsNullPrincipal (e.g data: being loaded in a type="content" iframe by chrome code).

The JSObject for a Document with a Window should totally be in the compartment of said Window and should have the same principal as the Window and Document involved.
Comment 49 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-13 08:13:50 PDT
If you mean the specific code, there's a bunch of stuff about inheriting principals and whatnot in the docshell LoadURI/InternalLoad code, iirc.
Comment 50 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-06-13 15:42:06 PDT
Comment on attachment 632264 [details] [diff] [review]
Part 8 - Check principal in IsCapabilityEnabled when there's no code on the stack. v1

Review of attachment 632264 [details] [diff] [review]:
-----------------------------------------------------------------

Funnily enough, jst wrote a patch with basically the same semantics over in bug 235457 8 years ago! At the time, he was defeated by the fact that XPConnect did security checks with no code on the stack, calling out into chrome code on the safe JS context. That's been fixed for a while now.
Comment 51 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-13 23:31:39 PDT
Comment on attachment 632263 [details] [diff] [review]
Part 2 - Pull object principals directly off the compartment. v4

Per discussion.
Comment 52 Bobby Holley (busy) 2012-06-14 09:45:07 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #50)

> Funnily enough, jst wrote a patch with basically the same semantics over in
> bug 235457 8 years ago! At the time, he was defeated by the fact that
> XPConnect did security checks with no code on the stack, calling out into
> chrome code on the safe JS context. That's been fixed for a while now.

So, it looks like this patch isn't quite out of the clear. It breaks some worker XHR tests. I debugged it, and found the following stacks which used to succeed and now fail:

http://pastebin.mozilla.org/1662404

The subject principal at the time is the Null principal. Smaug, how is this stuff supposed to work? Do we need / need to get rid of a cx push somewhere?
Comment 53 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-14 10:54:09 PDT
Why is subject principal null. Shouldn't it be system.
Comment 54 Bobby Holley (busy) 2012-06-15 03:59:59 PDT
Created attachment 633471 [details] [diff] [review]
Part 7.1 - Scope the push of the safe js context such that it doesn't include the call to XHR::Send. v1

This fixes the XHR timeouts with Part 8 applied. 1-liner, exactly what we talked about on IRC, blake. Applies on top of bug 765168.
Comment 55 Bobby Holley (busy) 2012-06-15 04:11:25 PDT
Gave this another try push: https://tbpl.mozilla.org/?tree=Try&rev=c203f65376fd
Comment 56 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-06-15 14:17:57 PDT
Comment on attachment 633471 [details] [diff] [review]
Part 7.1 - Scope the push of the safe js context such that it doesn't include the call to XHR::Send. v1

I think this is right, but I'd like bent to look at this as well to double check.
Comment 57 Bobby Holley (busy) 2012-06-18 09:00:35 PDT
Last try push was borked. trying again: https://tbpl.mozilla.org/?tree=Try&rev=b004a98d59b5
Comment 58 Bobby Holley (busy) 2012-06-20 02:31:35 PDT
There were some linux-only mochitest-ally failures on the last push. I'm hoping they were from something underneath my patches. Pushing again to find out: https://tbpl.mozilla.org/?tree=Try&rev=46fe539c2d74
Comment 59 Bobby Holley (busy) 2012-06-20 05:01:17 PDT
ok, a11y bustage looks real. Firing up a linux vm to investigate.
Comment 60 Bobby Holley (busy) 2012-06-20 09:27:41 PDT
So, the test is failing when it tries to send a key event. We fail the following universalxpconnect check were we used to succeed:

http://pastebin.mozilla.org/1668825

Basically, some privileged code calls nsDOMWindowUtils::SendKeyEvent on a content window. Along the way, we somehow end up pushing a context and entering a compartment such that we're no longer privileged, and fail the security check at nsHTMLInputElement::SetUserInput:

http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp#1080
Comment 61 Bobby Holley (busy) 2012-06-20 09:35:34 PDT
Also, the relevant JS stack is here: http://pastebin.mozilla.org/1668826
Comment 62 Bobby Holley (busy) 2012-06-24 08:26:54 PDT
Ok, so here's the issue. When we dispatch events, we push the context from the event target because we might call content event handlers. But in the failing test, we don't - we just call C++. And that C++ now fails, because it thinks its running with content privileges.

We used to do the right thing here because CAPS would grant UniversalXPConnect if there was no JS code on the callstack. But I plugged that little loophole in patch 8.

It seems to me that the correct solution here is to push the event target principal only if we're running a scripted event handler. We could do this in the event dispatch code, or in XPCWrappedJSClass. The latter would skip a QI (since we could just directly check mIID), which is probably faster. But it's also a bit less clean.

Smaug, Blake - can you guys weigh in? This is a pretty important patch to land for a number of reasons, and we're so close here.
Comment 63 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-24 08:45:32 PDT
(In reply to Bobby Holley (:bholley) from comment #62)
> It seems to me that the correct solution here is to push the event target
> principal only if we're running a scripted event handler.
Push principal? I thought principal pushing is being removed.

> We could do this
> in the event dispatch code, or in XPCWrappedJSClass. The latter would skip a
> QI (since we could just directly check mIID), which is probably faster. But
> it's also a bit less clean.
We would need the same thing also elsewhere, so it wouldn't make sense to hack event handling code only.
All the callbacks should be handled the same way.

(But note, onfoo event handlers are actually nsIJSEventListener objects)
Comment 64 Bobby Holley (busy) 2012-06-24 10:55:56 PDT
Actually though, do we even need to be doing this at all?

If we're running content code, we'll have to enter the content compartment (we'll get a compartment mismatch assertion if we don't). If we're pulling the subject principal directly off the context compartment, then we should be fine, no? Or is the cx pushing necessary for another reason?
Comment 65 Bobby Holley (busy) 2012-06-25 05:46:12 PDT
Sounds like we really just need to kill context pushing here to make this work. I'm going to start on that over in bug 767938.

CCing dvander and dmandelin to make sure they're aware of the situation here. Regardless of what we do about enablePrivilege, I'm pretty sure this bug does block IonMonkey, since browser security doesn't work without it. I didn't bring it up in our previous discussion because this had already landed at the time, and then got backed out. :-(
Comment 66 Luke Wagner [:luke] 2012-06-25 09:22:04 PDT
(In reply to Bobby Holley (:bholley) from comment #65)
If IM never crosses globals in jit code (and thus always reports the correct "current global"), what is the problem?
Comment 67 Bobby Holley (busy) 2012-06-25 09:34:01 PDT
(In reply to Luke Wagner [:luke] from comment #66)
> (In reply to Bobby Holley (:bholley) from comment #65)
> If IM never crosses globals in jit code (and thus always reports the correct
> "current global"), what is the problem?

Is there always guaranteed to be at least one JSStackFrame for every compartment entered?
Comment 68 Luke Wagner [:luke] 2012-06-25 09:41:04 PDT
Yes: IM always enters jit code with the entry function's StackFrame pushed.  From that initial stack frame, all calls stack within the same global.  Any cross-compartment call will do the normal C++ CrossCompartmentWrapper::call path, which will push at least one more StackFrame before entering jit code again.  The only way this could break is if two frames in the same global weren't equivalent from a security POV.  (Frame annotations were one such case but, as you know, we have another hack to deal with that ;)

Regardless, removing context-pushing and bug 767938 sound like most excellent things to do.  IIUC, this would get us a long way toward removing JSContext (or, with super-snappy, having one per thread).
Comment 69 Bobby Holley (busy) 2012-06-28 02:54:39 PDT
I've got patches to fix up event dispatch to only push a context when script is actually being entered. This is green on try, save one failure on android M-3:

https://tbpl.mozilla.org/?tree=Try&rev=628da1b54855

Now I need to figure out how to look into that...
Comment 70 Bobby Holley (busy) 2012-06-28 03:29:16 PDT
Created attachment 637446 [details] [diff] [review]
Part 7.2 - Use the safe JSContext rather than the current JSContext in IndexedDBDatabaseParent::HandleDatabaseEvent. v1

It's not used for anything substantial, and it can be (or always is?) called with no JS on the stack.
Comment 71 Bobby Holley (busy) 2012-06-28 03:30:40 PDT
Smaug pointed out that I need to pop the context if I don't repush, since we're in a loop. Gave this another push to try. Crossing fingers:

https://tbpl.mozilla.org/?tree=Try&rev=dd8b165df73c
Comment 72 Bobby Holley (busy) 2012-06-28 06:07:43 PDT
Huzzah - It worked! Uploading the last patch, and flagging smaug for review.
Comment 73 Bobby Holley (busy) 2012-06-28 06:08:41 PDT
Created attachment 637487 [details] [diff] [review]
Part 7.3 - Only push the context of the event target if the listener is scripted. v2
Comment 74 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-28 09:28:57 PDT
Comment on attachment 637487 [details] [diff] [review]
Part 7.3 - Only push the context of the event target if the listener is scripted. v2

>     // compiled the event handler itself go ahead and compile it
>-    if ((ls.mFlags & NS_PRIV_EVENT_FLAG_SCRIPT) && ls.mHandlerIsString) {
>+    if ((ls.mListenerType == LISTENER_TYPE_JSEVENTLISTENER) &&
>+        ls.mHandlerIsString)
>+    {
{ should be in the previous line

>+typedef enum {
{ should be on its own line

>+    LISTENER_TYPE_NATIVE = 0,
>+    LISTENER_TYPE_JSEVENTLISTENER,
>+    LISTENER_TYPE_WRAPPEDJS,
>+    LISTENER_TYPE_COUNT
Enum values should be in form eValueName

> struct nsListenerStruct
> {
>   nsRefPtr<nsIDOMEventListener> mListener;
>   PRUint32                      mEventType;
>   nsCOMPtr<nsIAtom>             mTypeAtom;
>   PRUint16                      mFlags;
>   bool                          mHandlerIsString;
>-  bool                          mWrappedJS;
>+  nsListenerType                mListenerType;
Is it guaranteed that PRUint16 + bool + nsListenerType <= 4 bytes?
We don't want to increase the size of nsListenerStruct unless really needed.

>+  // We're about to create a new nsJSEventListener, which means that we're
>+  // responsible for pushing the context of the event target. See the similar
>+  // comment in nsEventManagerListener.cpp.
>+  nsCxPusher pusher;
>+  if (!pusher.Push(aTarget))
>+    return NS_ERROR_FAILURE;
NS_ENSURE_STATE(pusher.Push(aTarget));


Those addressed, r=me
Comment 77 :Ms2ger 2012-06-29 05:21:44 PDT
Comment on attachment 630201 [details] [diff] [review]
Part 4 - Disallow calling EvaluateString{,WithValue} with a principal that doesn't match the global. v1

So, can we remove those principal arguments at some point?
Comment 78 Bobby Holley (busy) 2012-06-29 11:49:38 PDT
(In reply to :Ms2ger from comment #77)
> So, can we remove those principal arguments at some point?

Yep. Feel free to file a bug (and also one for removing the vestigial doGetObjectPrincipal_old stuff). We probably want to leave it the way it is for 6 weeks or so, and then rip it out.

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