109982 - file: URLs have access to cookies of any domain

Status: VERIFIED WONTFIX

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

Description

[Andreas M. "Clarence" Schneider]

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

[Mitchell Stoltz (not reading bugmail)]

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?

Comment 2

[Stephen P. Morse]

That's really interesting!  Definitely should be fixed ASAP in case someone does 
come up with an exploit based on it.

Comment 3

[Stephen P. Morse]

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

Comment 4

[Stephen P. Morse]

And now I'm really reassigning to the networking people.

Comment 5

[neeti]

darin

Comment 6

[Darin Fisher]

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?

Comment 7

[Stephen P. Morse]

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 8

[Darin Fisher]

adding mozilla1.0 keyword

Comment 9

[Darin Fisher]

-> 0.9.9

Comment 10

[[e-mail]]

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...

Comment 11

[Stephen P. Morse]

> "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

[Mitchell Stoltz (not reading bugmail)]

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.

Comment 13

[Doug Turner (:dougt)]

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

[Mitchell Stoltz (not reading bugmail)]

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?

Comment 15

[Stephen P. Morse]

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

[Doug Turner (:dougt)]

no no.  I think this should be fixed.

Comment 17

[Darin Fisher]

increasing the priority on this one.

Comment 18

[Darin Fisher]

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

[Darin Fisher]

Attachment: nsStandardURL patch

Comment 20

[Darin Fisher]

cc'ing bbaetz

Comment 21

[Stephen P. Morse]

> 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

[Darin Fisher]

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.

Comment 23

[Stephen P. Morse]

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

[Bradley Baetz (:bbaetz)]

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?

Comment 25

[Stephen P. Morse]

We ignore the ports when sending cookies

Comment 26

[Bradley Baetz (:bbaetz)]

>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?

Comment 27

[Stephen P. Morse]

>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

[benc]

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

[Darin Fisher]

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?

Comment 30

[Stephen P. Morse]

Any fix that involves changing the format of the cookies file wouldn't solve the 
problem for pre-existing cookies.

Comment 31

[Darin Fisher]

how about assuming that no-scheme == http ??

Comment 32

[Stephen P. Morse]

That might work.

Comment 33

[Darin Fisher]

-> morse to impl the proposed solution

Comment 34

[Stephen P. Morse]

Not going to make it in 0.9.9.  Pushing out to 1.0

Comment 35

[Stephen P. Morse]

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

[benc]

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.

Comment 37

[Stephen P. Morse]

Attachment: cookie manager to ignore host field on file: URL's

Comment 38

[Stephen P. Morse]

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

[Alec Flett]

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

Comment 40

[Stephen P. Morse]

> 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.

Comment 41

[Stephen P. Morse]

Attachment: addresses issues in comment 39

Comment 42

[Darin Fisher]

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.

Comment 43

[Stephen P. Morse]

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

[Darin Fisher]

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).

Comment 45

[Stephen P. Morse]

No, the caller doesn't have the nsIURI either.

Comment 46

[Stephen P. Morse]

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

[Darin Fisher]

morse: makes sense.

Comment 48

[Alec Flett]

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

Comment 49

[Stephen P. Morse]

Attachment: addresses issues raised in comment 48

Comment 50

[Stephen P. Morse]

Comment on attachment 71724 [details] [diff] [review]
addresses issues raised in comment 48

r=sgehani (Samir from Morse's machine)

Comment 51

[Christopher Blizzard (:blizzard)]

Comment on attachment 71724 [details] [diff] [review]
addresses issues raised in comment 48

a=blizzard on behalf of drivers for 0.9.9

Comment 52

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

> 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.

Comment 53

[Alec Flett]

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

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 71724 [details] [diff] [review]
addresses issues raised in comment 48

Removing has-approval for reconsideration by drivers.

Comment 56

[Stephen P. Morse]

> 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.

Comment 57

[Stephen P. Morse]

> so as far as the cookie module is contained,

Sorry, I meant to say "concerned", not "contained".

Comment 58

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 71724 [details] [diff] [review]
addresses issues raised in comment 48

Reinstating a=blizzard.

I wash my hands of this.

Comment 60

[Christopher Blizzard (:blizzard)]

After talking with people and reading the bug this doesn't seem to add any
security and it's not worth adding the code.

Comment 61

[Stephen P. Morse]

Resolving as won't fix based on above comments.

Comment 62

[benc]

-> 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.

Comment 63

[Daniel Veditz [:dveditz]]

Comment on attachment 71724 [details] [diff] [review]
addresses issues raised in comment 48

I know the ioservice stuff isn't going to work anymore

Comment 64

[Daniel Veditz [:dveditz]]

Superseded by more general bug 165334

Comment 65

[benc]

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).