Last Comment Bug 155114 - Stealing cookies based on path attribute
: Stealing cookies based on path attribute
Status: VERIFIED FIXED
[adt2 RTM] [ETA 08/05][verified-trunk]
:
Product: Core
Classification: Components
Component: Networking: Cookies (show other bugs)
: Trunk
: x86 Windows NT
: P1 critical (vote)
: mozilla1.1alpha
Assigned To: Stephen P. Morse
: Tom Everingham
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks: 143047 155083
  Show dependency treegraph
 
Reported: 2002-06-30 15:23 PDT by Stephen P. Morse
Modified: 2003-11-24 07:28 PST (History)
21 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for this bug as well as related bug 155083 (2.41 KB, patch)
2002-06-30 17:14 PDT, Stephen P. Morse
security-bugs: review+
Details | Diff | Splinter Review
address items in comment #5 (3.26 KB, patch)
2002-07-02 10:37 PDT, Stephen P. Morse
no flags Details | Diff | Splinter Review
Remove extra characters that accidentally crept into file (2.84 KB, patch)
2002-07-02 10:40 PDT, Stephen P. Morse
jst: superreview+
Details | Diff | Splinter Review
address issue in comment 8 (2.89 KB, patch)
2002-07-02 16:33 PDT, Stephen P. Morse
security-bugs: review+
morse: superreview+
chofmann: approval+
Details | Diff | Splinter Review
Fix sites that were broken by checkin of previous patch (1.87 KB, patch)
2002-07-17 12:43 PDT, Stephen P. Morse
no flags Details | Diff | Splinter Review
more efficient fix (1.92 KB, patch)
2002-07-17 13:51 PDT, Stephen P. Morse
security-bugs: review+
chofmann: superreview+
chofmann: approval+
Details | Diff | Splinter Review
additional patch for trunk (590 bytes, patch)
2002-07-19 01:11 PDT, Stephen P. Morse
security-bugs: review+
dveditz: superreview+
asa: approval+
Details | Diff | Splinter Review
additional patch for branch. (579 bytes, patch)
2002-07-19 01:12 PDT, Stephen P. Morse
security-bugs: review+
dveditz: superreview+
Details | Diff | Splinter Review
patch to undo previous patches on the branch (1.64 KB, patch)
2002-07-19 17:46 PDT, Stephen P. Morse
no flags Details | Diff | Splinter Review
patch to undo previous patches on the trunk (2.52 KB, patch)
2002-07-19 18:09 PDT, Stephen P. Morse
asa: approval+
Details | Diff | Splinter Review
patch to undo previous patch on the branch (1.43 KB, patch)
2002-07-19 18:14 PDT, Stephen P. Morse
no flags Details | Diff | Splinter Review
patch to undo previous patch on the branch (1.70 KB, patch)
2002-07-19 18:34 PDT, Stephen P. Morse
no flags Details | Diff | Splinter Review
simple patch that doesn't break my.netscape.com (1.14 KB, patch)
2002-07-21 17:08 PDT, Stephen P. Morse
security-bugs: review+
dveditz: superreview+
Details | Diff | Splinter Review
subdivided patch -- stops attacks, does not fix bug 155083 (518 bytes, patch)
2002-07-24 10:40 PDT, Stephen P. Morse
morse: review+
morse: superreview+
Details | Diff | Splinter Review

Description Stephen P. Morse 2002-06-30 15:23:33 PDT
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).
Comment 1 Stephen P. Morse 2002-06-30 17:14:32 PDT
Created attachment 89741 [details] [diff] [review]
Patch for this bug as well as related bug 155083
Comment 2 Stephen P. Morse 2002-06-30 17:21:23 PDT
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>
Comment 3 Stephen P. Morse 2002-06-30 17:58:16 PDT
IE is not susceptable to this attack!
Comment 4 Mitchell Stoltz (not reading bugmail) 2002-07-01 13:03:50 PDT
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.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2002-07-01 16:52:39 PDT
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?
Comment 6 Stephen P. Morse 2002-07-02 10:37:22 PDT
Created attachment 89946 [details] [diff] [review]
address items in comment #5
Comment 7 Stephen P. Morse 2002-07-02 10:40:39 PDT
Created attachment 89947 [details] [diff] [review]
Remove extra characters that accidentally crept into file
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2002-07-02 15:37:29 PDT
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.
Comment 9 Stephen P. Morse 2002-07-02 16:32:49 PDT
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.
Comment 10 Stephen P. Morse 2002-07-02 16:33:51 PDT
Created attachment 90009 [details] [diff] [review]
address issue in comment 8
Comment 11 Stephen P. Morse 2002-07-02 16:35:12 PDT
Comment on attachment 90009 [details] [diff] [review]
address issue in comment 8

