504877 - XOWs should check the full stack for UniversalXPConnect access

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Honza Bambas (:mayhemer)]

Bug 475053 reveals a bug caused by this change set. When accessing property of an iframe inside an iframe I get NS_ERROR_XPC_HAS_BEEN_SHUTDOWN.

STR:
- push patch v2.1 from bug 475053
- run test http://localhost:8888/tests/toolkit/components/passwordmgr/test/test_prompt_async.html

You get "Operation failed because the XPConnect subsystem has been shutdown" nsresult: "0x80570033 (NS_ERROR_XPC_HAS_BEEN_SHUTDOWN)" location: "JS frame :: http://localhost:8888/tests/toolkit/components/passwordmgr/test/test_prompt_async.html :: checkIframe :: line 395" data: no"

The exception is raised from js\src\xpconnect\src\xpcquickstubs.cpp (a generated file) line 944, function xpc_qsUnwrapThisFromCcxImpl. From some reason ccx.GetIdentityObject() returns null.

The structure of the test is: test_prompt_async.html refers subtst_prompt_async.html in an iframe ('iframe1') that refers 3 iframes refering authenticate.sjs pages. The code is (in a nut shell): iframe1.contentDocument.getElementById("iframe1").contentDocument.getElementById("ok").textContent. getElementById call throws the exception (dom_quickstubs.cpp|nsIDOMDocument_GetElementById).

Comment 1

[Honza Bambas (:mayhemer)]

http://hg.mozilla.org/mozilla-central/rev/5c4c1f31d00b

Comment 2

[Boris Zbarsky [:bzbarsky]]

Uh...  How could that changeset cause this bug?  That seems highly unlikely.  It's marging changes over from m-c to t-m!

Comment 3

[Boris Zbarsky [:bzbarsky]]

That is, if you used hg bisect you ended up with the wrong changeset due to multiple branches/merges hanging around....

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, so the ccx has a null mWrapper.  

The |obj| that the ccx was initialized with is an XPCCrossOriginWrappe, looks like.

Comment 5

[Andreas Gal :gal]

Did someone say "wrapper"?

Comment 6

[Boris Zbarsky [:bzbarsky]]

And, and as expected the issue here is that unwrapping the XPCCrossOriginWrapper fails a security check.  When unwrapping, the subject principal has "http://localhost:8888/" as the scheme+host+port and the object principal has "http://example.com/" as the scheme+host+port.

The exact URIs are:

"http://localhost:8888/tests/toolkit/components/passwordmgr/test/test_prompt_async.html"

"http://example.com/tests/toolkit/components/passwordmgr/test/authenticate.sjs?r=1&user=user3name&pass=user3pass&realm=mochirealm3&proxy_user=proxy_user2&proxy_pass=proxy_pass2&proxy_realm=proxy_realm2"

So this call should most certainly be throwing, assuming those principals are correct (and from reading the testcase source, they look correct to me).  If it wasn't before, that was a serious bug.

Now there _is_ an issue here, which is that XPCWrapper::UnwrapXOW eats the useful rv and just returns null, then this all propagates up to the XPCCallContext constructor, which treats lack of mWrapper and lack of mCurrentJSObject as par for the course and returns.  Then it has a null mWrapper, so hands back a null identity object, etc.

Maybe we could just hack quickstubs to use a different error code if we have a null identity object and the given object is a XOW?  That would be pretty simple, and security check failure is a heck of a lot more likely here than XPConnect being shut down, if the object is a XOW...  That still won't be as helpful as the actual error messages secman produces, so if we can figure out a way to get those to happen here, we should.  Would have saved Honza a lot of effort...

Comment 7

[Honza Bambas (:mayhemer)]

Notice UniversalXPConnect privilege in the localhost:8888 test body. 

It behaves correctly - got a security check failure - on 1.9.1 (either central)
_w/o_ the UniveralXPConnect privilege. This bug is valid and there is no need to report another one, I should be able
to access property of iframe inside an iframe when UniveralXPConnect _is_
engaged. That is what has been broken. 

I checked that when all subframes are from the same origin (localhost:8888) I
*CAN* access the properties. So, this is related to security checks.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Blake, is the issue here that Honza is running into that XOW only checks for privileges on the topmost stack frame?  Do we need a separate bug on that, or is that by-design?

Comment 9

[Honza Bambas (:mayhemer)]

Adding UniversalXPConnect privilege to checkIframe function (where the exception is thrown) helps.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #8)
> Blake, is the issue here that Honza is running into that XOW only checks for
> privileges on the topmost stack frame?  Do we need a separate bug on that, or
> is that by-design?

I'll use this bug for the UniversalXPConnect issue and another bug for the current title of this one.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

I filed bug 505915 on the remaining problem.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 390294 [details] [diff] [review]
Proposed fix

OK, but do we need to still do the frame annotation dance above that?  And do we not need to update nsContentUtils::CanAccessNativeAnon to match this code?

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #13)
> OK, but do we need to still do the frame annotation dance above that?

