Closed Bug 1562663 Opened 5 months ago Closed 2 months ago

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

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: annevk, Assigned: tt)

References

(Blocks 3 open bugs)

Details

Attachments

(7 files, 3 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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.

Blocks: 1562667

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?)
Flags: needinfo?(luke)
Blocks: 1563335

Sounds great!

Flags: needinfo?(luke)

Thank you, Tom.

Assignee: nobody → shes050117

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

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.)

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

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.)

Only for testing

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

Depends on D43237

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

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

Depends on D43239

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

(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.

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

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.

Attachment #9087718 - Attachment description: Bug 1562663 - P5 - Enable SAB for window.postMessage and worker.postMessage; → Bug 1562663 - P4 - Enable SAB for window.postMessage and worker.postMessage;
Attachment #9087717 - Attachment is obsolete: true
Blocks: 1579785
Attachment #9087718 - Attachment description: Bug 1562663 - P4 - Enable SAB for window.postMessage and worker.postMessage; → Bug 1562663 - P4b - Enable SAB for window.postMessage and worker.postMessage;

(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.

See Also: → 1575090, 1575092
Attachment #9087712 - Attachment is obsolete: true
Attachment #9087713 - Attachment is obsolete: true
Depends on: 1582200

(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.

See Also: 1575090, 1575092
Blocks: 1582200
No longer depends on: 1582200
Blocks: 1583251
Pushed by ttung@mozilla.com:
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
Depends on: 1584409
You need to log in before you can comment on or make changes to this bug.