1301406 - The cookies of the top-level page are not keyed with firstPartyDomain when first party isolation is turned on in e10s mode.

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P3)

Description

[Tim Huang[:timhuang]]

In e10s mode, the cookies of the top-level page don't have the mFirstPartyDomain in originAttributes. You can test this by https://people.mozilla.org/~tihuang/cookies_iframe.html, this page will create an iframe with the same first party as the top-level page, and allow you to set and display cookies in this iframe. And it also allows you to show cookies in the top-level page.

Steps to produce bug:
1. Enable the first party isolation.
2. Open https://people.mozilla.org/~tihuang/cookies_iframe.html
3. Set some cookies in the iframe, and those cookies will be displayed.
4. Press 'Show' button in the top-level page, we expect to see the cookies that have been set in the iframe since top-level page and iframe are within the same first party domain. However, there will be nothing to show.

The reason of this bug is that the cookie services will use the SerializedLoadContext() [1] to carry the originAttributes from content process to chrome process in e10s mode. And originAttributes will come from the loadContext, which is the docShell of the top-level page. This docShell will not have mFirstPartyDomain in its originAttributes. That's why cookies will not be keyed with mFirstPartyDomain in e10s mode.

[1] http://searchfox.org/mozilla-central/source/netwerk/cookie/CookieServiceChild.cpp#128

Comment 1

[Jim Mathies [:jimm]]

Tim, is there a bug we can hook this up to related to first party isolation?

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

Hi smaug, Jason and Ehsan
Please see the commit message for the reason why I need to fix this.

Originally I tried to pass LoadInfo, but it looks Cookie part just needs the OA, so in the end I pass NeckoOriginAttributes.

r? smaug for he's the original reviewer for firstPartyIsolation.
r? jdeull as he's the owner of Necko
r? ehsan for the PrivateBrowsing part. (I assume I can get the PB value from origin attributes)

Thanks all.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

One thing came to my mind... What would happen if the getter for OA in docshell would actually set
firstPartyDomain of the returned value to the value of the current document's principal OA's firstPartyDomain. Could that simplify various things? (We shouldn't still inherit that value when doing new top level loads to docshell of course.)

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8789869 [details] [diff] [review]
Patch.

But we should probably do this anyhow.

Comment 6

[(no longer active)]

Comment on attachment 8789869 [details] [diff] [review]
Patch.

Review of attachment 8789869 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on the PB part.

Comment 7

[Jason Duell]

Comment on attachment 8789869 [details] [diff] [review]
Patch.

Review of attachment 8789869 [details] [diff] [review]:
-----------------------------------------------------------------

So the cookie API's get/set functions take an *optional* nsIChannel.  This patch changes how we get originAttributes from a method that AFAICT can't fail (CookieServiceParent::GetOriginAttributesFromParams calls NeckoParent::GetValidatedAppInfo(), which gets the info from GetManagedTabContext(), which should always work), to relying on the optional channel that's passed in to the cookie APIs. I'm wondering if there may be cases where the channel is null, and what would happen in that case.

It does look like we pass in a channel more often than we used to (like the dummy channel nsHTMLDocument now passes in if the channel is missing), so maybe we're fine.

Do we know if the channel will always be non-null in these call sites?

   // nsContentSink.cpp:  channel relies on mParser being set
   https://dxr.mozilla.org/mozilla-central/rev/8a494adbc5cced90a4edf0c98cffde906bf7f3ae/dom/base/nsContentSink.cpp#314

   // NPAPI: relies on GetChannelFromNPP() succeeding: unclear to me if it ever
   // fails in practice (and I assume we care only about Flash):
   https://dxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPlugin.cpp?q=nsNPAPIPlugin.cpp&redirect_type=direct#2536
   https://dxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPlugin.cpp?q=nsNPAPIPlugin.cpp&redirect_type=direct#2595

The serializedLoadContext has an 'isNotNull' field so we can react appropriately if there's no load context in the child call.  I'm wondering if we need something like that here. Maybe I'm totally wrong to be worried, and we've already guaranteed that all these calls sites pass in a channel?  If you think so and you've run this through try (and it work on a browser with Flash), then that's probably good enough.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jason Duell [:jduell] (needinfo me) from comment #7)
> So the cookie API's get/set functions take an *optional* nsIChannel.  This
> patch changes how we get originAttributes from a method that AFAICT can't
> fail (CookieServiceParent::GetOriginAttributesFromParams calls
> NeckoParent::GetValidatedAppInfo(), which gets the info from
> GetManagedTabContext(), which should always work), 
Hi Jason
Thanks for your comments, however I am not sure if I understand your concerns entirely.
I didn't change the 'channel' part, the channel is still there.
We still can get the OA from the channel, but the point now is to get OA from the loadInfo, instead of from LoadContext.

Because now when docshell loads a document, the OA of the document may have different OA than its docshell's OA. (from Bug 1260931)

And for IPC it passes 'SerializedLoadContext' which is from the docshel of the child.
That's the part I'd like to fix, because we could pass some information from the *loadInfo*.

> If you think so and you've run this through try (and it work on a browser
> with Flash), then that's probably good enough.
Yes, the try passes,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=57d2a504bd55

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9fc3453e993
replace SerializedLoadContext with NeckoOriginAttributes. r=smaug, ehsan, jduell

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9fc3453e993

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this, as bug 1289001 depends on it.

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8789869 [details] [diff] [review]
Patch.

Approval Request Comment
[Feature/regressing bug #]:
bug 1260931

[User impact if declined]:
If privacy.firstparty.isolate is enabled, cookie from iframes won't be isolated.

[Describe test coverage new/current, TreeHerder]:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9a7523cef05df17959bd3c9bea5ddf8c20abc03

[Risks and why]: 
Low, the pref privacy.firstpart.isolate is default off.

[String/UUID change made/needed]:
No

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8789869 [details] [diff] [review]
Patch.

Fix a cookie issue related to first party isolation plus this patch is needed for bug 1289001. Take it in 51 aurora.

Comment 17

[Carsten Book [:Tomcat]]

seems we need fix first the conflict in  bug 1289001

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/825c2ee219dc