bug 1066190 - ensure that pinning checks are done for otherwise overridable errors r=mmc

security/certverifier/CertVerifier.cpp

@@ -418,6 +418,29 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage,
   }
 
   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;
   }

security/manager/ssl/tests/unit/test_pinning.js

@@ -37,6 +37,12 @@ function test_strict() {
     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));
@@ -68,6 +74,9 @@ function test_mitm() {
   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);
@@ -90,6 +99,9 @@ function test_disabled() {
   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() {
@@ -99,6 +111,9 @@ function test_enforce_test_mode() {
     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));
@@ -128,7 +143,7 @@ function check_pinning_telemetry() {
                          .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

security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp

@@ -55,6 +55,7 @@ const BadCertHost sBadCertHosts[] =
   { "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" },