Frederik Braun [:freddy] Reporter
	Description • 10 years ago

With the current plan to harden the security of Firefox, we want to disallow internal privileged pages to use inline JavaScript. Since their amount is fairly limited, we start this by rewriting them bit by bit.

Taking this bug requires some knowledge in event handlers (e.g. addEventListener) and their scope. This wiki page should be a good reference guide: https://wiki.mozilla.org/Security/Inline_Scripts_and_Styles

	Sunil Kumar
	Comment 1 • 10 years ago

Hi. I would like to work on this bug. Since I'm new to mozilla community, i need some help in getting started.

I use 64 bit ArchLinux. I'm more comfortable with using git than Mercurial. So I wanted to know which repository I have to fork and clone to work on this bug.

	Frederik Braun [:freddy] Reporter
	Comment 2 • 10 years ago

Build instructions are in the wiki <https://developer.mozilla.org/en-US/docs/Introduction> the git repo is <https://github.com/mozilla/gecko-dev>.

	Sunil Kumar
	Comment 3 • 10 years ago

Hi. I followed the instructions given in that link for building the product. But I'm getting the below error

 0:00.26 /usr/bin/make -f client.mk -s
 0:00.54 Traceback (most recent call last):
 0:00.54   File "/home/sunil/repos/git/gecko-dev/config/pythonpath.py", line 56, in <module>
 0:00.54     main(sys.argv[1:])
 0:00.54   File "/home/sunil/repos/git/gecko-dev/config/pythonpath.py", line 48, in main
 0:00.54     execfile(script, frozenglobals)
 0:00.54 NameError: name 'execfile' is not defined
 0:00.55 client.mk:345: recipe for target 'obj-x86_64-unknown-linux-gnu/CLOBBER' failed
 0:00.55 make: *** [obj-x86_64-unknown-linux-gnu/CLOBBER] Error 1
 0:00.58 0 compiler warnings present.

Please help.

	Frederik Braun [:freddy] Reporter
	Comment 4 • 10 years ago

A lot of people get automated emails when this bug changes. Let us move troubleshooting to another channel and keep this bug focused on the actual topic (rewriting the XUL pages).
Sunil, please make sure you have read through all necessary steps for setting up your build environment on MDN. Once you have confirmed that the error persists, try in #developers on irc.mozilla.org.

	vikneshwar
	Comment 5 • 10 years ago

The only inline script i found is [1] . Moved that to [2] 

[1]mozilla-central/source/browser/components/preferences/in-content/preferences.xul#172
[2]http://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/preferences.js

	Frederik Braun [:freddy] Reporter
	Comment 6 • 10 years ago

Comment on attachment 8431664 [details] [diff] [review]
pref.patch

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

This looks good! But there's a tricky thing you missed: preferences.xul includes other XUL documents (through #include preprocessor statements).
Can you try to address those too? Otherwise, we can file individual bugs for each included XUL file.

::: browser/components/preferences/in-content/preferences.js
@@ +18,1 @@
>  

Please put this piece of code in the DOMConentLoaded event listener below.

	Frederik Braun [:freddy] Reporter
	Comment 7 • 10 years ago

Comment on attachment 8431664 [details] [diff] [review]
pref.patch

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

I was trying to cite the piece of code in line 14-17: Don't add a new event listener, but use the DOMContentLoaded listener that already exists in preferences.js.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 8 • 10 years ago

Comment on attachment 8431664 [details] [diff] [review]
pref.patch

Wrong bug.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) Assignee
	Comment 9 • 10 years ago

vikneshwar: Are you still working on this?

	vikneshwar
	Comment 10 • 10 years ago

aryx , you can start with it if you want . Thank you :)

	Frederik Braun [:freddy] Reporter
	Comment 12 • 10 years ago

Comment on attachment 8472505 [details] [diff] [review]
patch, v2

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

This looks good! Patch still mentions you as the contributor though.

Did you notice how preferences.xul includes additional files that may need fixing?
Are you willing to work on them as well, aryx?

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) Assignee
	Comment 13 • 10 years ago

(In reply to Frederik Braun [:freddyb] from comment #12)
> Did you notice how preferences.xul includes additional files that may need
> fixing?
> Are you willing to work on them as well, aryx?
The onsyncfrompreference/onsynctopreference attributes should remain in place because the events are only synthesized?
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#429

Example: http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/main.xul#130

	Frederik Braun [:freddy] Reporter
	Comment 14 • 10 years ago

Yes, please keep the synthesized events in place, aryx.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) Assignee
	Comment 15 • 10 years ago

Successful Try run: https://tbpl.mozilla.org/?tree=Try&showall=1&rev=9b9c5276fc74

	Frederik Braun [:freddy] Reporter
	Comment 16 • 10 years ago

Comment on attachment 8476577 [details] [diff] [review]
patch, v3

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

