[tbb-commits] [tor-browser/tor-browser-68.1.0esr-9.0-2] Bug 467035 - Avoid leaking browser language via DTD r=Gijs, bzbarsky

gk at torproject.org gk at torproject.org
Mon Sep 16 13:41:46 UTC 2019


commit 2fe57c62af9706be45fc2085796a9398c3b10763
Author: Alex Catarineu <acat at torproject.org>
Date:   Mon Jul 8 10:47:05 2019 +0000

    Bug 467035 - Avoid leaking browser language via DTD r=Gijs,bzbarsky
    
    Differential Revision: https://phabricator.services.mozilla.com/D34187
    
    --HG--
    extra : moz-landing-system : lando
---
 .../browser_misused_characters_in_strings.js       |  1 +
 .../test/mochitest/formautofill/mochitest.ini      |  1 +
 dom/base/DOMParser.cpp                             | 11 +++++-
 dom/base/DOMParser.h                               |  8 +++-
 dom/base/Document.cpp                              | 39 ++-----------------
 dom/base/Document.h                                | 10 +++++
 dom/base/nsContentUtils.cpp                        | 28 ++++++++++++++
 dom/base/nsContentUtils.h                          |  7 ++++
 dom/security/nsContentSecurityManager.cpp          | 16 +++++++-
 dom/tests/mochitest/bugs/mochitest.ini             |  1 +
 dom/tests/mochitest/bugs/test_bug467035.html       | 45 ++++++++++++++++++++++
 dom/webidl/DOMParser.webidl                        |  5 +++
 parser/htmlparser/nsExpatDriver.cpp                |  6 ++-
 testing/marionette/l10n.js                         |  6 ++-
 .../firefox/firefox_puppeteer/api/l10n.py          |  1 +
 toolkit/content/widgets/datetimebox.js             |  1 +
 toolkit/content/widgets/pluginProblem.js           |  1 +
 toolkit/content/widgets/videocontrols.js           |  4 ++
 18 files changed, 150 insertions(+), 41 deletions(-)

