Bug 1066190 - Ensure that pinning checks are done for otherwise overridable errors. r=mmc, a=sledru
authorDavid Keeler <dkeeler@mozilla.com>
Fri, 12 Sep 2014 13:20:43 -0700
changeset 216782 1e3320340bd2
parent 216781 8a1cffa4c130
child 216783 792d0824a8f0
push id3910
push userryanvm@gmail.com
push date2014-09-18 14:48 +0000
treeherdermozilla-beta@af1dbe183e3d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmmc, sledru
bugs1066190
milestone33.0
Bug 1066190 - Ensure that pinning checks are done for otherwise overridable errors. r=mmc, a=sledru
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
@@ -394,16 +394,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,