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

RESOLVED FIXED in Firefox 51

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: timhuang, Unassigned)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox51 fixed, firefox52 fixed)

Details

(Whiteboard: [tor][domsecurity-backlog1])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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

2 years ago
Tim, is there a bug we can hook this up to related to first party isolation?
tracking-e10s: --- → +
Flags: needinfo?(tihuang)
Blocks: 1299996
Flags: needinfo?(tihuang)
(Assignee)

Updated

2 years ago
Assignee: nobody → allstars.chh
(Assignee)

Updated

2 years ago
Blocks: 1301637
(Reporter)

Updated

2 years ago
Blocks: 1301530
Created attachment 8789869 [details] [diff] [review]
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.
Attachment #8789739 - Attachment is obsolete: true
Attachment #8789869 - Flags: review?(jduell.mcbugs)
Attachment #8789869 - Flags: review?(ehsan)
Attachment #8789869 - Flags: review?(bugs)

Comment 4

2 years ago
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

2 years ago
Comment on attachment 8789869 [details] [diff] [review]
Patch.

But we should probably do this anyhow.
Attachment #8789869 - Flags: review?(bugs) → review+

Comment 6

2 years ago
Comment on attachment 8789869 [details] [diff] [review]
Patch.

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

r+ on the PB part.
Attachment #8789869 - Flags: review?(ehsan) → review+
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.
(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
Flags: needinfo?(allstars.chh) → needinfo?(jduell.mcbugs)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED

Comment 9

2 years ago
Pushed by yhuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9fc3453e993
replace SerializedLoadContext with NeckoOriginAttributes. r=smaug, ehsan, jduell

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f9fc3453e993
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1301530
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1301768
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1301778
Please request Aurora approval on this, as bug 1289001 depends on it.
Blocks: 1289001
status-firefox51: --- → affected
Flags: needinfo?(allstars.chh)
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
Flags: needinfo?(allstars.chh)
Attachment #8789869 - Flags: approval-mozilla-aurora?
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.
Attachment #8789869 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
seems we need fix first the conflict in  bug 1289001

Comment 18

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/825c2ee219dc
status-firefox51: affected → fixed
You need to log in before you can comment on or make changes to this bug.