Bugzilla@Mozilla – Bug 616264
Cookies set for www.foo.com. are sent to www.foo.com
Last modified: 2011-07-12 09:03:17 PDT
Summon comment box
Created attachment 494804 [details] step 1 Firefox considers the domains www.foo.com. (HOST1) to be the same as www.foo.com (HOST2) for cookie purposes. Note the trailing period in the first domain. Cookie values set by either HOST1/HOST2 can be retrieved by and sent to the other host. Tests showed that the hosts are considered different from crossdomain purposes. FF4b7, 3.6.11 and Safari 5.0.2 all exhibited the same behavior, while Chrome did not. See attached screenshots for demonstration. Steps used in screenshot 1. Visit www.tumblr.com. (controlled by me, sets a cookie) 2. Visit www.tumblr.com (note that the cookie I set is sent to tumblr.com) The tumblr issue isn't resolved yet. It seems to me that the Chrome way of handling hosts is more correct. However the specs may say otherwise. There may also be other areas of the code that exhibit similar behavior. User Agents FF4b7 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7) Gecko/20100101 Firefox/4.0b7 FF3.6.11 Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.11) Gecko/20101013 Ubuntu/10.04 (lucid) Firefox/3.6.11 Safari 5.0.2 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_5; en-us) AppleWebKit/533.18.1 (KHTML, like Gecko) Version/5.0.2 Safari/533.18.5 Chrome 8.0.552.215 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_5; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/8.0.552.215 Safari/534.10
Created attachment 494805 [details] step 2
Is there a reason why this particular behavior is bad? hosts of that form can't exist on the public web, correct?
(In reply to comment #2) > Is there a reason why this particular behavior is bad? hosts of that form > can't exist on the public web, correct? The browser considers the two domains different and I think the cookie handling should reflect that. In most cases, both domains will point to the same host and there will be no data leakage per se. I don't think I've seen this issue outside of tumblr. The bug is marked private to give tumblr time to fix the issue.
(In reply to comment #3) > The browser considers the two domains different and I think the cookie handling > should reflect that. In most cases, both domains will point to the same host > and there will be no data leakage per se. I misunderstood you. The issue is that any site could set a cookie for www.foo.com. and then www.foo.com would get it, correct? Or is it just that if your host has a period at the end?
The original Netscape spec said to do a "tail match" on the cookie domain with the host name, which would seem to treat foo. differently from foo (as we do elsewhere, e.g. same-origin checks). The latest draft of the proposed cookie spec update basically says the same thing (in many more words): http://tools.ietf.org/html/draft-ietf-httpstate-cookie-18#page-18 Our behavior is arguably both non-compliant and unsafe. We're definitely stripping the trailing dot explicitly, e.g. http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#1514 http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#2343 nsEffectiveTLDService, however, takes care to preserve the trailing dot. The permission manager does, too. The cookie code strips the trailing dot when saving a block permission (nsCookieService::Remove) but doesn't when calling CheckPrefs(), possibly leading to inconsistent results. That is, if you block a trailing-dot site from the cookie manager "block this site" checkbox then it won't actually be blocked, but if you block the site from the permissions tab of the page-info dialog (or about:data) then the trailing-dot site will in fact have its cookies blocked.
(In reply to comment #4) > I misunderstood you. The issue is that any site could set a cookie for > www.foo.com. and then www.foo.com would get it, correct? Or is it just that if > your host has a period at the end? both "foo.com" and "foo.com." save cookies into the same bucket. Both sites will receive all the cookies set by either of them. Scripts on either host will have access to the cookies from both sites (or, whichever was set most recently if they have cookie name collisions). Obviously this is mostly an edge case because most of the time the two forms do refer to the same host. But it's the kind of inconsistency that turns into a real gotcha if you find a site that's set up right (as tumblr appears to be at the moment).
FWIW I'm pretty sure this is a regression from 3.6 behavior, so we probably do want to block on it...
my understanding is that the more realistic case is: www.discovercard.com. (not controlled by me) www.discovercard.com[.localdomain] (controlled by me at your local internet cafe) where the [.localdomain] is part of an implied search path. although, realistically if i'm an evil dns provider, i might as well control everything instead of just properly abusing dns. in which case i could in fact replace www.discovercard.com. (well, until .com is DNSSEC signed in which case someone should hopefully complain that i broke the seal on .com...)
Per previous comments this seems to be a regression, so blocking.
Created attachment 497753 [details] [review] Patch (v1) I'm shamelessly stealing this bug from Dan!
Uh, I already had a complete patch in my queue for this... I'll skim your patch and see which one is more complete, and we can go from there. If it's mine, you've just tagged yourself for review!
(In reply to comment #11) > Uh, I already had a complete patch in my queue for this... > > I'll skim your patch and see which one is more complete, and we can go from > there. If it's mine, you've just tagged yourself for review! Fair enough! :-)
Created attachment 497896 [details] [review] Patch (v1.1) With a test fix!
Comment on attachment 497896 [details] [review] Patch (v1.1) This is the right idea, but not quite complete. New patch forthcoming...
Created attachment 498157 [details] [review] patch This handles a few more cases: it fails louder for the case where hosts are the string '.' (which is only relevant to file:// URIs, really); it explicitly does not accept such an argument for the 'domain=' attribute of Set-Cookie; and it tweaks the corresponding code in ThirdPartyUtil. (It would kinda be nice if that code wasn't copy-pasted...) I merged your tests into existing domain-ish cookie tests, and tweaked a few of the others. How does this look?
Comment on attachment 498157 [details] [review] patch So, this patch looks mostly correct to me. The only part which I don't really understand is why we need to fail explicitly for the "." case. I'm also asking for an additional review, since he's a peer and I don't feel 100% certain that I'm not missing anything on this patch.
(In reply to comment #16) > So, this patch looks mostly correct to me. The only part which I don't really > understand is why we need to fail explicitly for the "." case. Well, it just doesn't make much sense. We allow empty hosts explicitly for the file:// case (and only that), but asking for a domain cookie for that empty host is meaningless. > I'm also asking for an additional review, since he's a peer and I don't feel > 100% certain that I'm not missing anything on this patch. Cool, thanks for looking!
(In reply to comment #17) > (In reply to comment #16) > > So, this patch looks mostly correct to me. The only part which I don't really > > understand is why we need to fail explicitly for the "." case. > > Well, it just doesn't make much sense. We allow empty hosts explicitly for the > file:// case (and only that), but asking for a domain cookie for that empty > host is meaningless. Thanks for the clarification!
Comment on attachment 498157 [details] [review] patch Kicking to Shawn because he's a peer and has less review load.
(In reply to comment #19) > Kicking to Shawn because he's a peer and has less review load. Ha! I'll see if I can get to it tomorrow regardless.
Comment on attachment 498157 [details] [review] patch r=sdwilsh
Comment 7 was wrong; this is old behavior (though I did rework that code for trunk a while back). So we'll want backports here.
http://hg.mozilla.org/mozilla-central/rev/d4e6e2377500
Would be nice to get a branch patch for this.
As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)
Created attachment 511216 [details] [review] 1.9.2 patch Things work a bit differently on 1.9.2, and I don't want to be aggressive by backporting a bunch of other changes from 2.0. This makes pretty much the minimal set of changes to get sensible API behavior, and equivalent Set-Cookie behavior.
The patch for 1.9.1 is basically the same as for 1.9.2, so I'll wait for review feedback before posting it.
Created attachment 511512 [details] [review] 1.9.1 patch Posting 1.9.1 patch for the record.
Comment on attachment 511216 [details] [review] 1.9.2 patch r=sdwilsh
Comment on attachment 511512 [details] [review] 1.9.1 patch In that case, this one's almost the same!
(The only difference is that we don't have NetUtil or nsICookieManager2.getCookiesFromHost on 1.9.1.)
Comment on attachment 511512 [details] [review] 1.9.1 patch In that case, I don't see why I need to explicitly review this version then!
Would anyone like to land this on the 1.9.1 and 1.9.2 branches? I sadly don't have time for treewatching at the moment. :(
Once they get approved, I'll take care of landing them.
Comment on attachment 511216 [details] [review] 1.9.2 patch Approved for 1.9.2.17, a=dveditz for release-drivers
Comment on attachment 511512 [details] [review] 1.9.1 patch Approved for 1.9.1.19, a=dveditz for release-drivers
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a0c692da4524 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2b48d65b67c9
Dan: this turned the make check tests on 1.9.1 and 1.9.2 orange. Can you take a look please? Example log: <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6/1301610964.1301617247.7213.gz&fulltext=1>
Oh, you should be able to diff against and copy over the changes to the relevant tests from m-c's TestCookie. You can see from the log which ones are failing. If you have time to roll a patch I can review :)
Created attachment 523617 [details] [review] TestCookie.cpp fix for 1.9.2 This is basically <http://hg.mozilla.org/mozilla-central/raw-diff/d4e6e2377500/netwerk/test/TestCookie.cpp> (verbatim). It fixes the test failure on 1.9.2, but not on 1.9.1. I'm investigating why right now.
Comment on attachment 523617 [details] [review] TestCookie.cpp fix for 1.9.2 Wonderful. r=dwitte
Comment on attachment 523617 [details] [review] TestCookie.cpp fix for 1.9.2 The reason that this patch didn't fix 1.9.1 was a build problem. A clobber has fixed the problem completely, so I'll land it on both branches.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/34ce440e4b15 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4d8083b91ac8 And: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5db56d0cfa3b http://hg.mozilla.org/releases/mozilla-1.9.1/rev/011010dc4058 Because apparently I suck at pushing stuff today. :/
The branch fixes here have caused bug 650522 on 1.9.1 and 1.9.2.
Backed out from 1.9.1 and 1.9.2 due to the regression in bug 650522: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1a8da3d914c3 (default) http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fab6db5c327d (relbranch) http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9e0d3f6ee9af (default) http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b2756532de0f (relbranch)
Comment on attachment 511216 [details] [review] 1.9.2 patch un-approving the patches we had to back out.
Re-assigning to Ehsan to re-land this on 1.9.2 along with the patch in bug 650522. That patch still needs reviews before approval, you could put a roll-up patch in this bug so it shows up on our higher-priority "blocking" list.
Dan: ping? I need review on bug 650522. The code freeze for 1.9.2.18 is this monday, and if I don't get reviews in time, this will miss the release...
I missed it last weekend. :( I'll take a look by end of day tomorrow. If I forget, please ping!
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bb728fdcd717
Verified for 1.9.2.18 using original scenario in comment 0 and build Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.18pre) Gecko/20110607 Namoroka/3.6.18pre (.NET CLR 3.5.30729). Verified that the checked in test is passing as well.