This looks neat and very well executed. Thank you for this, aryx!
I'm not a module peer so all I can give is feedback+.
Let's ask Gavin, can you review this?

	:Gijs (he/him)
	Comment 17 • 10 years ago

Comment on attachment 8476577 [details] [diff] [review]
patch, v3

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

Stealing.

This is almost there, but please see my remarks about the sync stuff, and the inline style change. :-)

::: browser/components/preferences/in-content/advanced.js
@@ +65,5 @@
> +    setEventListener("offlineNotifyExceptions", "command", function (event) {
> +      gAdvancedPane.offlineAppSelected(event); })
> +    let bundlePrefs = document.getElementById("bundlePreferences");
> +    document.getElementById("offlineAppsList")
> +            .style.height = bundlePrefs.getString("offlineAppsList.height");

Freddy, is this OK per the CSP? This will dynamically add a style attribute... in particular, https://developer.mozilla.org/en-US/docs/Web/Security/CSP/Using_Content_Security_Policy#Writing_a_policy is a little awkwardly worded but says:

" A policy needs to include a default-src or style-src directive in order for CSP to restrict inline styles from being applied from a <style> element or a .style attribute"

Reading the spec, the 1.0 version seems to imply CSSOM is OK, and the 1.1 version here: https://w3c.github.io/webappsec/specs/content-security-policy/#directive-style-src seems to indicate that if you specify unsafe-eval, the above will fail.

Please advise? :-)

(note that this sets styles dynamically based on l10n properties; afaik there's basically no sane way to workaround this to include the l10n info 'statically' to avoid these issues)

::: browser/components/preferences/in-content/advanced.xul
@@ -313,5 @@
>          <hbox>
>            <vbox flex="1">
>              <label id="offlineAppsListLabel">&offlineAppsList2.label;</label>
>              <listbox id="offlineAppsList"
> -                    style="height: &offlineAppsList.height;;"

While I realize we don't need to update the non-incontent preferences.xul, could you port this particular bit to the windowed preferences so we don't duplicate the string as much? You can test by toggling the inContent and instantApply about:config flags.

::: browser/components/preferences/in-content/applications.js
@@ +942,5 @@
> +    setEventListener("filter", "command", gApplicationsPane.filter);
> +    setEventListener("handlersView", "select",
> +      gApplicationsPane.onSelectionChanged);
> +    setEventListener("typeColumn", "click", function (event) {
> +      gApplicationsPane.sort(event); });

Nit: this can be gApplicationsPane.sort, right? Ditto for the one below.

::: browser/components/preferences/in-content/sync.js
@@ +113,5 @@
> +    setEventListener("unlinkDevice", "click", function () {
> +      gSyncPane.startOver(true);
> +      return false;
> +    });
> +    setEventListener("tosPP-normal-ToS", "click", function () {

Nit: name this function (e.g. openToS) and the next one (e.g. openPrivacyPolicy) and pass them by-reference here and for tosPP-small-*.

::: browser/components/preferences/in-content/sync.xul
@@ +146,5 @@
>            &unlinkDevice.label;
>          </label>
>        </hbox>
>      </groupbox>
> +    <hbox id="tosPP-normal" pack="center">

Hey look, duplicate IDs. The things you find in code cleanup...

Could I ask you to:
1) update the IDs in the non-in-content one as well.
2) update the ID ref in sync.js (both in-content and not), which is missing from this patch? (line 174 in both, at time of writing:

document.getElementById("tosPP").hidden = this._usingCustomServer;

)

I'm guessing we need to hide both ToS's, or figure out the right one somehow?

::: browser/themes/shared/incontentprefs/preferences.css
@@ +249,5 @@
>  #weavePrefsDeck > vbox > groupbox,
>  #weavePrefsDeck > vbox > description,
>  #weavePrefsDeck > vbox > #pairDevice > label,
>  #weavePrefsDeck > #needsUpdate > hbox > #loginError,
> +#weavePrefsDeck > #hasFxaAccount > hbox:not(#tosPP-small) > label {

why only the -small version, and not the -normal one?

	:Gijs (he/him)
	Comment 18 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #17)
> ::: browser/components/preferences/in-content/advanced.js
> @@ +65,5 @@
> > +    setEventListener("offlineNotifyExceptions", "command", function (event) {
> > +      gAdvancedPane.offlineAppSelected(event); })
> > +    let bundlePrefs = document.getElementById("bundlePreferences");
> > +    document.getElementById("offlineAppsList")
> > +            .style.height = bundlePrefs.getString("offlineAppsList.height");
> 
> Freddy, is this OK per the CSP? This will dynamically add a style
> attribute... in particular,
> https://developer.mozilla.org/en-US/docs/Web/Security/CSP/
> Using_Content_Security_Policy#Writing_a_policy is a little awkwardly worded
> but says:
> 
> " A policy needs to include a default-src or style-src directive in order
> for CSP to restrict inline styles from being applied from a <style> element
> or a .style attribute"
> 
> Reading the spec, the 1.0 version seems to imply CSSOM is OK, and the 1.1
> version here:
> https://w3c.github.io/webappsec/specs/content-security-policy/#directive-
> style-src seems to indicate that if you specify unsafe-eval, the above will
> fail.
> 
> Please advise? :-)
> 
> (note that this sets styles dynamically based on l10n properties; afaik
> there's basically no sane way to workaround this to include the l10n info
> 'statically' to avoid these issues)

Now with added needinfo goodness that's not available from the splinter page...

	Frederik Braun [:freddy] Reporter
	Comment 19 • 10 years ago

Thanks for raising this Gijs!

As per our implementation CSSOM is *currently* fine.
I'd lean towards enabling it in a future proof way by adding unsafe-eval if we go with 1.1.
It is *potentially* bad given that CSS is a bit worse in XUL/Chrome land than in content. In this case, fortunately, the CSS properties don't come from something potentially malicious.

	:Gijs (he/him)
	Comment 21 • 10 years ago

Comment on attachment 8477903 [details] [diff] [review]
patch, v4

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

::: browser/components/preferences/in-content/advanced.xul
@@ +227,5 @@
>          <caption><label>&crashReporterSection.label;</label></caption>
>          <description>&crashReporterDesc.label;</description>
>          <hbox>
>            <checkbox id="submitCrashesBox"
> +                    oncommand=";"

Why leave the ";" everywhere?

::: browser/components/preferences/sync.js
@@ +171,5 @@
>        this.page = PAGE_HAS_ACCOUNT;
>        document.getElementById("accountName").value = Weave.Service.identity.account;
>        document.getElementById("syncComputerName").value = Weave.Service.clientsEngine.localName;
> +      document.getElementById("tosPP-normal").hidden = this._usingCustomServer;
> +      document.getElementById("tosPP-small").hidden = this._usingCustomServer;

This will hide and show both... is that really correct?

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) Assignee
	Comment 22 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #21)
> Comment on attachment 8477903 [details] [diff] [review]
> patch, v4
> >            <checkbox id="submitCrashesBox"
> > +                    oncommand=";"
> 
> Why leave the ";" everywhere?
As requested by freddyb over IRC due to bug 547189 and because I experienced an issue without it. I later discovered that the issue had a different cause, so they are gone in this version.

> ::: browser/components/preferences/sync.js
> @@ +171,5 @@
> >        this.page = PAGE_HAS_ACCOUNT;
> >        document.getElementById("accountName").value = Weave.Service.identity.account;
> >        document.getElementById("syncComputerName").value = Weave.Service.clientsEngine.localName;
> > +      document.getElementById("tosPP-normal").hidden = this._usingCustomServer;
> > +      document.getElementById("tosPP-small").hidden = this._usingCustomServer;
> 
> This will hide and show both... is that really correct?
I changed it to only manage the old Sync part ("-normal"), because the code block is for this condition and there isn't UI for custom Firefox Accounts servers.

Successful Try run: https://tbpl.mozilla.org/?tree=Try&rev=c4819c73b858

	:Gijs (he/him)
	Comment 23 • 10 years ago

Comment on attachment 8479223 [details] [diff] [review]
patch, v6

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

::: browser/components/preferences/in-content/sync.js
@@ +78,5 @@
>        this._init();
>      }.bind(this);
>  
>      Services.obs.addObserver(onReady, "weave:service:ready", false);
> +

Nit: no need for an empty line here.

@@ +198,5 @@
>    updateWeavePrefs: function () {
>      let service = Components.classes["@mozilla.org/weave/service;1"]
>                    .getService(Components.interfaces.nsISupports)
>                    .wrappedJSObject;
> +    // service.fxAccountsEnabled is false if sync is already configured for

Nit: "iff" is intentional here - it means "if and only if". Please leave it intact.

	:Gijs (he/him)
	Comment 24 • 10 years ago

(In reply to Archaeopteryx [:aryx] from comment #22)
> > ::: browser/components/preferences/sync.js
> > @@ +171,5 @@
> > >        this.page = PAGE_HAS_ACCOUNT;
> > >        document.getElementById("accountName").value = Weave.Service.identity.account;
> > >        document.getElementById("syncComputerName").value = Weave.Service.clientsEngine.localName;
> > > +      document.getElementById("tosPP-normal").hidden = this._usingCustomServer;
> > > +      document.getElementById("tosPP-small").hidden = this._usingCustomServer;
> > 
> > This will hide and show both... is that really correct?
> I changed it to only manage the old Sync part ("-normal"), because the code
> block is for this condition and there isn't UI for custom Firefox Accounts
> servers.

Makes perfect sense. Thanks a lot for all the hard work here, this kind of stuff always looks easy on the surface and then ends up being a boatload of annoying work. :-)

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/076b9e5d8208

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/076b9e5d8208