Bug 186593

Summary: User gesture context is not passed via MessageChannel
Product: WebKit Reporter: Dima Voytenko <dvoytenko>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: beidson, bfulgham, bugs-noreply, calvaris, cdumez, dbates, dino, esprehn+autocc, ews-bot+gtk-4, ews, fred.wang, kangil.han, wilander
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.chromium.org/p/chromium/issues/detail?id=851493
https://bugs.webkit.org/show_bug.cgi?id=147509
https://github.com/web-platform-tests/wpt/pull/11668
https://bugzilla.mozilla.org/show_bug.cgi?id=1469422
Bug Depends on: 187001    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch (more restricted version) none

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.