Opened 5 years ago

Closed 3 years ago

#14059 closed defect (wontfix)

Revision of existing double key cookie logic to meet requirements

Reported by: michael Owned by: michael
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords:
Cc: brade, mcs Actual Points:
Parent ID: #3246 Points:
Reviewer: Sponsor:

Description (last modified by michael)

Revise logic from #14058 to meet requirements implied in the #3246 mother bug and TBB online development meetings.

Complete implementation of what is termed double keying as both 1st party hostname and 3rd party hostname are stored and conditionally used when constructing the Cookie HTTP header.


Nonfunctional requirements

Adaption to common use cases

Common browsing use cases involving cookies must be supported while protecting against crossdomain tracking violations. This includes travel reservations, shopping carts, surveys, comment providers, approval and rating systems, journalist drop boxes, and (one-time|two-factor) authentication systems.

Allow granular cookie inspection

Fine grained cookie inspection must be enabled through new design of a user interface indexing either 1st or 3rd party URI contexts. This requirement does not specify the UI itself.

Application

Logic must be applicable to the current Tor Browser trunk.


Functional requirements

3rd party cookie storage

3rd party cookies are stored under the usual conditions, according to the Set-Cookie HTTP header (RFC 6265.) Their storage structure enables 1st party association as a new measure.

3rd party cookie retrieval

3rd party cookies are revealed according to host domain matching (RFC 6265) of 1st party URI contexts. This change mitigates the problem of identification across independent domains.

Legacy cookie behaviour

New 3rd party isolation must not depend on legacy cookie behaviour configuraion network.cookie.cookieBehavior.

Conditional operation

The configuration value privacy.thirdparty.isolate influences control of 3rd party cookie transmission. The private browsing channel attribute plays a binary role in enabling transmission but depends on privacy.thirdparty.isolate.

Logging facility

New logic must approximate the logging behaviour of legacy cookie logic.


Unclassified requirements

Complementary research

Mark Nottingham's IETF draft and Alex Fowler's corresponding announcement of Prefer:Safe include requirements relating to HTTP cookie control.

The Mozilla B2G privacy panel and announcement of the Mozilla Polaris project refer to 3rd party cookie control.

Child Tickets

Attachments (1)

msvb14058-283f7c6.patch (27.0 KB) - added by michael 5 years ago.
Corrected and improved according to comments from second stage of code review

Download all attachments as: .zip

Change History (22)

comment:1 Changed 5 years ago by michael

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

comment:2 Changed 5 years ago by michael

Keywords: TorBrowserTeam201501 added

comment:3 Changed 5 years ago by mcs

Cc: brade mcs added

comment:4 Changed 5 years ago by michael

Question: Do we want to limit requirements to session cookies?
Background: The TB ignores expiry (and other?) HTTP cookie parameters.
Answer: <yes/no?>

Question: Are Mozilla requirements applicable (for backporting to Firefox ESR?)
Answer: <yes/no?>

comment:5 Changed 5 years ago by michael

Revisit (in the long term) requirements analysis of Cookie Monster #4132 and similar addons to selectively broaden the scope of requirements. Being the can of worms that it is, this muthaticket could be missing (or unnecessarily rewriting) important implementation. Reuse code where appropriate.

comment:6 in reply to:  4 Changed 5 years ago by michael

Replying to myself:

Question: Do we want to limit requirements to session cookies?
Background: The TB ignores expiry (and other?) HTTP cookie parameters.

Errata: Actually, the TB is RFC 6265 compliant, but the Expires attribute is ignored unless network.cookie.lifetimePolicy is changed from its default value (2 == ignore persistence.)
Answer: Probably yes, leaving this corner case unattended could cause subtle problems in runtime or increase maintenance costs.

Question: Are Mozilla requirements applicable (for backporting to Firefox ESR?)
Answer: <yes/no?>

comment:7 Changed 5 years ago by michael

Status: assignedneeds_information

R&D is paused, and can procede as soon as questions are answered and consensus on requirements is reached.