sr=jst per comment 8
Comment 12 Mitchell Stoltz (not reading bugmail) 2002-07-02 16:56:01 PDT
Comment on attachment 90009 [details] [diff] [review]
address issue in comment 8

r=mstoltz.
Comment 13 Stephen P. Morse 2002-07-02 17:02:45 PDT
Fix checked in on trunk
Comment 14 chris hofmann 2002-07-02 17:10:59 PDT
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?
Comment 15 Stephen P. Morse 2002-07-04 17:01:18 PDT
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.
Comment 16 Stephen P. Morse 2002-07-05 14:14:33 PDT
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.
Comment 17 Tom Everingham 2002-07-09 15:14:49 PDT
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
Comment 18 Jaime Rodriguez, Jr. 2002-07-09 15:28:04 PDT
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".
Comment 19 Johnny Stenback (:jst, jst@mozilla.com) 2002-07-09 15:36:17 PDT
What about the issue mentioned in comment #15?
Comment 20 Stephen P. Morse 2002-07-09 15:43:53 PDT
regarding comment 15, adt agreed to go with the partial fix on the branch.  That 
is what I will be checking in
Comment 21 Stephen P. Morse 2002-07-09 16:05:43 PDT
Fixed for reading cookies has now been checked in on branch.  Fix for setting of 
cookies will not go on the branch.
Comment 22 Kai Engert (:kaie) (on vacation) 2002-07-16 13:09:29 PDT
The check in done to the branch breaks online banking with Deutsche Bank.

I have filed bug 157787.
Comment 23 Stephen P. Morse 2002-07-17 12:36:36 PDT
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.

Comment 24 Stephen P. Morse 2002-07-17 12:42:17 PDT
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.
Comment 25 Stephen P. Morse 2002-07-17 12:43:42 PDT
Created attachment 91684 [details] [diff] [review]
Fix sites that were broken by checkin of previous patch
Comment 26 Stephen P. Morse 2002-07-17 13:51:46 PDT
Created attachment 91697 [details] [diff] [review]
more efficient fix
Comment 27 Stephen P. Morse 2002-07-17 14:22:59 PDT
Comment on attachment 91697 [details] [diff] [review]
more efficient fix

sr=jag
Comment 28 Mitchell Stoltz (not reading bugmail) 2002-07-17 14:28:16 PDT
Comment on attachment 91697 [details] [diff] [review]
more efficient fix

Looks good. r=mstoltz.
Comment 29 chris hofmann 2002-07-17 14:44:05 PDT
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.
Comment 30 scottputterman 2002-07-17 16:37:52 PDT
adding mozilla1.0.1+ based on chofmann's comments.  Please check this in today.
Comment 31 Stephen P. Morse 2002-07-17 16:59:33 PDT
Checked in on branch and trunk.
Comment 32 Jaime Rodriguez, Jr. 2002-07-17 19:38:29 PDT
tever: can you pls verify this as fixed in tomorrow's build?
Comment 33 Tom Everingham 2002-07-18 18:40:05 PDT
I am still seeing this on the 07/18 builds - branch only.   WinNT4 and mac osX
at least.  Re-opening.
Comment 34 Tom Everingham 2002-07-18 18:46:46 PDT
oops, actually I didn't need to re-open this.  This is working on todays trunk.
Comment 35 Stephen P. Morse 2002-07-18 19:02:37 PDT
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.
Comment 36 Asa Dotzler [:asa] 2002-07-18 20:20:13 PDT
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.
Comment 37 Asa Dotzler [:asa] 2002-07-18 20:33:44 PDT
Cookies set by jhavascript now include the filename in the path instead of just
the directory. This is what's breaking blogger.com. 
Comment 38 Mitchell Stoltz (not reading bugmail) 2002-07-18 20:44:17 PDT
Steve, is that what we intended to do? The cookie path shouldn't include the
filename, should it?
Comment 39 Stephen P. Morse 2002-07-19 01:09:48 PDT
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.
Comment 40 Stephen P. Morse 2002-07-19 01:11:09 PDT
Created attachment 91926 [details] [diff] [review]
additional patch for trunk
Comment 41 Stephen P. Morse 2002-07-19 01:12:25 PDT
Created attachment 91927 [details] [diff] [review]
additional patch for branch.
Comment 42 Stephen P. Morse 2002-07-19 02:22:54 PDT
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.
Comment 43 Jaime Rodriguez, Jr. 2002-07-19 11:22:35 PDT
removing adt1.0.1+, as there is a new patch that needs reviews.
Comment 44 Daniel Veditz [:dveditz] 2002-07-19 16:01:58 PDT
Comment on attachment 91926 [details] [diff] [review]
additional patch for trunk

sr=dveditz

