Last Comment Bug 616264 - (CVE-2011-2362) Cookies set for www.foo.com. are sent to www.foo.com
(CVE-2011-2362)
: Cookies set for www.foo.com. are sent to www.foo.com
Status: RESOLVED FIXED
[sg:moderate] (sg:high for affected s...
: verified1.9.2
Product: Core
Classification: Components
Component: Networking: Cookies (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: :Ehsan Akhgari
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on: 650522
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-02 13:28 PST by David Chan [:dchan]
Modified: 2011-07-12 09:03 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+
.18+
.18-fixed
wontfix


Attachments
step 1 (275.05 KB, image/png)
2010-12-02 13:28 PST, David Chan [:dchan]
no flags Details
step 2 (558.15 KB, image/png)
2010-12-02 13:28 PST, David Chan [:dchan]
no flags Details
Patch (v1) (5.01 KB, patch)
2010-12-15 01:59 PST, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v1.1) (8.18 KB, patch)
2010-12-15 14:04 PST, :Ehsan Akhgari
dwitte: review-
Details | Diff | Splinter Review
patch (21.04 KB, patch)
2010-12-16 11:12 PST, dwitte@gmail.com
ehsan: review+
sdwilsh: review+
Details | Diff | Splinter Review
1.9.2 patch (23.81 KB, patch)
2011-02-09 15:52 PST, dwitte@gmail.com
sdwilsh: review+
dveditz: approval1.9.2.17-
christian: approval1.9.2.18+
Details | Diff | Splinter Review
1.9.1 patch (21.74 KB, patch)
2011-02-10 14:33 PST, dwitte@gmail.com
dveditz: approval1.9.1.19-
Details | Diff | Splinter Review
TestCookie.cpp fix for 1.9.2 (3.03 KB, patch)
2011-04-01 09:35 PDT, :Ehsan Akhgari
dwitte: review+
Details | Diff | Splinter Review

Description David Chan [:dchan] 2010-12-02 13:28:25 PST
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
Comment 1 David Chan [:dchan] 2010-12-02 13:28:41 PST
Created attachment 494805 [details]
step 2
Comment 2 Shawn Wilsher :sdwilsh 2010-12-02 13:56:15 PST
Is there a reason why this particular behavior is bad?  hosts of that form can't exist on the public web, correct?
Comment 3 David Chan [:dchan] 2010-12-02 14:18:18 PST
(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.
Comment 4 Shawn Wilsher :sdwilsh 2010-12-02 14:22:45 PST
(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?
Comment 5 Daniel Veditz [:dveditz] 2010-12-02 14:31:15 PST
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.
Comment 6 Daniel Veditz [:dveditz] 2010-12-02 14:36:16 PST
(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).
Comment 7 dwitte@gmail.com 2010-12-10 14:35:52 PST
FWIW I'm pretty sure this is a regression from 3.6 behavior, so we probably do want to block on it...
Comment 8 timeless 2010-12-11 17:00:21 PST
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...)
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2010-12-13 14:43:25 PST
Per previous comments this seems to be a regression, so blocking.
Comment 10 :Ehsan Akhgari 2010-12-15 01:59:10 PST
Created attachment 497753 [details] [diff] [review]
Patch (v1)

I'm shamelessly stealing this bug from Dan!
Comment 11 dwitte@gmail.com 2010-12-15 10:34:05 PST
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!
Comment 12 :Ehsan Akhgari 2010-12-15 14:04:18 PST
(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!  :-)
Comment 13 :Ehsan Akhgari 2010-12-15 14:04:59 PST
Created attachment 497896 [details] [diff] [review]
Patch (v1.1)

With a test fix!
Comment 14 dwitte@gmail.com 2010-12-16 11:08:22 PST
Comment on attachment 497896 [details] [diff] [review]
Patch (v1.1)

This is the right idea, but not quite complete. New patch forthcoming...
Comment 15 dwitte@gmail.com 2010-12-16 11:12:52 PST
Created attachment 498157 [details] [diff] [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 16 :Ehsan Akhgari 2010-12-16 23:39:04 PST
Comment on attachment 498157 [details] [diff] [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.
Comment 17 dwitte@gmail.com 2010-12-17 17:10:15 PST
(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!
Comment 18 :Ehsan Akhgari 2010-12-18 14:05:28 PST
(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 19 dwitte@gmail.com 2010-12-21 13:57:42 PST
Comment on attachment 498157 [details] [diff] [review]
patch

Kicking to Shawn because he's a peer and has less review load.
Comment 20 Shawn Wilsher :sdwilsh 2010-12-21 14:47:49 PST
(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 21 Shawn Wilsher :sdwilsh 2010-12-22 06:11:09 PST
Comment on attachment 498157 [details] [diff] [review]
patch

r=sdwilsh
Comment 22 dwitte@gmail.com 2010-12-22 13:54:38 PST
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.
Comment 24 Daniel Veditz [:dveditz] 2011-01-03 10:52:42 PST
Would be nice to get a branch patch for this.
Comment 25 christian 2011-01-04 15:47:27 PST
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)
Comment 26 dwitte@gmail.com 2011-02-09 15:52:08 PST
Created attachment 511216 [details] [diff] [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.
Comment 27 dwitte@gmail.com 2011-02-09 16:29:39 PST
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.
Comment 28 dwitte@gmail.com 2011-02-10 14:33:55 PST
Created attachment 511512 [details] [diff] [review]
1.9.1 patch

Posting 1.9.1 patch for the record.
Comment 29 Shawn Wilsher :sdwilsh 2011-02-11 05:56:30 PST
Comment on attachment 511216 [details] [diff] [review]
1.9.2 patch

r=sdwilsh
Comment 30 dwitte@gmail.com 2011-02-11 11:18:28 PST
Comment on attachment 511512 [details] [diff] [review]
1.9.1 patch

In that case, this one's almost the same!
Comment 31 dwitte@gmail.com 2011-02-11 11:19:12 PST
(The only difference is that we don't have NetUtil or nsICookieManager2.getCookiesFromHost on 1.9.1.)
Comment 32 Shawn Wilsher :sdwilsh 2011-02-11 20:18:01 PST
Comment on attachment 511512 [details] [diff] [review]
1.9.1 patch

In that case, I don't see why I need to explicitly review this version then!
Comment 33 dwitte@gmail.com 2011-03-28 23:06:44 PDT
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. :(
Comment 34 :Ehsan Akhgari 2011-03-29 12:18:17 PDT
Once they get approved, I'll take care of landing them.
Comment 35 Daniel Veditz [:dveditz] 2011-03-30 10:44:33 PDT
Comment on attachment 511216 [details] [diff] [review]
1.9.2 patch

Approved for 1.9.2.17, a=dveditz for release-drivers
Comment 36 Daniel Veditz [:dveditz] 2011-03-30 10:44:46 PDT
Comment on attachment 511512 [details] [diff] [review]
1.9.1 patch

Approved for 1.9.1.19, a=dveditz for release-drivers
Comment 38 :Ehsan Akhgari 2011-03-31 17:54:18 PDT
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>
Comment 39 dwitte@gmail.com 2011-03-31 17:56:35 PDT
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 :)
Comment 40 :Ehsan Akhgari 2011-04-01 09:35:48 PDT
Created attachment 523617 [details] [diff] [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 41 dwitte@gmail.com 2011-04-01 10:20:01 PDT
Comment on attachment 523617 [details] [diff] [review]
TestCookie.cpp fix for 1.9.2

Wonderful. r=dwitte
Comment 42 :Ehsan Akhgari 2011-04-01 11:29:45 PDT
Comment on attachment 523617 [details] [diff] [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.
Comment 44 :Ehsan Akhgari 2011-04-17 14:35:59 PDT
The branch fixes here have caused bug 650522 on 1.9.1 and 1.9.2.
Comment 46 Daniel Veditz [:dveditz] 2011-04-20 10:31:24 PDT
Comment on attachment 511216 [details] [diff] [review]
1.9.2 patch

un-approving the patches we had to back out.
Comment 47 Daniel Veditz [:dveditz] 2011-05-16 10:39:51 PDT
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.
Comment 48 :Ehsan Akhgari 2011-06-03 16:09:02 PDT
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...
Comment 49 dwitte@gmail.com 2011-06-03 16:21:44 PDT
I missed it last weekend. :( I'll take a look by end of day tomorrow. If I forget, please ping!
Comment 51 Al Billings [:abillings] 2011-06-07 16:13:51 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.