1371786 - Allow content scripts on reader mode pages

Status: REOPENED

Description

[Andy McKay]

Reader mode is not a Chrome privileged page according to https://dxr.mozilla.org/mozilla-central/source/browser/components/about/AboutRedirector.cpp, but I'd like someone from security to confirm that attaching a content script to this page will not damage our sandbox.

Also noting that file asks for security review before we do anything with those values. I'd be interested in knowing if we can write a test so that if anyone changes those and about:reader gets Chrome privs., we can fail that test and give ourselves a warning sign.

Comment 1

[Paul Theriault [:pauljt]]

We can take a look at this, but I'll just have to add it to our queue (I'll file a PI request). We should be able to get to it this quarter (someone may know the answer to this, but if its me doing this I need to figure out:
 - where does reader load content (child I think)
 - What APIs does reader mode expose to the child

There are several different threats here (ie different sandboxes we are talking about). Allowing web extensions to put scripts in reader mode pages might:
 -  give access for web extension to execute code in a privilege context (ie escape limitations of moz-extension:// privilege)
 -  Allowing web extension to create holes in our process sandbox (either deliberately or accidentally)

Depending on the architecture extension they may also expose these threats to the web.

The first step is to figure out the capabilities of reader mode.
From a process sandbox perspective, the IPC code for reader is here:
http://searchfox.org/mozilla-central/source/browser/modules/ReaderParent.jsm

Reader:FaviconRequest & Reader:ArticleGet both take URLs so it would be interesting to see if/how we enforce normal URI restrictions.

Comment 2

[Bob Silverberg [:bsilverberg]]

Paul, I will be working on implementing some of the reader mode WebExtension APIs this quarter, so I was wondering where this security review stands currently in your queue. Has a bug been filed for the review? I don't see anything listed as blocking this bug.

Comment 3

[Paul Theriault [:pauljt]]

Bob, it _was_ low as I assumed this wouldnt happen until after 57, but shouldnt be too much work, so Ill try to get it rolling.

Comment 4

[Paul Theriault [:pauljt]]

Also, let us know how you go with implementation. Some of this would be easier to test, than to actually code review for.

Comment 5

[Bob Silverberg [:bsilverberg]]

(In reply to Paul Theriault [:pauljt] from comment #4)
> Also, let us know how you go with implementation. Some of this would be
> easier to test, than to actually code review for.

I plan on starting this week on this. I might have held off on this aspect of the reader mode API until a security review was done, but it sounds like you could benefit from the implementation, so I will get moving on that.

Comment 6

[Paul Theriault [:pauljt]]

Thanks bob, cr is just starting the review this week too, so good timing.

Comment 7

[Bob Silverberg [:bsilverberg]]

We currently support running content scripts on about:blank, but in order to do so one has to add a specific flag when declaring the content script (matchAboutBlank: true). It looks like in order to support this we're going to need to do some things similar to how we handle about:blank, so I am wondering if we should add a flag for this too (i.e., matchAboutReader). I don't think we need that. It seems to me that if we're going to support content scripts on about:reader pages then any content script should be allowed, as long as the extension has access to the reader page's rendered URL, and requiring developers to specify an extra flag to allow the script to run on about:reader is unnecessary.

Please comment if you disagree.

Comment 8

[Christiane Ruetten [:cr]]

Some info I could gather so far... Besides UI status changes, Reader Mode parent seems to expose only two interesting API calls through its message handler:

- PlacesUtils.promiseFaviconLinkUrl(message.data.url)
- ReaderMode.downloadAndParseDocument(message.data.url)

I haven't followed the former all the way through into C++ land, because it looks very low risk, but the latter exposes XMLHttpRequest GETs to arbitrary URLs. The check whether Reader Mode is enabled for the URL is unfortunately done on the request result (so both .documentURIObject and .documentURIObject can be checked), and merely determines whether document content is returned or not. (This is a significant security issue in itself that will get a dedicated bug.)

The implication is that the Web Extension API should ensure that there are no unauthorized requests passed through to the parent, even though this needs to be enforced by the parent in the long run.

Comment 9

[Paul Theriault [:pauljt]]

(In reply to Bob Silverberg [:bsilverberg] from comment #7)
> We currently support running content scripts on about:blank, but in order to
> do so one has to add a specific flag when declaring the content script
> (matchAboutBlank: true). It looks like in order to support this we're going
> to need to do some things similar to how we handle about:blank, so I am
> wondering if we should add a flag for this too (i.e., matchAboutReader). I
> don't think we need that. It seems to me that if we're going to support
> content scripts on about:reader pages then any content script should be
> allowed, as long as the extension has access to the reader page's rendered
> URL, and requiring developers to specify an extra flag to allow the script
> to run on about:reader is unnecessary.
> 
> Please comment if you disagree.

I think I agree with you here - on the assumption injecting scripts into about:reader does not convey additional privileges, we can just rely on the host permissions model (but something we'd want to check in the review). I do also wonder about the implications of running script at an about:url, but i suppose we must already have good segregation here, otherwise about:blank is a security risk. 

(For example what I have a script running at about:reader?url=foo.com, and then I run fetch('about:reader?url=bar.com'). Is that a same origin request? Does the browser even 'fetch' that, i assume not)

Comment 10

[Paul Theriault [:pauljt]]

Well ignore that specific case: "TypeError: NetworkError when attempting to fetch resource."

But maybe there other same-origin issues here. (framing about:pages etc etc)

Comment 11

[Bob Silverberg [:bsilverberg]]

Created attachment 8904623 [details] [diff] [review]
Patch with a simplified implementation for initial security testing

Kris, could you please take a look at this patch to confirm whether this is even a correct approach to take to allow content scripts to be injected into about:reader pages? This is *not* a final implementation, but would be used by the security team to do some initial testing of the implications of granting access to about:reader pages. As such, I want to make sure that the implementation is doing the correct thing, or if I should be doing something else entirely to grant access to about:reader pages.

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8904623 [details] [diff] [review]
Patch with a simplified implementation for initial security testing

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

I'm giving this a cautions f+. However, please talk to someone who works on reader mode about how desirable they think this is.

And we're going to have to do a careful audit of the about:reader code to make sure it doesn't give code running in about:reader pages any dangerous privileges.

::: toolkit/components/extensions/WebExtensionPolicy.cpp
@@ +332,5 @@
> +  // Top-level about:reader is a special case. If we match the URL it is
> +  // trying to render, then we allow it.
> +  const nsAString& aboutReader = Substring(aDoc.URL().Spec(), 0, 12);
> +  if (aDoc.IsTopLevel() &&
> +      aboutReader.EqualsLiteral("about:reader")) {

Nit: `aDoc.URL().Scheme() == nsGkAtoms::about &&
      aDoc.URL().Host().EqualsLiteral("reader")`

And in that case, we should match permissions against the value of the `url` parameter rather than simply returning true.

Comment 13

[Bob Silverberg [:bsilverberg]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #12)
> Comment on attachment 8904623 [details] [diff] [review]
> Patch with a simplified implementation for initial security testing
> 
> Review of attachment 8904623 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm giving this a cautions f+. However, please talk to someone who works on
> reader mode about how desirable they think this is.
> 
> And we're going to have to do a careful audit of the about:reader code to
> make sure it doesn't give code running in about:reader pages any dangerous
> privileges.
> 

Thanks for the feedback, Kris. Yes, this is just an initial step and the code will look very different in the end, and there's a lot more investigation to be done into the safety of this. I just wanted to make sure that I'm making the changes in the right place.

Christiane, if it will help, you can apply this patch against a build to enable content scripts on about:reader pages to see what the impact is.

Comment 14

[Bob Silverberg [:bsilverberg]]

Christiane, have you been able to progress with the security review of this feature? Is there anything else you need from me to facilitate that?

Comment 15

[Bob Silverberg [:bsilverberg]]

Christiane, just checking in again to see where this stands. Is the completion of this review on your current to-do list, or is it on hold?

Comment 16

[Christiane Ruetten [:cr]]

Paul has worked on reader mode before, so he already has ideas.

Comment 17

[Paul Theriault [:pauljt]]

Bob, I've picked this up and done an initial review - see [1]. 

I'm a little concerned about comment 12 - as i mentioned in the review, we need to have a clear permission model of how we expect this to work. IE an extension MUST have both host permission url parameter to reader mode, AND the reader mode uri as a host permission (which I assume will be "about:reader"). 

Note that this means that your changes in WebExtensionPolicy.cpp have to figure to "does the extension have either a Host Permission which is currently set in url parameter of the query string.

Im really worried about this, as it seems massively error prone. What happens if the extension navigates reader mode to a different about:reader uri? I feel like that there are so many edge cases in trying to figure out the actual origin and url of reader mode, that we are likely make a mistake. 

Perhaps as an alternative, we could require the an extension wanting to inject script into reader mode must have both "about:reader" and "<all_urls>" otherwise access is not granted to inject script? That way we can rely on the regular warning to users for <all_urls> and we dont have to worry about reader mode being a bypass of host permissions (other than some bug in readermode allowing access great than what <all_urls> grants). 

What security model did you have in mind here?


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1390035#c5

Comment 18

[Bob Silverberg [:bsilverberg]]

Thanks Paul. I have read through the review bug at [1] as well as the review notes at [2]. My first question is where would you like to have this conversation? I have a couple of things to say in response, but I'm not sure whether they should go in this bug, in the bug at [1] or in the doc. What works best for you/for the process?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1390035#c5
[2] https://docs.google.com/document/d/1zux2CBtLun6Lav_ua8evDPyDAq3JZakyl_XRIyjGJoA/edit

Comment 19

[Paul Theriault [:pauljt]]

Email is probably easiest  - I'll send you a note now.

Comment 20

[Paul Theriault [:pauljt]]

I spoke with Bob this morning about this. The security model he had in mind was that there would be no permission for this privilege, other than needing the host permission for the url of the page. That is, the url parameter supplied in the about:reader?url=... must match, in the web extension MatchPattern sense of the word, a host permission specified in the manifest, otherwise the web extension is not allowed to inject into a reader mode page. I think that security model is decent, but I am concerned about how we would implement this. My concerns boil down to two questions:

1. Can we be sure that the content loaded in ReaderMode is the content specified by the URL parameter?
In this model, a Web Extension with limited host permissions might be able to induce reader mode to load (and thus read) content that it doesn't have the host permission for. I don't know think that script inside reader mode can directly do this, but there are many edge cases to consider: redirects, meta tags, DNS rebinding etc etc. I don't know the reader mode code well enough to be sure about any of this.

2. Secondly, I noticed in testing that reader mode can frame a number of about page, that normal content pages can not. Its not an issue in itself (all about pages are treated as cross-origin) but I wonder what implications there are for about: pages. The  risk I can think of here is that if a web extension could access other about pages, it might be able to get privileges that it shouldn't have. I haven't seen this issue arise, but on the other hand, I'm not certain we prevent it either. 

For both of these issues, I'd suggest we get input from someone who knows reader mode a little better than I do, just to confirm that these won't be problems.

Comment 21

[Bob Silverberg [:bsilverberg]]

Thanks for documenting our discussion, Paul. As we discussed, I was going to ask Gijs to help us better understand the answers to your questions about reader mode, but I believe he is away. Someone in fx-team suggested jaws, so I'm needinfoing him. This is regarding the questions at comment #20.

Comment 22

[Bob Silverberg [:bsilverberg]]

Transferring the needinfo to Gijs as I understand he's back from PTO now.

Comment 23

[:Gijs (Not available 3-19 Aug; he/him)]

Yeah, the reader page itself has an unprivileged about: URI (codebase principal for about:reader?url=... - I guess we could file a separate bug to try to make that null principal, just in case...). Bug 1204818 is about moving the readerized context into a sandboxed iframe. I would feel more comfortable giving webextensions access to such a sandboxed iframe document only, though obviously that would make it trickier for the webextension content script to do reader-mode-like things. I don't think we should give the reader mode parent window this principal because it would mean that named windows might make the resulting reader mode document accessible to other same-origin pages via named window access.

(In reply to Christiane Ruetten [:cr] from comment #8)
> Some info I could gather so far... Besides UI status changes, Reader Mode
> parent seems to expose only two interesting API calls through its message
> handler:
> 
> - PlacesUtils.promiseFaviconLinkUrl(message.data.url)
> - ReaderMode.downloadAndParseDocument(message.data.url)
> 
> I haven't followed the former all the way through into C++ land, because it
> looks very low risk, but the latter exposes XMLHttpRequest GETs to arbitrary
> URLs. The check whether Reader Mode is enabled for the URL is unfortunately
> done on the request result (so both .documentURIObject and
> .documentURIObject can be checked), and merely determines whether document
> content is returned or not. (This is a significant security issue in itself
> that will get a dedicated bug.)

Did this get filed? I have further thoughts on this, but I don't want to derail this bug with a full-on discussion.

Comment 24

[Christiane Ruetten [:cr]]

> Did this get filed? I have further thoughts on this, but I don't want to derail this bug with a full-on discussion.

Feel free to continue that discussion in bug 1396565. It has been closed, though, since it doesn't pose any threat as long as child network access is not sandboxed.

Comment 25

[:Gijs (Not available 3-19 Aug; he/him)]

(In reply to Paul Theriault [:pauljt] from comment #20)
> I spoke with Bob this morning about this. The security model he had in mind
> was that there would be no permission for this privilege, other than needing
> the host permission for the url of the page. That is, the url parameter
> supplied in the about:reader?url=... must match, in the web extension
> MatchPattern sense of the word, a host permission specified in the manifest,
> otherwise the web extension is not allowed to inject into a reader mode
> page. I think that security model is decent, but I am concerned about how we
> would implement this. My concerns boil down to two questions:
> 
> 1. Can we be sure that the content loaded in ReaderMode is the content
> specified by the URL parameter?
> In this model, a Web Extension with limited host permissions might be able
> to induce reader mode to load (and thus read) content that it doesn't have
> the host permission for. I don't know think that script inside reader mode
> can directly do this, but there are many edge cases to consider: redirects,
> meta tags, DNS rebinding etc etc. I don't know the reader mode code well
> enough to be sure about any of this.

This seems like an overlapping concern with what Christiane raised in comment #8. I *think* that it should be possible for us to ensure that the content requested for reader mode always matches the loaded URI.

Of course, script running in that page could simply run:

location.href = "about:reader?url=<some other url>"

 to trigger a reader mode load of another URL (assuming whatever sandboxing we do in that other bug doesn't disable navigation, which I haven't thought about yet). But assuming the webextensions code then correctly prevents access to that new document if the webextension shouldn't have permission to run on <some other url>, I think that should be OK. We already have checks to ensure that url is an http/https URL (so it can't be used to read files or chrome documents or about: pages or...). Though it looks like they might want moving so they get run prior to requesting the document in the downloadAndParse case.

> 2. Secondly, I noticed in testing that reader mode can frame a number of
> about page, that normal content pages can not. Its not an issue in itself
> (all about pages are treated as cross-origin) but I wonder what implications
> there are for about: pages. The  risk I can think of here is that if a web
> extension could access other about pages, it might be able to get privileges
> that it shouldn't have. I haven't seen this issue arise, but on the other
> hand, I'm not certain we prevent it either.

Unless something changed or I'm misremembering (please check scriptsecuritymanager::checkloaduri and friends) there are 3 types of about: uris:

1) system/chrome privileged ones (e.g. about:preferences)
2) unprivileged ones that content cannot link to (e.g. about:reader)
3) unprivileged ones that content *can* link to (e.g. about:blank)

I believe that anything can load (3), because even web content can. Pages with system principal can obviously do what they like, and pages in (2) can load other pages in (2). This is mostly because of about:cache, IIRC (which needs to be able to point to about:cache-entry, both of which are implemented in C++ *shudder*). We could try to break that if we thought it was worthwhile.


If we give webextensions access to reader mode as it exists today, in addition to the above, they will be able to adjust the narrate and reader mode preferences (font size, colours, etc.). I think that's fine (they could override them with custom JS/CSS anyway), but it does mean we may want to be careful about any additional functionality we add to reader mode.

They could also probably trigger pocketing the page. I... don't know if that's something that would bother us, I'm not familiar with whether we offer any pocket integration. On the one hand, I guess we probably offer bookmarks integration, on the other, that's presumably a separate permission in webextensions-land?

So on the whole, I am not super keen on doing this without the sandboxing, unless there are (a) good reasons that we need to do this today and/or (b) only allowing access to the sandboxed iframe is somehow hard/complicated/impossible/whatever for reasons that I do not really understand. :-)

Comment 27

[Bob Silverberg [:bsilverberg]]

Paul, I'm not sure where we stand on this now. Based on the feedback we received from Gijs, what are the next steps?

Comment 28

[Paul Theriault [:pauljt]]

(In reply to :Gijs from comment #25)
> This seems like an overlapping concern with what Christiane raised in
> comment #8. I *think* that it should be possible for us to ensure that the
> content requested for reader mode always matches the loaded URI.

cr's concern was around reader mode providing an API for the content process to load arbitrary URLs because we don't enforce the location of the URL prior to doing the XHR request. But the sandbox can't prevent access to loading http(s) URLs - any web page could load any resource so the parent has to allow all content process to load all web resources.

> 
> Of course, script running in that page could simply run:
> 
> location.href = "about:reader?url=<some other url>"
> 
>  to trigger a reader mode load of another URL (assuming whatever sandboxing
> we do in that other bug doesn't disable navigation, which I haven't thought
> about yet). But assuming the webextensions code then correctly prevents
> access to that new document if the webextension shouldn't have permission to
> run on <some other url>, I think that should be OK.

I was worried that about:reader?url=foo.com might would be treated same-origin as about:reader?url=foo.com. (and thus a web extension with ability to inject into foo.com, could read dom content of bar.com)
From testing it seems not to be the case, and that readermode same-origin checks take into account the url param. 

> We already have checks
> to ensure that url is an http/https URL (so it can't be used to read files
> or chrome documents or about: pages or...). Though it looks like they might
> want moving so they get run prior to requesting the document in the
> downloadAndParse case.

> > 2. Secondly, I noticed in testing that reader mode can frame a number of
> > about page, that normal content pages can not. Its not an issue in itself
> > (all about pages are treated as cross-origin) but I wonder what implications
> > there are for about: pages. The  risk I can think of here is that if a web
> > extension could access other about pages, it might be able to get privileges
> > that it shouldn't have. I haven't seen this issue arise, but on the other
> > hand, I'm not certain we prevent it either.
> 
> Unless something changed or I'm misremembering (please check
> scriptsecuritymanager::checkloaduri and friends) there are 3 types of about:
> uris:
> 
> 1) system/chrome privileged ones (e.g. about:preferences)
> 2) unprivileged ones that content cannot link to (e.g. about:reader)
> 3) unprivileged ones that content *can* link to (e.g. about:blank)
> 
> I believe that anything can load (3), because even web content can. Pages
> with system principal can obviously do what they like, and pages in (2) can
> load other pages in (2). This is mostly because of about:cache, IIRC (which
> needs to be able to point to about:cache-entry, both of which are
> implemented in C++ *shudder*). We could try to break that if we thought it
> was worthwhile.
> 
> 
> If we give webextensions access to reader mode as it exists today, in
> addition to the above, they will be able to adjust the narrate and reader
> mode preferences (font size, colours, etc.). I think that's fine (they could
> override them with custom JS/CSS anyway), but it does mean we may want to be
> careful about any additional functionality we add to reader mode.
> 
> They could also probably trigger pocketing the page. I... don't know if
> that's something that would bother us, I'm not familiar with whether we
> offer any pocket integration. On the one hand, I guess we probably offer
> bookmarks integration, on the other, that's presumably a separate permission
> in webextensions-land?

