Allow mozbrowser iframes to load content from others protocols than their embedders

NEW
Assigned to

Status

()

Core
Security: CAPS
2 years ago
2 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox50 affected)

Details

(Whiteboard: planula)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Currently, <iframe mozbrowser> does not allow to load chrome:// or about:// uris if their embedder is served from app:// or http://.

It would be useful to relax this to allow prototyping html based browsers. Since mozbrowser iframes are only allowed if the 'mozbrowser' permission is granted, and since there is no UI to allow such a thing it does not seems too scary.
Created attachment 8764229 [details]
Bug 1281440 - Allow mozbrowser iframes to load content from others protocols than their embedders.

Review commit: https://reviewboard.mozilla.org/r/60230/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60230/
Attachment #8764229 - Flags: review?(bzbarsky)
So just to be clear, this allows any principal that has the "browser" permission to load any subresource from anywhere it wants.

Under what conditions is the "browser" permission set on a principal?
Flags: needinfo?(21)
Comment on attachment 8764229 [details]
Bug 1281440 - Allow mozbrowser iframes to load content from others protocols than their embedders.

Anyway, I'd like bholley to sign off on this.
Attachment #8764229 - Flags: review?(bzbarsky) → review?(bobbyholley)
Comment on attachment 8764229 [details]
Bug 1281440 - Allow mozbrowser iframes to load content from others protocols than their embedders.

https://reviewboard.mozilla.org/r/60230/#review57128