Storing the trailing slash appears to be what IE does
Comment 45 Daniel Veditz [:dveditz] 2002-07-19 16:02:07 PDT
Comment on attachment 91927 [details] [diff] [review]
additional patch for branch.

sr=dveditz
Comment 46 Mitchell Stoltz (not reading bugmail) 2002-07-19 16:06:10 PDT
Comment on attachment 91926 [details] [diff] [review]
additional patch for trunk

r=mstoltz.
Comment 47 Mitchell Stoltz (not reading bugmail) 2002-07-19 16:06:29 PDT
Comment on attachment 91927 [details] [diff] [review]
additional patch for branch.

r=mstoltz.
Comment 48 Asa Dotzler [:asa] 2002-07-19 17:07:24 PDT
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.
Comment 49 Stephen P. Morse 2002-07-19 17:43:31 PDT
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.
Comment 50 Stephen P. Morse 2002-07-19 17:46:01 PDT
Created attachment 92052 [details] [diff] [review]
patch to undo previous patches on the branch
Comment 51 Stephen P. Morse 2002-07-19 18:09:12 PDT
Created attachment 92057 [details] [diff] [review]
patch to undo previous patches on the trunk
Comment 52 Stephen P. Morse 2002-07-19 18:14:53 PDT
Created attachment 92058 [details] [diff] [review]
patch to undo previous patch on the branch
Comment 53 Stephen P. Morse 2002-07-19 18:34:05 PDT
Created attachment 92059 [details] [diff] [review]
patch to undo previous patch on the branch
Comment 54 Asa Dotzler [:asa] 2002-07-19 18:47:11 PDT
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
Comment 55 Jaime Rodriguez, Jr. 2002-07-20 12:23:33 PDT
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. 
Comment 56 Stephen P. Morse 2002-07-20 12:38:00 PDT
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.
Comment 57 Jaime Rodriguez, Jr. 2002-07-20 13:00:06 PDT
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.
Comment 58 Stephen P. Morse 2002-07-20 14:50:29 PDT
patches have been backed out on trunk and branch.  Now we are back to ground 
zero with this bug.
Comment 59 Stephen P. Morse 2002-07-21 14:45:54 PDT
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.
Comment 60 Stephen P. Morse 2002-07-21 17:08:33 PDT
Created attachment 92175 [details] [diff] [review]
simple patch that doesn't break my.netscape.com
Comment 61 lchiang 2002-07-22 13:23:33 PDT
Steve - does your comment in comment 60 indicate that you are going to fix the
security bug in the original description for the branch?
Comment 62 Stephen P. Morse 2002-07-22 14:20:12 PDT
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 63 Mitchell Stoltz (not reading bugmail) 2002-07-22 17:15:25 PDT
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.
Comment 64 Daniel Veditz [:dveditz] 2002-07-22 19:00:52 PDT
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
Comment 65 Stephen P. Morse 2002-07-24 10:37:37 PDT
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.
Comment 66 Stephen P. Morse 2002-07-24 10:40:28 PDT
Created attachment 92584 [details] [diff] [review]
subdivided patch -- stops attacks, does not fix bug 155083
Comment 67 Stephen P. Morse 2002-07-24 10:41:51 PDT
Comment on attachment 92584 [details] [diff] [review]
subdivided patch -- stops attacks, does not fix bug 155083

carrying reviews forward
r=mstoltz, sr=dveditz
Comment 68 Samir Gehani 2002-07-25 14:52:04 PDT
Nav triage team: nsbeta1+/adt1
Comment 69 Jaime Rodriguez, Jr. 2002-07-29 13:22:04 PDT
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.
Comment 70 Jaime Rodriguez, Jr. 2002-07-30 12:10:29 PDT
lowering impact to adt2 per the ADT.
Comment 71 Tom Everingham 2002-07-30 14:51:57 PDT
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.
Comment 72 Jaime Rodriguez, Jr. 2002-08-06 17:04:54 PDT
let's get this checked into the trunk and see how it goes.
Comment 73 Stephen P. Morse 2002-08-11 23:34:32 PDT
Checked in on trunk.
Comment 74 Tom Everingham 2002-08-12 19:23:03 PDT
verified 08/12/02 trunk builds, winNT4, linux rh6, mac osX.  Not seeing any
regressions.
Comment 75 Kai Engert (:kaie) (on vacation) 2002-10-10 09:44:49 PDT
It was suggested, this bug might have introduced the regression reported in bug
164260.
Comment 76 Mitchell Stoltz (not reading bugmail) 2003-06-19 15:55:59 PDT
adding benc@netscape.com to CC list
Comment 77 Christopher Aillon (sabbatical, not receiving bugmail) 2003-11-24 07:28:07 PST
Opening.

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