diff --git a/browser/base/content/test/static/browser_misused_characters_in_strings.js b/browser/base/content/test/static/browser_misused_characters_in_strings.js
index eb17e92c7b59..4b1d9a75d3bb 100644
--- a/browser/base/content/test/static/browser_misused_characters_in_strings.js
+++ b/browser/base/content/test/static/browser_misused_characters_in_strings.js
@@ -272,6 +272,7 @@ add_task(async function checkAllTheFluents() {
     {}
   );
   let domParser = new DOMParser();
+  domParser.forceEnableDTD();
   for (let uri of uris) {
     let rawContents = await fetchFile(uri.spec);
     let resource = FluentResource.fromString(rawContents);
diff --git a/browser/components/payments/test/mochitest/formautofill/mochitest.ini b/browser/components/payments/test/mochitest/formautofill/mochitest.ini
index c58cdb9eefb3..9740f9e3e88f 100644
--- a/browser/components/payments/test/mochitest/formautofill/mochitest.ini
+++ b/browser/components/payments/test/mochitest/formautofill/mochitest.ini
@@ -6,4 +6,5 @@ support-files =
    ../../../../../../browser/extensions/formautofill/content/editCreditCard.xhtml
    ../../../../../../browser/extensions/formautofill/content/editAddress.xhtml
 
+skip-if = true # Bug 1446164
 [test_editCreditCard.html]
diff --git a/dom/base/DOMParser.cpp b/dom/base/DOMParser.cpp
index 6f0d30fd75a7..3f12ef8d7c69 100644
--- a/dom/base/DOMParser.cpp
+++ b/dom/base/DOMParser.cpp
@@ -33,7 +33,8 @@ DOMParser::DOMParser(nsIGlobalObject* aOwner, nsIPrincipal* aDocPrincipal,
       mPrincipal(aDocPrincipal),
       mDocumentURI(aDocumentURI),
       mBaseURI(aBaseURI),
-      mForceEnableXULXBL(false) {
+      mForceEnableXULXBL(false),
+      mForceEnableDTD(false) {
   MOZ_ASSERT(aDocPrincipal);
   MOZ_ASSERT(aDocumentURI);
 }
@@ -69,6 +70,10 @@ already_AddRefed<Document> DOMParser::ParseFromString(const nsAString& aStr,
       document->ForceEnableXULXBL();
     }
 
+    if (mForceEnableDTD) {
+      document->ForceSkipDTDSecurityChecks();
+    }
+
     nsresult rv = nsContentUtils::ParseDocumentHTML(aStr, document, false);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       aRv.Throw(rv);
@@ -183,6 +188,10 @@ already_AddRefed<Document> DOMParser::ParseFromStream(nsIInputStream* aStream,
     document->ForceEnableXULXBL();
   }
 
+  if (mForceEnableDTD) {
+    document->ForceSkipDTDSecurityChecks();
+  }
+
   // Have to pass false for reset here, else the reset will remove
   // our event listener.  Should that listener addition move to later
   // than this call?
diff --git a/dom/base/DOMParser.h b/dom/base/DOMParser.h
index 0a2db0ef4e2b..9a6545ad7f33 100644
--- a/dom/base/DOMParser.h
+++ b/dom/base/DOMParser.h
@@ -53,7 +53,12 @@ class DOMParser final : public nsISupports, public nsWrapperCache {
                                              SupportedType aType,
                                              ErrorResult& aRv);
 
-  void ForceEnableXULXBL() { mForceEnableXULXBL = true; }
+  void ForceEnableXULXBL() {
+    mForceEnableXULXBL = true;
+    ForceEnableDTD();
+  }
+
+  void ForceEnableDTD() { mForceEnableDTD = true; }
 
   nsIGlobalObject* GetParentObject() const { return mOwner; }
 
@@ -78,6 +83,7 @@ class DOMParser final : public nsISupports, public nsWrapperCache {
   nsCOMPtr<nsIURI> mBaseURI;
 
   bool mForceEnableXULXBL;
+  bool mForceEnableDTD;
 };
 
 }  // namespace dom
diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp
index a0823a6e457f..eebeada1c63c 100644
--- a/dom/base/Document.cpp
+++ b/dom/base/Document.cpp
@@ -1263,6 +1263,7 @@ Document::Document(const char* aContentType)
       mType(eUnknown),
       mDefaultElementType(0),
       mAllowXULXBL(eTriUnset),
+      mSkipDTDSecurityChecks(false),
       mBidiOptions(IBMBIDI_DEFAULT_BIDI_OPTIONS),
       mSandboxFlags(0),
       mPartID(0),
@@ -1987,38 +1988,6 @@ void Document::Reset(nsIChannel* aChannel, nsILoadGroup* aLoadGroup) {
   mChannel = aChannel;
 }
 
-/**
- * Determine whether the principal is allowed access to the localization system.
- * We don't want the web to ever see this but all our UI including in content
- * pages should pass this test.
- */
-bool PrincipalAllowsL10n(nsIPrincipal* principal) {
-  // The system principal is always allowed.
-  if (nsContentUtils::IsSystemPrincipal(principal)) {
-    return true;
-  }
-
-  nsCOMPtr<nsIURI> uri;
-  nsresult rv = principal->GetURI(getter_AddRefs(uri));
-  NS_ENSURE_SUCCESS(rv, false);
-
-  bool hasFlags;
-
-  // Allow access to uris that cannot be loaded by web content.
-  rv = NS_URIChainHasFlags(uri, nsIProtocolHandler::URI_DANGEROUS_TO_LOAD,
-                           &hasFlags);
-  NS_ENSURE_SUCCESS(rv, false);
-  if (hasFlags) {
-    return true;
-  }
-
-  // UI resources also get access.
-  rv = NS_URIChainHasFlags(uri, nsIProtocolHandler::URI_IS_UI_RESOURCE,
-                           &hasFlags);
-  NS_ENSURE_SUCCESS(rv, false);
-  return hasFlags;
-}
-
 void Document::DisconnectNodeTree() {
   // Delete references to sub-documents and kill the subdocument map,
   // if any. This is not strictly needed, but makes the node tree
@@ -3256,11 +3225,11 @@ DocumentL10n* Document::GetL10n() { return mDocumentL10n; }
 bool Document::DocumentSupportsL10n(JSContext* aCx, JSObject* aObject) {
   nsCOMPtr<nsIPrincipal> callerPrincipal =
       nsContentUtils::SubjectPrincipal(aCx);
-  return PrincipalAllowsL10n(callerPrincipal);
+  return nsContentUtils::PrincipalAllowsL10n(callerPrincipal);
 }
 
 void Document::LocalizationLinkAdded(Element* aLinkElement) {
-  if (!PrincipalAllowsL10n(NodePrincipal())) {
+  if (!nsContentUtils::PrincipalAllowsL10n(NodePrincipal())) {
     return;
   }
 
@@ -3291,7 +3260,7 @@ void Document::LocalizationLinkAdded(Element* aLinkElement) {
 }
 
 void Document::LocalizationLinkRemoved(Element* aLinkElement) {
-  if (!PrincipalAllowsL10n(NodePrincipal())) {
+  if (!nsContentUtils::PrincipalAllowsL10n(NodePrincipal())) {
     return;
   }
 
diff --git a/dom/base/Document.h b/dom/base/Document.h
index 82b7d66753ef..e65bb95d94c9 100644
--- a/dom/base/Document.h
+++ b/dom/base/Document.h
@@ -2764,8 +2764,16 @@ class Document : public nsINode,
                : mAllowXULXBL == eTriFalse ? false : InternalAllowXULXBL();
   }
 
+  /**
+   * Returns true if this document is allowed to load DTDs from UI resources
+   * no matter what.
+   */
+  bool SkipDTDSecurityChecks() { return mSkipDTDSecurityChecks; }
+
   void ForceEnableXULXBL() { mAllowXULXBL = eTriTrue; }
 
+  void ForceSkipDTDSecurityChecks() { mSkipDTDSecurityChecks = true; }
+
   /**
    * Returns the template content owner document that owns the content of
    * HTMLTemplateElement.
@@ -4401,6 +4409,8 @@ class Document : public nsINode,
 
   Tri mAllowXULXBL;
 
+  bool mSkipDTDSecurityChecks;
+
   // The document's script global object, the object from which the
   // document can get its script context and scope. This is the
   // *inner* window object.
diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp
index 2b416828e8c1..66134cdb691f 100644
--- a/dom/base/nsContentUtils.cpp
+++ b/dom/base/nsContentUtils.cpp
@@ -1676,6 +1676,34 @@ bool nsContentUtils::OfflineAppAllowed(nsIPrincipal* aPrincipal) {
   return NS_SUCCEEDED(rv) && allowed;
 }
 
+/* static */
+bool nsContentUtils::PrincipalAllowsL10n(nsIPrincipal* aPrincipal) {
+  // The system principal is always allowed.
+  if (IsSystemPrincipal(aPrincipal)) {
+    return true;
+  }
+
+  nsCOMPtr<nsIURI> uri;
+  nsresult rv = aPrincipal->GetURI(getter_AddRefs(uri));
+  NS_ENSURE_SUCCESS(rv, false);
+
+  bool hasFlags;
+
+  // Allow access to uris that cannot be loaded by web content.
+  rv = NS_URIChainHasFlags(uri, nsIProtocolHandler::URI_DANGEROUS_TO_LOAD,
+                           &hasFlags);
+  NS_ENSURE_SUCCESS(rv, false);
+  if (hasFlags) {
+    return true;
+  }
+
+  // UI resources also get access.
+  rv = NS_URIChainHasFlags(uri, nsIProtocolHandler::URI_IS_UI_RESOURCE,
+                           &hasFlags);
+  NS_ENSURE_SUCCESS(rv, false);
+  return hasFlags;
+}
+
 bool nsContentUtils::MaybeAllowOfflineAppByDefault(nsIPrincipal* aPrincipal) {
   if (!Preferences::GetRootBranch()) return false;
 
diff --git a/dom/base/nsContentUtils.h b/dom/base/nsContentUtils.h
index b80c6c91093b..46818cc43a1b 100644
--- a/dom/base/nsContentUtils.h
+++ b/dom/base/nsContentUtils.h
@@ -1984,6 +1984,13 @@ class nsContentUtils {
   static bool OfflineAppAllowed(nsIPrincipal* aPrincipal);
 
   /**
+   * Determine whether the principal is allowed access to the localization
+   * system. We don't want the web to ever see this but all our UI including in
+   * content pages should pass this test.
+   */
+  static bool PrincipalAllowsL10n(nsIPrincipal* aPrincipal);
+
+  /**
    * If offline-apps.allow_by_default is true, we set offline-app permission
    * for the principal and return true.  Otherwise false.
    */
diff --git a/dom/security/nsContentSecurityManager.cpp b/dom/security/nsContentSecurityManager.cpp
index d7724ff496ff..e7a0a9d1a72b 100644
--- a/dom/security/nsContentSecurityManager.cpp
+++ b/dom/security/nsContentSecurityManager.cpp
@@ -329,8 +329,20 @@ static bool IsImageLoadInEditorAppType(nsILoadInfo* aLoadInfo) {
 }
 
 static nsresult DoCheckLoadURIChecks(nsIURI* aURI, nsILoadInfo* aLoadInfo) {
-  // Bug 1228117: determine the correct security policy for DTD loads
-  if (aLoadInfo->GetExternalContentPolicyType() == nsIContentPolicy::TYPE_DTD) {
+  // In practice, these DTDs are just used for localization, so applying the
+  // same principal check as Fluent.
+  if (aLoadInfo->InternalContentPolicyType() ==
+      nsIContentPolicy::TYPE_INTERNAL_DTD) {
+    return nsContentUtils::PrincipalAllowsL10n(aLoadInfo->TriggeringPrincipal())
+               ? NS_OK
+               : NS_ERROR_DOM_BAD_URI;
+  }
+
+  // This is used in order to allow a privileged DOMParser to parse documents
+  // that need to access localization DTDs. We just allow through
+  // TYPE_INTERNAL_FORCE_ALLOWED_DTD no matter what the triggering principal is.
+  if (aLoadInfo->InternalContentPolicyType() ==
+      nsIContentPolicy::TYPE_INTERNAL_FORCE_ALLOWED_DTD) {
     return NS_OK;
   }
 
diff --git a/dom/tests/mochitest/bugs/mochitest.ini b/dom/tests/mochitest/bugs/mochitest.ini
index 571ca80e6339..26ce4e2b8c47 100644
--- a/dom/tests/mochitest/bugs/mochitest.ini
+++ b/dom/tests/mochitest/bugs/mochitest.ini
@@ -152,5 +152,6 @@ skip-if = toolkit == 'android'
 [test_bug1171215.html]
 support-files = window_bug1171215.html
 [test_bug1530292.html]
+[test_bug467035.html]
 [test_no_find_showDialog.html]
 skip-if = toolkit == 'android' # Bug 1358633 - window.find doesn't work for Android
diff --git a/dom/tests/mochitest/bugs/test_bug467035.html b/dom/tests/mochitest/bugs/test_bug467035.html
new file mode 100644
index 000000000000..ffcfe03e7c61
--- /dev/null
+++ b/dom/tests/mochitest/bugs/test_bug467035.html
@@ -0,0 +1,45 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=467035
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 467035</title>
+  <script src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+  <script type="application/javascript">
+
+  /** Test for Bug 467035 **/
+  SimpleTest.waitForExplicitFinish();
+  addLoadEvent(() => {
+    const s = `<!DOCTYPE html SYSTEM \"chrome://branding/locale/brand.dtd\">
+      <html xmlns=\"http://www.w3.org/1999/xhtml\">
+        <head>
+          <meta charset=\"utf-8\"/>
+          <title>&brandShortName;</title>
+        </head>
+      </html>`;
+
+    const parser = new DOMParser();
+    let doc = parser.parseFromString(s, 'application/xhtml+xml');
+    is(doc.getElementsByTagName('parsererror').length, 1, 'parseFromString cannot access locale DTD');
+
+    SpecialPowers.wrap(parser).forceEnableDTD();
+    doc = parser.parseFromString(s, 'application/xhtml+xml');
+    const isTitleLocalized = doc.getElementsByTagName('parsererror').length === 0 &&
+      typeof doc.title === 'string' &&
+      !!doc.title;
+    ok(isTitleLocalized, 'parseFromString can access locale DTD with forceEnableDTD');
+
+    SimpleTest.finish();
+  });
+  </script>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=467035">Mozilla Bug 467035</a>
+<p id="display"></p>
+<pre id="test">
+</pre>
+</body>
+</html>
diff --git a/dom/webidl/DOMParser.webidl b/dom/webidl/DOMParser.webidl
index 8afff03da225..6497374aff16 100644
--- a/dom/webidl/DOMParser.webidl
+++ b/dom/webidl/DOMParser.webidl
@@ -36,5 +36,10 @@ interface DOMParser {
   // principal it's using for the document.
   [ChromeOnly]
   void forceEnableXULXBL();
+
+  // Can be used to allow a DOMParser to load DTDs from URLs that
+  // normally would not be allowed based on the document principal.
+  [Func="IsChromeOrXBLOrUAWidget"]
+    void forceEnableDTD();
 };
 
diff --git a/parser/htmlparser/nsExpatDriver.cpp b/parser/htmlparser/nsExpatDriver.cpp
index dd09ba67d67e..9f2321cd2831 100644
--- a/parser/htmlparser/nsExpatDriver.cpp
+++ b/parser/htmlparser/nsExpatDriver.cpp
@@ -633,12 +633,16 @@ nsresult nsExpatDriver::OpenInputStreamFromExternalDTD(const char16_t* aFPIStr,
         mSink == nsCOMPtr<nsIExpatSink>(do_QueryInterface(mOriginalSink)),
         "In nsExpatDriver::OpenInputStreamFromExternalDTD: "
         "mOriginalSink not the same object as mSink?");
+    nsContentPolicyType policyType = nsIContentPolicy::TYPE_INTERNAL_DTD;
     nsCOMPtr<nsIPrincipal> loadingPrincipal;
     if (mOriginalSink) {
       nsCOMPtr<Document> doc;
       doc = do_QueryInterface(mOriginalSink->GetTarget());
       if (doc) {
         loadingPrincipal = doc->NodePrincipal();
+        if (doc->SkipDTDSecurityChecks()) {
+          policyType = nsIContentPolicy::TYPE_INTERNAL_FORCE_ALLOWED_DTD;
+        }
       }
     }
     if (!loadingPrincipal) {
@@ -648,7 +652,7 @@ nsresult nsExpatDriver::OpenInputStreamFromExternalDTD(const char16_t* aFPIStr,
     rv = NS_NewChannel(getter_AddRefs(channel), uri, loadingPrincipal,
                        nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS |
                            nsILoadInfo::SEC_ALLOW_CHROME,
-                       nsIContentPolicy::TYPE_DTD);
+                       policyType);
   }
   NS_ENSURE_SUCCESS(rv, rv);
 
diff --git a/testing/marionette/l10n.js b/testing/marionette/l10n.js
index ebbe071f483b..b4c6a98b9c14 100644
--- a/testing/marionette/l10n.js
+++ b/testing/marionette/l10n.js
@@ -21,7 +21,11 @@ const { XPCOMUtils } = ChromeUtils.import(
 );
 
 XPCOMUtils.defineLazyGlobalGetters(this, ["DOMParser"]);
-XPCOMUtils.defineLazyGetter(this, "domParser", () => new DOMParser());
+XPCOMUtils.defineLazyGetter(this, "domParser", () => {
+  const parser = new DOMParser();
+  parser.forceEnableDTD();
+  return parser;
+});
 
 const { NoSuchElementError } = ChromeUtils.import(
   "chrome://marionette/content/error.js"
diff --git a/testing/marionette/puppeteer/firefox/firefox_puppeteer/api/l10n.py b/testing/marionette/puppeteer/firefox/firefox_puppeteer/api/l10n.py
index 159f8dc29e46..9a61bf89439d 100644
--- a/testing/marionette/puppeteer/firefox/firefox_puppeteer/api/l10n.py
+++ b/testing/marionette/puppeteer/firefox/firefox_puppeteer/api/l10n.py
@@ -75,6 +75,7 @@ class L10n(BaseLib):
                 value = self.marionette.execute_script("""
                     Cu.importGlobalProperties(["DOMParser"]);
                     var parser = new DOMParser();
+                    parser.forceEnableDTD();
                     var doc = parser.parseFromString(arguments[0], "text/xml");
                     var node = doc.querySelector("elem[id='entity']");
 
diff --git a/toolkit/content/widgets/datetimebox.js b/toolkit/content/widgets/datetimebox.js
index 1865007a23ad..5153c3cfaf53 100644
--- a/toolkit/content/widgets/datetimebox.js
+++ b/toolkit/content/widgets/datetimebox.js
@@ -143,6 +143,7 @@ this.DateTimeInputBaseImplWidget = class {
      * Remove it when migrate to Fluent (bug 1504363).
      */
     const parser = new this.window.DOMParser();
+    parser.forceEnableDTD();
     let parserDoc = parser.parseFromString(
       `<!DOCTYPE bindings [
       <!ENTITY % datetimeboxDTD SYSTEM "chrome://global/locale/datetimebox.dtd">
diff --git a/toolkit/content/widgets/pluginProblem.js b/toolkit/content/widgets/pluginProblem.js
index e2cd6a0b6138..aee9942daa5a 100644
--- a/toolkit/content/widgets/pluginProblem.js
+++ b/toolkit/content/widgets/pluginProblem.js
@@ -17,6 +17,7 @@ this.PluginProblemWidget = class {
 
   onsetup() {
     const parser = new this.window.DOMParser();
+    parser.forceEnableDTD();
     let parserDoc = parser.parseFromString(
       `
       <!DOCTYPE bindings [
diff --git a/toolkit/content/widgets/videocontrols.js b/toolkit/content/widgets/videocontrols.js
index 991c01534229..a32d3e2261ae 100644
--- a/toolkit/content/widgets/videocontrols.js
+++ b/toolkit/content/widgets/videocontrols.js
@@ -2472,6 +2472,7 @@ this.VideoControlsImplWidget = class {
      * Remove it when migrate to Fluent.
      */
     const parser = new this.window.DOMParser();
+    parser.forceEnableDTD();
     let parserDoc = parser.parseFromString(
       `<!DOCTYPE bindings [
       <!ENTITY % videocontrolsDTD SYSTEM "chrome://global/locale/videocontrols.dtd">
@@ -2719,6 +2720,7 @@ this.NoControlsMobileImplWidget = class {
      * Remove it when migrate to Fluent.
      */
     const parser = new this.window.DOMParser();
+    parser.forceEnableDTD();
     let parserDoc = parser.parseFromString(
       `<!DOCTYPE bindings [
       <!ENTITY % videocontrolsDTD SYSTEM "chrome://global/locale/videocontrols.dtd">
@@ -2769,6 +2771,7 @@ this.NoControlsPictureInPictureImplWidget = class {
      * Remove it when migrate to Fluent.
      */
     const parser = new this.window.DOMParser();
+    parser.forceEnableDTD();
     let parserDoc = parser.parseFromString(
       `<!DOCTYPE bindings [
       <!ENTITY % videocontrolsDTD SYSTEM "chrome://global/locale/videocontrols.dtd">
@@ -2875,6 +2878,7 @@ this.NoControlsDesktopImplWidget = class {
      * Remove it when migrate to Fluent.
      */
     const parser = new this.window.DOMParser();
+    parser.forceEnableDTD();
     let parserDoc = parser.parseFromString(
       `<!DOCTYPE bindings [
       <!ENTITY % videocontrolsDTD SYSTEM "chrome://global/locale/videocontrols.dtd">





More information about the tbb-commits mailing list