Bug 1900132 - attempt to redirect www.example.com to example.com to avoid certificate domain name mismatch errors r=jschanck,smaug

Differential Revision: https://phabricator.services.mozilla.com/D212329

build/pgo/certs/badCertDomain2.certspec

@@ -0,0 +1,3 @@
+subject:badcertdomain2.example.com
+issuer:printableString/CN=Temporary Certificate Authority/O=Mozilla Testing/OU=Profile Guided Optimization
+extension:subjectAlternativeName:badcertdomain2.example.com

build/pgo/server-locations.txt

@@ -308,12 +308,14 @@ https://bad.include-subdomains.pinning-dynamic.example.com:443    privileged,cer
 https://badchain.include-subdomains.pinning.example.com:443       privileged,cert=staticPinningBad
 https://fail-handshake.example.com:443                            privileged,failHandshake
 
-# Host for bad cert domain fixup test
+# Hosts for bad cert domain fixup tests
 https://badcertdomain.example.com:443                             privileged,cert=badCertDomain
 https://www.badcertdomain.example.com:443                         privileged,cert=badCertDomain
 https://127.0.0.3:433                                             privileged,cert=badCertDomain
 https://badcertdomain.example.com:82                              privileged,cert=badCertDomain
 https://mismatch.badcertdomain.example.com:443                    privileged,cert=badCertDomain
+https://badcertdomain2.example.com:443                            privileged,cert=badCertDomain2
+https://www.badcertdomain2.example.com:443                        privileged,cert=badCertDomain2
 
 # Hosts for HTTPS-First upgrades/downgrades
 http://httpsfirst.com:80                                          privileged

docshell/base/nsDocShell.cpp

@@ -232,6 +232,7 @@
 #include "nsWidgetsCID.h"
 #include "nsXULAppAPI.h"
 
+#include "CertVerifier.h"
 #include "ThirdPartyUtil.h"
 #include "GeckoProfiler.h"
 #include "mozilla/NullPrincipal.h"
@@ -5775,12 +5776,6 @@ already_AddRefed<nsIURI> nsDocShell::MaybeFixBadCertDomainErrorURI(
     return nullptr;
   }
 
