Closed Bug 1485778 Opened 6 years ago Closed 6 years ago

New tab local file security error since bug 1362034

Categories

(WebExtensions :: Untriaged, defect)

63 Branch
defect
Not set
normal

Tracking

(firefox63 affected)

RESOLVED WONTFIX
Tracking Status
firefox63 --- affected

People

(Reporter: gingerbread_man, Unassigned)

References

Details

(Keywords: regression)

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0
20180823100106

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e17409c5199c0ade0de9899782d6a9f5f4970871&tochange=6f3907a1da34a62133832dc981e9005ee30e9388

STR
1. Install New Tab Context.
https://addons.mozilla.org/firefox/addon/new-tab-context/
2. Set up userChrome.js to load a local file in each new tab.
https://luke-baker.github.io
3. Restart Firefox for the above to take effect.
4. Right-click a tab and choose "New Tab".
5. Click Firefox's + button on the toolbar to create a new tab.

Actual results
4. The new tab is blank. Browser Console says
"Security Error: Content at moz-extension://… may not load or link to file:///…."
5. The new tab loads, with the local file in it.

Expected results
Same behavior as before the regression: the new tab loads, with the local file in it. The extension itself only creates a new tab. It doesn't try to load anything specific in it.
Flags: needinfo?(jkt)
This is failing on the code I added that creates a codebase principal. The question is, do we really wish to continue supporting this?

