bug 1066190 - ensure that pinning checks are done for otherwise overridable errors r=mmc
authorDavid Keeler <dkeeler@mozilla.com>
Fri, 12 Sep 2014 13:20:43 -0700 (2014-09-12)
changeset 205659 d02e70f0bf3d2f6028541d2a7c455266bc9597cd
parent 205658 9a997d74355334a4bb81dac36610eae395769502
child 205660 3488fc0bf6ef5419d084fac05cb1aae4ec07aff3
push id27498
push userkwierso@gmail.com
push dateWed, 17 Sep 2014 00:06:56 +0000 (2014-09-17)
treeherdermozilla-central@8252eae8278c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmmc
bugs1066190
milestone35.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 1066190 - ensure that pinning checks are done for otherwise overridable errors r=mmc
security/certverifier/CertVerifier.cpp
security/manager/ssl/tests/unit/test_pinning.js
security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
--- a/security/certverifier/CertVerifier.cpp
+++ b/security/certverifier/CertVerifier.cpp
@@ -413,16 +413,39 @@ CertVerifier::VerifyCert(CERTCertificate
       break;
     }
 
     default:
       rv = Result::FATAL_ERROR_INVALID_ARGS;
   }
 
   if (rv != Success) {
+    if (rv != Result::ERROR_KEY_PINNING_FAILURE) {
+      ScopedCERTCertificate certCopy(CERT_DupCertificate(cert));
+      if (!certCopy) {
+        return SECFailure;
+      }
+      ScopedCERTCertList certList(CERT_NewCertList());
+      if (!certList) {
+        return SECFailure;
+      }
+      SECStatus srv = CERT_AddCertToListTail(certList.get(), certCopy.get());
+      if (srv != SECSuccess) {
+        return SECFailure;
+      }
+      certCopy.forget(); // now owned by certList
+      PRBool chainOK = false;
+      srv = chainValidationCallback(&callbackState, certList, &chainOK);
+      if (srv != SECSuccess) {
+        return SECFailure;
+      }
+      if (!chainOK) {
+        rv = Result::ERROR_KEY_PINNING_FAILURE;
+      }
+    }
     PR_SetError(MapResultToPRErrorCode(rv), 0);
     return SECFailure;
   }
 
   return SECSuccess;
 }
 
 SECStatus
--- a/security/manager/ssl/tests/unit/test_pinning.js
+++ b/security/manager/ssl/tests/unit/test_pinning.js
@@ -32,16 +32,22 @@ function test_strict() {
   // In strict mode, we always evaluate pinning data, regardless of whether the
   // issuer is a built-in trust anchor. We only enforce pins that are not in
   // test mode.
   add_test(function() {
     Services.prefs.setIntPref("security.cert_pinning.enforcement_level", 2);
     run_next_test();
   });
 
+  // If a host should be pinned but other errors (particularly overridable
+  // errors) like 'unknown issuer' are encountered, the pinning error takes
+  // precedence. This prevents overrides for such hosts.
+  add_connection_test("unknownissuer.include-subdomains.pinning.example.com",
+    getXPCOMStatusFromNSS(MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE));
+
   // Issued by otherCA, which is not in the pinset for pinning.example.com.
   add_connection_test("bad.include-subdomains.pinning.example.com",
     getXPCOMStatusFromNSS(MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE));
 
   // These domains serve certs that match the pinset.
   add_connection_test("include-subdomains.pinning.example.com", Cr.NS_OK);
   add_connection_test("good.include-subdomains.pinning.example.com", Cr.NS_OK);
   add_connection_test("exclude-subdomains.pinning.example.com", Cr.NS_OK);
@@ -63,16 +69,19 @@ function test_mitm() {
   add_test(function() {
     Services.prefs.setIntPref("security.cert_pinning.enforcement_level", 1);
     run_next_test();
   });
 
   add_connection_test("include-subdomains.pinning.example.com", Cr.NS_OK);
   add_connection_test("good.include-subdomains.pinning.example.com", Cr.NS_OK);
 
+  add_connection_test("unknownissuer.include-subdomains.pinning.example.com",
+    getXPCOMStatusFromNSS(SEC_ERROR_UNKNOWN_ISSUER));
+
   // In this case, even though otherCA is not in the pinset, it is a
   // user-specified trust anchor and the pinning check succeeds.
   add_connection_test("bad.include-subdomains.pinning.example.com", Cr.NS_OK);
 
   add_connection_test("exclude-subdomains.pinning.example.com", Cr.NS_OK);
   add_connection_test("sub.exclude-subdomains.pinning.example.com", Cr.NS_OK);
   add_connection_test("test-mode.pinning.example.com", Cr.NS_OK);
 };
@@ -85,25 +94,31 @@ function test_disabled() {
   });
 
   add_connection_test("include-subdomains.pinning.example.com", Cr.NS_OK);
   add_connection_test("good.include-subdomains.pinning.example.com", Cr.NS_OK);
   add_connection_test("bad.include-subdomains.pinning.example.com", Cr.NS_OK);
   add_connection_test("exclude-subdomains.pinning.example.com", Cr.NS_OK);
   add_connection_test("sub.exclude-subdomains.pinning.example.com", Cr.NS_OK);
   add_connection_test("test-mode.pinning.example.com", Cr.NS_OK);
