1173214 - Use of the moz-icon protocol can confirm the existence of local files.

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Cody Crews]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150525141253

Steps to reproduce:

I added an image to a document using the moz-icon protocol and specified a local URI as the file for which to generate an icon.  When specifying the contentType parameter as one opposite of the file one is testing the existence of it becomes possible that an attacker could confirm that the local file does indeed exist.


Actual results:

Thanks to the contentType parameter being included the icon displayed will not match the contentType provided if the file does in fact exist.  I spent more time than I should have trying to find a way to render this image as part of a canvas without tainting it.  Right now I have an implementation setup which uses load times of the image instead to attempt to confirm the existence.  I'm currently putting the pieces together for two testcases one which will just show this off in a visual way(confirming the problem) and a much more complex testcase using load timing of the image which would make a brute force attack here possible.  I will upload at least the visual testcase shortly.


Expected results:

The moz-icon internal protocol should not allow a local file to be specified using the file:// protocol.

Comment 1

[Cody Crews]

Attachment: fileLocator-mozicon-testcase-new.html

This attachment shows the issue off visually.  Please note that I'm loading the image in an iframe as this is part of the much more complex version that I have been working on that I just kind of copied and pasted.  Also let me know if the windows version has any issues as I have only confirmed this issue in linux because I have no boxes running a windows OS setup right now.  I'll be keeping on eye on my email and such in case any other information on this is needed.  Thanks guys and I hope you all take this one as seriously as I do because locating files is the beginning of the end when someone has just a foothold on a remote machine.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So the basic problem is that the icon will be based on the file type of the file:// URI if it exists, else on the passed-in MIME type, yes?

But how are you then figuring out which icon you're ending up with, exactly?

In any case, I would be quite happy to make the moz-icon version with a URI not linkable from untrusted content, assuming we need to keep it at all...

Comment 3

[Cody Crews]

I was trying originally to render it as part of a canvas without producing the taint state, but that was proving pointless.  I did find that it is possible to use a mutation observer which observes changes of the images src attribute and then time the loading of the image using window.performance with the mutation observer setting a performance mark and then the onload event of the image setting another and measuring the difference.  This does work but there is an occasional anomaly in the timing which I was trying to devise a way to sift through.

I think making moz-icon not linkable from untrusted content should take care of it.  I know some chrome level css sheets use it but beyond that it may be just as easy to get rid of it altogether.

Comment 4

[Justin Dolske [:Dolske]]

(In reply to Not doing reviews right now from comment #2)

> In any case, I would be quite happy to make the moz-icon version with a URI
> not linkable from untrusted content, assuming we need to keep it at all...

We need it in chrome for a few things, where we want to show the same icon the user's OS shell would show. EG file icons in downloads (browser/components/downloads/DownloadsViewUI.jsm), and app icons for the apppicker (browser/components/preferences/in-content/applications.js).

I can't think of any reason to expose this to web content, although it's possible we're using it somewhere in our own untrusted content UI (but that would surprise me a bit).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Our file:// viewer uses it, I bet.

Comment 6

[:Gijs (he/him)]

(In reply to Not doing reviews right now from comment #5)
> Our file:// viewer uses it, I bet.

Doesn't use it with actual paths, though, just the "moz-icon://.exe" variant.

I wonder if we could just create a new scheme for that variant and make it implement nsINestedURI, so that the security checks would "just work" by checking the inner protocols... Alternatively, make the different scheme completely chrome-only.

Comment 7

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #6)
> I wonder if we could just create a new scheme for that variant

by which I mean, the one with full inner URIs which is scary and used here.

> and make it
> implement nsINestedURI, so that the security checks would "just work" by
> checking the inner protocols... Alternatively, make the different scheme
> completely chrome-only.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Doesn't use it with actual paths, though, just the "moz-icon://.exe" variant.

Right.  Hence me proposing what I propose in comment 2 at all.  ;)

> I wonder if we could just create a new scheme for that variant 

Yes, that seems like the sane thing to do to me.  Just have different schemes for the thing we want linkable from web content and the thing we don't want linkable.

Note that we can make this decision completely inside the icon protocol handler.  We already have just such a setup for about: URIs.

Comment 9

[Cody Crews]

I believe the anomalies that I mentioned before may be able to be filtered out if one keeps a record of mozPaintCount from before each image load and filters out any results that experience a paint count increase of more than 1.  I'll be testing that in code some point soon, then just comes the brute forcing logic :)

Can we go ahead and get this rated?

Comment 10

[Andrew McCreight [:mccr8]]

Detection of local files is sec-moderate per https://wiki.mozilla.org/Security_Severity_Ratings

Comment 11

[:Gijs (he/him)]

Attachment: Strawman patch

This is very much a strawman because
a) it has some xxx comments because I get muddle-headed over nsCOMPtr assignments and addrefs and I'm too lazy/busy (cross as applicable) to look up details right now, but it seems to not crash, so...
b) I will be gone from Friday until August so if it is acceptable someone will need to shepherd landing it for 40 (assuming uplift etc.)
c) I actually ended up not doing a scheme thing but based on the latest comments re: about, just made a thing that inherits (so nothing that relies on scheme checks will break, I think) and then exposes the inner URI where it can, and made NewURI return the right thing

