Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz draft
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Mon, 18 Jan 2016 14:54:18 +0000
changeset 322392 49375a644ff345fe479b060945f6fe61efe52df7
parent 322328 8cb42e7a16b42162c9930f37b9e1f820c2eb126b
child 513100 ec3b4e342fbf237e9992eab5ac8b73c48d520295
push id9603
push userpaolo.mozmail@amadzone.org
push dateMon, 18 Jan 2016 14:55:20 +0000
reviewersMattN, bz
bugs1217766
milestone46.0a1
Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz
dom/security/nsContentSecurityManager.cpp
dom/security/test/unit/test_isURIPotentiallyTrustworthy.js
toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
toolkit/components/passwordmgr/LoginManagerContent.jsm
toolkit/components/passwordmgr/moz.build
toolkit/components/passwordmgr/test/browser/browser.ini
toolkit/components/passwordmgr/test/browser/browser_hasInsecureLoginForms_streamConverter.js
toolkit/components/passwordmgr/test/browser/streamConverter_content.sjs
--- 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);
+}