+
+  add_connection_test("unknownissuer.include-subdomains.pinning.example.com",
+    getXPCOMStatusFromNSS(SEC_ERROR_UNKNOWN_ISSUER));
 }
 
 function test_enforce_test_mode() {
   // In enforce test mode, we always enforce all pins, even test pins.
   add_test(function() {
     Services.prefs.setIntPref("security.cert_pinning.enforcement_level", 3);
     run_next_test();
   });
 
+  add_connection_test("unknownissuer.include-subdomains.pinning.example.com",
+    getXPCOMStatusFromNSS(MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE));
+
   // Issued by otherCA, which is not in the pinset for pinning.example.com.
   add_connection_test("bad.include-subdomains.pinning.example.com",
     getXPCOMStatusFromNSS(MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE));
 
   // These domains serve certs that match the pinset.
   add_connection_test("include-subdomains.pinning.example.com", Cr.NS_OK);
   add_connection_test("good.include-subdomains.pinning.example.com", Cr.NS_OK);
   add_connection_test("exclude-subdomains.pinning.example.com", Cr.NS_OK);
@@ -123,17 +138,17 @@ function test_enforce_test_mode() {
 function check_pinning_telemetry() {
   let service = Cc["@mozilla.org/base/telemetry;1"].getService(Ci.nsITelemetry);
   let prod_histogram = service.getHistogramById("CERT_PINNING_RESULTS")
                          .snapshot();
   let test_histogram = service.getHistogramById("CERT_PINNING_TEST_RESULTS")
                          .snapshot();
   // Because all of our test domains are pinned to user-specified trust
   // anchors, effectively only strict mode and enforce test-mode get evaluated
-  do_check_eq(prod_histogram.counts[0], 2); // Failure count
+  do_check_eq(prod_histogram.counts[0], 4); // Failure count
   do_check_eq(prod_histogram.counts[1], 4); // Success count
   do_check_eq(test_histogram.counts[0], 2); // Failure count
   do_check_eq(test_histogram.counts[1], 0); // Success count
 
   let moz_prod_histogram = service.getHistogramById("CERT_PINNING_MOZ_RESULTS")
                              .snapshot();
   let moz_test_histogram =
     service.getHistogramById("CERT_PINNING_MOZ_TEST_RESULTS").snapshot();
--- a/security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
+++ b/security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
@@ -50,16 +50,17 @@ const BadCertHost sBadCertHosts[] =
   // pass pinning when security.cert_pinning.enforcement.level != strict and
   // otherCA is added as a user-specified trust anchor. See StaticHPKPins.h.
   { "include-subdomains.pinning.example.com", "localhostAndExampleCom" },
   { "good.include-subdomains.pinning.example.com", "localhostAndExampleCom" },
   { "bad.include-subdomains.pinning.example.com", "otherIssuerEE" },
   { "exclude-subdomains.pinning.example.com", "localhostAndExampleCom" },
   { "sub.exclude-subdomains.pinning.example.com", "otherIssuerEE" },
   { "test-mode.pinning.example.com", "otherIssuerEE" },
+  { "unknownissuer.include-subdomains.pinning.example.com", "unknownissuer" },
   { "nsCertTypeNotCritical.example.com", "nsCertTypeNotCritical" },
   { "nsCertTypeCriticalWithExtKeyUsage.example.com", "nsCertTypeCriticalWithExtKeyUsage" },
   { "nsCertTypeCritical.example.com", "nsCertTypeCritical" },
   { nullptr, nullptr }
 };
 
 int32_t
 DoSNISocketConfig(PRFileDesc *aFd, const SECItem *aSrvNameArr,