Closed Bug 109982 Opened 24 years ago Closed 24 years ago

file: URLs have access to cookies of any domain

Categories

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

x86
Windows NT
defect

Tracking

()

VERIFIED WONTFIX
mozilla1.0

People

(Reporter: c, Assigned: morse)

References

Details

Attachments

(4 obsolete files)

To reproduce: 1. Create an HTML file with this content: <script>alert(document.cookie)</script> 2. Store it somewhere on your computer 3. Access it with file://bugzilla.mozilla.org/<path to file> Actual result: You see your Bugzilla cookies. Expected result: Error message since the URI isn't valid (see bug 70871). At least no Bugzilla cookies. Note: I wasn't able to make an actual exploit out of this because of other Mozilla security mechanisms, but in principle this is similar to the recent IE cookie exploit (about://host/...). 2001-11-13-03-trunk, Win NT.
Depends on: 70871
The only potential exploit I can think of would involve placing a malicious HTML or JS file on the user's disk and running it. We have mechanisms in place to prevent an untrusted page running local files, but as an extra measure of protection, maybe we ought to prevent this behavior. This issue came up before WRT ftp requests being able to read cookies set by the HTTP server with the same hostname, and in that case it was decided there was little risk. The risk may be slightly greater here, so let's consider fixing it. Steve, what do you think? Is this worth fixing, as an added level of protection against a possible cookie-stealing attack?
Assignee: mstoltz → morse
Component: Security: General → Cookies
QA Contact: bsharma → tever
That's really interesting! Definitely should be fixed ASAP in case someone does come up with an exploit based on it.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.7
Target Milestone: mozilla0.9.7 → mozilla0.9.8
For clarity, let me illustrate the URL. Suppose in step 2 you store the file in c:\bug.html. Then the URL you visit is file://bugzilla.mozilla.org/c:/bug.html Now This is really a networking problem rather than a cookie problem. I say that for two reasons: 1. The URL is invalid. Yet the network layer actually fetched the file and displayed it. This is not what happens on nav4. 2. The URL is presented to the cookie module. The cookie module in turn calls on the networking module to extract the host part from the URL. That is done with the following call result = ioService->ExtractUrlPart(address, nsIIOService::url_Host | nsIIOService::url_Port, &start, &end, &host); Rather than returning an error code (since the URL is invalid), that call is returning a host of "bugzilla.mozilla.org". Therefore the cookie module is justified in returning the bugzilla cookies to such a host. If either or both of the above problems are corrected in the networking layer, this cookie problem would not occur. Therefore reassigning to networking. FWIW, I definitely consider this to be a serious problem that must get fixed. Even though there are no obvious attacks based on it, I can easily see how a person like Guninski could come up with one relatively quickly. Therefore nominating for nsbeta1
Component: Cookies → Networking
Keywords: nsbeta1
And now I'm really reassigning to the networking people.
Assignee: morse → neeti
Status: ASSIGNED → NEW
QA Contact: tever → benc
darin
Assignee: neeti → darin
ok, so if we fix bug 70871 this one would probably be fixed as well. as a simple fix we could check that the "host" matches the name of the user's system. doing so might be a little bit tricky, but we should be able to hack up something suitable. is this critical for 0.9.8?
Status: NEW → ASSIGNED
It's only critical if someone comes up with an attack based on it. Currently there are no known ones. But I wouldn't want to press our luck. I would image it's too late for 0.9.8 by now, but I would hate to see this go beyond 0.9.9. IMO, it absolutely must be done by 1.0.
adding mozilla1.0 keyword
Keywords: mozilla1.0
-> 0.9.9
Severity: normal → major
Priority: -- → P2
Target Milestone: mozilla0.9.8 → mozilla0.9.9
As with several other bugs, I have to ask: "Why would a file URL ever have cookies, isn't this an http-only convention?" If we do fix this as #6, then we need to update the milestone and priority in that bug...
> "Why would a file URL ever have cookies, isn't this an http-only convention?" No. Both http urls and file urls can set a cookie via javascript and also via metatags. The only method unavailable to file urls is via set-cookie headers.
I would like to (belatedly) restrict access to this bug, as it could potentially be exploited for malicious intent. Everyone currently CC'd on the bug should still be able to access it. If anyone objects, just uncheck the "Security-Sensitive Bug" box.
Group: security?
I don't get it. Maybe I am too dense or have not had enough coffee. Why do we care if a file url can read any cookie? heck, a file href can load my passwd file, or look at devices, ect. I think that we would have bigger problems if there was a file: exploit than just cookies.
Doug: "belt-and-suspenders" perhaps? For most other potential exploits involving inserting Web content into the file: domain, we have two levels of protection, for example, the CheckLoadURI restrictions and the profile directory salting provide two levels of protection against stealing profile information. Preventing file: URLs from reading cookies from other domains will provide an extra level of protection for cookies. Do you think we'll be breaking any legitimate functionality by fixing this bug?
A file href can load your password file, but it can't see the file that it loads. A file url that reads a cookie can transmit the information in that cookie to an external website. For example, suppose a website has tricked you into storing a file on your local disk and then bringing up that file in your browser. That file could then read cookies and transmit the information in those cookies to any website it chooses.
no no. I think this should be fixed.
increasing the priority on this one.
Severity: major → critical
Priority: P2 → P1
so i spoke to bbaetz a bit about this one and he suggested an alternative solution. why can't the cookie database compare the URL scheme to the URL scheme that set the cookie in the first place? in other words, it seems like the cookie service should not give out cookies to a file:// URL if the cookies were set by a http:// URL. is this possible? or would it mean a change to the cookies file format? (as i understand it, we want to avoid changing the cookies file format for backwards compatibility, right?) otherwise, my solution is to modify nsStandardURL to discard the auth section of the given URL if the nsStandardURL is configured to use the NO_AUTH url parser. the drawback of such a patch is that it would limit our ability to do something meaningful with the hostname of a file:// URL in the future. (limit == would have to re-solve this bug.)
Attached patch nsStandardURL patch (obsolete) — Splinter Review
cc'ing bbaetz
Keywords: patch
> why can't the cookie database compare the URL scheme to the URL > scheme that set the cookie in the first place? in other words, it seems > like the cookie service should not give out cookies to a file:// URL if the > cookies were set by a http:// URL. The protocol is currently not stored in the cookie file, so I don't have the information to do such a comparison.
morse: and i am correct in saying that modifying the cookie database to store the protocol scheme would be a bad idea? could you explain why? i just want to make sure that i understand all of the issues.
The database has several fields per cookie. I believe that the browser (both nav 4 and mozilla) and read as many fields as they know about and ignore the rest. So in theory it might be possible to add extra fields at the end without breaking backwards compatibility, although I've never verified this. And the protocol could be such an added field. However there might be third-party add-ons that read the cookie file. Such add-ons could be more dependent on the actual file structure and not work if we add new fields. Furthermore, consider the user who has collected cookies in the past. These cookies do not have protocol fields and there is no way that we can backfill them. So any test that we add now involving protocol matching would not work for pre-existing cookies.
Hmm. Well, maybe for any protocol except http/https, change the hostname to be: <scheme>:<hostname>. That would keep backwards compatability, and since : is illegal in a hostname (except for an ipv6 address. Maybe use ; instead?), won't break anything. How do we handle ports, or do we ignore the port when sending cookies?
We ignore the ports when sending cookies
>We ignore the ports when sending cookies That doesn't strike me as secure either - if I have shell access on a machine, then I can start up apache on port 9876, and get any cookie on the same machine. What about the 'secure' flag? How is that one handled in the data file? Also, are cookies set from non-http able to be persistant, or are they just session cookies?
>We ignore the ports when sending cookies I just checked the code and looks like I was wrong. We do include the port as part of the hostname. We strip it out when determining the domain and that's what I was remembering. > What about the 'secure' flag? How is that one handled in the data file? It's there as one of the fields. > Also, are cookies set from non-http able to be persistant, Yes.
I might be really off track here, esp since I've only read the relevant rfcs up to 1738, but it seems to me we have had file URLs done wrong in several places (in other bugs we contemplated sending a ?query to a file URL or allow 30x redirects to a file URL or discussions of form sumbissions to file URLs). Isn't a cookie a server construction? Isn't this impossible with flat files? Isn't this an HTTP transportable-only conception? It seems to me that this is the root of the security problem, we are contemplating the existince of cookie hyperspace. If we distinguish port numbers (which we should), and secure vs. not, then we ought to distinguish schemes too right? I have the same head-scratching feeling when I think of ftp and cookies. (someone please send me the relevant RFC...)
i'd really like to see the cookies database solve this problem internally by storing the protocol scheme along with each database entry. this seems like the best way to solve this problem in all cases. solving it only for file: or no-auth URLs is not really a complete solution. protocols are pluggable and suppose in the future someone adds a protocol handler that can steal http cookies... we'd have this bug all over again. i say let's not worry about potentially breaking 3rd party software (unless we have a real example of where this would cause problems), and just fix this in the cookies database. morse: what do you say? doesn't this seem like the only way to get this 100% correct?
Any fix that involves changing the format of the cookies file wouldn't solve the problem for pre-existing cookies.
how about assuming that no-scheme == http ??
That might work.
-> morse to impl the proposed solution
Assignee: darin → morse
Status: ASSIGNED → NEW
Component: Networking → Cookies
Status: NEW → ASSIGNED
Not going to make it in 0.9.9. Pushing out to 1.0
Target Milestone: mozilla0.9.9 → mozilla1.0
I have an even easier solution than trying to force the cookie module to keep track of the protocol scheme. Instead, the cookie module will consider the host part of a file URL to be "", regardless of what is actually in the URL. In other words, when presented with the URL of file://bugzilla.mozilla.org/c:/bug.html the cookie module will consider the host to be "". I'll try to come up with a patch that implements this.
Right now, file URL ignores the hostname field of the URI. I am hoping someday to fix this, so I think that you should give some thought about making sure that your fix won't be un-broken when file URL works correctly in the future.
Attachment #66254 - Attachment is obsolete: true
benc: this patch shouldn't be affected when you make your future change. But since you are concerned, how would you like to do the review on this patch? benc, alecf, please review. Thank.s
Comment on attachment 71488 [details] [diff] [review] cookie manager to ignore host field on file: URL's yikes! You should be using nsIURI::schemeIs(), not going through the ioservice. Also, cookie_GetHost() should take a char** not a char&* to return the host. - this is standard convention that I've mentioned in other reviews. (Even so, you should always be using nsXPIDLCString instead of manually freeing strings) finally, wouldn't it make sense to make cookie_GetHost() return an error if it encounters a file:// URL? then you wouldn't have to strdup("") sr=alecf
Attachment #71488 - Flags: needs-work+
> You should be using nsIURI::schemeIs(), not going through the ioservice. But that requires having an nsIURI object whereas all I had was an ascii spec for the url. So I added code that builds the nsIURI from the ascii spec. > Also, cookie_GetHost() should take a char** not a char&* to return the host. - done > Even so, you should always be using nsXPIDLCString instead of manually > freeing strings Well that's a moot point, now that I'm using the nsIURL instead. > finally, wouldn't it make sense to make cookie_GetHost() return an error if it > encounters a file:// URL? then you wouldn't have to strdup("") I'll still need to strdup because I have to have a string that is going to be stored as the host part in the cookie table.
Attached patch addresses issues in comment 39 (obsolete) — Splinter Review
Attachment #71488 - Attachment is obsolete: true
whoa! this is an extremely expensive way of testing the scheme... i don't think it's what alec had in mind. nsIIOService::extractScheme is what you want. fwiw: it's also not the correct way to get a nsIURI from a URI string. nsIURI's are created using nsIIOService::newURI (or nsIProtocolHandler::newURI). anything else is incorrect.
darin, I agree with you -- I also thought that using nsIURI::schemeIs() was the hard way to do things in this case. But it is clearly what alec asked for. Should I change it back to using nsInsIIOService as I originally had?
morse: i don't think alec realized that you don't already have a nsIURI pointer. btw: how is it that you have a URI string only? does the caller of this method perhaps have a nsIURI pointer? if so, then this method should probably be modified to pass the nsIURI pointer instead of the raw URL spec (provided of course that it is not a frozen public API).
No, the caller doesn't have the nsIURI either.
Let me explain my previous reply in more detail. All the cookie module gets is the set-cookie header that was passed to it from the networking layer. And that is an ascii string containing the textual URL. That is why neither the new GetHost routine nor any of its callers have an nsIURI object.
morse: makes sense.
Comment on attachment 71622 [details] [diff] [review] addresses issues in comment 39 ack! give me the benefit of the doubt that I'm reviewing a bunch of patches and may have made a mistake on this one. :) I thought there was a nsIURI handy. there's no need to create a STANDARDURL here - my bad. as for strdup("") - it sounds like that's an architectural flaw that we should fix at some point - remember that a heap has some accounting, so a one-byte allocation (i.e. "") aligns to 4 bytes, and then has probably 4 bytes of accounting data, effectively taking up 8 bytes of memory. So, if we go back to the previous patch, and use nsXPIDLCString instead, then you have sr=alecf - sorry about the nsIURI screw-up
Attachment #71622 - Flags: needs-work+
Attachment #71622 - Attachment is obsolete: true
Comment on attachment 71724 [details] [diff] [review] addresses issues raised in comment 48 r=sgehani (Samir from Morse's machine)
Attachment #71724 - Flags: superreview+
Attachment #71724 - Flags: review+
Comment on attachment 71724 [details] [diff] [review] addresses issues raised in comment 48 a=blizzard on behalf of drivers for 0.9.9
Attachment #71724 - Flags: approval+
> A file href can load your password file, but it can't see the file that it > loads. A file url that reads a cookie can transmit the information in that > cookie to an external website. Uh, a file href sure can see the file it loads. Slap this in a local HTML file: <iframe id="foo" src="/etc/passwd"></iframe> <input type="button" onClick='alert(document.getElementById("foo").contentDocument.documentElement.childNodes[1].firstChild.firstChild.data);'> and then click the button. Instead of alert()ing, we could send that data to a web server. I don't see why we're trying to protect cookies here, when the attacker could just send the cookies.txt to a remote host anyway.
Keywords: mozilla0.9.9+
shaver: how is the attacker going to get the path to the cookies file? if we're working under the assumption that the local file in question doesn't know the path to the profile directory (remember, it's salted) then this makes us safer. (Correct me if I'm wrong, but local files can't actually search the disk, it can only load known paths/urls, right? XPConnect is certainly disabled, so I can't think of another way to get a directory listing)
Load the root directory in the iframe. Look at the generated HTML via the DOM to find the names of the contents of the directory. Navigate based on those URLs, resetting the iframe's src as you need to inspect new directories or files. This is not hard; people did things like it based on 3.x attacks that managed to gain access to the DOM of a file: URL. If you insist, I'll write a script that tells you the path to your cookie file, starting the search in /home to save time. (Or Windows, you could probably load the registry's backing file and look in there for the base profile dir.) Local content wins, so let's not add complexity to an important code path to try and keep it from winning via one relatively insignificant means. Maybe we should have a warning in save-as telling people that they should be careful about accessing files they save to their local disk from untrusted sources?
Comment on attachment 71724 [details] [diff] [review] addresses issues raised in comment 48 Removing has-approval for reconsideration by drivers.
Attachment #71724 - Flags: approval+
> Local content wins, so let's not add complexity to an important code path to > try and keep it from winning via one relatively insignificant means. I'm not sure I understand this comment. What "important code path" are you referring to? The host field in the file: url is currently unused. And if it is used in the future, it is supposed to refer to the current host, which will never change for a given machine. So as far as the cookie module is contained, that field can be considered as blank. Maybe there are ways to subvert this as you've pointed out. But it certainly doesn't hurt to block this means of attack and it is relatively inexpensive to do so. Even if it's not a security flaw, it's certainly in violation of the cookie spec. A url (file: or otherwise) should never be sent cookies that another site has set.
> so as far as the cookie module is contained, Sorry, I meant to say "concerned", not "contained".
By "important code path", I meant "code path that is followed to get and set cookies", which I think we do quite a bit. (BTW, what's that PL_strcmp doing there?) I don't know what cookie spec you're talking about that addresses document.cookie, but maybe you can point me to an URL. Don't we already do cross-protocol cookie sharing (ftp://ftp.mozilla.org/foo.html can see the cookies set by http://ftp.mozilla.org/foo.html)? _That_ seems like a bigger security hole to me. (What does IE do with file://somehost/local/file.html, I wonder?) Anyway, sure, I relent. If you think this is such an important security bug that we need to keep it closed -- which hurts the quality of review, generally -- and slam it into 0.9.9, who am I to argue? I personally don't think it's a bug at all, and certainly not one worth spinning up the security-fire-drill apparatus, but I'll reinstate blizzard's approval and just try to ignore this whole discussion in the future.
Comment on attachment 71724 [details] [diff] [review] addresses issues raised in comment 48 Reinstating a=blizzard. I wash my hands of this.
Attachment #71724 - Flags: approval+
After talking with people and reading the bug this doesn't seem to add any security and it's not worth adding the code.
Attachment #71724 - Flags: approval+
Resolving as won't fix based on above comments.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → WONTFIX
Group: security?
-> cookie qa to Tom. Even if we can't think of a security hole right now, I think we should try to fix this at some future point. How about this attack: I'm on a LAN, users turn off checkload.URI because they need to access file URLs (there are a couple dozen dupes to that bug), and I publish a document so the file URL steals HTTP cookies? That's possible right? Cookies should be specific to hostname, port and scheme. These are all distinct namespaces and should not be confused. When cookies were just in HTTP, it made sense just to store the just the hostname and path, but whenever javascript added cookie support, that changed everything. HTTP used to not support virutal hosting, and look where that got them... "well, nobody at the time thought you would ever serve more than one domain's web site off one socket!" The person controlling an FTP and HTTP server for the same hostname might be completely different individuals or organizations. They might be behind a socket-routing firewall or a Local Director. They might be completely different systems. The weak association you can make between two services running on the same hostname+port or ip+port will continue to decouple over time. There is always the possibility of new protocol schemes as well. This problem about breaking other people that read our cookies file is not a good justification, if you ask me. If people are reading our cookie files, we need to create a file format specification and follow it. If they are reading our formats by reverse engineering, then they knew the risks when they built products on top of it.
QA Contact: benc → tever
Comment on attachment 71724 [details] [diff] [review] addresses issues raised in comment 48 I know the ioservice stuff isn't going to work anymore
Attachment #71724 - Attachment is obsolete: true
Superseded by more general bug 165334
VERIFIED: wontfix. If we modify checkloadURI, then we need to start caring about this. I've filed bug 209964 about file URL's producing cookies w/o a host value (file:///cookie.js).
Status: RESOLVED → VERIFIED
QA Contact: tever → cookieqa
Blocks: 235170
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: