use eTLD in cookies (stop sites setting cookies for the entire ".co.uk" domain)

RESOLVED FIXED in mozilla1.9beta2

Status

()

Core
Networking: Cookies
P3
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: dwitte@gmail.com, Assigned: dwitte@gmail.com)

Tracking

unspecified
mozilla1.9beta2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
wanted1.8.1.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [has-patch])

Attachments

(1 attachment, 2 obsolete attachments)

18.83 KB, patch
dwitte@gmail.com
: review+
dwitte@gmail.com
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
now that we have this shiny new eTLD service, we should use it in cookie code. should be a pretty simple patch; might even get it in for a6.

Comment 1

10 years ago
> we should use it in cookie code

Use it for what?
(Assignee)

Comment 2

10 years ago
checking if the domain the site is trying to set a cookie for is valid. there's currently nothing to stop a site setting a cookie for the entire ".co.uk" domain. see http://mxr.mozilla.org/mozilla/source/netwerk/cookie/src/nsCookieService.cpp#1985 and http://mxr.mozilla.org/mozilla/source/netwerk/cookie/src/nsCookieService.cpp#1818
(Assignee)

Updated

10 years ago
Blocks: 252342
(Assignee)

Updated

10 years ago
Depends on: 368989
(Assignee)

Comment 3

10 years ago
Created attachment 269187 [details] [diff] [review]
first-cut patch

something like this. note that the semantics in IsForeign(), used for enforcing the "allow cookies only from the originating website" pref, are now slightly different. previously, we'd consider the case where originating = "a.b.yahoo.com" and currenthost = "b.yahoo.com" to be foreign, since currenthost is one domain level up from the originating. with etld, we'll just see if the currenthost is in the base domain + 1 of the originating, i.e. "is b.yahoo.com within yahoo.com? yes -> not foreign". i think this is sane, though.

more importantly CheckDomain(), the method that really matters for enforcing sane cookie domains, should now work properly. note that this patch does depend on bug 368989, which is still a WIP. so until that's finalized and lands, this is just proof-of-concept.
(Assignee)

Updated

10 years ago
Blocks: 253974
(Assignee)

Updated

10 years ago
Target Milestone: --- → mozilla1.9beta1
(Assignee)

Comment 4

10 years ago
ftr we could also use etld to implement a more reliable per-host cookie limit (currently 50 per host). right now we count all cookies set for subdomains of the specific host in question, but a more sensible approach would be to count all cookies set for subdomains of the base domain + 1. this would make the limit stricter and non-circumventable.

Updated

10 years ago
Depends on: 386155

Updated

10 years ago
Depends on: 386154
(Assignee)

Comment 5

10 years ago
note to self, also need to handle failure cases.
(Assignee)

Updated

10 years ago
Target Milestone: mozilla1.9 M8 → mozilla1.9 M10

Updated

10 years ago
Flags: blocking1.9?
Summary: use eTLD in cookies → use eTLD in cookies (stop sites setting cookies for the entire ".co.uk" domain)

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
Comment on attachment 269187 [details] [diff] [review]
first-cut patch

>   mObserverService = do_GetService("@mozilla.org/observer-service;1");
>   if (mObserverService) {
>     mObserverService->AddObserver(this, "profile-before-change", PR_TRUE);
>     mObserverService->AddObserver(this, "profile-do-change", PR_TRUE);
>   }
> 
>   mPermissionService = do_GetService(NS_COOKIEPERMISSION_CONTRACTID);
> 
>+  nsresult rv;
>+  mTLDService = do_GetService(NS_EFFECTIVETLDSERVICE_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
You can error out here, but you've already registered observers to the observer service, so you'll leak...
(Assignee)

Comment 7

10 years ago
actually, they're weak refs, so it's safe... they won't leak or crash!

Updated

10 years ago
Priority: -- → P3

Updated

10 years ago
Whiteboard: [has-patch]
(Assignee)

Updated

10 years ago
Depends on: 402013
(Assignee)

Comment 8

10 years ago
Created attachment 288041 [details] [diff] [review]
v1

this should be the final thing. comment 3 applies here; in addition, i've also removed a couple of ToLowerCase() calls that should be unnecessary given URI normalization. this patch depends on bug 402013 (but can be modified if that doesn't go in).
Attachment #269187 - Attachment is obsolete: true
(Assignee)

Comment 9

10 years ago
Comment on attachment 288041 [details] [diff] [review]
v1

biesi, i'll buy you a creampie if you review this ;)
Attachment #288041 - Flags: review?(cbiesinger)
Comment on attachment 288041 [details] [diff] [review]
v1

