Bug 186593 - User gesture context is not passed via MessageChannel
Summary: User gesture context is not passed via MessageChannel
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords: InRadar
Depends on: 187001
Blocks:
  Show dependency treegraph
 
Reported: 2018-06-13 08:59 PDT by Dima Voytenko
Modified: 2019-05-16 10:45 PDT (History)
16 users (show)

See Also:


Attachments
Patch (8.74 KB, patch)
2018-06-15 03:14 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (12.26 KB, patch)
2018-06-18 02:08 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (more restricted version) (11.15 KB, patch)
2018-06-21 04:15 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (28.64 KB, patch)
2018-11-14 10:55 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (30.01 KB, patch)
2019-02-27 03:43 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (29.82 KB, patch)
2019-05-08 23:24 PDT, Frédéric Wang (:fredw)
fred.wang: review?
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews214 for win-future (13.64 MB, application/zip)
2019-05-09 01:28 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dima Voytenko 2018-06-13 08:59:43 PDT
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.
Comment 1 Frédéric Wang (:fredw) 2018-06-14 05:16:07 PDT
(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.
Comment 2 Dima Voytenko 2018-06-14 09:22:54 PDT
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.
Comment 3 Brady Eidson 2018-06-14 09:25:59 PDT
(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.
Comment 4 Dima Voytenko 2018-06-14 09:31:39 PDT
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.
Comment 5 Frédéric Wang (:fredw) 2018-06-15 03:14:14 PDT
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.
Comment 6 Frédéric Wang (:fredw) 2018-06-15 03:17:17 PDT
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!
Comment 7 Frédéric Wang (:fredw) 2018-06-18 02:08:43 PDT
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.
Comment 8 Dima Voytenko 2018-06-19 10:43:20 PDT
@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?
Comment 9 Brady Eidson 2018-06-19 12:51:58 PDT
(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?
Comment 10 Dima Voytenko 2018-06-20 10:03:08 PDT
(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?
Comment 11 Frédéric Wang (:fredw) 2018-06-21 04:15:28 PDT
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.
Comment 12 Chris Dumez 2018-06-21 07:45:33 PDT
Adding a few security people in cc.
Comment 13 Dima Voytenko 2018-06-21 10:42:21 PDT
(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.
Comment 14 Radar WebKit Bug Importer 2018-11-06 01:23:41 PST
<rdar://problem/45836796>
Comment 15 Frédéric Wang (:fredw) 2018-11-14 10:55:03 PST
Created attachment 354837 [details]
Patch
Comment 16 Frédéric Wang (:fredw) 2018-11-22 07:16:23 PST
@Brent: Can you please review this patch?
Comment 17 Frédéric Wang (:fredw) 2019-02-27 03:43:38 PST
Created attachment 363084 [details]
Patch

Rebasing...
Comment 18 Frédéric Wang (:fredw) 2019-02-27 07:15:31 PST
Review ping?
Comment 19 Frédéric Wang (:fredw) 2019-05-08 23:24:53 PDT
Created attachment 369470 [details]
Patch

Rebasing...
Comment 20 Build Bot 2019-05-09 01:28:08 PDT
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
Comment 21 Build Bot 2019-05-09 01:28:11 PDT
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
Comment 22 Dima Voytenko 2019-05-15 10:58:53 PDT
Hi! Is there any response here from security folk or otherwise?
Comment 23 Brent Fulgham 2019-05-15 12:09:10 PDT
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.
Comment 24 youenn fablet 2019-05-15 12:20:19 PDT
@fredw, is there a spec that describes the behavior of this patch?
Comment 25 Brent Fulgham 2019-05-15 12:20:40 PDT
We are reviewing this concept. We haven't decided if we agree this is a good approach yet.
Comment 26 Alex Christensen 2019-05-15 13:56:40 PDT
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.
Comment 27 Dima Voytenko 2019-05-15 14:32:42 PDT
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.
Comment 28 Alex Christensen 2019-05-16 10:43:44 PDT
(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.
Comment 29 Dima Voytenko 2019-05-16 10:45:38 PDT
Totally agree. Hence I'm looking if there's any feedback from the security folk on this use.