Bug 616264 (CVE-2011-2362)

Cookies set for www.foo.com. are sent to www.foo.com

RESOLVED FIXED

Status

()

Core
Networking: Cookies
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: dchan, Assigned: Ehsan)

Tracking

({verified1.9.2})

Trunk
x86
Mac OS X
verified1.9.2
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+, blocking1.9.2 .18+, status1.9.2 .18-fixed, status1.9.1 wontfix)

Details

(Whiteboard: [sg:moderate] (sg:high for affected sites))

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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
(Reporter)

Comment 1

7 years ago
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?
(Reporter)

Comment 3

7 years ago
(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).
Whiteboard: [sg:moderate] (sg:high for affected sites)
status1.9.1: --- → wanted
status1.9.2: --- → wanted

Updated

7 years ago
Assignee: nobody → dwitte
blocking2.0: --- → ?

Comment 7

7 years ago
FWIW I'm pretty sure this is a regression from 3.6 behavior, so we probably do want to block on it...

Comment 8

7 years ago
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.
blocking2.0: ? → beta9+
Keywords: regression
Created attachment 497753 [details] [diff] [review]
Patch (v1)

I'm shamelessly stealing this bug from Dan!
Assignee: dwitte → ehsan
Status: NEW → ASSIGNED
Attachment #497753 - Flags: review?(dwitte)

Comment 11

7 years ago
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] [diff] [review]
Patch (v1.1)

With a test fix!
Attachment #497753 - Attachment is obsolete: true
Attachment #497896 - Flags: review?(dwitte)
Attachment #497753 - Flags: review?(dwitte)

Comment 14

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

This is the right idea, but not quite complete. New patch forthcoming...
Attachment #497896 - Flags: review?(dwitte) → review-

Comment 15

7 years ago
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?
Attachment #498157 - Flags: review?(ehsan)
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.
Attachment #498157 - Flags: review?(ehsan)
Attachment #498157 - Flags: review?(bzbarsky)
Attachment #498157 - Flags: review+

Comment 17

7 years ago
(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 19

7 years ago
Comment on attachment 498157 [details] [diff] [review]
patch

Kicking to Shawn because he's a peer and has less review load.
Attachment #498157 - Flags: review?(bzbarsky) → review?(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 on attachment 498157 [details] [diff] [review]
patch

r=sdwilsh
Attachment #498157 - Flags: review?(sdwilsh) → review+
blocking1.9.1: --- → needed
blocking1.9.2: --- → needed

Comment 22

7 years ago
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.
Keywords: regression

Comment 23

7 years ago
http://hg.mozilla.org/mozilla-central/rev/d4e6e2377500
Assignee: ehsan → dwitte
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Would be nice to get a branch patch for this.

Comment 25

7 years ago
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)
blocking2.0: beta9+ → betaN+
blocking1.9.1: needed → ?
blocking1.9.2: needed → ?
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
blocking1.9.1: needed → .18+
blocking1.9.2: needed → .15+

Comment 26

7 years ago
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.
Attachment #511216 - Flags: review?(sdwilsh)

Comment 27

7 years ago
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

7 years ago
Created attachment 511512 [details] [diff] [review]
1.9.1 patch

Posting 1.9.1 patch for the record.
Comment on attachment 511216 [details] [diff] [review]
1.9.2 patch

r=sdwilsh
Attachment #511216 - Flags: review?(sdwilsh) → review+

Comment 30

7 years ago
Comment on attachment 511512 [details] [diff] [review]
1.9.1 patch

In that case, this one's almost the same!
Attachment #511512 - Flags: review?(sdwilsh)

Comment 31

7 years ago
(The only difference is that we don't have NetUtil or nsICookieManager2.getCookiesFromHost on 1.9.1.)
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!
Attachment #511512 - Flags: review?(sdwilsh)

Comment 33

7 years ago
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. :(
Keywords: checkin-needed
Attachment #511216 - Flags: approval1.9.2.17?
Attachment #511512 - Flags: approval1.9.1.19?
Once they get approved, I'll take care of landing them.
Keywords: checkin-needed
Comment on attachment 511216 [details] [diff] [review]
1.9.2 patch

Approved for 1.9.2.17, a=dveditz for release-drivers
Attachment #511216 - Flags: approval1.9.2.17? → approval1.9.2.17+
Comment on attachment 511512 [details] [diff] [review]
1.9.1 patch

Approved for 1.9.1.19, a=dveditz for release-drivers
Attachment #511512 - Flags: approval1.9.1.19? → approval1.9.1.19+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a0c692da4524
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2b48d65b67c9
status1.9.1: wanted → .20-fixed
status1.9.2: wanted → .18-fixed
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

7 years ago
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] [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.
Attachment #523617 - Flags: review?(dwitte)

Comment 41

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

Wonderful. r=dwitte
Attachment #523617 - Flags: review?(dwitte) → review+
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.
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.  :/
status1.9.1: .20-fixed → .19-fixed
status1.9.2: .18-fixed → .17-fixed
Depends on: 650522
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)
blocking1.9.1: .19+ → ?
blocking1.9.2: .17+ → ?
status1.9.1: .19-fixed → ?
status1.9.2: .17-fixed → ?
status1.9.1: ? → wanted
status1.9.2: ? → wanted
blocking1.9.1: ? → .20+
blocking1.9.2: ? → .18+
Comment on attachment 511216 [details] [diff] [review]
1.9.2 patch

un-approving the patches we had to back out.
Attachment #511216 - Flags: approval1.9.2.17+ → approval1.9.2.17-
Attachment #511512 - Flags: approval1.9.1.19+ → approval1.9.1.19-
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.
Assignee: dwitte → ehsan
blocking1.9.1: .20+ → ---
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

6 years ago
I missed it last weekend. :( I'll take a look by end of day tomorrow. If I forget, please ping!
Attachment #511216 - Flags: approval1.9.2.18?

Updated

6 years ago
Attachment #511216 - Flags: approval1.9.2.18? → approval1.9.2.18+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bb728fdcd717
status1.9.1: wanted → wontfix
status1.9.2: wanted → .18-fixed
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.
Keywords: verified1.9.2
Alias: CVE-2011-2362
Depends on: 667087
No longer depends on: 667087
Group: core-security
You need to log in before you can comment on or make changes to this bug.