Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz
--- a/dom/security/nsContentSecurityManager.cpp
+++ b/dom/security/nsContentSecurityManager.cpp
@@ -513,18 +513,24 @@ nsContentSecurityManager::IsURIPotential
*aIsTrustWorthy = false;
nsAutoCString scheme;
nsresult rv = aURI->GetScheme(scheme);
if (NS_FAILED(rv)) {
return NS_OK;
}
+ // According to the specification, the user agent may choose to extend the
+ // trust to other, vendor-specific URL schemes. We use this for "resource:",
+ // which is technically a substituting protocol handler that is not limited to
+ // local resource mapping, but in practice is never mapped remotely as this
+ // would violate assumptions a lot of code makes.
if (scheme.EqualsLiteral("https") ||
scheme.EqualsLiteral("file") ||
+ scheme.EqualsLiteral("resource") ||
scheme.EqualsLiteral("app") ||
scheme.EqualsLiteral("wss")) {
*aIsTrustWorthy = true;
return NS_OK;
}
nsAutoCString host;
rv = aURI->GetHost(host);
--- a/dom/security/test/unit/test_isURIPotentiallyTrustworthy.js
+++ b/dom/security/test/unit/test_isURIPotentiallyTrustworthy.js
@@ -17,16 +17,17 @@ XPCOMUtils.defineLazyServiceGetter(this,
add_task(function* test_isURIPotentiallyTrustworthy() {
for (let [uriSpec, expectedResult] of [
["http://example.com/", false],
["https://example.com/", true],
["http://localhost/", true],
["http://127.0.0.1/", true],
["file:///", true],
+ ["resource:///", true],
["about:config", false],
["urn:generic", false],
]) {
let uri = NetUtil.newURI(uriSpec);
Assert.equal(gContentSecurityManager.isURIPotentiallyTrustworthy(uri),
expectedResult);
}
});
--- a/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
+++ b/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
@@ -57,40 +57,39 @@ this.InsecurePasswordUtils = {
* because MITM attackers could replace https:// iframes if they are nested inside
* http:// documents with their own content, thus creating a security risk
* and potentially stealing user data. Under such scenario, a user might not
* get a Mixed Content Blocker message, if the main document is served over HTTP
* and framing an HTTPS page as it would under the reverse scenario (http
* inside https).
*/
_checkForInsecureNestedDocuments : function(domDoc) {
- let uri = domDoc.documentURIObject;
if (domDoc.defaultView == domDoc.defaultView.parent) {
// We are at the top, nothing to check here
return false;
}
- if (!LoginManagerContent.checkIfURIisSecure(uri)) {
+ if (!LoginManagerContent.isDocumentSecure(domDoc)) {
// We are insecure
return true;
}
// I am secure, but check my parent
return this._checkForInsecureNestedDocuments(domDoc.defaultView.parent.document);
},
/*
* Checks if there are insecure password fields present on the form's document
* i.e. passwords inside forms with http action, inside iframes with http src,
* or on insecure web pages. If insecure password fields are present,
* a log message is sent to the web console to warn developers.
*/
checkForInsecurePasswords : function (aForm) {
var domDoc = aForm.ownerDocument;
- let pageURI = domDoc.defaultView.top.document.documentURIObject;
- let isSafePage = LoginManagerContent.checkIfURIisSecure(pageURI);
+ let topDocument = domDoc.defaultView.top.document;
+ let isSafePage = LoginManagerContent.isDocumentSecure(topDocument);
if (!isSafePage) {
this._sendWebConsoleMessage("InsecurePasswordsPresentOnPage", domDoc);
}
// Check if we are on an iframe with insecure src, or inside another
// insecure iframe or document.
if (this._checkForInsecureNestedDocuments(domDoc)) {
--- a/toolkit/components/passwordmgr/LoginManagerContent.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ -20,16 +20,19 @@ XPCOMUtils.defineLazyModuleGetter(this,
"resource://gre/modules/LoginRecipes.jsm");
XPCOMUtils.defineLazyModuleGetter(this, "LoginHelper",
"resource://gre/modules/LoginHelper.jsm");
XPCOMUtils.defineLazyServiceGetter(this, "gContentSecurityManager",
"@mozilla.org/contentsecuritymanager;1",
"nsIContentSecurityManager");
+XPCOMUtils.defineLazyServiceGetter(this, "gNetUtil",
+ "@mozilla.org/network/util;1",
+ "nsINetUtil");
XPCOMUtils.defineLazyGetter(this, "log", () => {
let logger = LoginHelper.createLogger("LoginManagerContent");
return logger.log.bind(logger);
});
// These mirror signon.* prefs.
var gEnabled, gAutofillForms, gStoreWhenAutocompleteOff;
@@ -411,19 +414,17 @@ var LoginManagerContent = {
}
}
return null;
};
// Returns true if this window or any subframes have insecure login forms.
let hasInsecureLoginForms = (thisWindow, parentIsInsecure) => {
let doc = thisWindow.document;
- let isInsecure =
- parentIsInsecure ||
- !this.checkIfURIisSecure(doc.documentURIObject);
+ let isInsecure = parentIsInsecure || !this.isDocumentSecure(doc);
let hasLoginForm = !!this.stateForDocument(doc).loginForm;
return (hasLoginForm && isInsecure) ||
Array.some(thisWindow.frames,
frame => hasInsecureLoginForms(frame, isInsecure));
};
// Store the actual form to use on the state for the top-level document.
let topState = this.stateForDocument(topWindow.document);
@@ -1096,60 +1097,47 @@ var LoginManagerContent = {
},
passwordField: {
found: !!newPasswordField,
disabled: newPasswordField && (newPasswordField.disabled || newPasswordField.readOnly),
},
};
},
- /*
- * Checks whether the passed uri is secure
- * Check Protocol Flags to determine if scheme is secure:
- * URI_DOES_NOT_RETURN_DATA - e.g.
- * "mailto"
- * URI_IS_LOCAL_RESOURCE - e.g.
- * "data",
- * "resource",
- * "moz-icon"
- * URI_INHERITS_SECURITY_CONTEXT - e.g.
- * "javascript"
- * URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT - e.g.
- * "https",
- * "moz-safe-about"
+ /**
+ * Returns true if the provided document principal and URI are such that the
+ * page using them can safely host password fields.
+ *
+ * This means the page can't be easily tampered with because it is sent over
+ * an encrypted channel or is a local resource that never hits the network.
*
- * The use of this logic comes directly from nsMixedContentBlocker.cpp
- * At the time it was decided to include these protocols since a secure
- * uri for mixed content blocker means that the resource can't be
- * easily tampered with because 1) it is sent over an encrypted channel or
- * 2) it is a local resource that never hits the network
- * or 3) it is a request sent without any response that could alter
- * the behavior of the page. It was decided to include the same logic
- * here both to be consistent with MCB and to make sure we cover all
- * "safe" protocols. Eventually, the code here and the code in MCB
- * will be moved to a common location that will be referenced from
- * both places. Look at
- * https://bugzilla.mozilla.org/show_bug.cgi?id=899099 for more info.
+ * The system principal, codebase principals with secure schemes like "https",
+ * and local schemes like "resource" and "file" are all considered secure. If
+ * the page is sandboxed, then the document URI is checked instead.
+ *
+ * @param document
+ * The document whose principal and URI are to be considered.
*/
- checkIfURIisSecure : function(uri) {
- let isSafe = false;
- let netutil = Cc["@mozilla.org/network/util;1"].getService(Ci.nsINetUtil);
- let ph = Ci.nsIProtocolHandler;
-
- // Is the connection to localhost? Consider localhost safe for passwords.
- if (gContentSecurityManager.isURIPotentiallyTrustworthy(uri) ||
- netutil.URIChainHasFlags(uri, ph.URI_IS_LOCAL_RESOURCE) ||
- netutil.URIChainHasFlags(uri, ph.URI_DOES_NOT_RETURN_DATA) ||
- netutil.URIChainHasFlags(uri, ph.URI_INHERITS_SECURITY_CONTEXT) ||
- netutil.URIChainHasFlags(uri, ph.URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT)) {
-
- isSafe = true;
+ isDocumentSecure(document) {
+ let docPrincipal = document.nodePrincipal;
+ if (docPrincipal.isSystemPrincipal) {
+ return true;
}
- return isSafe;
+ // Fall back to the document URI for sandboxed documents that do not have
+ // the allow-same-origin flag, as they have a null principal instead of a
+ // codebase principal. Here there are still some cases that are considered
+ // insecure while they are secure, for example sandboxed documents created
+ // using a "javascript:" or "data:" URI from an HTTPS page. See bug 1162772
+ // for defining "window.isSecureContext", that may help in these cases.
+ let uri = docPrincipal.isCodebasePrincipal ? docPrincipal.URI
+ : document.documentURIObject;
+
+ // These checks include "file", "resource", HTTPS, and HTTP to "localhost".
+ return gContentSecurityManager.isURIPotentiallyTrustworthy(uri);
},
};
var LoginUtils = {
/**
* Get the parts of the URL we want for identification.
* Strip out things like the userPass portion
*/
--- a/toolkit/components/passwordmgr/moz.build
+++ b/toolkit/components/passwordmgr/moz.build
@@ -8,16 +8,18 @@ if CONFIG['MOZ_BUILD_APP'] == 'browser':
DEFINES['MOZ_BUILD_APP_IS_BROWSER'] = True
MOCHITEST_MANIFESTS += ['test/mochitest.ini']
MOCHITEST_CHROME_MANIFESTS += ['test/chrome.ini']
BROWSER_CHROME_MANIFESTS += ['test/browser/browser.ini']
XPCSHELL_TESTS_MANIFESTS += ['test/unit/xpcshell.ini']
TESTING_JS_MODULES += [
+ # Make this file available from the "resource:" URI of the test environment.
+ 'test/browser/form_basic.html',
'test/LoginTestUtils.jsm',
]
XPIDL_SOURCES += [
'nsILoginInfo.idl',
'nsILoginManager.idl',
'nsILoginManagerCrypto.idl',
'nsILoginManagerPrompter.idl',
--- a/toolkit/components/passwordmgr/test/browser/browser.ini
+++ b/toolkit/components/passwordmgr/test/browser/browser.ini
@@ -1,20 +1,22 @@
[DEFAULT]
support-files =
authenticate.sjs
form_basic.html
insecure_test.html
insecure_test_subframe.html
multiple_forms.html
+ streamConverter_content.sjs
[browser_DOMFormHasPassword.js]
[browser_DOMInputPasswordAdded.js]
[browser_filldoorhanger.js]
[browser_hasInsecureLoginForms.js]
+[browser_hasInsecureLoginForms_streamConverter.js]
[browser_notifications.js]
skip-if = true # Intermittent failures: Bug 1182296, bug 1148771
[browser_passwordmgr_editing.js]
skip-if = os == "linux"
[browser_context_menu.js]
skip-if = os == "linux"
[browser_passwordmgr_contextmenu.js]
[browser_passwordmgr_fields.js]
new file mode 100644
--- /dev/null
+++ b/toolkit/components/passwordmgr/test/browser/browser_hasInsecureLoginForms_streamConverter.js
@@ -0,0 +1,101 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+Cu.import("resource://gre/modules/LoginManagerParent.jsm", this);
+
+function* registerConverter() {
+ Cu.import("resource://gre/modules/Services.jsm", this);
+ Cu.import("resource://gre/modules/NetUtil.jsm", this);
+
+ /**
+ * Converts the "test/content" MIME type, served by the test over HTTP, to an
+ * HTML viewer page containing the "form_basic.html" code. The viewer is
+ * served from a "resource:" URI while keeping the "resource:" principal.
+ */
+ function TestStreamConverter() {}
+
+ TestStreamConverter.prototype = {
+ classID: Components.ID("{5f01d6ef-c090-45a4-b3e5-940d64713eb7}"),
+ contractID: "@mozilla.org/streamconv;1?from=test/content&to=*/*",
+ QueryInterface: XPCOMUtils.generateQI([
+ Ci.nsIRequestObserver,
+ Ci.nsIStreamListener,
+ Ci.nsIStreamConverter,
+ ]),
+
+ // nsIStreamConverter
+ convert() {},
+
+ // nsIStreamConverter
+ asyncConvertData(aFromType, aToType, aListener, aCtxt) {
+ this.listener = aListener;
+ },
+
+ // nsIRequestObserver
+ onStartRequest(aRequest, aContext) {
+ let channel = NetUtil.newChannel({
+ uri: "resource://testing-common/form_basic.html",
+ loadUsingSystemPrincipal: true,
+ });
+ channel.originalURI = aRequest.QueryInterface(Ci.nsIChannel).URI;
+ channel.loadGroup = aRequest.loadGroup;
+ channel.owner = Services.scriptSecurityManager
+ .createCodebasePrincipal(channel.URI, {});
+ // In this test, we pass the new channel to the listener but don't fire a
+ // redirect notification, even if it would be required. This keeps the
+ // test code simpler and doesn't impact the principal check we're testing.
+ channel.asyncOpen(this.listener, aContext);
+ },
+
+ // nsIRequestObserver
+ onStopRequest() {},
+
+ // nsIStreamListener
+ onDataAvailable() {},
+ };
+
+ let factory = XPCOMUtils._getFactory(TestStreamConverter);
+ let registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);
+ registrar.registerFactory(TestStreamConverter.prototype.classID, "",
+ TestStreamConverter.prototype.contractID, factory);
+ this.cleanupFunction = function () {
+ registrar.unregisterFactory(TestStreamConverter.prototype.classID, factory);
+ };
+}
+
+/**
+ * Waits for the given number of occurrences of InsecureLoginFormsStateChange
+ * on the given browser element.
+ */
+function waitForInsecureLoginFormsStateChange(browser, count) {
+ return BrowserTestUtils.waitForEvent(browser, "InsecureLoginFormsStateChange",
+ false, () => --count == 0);
+}
+
+/**
+ * Checks that hasInsecureLoginForms is false for a viewer served internally
+ * using a "resource:" URI.
+ */
+add_task(function* test_streamConverter() {
+ let originalBrowser = gBrowser.selectedBrowser;
+
+ yield ContentTask.spawn(originalBrowser, null, registerConverter);
+
+ let tab = gBrowser.addTab("http://example.com/browser/toolkit/components/" +
+ "passwordmgr/test/browser/streamConverter_content.sjs");
+ let browser = tab.linkedBrowser;
+ yield Promise.all([
+ BrowserTestUtils.switchTab(gBrowser, tab),
+ BrowserTestUtils.browserLoaded(browser),
+ // One event is triggered by pageshow and one by DOMFormHasPassword.
+ waitForInsecureLoginFormsStateChange(browser, 2),
+ ]);
+
+ Assert.ok(!LoginManagerParent.hasInsecureLoginForms(browser));
+
+ yield BrowserTestUtils.removeTab(tab);
+
+ yield ContentTask.spawn(originalBrowser, null, function* () {
+ this.cleanupFunction();
+ });
+});
new file mode 100644
--- /dev/null
+++ b/toolkit/components/passwordmgr/test/browser/streamConverter_content.sjs
@@ -0,0 +1,6 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+function handleRequest(request, response) {
+ response.setHeader("Content-Type", "test/content", false);
+}