This is a strawman because if someone creates a URI that doesn't use the protocol handler (is that possible? I don't know?) then maybe this doesn't work. That or a hundred other things I might be missing. But it was fun to write, I learned a thing or two, and it seems to at least break the exploit when used from non-file origin.

Going to explicitly not assign to self so someone else can carry this over the finish line if need be.

Comment 12

[Cody Crews]

Once I get things stabilized with what I have going on, I'll pick this up and try to correct anything needed after the review.  I'm sure I could learn a lot just by picking pieces :-)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8634718 [details] [diff] [review]
Strawman patch

The nsCOMPtr bits are fine.

> if someone creates a URI that doesn't use the protocol handler (is that
> possible? I don't know?)

Mostly no, short of people screwing up, but there is one case that matters here: if a URI is sent over IPC it'll end up using the IPCSerializable bits, and since those are getting forwarded it will just create the inner URI.

I think what we should do is the following:

1)  Land this patch as-is on trunk and backport to branches as needed.  Should be OK since e10s is not shipping on branches.

2)  File a followup for a trunk fix that plays nicer with IPC.  I think the right answer there is to actually fail out of nsIconProtocolHandler::NewChannel2 if the loading principal wouldn't normally be able to link to the mIconURL of the URI involved.  I'm not suggesting this for branches because I'm not sure we're reliably using NewChannel2 there, but I think we _are_ on trunk.  Christoph, can you confirm?  I guess as a fallback we can fail opening if there is no loadinfo and there's an mIconURL, as long as we're consistent about passing in a loadinfo in the cases we care about...

Comment 14

[Boris Zbarsky [:bzbarsky]]

In fact, Christoph, might you have time to do that followup fix?

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #13)
> right answer there is to actually fail out of
> nsIconProtocolHandler::NewChannel2 if the loading principal wouldn't
> normally be able to link to the mIconURL of the URI involved.  I'm not
> suggesting this for branches because I'm not sure we're reliably using
> NewChannel2 there, but I think we _are_ on trunk.  Christoph, can you
> confirm?

Unfortunately I can't, because I don't understand what you mean exactly. Everywhere within Gecko we should use ::NewChannel2 by now and all channels created within Gecko should have a loadInfo attached. Only channels not having a loadInfo are those created by addons.

> I guess as a fallback we can fail opening if there is no loadinfo
> and there's an mIconURL, as long as we're consistent about passing in a
> loadinfo in the cases we care about...

That should be reliable because, as I mentioned, every channel should have a loadInfo by now.

> In fact, Christoph, might you have time to do that followup fix?
I am happy to fix, but at the moment I don't understand what we have to fix. Maybe you could propose a solution and I am happy to take on the work.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't quite understand what's going on here.

But wouldn't it fit more with existing architecture if we made CheckLoadURI fail if a content page tries to load a moz-icon URL which contained a filename?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> if we made CheckLoadURI fail if a content page tries to load a moz-icon URL which
> contained a filename

Yes, it would.  That's basically what Gijs' patch does.  It's pretty hacky, though, and a cleaner solution is not obvious; the fact that the moz-icon ends up having some (but not all) of the security properties of the file it's using is pretty weird and can't really be expressed so far in terms the security manager understands.