I'm not too worried about either the narration or the pocketing case. That is a separate permissions for bookmarks, but i don't think this method would get a malicious add-on much in terms of value.


> So on the whole, I am not super keen on doing this without the sandboxing,
> unless there are (a) good reasons that we need to do this today and/or (b)
> only allowing access to the sandboxed iframe is somehow
> hard/complicated/impossible/whatever for reasons that I do not really
> understand. :-)

I'd support this recommendation too - I'll be honest, mostly because I am still a very long way from convincing myself that this is safe to do otherwise.

Comment 29

[Paul Theriault [:pauljt]]

(In reply to Bob Silverberg [:bsilverberg] from comment #27)
> Paul, I'm not sure where we stand on this now. Based on the feedback we
> received from Gijs, what are the next steps?

As per last comment, isolating the content document, and allowing a web extension to inject into _that_ context seems  a lot more safe to me. Removes all the weirdness and special snowflakes [1] of about: urls, so it makes assurance much easier.

The two key risks I see are:
 - ReaderMode might allow web extensions to bypass the web extension security model (access hosts which they don't have host permissions for). I haven't found an attack that does this, but I feel like I'm very close.
 - ReaderMode might allow some form of access to semi-privileged contexts. Gijs mostly convinced me above that this isn't a risk, but I'm a long way from a technical understanding of why I am sure this is safe.

At the same time, I'm also doing an audit of all the semi-privileged contexts we have in Firefox. And the more I learn, the less I like the idea of Web Extensions having access to this world. For example, did you know you can load certain pages from https://accounts.firefox.com in reader mode? We are explicitly trying to block access to privileged web pages in web extensions,so thats something we need to blck even if we use the iframe sandbox approach.

Comment 30

[Bob Silverberg [:bsilverberg]]

 (In reply to Paul Theriault [:pauljt] from comment #29)

Thanks Paul.

> 
> As per last comment, isolating the content document, and allowing a web
> extension to inject into _that_ context seems  a lot more safe to me.
> Removes all the weirdness and special snowflakes [1] of about: urls, so it
> makes assurance much easier.
>

So, are we saying that this should be blocked by bug 1204818, and that once bug 1204818 is implemented we'll need to target just that sandboxed content with any content scripts? I'm not sure how simple the latter is, but it seems like a significant change from what we're doing now. Kris, do you have any ideas about this? 
 
> The two key risks I see are:
>  - ReaderMode might allow web extensions to bypass the web extension
> security model (access hosts which they don't have host permissions for). I
> haven't found an attack that does this, but I feel like I'm very close.
>  - ReaderMode might allow some form of access to semi-privileged contexts.
> Gijs mostly convinced me above that this isn't a risk, but I'm a long way
> from a technical understanding of why I am sure this is safe.
> 
> At the same time, I'm also doing an audit of all the semi-privileged
> contexts we have in Firefox. And the more I learn, the less I like the idea
> of Web Extensions having access to this world. For example, did you know you
> can load certain pages from https://accounts.firefox.com in reader mode? We
> are explicitly trying to block access to privileged web pages in web
> extensions,so thats something we need to blck even if we use the iframe
> sandbox approach.

If we are able to implement this so that we only allow content scripts to run for URLs for which the extension has access (via host permissions), as opposed to allowing scripts to run for any about:reader URL (which is the intent), would that alleviate the above concern? Can a WebExtension currently run content scripts on https://accounts.firefox.com? If so, then opening that access up via reader mode doesn't really change anything. If not, then it seems like it wouldn't be an issue.

Comment 31

[Paul Theriault [:pauljt]]

(In reply to Bob Silverberg [:bsilverberg] from comment #30)
>  (In reply to Paul Theriault [:pauljt] from comment #29)
> 
> Thanks Paul.
> 
> > 
> > As per last comment, isolating the content document, and allowing a web
> > extension to inject into _that_ context seems  a lot more safe to me.
> > Removes all the weirdness and special snowflakes [1] of about: urls, so it
> > makes assurance much easier.
> >
> 
> So, are we saying that this should be blocked by bug 1204818, and that once
> bug 1204818 is implemented we'll need to target just that sandboxed content
> with any content scripts? I'm not sure how simple the latter is, but it
> seems like a significant change from what we're doing now. Kris, do you have
> any ideas about this? 
>  
> > The two key risks I see are:
> >  - ReaderMode might allow web extensions to bypass the web extension
> > security model (access hosts which they don't have host permissions for). I
> > haven't found an attack that does this, but I feel like I'm very close.
> >  - ReaderMode might allow some form of access to semi-privileged contexts.
> > Gijs mostly convinced me above that this isn't a risk, but I'm a long way
> > from a technical understanding of why I am sure this is safe.
> > 
> > At the same time, I'm also doing an audit of all the semi-privileged
> > contexts we have in Firefox. And the more I learn, the less I like the idea
> > of Web Extensions having access to this world. For example, did you know you
> > can load certain pages from https://accounts.firefox.com in reader mode? We
> > are explicitly trying to block access to privileged web pages in web
> > extensions,so thats something we need to blck even if we use the iframe
> > sandbox approach.
> 
> If we are able to implement this so that we only allow content scripts to
> run for URLs for which the extension has access (via host permissions), as
> opposed to allowing scripts to run for any about:reader URL (which is the
> intent), would that alleviate the above concern? 

In order to inject a content script, a web extension MUST have the host permission for the url loaded in the reader mode page. Otherwise its a straight bypass of the web extension security model. My concern is that reader mode, and about:pages in general are not designed to be a trust boundary, so letting scripts interact with them probably has unexpected side effects.  (more below)

> Can a WebExtension
> currently run content scripts on https://accounts.firefox.com? If so, then 
> opening that access up via reader mode doesn't really change anything. If
> not, then it seems like it wouldn't be an issue.

No it can't (or shouldn't be allowed) - see bug 1415644. Technically the only pages that load are static, so maybe not an exploitable bug, but we should have explicit checks to prevent access.

The bigger point here is that the semi-privileged contexts are a mess, and allowing web extensions to inject scripts into one of them, is inviting trouble. How much trouble, its hard to say - if this is a really important feature, we might consider accepting the risk, but I would recommend the iframe-sandboxed approach (i.e. do 1204818 first). That is, unless anyone else (kris?) has ideas on how to _safely_ inject script into semi-privileged about pages.

Comment 32

[Bob Silverberg [:bsilverberg]]

(In reply to Paul Theriault [:pauljt] from comment #31)
> 
> The bigger point here is that the semi-privileged contexts are a mess, and
> allowing web extensions to inject scripts into one of them, is inviting
> trouble. How much trouble, its hard to say - if this is a really important
> feature, we might consider accepting the risk, but I would recommend the
> iframe-sandboxed approach (i.e. do 1204818 first). That is, unless anyone
> else (kris?) has ideas on how to _safely_ inject script into semi-privileged
> about pages.

As far as I know, nobody is clamouring for this ability. It's just something that we recognized as a potential addition to reader mode support for WebExtensions. I'll let Mike comment on how important this feature is, but I think we're totally fine to wait for bug 1204818 to land first, and then re-asses how safe this is to do.

Do you agree, Mike?

Comment 33

[T]

Re: nobody is clamoring for this ability -- I filed a (now merged into this) bug a few days ago (bug 1418412) because not being able to inject content scripts into reader-view-rendered pages is a problem for me. As a (****) workaround, I am including Mozilla's Readability library in the extension itself and basically re-creating an approximate reader-view for the users that I can interact with, and directing users not to use the built-in reader view mode. 

Obviously this means a workaround exists, but it's incredibly inelegant for a number of reasons.

Comment 34

[Sergio]

Reg. exp and multiple word highlighter addon author here, this is important, if you want people "clamoring" for this ability i can post this ticket url on my github issues but i'm sure you don't want people spamming me-too like comments here.

Comment 35

[Andy McKay]

Thank you to everyone for the discussion of the security concerns with this API. At this point I think the security concerns outweigh the benefits we expect to get from this API. If bug 1204818 lands, then we can re-evaluate this request.

Comment 36

[Mike Conca [:mconca] (Denver, CO, USA UTC-6)]

(In reply to T from comment #33)
> Re: nobody is clamoring for this ability -- I filed a (now merged into this)
> bug a few days ago (bug 1418412) because not being able to inject content
> scripts into reader-view-rendered pages is a problem for me. As a (****)
> workaround, I am including Mozilla's Readability library in the extension
> itself and basically re-creating an approximate reader-view for the users
> that I can interact with, and directing users not to use the built-in reader
> view mode. 
> 
> Obviously this means a workaround exists, but it's incredibly inelegant for
> a number of reasons.

T, thanks for your reply. I appreciate that you have a real use case for this feature. Unfortunately, the security and privacy concerns around this seem very real, and preserving a user's security and privacy is one of the pillars upon which the WebExtensions ecosystem is predicated. For the moment, unfortunately, we'll need to wait until we have better sandboxing before content scripts can inject into the about:reader page.

Comment 37

[T]

(In reply to Mike Conca [:mconca] (Denver, CO, USA UTC-6) from comment #36)
> T, thanks for your reply. I appreciate that you have a real use case for
> this feature. Unfortunately, the security and privacy concerns around this
> seem very real, and preserving a user's security and privacy is one of the
> pillars upon which the WebExtensions ecosystem is predicated. For the
> moment, unfortunately, we'll need to wait until we have better sandboxing
> before content scripts can inject into the about:reader page.

Understood. Just wanted to address the lack of clamoring :-).

Comment 39

[[e-mail]]

I like that reader can black theme sites for that oled power saving goodness.
If it were re-implemented more like a "private tab" where select plugins (uBlock) can be enabled that would be nice.
The re-implementation could also have the benefit of persisting on navigation.
I know it's hard to apply to all sites but it could be greatly improved from it's current state.

Comment 40

[Amir S.]

(In reply to Paul Theriault [:pauljt] from comment #31)
> The bigger point here is that the semi-privileged contexts are a mess, and
> allowing web extensions to inject scripts into one of them, is inviting
> trouble. How much trouble, its hard to say - if this is a really important
> feature, we might consider accepting the risk, but I would recommend the
> iframe-sandboxed approach (i.e. do 1204818 first). That is, unless anyone
> else (kris?) has ideas on how to _safely_ inject script into semi-privileged
> about pages.

Would it be acceptable to treat reader-mode itself similar to a web extension and disallow reader-mode entirely for for semi-privileged contexts?

Comment 41

[Paul Theriault [:pauljt]]

(In reply to Amir S. from comment #40)
> (In reply to Paul Theriault [:pauljt] from comment #31)
> > The bigger point here is that the semi-privileged contexts are a mess, and
> > allowing web extensions to inject scripts into one of them, is inviting
> > trouble. How much trouble, its hard to say - if this is a really important
> > feature, we might consider accepting the risk, but I would recommend the
> > iframe-sandboxed approach (i.e. do 1204818 first). That is, unless anyone
> > else (kris?) has ideas on how to _safely_ inject script into semi-privileged
> > about pages.
> 
> Would it be acceptable to treat reader-mode itself similar to a web
> extension and disallow reader-mode entirely for for semi-privileged contexts?

This did occur to me - I wondered if we could just rewrite reader mode as a normal web extension  - i.e. a web extension which injects a content script. That alone wouldn't allow other extensions to interact with reader mode, but they could interact with the page itself. There may be things that reader-mode does which aren't possible in a web extension though, I'm not sure.

Comment 33 seems to imply there were limitations, and I wonder what those were (other than confusing state of having two reader-modes)

Comment 42

[:Gijs (Not available 3-19 Aug; he/him)]

(In reply to Paul Theriault [:pauljt] from comment #41)
> (In reply to Amir S. from comment #40)
> > Would it be acceptable to treat reader-mode itself similar to a web
> > extension and disallow reader-mode entirely for for semi-privileged contexts?
> 
> This did occur to me - I wondered if we could just rewrite reader mode as a
> normal web extension  - i.e. a web extension which injects a content script.
> That alone wouldn't allow other extensions to interact with reader mode, but
> they could interact with the page itself. There may be things that
> reader-mode does which aren't possible in a web extension though, I'm not
> sure.

There are privileged things that the reader mode UI does (mostly storing persistent prefs for various reader mode things, but also invoking pocket, plus on mobile we cache readerized versions of articles for offline use). Putting that UI in an arbitrary webpage would mean that an arbitrary webpage could access those controls. Plus, if the arbitrary webpage was still principal'd as the arbitrary webpage, then the webpage could run script in reader mode. We don't want any of those things to be true.

Comment 43

[Jonathan Kingston [:jkt] (Away until 20th Aug)]

> Comment 33 seems to imply there were limitations, and I wonder what those were (other than confusing state of having two reader-modes)

Limitations that I see are:
- The reader mode has the extensions protocol, this likely would get fixed. There is other bugs for registering custom protocols too for making this neater
- Exploitability as Gijs has concerns over
- Slowing down pages on deciding whether to display the reader button (perhaps this could remain native as a hook for reader extensions?)

> mostly storing persistent prefs for various reader mode things

I can't see any prefs that look like they couldn't be migrated to an extensions storage.

> but also invoking pocket

Could pocket whitelist these activities to certain extensions?
- We could also look into an implementation of the Web Share API [1]

> plus on mobile we cache readerized versions of articles for offline use

This also could be in the extensions storage right?

> Putting that UI in an arbitrary webpage would mean that an arbitrary webpage could access those controls. Plus, if the arbitrary webpage was still principal'd as the arbitrary webpage, then the webpage could run script in reader mode. We don't want any of those things to be true.

This is the biggest concern for me. However extensions can block scripts and use the DOMParser parsing apis. It likely would load the content in a hidden iframe and parse out the bits we care about whilst blocking image loads for perf.

I think we should explore this idea some more.

[1] https://developers.google.com/web/updates/2016/09/navigator-share

Comment 44

[:Gijs (Not available 3-19 Aug; he/him)]

See bug 1324222 for some more issues with moving all the reader code to a webextension.

Comment 45

[MarjaE]

I don't know much about the security issues.

But reader view makes things more readable. I find it helpful to export readable epubs, so I'll have them on my computer, and so I'll be able to read them on my e-readers. So what I'm getting here is that 57+ may not allow users to do that-- either we go into readable view, and can't export, or we stay in standard view, and can export, but may not be able to read. I'm still using 52 esr so haven't encountered this problem yet.

Comment 46

[Andy McKay]

Marking [design-decision-denied] as per comment 35 until bug 1204818 lands.

Comment 47

[Jonathan Kingston [:jkt] (Away until 20th Aug)]

Gijs and I spoke about this again, I'm not even sure if we will ever be comfortable with this after the sandboxed iframe.

We should focus on getting telemetry for how often reader is seen and then decide what the next steps are. Removal, rewriting as a privileged extension or allowing extensions to replace the page etc. Bug 1350349

Comment 49

[thomas]

Addressing the security concerns that were raised in comment 29 I'm wondering whether it'd be beneficial to use a different protocol for reader mode (e.g. instead of "about:reader?url=<url>" use "read:<url>") so that the same-origin policy could be applied the same way to reader mode pages as to regular web pages. That could deal with the "bypass web extension security model" concern since web extensions could be required to specify "reader://example.com/*"-style host permissions.

Note that other browsers also use custom protocols for reader mode as pointed out at https://github.com/flattr/flattr-extension/issues/27 and the "read:" protocol had already been suggested before at https://bugzilla.mozilla.org/show_bug.cgi?id=1381992#c2.

Comment 50

[:Gijs (Not available 3-19 Aug; he/him)]

(In reply to thomas from comment #49)
> Addressing the security concerns that were raised in comment 29 I'm
> wondering whether it'd be beneficial to use a different protocol for reader
> mode (e.g. instead of "about:reader?url=<url>" use "read:<url>")

Given our current implementation of nested URIs, no [that wouldn't be beneficial]. See also bug 1430257.

Comment 51

[Kris Maglione [:kmag]]

Closing all open bugs with the [design-decision-denied] whiteboard flag.

Comment 52

[:Gijs (Not available 3-19 Aug; he/him)]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #51)
> Closing all open bugs with the [design-decision-denied] whiteboard flag.

I think we would still eventually like to do something here, but only once the visible content is safely sandboxed away from the rest of the reader mode code.