comment:8 Changed 5 years ago by michael

Cookie UI Representation

Question: Do we want to implement a UI representation of cookies indicating party affinity?
Background: The existing FF and TB ignore party context completely.
Answer: <yes/no?>

Question: Do we want to index all cookies according to 1st or 3rd party or both?
Background: The existing FF and TB index cookies according to HTTP host only.
Answer: <yes/no?>

Question: Do we want to allow searching cookies according to 1st or 3rd party or both?
Background: The existing FF and TB don't allow searches according to party context.
Answer: <yes/no?>

Question: Do we want to simplify configuration of granular cookie control with a slider?
Background: The solution to #9387 is excellent, but only concerns JavaScript activation.
This would include non double key cookie configuration.
Answer: <yes/no?>

comment:9 in reply to:  7 Changed 5 years ago by gk

Replying to michael:

R&D is paused, and can procede as soon as questions are answered and consensus on requirements is reached.

No sure where to put my testing feedback. Given that the patch I tested is attached in this bug I put my comments here as well. I tested with the latest nightly + msvb14058-283f7c6.patch on top. In a clean en-US bundle I did

1) enable third party cookies in Mozilla's privacy settings (the patch does not contain a special pref I need to toggle as far as I can see)
2) install the Live HTTP Headers to log the traffic
3) restarted and opened the Live HTTP Headers console to log traffic
4) go to http://fundingpoint.net and saved all traffic logs
5) opened in a different tab https://people.torproject.org/~gk/misc/fundingpoint_iframe.html and saved all traffic.
6) searched for cookies in the logs.

I get the following in 4)

http://www.fundingpoint.net/

GET / HTTP/1.1
Host: www.fundingpoint.net
User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Firefox/31.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-us,en;q=0.5
Accept-Encoding: gzip, deflate
Connection: keep-alive

HTTP/1.1 200 OK
Date: Tue, 27 Jan 2015 11:47:45 GMT
Server: Apache/2.2.15 (CentOS)
X-Powered-By: PHP/5.4.23
Set-Cookie: PHPSESSID=sihiadjk37v8bmvboep6d0gj56; path=/
Set-Cookie: www_pyrocms=%2FjTZdv72Vxmghi%2F9HPFS1DfgA7%2Fysq5K%2BIfGLyW8TburMfS%2FMxGRVxUtGuwpBFilYQ5Yqj6bDRCj6XQV885b%2BkzcBmWsIqk%2FCyBrqARe2y4ytZ5UKGRdzPrZziPRjXEXZlEjzGA%2B%2FvVjljWB3x%2Ft9P76AxFt8Fm9fVmgbXlhO5b3gZgdGajvY59YyO%2FPr2d1dpARNwA5Xqly%2FEFaJk78mIHRiWIlGFmwtGMRc9eQDpvsW9WEmlwbGRwi9cHZV4o6X1PcHK4LIFJZ5IaFGShYacuwGC4Mxqc%2BH8AXBVl0gL47yeAx3E5bUGzjkohzwbJE48EsccGxVMQgPBbffxskc%2FeCNTHh0RmJnOoD%2FmivHKWJ08tU1HFQ1aqz%2FyskJARW; path=/; domain=www.fundingpoint.net
Expires: Thu, 19 Nov 1981 08:52:00 GMT

and I see these too (among others) in 5)

http://www.fundingpoint.net/

GET / HTTP/1.1
Host: www.fundingpoint.net
User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Firefox/31.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-us,en;q=0.5
Accept-Encoding: gzip, deflate
Cookie: PHPSESSID=sihiadjk37v8bmvboep6d0gj56; www_pyrocms=%2FjTZdv72Vxmghi%2F9HPFS1DfgA7%2Fysq5K%2BIfGLyW8TburMfS%2FMxGRVxUtGuwpBFilYQ5Yqj6bDRCj6XQV885b%2BkzcBmWsIqk%2FCyBrqARe2y4ytZ5UKGRdzPrZziPRjXEXZlEjzGA%2B%2FvVjljWB3x%2Ft9P76AxFt8Fm9fVmgbXlhO5b3gZgdGajvY59YyO%2FPr2d1dpARNwA5Xqly%2FEFaJk78mIHRiWIlGFmwtGMRc9eQDpvsW9WEmlwbGRwi9cHZV4o6X1PcHK4LIFJZ5IaFGShYacuwGC4Mxqc%2BH8AXBVl0gL47yeAx3E5bUGzjkohzwbJE48EsccGxVMQgPBbffxskc%2FeCNTHh0RmJnOoD%2FmivHKWJ08tU1HFQ1aqz%2FyskJARW; _ga=GA1.2.28869478.1422359271; GetResponseComWebform4642401=WebformCookie
Connection: keep-alive

But that is not expected to happen as the URL bar domain in 5) is different from the one in 4). It seems to me the patch is not working as expected or am I missing something here?

comment:10 Changed 5 years ago by gk

The same problem is visible when building without Gitian. I used the following steps:

-5) Check out tor-browser-31.4.0esr-4.5-1
-4) do a |patch -p1 < /path/to/msvb14058-283f7c6.patch|
-3) Build the Tor Browser
-2) Package Tor Browser (|make -C obj* package INNER_MAKE_PACKAGE=true|)
-1) Copy the contents over into a freshly unpacked Tor Browser: cp -a obj*/dist/firefox/* /path/to/torbrowser/tor-browser_*/Browser
0) Start Tor Browser as usual and follow the steps 1)-6) in comment 9

comment:11 Changed 5 years ago by michael

Probably your tests are failing due to lack of setting the preference:

privacy.thirdparty.isolate = 1

...unless we want to change this requirement? There's always the possibility of simply enabling due to binary PBM? But we decided in the last 2014 meeting that cookie behaviour should follow the pref in question.

Edit: The above is misleading, because the preference must be set (to == 1) as well as PBM being active. Otherwise cookie service ignores party affinity of the corresponding channel. Incidentally, the network.cookie.cookieBehavior preference must allow 3rd party cookies to pass.

The combination of these three characteristics is what triggers dual keying. If this is indeed what's causing a false test negative, then we should reconsider if this is intuitive for users or requires a new UI (maybe as part of #14061) or change in default preference.

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

comment:12 Changed 5 years ago by michael

Keywords: TorBrowserTeam201502R added; TorBrowserTeam201501 removed

comment:13 Changed 5 years ago by gk

Keywords: GeorgKoppen201502R added

comment:14 Changed 5 years ago by michael

Description: modified (diff)

comment:15 Changed 5 years ago by gk

Here comes the first part of the review:

1) nsCookie.*: looks good to me.
2) CookieServiceParent.cpp: "Method is called nowhere" is not correct (anymore). CookieService* are for e10s (see: https://bugzilla.mozilla.org/show_bug.cgi?id=537156) which is already on Mozilla's dev channels activated. Eventually we need to port double keying to it, too. But at least the comment should make that clear.
3) nsICookie2.idl: Why is aOrigin of type ACString and not AUTF8String (like host, rawHost etc.)?

comment:16 in reply to:  15 ; Changed 5 years ago by michael

Replying to gk:

2) CookieServiceParent.cpp: "Method is called nowhere" is not correct (anymore). CookieService* are for e10s (see: https://bugzilla.mozilla.org/show_bug.cgi?id=537156) which is already on Mozilla's dev channels activated. Eventually we need to port double keying to it, too. But at least the comment should make that clear.

Yes, and there are other e10s RecvGetCookieString related things. I'll simply improve the comment (stating e10s incompatibility) as I think your comment implied.

I'm not sure how stable the e10s architecture is to influence current Tor Browser party isolation features, do you know? My gut feeling is that finishing on the ESR arch early would boost later e10s completion.

3) nsICookie2.idl: Why is aOrigin of type ACString and not AUTF8String (like host, rawHost etc.)?

Most host string variables in implementation files are of type nsACString or nsCString which implies the IDL variables should use ACString. I assumed that's why you and Dan Witte made it ACString, but I agree that there should be some conformance.

Too bad there's so much AUTF8String already in nsCookie (frozen I think) so I'll change origin to be of type AUTF8String to conform.

comment:17 Changed 5 years ago by michael

Description: modified (diff)

comment:18 in reply to:  16 ; Changed 5 years ago by gk

Keywords: TorBrowserTeam201502R GeorgKoppen201502R removed
Status: needs_informationneeds_revision

Second part of the review:

1) Please document why you use one time mThirdPartyUtil->GetFirstPartyURIFromChannel and the other time mThirdPartyUtil->GetFirstPartyIsolationURI and what that implies.

2) You can't reuse requireHostMatch in SetCookieStringInternal as this would mean that the URL bar domain could influence unrelated cookies checks which it must not do.

3)

// origin matches matches

4) There are several places where you just use baseDomain in nsCookie::Create() which is especially consifusing in GetCookieFromRow() as the first comment is talks about to skip reading the baseDomain what we do that nevertheless. Could you add a comment on this baseDomain usage please?

comment:19 in reply to:  16 Changed 5 years ago by gk

Replying to michael:

Replying to gk:

3) nsICookie2.idl: Why is aOrigin of type ACString and not AUTF8String (like host, rawHost etc.)?

Most host string variables in implementation files are of type nsACString or nsCString which implies the IDL variables should use ACString. I assumed that's why you and Dan Witte made it ACString, but I agree that there should be some conformance.

Too bad there's so much AUTF8String already in nsCookie (frozen I think) so I'll change origin to be of type AUTF8String to conform.

Well, I was just wondering for the rationale behind it and did not look that deep. I mainly feared that it could break the cookie logic in a subtle way but testing indicated that I was wrong. Or maybe my tests were not elaborate enough.

comment:20 in reply to:  18 Changed 5 years ago by michael

Replying to gk:

1) Please document why you use one time mThirdPartyUtil->GetFirstPartyURIFromChannel and the other time mThirdPartyUtil->GetFirstPartyIsolationURI and what that implies.

GetFirstPartyIsolationURI is a special case of GetFirstPartyURIFromChannel which operates on the condition of party isolation (see requirement above 'Conditional operation') while the latter (GetFirstPartyURIFromChannel) passes the first party URI unconditionally.

Since we unconditionally store both keys when writing but make decisions (send cookie or not?) on the condition of privacy.thirdparty.isolate and the private browsing channel attribute, we use GetFirstPartyURIFromChannel when writing (SetCookieString) and GetFirstPartyIsolationURI when reading (GetCookieStringCommon.)

To clarify this difference I wrote two new comment lines in the source code near nsCookieService::GetCookieStringCommon. I think that's what you meant by 'document.'

2) You can't reuse requireHostMatch in SetCookieStringInternal as this would mean that the URL bar domain could influence unrelated cookies checks which it must not do.

Good catch, I'm still considering a solution.

3)

// origin matches matches

Thanks, I fixed that.

4) There are several places where you just use baseDomain in nsCookie::Create() which is especially consifusing in GetCookieFromRow() as the first comment is talks about to skip reading the baseDomain what we do that nevertheless. Could you add a comment on this baseDomain usage please?

There are two places that I've named the baseDomain variable near a call to nsCookie::Create, namely patch line 310 and line 344 but I haven't touched the persistent cookie code including GetCookieFromRow(). Should there be a comment near both lines 310 and 344 (in the patch msvb14058-283f7c6.patch) resembling 'unconditionally write the second (baseDomain) key as well as the first (host) key when storing cookies.' Is that what you mean?

Changed 5 years ago by michael

Attachment: msvb14058-283f7c6.patch added

Corrected and improved according to comments from second stage of code review

comment:21 Changed 3 years ago by gk

Resolution: wontfix
Status: needs_revisionclosed

No need for those tickets on our side anymore as Mozilla implemented double-keying of cookies which we ship in 7.0a3 when we switch to ESR 52.

Note: See TracTickets for help on using tickets.