This code is manually calling the aboutNewTabService itself to register the file path: https://luke-baker.github.io/chrome/NewTab_custom-page.uc.js
Flags: needinfo?(jkt) → needinfo?(gijskruitbosch+bugs)
Priority: -- → P3
(In reply to Jonathan Kingston [:jkt] from comment #1)
> This is failing on the code I added that creates a codebase principal. 

Which specific bit?

> The question is, do we really wish to continue supporting this?

Does the same thing happen if you use an add-on to control the new tab page?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jkt)
> principal = Services.scriptSecurityManager.createCodebasePrincipal(Services.io.newURI(url), {

From: https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/browser/components/extensions/parent/ext-tabs.js#579

> Does the same thing happen if you use an add-on to control the new tab page?

Nope we have tests for that (I will double check).

It's specifically just an extension unable to open file:// that has been set as a homepage.
Flags: needinfo?(jkt)
(In reply to Jonathan Kingston [:jkt] from comment #3)
> It's specifically just an extension unable to open file:// that has been set
> as a homepage.

Once again,
1. The extension itself merely opens a new tab. It doesn't try to load anything in it. It doesn't do anything to the homepage either.
2. The userChrome.js script loads the local file in each new tab. This isn't a supported setup, but it still works just fine when done via Firefox itself, e.g. + button, Ctrl+T, File → New Tab

(In reply to Jonathan Kingston [:jkt] from comment #1)
> The question is, do we really wish to continue supporting this?

Why is it a technical necessity to blame the extension for something it's not trying to do, and prevent it from doing the same thing Firefox can?

The extension wouldn't be necessary if Firefox had the menu item other browsers have (bug 623159, bug 1382929). The userChrome.js script wouldn't be necessary if a built-in feature hadn't been axed (bug 1118285, bug 1452144) or add-ons were able to load local files (bug 1246236, I guess). After having to painstakingly resort to these kludges, I'd at least like to know why it's so important to break them.
(In reply to Gingerbread Man from comment #4)
> (In reply to Jonathan Kingston [:jkt] from comment #3)
> > It's specifically just an extension unable to open file:// that has been set
> > as a homepage.
> 
> Once again,
> 1. The extension itself merely opens a new tab. It doesn't try to load
> anything in it. It doesn't do anything to the homepage either.

This isn't correct. It tries to open a new tab *with a particular URL*. The URL is a file URL. So the security checks fail, because the add-on isn't allowed to open such a URL. If it tried to update the URL of an existing tab with that URL, it'd also fail.

I understand that this is hard to follow from the perspective of the user, but the essential reason this is breaking is that we're instrumenting essentially every point at which we load *anything* with an indication of the privilege level of the thing that triggered that load. The privilege level of the user interacting with buttons in the core Firefox UI is normally "system", ie "anything goes". But in this case, the tab is opened by the add-on. At the point where the add-on opens it, we don't know anymore whether that call came via user interaction or not, and so it gets the add-on's level of privilege, and the add-on can't open the URL, and that's that.

> 2. The userChrome.js script loads the local file in each new tab. This isn't
> a supported setup, but it still works just fine when done via Firefox
> itself, e.g. + button, Ctrl+T, File → New Tab

Yes, because as said, the user interaction with core browser functionality gets "anything goes" levels of privilege.

> (In reply to Jonathan Kingston [:jkt] from comment #1)
> > The question is, do we really wish to continue supporting this?
> 
> Why is it a technical necessity to blame the extension for something it's
> not trying to do,

It *is* trying to open the custom URL that the userChrome.js set. It might not be checking what that URL is, but that's what it's doing. So it's being blamed for exactly what it is trying to do. The fact that the user chrome script changed what the thing is that the add-on is trying to do doesn't make a difference to the facts of the matter, or that security checks should block it.

> and prevent it from doing the same thing Firefox can?

We prevent add-ons from doing lots of things that Firefox can do. That's kind of the point of the webextensions privilege model - we're able to control what add-ons can and can't do.

> The extension wouldn't be necessary if Firefox had the menu item other
> browsers have (bug 623159, bug 1382929). The userChrome.js script wouldn't
> be necessary if a built-in feature hadn't been axed (bug 1118285, bug
> 1452144) or 

Why do you need a local file as a homepage? Why can't you just put that file on a webserver or in the add-on itself? It would also be fairly easy to write an add-on that basically gave you an HTML editor for the new tab page, or that let you use an HTML browse button to load the contents of a file and use that as a new tab page. The only thing that'd be different from a file: page is the content process it loads in (file: pages have their own process) and that it wouldn't be able to link to other file:/// pages.

> After having to painstakingly resort to these kludges, I'd at least like to
> know why it's so important to break them.

Because add-ons breaking out of their security model is a privilege escalation risk, ie risks creating security flaws in Firefox.

> add-ons were able to load local files (bug 1246236, I guess).

My understanding is that we do want to support this to some degree, but it might be behind extra permissions. That's not trivial to implement, and that's why it hasn't happened yet.


I think we should dupe this to bug 1246236.
I think the final decision is up to the webext folks. Rob, can you pass this on to the right people? Thanks!
Component: DOM: Security → Untriaged
Flags: needinfo?(rob)
Priority: P3 → --
Product: Core → WebExtensions
(In reply to Jonathan Kingston [:jkt] from comment #3)
> > principal = Services.scriptSecurityManager.createCodebasePrincipal(Services.io.newURI(url), {
> 
> From:
> https://searchfox.org/mozilla-central/rev/
> c45b9755593ce6cc91f558b21e544f5165752de7/browser/components/extensions/
> parent/ext-tabs.js#579

I'm confused. That opens a `url` with a codebase principal that matches the `url`. That can't be what's going on here, because the target URL is file:/// and then the principal would also be for `file:///`. What am I missing?
Flags: needinfo?(jkt)
(In reply to :Gijs (he/him) from comment #7)
> (In reply to Jonathan Kingston [:jkt] from comment #3)
> > > principal = Services.scriptSecurityManager.createCodebasePrincipal(Services.io.newURI(url), {
> > 
> > From:
> > https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/browser/components/extensions/parent/ext-tabs.js#579
> 
> I'm confused. That opens a `url` with a codebase principal that matches the
> `url`. That can't be what's going on here, because the target URL is
> file:/// and then the principal would also be for `file:///`. What am I
> missing?

When aboutNewTabService.newTabURL is overridden to file:///, this logic is executed:

https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/browser/components/extensions/parent/ext-tabs.js#584-585

and since context.principal is the extension's, the load is blocked.
IF using file:-URLs as a new Tab URL is an explicitly supported feature of Firefox, then the condition at lines 574 should also be false when options.url is not set by the extension, i.e. this branch is taken:
https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/browser/components/extensions/parent/ext-tabs.js#567

The principal originally added in bug 1378647:
https://searchfox.org/mozilla-central/diff/aad6d3674f408b409b27b8c294515800c28aad1f/browser/components/extensions/parent/ext-tabs.js#line-571
(and removed then restored later by jkt).

Shane: What should be the resolution of this bug report?
Flags: needinfo?(rob) → needinfo?(mixedpuppy)
In tabs.create we should probably do:

let discardable = !url.startsWith("about:") && !url.startsWith("file:");
Flags: needinfo?(mixedpuppy)
(In reply to :Gijs (he/him) from comment #5)
> At the point where the add-on opens it, we don't know anymore
> whether that call came via user interaction or not, and so it gets the
> add-on's level of privilege, and the add-on can't open the URL, and that's
> that.

It seems like a shortcoming for Firefox not to know that a click on a tab context menu is user interaction.

> Why do you need a local file as a homepage? Why can't you just put that file
> on a webserver or in the add-on itself?

The file isn't static; it changes frequently. In particular, a scheduled task modifies images used at regular intervals. Discussion at bug 1426954.

> It would also be fairly easy to write an add-on that basically gave you an HTML editor
> for the new tab page, or that let you use an HTML browse button to load the contents of a file and
> use that as a new tab page.

Such an add-on already exists (and is of no use to me):
https://addons.mozilla.org/firefox/addon/new-tab-override/

> The fact that the user chrome script changed what the thing is that
> the add-on is trying to do doesn't make a difference to the facts of
> the matter, or that security checks should block it.

Thank you. I certainly don't agree, but at least I can be sure the situation is correctly understood and the behavior is deemed appropriate.
(In reply to Gingerbread Man from comment #10)
> (In reply to :Gijs (he/him) from comment #5)
> > At the point where the add-on opens it, we don't know anymore
> > whether that call came via user interaction or not, and so it gets the
> > add-on's level of privilege, and the add-on can't open the URL, and that's
> > that.
> 
> It seems like a shortcoming for Firefox not to know that a click on a tab
> context menu is user interaction.

It does know that - but the webextension APIs are deliberately asynchronous, and at the point where the add-on opens a tab, for sure the 'user interaction' flag will no longer be set because (I expect) it's not called synchronously from the point where the user clicks things.

In any case, even if it was, we probably don't want to allow add-ons to do 'whatever' whenever the user interacts with an add-on menuitem...

These are all hard trade-offs, but clearly the line needs to be "somewhere", and in this particular case the line is clearly somewhere where it breaks your workflow, and I understand that that is unfortunate. :-(

> > Why do you need a local file as a homepage? Why can't you just put that file
> > on a webserver or in the add-on itself?
> 
> The file isn't static; it changes frequently. In particular, a scheduled
> task modifies images used at regular intervals. Discussion at bug 1426954.

That discussion is helpful. Still feels like something that could be done without resorting to file: URLs (e.g. you could just make the periodic updater create a new copy of the add-on that ships with the images you want?), but I understand it might be tricky. Probably best off discussing that on the other bug...

> > The fact that the user chrome script changed what the thing is that
> > the add-on is trying to do doesn't make a difference to the facts of
> > the matter, or that security checks should block it.
> 
> Thank you. I certainly don't agree, but at least I can be sure the situation
> is correctly understood and the behavior is deemed appropriate.

Well, it seems the webextension folks want to carve you out an exception here... I defer to them + jkt on whether that is wise.

My main concern is that we need to avoid a situation where an arbitrary webextension can do something like the following:

1) set the new tab page to load file:///etc/passwd
2) open a new tab, and thereby file:///etc/passwd
3) run a webext content script on that page.

It sounds like today, (1) is impossible, as well as (2) (this bug). I guess even if we make (2) possible, it'd still require 1 level of explicit user opt-in (ie the user script to set the new tab URL that way), which is at least as onerous as the user having to upload the same file into an <input type=file> thing... I just don't like making this any easier. :-)
Note: The bug here has a very specific scope, and the concerns from the above comments are not as relevant.

The bug here is: Extensions cannot open a local file as the new tab page, even if the user has explicitly set this as the new tab.

Minimal STR:
1. Open global JS console, run:
   aboutNewTabService.newTabURL = "file:///tmp/";
2. From an extension (no permissions needed), call:
   browser.tabs.create({});

Expected (Firefox 61, 62):
- New tab should open, showing content from "file:///tmp/"

Actual (Firefox 63):
- Tab is blank, console shows "Security Error: Content at moz-extension://ef6d7519-f2fb-4eb5-8eba-7becd0f6af28/ may not load or link to file:///tmp"

Another member of my team acknowledges that this bug needs to be fixed (see comment 9).
> let discardable = !url.startsWith("about:") && !url.startsWith("file:");

Extensions can't load files anywhere else, we are trying to prevent this explicitly where possible. In the examples Gijs gave this code change has prevented 1) from ever happening.
If we allow this we should try to make that condition explicit only to this use-case only.

So perhaps:

 let discardable = url && !url.startsWith("about:") && !(aboutNewTabService.newTabURL == url && url.startsWith("file:"));

> Note: The bug here has a very specific scope, and the concerns from the above comments are not as relevant.

tbh I think they are relevant, unless we are very explicit with that condition we open up the risk to allowing extensions to open file:// urls.

I personally would prefer we didn't open up this edge case:
- As you can see from that condition it's pretty hard to reason about.
- Given that this exact use-case can be solved with using localhost and instead using a simple web server that reads the file per request.
- The risk of opening this could increase the risk of file access by extensions to sensitive OS files.
Flags: needinfo?(jkt)
FYI, whether an extension can access a URL (including `file:` URLs) is already validated at https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/browser/components/extensions/parent/ext-tabs.js#557

So just checking for file: would be sufficient. Alternatively, set the condition to true at https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/browser/components/extensions/parent/ext-tabs.js#565

> Extensions can't load files anywhere else, we are trying to prevent this explicitly where possible.

> - The risk of opening this could increase the risk of file access by extensions to sensitive OS files.

Extensions can only override the new tab page by overriding it with a moz-extension:-URL. They cannot set it to any other URL.
When the new tab URL is set to a local file, the user (or external software) has done that themselves.
Opening a new tab is a common user action, and allowing extensions to open the default new tab (with no control over its URL) does not seem to be a high risk.


Let's also ask for the opinion from product:
When the user has set the new tab page to a file:-URL, should we display a blank page instead of that file:-URL when an extension creates a new tab?
Flags: needinfo?(mconca)
(In reply to Rob Wu [:robwu] from comment #14)
> FYI, whether an extension can access a URL (including `file:` URLs) is
> already validated at
> https://searchfox.org/mozilla-central/rev/
> 55da592d85c2baf8d8818010c41d9738c97013d2/browser/components/extensions/
> parent/ext-tabs.js#557

The thing is, ideally we should get rid of these manual checks and replace them with passing the correct principals to the APIs that actually load pages. That avoids a situation where all the consumers have to try to roll their own security checks, and inevitably end up making mistakes.

> Let's also ask for the opinion from product:
> When the user has set the new tab page to a file:-URL, should we display a
> blank page instead of that file:-URL when an extension creates a new tab?

Alternatively, we could either load about:newtab or avoid creating the tab.


Anyway, as long as we don't have plans to make it possible for add-ons to change the new tab URL to anything other than public (http/https) URIs or their own webextension URIs, I can live with punching holes in the restrictions here, but we do need to be quite careful about how we do that. I'd feel better if it was done behind the extra permissions being planned in bug 1246236, but I guess we can potentially add those restrictions after the fact if we think fixing this is urgent.


Also, what does the current state of things mean for the following situation:

1) add-on A sets new tab page to its add-on page, ie moz-extensions://{guid-for-add-on-a}/some/page.html .
2) add-on B calls tabs.create() without a URL

Do we refuse to open the page in that case, too?
> Do we refuse to open the page in that case, too?

I know we handle this as we explicitly have tests for it. However I'm not 100% on how.
> Also, what does the current state of things mean for the following situation:
>
> 1) add-on A sets new tab page to its add-on page, ie moz-extensions://{guid-for-add-on-a}/some/page.html .
> 2) add-on B calls tabs.create() without a URL

I was having a tired afternoon it seems.

2) calls tabs.create()
   - this then translates to url = moz-extensions://{guid-for-add-on-a}/some/page.html https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/browser/components/extensions/parent/ext-tabs.js#565
   - this then gets the extension.principal for 2)
   - extension 2) triggeringPrincipal is able to open the url as it's not chrome:// file:// about: etc.
   - From there this principal goes into scriptSecurityManager which prevents different codebase principals loading other protocols.
This entire bug is predicated on setting the new tab URL to a file via userChrome.js.  This is not a supported mechanism, which the reporter notes in Comment 4. Firefox limits the ability to load file:// URLs in many other places, and security has specifically recommended against this change in Comment 13.

We do not allow the new tab page to be set to a file via the WebExtensions API.
We do not allow users to set the new tab page to a file via the user interface (about:preferences).
In short, there currently is no supported method by which we currently allow the new tab page to be set to a file.

Punching holes in Firefox security to allow the new tab page to be set to a file via an unsupported mechanism does not seem like a good idea. It would be better to implement that feature via an officially supported mechanism, of which bug 1452144 and bug 1246236 are two possibilities.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(mconca)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.