bug 1278041 - skip TLS Feature checks so HPKP can be set r?mgoodwin draft
authorDavid Keeler <dkeeler@mozilla.com>
Mon, 20 Jun 2016 16:36:36 -0700
changeset 380167 b0bdce5701c1c23a3fc3fa74cd98b62f554c6ac6
parent 379985 3c5025f98e561a20e24d97c91a9e4e0ec28015ea
child 523664 4e684aefb63f25ae33c505f58d87b69facd9df7b
push id21157
push userdkeeler@mozilla.com
push dateMon, 20 Jun 2016 23:51:36 +0000
reviewersmgoodwin
bugs1278041
milestone50.0a1
bug 1278041 - skip TLS Feature checks so HPKP can be set r?mgoodwin This is safe because TLS Feature checks have already been done when connecting to the site in the first place. MozReview-Commit-ID: HfbcrAv4bCJ
security/manager/ssl/nsSiteSecurityService.cpp
security/manager/ssl/tests/unit/test_ocsp_must_staple.js
--- a/security/manager/ssl/nsSiteSecurityService.cpp
+++ b/security/manager/ssl/nsSiteSecurityService.cpp
@@ -697,22 +697,30 @@ nsSiteSecurityService::ProcessPKPHeader(
   NS_ENSURE_TRUE(cert, NS_ERROR_FAILURE);
   UniqueCERTCertificate nssCert(cert->GetCert());
   NS_ENSURE_TRUE(nssCert, NS_ERROR_FAILURE);
 
   mozilla::pkix::Time now(mozilla::pkix::Now());
   UniqueCERTCertList certList;
   RefPtr<SharedCertVerifier> certVerifier(GetDefaultCertVerifier());
   NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED);
+  // We don't want this verification to cause any network traffic that would
+  // block execution. Also, since we don't have access to the original stapled
+  // OCSP response, we can't enforce this aspect of the TLS Feature extension.
+  // This is ok, because it will have been enforced when we originally connected
+  // to the site (or it's disabled, in which case we wouldn't want to enforce it
+  // anyway).
+  CertVerifier::Flags flags = CertVerifier::FLAG_LOCAL_ONLY |
+                              CertVerifier::FLAG_TLS_IGNORE_STATUS_REQUEST;
   if (certVerifier->VerifySSLServerCert(nssCert, nullptr, // stapled ocsp
                                         now, nullptr, // pinarg
                                         host.get(), // hostname
                                         certList,
                                         false, // don't store intermediates
-                                        CertVerifier::FLAG_LOCAL_ONLY)
+                                        flags)
       != SECSuccess) {
     return NS_ERROR_FAILURE;
   }
 
   CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
   if (CERT_LIST_END(rootNode, certList)) {
     return NS_ERROR_FAILURE;
   }
--- a/security/manager/ssl/tests/unit/test_ocsp_must_staple.js
+++ b/security/manager/ssl/tests/unit/test_ocsp_must_staple.js
@@ -22,16 +22,39 @@ function add_ocsp_test(aHost, aExpectedR
 }
 
 function add_tests() {
   // ensure that the chain is checked for required features in children:
   // First a case where intermediate and ee both have the extension
   add_ocsp_test("ocsp-stapling-must-staple-ee-with-must-staple-int.example.com",
                 PRErrorCodeSuccess, true);
 
+  add_test(() => {
+    Services.prefs.setIntPref("security.cert_pinning.enforcement_level", 1);
+    Services.prefs.setBoolPref("security.cert_pinning.process_headers_from_non_builtin_roots", true);
+    let uri = Services.io.newURI("https://ocsp-stapling-must-staple-ee-with-must-staple-int.example.com",
+                                 null, null);
+    let keyHash = "VCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8=";
+    let backupKeyHash = "KHAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAN=";
+    let header = `max-age=1000; pin-sha256="${keyHash}"; pin-sha256="${backupKeyHash}"`;
+    let ssservice = Cc["@mozilla.org/ssservice;1"]
+                      .getService(Ci.nsISiteSecurityService);
+    let sslStatus = new FakeSSLStatus();
+    sslStatus.serverCert = constructCertFromFile("ocsp_certs/must-staple-ee-with-must-staple-int.pem");
+    ssservice.processHeader(Ci.nsISiteSecurityService.HEADER_HPKP, uri, header, sslStatus, 0);
+    ok(ssservice.isSecureURI(Ci.nsISiteSecurityService.HEADER_HPKP, uri, 0),
+       "ocsp-stapling-must-staple-ee-with-must-staple-int.example.com should have HPKP set");
+
+    // Clear accumulated state.
+    ssservice.removeState(Ci.nsISiteSecurityService.HEADER_HPKP, uri, 0);
+    Services.prefs.clearUserPref("security.cert_pinning.process_headers_from_non_builtin_roots");
+    Services.prefs.clearUserPref("security.cert_pinning.enforcement_level");
+    run_next_test();
+  });
+
   // Next, a case where it's present in the intermediate, not the ee
   add_ocsp_test("ocsp-stapling-plain-ee-with-must-staple-int.example.com",
                 MOZILLA_PKIX_ERROR_REQUIRED_TLS_FEATURE_MISSING, true);
 
   // We disable OCSP stapling in the next two tests so we can perform checks
   // on TLS Features in the chain without needing to support the TLS
   // extension values used.
   // Test an issuer with multiple TLS features in matched in the EE