XOWs should check the full stack for UniversalXPConnect access

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: mayhemer, Assigned: mrbkap)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

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

Updated

9 years ago
Summary: Change set 5c4c1f31d00b causes NS_ERROR_XPC_HAS_BEEN_SHUTDOWN → Change set 5c4c1f31d00b (Merge mozilla-central to tracemonkey) causes NS_ERROR_XPC_HAS_BEEN_SHUTDOWN
(Reporter)

Updated

9 years ago
Blocks: 475053
Uh...  How could that changeset cause this bug?  That seems highly unlikely.  It's marging changes over from m-c to t-m!
That is, if you used hg bisect you ended up with the wrong changeset due to multiple branches/merges hanging around....
OK, so the ccx has a null mWrapper.  

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

Comment 5

9 years ago
Did someone say "wrapper"?
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...
Assignee: general → nobody
Component: JavaScript Engine → XPConnect
QA Contact: general → xpconnect
Summary: Change set 5c4c1f31d00b (Merge mozilla-central to tracemonkey) causes NS_ERROR_XPC_HAS_BEEN_SHUTDOWN → Failure to unwrap XOW should lead to useful security exception in quickstubs
(Reporter)

Comment 7

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

Comment 9

9 years ago
Adding UniversalXPConnect privilege to checkIframe function (where the exception is thrown) helps.
(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.
Assignee: nobody → mrbkap
Summary: Failure to unwrap XOW should lead to useful security exception in quickstubs → XOWs should check the full stack for UniversalXPConnect access
I filed bug 505915 on the remaining problem.
Created attachment 390294 [details] [diff] [review]
Proposed fix
Attachment #390294 - Flags: superreview?(bzbarsky)
Attachment #390294 - Flags: review?(jst)

Updated

9 years ago
Attachment #390294 - Flags: review?(jst) → review+
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?
Attachment #390294 - Flags: superreview?(bzbarsky) → superreview+
(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.
> 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...
(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.
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.
Created attachment 391216 [details] [diff] [review]
With that
Attachment #390294 - Attachment is obsolete: true
Attachment #391216 - Flags: superreview?(bzbarsky)
Attachment #391216 - Flags: review?(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
Attachment #391216 - Flags: review?(jst) → review+
Attachment #391216 - Flags: superreview?(bzbarsky) → superreview+
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.
I filed bug 508928 on comment 20, point 2.
Created attachment 393079 [details] [diff] [review]
And that
Attachment #391216 - Attachment is obsolete: true
Attachment #393079 - Flags: superreview+
Attachment #393079 - Flags: review+
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Change the test?
(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.
Created attachment 406608 [details] [diff] [review]
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.
Attachment #393079 - Attachment is obsolete: true
Attachment #406608 - Flags: superreview?(bzbarsky)
Attachment #406608 - Flags: review?(jst)
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...
(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.
Status: REOPENED → ASSIGNED
> Yes, because we have to distinguish

But after this patch |fp| is unused in that function, no?
I am clearly failing hard at this bug.
Created attachment 406799 [details] [diff] [review]
Updated again

http://pastebin.mozilla.org/676963 is the interdiff.
Attachment #406608 - Attachment is obsolete: true
Attachment #406799 - Flags: superreview?(bzbarsky)
Attachment #406799 - Flags: review?(jst)
Attachment #406608 - Flags: superreview?(bzbarsky)
Attachment #406608 - Flags: review?(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.
Attachment #406799 - Flags: review?(jst) → review+
Comment on attachment 406799 [details] [diff] [review]
Updated again

sr=bzbarsky.  Thanks for the interdiff!
Attachment #406799 - Flags: superreview?(bzbarsky) → superreview+
Created attachment 407378 [details] [diff] [review]
With mochitests

This is what I'll land when the tree is next green.
Attachment #406799 - Attachment is obsolete: true
Attachment #407378 - Flags: superreview+
Attachment #407378 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/61ad31ff9d9e with a couple of fixes.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/mozilla-central/rev/5685c452dafd reflects the fact that I had to relax the restriction on setting properties through UniversalXPConnect.
You need to log in before you can comment on or make changes to this bug.