No way. Why on earth would it be a good idea to allow unprivileged code to load chrome:// and protected-about:// URIs? We've been painstakingly removing all these scary special-cases from the loading privilege code, so I'm not keen to add another one.
Attachment #8764229 - Flags: review?(bobbyholley) → review-
(In reply to Boris Zbarsky [:bz] (Out June 25-July 6) from comment #2)
> Under what conditions is the "browser" permission set on a principal?

It it set manually using the permission manager. Afaik there is no other way to set this permission.
Flags: needinfo?(21)
(In reply to Bobby Holley (busy) from comment #5)
> Comment on attachment 8764229 [details]
> Bug 1281440 - Allow mozbrowser iframes to load content from others protocols
> than their embedders.
> 
> https://reviewboard.mozilla.org/r/60230/#review57128
> 
> No way. Why on earth would it be a good idea to allow unprivileged code to
> load chrome:// and protected-about:// URIs?

In the case where the top level document is an html document served from https instead of chrome://browser/content.

It allows rapid prototyping of new UIs/concepts on top of Gecko directly, switching the browser ui on the fly, and refreshing it from a remote or a local web server instead of having to distribute an update.

Prototyping a browser UI is quick and easy when you only add tabs support, but an incredible amount of work when you need to implement every features. The goal here is to let the top level document load pages such as about:home, about:memory and potentially about:devtools (when it will land), ... All of this in order to reduce the amount of work.

Then in order to get about:home features to work, we would need to be able to load chrome:// uris, such as the one related to bookmarks.

One other goal would be to let those iframes load moz-extension: content as our HTML based UI prototype has also web extensions support.

I understand that there are security risks to allow non system-principal content to use mozbrowser. But this would still be privileged code if you consider that the mozbrowser permission should be granted.

I can explain it more in details if you are open to discussion. Or do a video demo of what :ochameau and myself want to build.
(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #7)
> (In reply to Bobby Holley (busy) from comment #5)
> > Comment on attachment 8764229 [details]
> > Bug 1281440 - Allow mozbrowser iframes to load content from others protocols
> > than their embedders.
> > 
> > https://reviewboard.mozilla.org/r/60230/#review57128
> > 
> > No way. Why on earth would it be a good idea to allow unprivileged code to
> > load chrome:// and protected-about:// URIs?
> 
> In the case where the top level document is an html document served from
> https instead of chrome://browser/content.
> 
> It allows rapid prototyping of new UIs/concepts on top of Gecko directly,
> switching the browser ui on the fly, and refreshing it from a remote or a
> local web server instead of having to distribute an update.
> 
> Prototyping a browser UI is quick and easy when you only add tabs support,
> but an incredible amount of work when you need to implement every features.
> The goal here is to let the top level document load pages such as
> about:home, about:memory and potentially about:devtools (when it will land),
> ... All of this in order to reduce the amount of work.
> 
> Then in order to get about:home features to work, we would need to be able
> to load chrome:// uris, such as the one related to bookmarks.
> 
> One other goal would be to let those iframes load moz-extension: content as
> our HTML based UI prototype has also web extensions support.
> 
> I understand that there are security risks to allow non system-principal
> content to use mozbrowser. But this would still be privileged code if you
> consider that the mozbrowser permission should be granted.
> 
> I can explain it more in details if you are open to discussion. Or do a
> video demo of what :ochameau and myself want to build.

Now that I think about it again. 

I realize that what :ochameau and myself are trying to build may be very confusing when summarized the way I did. 
And since I have a few other patches that may get you nuts if you don't have any ideas about our goals I really believed it will be a good idea to catch up at some point so I can make a small demo to you.
I wish I was good enough to explain it clearly in a bug but I have never been good at this kind of exercise :(

Then you may take a more informed decision about this bug and/or suggest me an other way to reach my goal.
If this is just for prototyping, why not inject a privileged utility function into the target context that allows it to load and navigate to restricted pages? Is any of this actually landing in-tree?
(In reply to Bobby Holley (busy) from comment #9)
> If this is just for prototyping, why not inject a privileged utility
> function into the target context that allows it to load and navigate to
> restricted pages? 

I'm not sure about what you're proposing tbh.

> Is any of this actually landing in-tree?

Not yet. This patch is one of the first patch of a series we would like to land in m-c.


Just to make sure you understand some parts of what we want to build, ochameau made a blog post about it: http://techno-barje.fr/post/2016/06/28/html-experiments/

Let me know what you think.
(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #10)
> (In reply to Bobby Holley (busy) from comment #9)
> > If this is just for prototyping, why not inject a privileged utility
> > function into the target context that allows it to load and navigate to
> > restricted pages? 
> 
> I'm not sure about what you're proposing tbh.

I am suggestion that the your addon use cloneInto/exportFunction/postMessage to expose the desired functionality to the privileged page.

> Just to make sure you understand some parts of what we want to build,
> ochameau made a blog post about it:
> http://techno-barje.fr/post/2016/06/28/html-experiments/
> 
> Let me know what you think.

In general it sounds like a great idea - I just don't think this change to the security policy is either necessary or desirable.
OK. Would you feel more comfortable if I move the change into nsFrameLoader::CheckURILoad. So instead of allowing everything, we would allow mozbrowser iframes to load different protocols ?
(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #12)
> OK. Would you feel more comfortable if I move the change into
> nsFrameLoader::CheckURILoad. So instead of allowing everything, we would
> allow mozbrowser iframes to load different protocols ?

No, my objection is that it's bad security practice to associated dangerous capabilities with an unrelated permission just because it works with the consumers we have now. Nothing about the mozbrowser permission implies that we should suddenly be able to load platform-internal URIs.

I've suggested a solution several times in this bug - can you explain why it's insufficient?
(In reply to Bobby Holley (busy) from comment #13)
> (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> needinfo? please) from comment #12)
> > OK. Would you feel more comfortable if I move the change into
> > nsFrameLoader::CheckURILoad. So instead of allowing everything, we would
> > allow mozbrowser iframes to load different protocols ?
> 
> No, my objection is that it's bad security practice to associated dangerous
> capabilities with an unrelated permission just because it works with the
> consumers we have now. Nothing about the mozbrowser permission implies that
> we should suddenly be able to load platform-internal URIs.

Fair enough. I have nothing against adding a new permission specifically dedicated to that fwiw.

> 
> I've suggested a solution several times in this bug - can you explain why
> it's insufficient?

I guess it is related to the fact that I would like to add something transparent for consumers.

By transparent I mean I would like them to just have to do iframe.src = 'about:preferences' without having to call a strange method that send a message to the child process to tell it to do content.location = 'about:preferences' from a frame script or something similar.

It is also related to the fact that I don't want to expose this via an addon as one of our goal is to ultimately get rid of the addon and I would much rather prefer have something in-tree.

It should likely be related to my lack of understanding about all the ramifications of such patches, but I do not see the differences between exposing a method that allow a browsing context to load a privileged url, versus allow that from the frameLoader side directly ?
(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #14)
> (In reply to Bobby Holley (busy) from comment #13)
> > (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> > needinfo? please) from comment #12)
> > > OK. Would you feel more comfortable if I move the change into
> > > nsFrameLoader::CheckURILoad. So instead of allowing everything, we would
> > > allow mozbrowser iframes to load different protocols ?
> > 
> > No, my objection is that it's bad security practice to associated dangerous
> > capabilities with an unrelated permission just because it works with the
> > consumers we have now. Nothing about the mozbrowser permission implies that
> > we should suddenly be able to load platform-internal URIs.
> 
> Fair enough. I have nothing against adding a new permission specifically
> dedicated to that fwiw.

That may be the correct long-term answer, but we'll want to discuss the requirements in detail, and do a security review, and all that stuff. Some of those questions might be "wouldn't it be better to make this thing we're trying to load unprivileged?", which may require additional engineering. Seems premature to do all that now.

> > 
> > I've suggested a solution several times in this bug - can you explain why
> > it's insufficient?
> 
> I guess it is related to the fact that I would like to add something
> transparent for consumers.
> 
> By transparent I mean I would like them to just have to do iframe.src =
> 'about:preferences' without having to call a strange method that send a
> message to the child process to tell it to do content.location =
> 'about:preferences' from a frame script or something similar.

I recognize that it's not ergonomic, but it seems like a small price to pay given that you're currently prototyping things and this isn't something we've committed to shipping.

Basically, the security code has long been a dumping ground for special cases and exceptions that people added to support some project at some point, and it's never clear how important those cases are or how often they apply, which makes it really difficult to audit and simplify the security model. Our security around loading permissions is particularly insane, which is why I'm being so stubborn about this.
 
> It is also related to the fact that I don't want to expose this via an addon
> as one of our goal is to ultimately get rid of the addon and I would much
> rather prefer have something in-tree.

Ultimately sure - I'm just saying let's solve this problem when this is something we're actually going to ship.

> It should likely be related to my lack of understanding about all the
> ramifications of such patches, but I do not see the differences between
> exposing a method that allow a browsing context to load a privileged url,
> versus allow that from the frameLoader side directly ?

Because it significantly restricts the scope of the change (you'd presumably just expose this to your particular scope, not every scope with the mozbrowser permission), doesn't expose users to additional risk while you're prototyping (presumably it would only take effect in environments where your stuff is enabled), and is more likely to evolve along with your project requirements rather than sitting in the tree indefinitely (since it lives in your code rather than in the platform).
You need to log in before you can comment on or make changes to this bug.