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/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);

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]

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?

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

dom/base/Document.cpp

@@ -1300,6 +1300,7 @@ Document::Document(const char* aContentType)
       mType(eUnknown),
       mDefaultElementType(0),
       mAllowXULXBL(eTriUnset),
+      mSkipDTDSecurityChecks(false),
       mBidiOptions(IBMBIDI_DEFAULT_BIDI_OPTIONS),
       mSandboxFlags(0),
       mPartID(0),
@@ -2287,38 +2288,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
@@ -3652,11 +3621,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;
   }
 
@@ -3694,7 +3663,7 @@ void Document::LocalizationLinkAdded(Element* aLinkElement) {
 }
 
 void Document::LocalizationLinkRemoved(Element* aLinkElement) {
-  if (!PrincipalAllowsL10n(NodePrincipal())) {
+  if (!nsContentUtils::PrincipalAllowsL10n(NodePrincipal())) {
     return;
   }
 

dom/base/Document.h

@@ -2997,8 +2997,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.
@@ -4811,6 +4819,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.

dom/base/nsContentUtils.cpp

@@ -1685,6 +1685,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;
 

dom/base/nsContentUtils.h

@@ -1976,6 +1976,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.

dom/security/nsContentSecurityManager.cpp

@@ -328,8 +328,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;
   }
 

dom/tests/mochitest/bugs/mochitest.ini

@@ -154,5 +154,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

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>

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();
 };
 

parser/htmlparser/nsExpatDriver.cpp

@@ -643,12 +643,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) {
@@ -658,7 +662,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);
 

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"

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']");
 

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">

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 [

toolkit/content/widgets/videocontrols.js

@@ -2546,6 +2546,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">
@@ -2793,6 +2794,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">
@@ -2843,6 +2845,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">
@@ -2980,6 +2983,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">