1354248 - Allow privileged about content process use page-icon protocol

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P5)

Description

[Ursula Sarracini (:ursula)]

The page-icon protocol only works in a chrome privileged scope, would be nice to spec out the work needed to allow the page-icon protocol to be used in content space

Comment 1

[Marco Bonardo [:mak]]

Yeah, here the questions are mostly about security. And I don't have answers, so we need someone from network and security to help us.

The protocol only hands-off image content types, and SVG documents. Currently those contents are local only (cached).

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Marco Bonardo [::mak] from comment #1)
> Yeah, here the questions are mostly about security. And I don't have
> answers, so we need someone from network and security to help us.

Wennie, can someone on your team advise us here?

Comment 3

[Wennie]

Hi Jonathan:can you take a look at this? Thanks!

Comment 4

[Jonathan Kingston [:jkt] he/him]

Wennie I actually don't think I am the right person to answer the security implications of exposing this protocol.

This is actually something we needed for containers in Bug 1315616 for extensions.

However we should restrict this protocol to web content being that it might expose the browsers history with the latency of cached icons. As it's not standards backed either it's another reason not to expose to the web.

Out of interest Ursula what use case do you envisage using this protocol for?

Dan is likely a better person to confirm the implications.

Comment 5

[Marco Bonardo [:mak]]

potentially, we could just need to expose it to just a given whitelist of internal Firefox content pages.

Comment 6

[Ursula Sarracini (:ursula)]

This was filed so that Activity Stream could display icons while being unprivileged. Right now it uses data URIs to display favicons for things like top sites, but eventually we want to be able to use the page-icon protocol while also running in content.

Comment 7

[Jonathan Kingston [:jkt] he/him]

Ah does it override about:newtab then.
We in containers basically guess at favicons for a similar use-case (although there is the annoyance of favicons from different containers too which I dunno how we should handle that). In our case we actually don't always have the favicon in the cache when the user loads the URL.

Comment 8

[Ursula Sarracini (:ursula)]

So Activity Stream doesn't actually "override" about:newtab, it just becomes the default about:newtab... aboutNewTabService.js finds outs if Activity Stream is enabled via a pref, and if so, lets about:newtab load Activity Stream's resource page instead of the other one. So it gets all the same behavior of the current about:newtab.

Comment 9

[Jonathan Kingston [:jkt] he/him]

I spoke to Dan about this at the all hands and he seemed to think it was ok. If the icons were to be injected into web content we should check to see that a canvas element can't read in the URL so to prevent the page capturing history.

If the icons were just opened up to the new tab page we might be ok too. I don't think extensions have the ability to inject content scripts into the new tab page.


I still think it would be more useful to check the favicon store and if not present fetch the URL and get the icon this may actually reduce some of the privacy risk also.

Comment 10

[Kuba Niewiarowski]

Related to bug 1315616?

Comment 11

[Kuba Niewiarowski]

Duh, bug 1385864. Ignore above.

Comment 12

[Marco Bonardo [:mak]]

I'm adding a "whitelisted" to the title, since I don't think it should be exposed globally. In particular, just fetching the favicon for a page, it would be possible to extrapolate the user's history.

Comment 13

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

We jump through a variety of hoops to get favicons to show up in things like about:home / about:newtab. It looks like sometimes we work around this by using data URIs. If we were able to use the page-icon service from the privileged about content process, we could side-step most of that complexity and probably improve memory usage while we're at it (since each data URI is a potentially large string that each about:newtab will hold in memory).

Comment 14

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

See bug 1184701 for a similar effort where we made the page thumbnail protocol work from the privileged about content process.

Comment 15

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

Hey mak, any objections if I try to move this forward?

Comment 16

[Marco Bonardo [:mak]]

I think the only concern is related to security/privacy, that boils down to our isolation safety, since if a third party should get access to this data from content they could extract info about the user history. I don't think it's a major problem considered that would be a break of our privileged/unprivileged boundary with a lot more implications.