-  // No point in going further if "www." is included in the hostname
-  // already. That is the only hueristic we're applying in this function.
-  if (StringBeginsWith(host, "www."_ns)) {
-    return nullptr;
-  }
-
   // Return if fixup enable pref is turned off.
   if (!mozilla::StaticPrefs::security_bad_cert_domain_error_url_fix_enabled()) {
     return nullptr;
@@ -5857,27 +5852,45 @@ already_AddRefed<nsIURI> nsDocShell::MaybeFixBadCertDomainErrorURI(
   }
 
   mozilla::pkix::Input serverCertInput;
-  mozilla::pkix::Result rv1 =
+  mozilla::pkix::Result result =
       serverCertInput.Init(certBytes.Elements(), certBytes.Length());
-  if (rv1 != mozilla::pkix::Success) {
+  if (result != mozilla::pkix::Success) {
     return nullptr;
   }
 
-  nsAutoCString newHost("www."_ns);
+  constexpr auto wwwPrefix = "www."_ns;
-  newHost.Append(host);
+  nsAutoCString newHost;
+  if (StringBeginsWith(host, wwwPrefix)) {
+    // Try www.example.com -> example.com
+    newHost.Assign(Substring(host, wwwPrefix.Length()));
+  } else {
+    // Try example.com -> www.example.com
+    newHost.Assign(wwwPrefix);
+    newHost.Append(host);
+  }
 
   mozilla::pkix::Input newHostInput;
-  rv1 = newHostInput.Init(
+  result = newHostInput.Init(
       BitwiseCast<const uint8_t*, const char*>(newHost.BeginReading()),
       newHost.Length());
-  if (rv1 != mozilla::pkix::Success) {
+  if (result != mozilla::pkix::Success) {
     return nullptr;
   }
 
-  // Check if adding a "www." prefix to the request's hostname will
+  // Because certificate verification returned Result::ERROR_BAD_CERT_DOMAIN /
-  // cause the response's certificate to match.
+  // SSL_ERROR_BAD_CERT_DOMAIN, a chain was built and we know whether or not
-  rv1 = mozilla::pkix::CheckCertHostname(serverCertInput, newHostInput);
+  // the root was a built-in.
-  if (rv1 != mozilla::pkix::Success) {
+  bool rootIsBuiltIn;
+  if (NS_FAILED(tsi->GetIsBuiltCertChainRootBuiltInRoot(&rootIsBuiltIn))) {
+    return nullptr;
+  }
+  mozilla::psm::SkipInvalidSANsForNonBuiltInRootsPolicy nameMatchingPolicy(
+      rootIsBuiltIn);
+
+  // Check if the certificate is valid for the new hostname.
+  result = mozilla::pkix::CheckCertHostname(serverCertInput, newHostInput,
+                                            nameMatchingPolicy);
+  if (result != mozilla::pkix::Success) {
     return nullptr;
   }
 
@@ -6062,9 +6075,10 @@ already_AddRefed<nsIURI> nsDocShell::AttemptURIFixup(
     }
   }
 
-  // If we have a SSL_ERROR_BAD_CERT_DOMAIN error, try prefixing the domain name
+  // If we have a SSL_ERROR_BAD_CERT_DOMAIN error, try adding or removing
-  // with www. to see if we can avoid showing the cert error page. For example,
+  // "www." to/from the beginning of the domain name to see if we can avoid
-  // https://example.com -> https://www.example.com.
+  // showing the cert error page. For example, https://example.com ->
+  // https://www.example.com or https://www.example.com -> https://example.com.
   if (aStatus ==
       mozilla::psm::GetXPCOMFromNSSError(SSL_ERROR_BAD_CERT_DOMAIN)) {
     newPostData = nullptr;

docshell/test/browser/browser_badCertDomainFixup.js

@@ -7,19 +7,6 @@
 // with www. when we encounter a SSL_ERROR_BAD_CERT_DOMAIN error.
 // For example, https://example.com -> https://www.example.com.
 
-const PREF_BAD_CERT_DOMAIN_FIX_ENABLED =
-  "security.bad_cert_domain_error.url_fix_enabled";
-const PREF_ALLOW_HIJACKING_LOCALHOST =
-  "network.proxy.allow_hijacking_localhost";
-
-const BAD_CERT_DOMAIN_ERROR_URL = "https://badcertdomain.example.com:443";
-const FIXED_URL = "https://www.badcertdomain.example.com/";
-
-const BAD_CERT_DOMAIN_ERROR_URL2 =
-  "https://mismatch.badcertdomain.example.com:443";
-const IPV4_ADDRESS = "https://127.0.0.3:433";
-const BAD_CERT_DOMAIN_ERROR_PORT = "https://badcertdomain.example.com:82";
-
 async function verifyErrorPage(errorPageURL) {
   let certErrorLoaded = BrowserTestUtils.waitForErrorPage(
     gBrowser.selectedBrowser
@@ -41,52 +28,73 @@ async function verifyErrorPage(errorPageURL) {
   });
 }
 
+// Turn off the pref and ensure that we show the error page as expected.
+add_task(async function testNoFixupDisabledByPref() {
+  await SpecialPowers.pushPrefEnv({
+    set: [["security.bad_cert_domain_error.url_fix_enabled", false]],
+  });
+  gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
+
+  await verifyErrorPage("https://badcertdomain.example.com");
+  await verifyErrorPage("https://www.badcertdomain2.example.com");
+
+  BrowserTestUtils.removeTab(gBrowser.selectedTab);
+  await SpecialPowers.popPrefEnv();
+});
+
 // Test that "www." is prefixed to a https url when we encounter a bad cert domain
 // error if the "www." form is included in the certificate's subjectAltNames.
-add_task(async function prefixBadCertDomain() {
+add_task(async function testAddPrefixForBadCertDomain() {
-  // Turn off the pref and ensure that we show the error page as expected.
-  Services.prefs.setBoolPref(PREF_BAD_CERT_DOMAIN_FIX_ENABLED, false);
-
-  gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
-  await verifyErrorPage(BAD_CERT_DOMAIN_ERROR_URL);
-  info("Cert error is shown as expected when the fixup pref is disabled");
-
-  // Turn on the pref and test that we fix the HTTPS URL.
-  Services.prefs.setBoolPref(PREF_BAD_CERT_DOMAIN_FIX_ENABLED, true);
   gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
   let loadSuccessful = BrowserTestUtils.browserLoaded(
     gBrowser.selectedBrowser,
     false,
-    FIXED_URL
+    "https://www.badcertdomain.example.com/"
+  );
+  BrowserTestUtils.startLoadingURIString(
+    gBrowser,
+    "https://badcertdomain.example.com"
   );
-  BrowserTestUtils.startLoadingURIString(gBrowser, BAD_CERT_DOMAIN_ERROR_URL);
   await loadSuccessful;
 
-  info("The URL was fixed as expected");
-
-  BrowserTestUtils.removeTab(gBrowser.selectedTab);
   BrowserTestUtils.removeTab(gBrowser.selectedTab);
 });
 
 // Test that we don't prefix "www." to a https url when we encounter a bad cert domain
 // error under certain conditions.
-add_task(async function ignoreBadCertDomain() {
+add_task(async function testNoFixupCases() {
-  Services.prefs.setBoolPref(PREF_BAD_CERT_DOMAIN_FIX_ENABLED, true);
   gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
 
   // Test for when "www." form is not present in the certificate.
-  await verifyErrorPage(BAD_CERT_DOMAIN_ERROR_URL2);
+  await verifyErrorPage("https://mismatch.badcertdomain.example.com");
-  info("Certificate error was shown as expected");
 
   // Test that urls with IP addresses are not fixed.
-  Services.prefs.setBoolPref(PREF_ALLOW_HIJACKING_LOCALHOST, true);
+  await SpecialPowers.pushPrefEnv({
-  await verifyErrorPage(IPV4_ADDRESS);
+    set: [["network.proxy.allow_hijacking_localhost", true]],
-  Services.prefs.clearUserPref(PREF_ALLOW_HIJACKING_LOCALHOST);
+  });
-  info("Certificate error was shown as expected for an IP address");
+  await verifyErrorPage("https://127.0.0.3:433");
+  await SpecialPowers.popPrefEnv();
 
   // Test that urls with ports are not fixed.
-  await verifyErrorPage(BAD_CERT_DOMAIN_ERROR_PORT);
+  await verifyErrorPage("https://badcertdomain.example.com:82");
-  info("Certificate error was shown as expected for a host with port");
+
+  BrowserTestUtils.removeTab(gBrowser.selectedTab);
+});
+
+// Test removing "www." prefix if the "www."-less form is included in the
+// certificate's subjectAltNames.
+add_task(async function testRemovePrefixForBadCertDomain() {
+  gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
+  let loadSuccessful = BrowserTestUtils.browserLoaded(
+    gBrowser.selectedBrowser,
+    false,
+    "https://badcertdomain2.example.com/"
+  );
+  BrowserTestUtils.startLoadingURIString(
+    gBrowser,
+    "https://www.badcertdomain2.example.com"
+  );
+  await loadSuccessful;
 
   BrowserTestUtils.removeTab(gBrowser.selectedTab);
 });

security/certverifier/CertVerifier.cpp

@@ -784,28 +784,6 @@ static bool CertIsSelfSigned(const BackCert& backCert, void* pinarg) {
   return rv == Success;
 }
 
-class SkipInvalidSANsForNonBuiltInRootsPolicy : public NameMatchingPolicy {
- public:
-  explicit SkipInvalidSANsForNonBuiltInRootsPolicy(bool rootIsBuiltIn)
-      : mRootIsBuiltIn(rootIsBuiltIn) {}
-
-  virtual Result FallBackToCommonName(
-      Time,
-      /*out*/ FallBackToSearchWithinSubject& fallBackToCommonName) override {
-    fallBackToCommonName = FallBackToSearchWithinSubject::No;
-    return Success;
-  }
-
-  virtual HandleInvalidSubjectAlternativeNamesBy
-  HandleInvalidSubjectAlternativeNames() override {
-    return mRootIsBuiltIn ? HandleInvalidSubjectAlternativeNamesBy::Halting
-                          : HandleInvalidSubjectAlternativeNamesBy::Skipping;
-  }
-
- private:
-  bool mRootIsBuiltIn;
-};
-
 static Result CheckCertHostnameHelper(Input peerCertInput,
                                       const nsACString& hostname,
                                       bool rootIsBuiltIn) {

security/certverifier/CertVerifier.h

@@ -135,6 +135,31 @@ class DelegatedCredentialInfo {
   uint32_t authKeyBits;
 };
 
+class SkipInvalidSANsForNonBuiltInRootsPolicy
+    : public pkix::NameMatchingPolicy {
+ public:
+  explicit SkipInvalidSANsForNonBuiltInRootsPolicy(bool rootIsBuiltIn)
+      : mRootIsBuiltIn(rootIsBuiltIn) {}
+
+  virtual pkix::Result FallBackToCommonName(
+      pkix::Time,
+      /*out*/ pkix::FallBackToSearchWithinSubject& fallBackToCommonName)
+      override {
+    fallBackToCommonName = pkix::FallBackToSearchWithinSubject::No;
+    return pkix::Success;
+  }
+
+  virtual pkix::HandleInvalidSubjectAlternativeNamesBy
+  HandleInvalidSubjectAlternativeNames() override {
+    return mRootIsBuiltIn
+               ? pkix::HandleInvalidSubjectAlternativeNamesBy::Halting
+               : pkix::HandleInvalidSubjectAlternativeNamesBy::Skipping;
+  }
+
+ private:
+  bool mRootIsBuiltIn;
+};
+
 class NSSCertDBTrustDomain;
 
 class CertVerifier {