Christoph, the information I was looking for is whether it's now the case that we use NewChannel2 consistently inside Gecko.  Sounds like we do, so that's good.

> Maybe you could propose a solution

That's comment 13 item 2, no?

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #17)
> > Maybe you could propose a solution
> 
> That's comment 13 item 2, no?

Jonas, you are probably in a better position to evaluate different approaches on how to fix that. I am happy to implement a fix, but as I mentioned in Comment 15, I don't fully understand how we could fix the issue at the moment. Jonas, can you provide some guidance?

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK.  I went ahead and landed Gijs' patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/2f97906c22f7

We still need that followup.  One option for it would be to use the infrastructure Bobby just added in bug 1186152; that's probably cleaner than my proposal in comment 13.

Comment 20

[Boris Zbarsky [:bzbarsky]]

And https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d75433812f to fix a test in what I hope was the right way....

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

this and the followup backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9fa7dd954cd6 for windows reftest failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=12216232&repo=mozilla-inbound

Comment 22

[Boris Zbarsky [:bzbarsky]]

Lovely.  That test is relying on being able to link to moz-icon things from untrusted content.  I'm not going to get a chance to sort through what's going on there this week, and next week Gijs is back, so I get back onto his plate.  :(

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f97906c22f7

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If there's still questions here, I'm happy to defer them to bz and bholley.

Comment 25

[:Gijs (he/him)]

Err, this got backed out and the backout landed on trunk -> reopening. Will look at the test as soon as I finish going through other bugmail and urgent stuff now that I'm back.

Comment 26

[:Gijs (he/him)]

Seems the reftest would need to be converted to a chrome mochitest and use http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/WindowSnapshot.js to ensure it's working, considering what it's trying to do. I'll try to get to that today or tomorrow.

Comment 27

[:Gijs (he/him)]

Attachment: Patch v1.1

Seth, can you doublecheck my test move from reftest to mochitest-chrome? I verified that it works for the icon in the tree, and that the screenshot compare breaks if I use e.g. "self" (the HTML file) for the file the icon points to.

I'm happy to rubberstamp bz's test change to the downloads test, and I didn't change the substance of the patch itself, which had r+, so if the reftest move is OK this should be good to land...

Comment 28

[:Gijs (he/him)]

(FWIW, I don't know why this test is disabled on win8, but I kept that fact in-place -- though I wrote the patch on my win8.1 machine and it wfm...)

Comment 29

[Cody Crews]

I see the work here is still progressing.  I'm around when I can be for right now guys.  I'll be back fully active hopefully within the next week or two.  I'm really really not liking NC lately.  My point was if there's anything I can do once I'm back I'll get to it.  I assume by then though that this will be handled so I'll just start focusing elsewhere.

Comment 30

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8643031 [details] [diff] [review]
Patch v1.1

Review of attachment 8643031 [details] [diff] [review]:
-----------------------------------------------------------------

I think bz is in a better position to review this patch conceptually than I am, so I'll avoid that.

Aside from that, the patch looks good, but there are some nits and minor issues:

::: image/decoders/icon/nsIconProtocolHandler.cpp
@@ +76,5 @@
> +  nsCOMPtr<nsIURL> iconURL;
> +  uri->GetIconURL(getter_AddRefs(iconURL));
> +  if (iconURL) {
> +    uri = new nsNestedMozIconURI();
> +    if (!uri) return NS_ERROR_OUT_OF_MEMORY;

Just delete this line; operator new is not fallible in Gecko by default. This code will never run.

@@ +78,5 @@
> +  if (iconURL) {
> +    uri = new nsNestedMozIconURI();
> +    if (!uri) return NS_ERROR_OUT_OF_MEMORY;
> +    rv = uri->SetSpec(aSpec);
> +    if (NS_FAILED(rv)) return rv;

Nit: I realize the surrounding code does this, but let's not add more code that contravenes the style guide. Please put the body of the |if| on a separate line and surround it with braces.

::: image/decoders/icon/nsIconURI.cpp
@@ +14,5 @@
>  #include "nsIURL.h"
>  #include "prprf.h"
>  #include "plstr.h"
>  #include <stdlib.h>
> +#include "nsNetUtil.h"

Nit: please try to keep the #include's semi-alphabetical.

@@ +657,5 @@
> +NS_IMETHODIMP
> +nsNestedMozIconURI::GetInnerURI(nsIURI** aURI)
> +{
> +  *aURI = mIconURL;
> +  NS_IF_ADDREF(*aURI);

Please avoid NS_IF_ADDREF. Instead:

nsCOMPtr<nsIURI> iconURL = mIconURL;
iconURL.forget(aURI);

::: image/decoders/icon/nsIconURI.h
@@ +40,5 @@
>    int32_t mIconState;  // -1 if not specified, otherwise index into
>                         // kStateStrings
>  };
>  
> +class nsNestedMozIconURI : public nsMozIconURI

Nit: You should mark nsNestedMozIconURI final.

Also, it'd be nice to add a comment that explains the purpose of this class.

@@ +50,5 @@
> +  NS_FORWARD_NSIIPCSERIALIZABLEURI(nsMozIconURI::)
> +
> +  NS_DECL_NSINESTEDURI
> +
> +  // nsNestedMozIconURI

Is this comment really adding anything?

::: image/test/mochitest/test_bug415761.html
@@ +25,5 @@
> +
> +var dest = self.parent;
> +dest.append("\u263a.ico");
> +if (dest.exists())
> +  dest.remove(false);

Nit: you should have braces around the body of this |if|.

@@ +42,5 @@
> +  var otherCanvas = document.createElement("canvas");
> +  otherCanvas.setAttribute("height", 32);
> +  otherCanvas.setAttribute("width", 32);
> +  document.body.appendChild(canvas);
> +  document.body.appendChild(otherCanvas);

I don't understand why you're appending these elements to the body, or why you're appending |canvas| twice.

In general, please add comments to this test.

Comment 31

[Seth Fowler [:seth] [:s2h]]

Heh, I guess this patch already had r+, but what can I say, you bring in a different reviewer and you get a different review. =)

Comment 32

[Daniel Veditz [:dveditz]]

Cody: I don't see where you've shown you can detect the state from remote web content (as opposed to the victim themselves visually seeing it). Given that I'm lowering the severity, but if you do find a way (or if I'm just an idiot and missed it) please let me know.

Comment 33

[Cody Crews]

It's fine, I had a way involving using an independent iframe and directly loading an image in it so that the window has an independent paint count.  By doing that you could load the image you expect to get a few times and time the loading using an overly complicated method.  Then you could change to a file you expect to find and load that many times factoring out any time the paint count increased more than 1.  By doing so you could get a rough idea at least as to if the file existed.  I still had some polishing to do with it and considering the state of how things have been lately I would think that not uploading something that could make this actually be used maliciously is for the best.

Yeah, I don't even have the rough mock up that I did have and as long as this is getting fixed we're all good.  That's what it's about more at the end of the day than anything else.  Thanks guys.

Comment 34

[:Gijs (he/him)]

Attachment: Patch v2

Apologies for the delay. I think this addresses all of your comments, Seth. Note that I had to QI mIconURL because it's an nsIURL not an nsIURI...

Comment 35

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8650499 [details] [diff] [review]
Patch v2

Review of attachment 8650499 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: image/test/mochitest/test_bug415761.html
@@ +66,5 @@
> +  refCanvas.setAttribute("height", 32);
> +  refCanvas.setAttribute("width", 32);
> +  refCanvas.getContext('2d').drawImage(refImage, 0, 0, 32, 32);
> +
> +  // Render the icon with 

Nit: whitespace, and probably missing text from the end of the comment.

Comment 36

[:Gijs (he/him)]

Landed on fx-team:

https://hg.mozilla.org/integration/fx-team/rev/d21ee41c1d04

Caused b2g xpcshell orange, (hopefully) fixed by:

https://hg.mozilla.org/integration/fx-team/rev/8c08d166507a

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d21ee41c1d04

Comment 38

[:Gijs (he/him)]

This breaks the icons used for local apps in feed view. Yet Another Reason to move the subscribe UI into chrome. :-\

Filed bug 1203981.