User gesture context is not passed via MessageChannel

• Component: Platform
• Status: NEW ---

[Dima Voytenko]

To repro:
1. Open http://output.jsbin.com/cidetu
2. Click on "Channel message" button.

Observe that the popup is blocked. Also observe that clicking on "Window message" opens popup as expected.

The user gesture context is propagated via a window "message" event, but is NOT propagated via a MessageChannel event. Not sure if this is a part of any spec, but it seems counter-intuitive since MessageChannel is essentially the same API. There might be questions of whether the user gesture context should be propagated to Web Workers, etc, but as far as cross-window propagation, I believe it should.

[Frédéric Wang (:fredw)]

(In reply to Dima Voytenko from comment #0)
> To repro:
> 1. Open http://output.jsbin.com/cidetu
> 2. Click on "Channel message" button.
> 
> Observe that the popup is blocked. Also observe that clicking on "Window
> message" opens popup as expected.
> 
> The user gesture context is propagated via a window "message" event, but is
> NOT propagated via a MessageChannel event. Not sure if this is a part of any
> spec, but it seems counter-intuitive since MessageChannel is essentially the
> same API. There might be questions of whether the user gesture context
> should be propagated to Web Workers, etc, but as far as cross-window
> propagation, I believe it should.

Hi Dima. I can confirm the bug on macOS and GTK. A quick debug in WebCore::DOMWindow::allowPopUp shows that UserGestureIndicator::processingUserGesture() returns false for "Channel Message" because UserGestureIndicator::currentToken() is null, so indeed it seems the user gesture context is not propagated somehow.

[Dima Voytenko]

If there are no other reasons, it'd be great if it did. IMHO MessageChannel, while essentially the same, is an improvement over plain window messaging: I see a lot fewer security bugs with code relying on it.

[Brady Eidson]

(In reply to Dima Voytenko from comment #2)
> If there are no other reasons, it'd be great if it did. IMHO MessageChannel,
> while essentially the same, is an improvement over plain window messaging: I
> see a lot fewer security bugs with code relying on it.

I'm not sure why you say there's fewer security bugs with message channels vs. postMessage

MessageChannels can cross browsing contexts in newly radical ways (e.g. web page -> service worker context), making the bug surface significantly larger.

The fallout from "blessing" them with the user gesture flag should be carefully considered.

[Dima Voytenko]

Port can be exchanged only once and post-handshake the continuous message is more secure: I'm seeing few bugs with constant origin/source/target verification.

[Frédéric Wang (:fredw)]

Created attachment 342800 [details]
Patch

Here is a quick proof-of-concept patch that just propagates the current user gesture via MessageWithMesagePorts. That seems to make Dima's test case work as expected.

(In reply to Brady Eidson from comment #3)
> MessageChannels can cross browsing contexts in newly radical ways (e.g. web
> page -> service worker context), making the bug surface significantly larger.
> 
> The fallout from "blessing" them with the user gesture flag should be
> carefully considered.

I agree, I think we would need feedback from security reviewers to be sure whether this change of behavior is acceptable.

It seems Chromium developers are considering allowing that (cf see also link) once they enable their new refactored code. Maybe they could give us their stance on this.

[Frédéric Wang (:fredw)]

Comment on attachment 342800 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342800&action=review

> Source/WebCore/dom/MessagePort.cpp:276
> +                UserGestureIndicator gestureIndicator(ProcessingUserGesture);

Oops, this is not what I wanted to do!

[Frédéric Wang (:fredw)]

Created attachment 342918 [details]
Patch

This new version now properly sets the current gesture in MessagePort::dispatchMessages and adds the missing IPC encoding/decoding of the gesture.

I've written a new test case, based on Dima's: http://output.jsbin.com/kiruqatiso
The difference is that the frame tries to automatically send the channel message 5 seconds after page load.

With this patch, WebKit allows to open the popup when you click "Channel Message" but not for the automatic channel message.

[Dima Voytenko]

@beidson, it looks like there's a patch. How would we go about "The fallout from "blessing" them with the user gesture flag should be carefully considered."? What are possible scenarios with user gesture propagation w.r.t. service workers or web workers? To note, this bit is not exposed to the APIs anywhere, but it seems that having an additional context that directly lead to this message would be an overall good thing?

[Brady Eidson]

(In reply to Dima Voytenko from comment #4)
> Port can be exchanged only once 

I'm unclear on what you mean here.

Can you rephrase?

[Dima Voytenko]

(In reply to Brady Eidson from comment #9)
> (In reply to Dima Voytenko from comment #4)
> > Port can be exchanged only once 
> 
> I'm unclear on what you mean here.
> 
> Can you rephrase?

You mention that there might be some concerns about merging the patch. I wanted to ask how you'd go about verifying/addressing these concerns?

[Frédéric Wang (:fredw)]

Created attachment 343234 [details]
Patch (more restricted version)

This is a new version that is much stricter about how user gesture context is passed by postMessage. Basically, now:

- DOMWindow::postMessage always passes the user gesture context (that's already the case now)
- MessagePort::postMessage passes the user gesture context as long as it is not used in a worker context (currently, it never passes the user gesture context hence Dima's use case fails).
- Worker::postMessage will never pass the user gesture context (that's already the case now).

I have not tested it extensively yet, but at least it makes Dima's use case http://output.jsbin.com/cidetu work.

@Brady Eidson: Do you think this would be an acceptable change from a security point of view?
@Dima: Do you think that would address your use cases?

If that sounds good, I'll try writing tests and prepare a patch for formal review.

Maybe relaxing the conditions for postMessage can be done in follow-up patches, if that's what we want.

[Chris Dumez]

Adding a few security people in cc.

[Dima Voytenko]

(In reply to Frédéric Wang (:fredw) from comment #11)
> Created attachment 343234 [details]
> Patch (more restricted version)
> 
> @Dima: Do you think that would address your use cases?

Thanks, Frédéric! It does solve the use case and makes window/channel messaging APIs more compatible. I'm definitely ok starting with a more strict version and expanding if other use cases emerge.

[Radar WebKit Bug Importer]

<rdar://problem/45836796>

[Frédéric Wang (:fredw)]

Created attachment 354837 [details]
Patch

[Frédéric Wang (:fredw)]

@Brent: Can you please review this patch?

[Frédéric Wang (:fredw)]

Created attachment 363084 [details]
Patch

Rebasing...

[Frédéric Wang (:fredw)]

Review ping?

[Frédéric Wang (:fredw)]

Created attachment 369470 [details]
Patch

Rebasing...

[EWS Watchlist]

Comment on attachment 369470 [details]
Patch

Attachment 369470 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12141288

New failing tests:
js/dom/custom-constructors.html

[EWS Watchlist]

Created attachment 369478 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Dima Voytenko]

Hi! Is there any response here from security folk or otherwise?

[Brent Fulgham]

Comment on attachment 369470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369470&action=review

> LayoutTests/ChangeLog:9
> +        tests message posting via Window, MessageChannel, BroadcastChannel although the latter is

We TEST message posting

> LayoutTests/ChangeLog:11
> +        from a subframe.

"We also verify that the message is coming from the same window or from a subframe." ?

> LayoutTests/ChangeLog:19
> +        * http/tests/messaging/postMessage-triggered-by-user-activation.html: Added.

Are these not in WPT because we use a different user interaction construct? Is there no way to just keep this in WPT to make future merge/updates easier?

> Source/WebCore/dom/MessagePort.cpp:159
> +    bool contextIsWorker = is<WorkerGlobalScope>(*m_scriptExecutionContext);

What about WorkletGlobalScope? Do we need to do something different for them?

> Source/WebCore/dom/UserGestureIndicator.h:66
> +

Please remove this extra whitespace.

[youenn fablet]

@fredw, is there a spec that describes the behavior of this patch?

[Brent Fulgham]

We are reviewing this concept. We haven't decided if we agree this is a good approach yet.

[Alex Christensen]

Comment on attachment 369470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369470&action=review

What is the motivation for this?  Firefox is even more restrictive than WebKit currently is, and Chrome is quite lenient in this case.  "Chrome does it so we should too" is not sufficient motivation to make this change.

> LayoutTests/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=186593#c3

#c3 should be removed.

> Source/WebCore/dom/UserGestureIndicator.h:155
> +template<typename> struct EnumTraits;
> +template<typename E, E...> struct EnumValues;
> +
> +enum class UserGestureType { EscapeKey, Other };

All this should be deleted.  You don't want to declare another enum class in namespace WTF, and Forward.h already has the forward declarations necessary.

> Source/WebCore/dom/UserGestureIndicator.h:170
> +        WebCore::UserGestureType::EscapeKey,
> +        WebCore::UserGestureType::Other

Since there are only two of these, you should instead use a bool as the storage type for the UserGestureType enum class and the traits are unneeded.

[Dima Voytenko]

I don't think this should be a decision based on how Firefox or Chrome do it. And I wouldn't qualify Chrome as more lenient in this case. But it does, imho, have a more semantically consistent interpretation: if an iframe is activated and a gesture can be passed to the parent window, it doesn't matter which API is used. From most of points of view, the difference between `window.postMessage` and `MessageChannel.postMessage` is minimal. 

This problem does not have to be solved by propagating the "user gesture" bit via postMessage - that's the implementation detail. Since this was the WebKit's approach, the idea was to extend it to message channel as well. But, as an example, Chrome does not use post messages for this.

[Alex Christensen]

(In reply to Dima Voytenko from comment #27)
> From most of points of view, the difference
> between `window.postMessage` and `MessageChannel.postMessage` is minimal. 
In order to allow a change like this, we must consider those "minimal" differences from all points of view.  Justifications like "most developers won't take advantage of a way to abuse something" are how we introduce bad security bugs and design issues into the web platform.

[Dima Voytenko]

Totally agree. Hence I'm looking if there's any feedback from the security folk on this use.

[Frédéric Wang (:fredw)]

Sorry for the delay replying to this. Some quick replies below about what I remember ; to be honest I haven't checked the details for a while so apologies if it's not accurate.

(In reply to Brent Fulgham from comment #23)
> Are these not in WPT because we use a different user interaction construct?
> Is there no way to just keep this in WPT to make future merge/updates easier?

I had opened a PR for this: https://github.com/web-platform-tests/wpt/pull/11668
However, an automated test in upstream WPT is not really useful, given that the WPT runner basically disables the tests. Hence the PR currently adds a manual test but it won't be imported into browsers anyway. Plus the spec is not clear. So I didn't push too hard on this until other browser vendors can comment.

(In reply to youenn fablet from comment #24)
> @fredw, is there a spec that describes the behavior of this patch?

Hi Youenn. I don't remember any explicit mention in the spec last time I checked, but I had relied on this definition of triggered by user activation: 

"The task in which the algorithm is running was queued by an algorithm that was triggered by user activation, and the chain of such algorithms started within a user-agent defined timeframe."
(https://html.spec.whatwg.org/multipage/interaction.html#triggered-by-user-activation )

I'll try to ask with someone who has better knowledge of the HTML5 spec, but I'm not sure why tasks queued by window.top.postMessage or channel.port1.postMessage should behave differently.

The popup blocking config and "triggered by user activation" are mentioned here: https://html.spec.whatwg.org/multipage/browsers.html#popup-blocker

(In reply to Alex Christensen from comment #26)
> What is the motivation for this?  Firefox is even more restrictive than
> WebKit currently is, and Chrome is quite lenient in this case.  "Chrome does
> it so we should too" is not sufficient motivation to make this change.

Talking only about WebKit, we have inconsistent behavior: User gesture is passed via window.top.postMessage but not via MessageChannel channel.port1.postMessage so the popup is only blocked in the latter case. Firefox and Chromium have their own interpretation of the spec but at least they handle these two cases consistently.

I was expecting to get Apple's feedback on this experimental patch before raising this to the WhatWG but as I understand correctly you don't have any concrete security concern besides "if there is no reason to relax security we shouldn't take any risk". So I guess we can move the discussion to the WhatWG now, so we can clarify the spec, write WPT tests etc

[Frédéric Wang (:fredw)]

cc'ing Maciej since he has been following https://github.com/whatwg/html/pull/3851

[Mustaq Ahmed]

A quick FYI from Chrome: we were able to close our MessageChannel issue and other similar issues [1] by switching to the new user activation model ("User Activation v2").  See the github link in the last post above.

[1] https://bugs.chromium.org/p/chromium/issues/list?q=blockedon%3A696617%20status%3Afixed%2Cverified&can=1