1844792 - Protocol handling dialog just requests icons (image files) from non-mozilla-controlled hosts directly from chrome/parent process

Status: RESOLVED FIXED

(Firefox :: File Handling, defect, P3)

Description

[:Gijs (he/him)]

Freddy and Christoph's appearance in the frontend meeting reminded me of this longstanding problem. There used to be worse things in our browser security-wise, and it looks like I never filed this but... yeah.

STR 1:

1. open settings
2. search for 'mailto'
3. click the dropdown

STR 2:

1. Set mailto to "always ask" in the settings
2. Open any site with a mailto link
3. click the mailto link

ER: any images shown are from cached local sources like the favicon DB, or things we ship with the browser

AR: we load images directly from http/https URIs. Specifically:

1. https://mail.google.com/favicon.ico which redirects to https://ssl.gstatic.com/ui/v1/icons/mail/rfr/gmail.ico

2. whatever else is defined in https://searchfox.org/mozilla-central/source/uriloader/exthandler/HandlerList.sys.mjs + "favicon.ico" (locale-dependent). On older en-US profiles this includes Yahoo Mail's https://compose.mail.yahoo.com/favicon.ico .

3. any user-accepted web-based handlers registered through registerProtocolHandler (again, + "favicon.ico")

This is not great because if there is a UAF or other native vulnerability in our image decoder or rendering code, this could be used to pwn Firefox in the parent process, as that's where the image is decoded and rendered in both of these UIs.

Web-based handlers are restricted to https these days (ie require secure context) when site-registered. In bug 1526890 I restricted the handlers we define inside our own shipped code to https, but it looks like that restriction was removed again in bug 1733497. Thankfully only csb seems to have handlers that use http, and I didn't think we shipped that locale to release (:flod, can you confirm?). We should probably fix that separately (I will file).

It is tedious but I imagine that the only way to 'really' solve this if we had to keep accepting third-party URIs would be to use a sandboxed iframe specifically for the image display?

We could try to use places favicon URIs but they might be unavailable locally. I don't know if we can easily provide a "fallback" solution in that case. Marco, do you have suggestions?

The recent migration/import work has also done some work around favicon sync and/or fetching after import, so I wonder if any of that could help us here. Mike, do you have thoughts?

Comment 1

[Francesco Lodolo [:flod]]

Thankfully only csb seems to have handlers that use http, and I didn't think we shipped that locale to release (:flod, can you confirm?)

Correct, we don't ship csb anymore (it was removed a long time ago, bug 1104047). I think the list was copied looking at all l10n repositories (and we have more than 150), ignoring if a locale effectively ships or not somewhere.

We should probably remove locales non shipping in Nightly from the list (csb, sah).

Comment 2

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #1)

Thankfully only csb seems to have handlers that use http, and I didn't think we shipped that locale to release (:flod, can you confirm?)

Correct, we don't ship csb anymore (it was removed a long time ago, bug 1104047). I think the list was copied looking at all l10n repositories (and we have more than 150), ignoring if a locale effectively ships or not somewhere.

We should probably remove locales non shipping in Nightly from the list (csb, sah).

Good idea, filed bug 1844800 for this.

Comment 3

[Marco Bonardo [:mak]]

