616264 - (CVE-2011-2362) Cookies set for www.foo.com. are sent to www.foo.com

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[David Chan [:dchan]]

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

Attachment: step 2

Comment 2

[Shawn Wilsher :sdwilsh]

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

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

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

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

(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

[[e-mail]]

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]

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)]

Per previous comments this seems to be a regression, so blocking.

Comment 10

[(no longer active)]

Attachment: Patch (v1)

I'm shamelessly stealing this bug from Dan!

Comment 11

[[e-mail]]

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

[(no longer active)]

(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

[(no longer active)]

Attachment: Patch (v1.1)

With a test fix!

Comment 14

[[e-mail]]

Comment on attachment 497896 [details] [diff] [review]
Patch (v1.1)

This is the right idea, but not quite complete. New patch forthcoming...

Comment 15

[[e-mail]]

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

[(no longer active)]

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

[[e-mail]]

(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

[(no longer active)]

(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

[[e-mail]]

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]

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

Comment on attachment 498157 [details] [diff] [review]
patch

r=sdwilsh

Comment 22

[[e-mail]]

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 23

[[e-mail]]

http://hg.mozilla.org/mozilla-central/rev/d4e6e2377500

Comment 24

[Daniel Veditz [:dveditz]]

Would be nice to get a branch patch for this.

Comment 25

[LegNeato]

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

[[e-mail]]

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

[[e-mail]]

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

[[e-mail]]

Attachment: 1.9.1 patch

Posting 1.9.1 patch for the record.

Comment 29

[Shawn Wilsher :sdwilsh]

Comment on attachment 511216 [details] [diff] [review]
1.9.2 patch

r=sdwilsh

Comment 30

[[e-mail]]

Comment on attachment 511512 [details] [diff] [review]
1.9.1 patch

In that case, this one's almost the same!

Comment 31

[[e-mail]]

(The only difference is that we don't have NetUtil or nsICookieManager2.getCookiesFromHost on 1.9.1.)

Comment 32

[Shawn Wilsher :sdwilsh]

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

[[e-mail]]

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

[(no longer active)]

Once they get approved, I'll take care of landing them.

Comment 35

[Daniel Veditz [:dveditz]]

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

Comment on attachment 511512 [details] [diff] [review]
1.9.1 patch

Approved for 1.9.1.19, a=dveditz for release-drivers

Comment 37

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a0c692da4524
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2b48d65b67c9

Comment 38

[(no longer active)]

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

[[e-mail]]

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

[(no longer active)]

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

[[e-mail]]

Comment on attachment 523617 [details] [diff] [review]
TestCookie.cpp fix for 1.9.2

Wonderful. r=dwitte

Comment 42

[(no longer active)]

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 43

[(no longer active)]

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

Comment 44

[(no longer active)]

The branch fixes here have caused bug 650522 on 1.9.1 and 1.9.2.

Comment 45

[(no longer active)]

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 46

[Daniel Veditz [:dveditz]]

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

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

[(no longer active)]

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

[[e-mail]]

I missed it last weekend. :( I'll take a look by end of day tomorrow. If I forget, please ping!

Comment 50

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bb728fdcd717

Comment 51

[Al Billings [:abillings - ex-MoCo]]

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.