bug 1391207 - error pages are always not secure r=Gijs
authorDana Keeler <dkeeler@mozilla.com>
Mon, 15 Oct 2018 19:34:14 +0000
changeset 499769 29ecf05f6a7bcb7d9f11fc0cdc9fe69ab4489f3e
parent 499768 4ccf0f4b0ef23fc71f5593ca269e625c0cca5d56
child 499770 900eabf8fceeec876fe998b1b91d5ddd09bb20f2
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1391207
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1391207 - error pages are always not secure r=Gijs Before this patch, if a TLS handshake completed but the server then closed the connection without reading or writing, Firefox would display a connection reset error page with a secure lock icon. This is misleading and confusing, so in this patch, nsSecureBrowserUIImpl::OnLocationChange checks if an error page is being loaded and sets the state to not secure. Differential Revision: https://phabricator.services.mozilla.com/D8472
browser/base/content/test/siteIdentity/browser_navigation_failures.js
security/manager/ssl/nsSecureBrowserUIImpl.cpp
--- a/browser/base/content/test/siteIdentity/browser_navigation_failures.js
+++ b/browser/base/content/test/siteIdentity/browser_navigation_failures.js
@@ -1,17 +1,18 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 // Tests that the site identity indicator is properly updated for navigations
 // that fail for various reasons. In particular, we currently test TLS handshake
