1276328 - See if BroadcastChannel can use the new OriginAttributes private browsing flag

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[(no longer active)]

We should be able to get rid of most of the code that bug 1148032 added and use the OriginAttributes flag instead.

Comment 1

[Andrea Marchesini [:baku]]

Indeed! We can simplify the code a lot when we have privateBrowsing flag in OriginAttributes.

Comment 2

[Andrea Marchesini [:baku]]

happy to review this!

Comment 3

[James Andreou]

Attachment: Bug1276328.patch

Waited for Andrea's OriginChannelKey to land first.

Comment 4

[James Andreou]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c3897ac58d039dd94d7b5f4e67066a48764ae5f

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52d483132bc1
Remove private browsing flags r=baku

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/52d483132bc1

Comment 7

[Dave Huseby [:huseby]]

I was doing a hand inspection of this patch to verify that we're not changing the behavior by eliminating the private browsing boolean.  Unfortunately, I think we may have broken something.  I'm going to need you to check me on this.  Here's the dots that I'm connecting:

1) In the BroadcastChannel and the InitializeRunnable classes, we removed the mPrivateBrowsing flag.  This in an of itself seems innocuous until you realize that it was being initialized on both the parent and client side of the IPC channel.

2) If you look at the patch, before this change, on the parent side (running on main thread), mPrivateBrowsing always gets overwritten by the value from the window doc, extracted using nsContentUtils::IsInPrivateBrowsing.  This is important because we have to enforce private browsing restrictions on the parent side, like all permissions.

3) Where it starts to come together is when you ask yourself, why did we overwrite the mPrivateBrowsing flag from the doc on the parent side?  It's because the cache key string generated in BackgroundParentImpl::AllocPBroadcastChannelParent was adding "|pb=true|" or "|pb=false|" depending on the value.  This is the way we isolate broadcast channels between documents from the same domain but in private browsing windows.  The user case is: the user has both a private browsing window open and a non-private browsing window open. In one window the user is logged into youtube with a work account.  In the other window the user is logged into youtube with a personal account.  If we didn't add the private browsing flag for isolation, then one window could open a broadcast channel to the other, thus allowing the private browsing window with personal account details to talk to the non PB window with work account details.  That's bad m'kay.

4) So now, here's where the problem comes in. The patch changes the cache key string generation to use the aOrigin passed into BackgroundParentImpl::AllocPBroacastChannelParent.  So far so good.  But that function is the handler for the constructor IPC message, so we need to look at where aOrigin comes from.  It comes from here: https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/ipc/ipdl/PBackgroundParent.cpp#1162 which is the string passed to the parent *from the child*.

5) So let's assume you have the same use case outlined in #3 and you can pwn the child window, all you need to do is send the broadcast channel constructor IPC message with the correctly formatted serialized origin attributes with the value of mPrivateBrowsingId you want and *boom* you've bypassed our private browsing isolation of broadcast channels.

Did I miss something?  We need to re-open and fix this.  The correct solution is to pull the origin attributes from the doc principal on the parent side only and use that when calculating the cache key string for the broadcast channel.

Comment 8

[Dave Huseby [:huseby]]

Worse yet, if you can pwn the child process, you could in theory construct a broadcast channel to any other window on *any other domain* even if first part isolation is enabled and all of our checks are turned on.

Comment 9

[Dave Huseby [:huseby]]

I wonder how many other places the child process is sending an origin+origin atributes serialized string to the parent and the parent is blindly trusting it?  I guess I'll find out soon :)  We need to always make sure to check if we're running on the parent side and then pulling the origin attributes from the parent side objects instead of using the origin passed in the IPC message.

Comment 10

[Dave Huseby [:huseby]]

BTW, any fixes for this bug should be put in Bug 1316075

Comment 11

[Andrea Marchesini [:baku]]

> 3) Where it starts to come together is when you ask yourself, why did we
> overwrite the mPrivateBrowsing flag from the doc on the parent side?  It's
> because the cache key string generated in

We didn't. mPrivateBrowsing flag from the doc was taken in the child side only (the doc doesn't exist on the parent side).

> details to talk to the non PB window with work account details.  That's bad
> m'kay.

Right. But this doesn't happen. We also have a test:
https://dxr.mozilla.org/mozilla-central/source/dom/broadcastchannel/tests/browser_private_browsing.js

Plus, we have a nice security check here:

https://dxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundParentImpl.cpp#629

When the BroadcastChannel actor is created, we dispatch a runnable to the main-thread:

https://dxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundParentImpl.cpp#646

where we do:

https://dxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundParentImpl.cpp#523

In this case origin is origin+OriginAttributes. If the child process tries to send a wrong IPC message, we kill it.

Comment 12

[(no longer active)]

I think Andrea summarized why the current code is correct well.

Also note that if a child process is pwned, it can bypass all sorts of PB checks that only happen in the child process side, similar to all sorts of other checks only happening to the child process.  We have never attempted to make PB be enforced correctly with a compromised content process.

Comment 13

[Dave Huseby [:huseby]]

After talking this out, this isn't really a security bug.