Closed Bug 1471496 Opened Last year Closed 7 months ago

Change the way we do cross-compartment wrapper for Window and Location

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Regressed 1 open bug)

Details

Attachments

(2 files)

Once bug 1363208 is fixed, we will no longer need to use filtering Xrays for cross-origin Window and Location.  We'll still want Xrays there for the chrome-to-content or extension-to-content cases, but for the normal web-exposed case we'll want a special CCW that has the following properties:

1)  Is fully transparent (like a normal transparent CCW).
2)  Does not enter the compartment of the target proxy (like Xrays).

We will need to ensure that Window and Location proxy handlers can handle being called in a compartment different from the WindowProxy/Location compartment.
Once this is done, the incumbent global usage from bug 1363208 can be removed in favor of the current global, because of point 2 above.
So CrossCompartmentWrapper inherits from Wrapper and Wrapper inherits from ForwardingProxyHandler...

I think it should be possible to add OurNewWrapper that also inherits from Wrapper but just uses the default traps provided by ForwardingProxyHandler? Another thought: maybe OurNewWrapper could assert it only wraps wrappers/proxies (it seems that will be true for Location and Window) and then it could just forward each trap directly to the target proxy's handler.

I don't know if that makes sense, but I'm happy to help out with wrapper stuff on the JS side if needed.
(In reply to Jan de Mooij [:jandem] from comment #2)
> I think it should be possible to add OurNewWrapper that also inherits from
> Wrapper but just uses the default traps provided by ForwardingProxyHandler?

Er, OurNewWrapper would have to inherit from *CrossCompartmentWrapper*.
Priority: -- → P2
Blocks: 1514050
Depends on: 1521906

The end result we want is that on the web cross-compartment wrappers for
WindowProxy and Location are always CrossOriginObjectWrapper. That needs to be true
for both cases that are different-origin (as now) and cases that are
same-origin, since they might become different-origin due to document.domain
changes but we don't want that to affect the wrappers involved.

On the web, all security checks are symmetric, so in WrapperFactory::Rewrap we
would have originSubsumesTarget == targetSubsumesOrigin in all web cases.

I claim that

originSubsumesTarget == targetSubsumesOrigin &&
(!targetSubsumesOrigin ||
(!originCompartmentPrivate->wantXrays &&
!targetCompartmentPrivate->wantXrays)) &&
"object is a WindowProxy or Location"

is a necessary and sufficient condition for using CrossOriginObjectWrapper.

Comparing to our current code, if originSubsumesTarget and targetSubsumesOrigin
are both false, then for the WindowProxy and Location cases we currently end up
with the following arguments to SelectWrapper:

securityWrapper: true
xrayType: XrayForDOMObject
waiveXrays: false

So SelectWrapper ends up returning CrossOriginObjectWrapper, which the new
condition keeps doing.

If originSubsumesTarget and targetSubsumesOrigin are both true, then there are
two cases. If both compartments have wantXrays false (which is always the case
on the web), then we end up with the following arguments to SelectWrapper:

securityWrapper: false
xrayType: NotXray
waiveXrays: false

and SelectWrapper returns CrossCompartmentWrapper. We want to do
CrossOriginObjectWrapper instead, as explained above.

Finally, if originSubsumesTarget and targetSubsumesOrigin are both true but one
of the compartments has wantXrays set, then we get:

securityWrapper: false
xrayType: XrayForDOMObject
waiveXrays: might be true or false

and then SelectWrapper might return a WaiveXrayWrapper or a PermissiveXrayDOM.
In this case we do not want to start returning CrossOriginObjectWrapper, and
this is a non-web case anyway, since web compartments can't set wantXrays.

Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ff333373fe4
part 1.  Fix IsPlatformObjectSameOrigin to do the right thing when we're doing first-party isolation but turning off its effects on scripted property access.  r=bholley
https://hg.mozilla.org/integration/autoland/rev/9658187a54fb
part 2.  Change the way we do cross-compartment wrappers for Window and Location so they don't ever need to be recomputed.  r=bholley

Log from failure above:

Found black to gray edge to gray Shape 0x24a0462d6970
  from black Object Proxy 0x24a046214080 (compartment 0x122010550) shape edge
  from black Object Window 0x24a046224100 (compartment 0x122010550) object slot edge
  from black ObjectGroup 0x24a0462afd30 group_global edge
  from black Object Function 0x24a0462d1900 (compartment 0x122010550) group edge
  from black Shape 0x24a0462c3078 setter edge
  from gray Object Object 0x24a04624f280 (compartment 0x122010550) shape edge
  from gray Object HTMLDocument 0x24a046236130 (compartment 0x122010550) Shadowing DOM proxy expando edge
  from root Preserved wrapper
Source: object 24a046214080
  global 24a046224100 [Window]
  class 1104c5cd0 Proxy
  lazy group
  flags: maybe_has_interesting_symbol not_native
  proto <dynamic>
Found black to gray edge to gray Shape 0x24a0462d6970
  from black Object Proxy 0x24a046214080 (compartment 0x122010550) ProxyObject_shape edge
  from black Object Window 0x24a046224100 (compartment 0x122010550) object slot edge
  from black ObjectGroup 0x24a0462afd30 group_global edge
  from black Object Function 0x24a0462d1900 (compartment 0x122010550) group edge
  from black Shape 0x24a0462c3078 setter edge
  from gray Object Object 0x24a04624f280 (compartment 0x122010550) shape edge
  from gray Object HTMLDocument 0x24a046236130 (compartment 0x122010550) Shadowing DOM proxy expando edge
  from root Preserved wrapper
Source: object 24a046214080
  global 24a046224100 [Window]
  class 1104c5cd0 Proxy
  lazy group
  flags: maybe_has_interesting_symbol not_native
  proto <dynamic>
Assertion failure: js::CheckGrayMarkingState(mJSRuntime), at /builds/worker/workspace/build/src/xpcom/base/CycleCollectedJSRuntime.cpp:1119
Depends on: 1525346

Bug 1525346 tracks fixing that failure. Many thanks to jandem and jonco for figuring it out!

Flags: needinfo?(bzbarsky)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/317151999412
part 1.  Fix IsPlatformObjectSameOrigin to do the right thing when we're doing first-party isolation but turning off its effects on scripted property access.  r=bholley
https://hg.mozilla.org/integration/autoland/rev/00cdd5991ace
part 2.  Change the way we do cross-compartment wrappers for Window and Location so they don't ever need to be recomputed.  r=bholley

Backed out as Boris requested on irc because bug 1525346 was also backed out for failures:
backout link: https://hg.mozilla.org/integration/autoland/rev/bc6e94302fbf528815a89e33d9fc2fce8d7f1edf

Flags: needinfo?(bzbarsky)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/462cfc9e96dc
part 1.  Fix IsPlatformObjectSameOrigin to do the right thing when we're doing first-party isolation but turning off its effects on scripted property access.  r=bholley
https://hg.mozilla.org/integration/autoland/rev/4abfd3bb9934
part 2.  Change the way we do cross-compartment wrappers for Window and Location so they don't ever need to be recomputed.  r=bholley
Flags: needinfo?(bzbarsky)
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Assignee: nobody → bzbarsky
Depends on: 1530388
Depends on: 1530292
Component: DOM → DOM: Core & HTML
Regressions: 1557393
You need to log in before you can comment on or make changes to this bug.