-// failures and about: pages that don't actually exist.
-// See bug 1492424 and bug 1493427.
+// failures, about: pages that don't actually exist, and situations where the
+// TLS handshake completes but the server then closes the connection.
+// See bug 1492424, bug 1493427, and bug 1391207.
 
 const kSecureURI = getRootDirectory(gTestPath).replace("chrome://mochitests/content",
                                                        "https://example.com") + "dummy_page.html";
 add_task(async function() {
   await BrowserTestUtils.withNewTab(kSecureURI, async (browser) => {
     let identityMode = window.document.getElementById("identity-box").className;
     is(identityMode, "verifiedDomain", "identity should be secure before");
 
@@ -34,8 +35,101 @@ add_task(async function() {
     // Try to load an about: page that doesn't exist
     BrowserTestUtils.loadURI(browser, BAD_ABOUT_PAGE_URI);
     await BrowserTestUtils.browserLoaded(browser, false, BAD_ABOUT_PAGE_URI, true);
 
     let newIdentityMode = window.document.getElementById("identity-box").className;
     is(newIdentityMode, "unknownIdentity", "identity should be unknown (not secure) after");
   });
 });
+
+// Helper function to start a TLS server that will accept a connection, complete
+// the TLS handshake, but then close the connection.
+function startServer(cert) {
+  let tlsServer = Cc["@mozilla.org/network/tls-server-socket;1"]
+                    .createInstance(Ci.nsITLSServerSocket);
+  tlsServer.init(-1, true, -1);
+  tlsServer.serverCert = cert;
+
+  let input, output;
+
+  let listener = {
+    onSocketAccepted(socket, transport) {
+      let connectionInfo = transport.securityInfo
+                           .QueryInterface(Ci.nsITLSServerConnectionInfo);
+      connectionInfo.setSecurityObserver(listener);
+      input = transport.openInputStream(0, 0, 0);
+      output = transport.openOutputStream(0, 0, 0);
+    },
+
+    onHandshakeDone(socket, status) {
+      input.asyncWait({
+        onInputStreamReady(readyInput) {
+          try {
+            input.close();
+            output.close();
+          } catch (e) {
+            info(e);
+          }
+        },
+      }, 0, 0, Services.tm.currentThread);
+    },
+
+    onStopListening() {
+    },
+  };
+
+  tlsServer.setSessionTickets(false);
+  tlsServer.asyncListen(listener);
+
+  return tlsServer;
+}
+
+// Test that if we complete a TLS handshake but the server closes the connection
+// just after doing so (resulting in a "connection reset" error page), the site
+// identity information gets updated appropriately (it should indicate "not
+// secure").
+add_task(async function() {
+  await SpecialPowers.pushPrefEnv({
+    // This test fails on some platforms if we leave IPv6 enabled.
+    set: [["network.dns.disableIPv6", true]],
+  });
+  let certService = Cc["@mozilla.org/security/local-cert-service;1"]
+                      .getService(Ci.nsILocalCertService);
+  let certOverrideService = Cc["@mozilla.org/security/certoverride;1"]
+                              .getService(Ci.nsICertOverrideService);
+
+  let cert = await new Promise((resolve, reject) => {
+    certService.getOrCreateCert("broken-tls-server", {
+      handleCert(c, rv) {
+        if (!Components.isSuccessCode(rv)) {
+          reject(rv);
+          return;
+        }
+        resolve(c);
+      },
+    });
+  });
+  // Start a server and trust its certificate.
+  let server = startServer(cert);
+  let overrideBits = Ci.nsICertOverrideService.ERROR_UNTRUSTED |
+                     Ci.nsICertOverrideService.ERROR_MISMATCH;
+  certOverrideService.rememberValidityOverride("localhost", server.port, cert,
+                                               overrideBits, true);
+
+  // Un-do configuration changes we've made when the test is done.
+  registerCleanupFunction(() => {
+    certOverrideService.clearValidityOverride("localhost", server.port);
+    server.close();
+  });
+
+  // Open up a new tab...
+  await BrowserTestUtils.withNewTab("about:blank", async (browser) => {
+    const TLS_HANDSHAKE_FAILURE_URI = `https://localhost:${server.port}/`;
+    // Try to connect to a server where the TLS handshake will succeed, but then
+    // the server closes the connection right after.
+    BrowserTestUtils.loadURI(browser, TLS_HANDSHAKE_FAILURE_URI);
+    await BrowserTestUtils.browserLoaded(browser, false, TLS_HANDSHAKE_FAILURE_URI, true);
+
+    let identityMode = window.document.getElementById("identity-box").className;
+    is(identityMode, "unknownIdentity", "identity should be 'unknown'");
+  });
+});
--- a/security/manager/ssl/nsSecureBrowserUIImpl.cpp
+++ b/security/manager/ssl/nsSecureBrowserUIImpl.cpp
@@ -398,31 +398,36 @@ nsSecureBrowserUIImpl::OnLocationChange(
 
   // Clear any state that varies by location.
   if (!(aFlags & LOCATION_CHANGE_SAME_DOCUMENT)) {
     mOldState = 0;
     mState = 0;
     mTopLevelSecurityInfo = nullptr;
   }
 
-  // NB: aRequest may be null. It may also not be QI-able to nsIChannel.
-  nsCOMPtr<nsIChannel> channel(do_QueryInterface(aRequest));
-  if (channel) {
-    MOZ_LOG(gSecureBrowserUILog, LogLevel::Debug,
-            ("  we have a channel %p", channel.get()));
-    nsresult rv = UpdateStateAndSecurityInfo(channel, aLocation);
-    // Even if this failed, we still want to notify downstream so that we don't
-    // leave a stale security indicator. We set everything to "not secure" to be
-    // safe.
-    if (NS_WARN_IF(NS_FAILED(rv))) {
+  if (aFlags & LOCATION_CHANGE_ERROR_PAGE) {
+    mState = STATE_IS_INSECURE;
+    mTopLevelSecurityInfo = nullptr;
+  } else {
+    // NB: aRequest may be null. It may also not be QI-able to nsIChannel.
+    nsCOMPtr<nsIChannel> channel(do_QueryInterface(aRequest));
+    if (channel) {
       MOZ_LOG(gSecureBrowserUILog, LogLevel::Debug,
-              ("  Failed to update security info. "
-               "Setting everything to 'not secure' to be safe."));
-      mState = STATE_IS_INSECURE;
-      mTopLevelSecurityInfo = nullptr;
+              ("  we have a channel %p", channel.get()));
+      nsresult rv = UpdateStateAndSecurityInfo(channel, aLocation);
+      // Even if this failed, we still want to notify downstream so that we don't
+      // leave a stale security indicator. We set everything to "not secure" to be
+      // safe.
+      if (NS_WARN_IF(NS_FAILED(rv))) {
+        MOZ_LOG(gSecureBrowserUILog, LogLevel::Debug,
+                ("  Failed to update security info. "
+                 "Setting everything to 'not secure' to be safe."));
+        mState = STATE_IS_INSECURE;
+        mTopLevelSecurityInfo = nullptr;
+      }
     }
   }
 
   mozilla::dom::ContentBlockingLog* contentBlockingLog = nullptr;
   nsCOMPtr<nsIDocShell> docShell = do_QueryReferent(mDocShell);
   if (docShell) {
     nsIDocument* doc = docShell->GetDocument();
     if (doc) {