1487032 - Store origin/site info in CompartmentPrivate

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P2)

Description

[Jan de Mooij [:jandem]]

There are a few related things we need on the CompartmentPrivate:

A) An origin string. Then when we create a new realm in a given zone, we can look for an existing compartment with a matching origin and share that compartment. An equivalent option is to grab a random realm (there must be at least one realm in every compartment) and get its principal's origin.

B) A flag indicating whether any global has set document.domain.

C) Once WindowProxy and Location do security checks (bug 1363208), the new Wrap(compartment1, compartment2) algorithm will need to check whether two origins are same-site. If not same-site, we will return an opaque wrapper because there's no way these compartments can access each other. (If they *are* same-site, we can also check the document.domain flag, see B.)

Boris suggested storing a SiteIdentifier containing {origin attributes, scheme, base domain, maybe port} to allow fast same-site checks. We could create it as follows, if we don't have code for this yet:

<quote bz>
1) Split the string on '^' to get the origin-without-attributes and origin attributes.  Maybe this should be done via OriginAttributes::PopulateFromOrigin, but I'm not sure whether we need all the parsing that does here.

2) Compare the origin attributes.  If not same, return false.

3) Compare the origin-without-attributes strings.  If same, return true.

4) Try to create URIs from both origin-without-attributes strings.  If either fails return false.

5) Ask the nsIEffectiveTLDService for the base domains for the URIs and compare them.  Also compare the schemes and ports?  Not sure whether document.domain can cross ports.
</quote bz>

Comment 1

[Jan de Mooij [:jandem]]

One question is: how do we want wrap() and the SiteIdentifier to behave when expanded principals are involved?

Comment 2

[Bobby Holley (:bholley)]

Why do we need SiteIdentifier rather than just storing a principal, and not exposing the path bits of the underlying URI?

Comment 3

[Boris Zbarsky [:bzbarsky]]

A "site" in this case is "tld+1" (as in "can touch each other if both munge document.domain").  While we _can_ derive this from the URI in the principal (or just a URI in general) on every wrap operation, I worry about that being slow.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3)
> A "site" in this case is "tld+1" (as in "can touch each other if both munge
> document.domain").  While we _can_ derive this from the URI in the principal
> (or just a URI in general) on every wrap operation, I worry about that being
> slow.

Ah, right - of course.

In any case, splitting on the ^ shouldn't be necessary, because BasePrincipal already has an atomized version of the stringified origin attributes. So I think we just want to atomize the eTLD+1 origin (including scheme and port), and then we can do atom comparisons.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #4)
> In any case, splitting on the ^ shouldn't be necessary, because
> BasePrincipal already has an atomized version of the stringified origin
> attributes. So I think we just want to atomize the eTLD+1 origin (including
> scheme and port), and then we can do atom comparisons.

And in fact, it may just make sense to have SiteIdentifier just wrap BasePrincipal, indicating a guarantee that any codebase principals inside have been stripped to eTLD+1 (including those embedded within expanded principals). That will let us leverage the existing comparison logic for all the different principal types (System, Expanded, Codebase, Nonce), and avoid duplicating it.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: WIP

This adds a SiteIdentifier class (wrapping a BasePrincipal*) + a virtual GetSiteIdentifier method on BasePrincipal. We then store the SiteIdentifier in CompartmentOriginInfo, along with the origin principal and a had-document-domain changes flag.

I'm sure this is still full of problems but I'm just curious if this is roughly what people had in mind (I'll be on PTO the rest of the day).

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 9005181 [details] [diff] [review]
WIP

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

Just had a ~60 second look, but generally seems like the direction I was envisioning. Thanks!

::: js/xpconnect/src/xpcprivate.h
@@ +2912,5 @@
> +    // All globals in the compartment must have principals equal() to this one.
> +    // Note that individual globals and principals can have their domain changed
> +    // via document.domain, so this principal must not be used for things like
> +    // subsumesConsideringDomain. Use JS::GetRealmPrincipals for that.
> +    RefPtr<mozilla::BasePrincipal> mOrigin;

Why do we have this if we also have mSite below?

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Bobby Holley (:bholley) from comment #7)
> Just had a ~60 second look, but generally seems like the direction I was
> envisioning.

Great, thanks!

> > +    RefPtr<mozilla::BasePrincipal> mOrigin;
> 
> Why do we have this if we also have mSite below?

With compartment-per-origin I thought a.foo.com and b.foo.com would always be in different compartments but they do have the same site (= foo.com). We'd use mOrigin to determine which globals can end up in the same compartment. Does that make sense?

Comment 9

[Bobby Holley (:bholley)]

(In reply to Jan de Mooij [:jandem] from comment #8)
> With compartment-per-origin I thought a.foo.com and b.foo.com would always
> be in different compartments but they do have the same site (= foo.com).
> We'd use mOrigin to determine which globals can end up in the same
> compartment. Does that make sense?

Yes it does - sorry I keep forgetting the finer points of the plan.

So we're generally aiming for compartment-per-origin, but need to dynamically treat different compartments as same-origin if they're same-site and both had document.domain set - is that right? If so, maybe add a couple comments to that effect?

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> Also compare the schemes and ports?  Not sure whether
> document.domain can cross ports.

After reading the code and doing some experiments:

1) The document.domain setter code ignores the port value if it's present. Setting domain to "foo.com:2974" does exactly the same thing as setting it to "foo.com". Both will result in (host = foo.com, port = -1/null).

2) Origins foo.com:1111 and foo.com:2222 can set document.domain to "foo.com" to communicate.

So I need to change the patch to also clear the port number for codebase URIs in site identifiers.

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #10)
> 1) The document.domain setter code ignores the port value if it's present.

FWIW, Chrome and Safari reject setting document.domain to something that includes a port number. It works for us because in CreateInheritingURIForHost we call SetPort(-1) followed by SetHostPort(aHostString). The port number has no effect because we call this a second time with just the host name.

I think their behavior matches the spec? I'll take a closer look.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1487032 - Store origin/site info in CompartmentPrivate. r?bholley

This will let us answer the following questions (in a performant way):

1) What's the compartment's origin? Necessary to implement compartment-per-origin.
2) What's the origin's site? Necessary for the new Wrap() algorithm.
3) Has any realm in the compartment set document.domain? Necessary for the new Wrap() algorithm.

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 9007763 [details]
Bug 1487032 - Store origin/site info in CompartmentPrivate. r?bholley

Bobby Holley (:bholley) has approved the revision.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/416aff73b2ee
Store origin/site info in CompartmentPrivate. r=bholley

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/416aff73b2ee