Closed Bug 155114 Opened 22 years ago Closed 22 years ago

Stealing cookies based on path attribute

Categories

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

x86
Windows NT
defect

Tracking

()

VERIFIED FIXED
mozilla1.1alpha

People

(Reporter: morse, Assigned: morse)

References

Details

(Whiteboard: [adt2 RTM] [ETA 08/05][verified-trunk])

Attachments

(1 file, 13 obsolete files)

518 bytes, patch
morse
: review+
Details | Diff | Splinter Review
The test for determining if a site can read a cookie involves doing a match on 
the leading portion of the path.  For example, if there is a cookie set for

   mywebpage.netscape.com/stephenpmorse

then a site at

   mywebpage.netscape.com/mitchelstoltz

cannot read that cookie whereas a site in the following path can read it.

   mywebpage.netscape.com/stephenmpmorse/pictures

That is what was intended.  However an attacker's site at

   mywebpage.netscape.com/stephepmorseattack

can read that cookie.  This attack works in 4.x as well as mozilla.

Note that the same is true for setting of cookies in 4.x.  However there is a 
separate bug with setting of cookies in mozilla (see bug 155083).
Severity: normal → critical
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.1alpha
For a demonstration of this attack, go to the following site:

   http://home.pacbell.net/spmorse/ellis/ellisjw.html

That will set a cookie called ShowExtraButton

Now go to http://home.pacbell.net/spmorse/ellis/ellisattack/attack.htm.  That 
will attempt to read the cookie and will be successful.  That's OK because this 
was a subpath of the original path.

Next go to http://home.pacbell.net/spmorse/ellisattack/attack.htm.  That too 
will attempt to read the cookie and will be successful.  That's bad because this 
is an attacker site.

The html in attack.htm in both of the above attempts to read a cookie is as 
follows:

  <html>
    <body>
      <script>
        alert("cookie="+document.cookie);
        document.cookie = "x=y;path=/spmorse/ellis";
      </script>
      Cookie Setting Test
    </body>
  </html>
IE is not susceptable to this attack!
Keywords: nsbeta1
Whiteboard: [adt1 RTM] [ETA Needed]
Whiteboard: [adt1 RTM] [ETA Needed] → [adt1 RTM] [ETA 7-1]
Blocks: 155083
Comment on attachment 89741 [details] [diff] [review]
Patch for this bug as well as related bug 155083

r=mstoltz. This is definitely a stop-ship.
Attachment #89741 - Flags: review+
Comment on attachment 89741 [details] [diff] [review]
Patch for this bug as well as related bug 155083

