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

Set correct embedder policy and cross-origin isolation mode #1545

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yutakahirano
Copy link
Contributor

@yutakahirano yutakahirano commented Oct 5, 2020

@yutakahirano
Copy link
Contributor Author

This is ready for review.

cc: @domenic

@domenic
Copy link
Contributor

domenic commented Oct 5, 2020

Interesting. /cc @annevk. I would have expected this to be done more similar to how shared workers are done.

In particular, is it OK to leave the service worker agent cluster's "cross-origin isolated" as false? That will impact its ability to postMessage SharedArrayBuffers to itself, for example, as well as whether the SharedArrayBuffer global exists. (Or maybe those are broken and should use the capability??)

@annevk
Copy link
Member

annevk commented Oct 6, 2020

When I reread whatwg/html#5435 (comment) and whatwg/html#5435 (comment) I wonder if I misread the latter. In particular 5 there does not depend on both whereas I suggest above that it would. Given how we defined [CrossOriginIsolated] in IDL it would make sense to me to align SharedArrayBuffer with that. What do you think @yutakahirano?

Now then there is the question of shared/service workers. I think they should have a shared model. And I suppose the main question there is whether user agents can guarantee to run those in their own process, even on Android. That is, can a COEP shared/service worker always get a process separate from the document it is associated with (which could be a third-party document that does not have the capability)? If we can make that guarantee it seems okay for them to always have the capability. I'd rather not end up in a situation where they sometimes have the capability though, depending on system resources.

Does that help and make sense?

@yutakahirano
Copy link
Contributor Author

@domenic Oh sorry I missed that. I think it's good to add a boolean argument to https://html.spec.whatwg.org/multipage/webappapis.html#obtain-a-service-worker-agent, and give the appropriate value in https://w3c.github.io/ServiceWorker/#run-service-worker-algorithm. What do you think?

@annevk

I suppose the main question there is whether user agents can guarantee to run those in their own process, even on Android. That is, can a COEP shared/service worker always get a process separate from the document it is associated with (which could be a third-party document that does not have the capability)?

I'm not sure if I understand. Chromium implementation is going to allocate a process for (origin or site, agent cluster's cross-origin isolated). We don't take the capability into account.

@annevk
Copy link
Member

annevk commented Oct 6, 2020

@yutakahirano so the scenario is that A embeds B and B has a shared/service worker Bsw. All have the appropriate COOP+COEP headers. But A doesn't delegate the capability to B. Now in resource-constrained environments the model allows for A and B to be in the same process and I think the idea with the capability is (please correct me if I'm wrong) that B not having access to certain features means it's harder to attack A.

Now, if Bsw were to share that process, it could attack A.

@yutakahirano
Copy link
Contributor Author

I think I brought up the point at whatwg/html#5752 (comment). At that time the conclusion was don't care.

@annevk
Copy link
Member

annevk commented Oct 6, 2020

Okay, I do think we should care about it as otherwise the cross-origin isolated capability is security theater and we would be better of not having the additional complexity. Perhaps I'm missing some subtlety in the motivation behind it?

@yutakahirano
Copy link
Contributor Author

Is this something we want to talk about at SW WG in TPAC?
@mikewest @camillelamy are you interested in this topic?

@domenic
Copy link
Contributor

domenic commented Oct 6, 2020

I think it's good to add a boolean argument to https://html.spec.whatwg.org/multipage/webappapis.html#obtain-a-service-worker-agent, and give the appropriate value in https://w3c.github.io/ServiceWorker/#run-service-worker-algorithm. What do you think?

That seems right to me.

Regarding the capability issue, the attack in #1545 (comment) is an interesting point. I hadn't fully considered the process model implications here. /cc @arturjanc

@yutakahirano
Copy link
Contributor Author

After seeing whatwg/html#6060, I'm starting thinking it makes sense to introduce a platform dependency to cross-origin-isolated capability. It represents whether web developers can use SAB and co. On some platforms COOP+COEP is sufficient to ensure the security guarantees but on other platforms it's more difficult.

The cross-origin isolated capability on shared and service workers is false, when the user agent may put a cross-origin environment into the process.

@annevk
Copy link
Member

annevk commented Oct 16, 2020

It's a bit mucky though as there will also be platforms where it doesn't matter if A delegates to B or not as they never share a process. But if it didn't delegate B would not and Bsw would have it. Maybe that's okay, but it doesn't seem satisfying nor very clear to web developers.

@mfalken
Copy link
Member

mfalken commented Jan 22, 2021

(belated) Notes from Service Worker WG meeting on Nov 20:

  • The majority of time was spent trying to understand the issue.
  • A clear description of the problem/attack would help the WG understand the issue.

yutakahirano pushed a commit to yutakahirano/html that referenced this pull request Jan 25, 2021
@yutakahirano
Copy link
Contributor Author

