bug 1345612 - avoid calling NS_NewURI on IP addresses when checking certificate overrides r?Cykesiopka draft
authorDavid Keeler <dkeeler@mozilla.com>
Mon, 20 Mar 2017 13:42:27 -0700
changeset 513620 d727aad38d45391285e8426d8590b530f8459040
parent 504632 4c987b7ed54a630a7de76adcc2eb00dab49d5dfd
child 550720 1b9f651df9ebea5fafbc0384637ff36969e888ce
push id50887
push userbmo:dkeeler@mozilla.com
push dateFri, 24 Mar 2017 17:57:18 +0000
reviewersCykesiopka
bugs1345612
milestone55.0a1
bug 1345612 - avoid calling NS_NewURI on IP addresses when checking certificate overrides r?Cykesiopka When determining if a certificate error override is allowed for a host, we consult nsISiteSecurityService::IsSecureURI to see if the host is HSTS/HPKP. This API takes an nsIURI, but the calling code only has a hostname as an nsCString. Calling NS_NewURI works in all situations we will encounter except when the hostname is an IPv6 address. Since IP addresses are never HSTS/HPKP anyway, we can skip the NS_NewURI / IsSecureURI calls in those cases as a workaround. MozReview-Commit-ID: JXa8cGvqqTA
security/manager/ssl/SSLServerCertVerification.cpp
security/manager/ssl/tests/unit/test_cert_overrides.js
security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
--- a/security/manager/ssl/SSLServerCertVerification.cpp
+++ b/security/manager/ssl/SSLServerCertVerification.cpp
@@ -280,16 +280,17 @@ class CertErrorRunnable : public SyncRun
       mProviderFlags(providerFlags)
   {
   }
 
   virtual void RunOnTargetThread();
   RefPtr<SSLServerCertVerificationResult> mResult; // out
 private:
   SSLServerCertVerificationResult* CheckCertOverrides();
+  nsresult OverrideAllowedForHost(/*out*/ bool& overrideAllowed);
 
   const void* const mFdForLogging; // may become an invalid pointer; do not dereference
   const nsCOMPtr<nsIX509Cert> mCert;
   const RefPtr<nsNSSSocketInfo> mInfoObject;
   const PRErrorCode mDefaultErrorCodeToReport;
   const uint32_t mCollectedErrors;
   const PRErrorCode mErrorCodeTrust;
   const PRErrorCode mErrorCodeMismatch;
@@ -468,16 +469,83 @@ DetermineCertOverrideErrors(const Unique
       PR_SetError(MapResultToPRErrorCode(result), 0);
       return SECFailure;
     }
   }
 
   return SECSuccess;
 }
 
+// Helper function to determine if overrides are allowed for this host.
+// Overrides are not allowed for known HSTS or HPKP hosts. However, an IP
+// address is never considered an HSTS or HPKP host.
+nsresult
+CertErrorRunnable::OverrideAllowedForHost(/*out*/ bool& overrideAllowed)
+{
+  overrideAllowed = false;
+
+  // If this is an IP address, overrides are allowed, because an IP address is
+  // never an HSTS or HPKP host. nsISiteSecurityService takes this into account
+  // already, but the real problem here is that calling NS_NewURI with an IPv6
+  // address fails. We do this to avoid that. A more comprehensive fix would be
+  // to have Necko provide an nsIURI to PSM and to use that here (and
+  // everywhere). However, that would be a wide-spanning change.
+  const nsACString& hostname = mInfoObject->GetHostName();
+  if (net_IsValidIPv6Addr(hostname.BeginReading(), hostname.Length())) {
+    overrideAllowed = true;
+    return NS_OK;
+  }
+
+  // If this is an HTTP Strict Transport Security host or a pinned host and the
+  // certificate is bad, don't allow overrides (RFC 6797 section 12.1,
+  // HPKP draft spec section 2.6).
+  bool strictTransportSecurityEnabled = false;
+  bool hasPinningInformation = false;
+  nsCOMPtr<nsISiteSecurityService> sss(do_GetService(NS_SSSERVICE_CONTRACTID));
+  if (!sss) {
+    MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
+            ("[%p][%p] couldn't get nsISiteSecurityService to check HSTS/HPKP",
+            mFdForLogging, this));
+    return NS_ERROR_FAILURE;
+  }
+  nsCOMPtr<nsIURI> uri;
+  nsresult rv = NS_NewURI(getter_AddRefs(uri),
+                          NS_LITERAL_CSTRING("https://") + hostname);
+  if (NS_FAILED(rv)) {
+    MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
+            ("[%p][%p] Creating new URI failed", mFdForLogging, this));
+    return rv;
+  }
+  rv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS,
+                        uri,
+                        mProviderFlags,
+                        mInfoObject->GetOriginAttributes(),
+                        nullptr,
+                        &strictTransportSecurityEnabled);
+  if (NS_FAILED(rv)) {
+    MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
+            ("[%p][%p] checking for HSTS failed", mFdForLogging, this));
+    return rv;
+  }
+  rv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HPKP,
+                        uri,
+                        mProviderFlags,
+                        mInfoObject->GetOriginAttributes(),
+                        nullptr,
+                        &hasPinningInformation);
+  if (NS_FAILED(rv)) {
+    MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
+            ("[%p][%p] checking for HPKP failed", mFdForLogging, this));
+    return rv;
+  }
+
+  overrideAllowed = !strictTransportSecurityEnabled && !hasPinningInformation;
+  return NS_OK;
+}
+
 SSLServerCertVerificationResult*
 CertErrorRunnable::CheckCertOverrides()
 {
   MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("[%p][%p] top of CheckCertOverrides\n",
                                     mFdForLogging, this));
   // "Use" mFdForLogging in non-PR_LOGGING builds, too, to suppress
   // clang's -Wunused-private-field build warning for this variable:
   Unused << mFdForLogging;