+PRBool
+cookie_pathOK(const char* cookiePath, const char* currentPath) {

This method should be static.

Further down in the diff:

+
+  /* Strip down everything after the last slash to get the path,
+   * ignoring slashes in the query string part.
+   */
+  char * iter = PL_strchr(cur_path.get(), '?');
+  if(iter) {
+    *iter = '\0';
+  }
+  iter = PL_strrchr(cur_path.get(), '/');
+  if(iter) {
+    *iter = '\0';
+  }

The above code will cut off the trailing '/' in the path...

+
+  /* set path if none found in header, else verify that host has authority for
indicated path */
   if(!path_from_header) {
...
+  } else {
+    if(!cookie_pathOK(path_from_header, cur_path.get())) {

And here we pass in the string w/o the traisling slash as the second argument
to cookie_pathOK(). cookie_pathOK() requires that the last character (assuming
the current path and the cookie path are equal lengths) in the second argument
is a '/', so if we ever get here in such a situation the cookie path will not
be considerd OK. Is that what this is intended to do?
Attached patch address items in comment #5 (obsolete) — Splinter Review
Attachment #89741 - Attachment is obsolete: true
Attachment #89946 - Attachment is obsolete: true
Comment on attachment 89947 [details] [diff] [review]
Remove extra characters that accidentally crept into file

- cookie_pathOK(const char* cookiePath, const char* currentPath) {

...
+  // determine length of each, excluding trailing slash if present
+  int cookiePathLen = PL_strlen(cookiePath);
+  int currentPathLen = PL_strlen(currentPath);
+  if (cookiePathLen && cookiePath[cookiePathLen-1] == '/') {
+    cookiePathLen--;
+  }
+  if (currentPathLen && currentPath[currentPathLen-1] == '/') {
+    currentPathLen--;
+  }
+
+  // test for equality case
+  if (!PL_strcmp(currentPath, cookiePath)) {

Assume you run into a case where currentPath and cookiePath are the same paths
with the only difference being that the cookiePath has a trailing '/' and one
doesn't. In such a case the cookie path is ok, but this code would claim that
it's not ok. If the above check would be written as:

+  if (currentPathLen == cookiePathLen &&
+      !PL_strncmp(currentPath, cookiePath, currentPathLen)) {

Then that case would be taken care of too, right? Or are we guaranteed that the
cookie path never contains trailing slashes? If so, then why do we have the
code above that decrements the cookiePathLen if the cookiePath ends with a '/'?

And how would this code (and the rest of the code around this code) handle
broken paths that have multiple trailing slashes (i.e. foo/bar//baz///)? 

sr=jst with the above either fixed or proven to not be a problem.
Attachment #89947 - Flags: superreview+
Oops, you are absolutely correct -- my test in the equality case was flawed.  I 
did go to the effort of updating the lengths to get rid of trailing slashes, but 
then I forgot to take that updated length into account when making the equality 
test.

Attaching new patch with you change.

Regarding multiple trailing slashes, the only way that could creep in is if the 
server sent back a set-cookie header with a bad path attribute.  In that case 
the path test will fail and the cookie will never get set.
Attached patch address issue in comment 8 (obsolete) — Splinter Review
Attachment #89947 - Attachment is obsolete: true
Comment on attachment 90009 [details] [diff] [review]
address issue in comment 8

sr=jst per comment 8
Attachment #90009 - Flags: superreview+
Comment on attachment 90009 [details] [diff] [review]
address issue in comment 8

r=mstoltz.
Attachment #90009 - Flags: review+
Fix checked in on trunk
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment on attachment 90009 [details] [diff] [review]
address issue in comment 8

a=chofmann for 1.0.1.  add fixed1.0.1 after the change is on the branch.  

tever, can you verify this one branch and trunk?
Attachment #90009 - Flags: approval+
Keywords: mozilla1.0.1+
OK, we've hit a problem with this patch.  See bug 155768 comment 5 for details.

Basically we are breaking a site that is attempting to set a cookie for a path 
that it does not control.  The site is in error and is violating the RFC2109 
spec.

Note that the patch in this bug report is really a combined patch for two bugs.  
One part of the patch prevents the setting of cookies when the path test fails 
(that is for bug 155083) and the other is for reading of cookies when the path 
test fails (that is for this bug).  The setting is the less serious offense, and 
I can not think of any attack based on that.  The site that we are breaking in 
bug 155768 has to do with the setting of cookies.

Therefore I see two possible ways to go:

1. If the problem is localized to just that one site and the site is considered 
unimportant, we can evangelize the site and ask them to fix their set-cookie 
header.

2. If the problem is more widespread and/or occurs on important sites, we need 
to back off on the part of the patch that involves the setting of cookies.  We 
would still keep the part dealing with the reading of cookies because that is 
the part for which we have a known attack as described in this bug report.

My recommendation would be to go with (1) on the trunk since we would have time 
to see who else we are breaking and go with (2) on the branch so that has no 
risk and does stop the attack.
Just in case it's not clear, the part of the patch that involves the setting of 
cookies is the section for 

  @@ -1272,19 +1299,29 @@

Just remove that part of the patch, and you will be left with a patch that 
involves the reading of cookies only.
using testcases from comment #2, verified that site with incorrect path is not
able to read cookies

verified trunk - 07/08/2002 builds, win NT4, linux rh6, mac osX

will need tested on branch
Status: RESOLVED → VERIFIED
Whiteboard: [adt1 RTM] [ETA 7-1] → [adt1 RTM] [ETA 7-1][verified-trunk]
adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch. pls check
this into asap, then replace "mozilla1.0.1+" with "fixed1.0.1".
Blocks: 143047
Keywords: nsbeta1adt1.0.1+, nsbeta1+
Whiteboard: [adt1 RTM] [ETA 7-1][verified-trunk] → [adt1 RTM] [ETA 07/10][verified-trunk]
What about the issue mentioned in comment #15?
regarding comment 15, adt agreed to go with the partial fix on the branch.  That 
is what I will be checking in
Fixed for reading cookies has now been checked in on branch.  Fix for setting of 
cookies will not go on the branch.
The check in done to the branch breaks online banking with Deutsche Bank.

I have filed bug 157787.
Rather than post the patch to bug reports for individual sites that are now 
broken (e.g., bug 157787 and others), I'm going to reopen this report and post 
a new patch here.  Then when this is checked in and working, we can close down 
those other reports as works-for-me.

Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Attaching patch to be applied to the previous patch that was already checked 
in.

This new patch will take the path attribute as given by the set-cookie header 
and store that in the cookie.  But when testing for a valid path, it will ignore 
anything in the path attribute that follows the last slash, and compare that to 
the path in the requesting url (again ignoring anything that follows the last 
slash).

This fixes the attack described here, and does not break the sites that were 
broken by the previous patch.

The patch I'm about to attach applies to the trunk.  For the branch, the last 
part of the patch (i.e., starting with "@@ -1309) should be ignored because that 
part was never checked into the branch.
Status: REOPENED → ASSIGNED
Keywords: fixed1.0.1
Whiteboard: [adt1 RTM] [ETA 07/10][verified-trunk] → [adt1 RTM] [ETA 07/18]
Attached patch more efficient fix (obsolete) — Splinter Review
Attachment #91684 - Attachment is obsolete: true
Comment on attachment 91697 [details] [diff] [review]
more efficient fix

sr=jag
Attachment #91697 - Flags: superreview+
Comment on attachment 91697 [details] [diff] [review]
more efficient fix

Looks good. r=mstoltz.
Attachment #91697 - Flags: superreview+ → review+
Comment on attachment 91697 [details] [diff] [review]
more efficient fix

looks like jag's sr didn't take. add that, and a=chofmann for 1.0.1 and trunk.
Attachment #91697 - Flags: superreview+
Attachment #91697 - Flags: approval+
adding mozilla1.0.1+ based on chofmann's comments.  Please check this in today.
Keywords: mozilla1.0.1+
Checked in on branch and trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
tever: can you pls verify this as fixed in tomorrow's build?
I am still seeing this on the 07/18 builds - branch only.   WinNT4 and mac osX
at least.  Re-opening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
oops, actually I didn't need to re-open this.  This is working on todays trunk.
Status: REOPENED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
My fault.  The patch in this bug report is for the trunk and only part of it 
applies to the branch.  But when removing the part that didn't apply to the 
branch, I removed too much.  So I just checked into the branch the part that I 
shouldn't have removed.

Tever, try again with tomorrows branch build and things should be better.
This is the only checkin related to cookies which landed between yesterday's and
today's builds. Today's build, 2002071808, breaks blogger.com's blog posting.
Every time I load the blogger tool or submit a post, blogger.com warns me that
it can tell from my browser cookies that something wen't wrong with the previous
post. I suspect that changes for this bug caused that to regress. 

To test this go to blogger.com and log in as user mozillatest pass mozillatest.
On the right side of the page you'll see a blue box titled Your Blogs. In that
box select the blog "mozillatest" and then attempt to post anything to this blog
and see the warning. 

This page uses cookies set by JavaScript.
Cookies set by jhavascript now include the filename in the path instead of just
the directory. This is what's breaking blogger.com. 
Steve, is that what we intended to do? The cookie path shouldn't include the
filename, should it?
Let's try that once more.

Previously I argued that we didn't need to strip the file name off of the path 
because the pathok test was doing that stripping.  But I overlooked the test for 
duplicate cookies in the cookie list.  When a cookie is added, a test is made to 
see if there already exists a cookie with the same name, domain, and path.

What is happening in the blogger website is that a cookie is being set with no 
path specified.  So we were using the requesting URL as the path, and not 
stripping the file name.  So the path was being set to "/blog_form.pyra". Then 
later they set a javascript cookie (the fact that it was a js cookie was not 
relevant) and explicitly specified a path of "/".  That had a different path 
from the previous cookie, so it was set a a separate cookie rather than 
overwriting the previous one.

The fix is to put back the orginal test for finding the last slash and 
terminating the path there.  However the original test ended the path string 
just before the slash (not including it) and that was what made the attack 
possible.  I know realize that the correct thing to do is to terminate the path 
just after the slash.

Attaching patches for both branch and trunk (they are slightly different) to 
rectify this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch additional patch for trunk (obsolete) — Splinter Review
Attached patch additional patch for branch. (obsolete) — Splinter Review
Let me clarify what this latest patch is doing and what it is not doing.

It does not affect explicit path attributes specified when setting the cookie.  
So if the set-cookie header had a path= attribute, that value is what will be 
stored as the path part of the cookie.

It does affect what we store in the path part of a cookie when there is no path= 
attribute.  In that case we take the path part of requesting URL and strip off 
the filename.
Status: REOPENED → ASSIGNED
Keywords: fixed1.0.1
removing adt1.0.1+, as there is a new patch that needs reviews.
Keywords: adt1.0.1+
Whiteboard: [adt1 RTM] [ETA 07/18] → [adt1 RTM] [ETA 07/22] [needs reviews]
Comment on attachment 91926 [details] [diff] [review]
additional patch for trunk

sr=dveditz

Storing the trailing slash appears to be what IE does
Attachment #91926 - Flags: superreview+
Comment on attachment 91927 [details] [diff] [review]
additional patch for branch.

sr=dveditz
Attachment #91927 - Flags: superreview+
Comment on attachment 91926 [details] [diff] [review]
additional patch for trunk

r=mstoltz.
Attachment #91926 - Flags: review+
Comment on attachment 91927 [details] [diff] [review]
additional patch for branch.

r=mstoltz.
Attachment #91927 - Flags: review+
Comment on attachment 91926 [details] [diff] [review]
additional patch for trunk

a=asa (on behalf of drivers) for checkin to 1.1

We're trying really hard to get 1.1beta out the door and this is the last
outstanding issue. Can you please land this before Monday? Thanks.
Attachment #91926 - Flags: approval+
I'm afraid I'm going to have to throw in the towel on this one.  Initially I 
thought that this latest patch fixed the problem with my.netscape.com, but 
further testing revealed that it hadn't.  That's why I didn't make a request of 
drivers and adt for branch/trunk checkin approval.

After spending a day on this, I believe I know what the problem is, but any fix 
at this late date runs the risk of breaking something else.  So I'm going to 
post a patch shortly that backs out everything and puts the path testing to the 
way it used to be.  That means we will still be vulnerable to this attack, but 
we are guaranteed that no websites will be broken.

I'll continue working on stoping the attack, and and will land it on the trunk 
after 1.1.
Attachment #92052 - Attachment is obsolete: true
Attachment #92058 - Attachment is obsolete: true
Comment on attachment 92057 [details] [diff] [review]
patch to undo previous patches on the trunk

a=asa (on behalf of drivers) for checkin to 1.1
Attachment #92057 - Flags: approval+
marking this bug as nsbeta1- because of Comment #49 From Stephen P. Morse.
making this change, this late in the game seems to be far too risky, and could
brak many sites. 
Keywords: nsbeta1+nsbeta1-
Whiteboard: [adt1 RTM] [ETA 07/22] [needs reviews] → [adt1] [Not for MachV]
But we have to change something now because the checkin that was made
breaks websites.  What I said was that we shouldn't make another checkin
to try to fix the bug (stop the cookie attack).  But we need to make a
checkin to undo the checkins that were made in this area so far.  That's
what I'm requesting adt approval for.

This was already checked in (i.e., backed out) on the trunk.  I need driver's 
and adt approval to do the same on the branch.
ADT approval for attachment 92059 [details] [diff] [review], to undo previous patch on the branch. leaving
as adt1 and nsbeta1, so Nav triage can look at this for the next release.
Keywords: nsbeta1-mozilla1.2, nsbeta1
patches have been backed out on trunk and branch.  Now we are back to ground 
zero with this bug.
Now that I've had time to analyzer this carefully, here is what was happening
with the collective set of patches in this bug report.  Below are the changes
that were made when setting a cookie and when getting a cookie.

SETTING COOKIES:

   case 1. no path attribute supplied:

      new behavior:
         remove file name from requesting URL but leave the slash,
            and use the result as the path attribute of the cookie being set
      original behavior:
         we were removing the slash as well

      Note:
        This will stop the attack described in this bug report:
           path of requesting URL when setting cookie = /mypath/myfile
           path of requesting URL when getting cookie = /mypathattack/...


   case 2. path attribute supplied:
   
      new behavior:
         check that requesting URL is allowed to set cookie for that path
      previous behavior:
         no such test was being made

      Note:
         This was the concern in bug 155083

   Note:
     If site is dumb enough to give a path attribute of "/mypath"
       instead of either "/mypath/" or "mypath/myfile", then attack is again
       possible
     But that's the site's problem, not ours (my.netscape.com makes this error)


GETTING COOKIES:

   previous behavior:

      check that stored path attribute is prefix of path of requesting URL

   new behavior:

      ignore everything past last slash in stored path attribute
         Note:
            This caused the problem in my.netscape.com
               stored path attrib = /mypath
               path of requesting URL = /myotherpath/myfile
               cookie was being sent out, and it confused the site
            It could be argued that this is a site problem because it set
               the path attribute to /mypath instead of /mypath/

      ignore everything past last slash in path of requesting URL
         this really wasn't necessary, but it was harmless

      check that stored path attribute is prefix of path of requesting URL


BOTTOM LINE:

1. Change under "setting cookies, case 1" (don't remove slash)
   is necessary to stop current attack.  This part of patch must be retained.
2. Change under "setting cookies, case 2" (test if site can set cookie)
   is necessary to fix bug 155083.  This part of patch should be retained.
3. Change under "Getting Cookies (ignore everything past last slash)
   is unnecessary since filename was already removed when setting the cookie.
   And it was this change that broke the my.netscape.com site (even though the
   my.netscape.com site had a contributing error).  So this part of the patch
   must not be retained.

Will post a revised patch that does 1, 2, but not 3.
Attachment #90009 - Attachment is obsolete: true
Attachment #91697 - Attachment is obsolete: true
Attachment #91926 - Attachment is obsolete: true
Attachment #91927 - Attachment is obsolete: true
Attachment #92057 - Attachment is obsolete: true
Attachment #92059 - Attachment is obsolete: true
Steve - does your comment in comment 60 indicate that you are going to fix the
security bug in the original description for the branch?
It's too late for branch by now.  So unless there is a strong outcry to the 
contrary, I am thinking of the trunk only.
Comment on attachment 92175 [details] [diff] [review]
simple patch that doesn't break my.netscape.com

> +    if(PL_strncmp(cur_path.get(), path_from_header, PL_strlen(path_from_header))) {

This line looks inefficient - any way to do this without having to do two
passes over the string?

This patch looks OK to me, although we probably want to test it against some
top sites to check for regressions. Let's land on the trunk and get some
testing ASAP.
r=mstoltz.
Attachment #92175 - Flags: review+
Comment on attachment 92175 [details] [diff] [review]
simple patch that doesn't break my.netscape.com

Eventually we need to clean up the string usage in cookies. This chunk is
already half in the new world, this is how it might look fully
converted: (note, not tested)

  if(path_from_header.IsEmpty()) {
    /* Strip down everything after the last slash to get the path,
     * ignoring slashes in the query string part.
     * Need to strip any query string first
     */
    PRInt32 queryPos = cur_path.FindChar('?');
    if (queryPos != kNotFound) {
      cur_path.Truncate(queryPos);
    }
    PRInt32 lastslash = cur_path.RFindChar('/');
    if (lastslash != kNotFound) {
      cur_path.Truncate(lastslash+1);
    }
    path_from_header = cur_path;
  } else {
    /* specified path must be a subset of the real path */
    if(cur_path.Compare(path_from_header,PR_FALSE,path_from_header.Length())
      return;
  }

But I guess that's a chore for another day.

sr=dveditz
Attachment #92175 - Flags: superreview+
Let's do something real safe here.  The patch presented really fixes two bugs -- 
this one and bug 155083.  I originally combined the patches for convenience but 
now realize that that was a big mistake.

The part of the patch for bug 155083 is to bring our path-checking into 
conformity with the spec.  And that will break some (hopefully unimportant) 
websites who were violating the spec.

The part of the patch for this bug will stop the attack described.  It will NOT 
break any websites.

So I'm subdividing the patch and posting separate ones for each bug report.  
That way we can decide if we want to simply stop the attack but not worry about 
RFC 2109 conformity at this time.
Attachment #92175 - Attachment is obsolete: true
Comment on attachment 92584 [details] [diff] [review]
subdivided patch -- stops attacks, does not fix bug 155083

carrying reviews forward
r=mstoltz, sr=dveditz
Attachment #92584 - Flags: superreview+
Attachment #92584 - Flags: review+
Nav triage team: nsbeta1+/adt1
Keywords: nsbeta1nsbeta1+
Seems as the new fix is less risky, and only works to plug the security holes
[adt1 RTM]. Let's try and get this one on the branch.
Keywords: adt1.0.1
Whiteboard: [adt1] [Not for MachV] → [adt1 RTM] [ETA 07/31]
Keywords: mozilla1.0.1
Whiteboard: [adt1 RTM] [ETA 07/31] → [adt1 RTM] [ETA 08/01]
lowering impact to adt2 per the ADT.
Whiteboard: [adt1 RTM] [ETA 08/01] → [adt2 RTM] [ETA 08/05]
ok, I've been testing this new patch using 2 experimental builds (winNT4 and
linux rh6) for a few days now and it appears to fix the problem.  Also, I
checked all of the bugs I could find that the original patch affected and I
don't see any regressions.   Looks good to me.
let's get this checked into the trunk and see how it goes.
Keywords: adt1.0.1
Checked in on trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
verified 08/12/02 trunk builds, winNT4, linux rh6, mac osX.  Not seeing any
regressions.
Status: RESOLVED → VERIFIED
Whiteboard: [adt2 RTM] [ETA 08/05] → [adt2 RTM] [ETA 08/05][verified-trunk]
It was suggested, this bug might have introduced the regression reported in bug
164260.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: