Closed Bug 155083 Opened 22 years ago Closed 22 years ago

path test is not made when site sets a cookie

Categories

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

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

Tracking

()

Status:
RESOLVED WONTFIX
Milestone:
mozilla1.2alpha

People

(Reporter: morse, Assigned: morse)

References

Details

Attachments

(2 files, 2 obsolete files)

Check for rfc 2109 conformity
1.05 KB, patch
morse
: review+
morse
: superreview+
Details | Diff | Splinter Review
Backing out with comment 31 taken into account
1.42 KB, patch
dveditz
: review+
roc
: superreview+
Details | Diff | Splinter Review
RFC 2109, section 4.3.2 states: To prevent possible security or privacy violations, a user agent rejects a cookie (shall not store its information) if any of the following is true: * The value for the Path attribute is not a prefix of the request-URI. This test is not being made in the mozilla codebase. The following html demonstrates this. Unless this html is put in the /a path or some subpath of it, the cookie should not be set. And indeed it does not get set in N4.x. But it does get set in mozilla. <html> <body> <script> document.cookie = "x=y;path=/a"; </script> Cookie Setting Test </body> </html> Marking this a security-sensitive for now, because the RFC says that this test is supposed to prevent possible security violations. However I can think of no attack based on this, other than a DOS attack. Note that the test is indeed being made when reading cookies -- it's just when setting cookies that the test was overlooked. So it is not possible for some other site on the same host but in a different path to steal cookies.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.1alpha
I'm attaching a patch that fixes this problem. However I am not suggesting this patch get checked in, because I just discovered a bigger problem in this area, and that one does indeed lead to a security attack. I'm about to open a separate bug report on that problem.
Serious problem that I alluded to in comment 1 is described in bug 155114.
Plussing to get noticed (although nsbeta1+ is taken to mean Buffy now). Setting bug dependency: although this could be fixed independently there's a combined patch in bug 155114
Depends on: 155114
Keywords: nsbeta1+
This patch breaks at least one website -- see bug 155768. But that website was setting a cookie for a path that it did not control, so we are justified in breaking it. That bug has been turned over to evangelism. (Note: this patch did not go into the branch, so Mozilla 1.0.1 and Netscape 7 will not break that website. But any future releases will (until that website is evangelized).
Marking fixed since the part of the patch for bug 155114 that fixes this has been checked into the trunk. That part of the patch will not be checked into the branch, as discussed in comment 5.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Another website that this patch breaks is shown in bug 157330. But in that case the site was setting a cookie that has a path attribute containing the full path including the file name itself.
The site in bug 157330 isn't broken after all. It's just that the patch that was applied (contained in bug 155114) was not quite right. The patch has since been corrected, and now the site in bug 157330 is working.
But here's a website that the patch definitely breaks because this site was violating RFC2109. It's the site mentioned in bug 156626. That's been turned over to evangelism.
re-opening since patch in bug# 155114 has been backed out, tested on trunk using supplied test case and not working, 07/23/02 winNT4 build
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Patch in bug 155114 will no longer be a composite patch for the two bug reports -- that was creating confusion. So I've posted a new patch for bug 155114 that simply stops the attack described in that report but does not fix this bug. Will attach the part of the patch that fixed this bug into this bug report. It should be noted that this will break some sites -- notably those who are in violation of the RFC2109 spec regarding setting of the path attribute. If there are only a few of them and they are unimportant, then maybe that should be an evangelism issue. On the other hand, if we discover that there are numerous sites and/or important sites that are broken, then we should not take this patch. The only way to find out is to land this on the trunk (after it is again open to general checkins) and see what developes.
Status: REOPENED → ASSIGNED
Attachment #89736 - Attachment is obsolete: true
Comment on attachment 92585 [details] [diff] [review] Check for rfc 2109 conformity bringing reviews forward from bug 155114 r=mstoltz sr=dveditz
Attachment #92585 - Flags: superreview+
Attachment #92585 - Flags: review+
One site that this patch will break is in bug 155768.
If the first comment is correct and this patch will provide a check that N4 already has, why will this fix break the site in bug 155768 as it works fine using N4?
My apologies, the first comment is not correct. This bug is present in N4.x, in mozilla, and in IE. Here is the test case: 1. Go to http://home.pacbell.net/spmorse/setcookie.htm. Code is shown below <html> <body> <script> document.cookie = "x=y;path=/spmorse/xxx"; document.cookie = "c=d"; alert(document.cookie); </script> Cookie Setting Test </body> </html> 2. Go to http://home.pacbell.net/spmorse/xxx/showcookie.htm. Code is shown below <html> <body> <script> alert(document.cookie); </script> Cookie Reading Test </body> </html> Note that SetCookie will set a cookie will set two cookies, only one of which it can read back. However both cookies will be readable by ShowCookie. This is incorrect. If it sets a cookie, it certainly should be able to read it. The error is that it never should have been allowed to set the cookie for the xxx path.
internal site where testcase stored http://10.169.103.61/webpub/testcases/cookiepath.html
Target Milestone: mozilla1.1alpha → mozilla1.2alpha
After consulting with several people, we have decided to check this patch in. It will break some websites -- namely those that are not in conformance with the cookie spec. But we want to find out which websites they are, and then decide what to do about it. If it's just a handful of unimportant sites, then we can address that with evangelism. If there are many sites involved, or if there are important sites involved, then we might decide to back this patch out and mark this bug as won't fix. In any case, we need to get it in now in order to get testing so that we can make a decision. Therefore checking this patch in now.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Is there a difference between cookies set by the server and cookies set in JavaScript? Shouldn't we let the server set cookies to whatever path it will and let the server take care of not using wrong paths? I understand that in client-side js we should guard it, but shouldn't we let the server decide what to do? This might just fix a problem for Finnish people (bug 155768, they aren't using js to set the cookie, right?).
Whether the cookie is set with a COOKIE: header in the http response, or by a document.cookie call in javascript, it is the server that is initiating it in either case. So I don't understand the distinction that you are making.
The difference is that the user of the server might write the javascript whereas the server knows what it sets itself. As I understand the security issue here (correct me if I'm wrong), the problem is that without this path check (for example) a geocities user might set a cookie (using javascript) for a path he doesn't own, affecting another geocities user. But if the server geocities pages are running on decides to set a cookie, the browser should accept it. The server and all its paths are controlled by the server itself (or its administrator). Why prevent it from setting the cookie to another path it owns? Just correct me if I have something wrong here (actually this thought was first expressed by a colleague of mine as we all suffer from this) and I'll try to swallow it..
What about the geocities site administrator who writes the home page for geocities and decides to use javascript cookies rather than http cookies because he finds it easier? It would be strange for him to be able to violate the path restriction in one case and not in the other. Also what about cookies set with metatags? A metatag should behave exacty like a real http header would behave. And metatags are written by the geocities user. And what about plugins that set cookies?
It's already strange. Other browsers don't have a problem with his approach. Meta tags and any other possible ways to set cookies could be path-checked while allowing http responses to set the cookies. Now I must admit I'm not sure if this would help in the case of bug 155768 as I don't know how it sets the cookie, but if it is set in http response, it would. The problem having this as an evangelism issue is that I find it unlikely they will do anything about it unless some _major_ browser would have the same problem. I'd love to be proven wrong though.
Let me rephrase my thoughts. Can you really say that a http response is tied to the path of the request? It's a response from the _server_, and does not necessarily have any relation to the real content the client requested (for example error conditions). I think the server should be able to set a cookie regardless the path the client requested. Ok, my thoughts violate the RFC... I think the RFC is stupid :) But anyway, I understand if you disagree with me.
In order to determine whether we want to keep this fix and evangelize the offending sites, or drop this fix, we need to know which sites this fix actually breaks. So I'll start posting a list of them in this bug report. The ones we know about so far are: bug 155768: http://solo.nordea.fi bug 165681: https://webmail.bluewin.ch/sslindex_e.html
See bug 156725 -- this patch broke citibank too.
Attached patch Backing out (obsolete) — — Splinter Review
Reopening since fix is going to be backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 101271 [details] [diff] [review] Backing out >-#if 1 >+#if 0 > /* > * The following test is part of the RFC2109 spec. Loosely speaking, it says that a site > * cannot set a cookie for a path that it is not on. See bug 155083. However this patch Please restructure this so the comment is in the block to which it applies. It would be a lot better as if () { } #if 0 else { // comment } #endif Or maybe if () { #if 0 } else { // comment #endif }
Attachment #101271 - Attachment is obsolete: true
Comment on attachment 101343 [details] [diff] [review] Backing out with comment 31 taken into account sr=roc+moz
Attachment #101343 - Flags: superreview+
I think this should be checked in ASAP and then this bug should be opened. We're heading for a WONTFIX and IE does exactly the same thing as we do, plus the potential downside is small.
Bug 172097 is possibly related.
Comment on attachment 101343 [details] [diff] [review] Backing out with comment 31 taken into account thanks Steve, r=dveditz
Attachment #101343 - Flags: review+
I'm removing the security cloak from the bug... this is not really a security exploit, it's a known (and relied-on) behavior of IE. Doesn't match the spec, but the spec doesn't match the web and we'll have to follow suit.
Group: security?
Patch backed out
Status: REOPENED → ASSIGNED
We won't fix this one.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → WONTFIX
I have a suggestion for a possible workaround that would stay within the spirit of the RFC spec while still allowing web sites to work: If it turns out that a cookie's path is invalid, modify the path internally to save it to what would be a valid path. That way, any writes/reads of cookies to/from invalid paths would be internally redirected to the "correct" location.
I don't believe that would work--the sites that are breaking are in fact relying on the path being correct when they try to read the cookie later. We *do* correctly enforce paths when /getting/ a cookie.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: