994849 - Move inline scripts and styles for security-related about: XHTML pages into separate files

Status: RESOLVED FIXED

(SeaMonkey :: Security, defect)

Description

[rsx11m]

+++ This bug was initially created as a clone of Bug #960566 +++

> With the current plan to harden the security of Firefox, we want to
> disallow internal privileged pages to use inline JavaScript.

(Quoting bug 960566 comment #13)
> (In reply to rsx11m from bug 960566 comment #5)
> >   blockedFlags: SCRIPT | UNTRUSTED | HIDE,
> >   blockedURI: "chrome://communicator/content/blockedSite.xhtml",
> 
> >   certerrorFlags: SCRIPT | UNTRUSTED | HIDE,
> >   certerrorURI: "chrome://communicator/content/certError.xhtml",
> 
> These two are potentially tricky; at the end of the files they both read:
> 
>     <!--
>     - Note: It is important to run the script this way, instead of using
>     - an onload handler. This is because error pages are loaded as
>     - LOAD_BACKGROUND, which means that onload handlers will not be executed.
>     -->
>     <script type="application/javascript">initPage();</script>

Actually, that's more straightforward than it initially appeared. While a window.onload definition indeed isn't executed here, the script can just be included from the file similar to bug 948879 attachment 8349774 [details] [diff] [review] at the very end, thus ensuring that the DOM is completely loaded when it is executed. Handlers can be defined and initPage(); called there as part of the main JavaScript code.

Due to the similarity of these two pages I'll handle them both in a single bug. There are also some style definitions in blockedSite.xhtml which should probably go into theme-specific CSS files rather than parallel to the XHTML file.

Comment 1

[rsx11m]

Attachment: xul:button binding issue

Now this is interesting. After moving the <script> parts from blockedSite.xhtml to a new blockedSite.js file, I'm suddenly seeing line breaks and an empty button (without any function) before the location of the reported web site.

Upon closer inspection, this <xul:button> comes from the implementation of the buttons as bindings as defined in certError.{css,xml}, using the <span> element as anchor. However, safeBrowsing.dtd contains string definitions which include "<span id='malware_sitename'/>" to insert the site name into it. Consequently, this is overlayed with an empty <xul:button> definition from the binding.

The question I'm puzzled about is why that doesn't show up in the current releases, but only after I've moved the script code into a separate file?

So, the solution I envision is for certError.css to apply to "span.button" rather than just "span" for the binding, then to rewrite <span class="button"> for all occurrences where the <span> actually serves as a button. This should leave any "true" <span> like the ones in the string definitions untouched (and also makes the code a bit more self-explanatory).

Comment 2

[rsx11m]

Attachment: Proposed patch

Modifications to certError.*:
 - <script> content moved to certError.js (no change in the actual code)
 - on...="" attributes converted to eventListeners in certError.js
 - comment #1 addressed (though no impact here)

Modifications to blockedSite.*:
 - <script> content moved to blockedSite.js (no change in the actual code)
 - <style> content moved to blockedSite.css for both classic and modern themes
 - no event listeners required
 - comment #1 addressed

Verified function with a self-signed certificate for certError.*, and with the test pages provided for blockedSite.*; no change in behavior or appearance.

Comment 3

[[e-mail]]

What happens is that when the script is inline it executes before the XBL binding attaches. This means that the span's text content prevents the XBL binding from attaching.

Moving the script out-of-line causes it to initiate a separate request, giving time for the XBL to bind to the span. Adding the text content does not detach the XBL binding (there is a bug on this, and web components handles things differently and so it will probably never be fixed).

So yes, it is a bug in that the XBL should not apply to that span in the first place.

Comment 4

[[e-mail]]

Comment on attachment 8406151 [details] [diff] [review]
Proposed patch

Basically the patch looks fine, but there are some style nits in the code you could fix while you're there.

>+  switch (getErrorCode()) {
>+    case "malwareBlocked" :
>+      initPage_malware();
>+      break;
>+    case "phishingBlocked" :
>+      initPage_phishing();
>+      break;
>+  }
Nit: no space before :

>-span {
>+span.button {
Nit: just use .button; as we only use it on spans, it's a useless extra check.

>+document.getElementById("technicalContentHeading")
>+        .addEventListener("click", function() { toggle("technicalContent"); });
>+
>+document.getElementById("expertContentHeading")
>+        .addEventListener("click", function() { toggle("expertContent"); });
A nice follow up would be to make the same toggle function work as an event listener for either toggle.

>+
>+
Nit: double blank line

>+function endsWith(haystack, needle) {
>+  return haystack.slice(-needle.length) == needle;
>+}
We should switch the callers to use haystack.endsWith(needle) instead.

>+#buttons > #ignoreWarningButton {
>+  float: right;
>+  font-size: smaller;
>+}
Modern specifies the font on its button, which overrides this font size, so you could either remove the font size (keeping the existing behaviour) or adjust the CSS to apply the font size to the button directly.

Comment 5

[rsx11m]

Attachment: Patch for check-in

(In reply to [e-mail] from comment #4)
> Nit: no space before :
> Nit: double blank line

Both done.

> Nit: just use .button; as we only use it on spans, it's a useless extra
> check.

Done, though I figured that being more specific should make it more efficient (i.e., don't check the class for anything other than <span> to start with), but apparently that may not be the case.

> A nice follow up would be to make the same toggle function work as an event
> listener for either toggle.

Well, the overhead is small here for the 1-line listener, and there are only two occurrences (on top of that I don't know how to tie the same event listener to two elements other than just letting them call the same function, which is basically what's done here simply with a different argument passed).

> We should switch the callers to use haystack.endsWith(needle) instead.

All needles and haystacks removed along with the now redundant local endsWith() function.

> Modern specifies the font on its button, which overrides this font size, so
> you could either remove the font size (keeping the existing behaviour) or
> adjust the CSS to apply the font size to the button directly.

I removed it given that this bug does not aim for making any changes to current function or appearance.

Comment 6

[rsx11m]

Push for comm-central, please.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/214f7086063f

Comment 8

[[e-mail]]

(In reply to rsx11m)
> +++ This bug was initially created as a clone of Bug #960566 +++
> 
> > With the current plan to harden the security of Firefox, we want to
> > disallow internal privileged pages to use inline JavaScript.

So, it's unclear as to whether these two pages count as privileged, but never mind.

(In reply to rsx11m from comment #5)
> (In reply to comment #4)
> > Nit: just use .button; as we only use it on spans, it's a useless extra
> > check.
> 
> Done, though I figured that being more specific should make it more
> efficient (i.e., don't check the class for anything other than <span> to
> start with), but apparently that may not be the case.

Checking the class is heavily optimised because it's done so much.

> > A nice follow up would be to make the same toggle function work as an event
> > listener for either toggle.
> 
> Well, the overhead is small here for the 1-line listener, and there are only
> two occurrences (on top of that I don't know how to tie the same event
> listener to two elements other than just letting them call the same
> function, which is basically what's done here simply with a different
> argument passed).

Actually I notice that the toggle function is used in other places so it's not as clear-cut as I thought.

Comment 9

[rsx11m]

(In reply to [e-mail] from comment #8)
> So, it's unclear as to whether these two pages count as privileged, but never mind.

Err, right. They are marked as "untrusted" in nsAbout.js, so likely they aren't privileged despite having action buttons in them. According to Frederik's bug 960566 comment #8 it's desirable to have those 2nd-tier pages separated as well, thus not a wasted effort anyway.