Closed Bug 1145470 Opened 9 years ago Closed 7 years ago

Remove the hiddenwindow special case in CheckLoadURIWithPrincipal

Categories

(Core :: Security, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: Gijs)

References

Details

(Keywords: addon-compat)

Attachments

(1 file)

Need to fix the social API to stop depending on that hackery first.
Flags: needinfo?(mhammond)
Depends on: 1145472
The long term goal here was always to replace the frameworker with real workers - bug 898706.  Bug 936302 seems to be the main blocker here.  Sadly it seems that at least 4 social providers (pocket, mixi, wiebo, cliqz) depend on these workers for their social integration.
Flags: needinfo?(mhammond)
The dep got fixed, so presumably we can do this now? Try push on OSX and linux64 to see if I'm right:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=78e2040b5043
(In reply to :Gijs Kruitbosch from comment #2)
> The dep got fixed, so presumably we can do this now? Try push on OSX and
> linux64 to see if I'm right:
> 
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=78e2040b5043

Well, so much for that, looks like the tab thumbnail/preview code tries to use it:

13:04:13     INFO -  80 INFO Console message: Security Error: Content at resource://gre-resources/hiddenWindow.html may not load or link to chrome://global/content/mozilla.xhtml.

http://archive.mozilla.org/pub/firefox/try-builds/gijskruitbosch@gmail.com-78e2040b5043d303a7a9e5e815143dea0d533fb9/try-linux64/try_ubuntu64_vm_test-mochitest-e10s-browser-chrome-3-bm118-tests1-linux64-build200.txt.gz
Depends on: 1288030
Depends on: 1351300
Depends on: 1338215
(In reply to :Gijs from comment #4)
> Maaaaybe we can do this after bug 1351300 lands, let's find out:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=5c9dbdad131caa31459d3700ce3e4c8d12b81633

This looks green enough, patch incoming.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8852838 [details]
Bug 1145470 - remove hiddenWindow specialcasing from CheckLoadURI code,

https://reviewboard.mozilla.org/r/125004/#review127710

Gijs++
Attachment #8852838 - Flags: review?(bobbyholley) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f62917e98036
remove hiddenWindow specialcasing from CheckLoadURI code, r=bholley
https://hg.mozilla.org/mozilla-central/rev/f62917e98036
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
https://dxr.mozilla.org/addons/search?q=hiddendomwindow&=mozilla-central suggests that there's going to be at least some add-on breakage from this. Not everything people do with this window will break, but expecting it to be effectively chrome-privileged *will* break. Triaging all the 12,000 hits doesn't seem feasible.

I'm pretty comfortable with some breakage - webextensions give people other options, and if people are still maintaining XUL/XPCOM-based addons they can use the createWindowlessBrowser() APIs like our own consumers have done. If we find out (probably not until beta, because eh) that this breaks half the world, it should be reasonably straightforward to back this out at that stage, but if at all possible I'd prefer not to do that.

Jorge, how do I get this relnoted for compat ASAP? :-)
Flags: needinfo?(jorge)
Keywords: addon-compat
Looks like this is why uBO isn't working for me today on m-c tip...
Security Error: Content at resource://gre-resources/hiddenWindow.html may not load or link to chrome://ublock0/content/background.html#1.11.4.
(In reply to :Gijs from comment #11)
> Jorge, how do I get this relnoted for compat ASAP? :-)

We're currently working on the Firefox 54 compat comms, so 55 is a few weeks away.

If this causes significant breakage in add-ons, I would ask if this is something that can be put on hold until 57. Many legacy add-ons won't be ported to WebExtensions and those developers won't have much motivation to keep their add-ons working for only a couple of cycles.
Flags: needinfo?(jorge)
(In reply to Jorge Villalobos [:jorgev] from comment #13)
> (In reply to :Gijs from comment #11)
> > Jorge, how do I get this relnoted for compat ASAP? :-)
> 
> We're currently working on the Firefox 54 compat comms, so 55 is a few weeks
> away.
> 
> If this causes significant breakage in add-ons, I would ask if this is
> something that can be put on hold until 57.

I don't know how large the breakage will be, see comment #11 . Really, only 1 way to find out.

> Many legacy add-ons won't be
> ported to WebExtensions and those developers won't have much motivation to
> keep their add-ons working for only a couple of cycles.

Well, uBO at least already has a webextension in the works, AIUI. From a security sanity perspective, I would like to see this code gone, and the problem will only get worse once I try to also fix bug 1320124 (which is getting close now)... but I defer to bz/bholley as to how important this is (as security hardening / defense in depth) in the grand scheme of things.
(In reply to :Gijs from comment #14)
> (In reply to Jorge Villalobos [:jorgev] from comment #13)
> > (In reply to :Gijs from comment #11)
> > > Jorge, how do I get this relnoted for compat ASAP? :-)
> > 
> > We're currently working on the Firefox 54 compat comms, so 55 is a few weeks
> > away.
> > 
> > If this causes significant breakage in add-ons, I would ask if this is
> > something that can be put on hold until 57.
> 
> I don't know how large the breakage will be, see comment #11 . Really, only
> 1 way to find out.
> 
> > Many legacy add-ons won't be
> > ported to WebExtensions and those developers won't have much motivation to
> > keep their add-ons working for only a couple of cycles.
> 
> Well, uBO at least already has a webextension in the works, AIUI. From a
> security sanity perspective, I would like to see this code gone, and the
> problem will only get worse once I try to also fix bug 1320124 (which is
> getting close now)... but I defer to bz/bholley as to how important this is
> (as security hardening / defense in depth) in the grand scheme of things.

I would like to see this code go, but I don't think it's worth rocking the boat over it before XPCOM addons are gone for good, given that's only a few months away. So I agree with jorge's assessment here, assuming we do think there's a reasonable risk that this will break addons.
Depends on: 1352513
This breaks Forecastfox (fix version) which is a very popular extension.
Depends on: 1352910
No longer depends on: 1352866
IMHO the bug 1145470 fixed wrong currently, because for the hidden window should be "allowed to load URI no matter what.", see the source code (https://hg.mozilla.org/mozilla-central/annotate/09ee763947c3/caps/nsScriptSecurityManager.cpp#l909).
But currently it occurs only if "security.allow_chrome_frames_inside_content" set to true.
I'm not sure that is good idea and possibly this exception for `sourceSpec.EqualsLiteral("resource://gre-resources/hiddenWindow.html")` should take place regardless of value `sCanLoadChromeInContent` (which is the value of security.allow_chrome_frames_inside_content).

Thus ATM the only way to allow such (very many) plugings in ffox >= 55 is to set (in about:config): security.allow_chrome_frames_inside_content = true.
(In addition to `browser.tabs.remote.autostart = false` for plugins not compatible with multi-processing).

If it wanted so, then why this preference (security.allow_chrome_frames_inside_content) was set to "false" per default?
I mean why EACH new firefox version should be ALWAYS incompatible?

@Mozilla: how long you will continue to scoff the developers of addons? Almost each new version of firefox breaks the compatibility of many addons (if a bit larger as "Hello world").
The only reason, why I (and many other) still don't switched to chromium and co, is the Ubiquity plugin, but it will be always more complex to maintain it (as regards firefox upgrade), just take a look https://bitbucket.org/satyr/ubiquity/issues/63/ubiquity-and-electrolysis
(In reply to sebres from comment #17)

The previous default was a security risk, and this is part of the reason why it is now turned off by default. It's not reasonable for the majority of users to have a less secure browser so that a minority of users can run add-ons that are out of date.

The pref wasn't originally planned, and the only reason it was added was so we could react if a *lot* of add-ons broke. But during beta I haven't heard or seen any complaints, and so we shipped with the pref turned off.

> Thus ATM the only way to allow such (very many) plugings in ffox >= 55 is to
> set (in about:config): security.allow_chrome_frames_inside_content = true.

Which "very many" plugins does this affect? Can you provide a list? The ticket you linked to doesn't mention any other add-ons besides ubiquity.

> (In addition to `browser.tabs.remote.autostart = false` for plugins not
> compatible with multi-processing).

Multi-processing (to a very large degree) and this fix (to a much smaller degree) both contribute to a more secure version of Firefox. Turning both of them off deliberately to run legacy code isn't a configuration we will invest a lot of time to support. The add-ons in question are all going to break completely with the release of Firefox 57, see also e.g. https://blog.mozilla.org/addons/2017/08/10/upcoming-changes-compatibility/ . We have been making quite a lot of noise about this for a while now.

> If it wanted so, then why this preference
> (security.allow_chrome_frames_inside_content) was set to "false" per default?
> I mean why EACH new firefox version should be ALWAYS incompatible?

This isn't my experience of updating Firefox, nor is it that of most users. But ironically, part of how we are making it easier for add-ons to remain compatible with future versions of Firefox is by the move to webextensions - a move which these same add-ons that you're having issues with are apparently not currently making. WebExtensions provide more stable APIs that are less "close to the metal" and therefore more stable in the face of internal changes in Firefox itself.

> Almost each new version of firefox breaks the compatibility of many addons
> (if a bit larger as "Hello world").
> The only reason, why I (and many other) still don't switched to chromium and
> co, is the Ubiquity plugin, but it will be always more complex to maintain
> it (as regards firefox upgrade), just take a look
> https://bitbucket.org/satyr/ubiquity/issues/63/ubiquity-and-electrolysis

Feel free to have a look at the fix I provided for uBlock Origin, where breakage was reported pretty much immediately after this change was made on nightly: https://github.com/gorhill/uBlock/pull/2498/files . Ultimately though, to keep using it in Firefox 57 and beyond, you'll need to convert it to a webextension. https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Porting_a_legacy_Firefox_add-on can help you with this.
@Gijs Thx for you answer.

Just BTW...
> you'll need to convert it to a webextension

Ubiquity is an overlay extension that actively interacts with the page content (can not only get the page-content and properties, but even modify it, e. g. "translate", etc).

A lot of changes is necessary by switch to the webextension-technology.
Unfortunately some things (needed by Ubiquity) are not possibly at all (resp. not yet available) in API for webextension or just not clear how the back-porting may be done at some place (too few documentation for my taste, I meant not the "Hello world" cases).
Another thing that stops myself to make the porting of Ubiquity to webextension, is the thought "What occurs by release of firefox v.70? New Webextension v.2, that will be AGAIN totally incompatible technology?".
See Also: → 1460685
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: