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)
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.
Comment 1•24 years ago
|
||
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
| Assignee | ||
Comment 2•24 years ago
|
||
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
| Assignee | ||
Updated•24 years ago
|
Target Milestone: mozilla0.9.7 → mozilla0.9.8
| Assignee | ||
Comment 3•24 years ago
|
||
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
| Assignee | ||
Comment 4•24 years ago
|
||
And now I'm really reassigning to the networking people.
Assignee: morse → neeti
Status: ASSIGNED → NEW
QA Contact: tever → benc
Comment 6•24 years ago
|
||
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
| Assignee | ||
Comment 7•24 years ago
|
||
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.
Comment 9•24 years ago
|
||
-> 0.9.9
Severity: normal → major
Priority: -- → P2
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Comment 10•24 years ago
|
||
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...
| Assignee | ||
Comment 11•24 years ago
|
||
> "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.
Comment 12•24 years ago
|
||
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?
Comment 13•24 years ago
|
||
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.
Comment 14•24 years ago
|
||
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?
| Assignee | ||
Comment 15•24 years ago
|
||
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.
Comment 16•24 years ago
|
||
no no. I think this should be fixed.
Comment 17•24 years ago
|
||
increasing the priority on this one.
Severity: major → critical
Priority: P2 → P1
Comment 18•24 years ago
|
||
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.)
Comment 19•24 years ago
|
||
Comment 20•24 years ago
|
||
cc'ing bbaetz
| Assignee | ||
Comment 21•24 years ago
|
||
> 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.
Comment 22•24 years ago
|
||
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.
| Assignee | ||
Comment 23•24 years ago
|
||
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.
Comment 24•24 years ago
|
||
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?
| Assignee | ||
Comment 25•24 years ago
|
||
We ignore the ports when sending cookies
Comment 26•24 years ago
|
||
>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?
| Assignee | ||
Comment 27•24 years ago
|
||
>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.
Comment 28•24 years ago
|
||
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...)
Comment 29•24 years ago
|
||
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?
| Assignee | ||
Comment 30•24 years ago
|
||
Any fix that involves changing the format of the cookies file wouldn't solve the
problem for pre-existing cookies.
Comment 31•24 years ago
|
||
how about assuming that no-scheme == http ??
| Assignee | ||
Comment 32•24 years ago
|
||
That might work.
Comment 33•24 years ago
|
||
-> morse to impl the proposed solution
Assignee: darin → morse
Status: ASSIGNED → NEW
Component: Networking → Cookies
| Assignee | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
| Assignee | ||
Comment 34•24 years ago
|
||
Not going to make it in 0.9.9. Pushing out to 1.0
Target Milestone: mozilla0.9.9 → mozilla1.0
| Assignee | ||
Comment 35•24 years ago
|
||
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.
Comment 36•24 years ago
|
||
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.
| Assignee | ||
Comment 37•24 years ago
|
||
Attachment #66254 -
Attachment is obsolete: true
| Assignee | ||
Comment 38•24 years ago
|
||
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 39•24 years ago
|
||
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+
| Assignee | ||
Comment 40•24 years ago
|
||
> 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.
| Assignee | ||
Comment 41•24 years ago
|
||
| Assignee | ||
Updated•24 years ago
|
Attachment #71488 -
Attachment is obsolete: true
Comment 42•24 years ago
|
||
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.
| Assignee | ||
Comment 43•24 years ago
|
||
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?
Comment 44•24 years ago
|
||
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).
| Assignee | ||
Comment 45•24 years ago
|
||
No, the caller doesn't have the nsIURI either.
| Assignee | ||
Comment 46•24 years ago
|
||
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.
Comment 47•24 years ago
|
||
morse: makes sense.
Comment 48•24 years ago
|
||
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+
| Assignee | ||
Comment 49•24 years ago
|
||
| Assignee | ||
Updated•24 years ago
|
Attachment #71622 -
Attachment is obsolete: true
| Assignee | ||
Comment 50•24 years ago
|
||
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 51•24 years ago
|
||
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+
Updated•24 years ago
|
Keywords: mozilla0.9.9+
Comment 52•24 years ago
|
||
> 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+
Comment 53•24 years ago
|
||
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)
Comment 54•24 years ago
|
||
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 55•24 years ago
|
||
Comment on attachment 71724 [details] [diff] [review]
addresses issues raised in comment 48
Removing has-approval for reconsideration by drivers.
Attachment #71724 -
Flags: approval+
| Assignee | ||
Comment 56•24 years ago
|
||
> 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.
| Assignee | ||
Comment 57•24 years ago
|
||
> so as far as the cookie module is contained,
Sorry, I meant to say "concerned", not "contained".
Comment 58•24 years ago
|
||
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 59•24 years ago
|
||
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+
Comment 60•24 years ago
|
||
After talking with people and reading the bug this doesn't seem to add any
security and it's not worth adding the code.
Updated•24 years ago
|
Attachment #71724 -
Flags: approval+
| Assignee | ||
Comment 61•24 years ago
|
||
Resolving as won't fix based on above comments.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → WONTFIX
Updated•23 years ago
|
Group: security?
Comment 62•23 years ago
|
||
-> 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 63•23 years ago
|
||
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
Comment 64•23 years ago
|
||
Superseded by more general bug 165334
Comment 65•22 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•