bug 1254667 - change certificate verification SHA1 policy to "allow for locally-installed roots" r=jcj
authorDavid Keeler <dkeeler@mozilla.com>
Mon, 28 Mar 2016 12:52:40 -0700
changeset 291143 8772f2293eaba94a6890eadb525711e5d11ebf63
parent 291142 a31c3e92f148e297732eceabc01fe5e5df4bacbc
child 291144 21a8c1db64fa61200e5e17f6140589407478c911
push id19656
push usergwagner@mozilla.com
push dateMon, 04 Apr 2016 13:43:23 +0000
treeherderb2g-inbound@e99061fde28a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjcj
bugs1254667
milestone48.0a1
bug 1254667 - change certificate verification SHA1 policy to "allow for locally-installed roots" r=jcj Before this patch, the default policy for the use of SHA1 in certificate signatures was "allow all" due to compatibility concerns. After gathering telemetry, we are confident that we can enforce the policy of "allow for locally-installed roots" (or certificates valid before 2016) without too much breakage. MozReview-Commit-ID: 8GxtgdbaS3P
browser/app/profile/firefox.js
mobile/android/app/mobile.js
netwerk/base/security-prefs.js
security/certverifier/CertVerifier.cpp
security/manager/ssl/tests/unit/test_ocsp_caching.js
security/manager/ssl/tests/unit/test_ocsp_stapling_expired.js
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -1438,21 +1438,16 @@ pref("security.mixed_content.block_activ
 pref("security.insecure_password.ui.enabled", true);
 #else
 pref("security.insecure_password.ui.enabled", false);
 #endif
 
 // 1 = allow MITM for certificate pinning checks.
 pref("security.cert_pinning.enforcement_level", 1);
 
-// NB: Changes to this pref affect CERT_CHAIN_SHA1_POLICY_STATUS telemetry.
-// See the comment in CertVerifier.cpp.
-// 0 = allow SHA-1
-pref("security.pki.sha1_enforcement_level", 0);
-
 // Required blocklist freshness for OneCRL OCSP bypass
 // (default is 1.25x extensions.blocklist.interval, or 30 hours)
 pref("security.onecrl.maximum_staleness_in_seconds", 108000);
 
 // Override the Gecko-default value of false for Firefox.
 pref("plain_text.wrap_long_lines", true);
 
 // If this turns true, Moz*Gesture events are not called stopPropagation()
--- a/mobile/android/app/mobile.js
+++ b/mobile/android/app/mobile.js
@@ -513,21 +513,16 @@ pref("security.alternate_certificate_err
 pref("security.warn_viewing_mixed", false); // Warning is disabled.  See Bug 616712.
 
 // Block insecure active content on https pages
 pref("security.mixed_content.block_active_content", true);
 
 // Enable pinning
 pref("security.cert_pinning.enforcement_level", 1);
 
-// NB: Changes to this pref affect CERT_CHAIN_SHA1_POLICY_STATUS telemetry.
-// See the comment in CertVerifier.cpp.
-// Allow SHA-1 certificates
-pref("security.pki.sha1_enforcement_level", 0);
-
 // Required blocklist freshness for OneCRL OCSP bypass
 // (default is 1.25x extensions.blocklist.interval, or 30 hours)
 pref("security.onecrl.maximum_staleness_in_seconds", 108000);
 
 // Only fetch OCSP for EV certificates
 pref("security.OCSP.enabled", 2);
 
 // Override some named colors to avoid inverse OS themes
--- a/netwerk/base/security-prefs.js
+++ b/netwerk/base/security-prefs.js
@@ -39,16 +39,20 @@ pref("security.remember_cert_checkbox_de
 pref("security.ask_for_password",        0);
 pref("security.password_lifetime",       30);
 
 pref("security.OCSP.enabled", 1);
 pref("security.OCSP.require", false);
 pref("security.OCSP.GET.enabled", false);
 
 pref("security.pki.cert_short_lifetime_in_days", 10);
+// NB: Changes to this pref affect CERT_CHAIN_SHA1_POLICY_STATUS telemetry.
+// See the comment in CertVerifier.cpp.
+// 3 = allow SHA-1 for certificates issued before 2016 or by an imported root.
+pref("security.pki.sha1_enforcement_level", 3);
 
 pref("security.webauth.u2f", false);
 pref("security.webauth.u2f.softtoken", false);
 pref("security.webauth.u2f.usbtoken", false);
 
 pref("security.ssl.errorReporting.enabled", true);
 pref("security.ssl.errorReporting.url", "https://data.mozilla.com/submit/sslreports");
 pref("security.ssl.errorReporting.automatic", false);
--- a/security/certverifier/CertVerifier.cpp
+++ b/security/certverifier/CertVerifier.cpp
@@ -318,30 +318,32 @@ CertVerifier::VerifyCert(CERTCertificate
                                     : NSSCertDBTrustDomain::FetchOCSPForEV;
 
       CertPolicyId evPolicy;
       SECOidTag evPolicyOidTag;
       SECStatus srv = GetFirstEVPolicy(cert, evPolicy, evPolicyOidTag);
       for (size_t i = 0;
            i < sha1ModeConfigurationsCount && rv != Success && srv == SECSuccess;
            i++) {
-        // Because of the try-strict and fallback approach, we have to clear any
-        // previously noted telemetry information
-        if (pinningTelemetryInfo) {
-          pinningTelemetryInfo->Reset();
-        }
         // Don't attempt verification if the SHA1 mode set by preferences
         // (mSHA1Mode) is more restrictive than the SHA1 mode option we're on.
         // (To put it another way, only attempt verification if the SHA1 mode
         // option we're on is as restrictive or more restrictive than
         // mSHA1Mode.) This allows us to gather telemetry information while
         // still enforcing the mode set by preferences.
         if (SHA1ModeMoreRestrictiveThanGivenMode(sha1ModeConfigurations[i])) {
           continue;
         }
+
+        // Because of the try-strict and fallback approach, we have to clear any
+        // previously noted telemetry information
+        if (pinningTelemetryInfo) {
+          pinningTelemetryInfo->Reset();
+        }
+
         NSSCertDBTrustDomain
           trustDomain(trustSSL, evOCSPFetching,
                       mOCSPCache, pinArg, ocspGETConfig,
                       mCertShortLifetimeInDays, mPinningMode, MIN_RSA_BITS,
                       ValidityCheckingMode::CheckForEV,
                       sha1ModeConfigurations[i], builtChain,
                       pinningTelemetryInfo, hostname);
         rv = BuildCertChainForOneKeyUsage(trustDomain, certDER, time,
@@ -407,31 +409,31 @@ CertVerifier::VerifyCert(CERTCertificate
                     MOZ_ARRAY_LENGTH(keySizeStatuses),
                     "keySize array lengths differ");
 
       size_t keySizeOptionsCount = MOZ_ARRAY_LENGTH(keySizeStatuses);
 
       for (size_t i = 0; i < keySizeOptionsCount && rv != Success; i++) {
         for (size_t j = 0; j < sha1ModeConfigurationsCount && rv != Success;
              j++) {
-          // invalidate any telemetry info relating to failed chains
-          if (pinningTelemetryInfo) {
-            pinningTelemetryInfo->Reset();
-          }
-
           // Don't attempt verification if the SHA1 mode set by preferences
           // (mSHA1Mode) is more restrictive than the SHA1 mode option we're on.
           // (To put it another way, only attempt verification if the SHA1 mode
           // option we're on is as restrictive or more restrictive than
           // mSHA1Mode.) This allows us to gather telemetry information while
           // still enforcing the mode set by preferences.
           if (SHA1ModeMoreRestrictiveThanGivenMode(sha1ModeConfigurations[j])) {
             continue;
           }
 
+          // invalidate any telemetry info relating to failed chains
+          if (pinningTelemetryInfo) {
+            pinningTelemetryInfo->Reset();
+          }
+
           NSSCertDBTrustDomain trustDomain(trustSSL, defaultOCSPFetching,
                                            mOCSPCache, pinArg, ocspGETConfig,
                                            mCertShortLifetimeInDays,
                                            mPinningMode, keySizeOptions[i],
                                            ValidityCheckingMode::CheckingOff,
                                            sha1ModeConfigurations[j],
                                            builtChain, pinningTelemetryInfo,
                                            hostname);
@@ -479,17 +481,17 @@ CertVerifier::VerifyCert(CERTCertificate
       }
 
       if (keySizeStatus) {
         *keySizeStatus = KeySizeStatus::AlreadyBad;
       }
       // Only collect CERT_CHAIN_SHA1_POLICY_STATUS telemetry indicating a
       // failure when mSHA1Mode is the default.
       // NB: When we change the default, we have to change this.
-      if (sha1ModeResult && mSHA1Mode == SHA1Mode::Allowed) {
+      if (sha1ModeResult && mSHA1Mode == SHA1Mode::ImportedRoot) {
         *sha1ModeResult = SHA1ModeResult::Failed;
       }
 
       break;
     }
 
     case certificateUsageSSLCA: {
       NSSCertDBTrustDomain trustDomain(trustSSL, defaultOCSPFetching,
--- a/security/manager/ssl/tests/unit/test_ocsp_caching.js
+++ b/security/manager/ssl/tests/unit/test_ocsp_caching.js
@@ -57,16 +57,17 @@ function add_ocsp_test(aHost, aExpectedR
               " OCSP request" + (aResponses.length == 1 ? "" : "s"));
       });
 }
 
 function run_test() {
   do_get_profile();
   Services.prefs.setBoolPref("security.ssl.enable_ocsp_stapling", true);
   Services.prefs.setIntPref("security.OCSP.enabled", 1);
+  Services.prefs.setIntPref("security.pki.sha1_enforcement_level", 3);
   add_tls_server_setup("OCSPStaplingServer", "ocsp_certs");
 
   let ocspResponder = new HttpServer();
   ocspResponder.registerPrefixHandler("/", function(request, response) {
 
     do_print("gFetchCount: " + gFetchCount);
     let responseFunction = gResponsePattern[gFetchCount];
     Assert.notEqual(undefined, responseFunction);
@@ -131,18 +132,16 @@ function add_tests() {
   add_ocsp_test("ocsp-stapling-none.example.com", SEC_ERROR_OCSP_UNKNOWN_CERT,
                 [
                   respondWithError,
                   respondWithError,
                   respondWithError,
                   respondWithError,
                   respondWithError,
                   respondWithError,
-                  respondWithError,
-                  respondWithError,
                 ],
                 "No stapled response -> a fetch should have been attempted");
 
   // A valid Good response from the OCSP responder must override the cached
   // Unknown response.
   //
   // Note that We need to make sure that the Unknown response and the Good
   // response have different thisUpdate timestamps; otherwise, the Good
--- a/security/manager/ssl/tests/unit/test_ocsp_stapling_expired.js
+++ b/security/manager/ssl/tests/unit/test_ocsp_stapling_expired.js
@@ -26,16 +26,17 @@ function add_ocsp_test(aHost, aExpectedR
             "Should have made " + aExpectedRequestCount +
             " fallback OCSP request" + (aExpectedRequestCount == 1 ? "" : "s"));
     });
 }
 
 do_get_profile();
 Services.prefs.setBoolPref("security.ssl.enable_ocsp_stapling", true);
 Services.prefs.setIntPref("security.OCSP.enabled", 1);
+Services.prefs.setIntPref("security.pki.sha1_enforcement_level", 3);
 var args = [["good", "default-ee", "unused"],
              ["expiredresponse", "default-ee", "unused"],
              ["oldvalidperiod", "default-ee", "unused"],
              ["revoked", "default-ee", "unused"],
              ["unknown", "default-ee", "unused"],
             ];
 var ocspResponses = generateOCSPResponses(args, "ocsp_certs");
 // Fresh response, certificate is good.
@@ -48,19 +49,19 @@ var oldValidityPeriodOCSPResponseGood = 
 var ocspResponseRevoked = ocspResponses[3];
 // Fresh signature, certificate is unknown.
 var ocspResponseUnknown = ocspResponses[4];
 
 // sometimes we expect a result without re-fetch
 var willNotRetry = 1;
 // but sometimes, since a bad response is in the cache, OCSP fetch will be
 // attempted for each validation - in practice, for these test certs, this
-// means 8 requests because various hash algorithm and key size combinations
+// means 6 requests because various hash algorithm and key size combinations
 // are tried.
-var willRetry = 8;
+var willRetry = 6;
 
 function run_test() {
   let ocspResponder = new HttpServer();
   ocspResponder.registerPrefixHandler("/", function(request, response) {
     if (gCurrentOCSPResponse) {
       response.setStatusLine(request.httpVersion, 200, "OK");
       response.setHeader("Content-Type", "application/ocsp-response");
       response.write(gCurrentOCSPResponse);