Bug 1217766 - All PDFs trigger the insecure password warning. r=MattN,bz

dom/security/nsContentSecurityManager.cpp

@@ -518,8 +518,14 @@ nsContentSecurityManager::IsURIPotentiallyTrustworthy(nsIURI* aURI, bool* aIsTru
     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;

dom/security/test/unit/test_isURIPotentiallyTrustworthy.js

@@ -22,6 +22,7 @@ add_task(function* test_isURIPotentiallyTrustworthy() {
     ["http://localhost/", true],
     ["http://127.0.0.1/", true],
     ["file:///", true],
+    ["resource:///", true],
     ["about:config", false],
     ["urn:generic", false],
   ]) {

toolkit/components/passwordmgr/InsecurePasswordUtils.jsm

@@ -62,12 +62,11 @@ this.InsecurePasswordUtils = {
    * 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;
     }
@@ -84,8 +83,8 @@ this.InsecurePasswordUtils = {
    */
   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);

toolkit/components/passwordmgr/LoginManagerContent.jsm

@@ -25,6 +25,9 @@ XPCOMUtils.defineLazyModuleGetter(this, "LoginHelper",
 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");
@@ -416,9 +419,7 @@ var LoginManagerContent = {
     // 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,
@@ -1101,50 +1102,37 @@ var LoginManagerContent = {
     };
   },
 
-  /*
-   * 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.
    *
-   *   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.
+   * 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 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);
   },
 };
 

toolkit/components/passwordmgr/moz.build

@@ -13,6 +13,8 @@ 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',
 ]
 

toolkit/components/passwordmgr/test/browser/browser.ini

@@ -5,11 +5,13 @@ support-files =
   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]

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

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