Closed Bug 1276328 Opened 7 years ago Closed 7 years ago

See if BroadcastChannel can use the new OriginAttributes private browsing flag


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




Tracking Status
firefox50 --- fixed


(Reporter: ehsan.akhgari, Assigned: jandreou25)



(Whiteboard: btpp-active)


(1 file)

We should be able to get rid of most of the code that bug 1148032 added and use the OriginAttributes flag instead.
Indeed! We can simplify the code a lot when we have privateBrowsing flag in OriginAttributes.
Whiteboard: btpp-active
happy to review this!
Attached patch Bug1276328.patchSplinter Review
Waited for Andrea's OriginChannelKey to land first.
Attachment #8760924 - Flags: review?(amarchesini)
Attachment #8760924 - Flags: review?(amarchesini) → review+
Keywords: checkin-needed
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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: 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.
Flags: needinfo?(tanvi)
Flags: needinfo?(ehsan)
Flags: needinfo?(amarchesini)
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.
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.
BTW, any fixes for this bug should be put in Bug 1316075
> 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:

Plus, we have a nice security check here:

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

where we do:

In this case origin is origin+OriginAttributes. If the child process tries to send a wrong IPC message, we kill it.
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(ehsan)
After talking this out, this isn't really a security bug.
Flags: needinfo?(tanvi)
Priority: -- → P1
Blocks: 1322760
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.