@@ -492,87 +560,42 @@ CertErrorRunnable::CheckCertOverrides()
   mInfoObject->GetPort(&port);
 
   nsAutoCString hostWithPortString(mInfoObject->GetHostName());
   hostWithPortString.Append(':');
   hostWithPortString.AppendInt(port);
 
   uint32_t remaining_display_errors = mCollectedErrors;
 
-
-  // If this is an HTTP Strict Transport Security host or a pinned host and the
-  // certificate is bad, don't allow overrides (RFC 6797 section 12.1,
-  // HPKP draft spec section 2.6).
-  bool strictTransportSecurityEnabled = false;
-  bool hasPinningInformation = false;
-  nsCOMPtr<nsISiteSecurityService> sss(do_GetService(NS_SSSERVICE_CONTRACTID));
-  if (!sss) {
-    MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
-           ("[%p][%p] couldn't get nsISiteSecurityService to check for HSTS/HPKP\n",
-            mFdForLogging, this));
-    return new SSLServerCertVerificationResult(mInfoObject,
-                                               mDefaultErrorCodeToReport);
-  }
-  nsCOMPtr<nsIURI> uri;
-  nsresult nsrv = NS_NewURI(getter_AddRefs(uri),
-                            NS_LITERAL_CSTRING("https://") +
-                            mInfoObject->GetHostName());
-  if (NS_FAILED(nsrv)) {
-    MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
-            ("[%p][%p] Creating new URI failed\n", mFdForLogging, this));
-    return new SSLServerCertVerificationResult(mInfoObject,
-                                               mDefaultErrorCodeToReport);
-  }
-  nsrv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS,
-                          uri,
-                          mProviderFlags,
-                          mInfoObject->GetOriginAttributes(),
-                          nullptr,
-                          &strictTransportSecurityEnabled);
-  if (NS_FAILED(nsrv)) {
-    MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
-           ("[%p][%p] checking for HSTS failed\n", mFdForLogging, this));
-    return new SSLServerCertVerificationResult(mInfoObject,
-                                               mDefaultErrorCodeToReport);
-  }
-  nsrv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HPKP,
-                          uri,
-                          mProviderFlags,
-                          mInfoObject->GetOriginAttributes(),
-                          nullptr,
-                          &hasPinningInformation);
-  if (NS_FAILED(nsrv)) {
-    MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
-           ("[%p][%p] checking for HPKP failed\n", mFdForLogging, this));
+  bool overrideAllowed;
+  if (NS_FAILED(OverrideAllowedForHost(overrideAllowed))) {
     return new SSLServerCertVerificationResult(mInfoObject,
                                                mDefaultErrorCodeToReport);
   }
 