>-nsCookieService::IsInDomain(const nsACString &aDomain,
>-   * special case for domainName = .hostName
>-   *    e.g., hostName = netscape.com
>-   *          domainName = .netscape.com

Maybe it's me being out of this code for too long, but where did that check go? I can't find the special case for those host cookies anymore.
(Assignee)

Comment 11

10 years ago
the base domain for netscape.com will be netscape.com, which the final StringEndsWith check will match (note the prepended dots prior to the check).

you raise a point though, it might be worth adding a "fast path" check to avoid calling up the etld service in that trivial case. the only hole that might add is if the website hostname actually is a tld (e.g. http://co.tv/ from https://bugzilla.mozilla.org/show_bug.cgi?id=252342#c17), then we'd allow it to set domain cookies, whereas the patch as it stands would downgrade them to host cookies only. given the negligible perf impact of the etld hash lookups, i'm inclined to leave the patch as is.
Comment on attachment 288041 [details] [diff] [review]
v1

>@@ -1736,56 +1686,46 @@ nsCookieService::IsForeign(nsIURI *aHost
>-  ToLowerCase(currentHost);
>-  ToLowerCase(firstHost);

IPv6 addresses contain hex chars so you might want to keep the lowercasing, although I guess the URIs should be normalized by this point. Since firstHost is now only needed in the rare case of IP addresses you could move its GetAsciiHost and dot trimming into the if-block dealing with that so it's not done every time. Every little bit helps? On the other hand this whole function isn't called unless people have set their prefs to check foreign cookies.

I didn't realize our foreign cookie checks always had a "fuzz" factor. Given that switching to a base-domain check is fine, but I'm not sure that's what people choosing the "no 3rd party cookies" think it means. What restrictions do IE and Safari use when they block foreign cookies?

>-     * note: RFC2109 section 4.3.2 requires that we check the following:
>-     * that the portion of host not in domain does not contain a dot.
>-     * this prevents hosts of the form x.y.co.nz from setting cookies in the
>-     * entire .co.nz domain. however, it's only a only a partial solution and
>-     * it breaks sites (IE doesn't enforce it), so we don't perform this check.

I think we should keep this note since we're still not following the spec exactly.

r=dveditz
Attachment #288041 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 13

10 years ago
(In reply to comment #12)

thanks for the surprise review - will fix up your comments when i check in.

wrt the foreign cookie checks, hmm. my own interpretation of "third party" is "a completely different domain" - i.e. something the base-domain check would catch. i'm not sure what rules IE uses (and don't have it handy to check), but all the language i've seen relating to their "accept cookies for the originating website only" just speaks of it rejecting "third party websites". given that domain cookies can be set for superdomains of a host, right up to and including the base domain, i wouldn't personally consider any of those to be third party.
(Assignee)

Comment 14

10 years ago
Comment on attachment 288041 [details] [diff] [review]
v1

mconnor, care to do the honors? ;)
Attachment #288041 - Flags: superreview?(mconnor)
Comment on attachment 288041 [details] [diff] [review]
v1

sr=me with dveditz's comments addressed
Attachment #288041 - Flags: superreview?(mconnor) → superreview+
(Assignee)

Comment 16

10 years ago
Created attachment 288748 [details] [diff] [review]
v2 (for checkin)

final patch w/ review comments addressed.

per discussion on IRC, i added a fast path check in IsForeign() to bail quickly if the hosts are identical. i believe this is also necessary for correctness in case the host itself is an eTLD (e.g. http://co.tv, evil!), because GetBaseDomain() will fail there. without this, we'd never let co.tv set any cookies if the user had that pref set. (we don't need any extra casing in CheckDomain() for this case, because that simply controls downgrading to a host cookie, which will rightly happen here.)
Attachment #288041 - Attachment is obsolete: true
Attachment #288748 - Flags: superreview+
Attachment #288748 - Flags: review+
(Assignee)

Comment 17

10 years ago
checked in!
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
If you need more Mochitest domains to properly test this, it's trivial to add them.
Flags: in-testsuite?
(Assignee)

Comment 19

10 years ago
added unit tests to cover blocking tld domain cookies and foreign cookie blocking.

Checking in test/TestCookie.cpp;
/cvsroot/mozilla/netwerk/test/TestCookie.cpp,v  <--  TestCookie.cpp
new revision: 1.22; previous revision: 1.21
done
Flags: in-testsuite? → in-testsuite+
Flags: wanted1.8.1.x+
Duplicate of this bug: 415214
(Assignee)

Comment 21

9 years ago
if we want this on 1.8.1.x, it'll require a wholesale backport of the eTLD service, along with the patch here. although, it wouldn't be too scary, given that this would be the only consumer on 1.8 branch (at least for now). i can do the backporting if we'd actually take it.
You need to log in before you can comment on or make changes to this bug.