1222924 - Stop exposing the moz-icon URL scheme to the web

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Frederik Braun [:freddy]]

For web compat and reduced security surface, I suggest we stop allowing moz-icon URLs on the web.

Comment 1

[Masatoshi Kimura [:emk]]

The ftp directory listing uses moz-icon.

Comment 2

[:Gijs (he/him)]

To what degree is this still web-exposed post bug 1173214 ?

Comment 3

[Bobby Holley (:bholley)]

I have no idea - do the observations in https://blog.mozilla.org/nnethercote/2015/11/05/moz-icon-a-curious-corner-of-firefox/ still work?

Comment 4

[:Gijs (he/him)]

(In reply to Bobby Holley (busy) from comment #3)
> I have no idea - do the observations in
> https://blog.mozilla.org/nnethercote/2015/11/05/moz-icon-a-curious-corner-of-
> firefox/ still work?

Yes. http://jsbin.com/fivudigoqe/edit?html,output

I'd forgotten I only changed a subset of moz-icon to be unusable from the web. This helps with the ftp file listing issue described in comment #1.

We could work around that somehow, though I'm not sure offhand exactly how we would... In principle the file listing page itself is just a page "like any other". We could display the same set of icons as before and pass the data some other way, but if we use blob URIs or data URIs to accomplish this somehow, it seems we'd be likely to give a same-origin web page access to the actual image data, which would be worse than the status quo.

Looking at it the other way, we could start treating such directory listings as "different" and make them non-same-origin with other things using the same protocol, but that likewise makes me uneasy - it feels like just shifting the problem around, and it might break existing users, maybe?

A third solution would be accepting the loss of icons and just using generic file/folder icons (or potentially shipping icons for a particular set of files and hardcoding those).

Finally, of course, we could decide to do nothing...

It feels to me like I'm missing a more trivial solution. Thoughts, Bobby?

Comment 5

[:Gijs (he/him)]

I guess as another option, we could make moz-icon inaccessible from ftp but not from file: or chrome:, which would only break the icons on ftp, which I'm less bothered about than file: or chrome: (though I'm not sure how bothered I am even about those, I think other people will feel differently).

Comment 6

[Bobby Holley (:bholley)]

(In reply to :Gijs Kruitbosch from comment #5)
> I guess as another option, we could make moz-icon inaccessible from ftp but
> not from file: or chrome:, which would only break the icons on ftp, which
> I'm less bothered about than file: or chrome: (though I'm not sure how
> bothered I am even about those, I think other people will feel differently).

Yes, this is what I was going to propose - making it file://, resource://, and chrome:// only, which seems like good bang for our buck. Boris, do you think that would be acceptable?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I guess I can live with that, yeah.

Comment 8

[nemo]

Mildly sad as a user at the loss of icons in ftp, which I still do in fact use periodically.  I guess ftp directory listings are considered remote web pages, which explains why NoScript breaks sorting on them.  But, oh well, I understand it is yet another protocol fading just as fast as gopher.

One question though.  Firefox *does* support sftp:// and rather nicely too.  sftp I would think is used a bit more by devs.

Firefox sftp:// also suffers from the NoScript blocking of sorting (although presumably if you can sftp to a site you trust it to run scripts in documents) which makes me think that maybe moz-icon would end up being broken there too?

Would you consider adding that protocol to your whitelist?  I mean, you're going to auth anyway to an sftp site.

Comment 9

[John Drinkwater (:beta)]

Shame, a few years ago when icon: was being drafted I opened Bug 599904 to track it because I thought it would be *really* useful. There is still a benefit —imo— for being able to show the user an icon they would recognise for a file type.

Comment 10

[Ethan Tseng [:ethan]]

Arthur, we would like to remove this bug from our MVP.
Do you agree? Did the Tor Browser have a fix for this?

Comment 11

[Arthur Edelstein [:arthur]]

We don't yet have a fix for this, unfortunately. But it's a good reminder and I can work on it soon. I opened a tor ticket: https://trac.torproject.org/projects/tor/ticket/23424

Comment 12

[:Gijs (he/him)]

Attachment: Bug 1222924 - stop allowing webpages to link to moz-icon: ,

Review commit: https://reviewboard.mozilla.org/r/202202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202202/

Comment 13

[:Gijs (he/him)]

I was reminded of this bug by bug 1419811 and bug 1419780, so I figured I'd give it a go.

