1562663 - Ensure postMessage() blocks SharedArrayBuffer unless COOP and COEP are set

Status: RESOLVED FIXED

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

Description

[Anne (:annevk)]

Unless a document sets

• Cross-Origin-Opener-Policy: same-origin
• Cross-Origin-Embedder-Policy: require-corp

postMessage() should not allow SharedArrayBuffer to be posted.

As discussed at https://github.com/whatwg/html/issues/4732. Proposed change to the standard at https://github.com/whatwg/html/pull/4734.

Comment 1

[Anne (:annevk)]

Suggested roadmap:

• We create a boolean on the Gecko abstraction roughly matching "agent cluster" that's always false.
• We make postMessage() et al check this flag when a SharedArrayBuffer is passed and throw if it's false.
• We ship that.
• At this point we can experiment with shipping SharedArrayBuffer by default (see bug 1562667).
• We make the combination of the two headers (with those specific values) mentioned in OP set this flag to true for all affected agent clusters. (We should not ship this until we are sure there's no bad way left to use SharedArrayBuffer and SharedArrayBuffer parallel execution can be changed to sequental execution via a so-called "oh crap" boolean. Not sure if we have a tracking issue for that yet. Luke?)

Comment 2

[Luke Wagner [:luke]]

Sounds great!

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thank you, Tom.

Comment 4

[Anne (:annevk)]

https://github.com/web-platform-tests/wpt/issues/17690 has pointers to PRs for tests for this. Review appreciated. (Ideally multiple browsers review them.)

Comment 5

[Anne (:annevk)]

Clarification to comment 1. We don't have "agent cluster" as a concept (see bug 1567483). However, given COOP/COEP the boolean can be on the tab group as all theoretical agent clusters within that tab group will have the same value by design. (We'll still need to enforce the theoretical agent cluster boundaries though, probably on the DOM side for now by serializing the site (or doing a same-site comparison on the origin) and the tab group identifier when serializing a SharedArrayBuffer object. That's covered by the aforementioned bug.)

Comment 6

[Tom Tung [:tt, :ttung]]

There are three ways to postMessage from my understanding:

1. {window/worker/client}.postMessage
2. BroadcastChannel.postMessage
3. MessageChannel.port.postMessage

I have finished mostly for case 1.

I will need to check more, but my understanding of the code:
It seems that we have denied structureClone'ing sharedArrayBuffer for the last two cases (2. 3.) by default [1-4]. The reason is probably that the way they handle the postMessage. When they receive a request for postMessage on a content process, they will dispatch a request to XXXService on the parent process and then dispatch to all the content processes which need to be notified.

I wonder the simplest way to make them be able to not denying the sharedArraryBuffer by default while doing structureClone is that change the architecture for both BroadcastChannel and MessageChannel. If we have, for instance, BroadcastChannelService on each content process to see if there a BroadcastChannel to be postMessage'ed before sending the request of postMessage to the BroadcastChannelService on the parent process, then I guess it's possible to fix the problem. (So that, we can check if the postMessage will need to post a message to different process; And, it's there is no need, we can check the COOP, COEP, agent cluster stuff to decide the policy)

[1] https://searchfox.org/mozilla-central/rev/e62c920f7f6463239c6634113f8a8351e263b936/dom/broadcastchannel/BroadcastChannel.cpp#348
[2] https://searchfox.org/mozilla-central/rev/e62c920f7f6463239c6634113f8a8351e263b936/dom/ipc/StructuredCloneData.cpp#118
[3] https://searchfox.org/mozilla-central/rev/e62c920f7f6463239c6634113f8a8351e263b936/dom/messagechannel/MessagePort.cpp#339
[4] https://searchfox.org/mozilla-central/rev/e0b0c38ee83f99d3cf868bad525ace4a395039f1/dom/ipc/StructuredCloneData.cpp#118

Comment 7

[Anne (:annevk)]

That's great Tom as we only need 1 for the first phase of this project. SharedArrayBuffer support for 2 and 3 is tracked by bug 1360434 and bug 1360190 and I would love to see them fixed, but they are lesser in priority. (Definitely worth thinking about potential designs now though and discuss with others.)

Comment 8

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1562663 - Base Patch: Enable the pref for SAB;

Only for testing

Comment 9

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1562663 - Base Patch: Set coep for worker;

Note that will be implemented in another bug; Probably by Perry;

Depends on D43237

Comment 10

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1562663 - P1 - Have an agent cluster id for each DocGroup;

The purpose of this patch is to know if two different global objects are in the
same agent cluster.

To achieve this, the plan to to add an id for each DocGroup in this patch. And,
have an id on each workers. The final goal is to pass these ids to ClientInfo
so that we can check if two different global are in the same agent cluster group
by checking the AgentClusterId on their ClientInfos.

Note that eventually, we should have a function to check if two globals are in
the same agent cluster by JS team. This is just a hack on the DOM side.

Depends on D43238

Comment 11

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1562663 - P2 - Pass the agent cluster id to the ClientInfo so that we can check whether two clients are in the same agent cluster;

The agent cluster id is set when the EnsureClientSource is executed;

Depends on D43239

Comment 12

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1562663 - P3 - Have agent cluster Ids for workers;

Dedicated workers should be in the same agent cluster with its parent/creator.
The other workers (ShareWorker/ServiceWorker/ChromeWorker) create another agent
cluster when they are created from the creator.

Depends on D43240

Comment 13

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1562663 - P4 - Set COOP & COEP on ClientInfo so that we can check the value on any thread;

Depends on D43241

Comment 14

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1562663 - P4b - Enable SAB for window.postMessage and worker.postMessage;

Depends on D43242

Comment 15

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt, :ttung] (ooo until Aug 23rd) from comment #14)

Created attachment 9087718 [details]
Bug 1562663 - P5 - Enable SAB for window.postMessage and worker.postMessage;

Depends on D43242

I will need another patch for disabling/enabling tests.

Comment 16

[Tom Tung [:tt, :ttung]]

I was assuming all the calls for workers would be run only after the WorkerPrivate::ExecutionReady() [1] is called. So, I tried to check the COEP header & agent cluster id for workers from ClientInfo while postMessage'ing. However, the assumption is wrong.

// windows
let worker = new Worker();
worker.postMessage(); // [a]

[a] (on the main thread) can be called before the ExecutionReady(on worker thread) is executed and that might indicate that the attributes on WorkerPrivate object are not up-to-date yet. Also, I'm not sure if it's guaranteed that the response header is ready yet at [a]. If it's not, then we cannot check the COEP at this moment. We might need to block the postMessage until the header is ready. I might need to figure another way out to solve this.

[1] https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/dom/workers/WorkerPrivate.cpp#3067

Comment 17

[Anne (:annevk)]

If the instantiating window has COOP+COEP then the dedicated worker will either have COEP or it'll fail to be created. And the success of the postMessage() call should not depend on that. It would behave exactly as if you did a postMessage() to a worker URL that resulted in a network error.

Comment 18

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1562663 - P4a - Deny sharing memery by default for DataClonePolicy;

Depends on D43241

Comment 19

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt, :ttung] from comment #9)

