Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make COOP+COEP do not imply crossOriginIsolated. #6098

Merged

Conversation

ArthurSonzogni
Copy link
Member

@ArthurSonzogni ArthurSonzogni commented Oct 23, 2020

The specification currently requires COOP + COEP to give access to
crossOriginIsolated capabilities like SharedArrayBuffer.

Some platforms can't easily support multiple processes (like Android
Webview). Therefore, they can't really support crossOriginIsolated.
However the are no strong reasons for them not to enforce COEP (and
maybe COOP) when their associated headers are present.

It would be great enforcing COEP (and maybe COOP) on all platforms,
desptie the lack of crossOriginIsolated capabilities.

This patch makes the specification to allow (instead of requiring)
platform to set the crossOriginIsolated flag when both COOP and COEP are
used.

Setting crossOriginIsolated becomes platform dependent. In exchange, we
can enforce COEP (and COOP) in a non platform dependent way, without
conflicting with the specification about crossOriginIsolated.


/browsers.html ( diff )
/origin.html ( diff )
/structured-data.html ( diff )
/webappapis.html ( diff )
/window-object.html ( diff )
/workers.html ( diff )

@yutakahirano
Copy link
Member

yutakahirano commented Oct 23, 2020

@annevk @domenic

I think it's good to make BCG's cross-origin isolated and agent cluster's cross-origin isolated platform independent, and cross-origin isolated capability platform dependent. That means even on Android WebView document.domain mutation is no-op when COOP: same-origin and COEP: require-corp is specified. What do you think?

@ArthurSonzogni
Copy link
Member Author

ArthurSonzogni commented Oct 23, 2020

I think it's good to make BCG's cross-origin isolated and agent cluster's cross-origin isolated platform independent, and cross-origin isolated capability platform independent. That means even on Android WebView document.domain mutation is no-op when COOP: same-origin and COEP: require-corp is specified. What do you think?

I agree it would be a shame not restricting document.domain.

I have been a bit confused by all the different cross-origin-isolated concept. So we have:

  1. Browsing context group cross-origin isolated.
  2. Agent cluster's cross-origin isolated
  3. The windows's cross-origin isolated capability.

I guess I should just add a platform-specific exception to:
https://html.spec.whatwg.org/multipage/window-object.html#script-settings-for-window-objects:concept-settings-object-cross-origin-isolated-capability
instead.

@yutakahirano
Copy link
Member

My recommendation would be...

@domenic
Copy link
Member

domenic commented Oct 23, 2020

I agree with @yutakahirano's suggestions. To give more detail on a particularly tricky part:

Modifying logic at https://html.spec.whatwg.org/multipage/window-object.html#script-settings-for-window-objects and adding a note.

My suggestion concretely here is that before step 4, you establish a boolean. Something like:

  1. Let canBeCOI be true if the user agent is willing to give access to COI features to window, and false otherwise.

then you can modify the "cross-origin isolated capability" to be something like:

Return true if all of the following hold:

  • realm's agent cluster's cross-origin-isolated is true
  • canBeCOI is true
  • window's associated Document is allowed to use the "cross-origin-isolated" feature
    Otherwise, return false

Without establishing the boolean ahead of time, if you just wrote something like "return false at the user agent's discretion" in the COIC algorithm, it would be possible per-spec for the value to change over time.

@ArthurSonzogni
Copy link
Member Author

Thanks Yutaka and Domenic, this is really helpful!

@yutakahirano
Copy link
Member

@ArthurSonzogni , what's the status of this bug?

@ArthurSonzogni
Copy link
Member Author

@ArthurSonzogni , what's the status of this bug?

I just need to apply Domenic suggestions and yours. Nothing looks hard, I just need to produce some work ;-)

I was focused on my main project. Enabling COOP+COEP on every platforms is a side project that I wanted to see progress too, but this isn't my main priority and nobody really depends on this. Maybe you do somehow?

I've booked some time Wednesday for fixing this. I will update this thread.

@yutakahirano
Copy link
Member

Thank you!

@ArthurSonzogni
Copy link
Member Author

ArthurSonzogni commented Nov 18, 2020

Modifying logic at https://html.spec.whatwg.org/multipage/window-object.html#script-settings-for-window-objects and adding a note. ~Yutaka

