[tor-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 tor-commits
mailing list