Created attachment 9087713 [details]
Bug 1562663 - Base Patch: Set coep for worker;

Note that will be implemented in another bug; Probably by Perry;

Depends on D43237

This patch is just a prototype of setting & storing COEP on workers so that the other patches can get the value. It might be implemented on bug 1575090 or 1575092. The reason why I didn't set the dependency is that I can ask review for that patch if somehow these two bugs haven't done when all the patches are r+ here.

Setting them as see also since they are somehow having connections with this issue.

Comment 20

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06979ef8d3b4cef8757b981d0d8ab81e7b8c7598

Comment 21

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt, :ttung] from comment #19)

(In reply to Tom Tung [:tt, :ttung] from comment #9)

Created attachment 9087713 [details]
Setting them as see also since they are somehow having connections with this issue.

I changed the way for checking, so I abandon that patch and thus remove the dependencies.

Comment 22

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1562663 - P5 - Fix try failures;

Comment 23

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1562663 - P6 - Fix try failure on test_postMessage_closed.html;

Depends on D46492

Comment 24

[Tom Tung [:tt, :ttung]]

try with P6: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3635315ae93e518d7b98e17f4261cf04b9bd3d02

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee767d039565
P1 - Have an agent cluster id for each DocGroup; r=nika
https://hg.mozilla.org/integration/autoland/rev/0c116198d2b6
P2 - Pass the agent cluster id to the ClientInfo so that we can check whether two clients are in the same agent cluster; r=nika
https://hg.mozilla.org/integration/autoland/rev/e2f8f0d45276
P3 - Have agent cluster Ids for workers; r=perry,nika
https://hg.mozilla.org/integration/autoland/rev/f6a917ec38b8
P4a - Deny sharing memery by default for DataClonePolicy; r=nika,lth
https://hg.mozilla.org/integration/autoland/rev/4e17386780fc
P4b - Enable SAB for window.postMessage and worker.postMessage; r=nika
https://hg.mozilla.org/integration/autoland/rev/b07096708603
P5 - Fix try failures; r=nika,lth
https://hg.mozilla.org/integration/autoland/rev/bdf65c5369ab
P6 - Fix try failure on test_postMessage_closed.html; r=nika

Comment 26

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/ee767d039565
https://hg.mozilla.org/mozilla-central/rev/0c116198d2b6
https://hg.mozilla.org/mozilla-central/rev/e2f8f0d45276
https://hg.mozilla.org/mozilla-central/rev/f6a917ec38b8
https://hg.mozilla.org/mozilla-central/rev/4e17386780fc
https://hg.mozilla.org/mozilla-central/rev/b07096708603
https://hg.mozilla.org/mozilla-central/rev/bdf65c5369ab