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

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect, P3)

Description

[[e-mail]]

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

[Jesse Ruderman]

> we should use it in cookie code

Use it for what?

Comment 2

[[e-mail]]

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

Comment 3

[[e-mail]]

Attachment: 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.

Comment 4

[[e-mail]]

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.

Comment 5

[[e-mail]]

note to self, also need to handle failure cases.

Comment 6

[Shawn Wilsher :sdwilsh]

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...

Comment 7

[[e-mail]]

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

Comment 8

[[e-mail]]

Attachment: 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).

Comment 9

[[e-mail]]

Comment on attachment 288041 [details] [diff] [review]
v1

biesi, i'll buy you a creampie if you review this ;)

Comment 10

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 11

[[e-mail]]

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 12

[Daniel Veditz [:dveditz]]

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

Comment 13

[[e-mail]]

(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.

Comment 14

[[e-mail]]

Comment on attachment 288041 [details] [diff] [review]
v1

mconnor, care to do the honors? ;)

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 288041 [details] [diff] [review]
v1

sr=me with dveditz's comments addressed

Comment 16

[[e-mail]]

Attachment: 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.)

Comment 17

[[e-mail]]

checked in!

Comment 18

[Jeff Walden [:Waldo]]

If you need more Mochitest domains to properly test this, it's trivial to add them.

Comment 19

[[e-mail]]

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

Comment 21

[[e-mail]]

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.