-  if (!strictTransportSecurityEnabled && !hasPinningInformation) {
+  if (overrideAllowed) {
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
            ("[%p][%p] no HSTS or HPKP - overrides allowed\n",
             mFdForLogging, this));
     nsCOMPtr<nsICertOverrideService> overrideService =
       do_GetService(NS_CERTOVERRIDE_CONTRACTID);
     // it is fine to continue without the nsICertOverrideService
 
     uint32_t overrideBits = 0;
 
-    if (overrideService)
-    {
+    if (overrideService) {
       bool haveOverride;
       bool isTemporaryOverride; // we don't care
       const nsACString& hostString(mInfoObject->GetHostName());
-      nsrv = overrideService->HasMatchingOverride(hostString, port,
-                                                  mCert,
-                                                  &overrideBits,
-                                                  &isTemporaryOverride,
-                                                  &haveOverride);
-      if (NS_SUCCEEDED(nsrv) && haveOverride)
-      {
+      nsresult rv = overrideService->HasMatchingOverride(hostString, port,
+                                                         mCert,
+                                                         &overrideBits,
+                                                         &isTemporaryOverride,
+                                                         &haveOverride);
+      if (NS_SUCCEEDED(rv) && haveOverride) {
        // remove the errors that are already overriden
         remaining_display_errors &= ~overrideBits;
       }
     }
 
     if (!remaining_display_errors) {
       // This can double- or triple-count one certificate with multiple
       // different types of errors. Since this is telemetry and we just
@@ -616,18 +639,18 @@ CertErrorRunnable::CheckCertOverrides()
     nsCOMPtr<nsIInterfaceRequestor> cb;
     sslSocketControl->GetNotificationCallbacks(getter_AddRefs(cb));
     if (cb) {
       nsCOMPtr<nsIBadCertListener2> bcl = do_GetInterface(cb);
       if (bcl) {
         nsIInterfaceRequestor* csi
           = static_cast<nsIInterfaceRequestor*>(mInfoObject);
         bool suppressMessage = false; // obsolete, ignored
-        nsrv = bcl->NotifyCertProblem(csi, mInfoObject->SSLStatus(),
-                                      hostWithPortString, &suppressMessage);
+        Unused << bcl->NotifyCertProblem(csi, mInfoObject->SSLStatus(),
+                                         hostWithPortString, &suppressMessage);
       }
     }
   }
 
   // pick the error code to report by priority
   PRErrorCode errorCodeToReport = mErrorCodeTrust    ? mErrorCodeTrust
                                 : mErrorCodeMismatch ? mErrorCodeMismatch
                                 : mErrorCodeTime     ? mErrorCodeTime
--- a/security/manager/ssl/tests/unit/test_cert_overrides.js
+++ b/security/manager/ssl/tests/unit/test_cert_overrides.js
@@ -14,31 +14,31 @@
 do_get_profile();
 
 function check_telemetry() {
   let histogram = Cc["@mozilla.org/base/telemetry;1"]
                     .getService(Ci.nsITelemetry)
                     .getHistogramById("SSL_CERT_ERROR_OVERRIDES")
                     .snapshot();
   equal(histogram.counts[0], 0, "Should have 0 unclassified counts");
-  equal(histogram.counts[2], 8,
+  equal(histogram.counts[2], 9,
         "Actual and expected SEC_ERROR_UNKNOWN_ISSUER counts should match");
   equal(histogram.counts[3], 1,
         "Actual and expected SEC_ERROR_CA_CERT_INVALID counts should match");
   equal(histogram.counts[4], 0,
         "Actual and expected SEC_ERROR_UNTRUSTED_ISSUER counts should match");
   equal(histogram.counts[5], 1,
         "Actual and expected SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE counts should match");
   equal(histogram.counts[6], 0,
         "Actual and expected SEC_ERROR_UNTRUSTED_CERT counts should match");
   equal(histogram.counts[7], 0,
         "Actual and expected SEC_ERROR_INADEQUATE_KEY_USAGE counts should match");
   equal(histogram.counts[8], 2,
         "Actual and expected SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED counts should match");
-  equal(histogram.counts[9], 10,
+  equal(histogram.counts[9], 13,
         "Actual and expected SSL_ERROR_BAD_CERT_DOMAIN counts should match");
   equal(histogram.counts[10], 5,
         "Actual and expected SEC_ERROR_EXPIRED_CERTIFICATE counts should match");
   equal(histogram.counts[11], 2,
         "Actual and expected MOZILLA_PKIX_ERROR_CA_CERT_USED_AS_END_ENTITY counts should match");
   equal(histogram.counts[12], 1,
         "Actual and expected MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA counts should match");
   equal(histogram.counts[13], 1,
@@ -53,21 +53,21 @@ function check_telemetry() {
         "Actual and expected MOZILLA_PKIX_ERROR_EMPTY_ISSUER_NAME counts should match");
 
   let keySizeHistogram = Cc["@mozilla.org/base/telemetry;1"]
                            .getService(Ci.nsITelemetry)
                            .getHistogramById("CERT_CHAIN_KEY_SIZE_STATUS")
                            .snapshot();
   equal(keySizeHistogram.counts[0], 0,
         "Actual and expected unchecked key size counts should match");
-  equal(keySizeHistogram.counts[1], 12,
+  equal(keySizeHistogram.counts[1], 16,
         "Actual and expected successful verifications of 2048-bit keys should match");
   equal(keySizeHistogram.counts[2], 0,
         "Actual and expected successful verifications of 1024-bit keys should match");
-  equal(keySizeHistogram.counts[3], 58,
+  equal(keySizeHistogram.counts[3], 60,
         "Actual and expected verification failures unrelated to key size should match");
 
   run_next_test();
 }
 
 // Internally, specifying "port" -1 is the same as port 443. This tests that.
 function run_port_equivalency_test(inPort, outPort) {
   Assert.ok((inPort == 443 && outPort == -1) || (inPort == -1 && outPort == 443),
@@ -108,16 +108,17 @@ function run_test() {
 
   let fakeOCSPResponder = new HttpServer();
   fakeOCSPResponder.registerPrefixHandler("/", function (request, response) {
     response.setStatusLine(request.httpVersion, 500, "Internal Server Error");
   });
   fakeOCSPResponder.start(8888);
 
   add_simple_tests();
+  add_localhost_tests();
   add_combo_tests();
   add_distrust_tests();
 
   add_test(function () {
     fakeOCSPResponder.stop(check_telemetry);
   });
 
   run_next_test();
@@ -266,16 +267,29 @@ function add_simple_tests() {
     Assert.ok(certOverrideService.hasMatchingOverride(uri.asciiHost, 8443, cert, {}, {}),
               "IDN certificate should have matching override using ascii host");
     Assert.ok(!certOverrideService.hasMatchingOverride(uri.host, 8443, cert, {}, {}),
               "IDN certificate should not have matching override using (non-ascii) host");
     run_next_test();
   });
 }
 
+function add_localhost_tests() {
+  add_cert_override_test("localhost",
+                         Ci.nsICertOverrideService.ERROR_MISMATCH |
+                         Ci.nsICertOverrideService.ERROR_UNTRUSTED,
+                         SEC_ERROR_UNKNOWN_ISSUER);
+  add_cert_override_test("127.0.0.1",
+                         Ci.nsICertOverrideService.ERROR_MISMATCH,
+                         SSL_ERROR_BAD_CERT_DOMAIN);
+  add_cert_override_test("::1",
+                         Ci.nsICertOverrideService.ERROR_MISMATCH,
+                         SSL_ERROR_BAD_CERT_DOMAIN);
+}
+
 function add_combo_tests() {
   add_cert_override_test("mismatch-expired.example.com",
                          Ci.nsICertOverrideService.ERROR_MISMATCH |
                          Ci.nsICertOverrideService.ERROR_TIME,
                          SSL_ERROR_BAD_CERT_DOMAIN,
                          /The certificate is only valid for <a id="cert_domain_link" title="doesntmatch\.example\.com">doesntmatch\.example\.com<\/a>/);
   add_cert_override_test("mismatch-notYetValid.example.com",
                          Ci.nsICertOverrideService.ERROR_MISMATCH |
--- a/security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
+++ b/security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
@@ -72,16 +72,17 @@ const BadCertHost sBadCertHosts[] =
   { "end-entity-issued-by-non-CA.example.com", "eeIssuedByNonCA" },
   { "inadequate-key-size-ee.example.com", "inadequateKeySizeEE" },
   { "badSubjectAltNames.example.com", "badSubjectAltNames" },
   { "ipAddressAsDNSNameInSAN.example.com", "ipAddressAsDNSNameInSAN" },
   { "noValidNames.example.com", "noValidNames" },
   { "bug413909.xn--hxajbheg2az3al.xn--jxalpdlp", "idn-certificate" },
   { "emptyissuername.example.com", "emptyIssuerName" },
   { "ev-test.example.com", "ev-test" },
+  { "localhost", "unknownissuer" },
   { nullptr, nullptr }
 };
 
 int32_t
 DoSNISocketConfigBySubjectCN(PRFileDesc* aFd, const SECItem* aSrvNameArr,
                              uint32_t aSrvNameArrSize)
 {
   for (uint32_t i = 0; i < aSrvNameArrSize; i++) {