Closed Bug 155114 Opened 23 years ago Closed 23 years ago

Stealing cookies based on path attribute

Categories

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

Product:
Core
Component:
Networking: Cookies
Platform:
x86
Windows NT
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.1alpha

People

(Reporter: morse, Assigned: morse)

References

Details

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

Attachments

(1 file, 13 obsolete files)

subdivided patch -- stops attacks, does not fix bug 155083
518 bytes, patch
morse
: review+
morse
: superreview+
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: 23 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: nsbeta1 → adt1.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.
Keywords: mozilla1.0.1+ → fixed1.0.1
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: 23 years ago → 23 years ago
Keywords: mozilla1.0.1+ → fixed1.0.1
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: 23 years ago → 23 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: nsbeta1 → nsbeta1+
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: 23 years ago → 23 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

Creator:
Created:
Updated:
Size: