Opened 8 years ago

Closed 20 months ago

#3246 closed enhancement (fixed)

Isolate HTTP cookies according to first and third party domain contexts

Reported by: mikeperry Owned by: michael
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: backport-to-mozilla, tbb-linkability, tbb-usability-website, tbb-bounty, tbb-firefox-patch, ff52-esr-will-have
Cc: lunar@…, gk, StrangeCharm, michael, brade, mcs, hultmann Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by michael)

Right now, we've set Tor Browser to block third party cookies. This will probably break some sites. There is a less intrusive option described at https://wiki.mozilla.org/Thirdparty that we should use.

Rebase and test existing patches (originating from https://bugzilla.mozilla.org/show_bug.cgi?id=565965)

Revise requirements according to preliminary tests and devise a broad test plan.

Reimplement and retest to guarantee proper isolation without severely impeding cookie dependent applications.

Document the implementation and optionally a contrast of browser cookie handling.

Pave the way towards a improved privacy panel including a new cookie inspector and API supporting such UI.


Note: This is a metaticket composed of work items in child tickets.

Child Tickets

TicketTypeStatusOwnerSummary
#14057taskclosedmichaelImplement a test plan for double key cookie logic
#14058taskclosedmichaelReconstruction of obsolete Mozilla patch logic
#14059defectclosedmichaelRevision of existing double key cookie logic to meet requirements
#14061enhancementclosedtbb-teamDevelop a new cookie inspector accommodating double key logic
#14062enhancementclosedmichaelRevise nsCookie(Service?) APIs to accommodate double key logic
#14081taskclosedtbb-teamContrast our cookie strategy with other leading browsers
#14083enhancementclosedtbb-teamExtend cookies.sqlite database double key cookie logic
#14707taskclosedtbb-teamMaintain a checklist of symptoms caused by cookie transmission failure

Attachments (7)

DoubleKeyCookies.patch (28.9 KB) - added by mikeperry 6 years ago.
Local copy of patch from https://bugzilla.mozilla.org/show_bug.cgi?id=565965
NewCookieManager2.png (27.3 KB) - added by mikeperry 6 years ago.
Mockup UI window for managing double-keyed cookies and other site data
msvb3246-306bbfd_a0.patch​ (22.4 KB) - added by michael 4 years ago.
Prealpha rekeyed patch of previous attachments
msvb3246-306bbfd_a1.patch​ (22.5 KB) - added by michael 4 years ago.
Replace GetOriginatingURI with GetFirstPartyURI
msvb3246-d006262_a2.patch (19.0 KB) - added by michael 4 years ago.
Rebased patch of former DoubleKeyCookies.patch (instead of the more recent https://bug565965.bugzilla.mozilla.org/attachment.cgi?id=648681)
msvb-report1.txt (8.8 KB) - added by michael 4 years ago.
Midprogress report after testing concluded rebased patches fail.
msvb3246-283f7c6_a0.patch (18.9 KB) - added by michael 4 years ago.
Revised double key logic verified with session cookies (applies to 283f7c6)

Download all attachments as: .zip

Change History (49)

comment:1 Changed 8 years ago by mikeperry

Keywords: MikePerryIterationFires20110529 added

comment:2 Changed 8 years ago by mikeperry

Keywords: MikePerryIterationFires20110529 removed

Patch is for FF3.6. Quite a bit more work than just applying it...

comment:3 Changed 7 years ago by lunar

Cc: lunar@… added

comment:4 Changed 7 years ago by gk

Cc: g.koppen@… added

comment:5 Changed 7 years ago by mikeperry

Priority: normalmajor

comment:6 Changed 7 years ago by mikeperry

Cc: StrangeCharm added

comment:7 Changed 7 years ago by StrangeCharm

Keywords: backport-to-mozilla added

comment:8 Changed 7 years ago by mikeperry

Milestone: TorBrowserBundle 2.3.x-stable

comment:9 Changed 6 years ago by mikeperry

Keywords: tbb-linkability added
Milestone: TorBrowserBundle 2.3.x-stable
Priority: majornormal

Hard time saying if this is major or not. Disabling third party cookies entirely doesn't seem to break too much.. That, or people are silently re-enabling them? Lowering it, but tagging it with tbb-linkability to keep an eye on it either way.

Changed 6 years ago by mikeperry

Attachment: DoubleKeyCookies.patch added

comment:10 Changed 6 years ago by mikeperry

Priority: normalmajor
Type: defectenhancement

gk updated that patch. Also, reclassifying this as 'major enhancement'. Since we disable third party cookies, this is not a defect. But allowing third party customization to work through federated login might be nice.

comment:11 Changed 6 years ago by gk

Mike, I plan to push that patch a bit further investigating corner cases and thinking about ways to cope with them. I'll begin with examples given in https://wiki.mozilla.org/Thirdparty and in the bug ticket itself. If you have some weired use cases (i.e. websites) I should test, please post them here, thanks.

comment:12 Changed 6 years ago by mikeperry

Keywords: tbb-usability-website added

My first tests would be anything with Disqus and other third party commenting systems. Blogger issues have also occasionally complained about issues, too. Off the top of my head I can't remember any URLs though.

comment:13 Changed 6 years ago by mikeperry

Oh, also reCAPTCHA sites, apparently. See #3546.

comment:14 Changed 6 years ago by mikeperry

Keywords: tbb-bounty added

Changed 6 years ago by mikeperry

Attachment: NewCookieManager2.png added

Mockup UI window for managing double-keyed cookies and other site data

comment:15 Changed 6 years ago by mikeperry

Parent ID: #2871

comment:16 Changed 5 years ago by gk

Cc: gk added; g.koppen@… removed

comment:17 Changed 5 years ago by michael

Cc: michael added

Changed 4 years ago by michael

Prealpha rekeyed patch of previous attachments

comment:18 Changed 4 years ago by michael

There's a couple problems with the recent msvb3246-306bbfd_a0 (rebase of DoubleKeyCookies.patch​.) First, the previous attempts used a deprecated GetOriginatingURI from nsICookiePermission, which doesn't exist today [1]. The call was simply removed in the patch rebase, probably a bad thing. Second, it's clear that some major testing needs to prove this patch, as a number of comments (look for FIXME or 'not sure' expressions) suggest.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=769589

Changed 4 years ago by michael

Replace GetOriginatingURI with GetFirstPartyURI

comment:19 Changed 4 years ago by michael

Dan Witte writes in [1] that 'Logging in (huffingtonpost.com) with Connect works fine', but unfortunately the current state of tor-browser + msvb3246-306bbfd_a1.patch (the rebased patch of Georg Koppen's rebase of Dan Witte's patch) does not confirm this. After applying msvb3246-306bbfd_a1, building, running firefox(1), logging in to the Facebook, browsing to a huffingtonpost.com page and clicking the 'Comment' button of the 'Add a comment...' Facebook widget at the bottom, nothing happens (as if a third party cookie transmission were stopped.)

The cookie logic does seem to work on internal Facebook domain sites [2]. However, in this case the same results are obtained whether applying the patch or not. More bizarre, once the comment is accepted, viewing the page from another HTTP session fails to include the comment.

Nevertheless, Disqus sites work just fine with or without the rebased patch. We can assume that a 2012 Disqus API change caused this positive development without changes on our part.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=565965#c7
[2] https://developers.facebook.com/blog/post/2009/02/19/commenting-with-facebook-connect/

Last edited 4 years ago by michael (previous) (diff)

comment:20 in reply to:  description Changed 4 years ago by michael

Some peripheral but relevant information follows.


Replying to mikeperry:

Right now, we've set Tor Browser to block third party cookies.

By the way, what this technically means is that we've changed the Mozilla default preference:

  name: network.cookie.cookieBehavior
  value: 0 (or 3)

...to:

  name: network.cookie.cookieBehavior
  value: 1

As with all preferences, this is determined by parsing prefs.js (or manually given in the URL bar 'about:config'.) The tor-browser source of this default value change is located in browser/app/profile/000-tor-browser.js.


Cookie preferences are documented in Dan Witte's Mozilla developer doc,
...with mappings to source values in netwerk/cookie/CookieServiceChild.cpp:

static const int32_t BEHAVIOR_ACCEPT = 0;        // 0 = accept all cookies
static const int32_t BEHAVIOR_REJECTFOREIGN = 1; // 1 = only accept first party
static const int32_t BEHAVIOR_REJECT = 2;        // 2 = block all cookies
static const int32_t BEHAVIOR_LIMITFOREIGN = 3;  // 3 = use p3p settings
Last edited 4 years ago by michael (previous) (diff)

comment:21 Changed 4 years ago by michael

Keywords: TorBrowserTeam201407 added

Changed 4 years ago by michael

Attachment: msvb3246-d006262_a2.patch added

Rebased patch of former DoubleKeyCookies.patch (instead of the more recent https://bug565965.bugzilla.mozilla.org/attachment.cgi?id=648681)

comment:22 in reply to:  19 ; Changed 4 years ago by michael

Replying to michael:

After applying msvb3246-306bbfd_a1, building, running firefox(1), logging in to the Facebook, browsing to a huffingtonpost.com page and clicking the 'Comment' button of the 'Add a comment...' Facebook widget at the bottom, nothing happens (as if a third party cookie transmission were stopped.)

On application of the newer msvb3246-d006262_a2, cookie transmission starts working again but only when cookie policy is set to 'accept all cookies by default' which is not what we want.

OBJECTIVE

The desired outcome from patch application is to interpret double keyed cookies as first party when they refer to foreign hosts but originate from content associated with the domain of the 'URL bar.'

This allows us to forego changing cookie policy to 'accept all cookies by default' and instead keep it to 'only accept from the originating site (block third party cookies)' while transmitting double key matched cookies to foreign hosts.

Assuming a URL bar entry 'http://www.huffingtonpost.com/...' and attempt to add a comment at the bottom of the page after successfully logging in to the Facebook. Clicking 'Comment' sends a POST to the Facebook, and if our patchy browser interprets the cookie relation correctly the following headers are sent:

POST /ajax/connect/feedback.php HTTP/1.1
Host: www.facebook.com
User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20100101 Firefox/24.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Referer: https://www.facebook.com/plugins/comments.php?api_key=46744042133&channel_url=http%3A%2F%2Fstatic.ak.facebook.com%2Fconnect%2Fxd_arbiter%2FDhmkJ2TR0QN.js%3Fversion%3D41%23cb%3Dfc0b4e0b1f6ffa%26domain%3Dwww.huffingtonpost.com%26origin%3Dhttp%253A%252F%252Fwww.huffingtonpost.com%252Ff31aac803dd199c%26relation%3Dparent.parent&colorscheme=light&href=http%3A%2F%2Fwww.huffingtonpost.com%2Fjayson-demers%2Fhow-psychology-will-shape_b_5534545.html&locale=en_US&numposts=10&sdk=joey&skin=light&width=570
Content-Length: 863
Cookie: datr=S5qxU8zgo0o0j9GXcZHsMf0D; c_user=100004777967399; fr=0NtIaKuN7awUtojsX.AWWJtD9NlL3M3WWPxt_kxkoq9kc.BTsZpa.Em.AAA.AWVegird; xs=155%3And5eYC31G0PPqA%3A2%3A1404148314%3A3084; csm=2; s=Aa42d9MBjJhIEcDC.BTsZpa; lu=RgG3RP0d6b5MvtBc9MpH3Z8A

PROBLEM

Without correct patch logic, the same headers are sent except the cookie is considered third party for which transmission is blocked (as long as our default value of block third party cookies holds true.)

comment:23 in reply to:  22 ; Changed 4 years ago by gk

Replying to michael:

Replying to michael:

After applying msvb3246-306bbfd_a1, building, running firefox(1), logging in to the Facebook, browsing to a huffingtonpost.com page and clicking the 'Comment' button of the 'Add a comment...' Facebook widget at the bottom, nothing happens (as if a third party cookie transmission were stopped.)

On application of the newer msvb3246-d006262_a2, cookie transmission starts working again but only when cookie policy is set to 'accept all cookies by default' which is not what we want.

OBJECTIVE

The desired outcome from patch application is to interpret double keyed cookies as first party when they refer to foreign hosts but originate from content associated with the domain of the 'URL bar.'

This allows us to forego changing cookie policy to 'accept all cookies by default' and instead keep it to 'only accept from the originating site (block third party cookies)' while transmitting double key matched cookies to foreign hosts.

Well, we actually want accept cookies from third parties. The example in your last comment is a good one in this regard. The cookie from facebook.com is still a third party cookie even if we bind it to the URL bar. So, my initial feeling is that we should have the option "Allow all cookies" checked (we want to allow all of them but need to bind the third party ones to the URL bar domain (too)) as we want the ones from other domains, too. That said, the logic governing whatever option we choose should be, of course, the double-keying logic.

comment:24 in reply to:  23 ; Changed 4 years ago by michael

Replying to gk:

Replying to michael:

The desired outcome from patch application is to interpret double keyed cookies as first party when they refer to foreign hosts but originate from content associated with the domain of the 'URL bar.'

This allows us to forego changing cookie policy to 'accept all cookies by default' and instead keep it to 'only accept from the originating site (block third party cookies)' while transmitting double key matched cookies to foreign hosts.

The cookie from facebook.com is still a third party cookie even if we bind it to the URL bar. So, my initial feeling is that we should have the option "Allow all cookies" checked (we want to allow all of them but need to bind the third party ones to the URL bar domain (too)) as we want the ones from other domains, too. That said, the logic governing whatever option we choose should be, of course, the double-keying logic.

The outcome of our different approaches is equivalent. I like your idea best, to set "Allow all cookies" but still reject third party cookies not associated with the URL bar domain. By the way, looks like the (presently defective) code to test this is in netwerk/cookie/nsCookieService.cpp:nsCookieService::CheckPrefs().

comment:25 in reply to:  24 Changed 4 years ago by gk

Replying to michael:

The outcome of our different approaches is equivalent. I like your idea best, to set "Allow all cookies" but still reject third party cookies not associated with the URL bar domain.

There won't be such kind of third party cookies left if this bug is fixed. That's why I think we should select the "Accept cookies from sites".

Changed 4 years ago by michael

Attachment: msvb-report1.txt added

Midprogress report after testing concluded rebased patches fail.

comment:26 Changed 4 years ago by erinn

Keywords: tbb-firefox-patch added

comment:27 Changed 4 years ago by erinn

Component: Firefox Patch IssuesTor Browser
Owner: changed from mikeperry to tbb-team

comment:28 Changed 4 years ago by michael

In case this can of worms is not yet nasty enough, there's always the 2013 cookie mother patch [1].

[1] https://blog.mozilla.org/privacy/2013/02/25/firefox-getting-smarter-about-third-party-cookies/

comment:29 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201408 added; TorBrowserTeam201407 removed

comment:30 Changed 4 years ago by mikeperry

Owner: changed from tbb-team to michael
Status: newassigned

comment:31 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201410 added; TorBrowserTeam201408 removed

Firefox 31 will probably change more things here. Putting this out until we get some builds for it. Feel free to keep working on it if you like though.

comment:32 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201410 removed

comment:33 Changed 4 years ago by michael

It seems we should leave 'network.cookie.cookieBehavior' configuration logic unchanged and enforce double key equality according to 'privacy.thirdparty.isolate' instead. This was suggested in the weekly TBB development meeting on 22 December 2014.

Changed 4 years ago by michael

Attachment: msvb3246-283f7c6_a0.patch added

Revised double key logic verified with session cookies (applies to 283f7c6)

comment:34 Changed 4 years ago by michael

To answer recent comments from both Tor and Mozilla, the latest msvb3246-283f7c6_a0.patch correctly implements double key session cookies. Questions remain on a number of topics (like if persistent cookies and parameters are correctly handled) so this is considered a first installment.

comment:35 Changed 4 years ago by michael

This ticket has become such a ball of lint that I've subdivided topics into child tickets:

This should clarify the workflow. I'll modify the ticket's title and description to reflect the new substructure unless an observer or other interested party complains.

Last edited 4 years ago by michael (previous) (diff)

comment:36 Changed 4 years ago by michael

Keywords: TorBrowserTeam201503 added

comment:37 Changed 4 years ago by mcs

Cc: brade mcs added

comment:38 Changed 4 years ago by hultmann

Cc: hultmann added

comment:39 Changed 4 years ago by michael

Description: modified (diff)
Summary: Apply third party cookie patchIsolate HTTP cookies according to first and third party domain contexts

comment:40 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201503 removed

comment:41 Changed 2 years ago by gk

Keywords: ff52-esr-will-have added
Severity: Normal

comment:42 Changed 20 months ago by gk

Resolution: fixed
Status: assignedclosed

This will ship in 7.0a3 when we switch to ESR52.

Note: See TracTickets for help on using tickets.