My suggestion concretely here is that before step 4, you establish a boolean. Something like:
(...)
Without establishing the boolean ahead of time, if you just wrote something like "return false at the user agent's discretion" in the COIC algorithm, it would be possible per-spec for the value to change over time. ~Domenic

The section linked above is called once for every "window" object being created. So this doesn't really represent a "global" per-agent variable. Maybe there is another section that is called only once, where I can put this variable instead?

(Or maybe I can just add a note explaining the agent have to make a decision and not change its mind over time.

@domenic
Copy link
Member

domenic commented Nov 18, 2020

I don't know if it's important for the canBeCOI boolean to be the same across the entire agent. Having it be per-window seems reasonable, since it's just applying an implementation-defined restriction on top of the existing agent cluster-wide cross-origin-isolated situation.

@ArthurSonzogni ArthurSonzogni force-pushed the coop-coep-not-imply-cross-origin-isolated branch from 590a022 to c79e8f3 Compare November 18, 2020 17:56
@ArthurSonzogni
Copy link
Member Author

I made an update. It's a bit unfortunate that "canBeCOI" can't be made more global. I am defining from "Script settings for Window objects", but I also need to use it from the part about workers.

I would be happy to get some feedback from there. I still need to do the suggested change in the w3c/ServiceWorker repository.

@domenic
Copy link
Member

domenic commented Nov 18, 2020

If we wanted it to be more global, we could modify the agent cluster's cross-origin-isolated at the time that gets set (which is roughly at agent cluster creation time). However, I haven't thought through the full implications of that, and am unsure it's a great idea. I remember feeling that the current design you've uploaded is good, when we discussed this 26 days ago.

I'm currently attending BlinkOn so my time for nontrivial reviews like this one is a bit scarce. But as soon as I'm able (probably Friday?) I'll do a review of the latest changes you've made, and also think more deeply about whether it would be a good idea to make it change the agent cluster's cross-origin isolated. Thoughts from @yutakahirano or @annevk would be welcome in the meantime.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach is fine. If you wanted something more global you would have to change the browsing context group and agent cluster's cross-origin isolated concept to a tri-state I think, but I'm not sure what the observable differences would be. I would hope none.

source Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Nov 19, 2020

Hmm, thinking about it more that tri-state would be a lot cleaner as it more closely matches what an implementation would want to do. It would not process-isolate the agent cluster after all. I think it's worth another look to see how hard that would be.

@ArthurSonzogni
Copy link
Member Author

Hmm, thinking about it more that tri-state would be a lot cleaner as it more closely matches what an implementation would want to do. It would not process-isolate the agent cluster after all. I think it's worth another look to see how hard that would be.

That was my feeling since the beginning. I would be happy to do this.

So, I guess I would need to modify:
https://html.spec.whatwg.org/#bcg-cross-origin-isolated

Replace:

A browsing context group has a cross-origin isolated boolean. It is initially false.

by:

A browsing context group has a cross-origin-isolated tri-state. It can take the following values:
- process-isolation
- logical-isolation
- none

It is initially 'none'.

Then modify:
https://html.spec.whatwg.org/#browsing-context-group-switches-due-to-cross-origin-opener-policy:bcg-cross-origin-isolated

Replace:

If navigationCOOP is "same-origin-plus-COEP", then set newBrowsingContext's group's cross-origin isolated to true.

By:

If navigationCOOP is "same-origin-plus-COEP", then set newBrowsingContext's group's cross-origin isolated to
- 'process-isolation' if canBeCrossOriginIsolated
- 'logical-isolation' otherwise.

What do you think of this in general?

@annevk
Copy link
Member

annevk commented Nov 19, 2020

Yeah, I think that's right. And note that aside from the browsing context group's cross-origin isolated, you'll also need to do this for the similar boolean on agent cluster (which just copies the value from the browsing context group it's associated with).

Edit: I would try to avoid the word "process" to not bake particular computer architectures too much into the design, even though they are somewhat in the end.

@ArthurSonzogni
Copy link
Member Author

@annevk : The patch with your suggestions from #6098 (comment) is ready.
If you have some time, could you please take a look?

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good overall. My concerns are mainly about something that was already there (SAB protection) and the exact wording to use in the various places that need changes, but the architecture seems sound to me.

