1532287 - Make sure Cross-Origin policy is passed to workers and service workers and respected in SW fetch

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement, P2)

Description

[Valentin Gosu [:valentin] (he/him)]

Bug 1525036 adds the crossOriginPolicy to the BrowsingContext, but we also need to make sure it is passed when creating new workers/service-workers, and a SW fetch checks the policy of the service-worker.

See https://gist.github.com/annevk/17f580379c45802d5c3aef5a8fd53c7d

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug and there is no activity for 2 weeks.
:overholt, could you have a look please?

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Restating Overview:

• We want to make sure that opaque response data and meta-data never ends up inside a process where SharedArrayBuffer/anything that enables high-resolution-timers and thereby enable Spectre-style attacks can get at the data.
• We are not creating a separate storage partition, we are creating a notional "CORS clean room" which explicitly results in errors if any attempt is made to bring opaque (non-CORS) data into the clean room.
• Because the crossOriginPolicy of the top-level script Response of a ServiceWorker defines the global's crossOriginPolicy and we persist that response, a given WebIDL ServiceWorker instance should always have the same crossOriginPolicy.
• All the other checks can then be performed based on the value on the global/client.

At an implementation level (which potentially will need to be addressed in other bugs), our SW concerns are:

• The decision of where to spawn the ServiceWorker (in the parent-intercept case) doesn't actually have the top-level script Response available. It just has the registration info as stored in ServiceWorkerRegistrationData[1].
  • So we need to cache additional information on the registration and update ServiceWorkerRegistrar to provide it. This information characterizes only the installed worker, and currentWorkerHandlesFetch is already updated in lock-step. The cacheName is currently stored as a {}-enclosed UUID and could be hacked up to overload that info. More ideally, perhaps we could move to a rust-based ServiceWorkerRegistrar since we're dealing with an IPDL type that allows for drawing an implementation boundary.
• The WorkerPrivate::Constructor invocation needs the crossOriginPolicy at instantiation time which is also before the Response is available.
• The ScriptLoader will need to be updated to process the crossOriginPolicy and assert that it's consistent with the global and the current process (which can be a project:fission provided helper.)
• We need checks and assertions that ensure that we never send opaque data/metadata to clean-room processes in the Cache API, inclusive of fd's. It might make sense to make these changes directly in the Response implementation.
• FetchDriver also needs to be updated to apply the https://hg.mozilla.org/integration/autoland/rev/b77be7643141a8f3646257e599b38a3be862e6e0 logic so that it applies for workers. In that patch the (document, main-thread-specific) BrowsingContext is extracted from the document.
  • The FetchDriver gets the WorkerPrivate's nsIPrincipal, nsILoadGroup, CookieSettings (via its mLoadInfo.mCookieSettings), and ClientInfo. There's always the question of what's the practical and semantically ideal way to get this data across and unify it across documents/workers. Ideally that's the client. And right now the pseudo-spec text says "Let corsCredentials be request's client's global object's cross-origin policy" so arguably client is the right answer here.
  • So we might also want to refactor crossOriginPolicy off of the BrowsingContext and onto the client, as much of the point of the Clients API's implementation in Gecko internals was to unify the code.

1: https://searchfox.org/mozilla-central/source/dom/serviceworkers/ServiceWorkerRegistrarTypes.ipdlh

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

I'm notionally taking this, but if anyone wants to steal, feel free. I'll switch to ASSIGNED next week once work begins in earnest.

Comment 4

[Anne (:annevk)]

Maybe we should have a chat about what we need here for a minimum viable product and what we need long term. At a minimum I think we need these things:

1. Dedicated workers need to respect COEP of their creator. Dedicated workers have ways of fetching opaque responses and therefore need to block loading if COEP is not set for them. (They should not inherit COEP as that leads to inconsistent fetching state.)
2. We need to ensure that service workers end up in a separate process from the process that has the "allow postMessage() for SharedArrayBuffer" bit set (i.e., a document that opted in with COOP + COEP).
3. If a service worker feeds an opaque response to a process that has the bit set, ensure it has the relevant header and otherwise network error before the response reaches the process.

In the future we want:

1. Allowing shared/service workers to opt into COEP themselves so they can use SharedArrayBuffer in threaded fashion as well. (I'm not actually sure if we allow nested dedicated workers yet in these worker types. If that's not a thing that would also need to become a thing for this to be truly useful.)

Background reading on COOP and COEP: https://docs.google.com/document/d/1zDlfvfTJ_9e8Jdc8ehuV4zMEu9ySMCiTGMS9y0GU92k/edit.

Happy to chat more in person if that would help.

Comment 5

[Anne (:annevk)]

(To be clear, this still blocks resab, just no longer directly.)

Comment 6

[Eden Chuang[:edenchuang]]

Attachment: Bug 1532287 - P1 Saving the loading document/worker's COEP in InternalRequest.

Comment 7

[Eden Chuang[:edenchuang]]

Attachment: P2 Propagate loading document/worker's COEP to nsHttpChannel through nsILoadInfo

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9217f5aa798c
P1 Saving the loading document/worker's COEP in InternalRequest. r=dom-workers-and-storage-reviewers,perry
https://hg.mozilla.org/integration/autoland/rev/7c9708ccec41
P2 Propagate loading document/worker's  COEP to nsHttpChannel through nsILoadInfo r=necko-reviewers,valentin,JuniorHsu

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5571b75d82b6
Set wpt tests as passing. CLOSED TREE DONTBUILD

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/9217f5aa798c
https://hg.mozilla.org/mozilla-central/rev/7c9708ccec41
https://hg.mozilla.org/mozilla-central/rev/5571b75d82b6