commit 2fe57c62af9706be45fc2085796a9398c3b10763 Author: Alex Catarineu acat@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%5C%22%3E + <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">