We should also enforce MessageEvent.origin in other messaging APIs (as listed in https://developer.mozilla.org/en-US/docs/Web/API/MessageEvent):
- window.postMessage (what I originally intended to cover with this bug)
- MessagePort.postMessage
- Broadcastchannel.postMessage
etc.

WIP CLs:
- https://chromium-review.googlesource.com/c/chromium/src/+/1399287
- https://chromium-review.googlesource.com/c/chromium/src/+/1399507

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3ae9799de9c6e85984b1b1c4618101bf2702d99e

commit 3ae9799de9c6e85984b1b1c4618101bf2702d99e
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Fri Jan 11 01:01:32 2019

Verify |source_origin| received by RFPH::OnRouteMessageEvent.

If a compromised renderer sends a |source_origin| that is incompatible
with the process lock, then such IPC should be ignored and the renderer
should be terminated.

Bug:  915721 
Change-Id: I293aaf7f0c3d981158cf0814b08e521f1a94add0
Reviewed-on: https://chromium-review.googlesource.com/c/1399287
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621839}
[modify] https://crrev.com/3ae9799de9c6e85984b1b1c4618101bf2702d99e/content/browser/bad_message.h
[modify] https://crrev.com/3ae9799de9c6e85984b1b1c4618101bf2702d99e/content/browser/frame_host/render_frame_proxy_host.cc
[modify] https://crrev.com/3ae9799de9c6e85984b1b1c4618101bf2702d99e/content/browser/security_exploit_browsertest.cc
[modify] https://crrev.com/3ae9799de9c6e85984b1b1c4618101bf2702d99e/tools/metrics/histograms/enums.xml

+dcheng@ in case he has any insights or suggestions on how to debug the mojo problem

For BroadcastChannel I have a WIP CL @ https://crrev.com/c/1407634.

Unfortunately, it doesn't work (mojo message from the renderer never makes it through to the browser after my changes - i.e. BroadcastChannelProvider::Connection::OnMessage never gets called).  :-(

Also - I have low confidence that my changes in the mojo manifest are correct (they do prevent a NOTREACHED from being reached though, so maybe they are okay).

Some drive-by comments:

MessagePort.postMessage never goes through the browser process so there is nothing you can enforce. But then it doesn't set MessageEvent.orgin either today afaik (although there have been times where we have wanted to do so if I remember correctly).

Re BroadcastChannel(Provider), the reason nothing is getting through in your CL is because you throw away the BroadcastChannelProviderPtr right after creating it, immediately severing all the connections on that pipe. Unfortunately what you're doing can't work for correctness either: it is important for proper ordering etc that all BroadcastChannelClient pairs from any particular renderer thread have correct relative ordering, and thus are all associated with one BroadcastChannelProviderPtr. Hence the ThreadLocal BroadcastChannelProvider rather than having separate per ExecutionContext ones like you're trying to do. We could probably get away with having one per origin per thread rather than one per thread like today (although ideally it would be one per unit of related similar origin browsing context, or whatever the html spec language is), but that's still different from what you're trying to do. The per-execution-context interfaces thing we have in chrome unfortunately doesn't map very well to the way web standards and relative ordering of operations are specified.

Thanks mek@!


RE: nothing is getting through in your CL is because you throw away the BroadcastChannelProviderPtr right after creating it

Doh... you're right!  This is not the first time I make that mistake... :-(  I guess I could fix this by storing BroadcastChannelProviderPtr in ExecutionContext (see the latest patchset in https://crrev.com/c/1407634).


RE: correct relative ordering (between pairs of BroadcastChannelProvider *and* BroadcastChannelProvider and legacy IPC or other frame stuff)

Good point, I should have considered this before...

In theory, making BroadcastChannelProvider an *associated* interface would solve the ordering problem, right?  OTOH, I am not quite sure how to achieve that:
1) RenderFrameImpl has a GetRemoteAssociatedInterfaces method, but it seems that this is only exposed at the //content layer (and not exposed to the //blink layer).
2) I would have to look closer at how one is supposed to register associated, per-frame^H^HexecutionContext interfaces on the browser side.  I guess I need to read more about WebContentsFrameBindingSet<T>...


WDYT?  Should I try to push forward with associated interfaces?  Or should I give up, keep sending an origin over IPC and verify the origin via CanAccessDataFromOrigin (I think that having test coverage for this verification would require a one-off instrumentation to the product code to inject a test implementation of BroadcastChannelProvier - this seems yucky and I'd rather not do it...).

1) this is exposed to blink via WebLocalFrameClient::GetRemoteNavigationAssociatedInterfaces() afaict?

I guess using channel associated interface bindings would work, yes. It would impose more of a strict ordering than necessary. Also https://chromium.googlesource.com/chromium/src/+/HEAD/ipc/README.md#using-channel_associated-interfaces says "NOTE: Channel-associated interfaces are an interim solution to make the transition to Mojo IPC easier in Chrome. You should not design new features which rely on this system. ", so that really doesn't seem like the correct solution...

So not sure what the overarching plan here is. Per execution context interfaces are often fundamentally incompatible with the way most specs are currently written, channel-associated interfaces are deprecated/discouraged, and we don't have anything else that lets you avoid passing the origin from the renderer to the browser afaik.

RE: this [associated interfaces] is exposed to blink via WebLocalFrameClient::GetRemoteNavigationAssociatedInterfaces() afaict

Right, but BroadcastChannel can also be used from workers and I think associated interfaces are not (yet?) plumbed there... :-/

Ah yes, good point.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4cc4be8415a9211fe9b92a8a2bf0b777672e3186

commit 4cc4be8415a9211fe9b92a8a2bf0b777672e3186
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Jan 16 23:23:20 2019

Verify |origin| received by BroadcastChannelProvider::ConnectToChannel.

The |origin| is sent from a renderer process and is therefore
untrustworthy and should be verified via
ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin.

Bug:  915721 
Change-Id: I42eae533edb47c4e100de5dfdda5f664457e0c3a
Reviewed-on: https://chromium-review.googlesource.com/c/1410264
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623443}
[modify] https://crrev.com/4cc4be8415a9211fe9b92a8a2bf0b777672e3186/content/browser/broadcast_channel/broadcast_channel_provider.cc
[modify] https://crrev.com/4cc4be8415a9211fe9b92a8a2bf0b777672e3186/content/browser/broadcast_channel/broadcast_channel_provider.h
[modify] https://crrev.com/4cc4be8415a9211fe9b92a8a2bf0b777672e3186/content/browser/isolated_origin_browsertest.cc
[modify] https://crrev.com/4cc4be8415a9211fe9b92a8a2bf0b777672e3186/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/4cc4be8415a9211fe9b92a8a2bf0b777672e3186/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/4cc4be8415a9211fe9b92a8a2bf0b777672e3186/content/test/BUILD.gn

I think there is nothing more to be done here - marking as fixed.