diff --git a/build/pgo/certs/badCertDomain2.certspec b/build/pgo/certs/badCertDomain2.certspec new file mode 100644 index 000000000000..d70891a215e4 --- /dev/null +++ b/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 diff --git a/build/pgo/certs/cert9.db b/build/pgo/certs/cert9.db index 92235bfa3102..9005540772bb 100644 Binary files a/build/pgo/certs/cert9.db and b/build/pgo/certs/cert9.db differ diff --git a/build/pgo/certs/key4.db b/build/pgo/certs/key4.db index 870045623850..59d870a9b4b3 100644 Binary files a/build/pgo/certs/key4.db and b/build/pgo/certs/key4.db differ diff --git a/build/pgo/certs/mochitest.client b/build/pgo/certs/mochitest.client index d19a92306dc1..c8a0c8a478e0 100644 Binary files a/build/pgo/certs/mochitest.client and b/build/pgo/certs/mochitest.client differ diff --git a/build/pgo/server-locations.txt b/build/pgo/server-locations.txt index 7b8b65804266..ddd393968764 100644 --- a/build/pgo/server-locations.txt +++ b/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 diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp index a164412f7c48..0dbf1c30f810 100644 --- a/docshell/base/nsDocShell.cpp +++ b/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 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 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); - newHost.Append(host); + constexpr auto wwwPrefix = "www."_ns; + 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(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 - // cause the response's certificate to match. - rv1 = mozilla::pkix::CheckCertHostname(serverCertInput, newHostInput); - if (rv1 != mozilla::pkix::Success) { + // Because certificate verification returned Result::ERROR_BAD_CERT_DOMAIN / + // SSL_ERROR_BAD_CERT_DOMAIN, a chain was built and we know whether or not + // the root was a built-in. + 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 nsDocShell::AttemptURIFixup( } } - // If we have a SSL_ERROR_BAD_CERT_DOMAIN error, try prefixing the domain name - // with www. to see if we can avoid showing the cert error page. For example, - // https://example.com -> https://www.example.com. + // If we have a SSL_ERROR_BAD_CERT_DOMAIN error, try adding or removing + // "www." to/from the beginning of the domain name to see if we can avoid + // 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; diff --git a/docshell/test/browser/browser_badCertDomainFixup.js b/docshell/test/browser/browser_badCertDomainFixup.js index 4bf6ddcef309..1bc8cfa47717 100644 --- a/docshell/test/browser/browser_badCertDomainFixup.js +++ b/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() { - // 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); +add_task(async function testAddPrefixForBadCertDomain() { 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() { - Services.prefs.setBoolPref(PREF_BAD_CERT_DOMAIN_FIX_ENABLED, true); +add_task(async function testNoFixupCases() { gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser); // Test for when "www." form is not present in the certificate. - await verifyErrorPage(BAD_CERT_DOMAIN_ERROR_URL2); - info("Certificate error was shown as expected"); + await verifyErrorPage("https://mismatch.badcertdomain.example.com"); // Test that urls with IP addresses are not fixed. - Services.prefs.setBoolPref(PREF_ALLOW_HIJACKING_LOCALHOST, true); - await verifyErrorPage(IPV4_ADDRESS); - Services.prefs.clearUserPref(PREF_ALLOW_HIJACKING_LOCALHOST); - info("Certificate error was shown as expected for an IP address"); + await SpecialPowers.pushPrefEnv({ + set: [["network.proxy.allow_hijacking_localhost", true]], + }); + 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); - info("Certificate error was shown as expected for a host with port"); + await verifyErrorPage("https://badcertdomain.example.com:82"); + + 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); }); diff --git a/security/certverifier/CertVerifier.cpp b/security/certverifier/CertVerifier.cpp index bc9b9627fbfa..c7cee4b1c5ef 100644 --- a/security/certverifier/CertVerifier.cpp +++ b/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) { diff --git a/security/certverifier/CertVerifier.h b/security/certverifier/CertVerifier.h index ddf42108ace3..6748a6ab887e 100644 --- a/security/certverifier/CertVerifier.h +++ b/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 {