(In reply to :Gijs (he/him) from comment #0)

We could try to use places favicon URIs but they might be unavailable locally. I don't know if we can easily provide a "fallback" solution in that case. Marco, do you have suggestions?

There is no easy fallback, for similar reasons: we don't want the favicon protocols to fallback to network, unless we can guarantee full safety and privacy awareness.
One could query the favicons API (getFaviconURLForPage or getFaviconDataForPage) and then eventually fallback if nothing is found, but as you said there is no guarantee we have a cached icon. It would reduce the likelihood of the problem happening, but you'd still have to handle the non-cached case somehow.
Or we could provide, as fallback, a set of generic icons like mail, media, p2p and a generic network link... but then you'd need a map between most common protocols and these categories. In this case you could first try the cache, and then fallback to a reasonable icon, accepting that it won't be the actual service favicon.

Comment 4

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #3)

(In reply to :Gijs (he/him) from comment #0)

We could try to use places favicon URIs but they might be unavailable locally. I don't know if we can easily provide a "fallback" solution in that case. Marco, do you have suggestions?

There is no easy fallback, for similar reasons: we don't want the favicon protocols to fallback to network, unless we can guarantee full safety and privacy awareness.

Right, I meant something more like url(page-icon:blah, "chrome://some-fallback-icon.png"). Do we have any CSS/URL based mechanisms for this or would we need to route via a JS API that does the fallback work?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

AIUI, the page-icon protocol will automatically use the default favicon (chrome://global/skin/icons/defaultFavicon.svg) if it cannot find an image in the local database: https://searchfox.org/mozilla-central/rev/d6960b4e32d09bff32865694e32384eb9bca4af5/toolkit/components/places/PageIconProtocolHandler.cpp#362

Maybe I'm misunderstanding this conversation so far, but is that not sufficient? Or did we want a different fallback icon in this case?

Comment 6

[Marco Bonardo [:mak]]

(In reply to :Gijs (he/him) from comment #4)

Right, I meant something more like url(page-icon:blah, "chrome://some-fallback-icon.png"). Do we have any CSS/URL based mechanisms for this or would we need to route via a JS API that does the fallback work?

The only fallback we have for page-icon: is to the default favicon (the globe).
If I remember correctly the moz-icon: protocol can show file types icons like moz-icon://.html?size=32, not sure if that helps.
In any case you'll need either javascript code, or change the cpp implementation of page-icon to recognize specific protocols and return specific icons.
I don't think css would be able to distinguish a default-icon from any other one to override only in certain cases.

Comment 7

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) (:⚙️) (PTO: July 17-21) from comment #5)

AIUI, the page-icon protocol will automatically use the default favicon (chrome://global/skin/icons/defaultFavicon.svg) if it cannot find an image in the local database: https://searchfox.org/mozilla-central/rev/d6960b4e32d09bff32865694e32384eb9bca4af5/toolkit/components/places/PageIconProtocolHandler.cpp#362

Maybe I'm misunderstanding this conversation so far, but is that not sufficient? Or did we want a different fallback icon in this case?

I think it would be nice to have icons at least for the shipped providers, even on a clean profile, and the simplest way I could think of would be to package those somehow and then source them using chrome or similar URLs. But perhaps a more sensible thing to do would be to import them into the favicon database if it doesn't have any entries for those URLs, or something? Or... do we think that's overkill and showing a default favicon for this case would be fine?

Is the favicon work for sync that was discussed at some point stalled and/or would it help here? (bug 428378 got wontfixed so I'm guessing no help from that quarter...)

Comment 8

[Marco Bonardo [:mak]]

I don't think favicons from Sync help us, it's quite a cost.
Adding icons to Places database may work, but it will have its downsides, e.g. if the db corrupts or the user removes it, they are gone. So we'd need a way to tell if the db was filled with these icons already, and a way to not clear those with History. Not technically impossible but seems expensive again.

Querying Places, and eventually falling back to a set of local icons is still the best bet.
Now the question is how much we want to be specific with local icons:

1. if we care a lot, maybe we could use tippy top icons? Top Sites use tippy top icons for common sites, maybe we could expand on that for common sites with protocols? See https://searchfox.org/mozilla-central/source/browser/components/newtab/data/content/tippytop and https://github.com/mozilla/tippy-top-sites
2. if we still want to provide some differentiator, we could categorize most common protocols (how do we fetch that list?) and fallback to a set of icons for media, p2p, document, network...
3. if we really don't mind, just show the globe or a generic "protocol" icon

Icons are nice, but imo not the end of the world if they are missing. If we're going to add some local icons, I'd probably evaluate the tippy-top approach rather than categorization. So I'd say either 1 or 3, if 1 ends up being complex.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

The Sync work for favicons never really got past the theoretical - the storage size was always seen as prohibitive. We ended up with two potential solutions:

1. Use OHTTP and a favicon caching service to anonymously request favicons over the network. This doesn't help us here, I think.
2. Pre-pack a series of favicons, similar to the TippyTop set, with the build as fallbacks. This is similar to what Marco mentions in comment 8.

The short-term fix here is probably to use page-icon:// to try to get the favicon, and fallback to the default globe icon. The longer-term fix is to build out a more robust fallback mechanism when a favicon doesn't exist in the database.

Comment 10

[Daniel Veditz [:dveditz]]

If we don't ship locales with http: urls (that could be hijacked on the net), then these urls are going to come from our vetted lists or sites that the user trusts enough to add as a mailto: handler. Loading these in the parent isn't great, but the practical risk of being compromised through a malicious favicon (assuming a known exploitable image bug) seems low.

Comment 11

[Tom Schuster]

Adding a CSP for the appManager dialog makes it pretty obvious that this is happening: D235439.

Comment 12

[:Gijs (he/him)]

Attachment: (secure)

Comment 13

[:Gijs (he/him)]

Forward-duping as bug 2000535 has a more viable patch and I never got around to updating my earlier attempt here...

Comment 14

[:Gijs (he/him)]

Annnnd unduping again because I realized that it's not actually the same dialog, even though both involve protocol handlers.

This bug is (also) about:

https://searchfox.org/firefox-main/rev/d88792ab9de45efd74909d34fb20c8e8405dbb70/toolkit/mozapps/handling/content/appChooser.xhtml#8-9

<?csp default-src chrome:; style-src chrome: 'unsafe-inline'; img-src http:
https: moz-icon:; ?>

which bug 2000535 doesn't touch.

Tom, do we have a separate patch for this already, and/or do you want to take this?

Comment 15

[Tom Schuster]

Tom, do we have a separate patch for this already, and/or do you want to take this?

I don't have a patch for this yet, but I can write a quick patch.

Do you want me to use this bug?

Comment 16

[Tom Schuster]

Attachment: (secure)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8b39e2f00010
https://hg.mozilla.org/integration/autoland/rev/a5dc7e0d9932
Use moz-remote-image: for appChooser.xhtml. r=Gijs

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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