I thought that really, all we needed was the right URI flags on nsMozIconProtocolHandler, but it turns out it already had URI_UI_RESOURCE . file: things can't link to that, and probably shouldn't be able to, but there's a special case in https://dxr.mozilla.org/mozilla-central/rev/72ee4800d4156931c89b58bd807af4a3083702bb/caps/nsScriptSecurityManager.cpp#840-844 that makes an exception for moz-icon and allows anything to load it (Note: only applies to the non-nested version, so moz-icon://.ext, not moz-icon://file:///path/to/file.ext).

So instead, I removed that exception, and added a more explicit and specific exception further up, limited to file: principals.

Alternatively, I *think* we could switch moz-icon to URI_IS_LOCAL_FILE, but it seems we have a bunch of checks elsewhere in the codebase that implicitly assume that that === file: URIs, plus it feels like a security "downgrade" from UI_RESOURCE, so I was hesitant about going in that direction. On the plus side, less specialcasing in nsScriptSecurityManager... :-\

Comment 14

[:Gijs (he/him)]

Note that all this is fixing comment #0, but I don't know if it would help against TOR-relevant user/machine fingerprinting for the presence of a moz-icon handler (which currently throws a security error). That is, I haven't checked if it's possible to detect that vs. not having a handler for the protocol at all.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8931126 [details]
Bug 1222924 - stop allowing webpages to link to moz-icon: ,

https://reviewboard.mozilla.org/r/202202/#review209582

Sorry about the delay getting to this. This looks good to me.

Comment 16

[:Gijs (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec10a8c90069f858e3f274f4967a1102814209e5

Comment 17

[:Gijs (he/him)]

Looks like I need to update dom/security/test/mixedcontentblocker/test_bug803225.html to remove the moz-icon tests, as after this patch https: can no longer load moz-icon links anyway, so it's not relevant for mixed-content blocking.

Comment 18

[:Gijs (he/him)]

Also looks like this'll break image/test/reftest/downscaling/downscale-moz-icon-1.html on android. dholbert, should I just disable the reftest only on android, or would that be bad? Looks like we don't use http: to access reftests anywhere else, and I'd prefer not to introduce yet another reftest-only pref (we had to do this for view-source already...) if possible.

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to :Gijs from comment #18)
> Also looks like this'll break
> image/test/reftest/downscaling/downscale-moz-icon-1.html on android.
> dholbert, should I just disable the reftest only on android, or would that
> be bad? 

That seems fine to me, though it'd be nice to leave behind a comment in the reftest.list file with a brief explanation for the disabling.(the moz-icon protocol being disabled over http & other emote , and reftests all being served as http on android)

Note that this reftest is listed *twice* in reftest.list (to get tested under two different configurations), so you'll need to adjust both lines. Conveniently, both lines already have a skip-if(...) annotation, so you can probably just add "Android||" to the beginning of that annotation's condition.

> Looks like we don't use http: to access reftests anywhere else

(Yeah, I'm pretty sure that's correct -- specifically, Android is the only platform where they're *always* served over HTTP. On desktop, individual reftests *can* opt in to being served over http, but that's not relevant here.)

Comment 20

[Daniel Holbert [:dholbert]]

> & other emote ,

"& other remote protocols"

Comment 21

[:Gijs (he/him)]

Comment on attachment 8931126 [details]
Bug 1222924 - stop allowing webpages to link to moz-icon: ,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202202/diff/1-2/

Comment 22

[:Gijs (he/him)]

Comment on attachment 8931126 [details]
Bug 1222924 - stop allowing webpages to link to moz-icon: ,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202202/diff/2-3/

Comment 23

[Daniel Holbert [:dholbert]]

The reftest.list tweaks look good. But: shouldn't we also add some new tests (or test-tweaks) to validate that moz-icon is indeed not exposed on the web anymore?  (to verify that this change actually takes effect & doesn't get reverted/relaxed by accident)

(Maybe such tests are already included and I overlooked them - if so, sorry about that)

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec15d17d85cb
stop allowing webpages to link to moz-icon: , r=mrbkap

Comment 25

[:Gijs (he/him)]

(In reply to Daniel Holbert [:dholbert] from comment #23)
> The reftest.list tweaks look good. But: shouldn't we also add some new tests
> (or test-tweaks) to validate that moz-icon is indeed not exposed on the web
> anymore?  (to verify that this change actually takes effect & doesn't get
> reverted/relaxed by accident)
> 
> (Maybe such tests are already included and I overlooked them - if so, sorry
> about that)

Sorry, didn't see this comment before looking at the trypush and then pushing from reviewboard. :-\

We should. In fact, I kind of expected the trypushes to point to some existing tests, but apparently not. I'll file a followup bug.

Comment 26

[Raul Gurzau (:RaulG)]

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