Technically, we don't, but I left it in as a fast path. I guess I could remove it if you don't think it's worth it.

> And do
> we not need to update nsContentUtils::CanAccessNativeAnon to match this code?

Oops, we do. I'll do that ASAP.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Technically, we don't, but I left it in as a fast path.

What does it fast-path?  The system principal, I guess?

We should have comments both here and in CanAccessNativeAnon saying to update the other on changes...

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #15)
> What does it fast-path?  The system principal, I guess?

Also the case where the top frame is UniversalXPConnect enabled.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I don't care about optimizing the enablePrivilege case much...  So if we need this for system (and expect to hit this code a lot with the system principal), we can keep the fast path, with a comment to that effect.  Otherwise we can remove it.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With that

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 391216 [details] [diff] [review]
With that

- In nsContentUtils::CanAccessNativeAnon():

+  if (NS_SUCCEEDED(sSecurityManager->IsSystemPrincipal(principal, &privileged)) &&
       privileged) {
     // UniversalXPConnect things are allowed to touch us.

Update the comment here...

- In AllowedToAct() (XPCSystemOnlyWrapper.cpp):

+  if (NS_SUCCEEDED(ssm->IsSystemPrincipal(principal, &privileged)) &&
       privileged) {
     // UniversalXPConnect things are allowed to touch us.

... and here.

r=jst

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 391216 [details] [diff] [review]
With that

Looks ok with:

1)  jst's comments addressed
2)  Comments in all three of these places (content utils, XOW, SOW) that need to have identical code.  Or better yet a bug on refactoring those, in addition to the comments.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

I filed bug 508928 on comment 20, point 2.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: And that

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/5ea7f2882982

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

I had to back this out -- it caused mochitest orange (in particular on docshell/test/navigation/test_grandchild.html). I think the problem is that before, we were always ignoring the UniversalXPConnect that the script requests. Now, we're getting a mix of some "allowed to access" and some "not allowed to access" behavior, which is confusing the XOW code.

I'll look into this more tomorrow.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

The other day, I tried to to make cross-origin+UniversalXPConnect have XPCNativeWrapper-like behavior, but there's another test that relies on cross-origin XOWs+UniversalXPConnect being able to access arbitrary properties of the underlying object. I'm still thinking about what to do here.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Change the test?

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #26)
> Change the test?

I thought about this for a while and realized that with the tools given, it is either extremely hard or impossible to use UniversalXPConnect and XOWs with this bug. Basically, you have to manually JS_ClearScope whenever you leave a block with UniversalXPConnect after having used a XOW. Rather than introduce such complexity into the tests, I'll fix this in XOWs by detecting that we've switched from UniversalXPConnect to non- or vice-versa and JS_ClearScope there.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated fix

So, the way this patch works is a little subtle. Basically, we refuse to *set* any properties on a cross-origin XOW w/ universalxpconnect because the JS API doesn't let us have fine-grained enough control over which object gets the property. When we're resolving a property, we quietly substitute our UniversalXPConnect object for ourselves, which means that we tell the engine "hey, that object over there has the property" and it leave our object alone. Note that when we actually *call* the resolved function, 'this' will be *our* object, not the secret one... That one never leaks to script.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Why is the change to AllowedToAct in the SOW code ok?

Do you still need the GetCxSubjectPrincipalAndFrame in XPCNativeWrapper::GetWrappedNative?

In GetUXPCObject, don't you need to unset sUXPCObjectSlot if one of the things after you set it fails?

Would be good to document the UXPC setup somewhere around GetUXPCObject or something...

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #29)
> Why is the change to AllowedToAct in the SOW code ok?

Yikes, sorry, bad merge.

> Do you still need the GetCxSubjectPrincipalAndFrame in
> XPCNativeWrapper::GetWrappedNative?

Yes, because we have to distinguish between "this principal is valid because there is some code running" and "we returned this random principal to you courtesy of some random context's global object".

> In GetUXPCObject, don't you need to unset sUXPCObjectSlot if one of the things
> after you set it fails?

Yeah. I've flipped this around so we set the slot *last*.

> Would be good to document the UXPC setup somewhere around GetUXPCObject or
> something...

Working on the comment now.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> Yes, because we have to distinguish

But after this patch |fp| is unused in that function, no?

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

I am clearly failing hard at this bug.

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated again

http://pastebin.mozilla.org/676963 is the interdiff.

Comment 34

[Johnny Stenback (:jst)]

Comment on attachment 406799 [details] [diff] [review]
Updated again

r=jst, but I'd love to see this go in with dedicated tests for this.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 406799 [details] [diff] [review]
Updated again

sr=bzbarsky.  Thanks for the interdiff!

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With mochitests

This is what I'll land when the tree is next green.

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/61ad31ff9d9e with a couple of fixes.

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/5685c452dafd reflects the fact that I had to relax the restriction on setting properties through UniversalXPConnect.