Closed Bug 952192 Opened 10 years ago Closed 10 years ago

Add Components.utils.getObjectPrincipal()

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: msucan, Assigned: gkrizsanits)

References

Details

Attachments

(1 file)

Follow up from bug 843004 comment 55:

Bobby Holley said:
> (In reply to Mihai Sucan [:msucan] from bug 843004 comment #50)
>> True, same-origin, system/chrome code that inspects and evals code in other
>> chrome windows. Use case: see the browser console. I need to check if the
>> object is from the same-origin or if it's not. If yes, then I expect it to
>> be "safe" for inspection.
>
> Ok. I'd be up for adding Cu.getObjectPrincipal(obj) (where wrapper-valued
> |obj| arguments would be unwrapped before getting the principal). You
> could then do Cu.getObjectPrincipal(obj).equals(Cu.getObjectPrincipal(yourGlobal)).

Additionally, it would be very useful for us to have an API to tell the difference between JS objects from jetpack addons and XUL extensions.
(In reply to Mihai Sucan [:msucan] from comment #0)

> Additionally, it would be very useful for us to have an API to tell the
> difference between JS objects from jetpack addons and XUL extensions.

How do you want to do that, exactly? In the current world, you could just detect whether the global is a sandbox or a window. But the e10s team has plans to hoist XUL extensions (at least overlays) into sandboxes as well.
(In reply to Bobby Holley (:bholley) from comment #1)
> (In reply to Mihai Sucan [:msucan] from comment #0)
> 
> > Additionally, it would be very useful for us to have an API to tell the
> > difference between JS objects from jetpack addons and XUL extensions.
> 
> How do you want to do that, exactly? In the current world, you could just
> detect whether the global is a sandbox or a window. But the e10s team has
> plans to hoist XUL extensions (at least overlays) into sandboxes as well.

That sounds good then. How does one check if a given object is in a sandbox?
(In reply to Mihai Sucan [:msucan] from comment #2)
> > How do you want to do that, exactly? In the current world, you could just
> > detect whether the global is a sandbox or a window. But the e10s team has
> > plans to hoist XUL extensions (at least overlays) into sandboxes as well.
> 
> That sounds good then. How does one check if a given object is in a sandbox?

Er, what? I'm saying that using the presence of a sandbox to differentiate jetpack and XUL will work now, but will break very soon.
(In reply to Bobby Holley (:bholley) from comment #3)
> (In reply to Mihai Sucan [:msucan] from comment #2)
> > > How do you want to do that, exactly? In the current world, you could just
> > > detect whether the global is a sandbox or a window. But the e10s team has
> > > plans to hoist XUL extensions (at least overlays) into sandboxes as well.
> > 
> > That sounds good then. How does one check if a given object is in a sandbox?
> 
> Er, what? I'm saying that using the presence of a sandbox to differentiate
> jetpack and XUL will work now, but will break very soon.

Sorry, I think I misunderstood "but the e10s team has plans to hoist XUL extensions (at least overlays) into sandboxes as well".

To answer the question "how do you want to do that?" This is what I would like suggestions for from you. Could we have an API like Cu.isObjectFromAddon()/isGlobalFromAddon()? This is overgeneralizing, so I am asking for better ideas.
Hm, maybe it was I who misunderstood you. :-)

Are you trying to tell the difference between Jetpack addons and XUL addons? Or are you trying to tell the difference between addons and everything else?
(In reply to Bobby Holley (:bholley) from comment #5)
> Hm, maybe it was I who misunderstood you. :-)

No worries. :) Thanks for your help with this.

> Are you trying to tell the difference between Jetpack addons and XUL addons?
> Or are you trying to tell the difference between addons and everything else?

I'd like to tell the difference between objects from addons and everything else. (where "addons" means both jetpack and XUL ones) This would be the Cu.isObjectFromAddon()/isGlobalFromAddon() method I mentioned in my previous comment.

With Cu.getObjectPrincipal() we'll be able to tell if object X is from the system principal or from some other principal. However, as I understood from the discussion we had in bug 843004, objects from addons also share the system principal. For pretty output of objects we can trust to read properties and execute methods from chrome code (system principal), but I would like to not give the same "trust" to objects from addons (if possible).
Assignee: nobody → gkrizsanits
(In reply to Mihai Sucan [:msucan] from comment #6)
> I'd like to tell the difference between objects from addons and everything
> else. (where "addons" means both jetpack and XUL ones) This would be the
> Cu.isObjectFromAddon()/isGlobalFromAddon() method I mentioned in my previous
> comment.

I've been thinking about this and I'm leaning toward not doing this. Problem is
that there is nothing that would block add-ons to spawn their own windows I guess...

Even jetpack addons add iframes to the hidden window, and there are plans to use
the invisible docshell instead of sandboxes... So it's not trivial to get this right.

System principal means trusted code, so we shouldn't really make a distinction between
addons and not addons, the very same issues can happen with our internal scripts (just
it's less likely), so we should do the same safety check in every case probably. 

> 
> With Cu.getObjectPrincipal() we'll be able to tell if object X is from the
> system principal or from some other principal. However, as I understood from
> the discussion we had in bug 843004, objects from addons also share the
> system principal. For pretty output of objects we can trust to read
> properties and execute methods from chrome code (system principal), but I
> would like to not give the same "trust" to objects from addons (if possible).

So I'm going to add getObjectPrincipal and help you with getting those extra checks
you will need before those property checks to avoid any unwanted side effects in the
rare cases.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7)
> ...
> 
> So I'm going to add getObjectPrincipal and help you with getting those extra
> checks
> you will need before those property checks to avoid any unwanted side
> effects in the
> rare cases.

Thank you! No problem about the addons distinction - that request was optional / "nice to have" (for now). The getObjectPrincipal() is sufficient for us to allow pretty object output in the browser console.
I ended up turning getObjectPrincipal on nsIScriptSecurityManager into a script callable function since that is where this really belongs to. Cleaned up patch is coming soon...

https://tbpl.mozilla.org/?tree=Try&rev=cf77d139632f
To make it script available it has to operate on Value instead of JSObject, so I got rid of the old version, which only simplified things I think, and added the new script available one.
Attachment #8357154 - Flags: review?(bobbyholley+bmo)
Comment on attachment 8357154 [details] [diff] [review]
nsIScriptSecurityManager.getObjectPrincipal should be script available

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

Nice - this is better than my proposal. r=bholley
Attachment #8357154 - Flags: review?(bobbyholley+bmo) → review+
sorry had to backout this changes for XPC testfailures like https://tbpl.mozilla.org/php/getParsedLog.php?id=32754667&tree=Mozilla-Inbound
It was my fault. I should have added the test file one line higher. Try build gave this error too, and I accidentally thought it's one of the intermittent failures that test always hits.

second attempt:

https://hg.mozilla.org/integration/mozilla-inbound/rev/595fd5dfbb5a
https://hg.mozilla.org/mozilla-central/rev/595fd5dfbb5a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: