Bug 1432966: Sanitize HTML fragments created for chrome-privileged do…

…cuments.
BrowserWorks · Jan 31, 2018 · d7f689c · d7f689c · jbeich · Feb 2, 2018
1 parent fd18ba7
commit d7f689c
Show file tree

Hide file tree

Showing 24 changed files with 307 additions and 68 deletions.
diff --git a/browser/base/content/browser-media.js b/browser/base/content/browser-media.js
@@ -34,11 +34,23 @@ var gEMEHandler = {
     }
     return true;
   },
-  getLearnMoreLink(msgId) {
-    let text = gNavigatorBundle.getString("emeNotifications." + msgId + ".learnMoreLabel");
+  getEMEDisabledFragment(msgId) {
+    let mainMessage = gNavigatorBundle.getString("emeNotifications.drmContentDisabled.message");
+    let [prefix, suffix] = mainMessage.split(/%(?:1\$)?S/).map(s => document.createTextNode(s));
+    let text = gNavigatorBundle.getString("emeNotifications.drmContentDisabled.learnMoreLabel");
     let baseURL = Services.urlFormatter.formatURLPref("app.support.baseURL");
-    return "<label class='text-link' href='" + baseURL + "drm-content'>" +
-           text + "</label>";
+    let link = document.createElement("label");
+    link.className = "text-link";
+    link.setAttribute("href", baseURL + "drm-content");
+    link.textContent = text;
+
+    let fragment = document.createDocumentFragment();
+    [prefix, link, suffix].forEach(n => fragment.appendChild(n));
+    return fragment;
+  },
+  getMessageWithBrandName(notificationId) {
+    let msgId = "emeNotifications." + notificationId + ".message";
+    return gNavigatorBundle.getFormattedString(msgId, [this._brandShortName]);
   },
   receiveMessage({target: browser, data: data}) {
     let parsedData;
@@ -56,8 +68,8 @@ var gEMEHandler = {
 
     let notificationId;
     let buttonCallback;
-    let params = [];
-    switch (status) {
+    // Notification message can be either a string or a DOM fragment.
+    let notificationMessage;    switch (status) {
       case "available":
       case "cdm-created":
         // Only show the chain icon for proprietary CDMs. Clearkey is not one.
@@ -70,18 +82,18 @@ var gEMEHandler = {
       case "api-disabled":
       case "cdm-disabled":
         notificationId = "drmContentDisabled";
-        buttonCallback = gEMEHandler.ensureEMEEnabled.bind(gEMEHandler, browser, keySystem)
-        params = [this.getLearnMoreLink(notificationId)];
+        buttonCallback = gEMEHandler.ensureEMEEnabled.bind(gEMEHandler, browser, keySystem);
+        notificationMessage = this.getEMEDisabledFragment();
         break;
 
       case "cdm-insufficient-version":
         notificationId = "drmContentCDMInsufficientVersion";
-        params = [this._brandShortName];
+        notificationMessage = this.getMessageWithBrandName(notificationId);
         break;
 
       case "cdm-not-installed":
         notificationId = "drmContentCDMInstalling";
-        params = [this._brandShortName];
+        notificationMessage = this.getMessageWithBrandName(notificationId);
         break;
 
       case "cdm-not-supported":
@@ -92,45 +104,28 @@ var gEMEHandler = {
         Cu.reportError(new Error("Unknown message ('" + status + "') dealing with EME key request: " + data));
         return;
     }
+
+    // Now actually create the notification
 
-    this.showNotificationBar(browser, notificationId, keySystem, params, buttonCallback);
-  },
-  showNotificationBar(browser, notificationId, keySystem, labelParams, callback) {
     let box = gBrowser.getNotificationBox(browser);
     if (box.getNotificationWithValue(notificationId)) {
       return;
     }
 
-    let msgPrefix = "emeNotifications." + notificationId + ".";
-    let msgId = msgPrefix + "message";
-
-    let message = labelParams.length ?
-                  gNavigatorBundle.getFormattedString(msgId, labelParams) :
-                  gNavigatorBundle.getString(msgId);
-
     let buttons = [];
-    if (callback) {
+    if (buttonCallback) {
+      let msgPrefix = "emeNotifications." + notificationId + ".";
       let btnLabelId = msgPrefix + "button.label";
       let btnAccessKeyId = msgPrefix + "button.accesskey";
       buttons.push({
         label: gNavigatorBundle.getString(btnLabelId),
         accessKey: gNavigatorBundle.getString(btnAccessKeyId),
-        callback
+        callback: buttonCallback,
       });
     }
 
     let iconURL = "chrome://browser/skin/drm-icon.svg#chains-black";
-
-    // Do a little dance to get rich content into the notification:
-    let fragment = document.createDocumentFragment();
-    let descriptionContainer = document.createElement("description");
-    // eslint-disable-next-line no-unsanitized/property
-    descriptionContainer.innerHTML = message;
-    while (descriptionContainer.childNodes.length) {
-      fragment.appendChild(descriptionContainer.childNodes[0]);
-    }
-
-    box.appendNotification(fragment, notificationId, iconURL, box.PRIORITY_WARNING_MEDIUM,
+	box.appendNotification(notificationMessage, notificationId, iconURL, box.PRIORITY_WARNING_MEDIUM,
                            buttons);
   },
   showPopupNotificationForSuccess(browser, keySystem) {

diff --git a/browser/components/customizableui/CustomizableWidgets.jsm b/browser/components/customizableui/CustomizableWidgets.jsm
@@ -421,7 +421,7 @@ const CustomizableWidgets = [
       // Put it all together...
       let contents = bundle.getFormattedString("appMenuRemoteTabs.mobilePromo.text2", formatArgs);
       // eslint-disable-next-line no-unsanitized/property
-      promoParentElt.innerHTML = contents;
+      promoParentElt.unsafeSetInnerHTML(contents);
       // We manually manage the "click" event to open the promo links because
       // allowing the "text-link" widget handle it has 2 problems: (1) it only
       // supports button 0 and (2) it's tricky to intercept when it does the

diff --git a/browser/modules/ExtensionsUI.jsm b/browser/modules/ExtensionsUI.jsm
@@ -487,10 +487,10 @@ this.ExtensionsUI = {
             let doc = this.browser.ownerDocument;
         // eslint-disable-next-line no-unsanitized/property
             doc.getElementById("addon-installed-notification-header")
-               .innerHTML = msg1;
+               .unsafeSetInnerHTML(msg1);
             // eslint-disable-next-line no-unsanitized/property
             doc.getElementById("addon-installed-notification-message")
-               .innerHTML = msg2;
+               .unsafeSetInnerHTML(msg2);
           } else if (topic == "dismissed") {
             resolve();
           }

diff --git a/browser/modules/webrtcUI.jsm b/browser/modules/webrtcUI.jsm
@@ -626,21 +626,26 @@ function prompt(aBrowser, aRequest) {
               bundle.getString("getUserMedia.shareScreen.learnMoreLabel");
             let baseURL =
               Services.urlFormatter.formatURLPref("app.support.baseURL");
-            let learnMore =
-              "<label class='text-link' href='" + baseURL + "screenshare-safety'>" +
-              learnMoreText + "</label>";
+
+            let learnMore = chromeWin.document.createElement("label");
+            learnMore.className = "text-link";
+            learnMore.setAttribute("href", baseURL + "screenshare-safety");
+            learnMore.textContent = learnMoreText;
 
             if (type == "screen") {
               string = bundle.getFormattedString("getUserMedia.shareScreenWarning.message",
-                                                 [learnMore]);
+                                                 ["<>"]);
             } else {
               let brand =
                 doc.getElementById("bundle_brand").getString("brandShortName");
               string = bundle.getFormattedString("getUserMedia.shareFirefoxWarning.message",
-                                                 [brand, learnMore]);
+                                                 [brand, "<>"]);
             }
-            // eslint-disable-next-line no-unsanitized/property
-            warning.innerHTML = string;
+
+            let [pre, post] = string.split("<>");
+            warning.textContent = pre;
+            warning.appendChild(learnMore);
+            warning.appendChild(chromeWin.document.createTextNode(post));
           }
 
           let perms = Services.perms;

diff --git a/devtools/client/responsive.html/components/browser.js b/devtools/client/responsive.html/components/browser.js
@@ -15,6 +15,12 @@ const { DOM: dom, createClass, addons, PropTypes } =
 const e10s = require("../utils/e10s");
 const message = require("../utils/message");
 
+// Allow creation of HTML fragments without automatic sanitization, even
+// though we're in a chrome-privileged document.
+// This is, unfortunately, necessary in order to React to function
+// correctly.
+document.allowUnsafeHTML = true;
+
 module.exports = createClass({
 
   /**

diff --git a/devtools/shared/gcli/source/lib/gcli/util/util.js b/devtools/shared/gcli/source/lib/gcli/util/util.js
@@ -498,7 +498,11 @@ exports.setContents = function(elem, contents) {
     return;
   }
 
-  if ('innerHTML' in elem) {
+  if ('unsafeSetInnerHTML' in elem) {
+    // FIXME: Stop relying on unsanitized HTML.
+    elem.unsafeSetInnerHTML(contents);
+  }
+  else if ('innerHTML' in elem) {
     elem.innerHTML = contents;
   }
   else {

diff --git a/devtools/shared/tests/browser/browser_l10n_localizeMarkup.js b/devtools/shared/tests/browser/browser_l10n_localizeMarkup.js
@@ -18,7 +18,7 @@ add_task(function* () {
 
   info("Create the test markup");
   let div = document.createElement("div");
-  div.innerHTML =
+  div.unsafeSetInnerHTML(
   `<div data-localization-bundle="devtools/client/locales/startup.properties">
      <div id="d0" data-localization="content=inspector.someInvalidKey"></div>
      <div id="d1" data-localization="content=inspector.label">Text will disappear</div>
@@ -32,7 +32,7 @@ add_task(function* () {
        <div id="d5" data-localization="content=toolbox.defaultTitle"></div>
      </div>
    </div>
-  `;
+  `);
 
   info("Use localization helper to localize the test markup");
   localizeMarkup(div);

diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp
@@ -3722,6 +3722,12 @@ Element::SetInnerHTML(const nsAString& aInnerHTML, ErrorResult& aError)
   SetInnerHTMLInternal(aInnerHTML, aError);
 }
 
+void
+Element::UnsafeSetInnerHTML(const nsAString& aInnerHTML, ErrorResult& aError)
+{
+  SetInnerHTMLInternal(aInnerHTML, aError, true);
+}
+
 void
 Element::GetOuterHTML(nsAString& aOuterHTML)
 {

diff --git a/dom/base/Element.h b/dom/base/Element.h
@@ -1128,6 +1128,7 @@ class Element : public FragmentOrElement
 
   NS_IMETHOD GetInnerHTML(nsAString& aInnerHTML);
   virtual void SetInnerHTML(const nsAString& aInnerHTML, ErrorResult& aError);
+  void UnsafeSetInnerHTML(const nsAString& aInnerHTML, ErrorResult& aError);
   void GetOuterHTML(nsAString& aOuterHTML);
   void SetOuterHTML(const nsAString& aOuterHTML, ErrorResult& aError);
   void InsertAdjacentHTML(const nsAString& aPosition, const nsAString& aText,

diff --git a/dom/base/FragmentOrElement.cpp b/dom/base/FragmentOrElement.cpp
@@ -2388,7 +2388,8 @@ ContainsMarkup(const nsAString& aStr)
 }
 
 void
-FragmentOrElement::SetInnerHTMLInternal(const nsAString& aInnerHTML, ErrorResult& aError)
+FragmentOrElement::SetInnerHTMLInternal(const nsAString& aInnerHTML, ErrorResult& aError,
+                                        bool aNeverSanitize)
 {
   FragmentOrElement* target = this;
   // Handle template case.
@@ -2442,6 +2443,9 @@ FragmentOrElement::SetInnerHTMLInternal(const nsAString& aInnerHTML, ErrorResult
     contextNameSpaceID = shadowRoot->GetHost()->GetNameSpaceID();
   }
 
+  auto sanitize = (aNeverSanitize ? nsContentUtils::NeverSanitize
+                                  : nsContentUtils::SanitizeSystemPrivileged);
+
   if (doc->IsHTMLDocument()) {
     int32_t oldChildCount = target->GetChildCount();
     aError = nsContentUtils::ParseFragmentHTML(aInnerHTML,
@@ -2450,14 +2454,17 @@ FragmentOrElement::SetInnerHTMLInternal(const nsAString& aInnerHTML, ErrorResult
                                                contextNameSpaceID,
                                                doc->GetCompatibilityMode() ==
                                                  eCompatibility_NavQuirks,
-                                               true);
+                                               true,
+                                               sanitize);
     mb.NodesAdded();
     // HTML5 parser has notified, but not fired mutation events.
     nsContentUtils::FireMutationEventsForDirectParsing(doc, target,
                                                        oldChildCount);
   } else {
     RefPtr<DocumentFragment> df =
-      nsContentUtils::CreateContextualFragment(target, aInnerHTML, true, aError);
+      nsContentUtils::CreateContextualFragment(target, aInnerHTML, true,
+                                               sanitize,
+                                               aError);
     if (!aError.Failed()) {
       // Suppress assertion about node removal mutation events that can't have
       // listeners anyway, because no one has had the chance to register mutation

diff --git a/dom/base/FragmentOrElement.h b/dom/base/FragmentOrElement.h
@@ -375,7 +375,8 @@ class FragmentOrElement : public nsIContent
 
 protected:
   void GetMarkup(bool aIncludeSelf, nsAString& aMarkup);
-  void SetInnerHTMLInternal(const nsAString& aInnerHTML, ErrorResult& aError);
+  void SetInnerHTMLInternal(const nsAString& aInnerHTML, ErrorResult& aError,
+                            bool aNeverSanitize = false);
 
   // Override from nsINode
   virtual nsINode::nsSlots* CreateSlots() override;