source Outdated
@@ -8078,7 +8078,8 @@ interface <dfn>DOMStringList</dfn> {
<span>agent cluster</span>.</p></li>

<li>
<p>If <var>agentCluster</var>'s <span>cross-origin isolated</span> is false, then throw a
<p>If <var>agentCluster</var>'s <span>cross-origin isolated</span> is not <code
data-x="bcg-isolation-physic">isolation-physic</code>, then throw a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this isn't checking the capability. Wouldn't that mean that a cross-origin document can use SharedArrayBuffer?

cc @domenic @yutakahirano

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now you say it, I am surprised too. My understanding is that we should check the capability.

That's what we implemented on Chrome here:
https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/execution_context/execution_context.cc;l=184;bpv=1;bpt=1?q=sharedarraybuffertran

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, this requires talking about the destination realms.

I had some questions about it previously: https://crbug.com/1144838
Here is a copy of my questions:

I discussed:
https://chromium-review.googlesource.com/c/chromium/src/+/2343658
with sigurds@. It made me ask myself several questions, that I answered by adding tests:
https://chromium-review.googlesource.com/c/chromium/src/+/2514211

I can now say the following:

From
- a COOP/COEP page,
- an OS without SharedArrayBuffer enabled by default (e.g. Android),
Then it is possible to share a SharedArrayBuffer from a non crossOriginIsolated document iframe.

Specifically, we have:
1. (sender.crossOriginIsolated == false) + (receiver.crossOriginIsolated == false) => Blocked.
2. (sender.crossOriginIsolated == true ) + (receiver.crossOriginIsolated == true ) => Transfered
3. (sender.crossOriginIsolated == true ) + (receiver.crossOriginIsolated == false) => Blocked
4. (sender.crossOriginIsolated == false) + (receiver.crossOriginIsolated == true ) => Transfered

This is not necessary bad, what matters is the receiver.
I wanted to make sure you are okay with case 4. WDYT?

I made some tests here:
https://chromium-review.googlesource.com/c/chromium/src/+/2514211
Unfortunately, not WPT test, because this rely on flags not enabled yet.

In the spec, I don't know the destination realms.
I guess I should update:

StructuredSerializeInternal ( value, forStorage [ , memory ] )

into

StructuredSerializeInternal ( value, forStorage [ , memory ] [, destinationRealm] )

and plumb |destinationRealm| everywhere this function is called.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up moving this part toward the deserialization, because targetRealm was already plumbed. I didn't wanted to plumb destinationRealm on the serialization side. Does this sound good to you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should split this part out into its own PR given it's quite separate from what this PR is doing. Having said that, I think we can also check this when serializing as there is a current Realm. The difference here is whether postMessage() throws or the target gets a messageerror event. I suspect we want postMessage() to throw, though I also don't care too strongly. I was hoping @domenic and @yutakahirano would chime in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, woah, let's not make changes to structured serialization in this PR, especially one that changes the signature. That's a big change that needs more discussion in a dedicated issue, not a comment thread on a PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it out.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated
<span>agent cluster</span> are automatically origin-isolated. The `<code
<p class="note">Similarly, <code>Document</code>s with <span>agent cluster</span>'s
<span>cross-origin isolated</span> <code data-x="bcg-isolation-physic">isolation-physic</code>
are automatically origin-isolated. The `<code
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all the not-"none" values offer the same features as Origin Isolation in terms of what is observable to web developers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are close. The observable differences I have in mind are:

  • difference when opening a new window from a sandboxed iframe. The opener can't influence the openee with sandbox-flags. I think this result in error page.
  • difference, because we might swap browsing context group during a navigation, depending on the various headers.

Outside of navigations

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@ArthurSonzogni ArthurSonzogni force-pushed the coop-coep-not-imply-cross-origin-isolated branch 2 times, most recently from 68d940c to 9c0351e Compare December 15, 2020 01:15
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In penance for the long delays in review, I'll help out by pushing some commits to fix the issues I mention in this review, plus some editorial touchups.

source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
ArthurSonzogni and others added 4 commits December 17, 2020 14:12
The [specification] currently requires [COOP] + [COEP] to give access to
crossOriginIsolated capabilities like SharedArrayBuffer.

Some platforms can't easily support multiple processes (like Android
Webview). Therefore, they can't really support crossOriginIsolated.
However the are no strong reasons for them not to enforce COEP (and
maybe COOP) when their associated headers are present.

