Closed Bug 1424261 (CVE-2018-5140) Opened 3 years ago Closed 3 years ago

moz-icon images can be linked and read from the web


(Firefox :: Untriaged, defect)

58 Branch
Not set



Firefox 59
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed


(Reporter: qab, Assigned: Gijs)



(Keywords: csectype-disclosure, sec-low, Whiteboard: [post-critsmash-triage][adv-main59+])


(4 files)

Attached file svger.html
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

Tested on Nightly 59.0a1 (2017-12-07) (64-bit).

View attached PoC.

Actual results:

It seems that we can still link moz-icon urls if we include the current pages URL within it like so: moz-icon:http://localhost/poc.html?size=64&contentType=application/xml 

I then noticed that we can read the image through canvas.toDataURL. So we are now able to determine which application is used to open certain file types, as well as potential fingerprinting of user within private browsing (?).

Expected results:

moz-icon should be blocked from linking on the web. Also I think its worth checking why canvas.toDataURL is working here.
Doesn't work on macOS with e10s because of sandboxing (see bug 1419811).

Prior to bug 1222924, you could just do this the easy way by using moz-icon://.xml?size=16 , so it seems this being broken on 58 beta is kind of expected - but I think being able to read it doesn't work on 57/58 with just moz-icon://.xml?size=16 .

Either way, it's unexpected that it's still possible to do this on nightly. On the other hand, I'm a bit confused/worried that it seems in/after bug 1173214, we didn't restrict moz-icon to only be have nested file: URIs.
I haven't managed to reproduce on Linux so far...  Also sandboxing?
It seems like the URL is treated completely same origin, we can even frame it and execute js in it.

<iframe src="moz-icon:http://localhost/evil.ani" onload="window[0].eval('alert(location)')"></iframe>

Perhaps there is something checking to see if a given url contains {uri-scheme}:http://{same-origin-domain}.. and automatically gives it same-origin status?
There are at least 4 ways of fixing this:


should be checking aTargetURI instead of aTargetBaseURI .


and any other entrypoints into moz-icon URI constructors should only allow 'file' inner URIs.

- the contentType query parameter shouldn't do anything when there's a nested URI (not 100% sure if this is a red herring - I'd expect "<extension>" might also get you an icon for that extension, without the contentType param)


I'll see what I can do about getting patches for these. At least the moz-icon inner http URI one should be easy to fix. I don't know how much fallout there would be from fixing the first issue. From what I can tell quickly, moz-icon is the only UI_RESOURCE URI type that is ever nested, so it should be fine, but perhaps I'm missing something. Like, perhaps that'll break file: directory listing icons again (sigh...).

bug 1171853 is going to be more work to address, and isn't generally an issue because if you've got a nested URI that points to, it's mostly sensible that it's same-origin with "actual" . But for that to be the case, the data that a channel to that URI provides would actually want to come from, which clearly isn't the case here.

It would help if there was a sec rating on this bug so I had an idea about how cautious I need to be about pushing to try. While I'm annoyed that I missed this, I don't know how severe "know what applications the user has installed" is, in the general case. Sec-moderate, from a quick look at the severity ratings page?
Assignee: nobody → gijskruitbosch+bugs
Ever confirmed: true
Attached file dump-icon.txt
I tried to create a blob URL within the moz-icon URL and apparently I got a possible stack corruption tab crash:
Possible Stack Corruption starting at xul!mozilla_dump_image+0x0000000000928669 (Hash=0xa1943e15.0xf278a43e)

Also I attached the dump log where possible stack corruption was seen (using 'exploitable' module)

To repro:

<iframe src="moz-icon:http://localhost/evil.ani" onload="window[0].eval(`URL.createObjectURL(new Blob([''],{type:'text/html'}))`)"></iframe>

Doesn't work within view-source: page so it seems unique to moz-icon. This may be a sign that this context will result in unexpected behavior. Will test more.
New crash report using similar poc as above (except I loaded a moz-icon uri with a lot of junk in it)

Notice some of the frames are hex with no name, isn't that bad?:

Should I open a new bug for that? or deal with it all here?
(In reply to Abdulrahman Alqabandi from comment #6)
> Should I open a new bug for that? or deal with it all here?

File a new bug. Looks like issues with blob serialization/deserialization.
(In reply to :Gijs from comment #8)
> (In reply to Abdulrahman Alqabandi from comment #6)
> > Should I open a new bug for that? or deal with it all here?
> File a new bug. Looks like issues with blob serialization/deserialization.

Done. Bug 1424891 filed.
Attached patch Fix CheckLoadURISplinter Review
Attachment #8936427 - Flags: review?(bzbarsky)
Attachment #8936428 - Flags: review?(valentin.gosu)
Attachment #8936428 - Attachment is patch: true
Attachment #8936427 - Attachment is patch: true
Comment on attachment 8936428 [details] [diff] [review]
Stop accepting 'whatever' for moz-icon URIs

Review of attachment 8936428 [details] [diff] [review]:

Looks good!

::: image/decoders/icon/nsIconURI.cpp
@@ +219,5 @@
>    nsAutoCString iconSpec(aSpec);
>    if (!Substring(iconSpec, 0,
> +                 MOZICON_SCHEME_LEN).EqualsLiteral(MOZICON_SCHEME) ||
> +      (!Substring(iconSpec, MOZICON_SCHEME_LEN, 7).EqualsLiteral("file://") &&
> +       !Substring(iconSpec, MOZICON_SCHEME_LEN, 8).EqualsLiteral("//stock/") &&

You don't need this line. if the substring doesn't start with //, it definitely doesn't start with //stock. We can mention it in a comment, if you want to.

@@ +304,5 @@
> +    // The inner URI should be a 'file:' one. If not, bail.
> +    bool isFile = false;
> +    if (!NS_SUCCEEDED(mIconURL->SchemeIs("file", &isFile)) || !isFile) {
> +      return NS_ERROR_MALFORMED_URI;
> +    }

We should probably add a test to test_moz_icon_uri.js in a follow up patch.
Attachment #8936428 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8936427 [details] [diff] [review]
Fix CheckLoadURI

Attachment #8936427 - Flags: review?(bzbarsky) → review+
bug 1173214 was sec-low, and this doesn't even confirm the existence of actual local files, just whether specific mime types are set to be handled by specific programs, which seems less serious (given that up until pretty recently, you could just ask the 'plugins' object whether the adobe acrobat npapi plugin was installed, for instance). So going to mark as sec-low and land the patches. Ni'ing Dan so he can change severity if necessary.

I'll try to address point 4 from comment 4 in bug 1171853 either before or after the Christmas / New Year's break, depending on the rest of my queue. I don't currently have any reason to believe it's urgent given the other restrictions that are in place.
Flags: needinfo?(dveditz)
Bug 1173214's reporter never provided a working PoC thus the sec-low.
(In reply to Abdulrahman Alqabandi from comment #15)
> Bug 1173214's reporter never provided a working PoC thus the sec-low.

It's true that they didn't provide a working PoC, but even detecting arbitrary local files is only sec-moderate, and as I already said, this bug seems less severe (it doesn't detect arbitrary local files - it detects application handlers that are (or were!) installed, and not even their paths).

Either way, whether it's sec-low or sec-moderate, this can land without sec-approval, which is the salient point for me landing it right now... Seems pretty clear this isn't sec-high/crit.
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Group: firefox-core-security → core-security-release
Doesn't seem worth backporting to me, but feel free to set the status to affected and request approval if you feel strongly otherwise.
Duplicate of this bug: 1424891
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main59+]
Alias: CVE-2018-5140
Group: core-security-release
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.