Introducing implementation-dependency has already been done in whatwg/html#6098 (Search for "The one chosen is implementation-defined" in https://html.spec.whatwg.org/multipage/workers.html#run-a-worker).

Updated the change.

docs/index.bs Outdated
@@ -2875,6 +2875,8 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Assert: |script| is not null.
1. Let |startFailed| be false.
1. Let |agent| be the result of [=obtain a service worker agent|obtaining a service worker agent=], and run the following steps in that context:
1. Let |agentCluster| be the [=agent cluster=] which contains |agent|.
1. If |workerGlobalScope|'s [=WorkerGlobalScope/embedder policy=] is "<code>[=embedder policy value/require-corp=]</code>", then set |agentCluster|'s [=agent-cluster-cross-origin-isolation|cross-origin isolation mode=] to "<code>[=cross-origin isolation mode/logical=]</code>" or "<code>[=cross-origin isolation mode/concrete=]</code>". The one chosen is implementation-defined.
Copy link
Member

Choose a reason for hiding this comment

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

It's a small thing, but since you have to change HTML anyway I would slightly prefer this being an operation you call in HTML and that operation takes care of the implementation-defined setting of either value.

That way if we ever have to change this we don't have to touch service workers.

Base automatically changed from master to main February 4, 2021 19:56
@yutakahirano yutakahirano deleted the yhirano/cross-origin-isolated branch February 8, 2021 11:39
yutakahirano pushed a commit to yutakahirano/html that referenced this pull request Mar 1, 2021
@yutakahirano yutakahirano restored the yhirano/cross-origin-isolated branch September 1, 2021 09:37
@yutakahirano yutakahirano reopened this Sep 1, 2021
@yutakahirano
Copy link
Contributor Author

Reopening as suggested at whatwg/html#6325 (comment).

 - Set the correct embedder policy to the service worker and its global.
 - Set the correct cross-origin isolation mode to the agent cluster.
@yutakahirano yutakahirano changed the title Define settings object's cross-origin isolated capability for SW Set correct embedder policy and cross-origin isolation mode Sep 1, 2021
@yutakahirano
Copy link
Contributor Author

yutakahirano commented Sep 1, 2021

cc: @ArthurSonzogni @nhiroki

I updated the PR. How does this look?

@@ -2901,7 +2906,7 @@ spec: rfc7231; urlPrefix: https://tools.ietf.org/html/rfc7231
1. Let |script| be |serviceWorker|'s [=service worker/script resource=].
1. Assert: |script| is not null.
1. Let |startFailed| be false.
1. Let |agent| be the result of [=obtain a service worker agent|obtaining a service worker agent=], and run the following steps in that context:
1. Let |agent| be the result of [=obtain a service worker agent|obtaining a service worker agent=] with |serviceWorker|'s [=service worker/embedder policy=], and run the following steps in that context:
Copy link
Contributor Author

@yutakahirano yutakahirano Sep 1, 2021

Choose a reason for hiding this comment

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

(Let's assume the procedure sets agent's agent cluster's cross-origin isolation mode. I'll fix the html side.)

@yutakahirano
Copy link
Contributor Author

@annevk would you take a look again?

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.

Thanks for working on this @yutakahirano! I think this looks reasonable, modulo the comparison operation. It would be nice if we could share more logic with workers, but I suppose that requires a bigger rewrite on the HTML side.

1. If |response|'s [=response/cache state=] is not "`local`", set |registration|'s [=last update check time=] to the current time.
1. Set |hasUpdatedResources| to true if any of the following are true:
* |newestWorker| is null.
* |newestWorker|'s [=service worker/script url=] is not |url| or |newestWorker|'s [=service worker/type=] is not |job|'s [=worker type=].
* |newestWorker|'s [=script resource map=][|url|]'s [=response/body=] is not byte-for-byte identical with |response|'s [=response/body=].
* |newestWorker|'s [=service worker/embedder policy=] does not equal to |embedderPolicy|.
Copy link
Member

Choose a reason for hiding this comment

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

Since an embedder policy is essentially a struct these days, I think we need to define the comparison operation. Or do all values need to be equal here, including the reporting values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a strong opinion. I think it's reasonable to update the script if coep's value changes, but I'm not so sure about other properties (reporting value, endpoint, reporting endpoint).

@ArthurSonzogni @nhiroki @jakearchibald do you have any opinions?

Copy link
Member

Choose a reason for hiding this comment

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

(Not a SW specialist)
I guess this govern whether or a not the new script should be used to replace the old SW at some point. As a developer, I think this is desirable to make it happen when any of those 4 COEP attributes are changed.
Do you foresee any strong drawback to this?

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

Successfully merging this pull request may close these issues.

5 participants