It would be great enforcing COEP (and maybe COOP) on all platforms,
desptie the lack of crossOriginIsolated capabilities.

This patch makes the specification to allow (instead of requiring)
platform to set the crossOriginIsolated flag when both COOP and COEP are
used.

Setting crossOriginIsolated becomes platform dependent. In exchange, we
can enforce COEP (and COOP) in a non platform dependent way, without
conflicting with the specification about crossOriginIsolated.

[Bug]: whatwg#6060

[specification]:
https://html.spec.whatwg.org/#cross-origin-opener-policies

[COOP]:
https://html.spec.whatwg.org/#cross-origin-opener-policy

[COEP]:
https://html.spec.whatwg.org/#coep
@domenic domenic force-pushed the coop-coep-not-imply-cross-origin-isolated branch from 485b55e to f28344f Compare December 17, 2020 19:14
@domenic
Copy link
Member

domenic commented Dec 17, 2020

@annevk @yutakahirano @ArthurSonzogni PTAL. My changes are:

  • Editorial fixups (including things like "cross-origin-isolation" -> "cross-origin isolation mode")
  • Fixes to the issues @yutakahirano identified
  • Changed the structured serialization to check the capability of the current realm. (We can discuss whether it checks the destination realm in a separate issue; IMO the spec is correct that such checking is not necessary, but we can discuss.)
  • Since cross-origin isolated mode = logical/concrete => is origin-keyed = true (but not vice-versa), I updated two locations where it was more correct to talk about is origin-keyed. I.e. document.domain was checking both conditions, so I simplified it to only check is origin-keyed. And the non-normative note about window agent clusters was talking only about cross-origin isolated mode, but what it was saying was applicable to both, so I updated it to talk about origin-keying.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that we would split out the structured serialization bug fix entirely, but I guess it's okay to include it here if we spell it out in the commit message. I found a couple nits, seems okay to me to land this modulo those, but I'd appreciate one more review since it's a bit harder to review on a small screen.

@@ -80423,9 +80462,6 @@ interface <dfn>BarProp</dfn> {
a registrable domain suffix of and is not equal to</span> <var>effectiveDomain</var>, then throw
a <span>"<code>SecurityError</code>"</span> <code>DOMException</code>.</p></li>

<li><p>If the <span>surrounding agent</span>'s <span>agent cluster</span>'s <span>cross-origin
isolated</span> is true, then return.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this removed? Is it redundant with is origin-keyed now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. Another thing to spell out in the commit message.

<p class="note">It is difficult on some platforms to provide the security properties required by
the <span data-x="concept-settings-object-cross-origin-isolated-capability">cross-origin
isolated capability</span>. Only "<code
data-x="cross-origin-isolation-concrete">concrete</code>" might grant access to it. "<code
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/might/will/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using "might" was intentional. This also depends on the feature policy bit.

For instance: BCG cross-origin-isolation-concrete + document using Feature-Policy: cross-origin-isolated 'none' won't grant the window.crossOriginIsolated bit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my last patchset. I used can. It sounds less confusing than might, and more precise than will.

isolated capability</span>. Only "<code
data-x="cross-origin-isolation-concrete">concrete</code>" might grant access to it. "<code
data-x="cross-origin-isolation-logical">logical</code>" won't, and is used by implementations on
other platforms.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This other is weird, since we already started with some platforms that might use "logical".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I removed ,and is used by implementations on other platforms.

Copy link
Member Author

@ArthurSonzogni ArthurSonzogni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @domenic for the big update!

<p class="note">It is difficult on some platforms to provide the security properties required by
the <span data-x="concept-settings-object-cross-origin-isolated-capability">cross-origin
isolated capability</span>. Only "<code
data-x="cross-origin-isolation-concrete">concrete</code>" might grant access to it. "<code
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my last patchset. I used can. It sounds less confusing than might, and more precise than will.

isolated capability</span>. Only "<code
data-x="cross-origin-isolation-concrete">concrete</code>" might grant access to it. "<code
data-x="cross-origin-isolation-logical">logical</code>" won't, and is used by implementations on
other platforms.</p>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I removed ,and is used by implementations on other platforms.

@annevk
Copy link
Member

annevk commented Jan 4, 2021

@ArthurSonzogni could you prepare a tentative commit message that also accounts for the additional changes as discussed above and post it as a comment? I think this is good to go. (I pushed one more nit.)

@ArthurSonzogni
Copy link
Member Author

Thanks @annevk !

Tentative commit message:

The [specification] currently requires [COOP] + [COEP] to give access to
crossOriginIsolated capabilities like SharedArrayBuffer.

Some platforms can't easily support multiple processes (like Android
Webview). Therefore, they can't really support crossOriginIsolated.
However the are no strong reasons for them not to enforce COEP (and
maybe COOP) when their associated headers are present.