About the implementation, as pointed out on Matrix, it will likely require IPC because other processed can't read from db.

Comment 17

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

Attachment: Bug 1354248 - Allow privileged about content process use page-icon protocol. r?nika!,ckerschb!,mak!

Comment 18

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

Hey nika,

I've just started setting up the boilerplate for getting the page-icon:// protocol to work from the privileged about content process. It's at the point where it compiles, but that's it - I haven't actually done the interesting part where I pass the stream from the parent process down to content. Apparently, you have some goodies in the works that might make this easier for me to do? Are there any examples I can look at?

Comment 19

[Nika Layzell [:nika] (ni? for response)]

We discussed the possibilities a bit on matrix, so clearing ni

Comment 20

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

Hey nika,

Thanks for all your help so far! So what I have in https://phabricator.services.mozilla.com/D145622 mostly works - I can get page-icon: URIs loading in the privileged about content process when they point at an existing favicon. Yay!

What doesn't appear to work is the failure case, when a favicon doesn't exist (if you, for example, request page-icon:https://www.page-does-not-exist.com), we fail to get the default icon streamed down to the content process. I'm not sure why that is...

I'm creating a Pipe and passing the nsIOutputStream along to the FaviconDataCallback thing to have the existing infrastructure write to, and then on the other side of the pipe, I'm using NS_AsyncCopy to copy the nsIInputStream down to the DataPipeSender. I'm not 100% sure why the Pipe is necessary - if I just pass the DataPipeSender to the FaviconDataCallback to write to, the content process doesn't seem to get it.

At any rate, in the failure case, when no favicon can be found, we go through some rigamarole to get the default favicon here: https://searchfox.org/mozilla-central/rev/390f7009e9e5a9585297aeea5fb9202f800eaf12/toolkit/components/places/PageIconProtocolHandler.cpp#57-104

My patch mostly keeps that the same except for making the nsCOMPtr<nsIChannel> a Maybe<nsCOMPtr<nsIChannel>>.

Any idea why the default favicon case isn't working? And is there a reason I need the Pipe and can't just ->Write into the DataPipeSender?

Comment 21

[Nika Layzell [:nika] (ni? for response)]

I left some comments in the phabricator revision, so clearing this needinfo

Comment 22

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

Attachment: Bug 1354248 - Part 1: Split PageThumbStreamGetter into a common utility class. r?#necko-reviewers!

Comment 23

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

Attachment: Bug 1354248 - Part 2: Abstract out the IPC message that RemoteStreamGetter uses. r?#necko-reviewers!

Depends on D147180

Comment 24

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

Attachment: Bug 1354248 - Part 3: Supply LoadInfoArgs through the RemoteStreamGetter mechanism. r?#necko-reviewers!

Depends on D147181

Comment 25

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

Attachment: Bug 1354248 - Part 4: Make PageIconProtocolHandler use RemoteStreamGetter. r?nika!,mak!,ckerschb!,#necko-reviewers!

Depends on D147334

Comment 26

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

ni?ing myself to send a message to firefox-dev about this capability once this lands and sticks.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f60dbe1d87b
Part 1: Split PageThumbStreamGetter into a common utility class. r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/12c098c6d1a5
Part 2: Abstract out the IPC message that RemoteStreamGetter uses. r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/3bbdfa6e9b99
Part 3: Supply LoadInfoArgs through the RemoteStreamGetter mechanism. r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/2f3bf2d5f1a6
Part 4: Make PageIconProtocolHandler use RemoteStreamGetter. r=necko-reviewers,nika,mak,ckerschb,kershaw

Comment 28

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/0f60dbe1d87b
https://hg.mozilla.org/mozilla-central/rev/12c098c6d1a5
https://hg.mozilla.org/mozilla-central/rev/3bbdfa6e9b99
https://hg.mozilla.org/mozilla-central/rev/2f3bf2d5f1a6

Comment 29

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

Sent a message to firefox-dev: https://groups.google.com/g/firefox-dev/c/-FJSd_mbsNE