Closed Bug 754202 Opened 12 years ago Closed 12 years ago

Pull principals directly off the context/object compartment

Categories

(Core :: Security: CAPS, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: bholley, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(11 files, 3 obsolete files)

1.67 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.21 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.53 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
25.30 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
7.10 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
5.08 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.27 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.20 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
942 bytes, patch
mrbkap
: review+
bent.mozilla
: review+
Details | Diff | Splinter Review
1.09 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
9.10 KB, patch
smaug
: review+
Details | Diff | Splinter Review
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. :-)
Summary: Pull principals directly off the context/caller compartment → Pull principals directly off the context/object compartment
Blocks: 754156
IonMonkey will break gecko security without this.
Blocks: IonMonkey
Make old_doGetObjectPrincipal DEBUG-only, please?
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.
Attachment #625948 - Flags: review?(mrbkap)
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.
Attachment #625951 - Flags: review?(bzbarsky)
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.
Attachment #625948 - Flags: review?(mrbkap) → review?(bzbarsky)
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 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!
Attachment #625950 - Flags: review?(bzbarsky) → review+
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....
Attachment #625951 - Flags: review?(bzbarsky) → review-
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.

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

See comment 4.
(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!
Hrm.  That seems ... highly suboptimal.  Did you look into why we have this test?  Is anything relying on the old behavior?
Comment 14 was in response to comment 12...
(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?
> 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.
(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?
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 on attachment 625948 [details] [diff] [review]
Part 1 - Fix content->chrome postMessage mochitest. v1

r=me
Attachment #625948 - Flags: review?(bzbarsky) → review+
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 :)
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 :-(
Attachment #625951 - Attachment is obsolete: true
Attachment #630200 - Flags: review?(mrbkap)
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'?
Attachment #630202 - Flags: review?(mrbkap)
No more principal pushing!
Attachment #630204 - Flags: review?(mrbkap)
(In reply to Bobby Holley (:bholley) from comment #25)
\o/ \o/ \o/ \o/ \o/ hell yeah \o/ \o/ \o/ \o/ \o/
Attachment #630200 - Attachment is obsolete: true
Attachment #630200 - Flags: review?(mrbkap)
Attachment #630223 - Flags: review?(mrbkap)
mrbkap said on IRC that we should just assert that js::GetContextCompartment never returns null.
Pushed the updated stack here to try: https://tbpl.mozilla.org/?tree=Try&rev=4a4414893cb1
Looks green :-)
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?
Attachment #630201 - Flags: review?(mrbkap) → review+
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.
Attachment #630202 - Flags: review?(mrbkap) → review+
Attachment #630203 - Flags: review?(mrbkap) → review+
Attachment #630204 - Flags: review?(mrbkap) → review+
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*.
Attachment #630223 - Flags: review?(mrbkap) → review+
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
Assignee: nobody → bobbyholley+bmo
Target Milestone: --- → mozilla16
Depends on: 763129
Depends on: 762920
Backed out patches 3-7 for various regressions:
http://hg.mozilla.org/mozilla-central/rev/90107a2a0c64
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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...
I redid the backout, but backing out all patches rather than just patches 3-7:
https://hg.mozilla.org/mozilla-central/rev/8d57c055f951
Depends on: 763433
Depends on: 763475
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.
Attachment #625950 - Attachment is obsolete: true
Attachment #632263 - Flags: review?(bzbarsky)
> 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.
(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
(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?
> 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.
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 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.
Attachment #632264 - Flags: review?(mrbkap) → review+
Comment on attachment 632263 [details] [diff] [review]
Part 2 - Pull object principals directly off the compartment. v4

Per discussion.
Attachment #632263 - Flags: review?(bzbarsky) → review-
(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?
Why is subject principal null. Shouldn't it be system.
Blocks: framesandbox
Depends on: 765168
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.
Attachment #633471 - Flags: review?(mrbkap)
Attachment #625950 - Attachment is obsolete: false
Attachment #632263 - Attachment is obsolete: true
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.
Attachment #633471 - Flags: review?(mrbkap)
Attachment #633471 - Flags: review?(bent.mozilla)
Attachment #633471 - Flags: review+
Last try push was borked. trying again: https://tbpl.mozilla.org/?tree=Try&rev=b004a98d59b5
Attachment #633471 - Flags: review?(bent.mozilla) → review+
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
ok, a11y bustage looks real. Firing up a linux vm to investigate.
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
Also, the relevant JS stack is here: http://pastebin.mozilla.org/1668826
Blocks: 766641
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.
(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)
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?
Depends on: 767938
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. :-(
(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?
(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?
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).
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...
It's not used for anything substantial, and it can be (or always is?) called with no JS on the stack.
Attachment #637446 - Flags: review?(bent.mozilla)
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
Huzzah - It worked! Uploading the last patch, and flagging smaug for review.
Attachment #637446 - Flags: review?(bent.mozilla) → review+
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
Attachment #637487 - Flags: review?(bugs) → review+
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?
(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.
Depends on: CVE-2012-4205
No longer depends on: CVE-2012-4205
Depends on: CVE-2012-3991
Depends on: 774633
Depends on: CVE-2012-4195
No longer depends on: CVE-2012-3991
Blocks: 783957
Depends on: CVE-2012-4192
Depends on: 801305
Depends on: 812546
Depends on: 825164
No longer depends on: 767938
You need to log in before you can comment on or make changes to this bug.