This patch makes the specification to allow (instead of requiring)
platform to grant the crossOriginIsolated capability when both COOP and
COEP are used.

The browsing context group's cross-origin-isolation mode becomes a
tri-state:
- none     (not COOP+COEP)
- logical  (COOP+COEP, without granting crossOriginIsolated)
- concrete (COOP+COEP, with granting crossOriginIsolated)

[Bug]: https://github.com/whatwg/html/issues/6060

[specification]:
https://html.spec.whatwg.org/#cross-origin-opener-policies

[COOP]:
https://html.spec.whatwg.org/#cross-origin-opener-policy

[COEP]:
https://html.spec.whatwg.org/#coep

---

Additional minor change:

When setting document.domain, both properties were previously checked:
1) agent-cluster's cross-origin-isolation mode.
2) agent-cluster's origin-keyed.
The first has been removed, since it already implies the second.

@annevk
Copy link
Member

annevk commented Jan 5, 2021

Thanks, we should also mention the change to StructuredSerializeInternal, but I can merge that in as well. If there are no further comments I'll merge this tomorrow.

@ArthurSonzogni
Copy link
Member Author

ArthurSonzogni commented Jan 5, 2021

Thanks, we should also mention the change to StructuredSerializeInternal, but I can merge that in as well. If there are no further comments I'll merge this tomorrow.

Did we change StructuredSerializeInternal? I see only some "data-x=" being now properly set. The content is the same.
We previously discussed updating StructuredSerializeInternal, but we said this was unrelated to this current patch, so the change was removed.

@annevk
Copy link
Member

annevk commented Jan 6, 2021

It went from checking cross-origin isolated to cross-origin isolated capability, right? That's a big change, even if one could consider it an oversight from when we introduced cross-origin isolated capability.

@ArthurSonzogni
Copy link
Member Author

Indeed! Updated tentative commit message. (Feel free to improve it, if needed)

Make COOP+COEP do not imply crossOriginIsolated.

This contains 3 changes:
---

The [specification] currently requires [COOP] + [COEP] to give access to
crossOriginIsolated capabilities like SharedArrayBuffer.

Some platforms can't easily support multiple processes (like Android
Webview). Therefore, they can't really support crossOriginIsolated.
However the are no strong reasons for them not to enforce COEP (and
maybe COOP) when their associated headers are present.

This patch makes the specification to allow (instead of requiring)
platform to grant the crossOriginIsolated capability when both COOP and
COEP are used.

The browsing context group's cross-origin-isolation mode becomes a
tri-state:
- none     (not COOP+COEP)
- logical  (COOP+COEP, without granting crossOriginIsolated)
- concrete (COOP+COEP, with granting crossOriginIsolated)

[Bug]:  https://github.com/whatwg/html/issues/6060
[COOP]: https://html.spec.whatwg.org/#cross-origin-opener-policy
[COEP]: https://html.spec.whatwg.org/#coep
[specification]:
https://html.spec.whatwg.org/#cross-origin-opener-policies

---

When setting document.domain, both properties were previously checked:
1. agent-cluster's cross-origin-isolation mode.
2. agent-cluster's origin-keyed.
The first has been removed, since it already implies the second.

---

When serializing a sharedArrayBuffer, check the cross-origin isolated
capability instead of the agent-cluster cross-origin isolation mode.
This was an oversight from when the cross-origin isolated capability was
first introduced.

@annevk annevk merged commit cbcf6ac into whatwg:master Jan 6, 2021
@annevk
Copy link
Member

annevk commented Jan 6, 2021

Thanks so much @ArthurSonzogni!

@ArthurSonzogni
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants