bug 1465562 - ensure succeededCertChain is set in TLS handshakes with session resumption r=fkiefer
☠☠ backed out by 3a3bbb2e68ef ☠ ☠
authorDavid Keeler <dkeeler@mozilla.com>
Thu, 07 Jun 2018 10:41:25 -0700
changeset 421970 2b65a8f996222369279cc2152ecaa7eaee93898a
parent 421969 7deab88709c76a757173f029e3a2479d6517f620
child 421971 674d58c3a8f3769ae863cf56092cb1a552eb27f0
push id34113
push userbtara@mozilla.com
push dateSat, 09 Jun 2018 12:05:46 +0000
treeherdermozilla-central@21bc698a4d9c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfkiefer
bugs1465562
milestone62.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 1465562 - ensure succeededCertChain is set in TLS handshakes with session resumption r=fkiefer When doing TLS session resumption, Firefox currently does not have enough information to trivially reconstitute the original connection's security information. Consequently, we have to rebuild the certificate chain in the handshake callback. Before this patch, we determined the EV and CT status of the connection but did not set the succeeded cert chain unless the certificate was EV. This was insufficient. In this patch, we set the succeeded cert chain regardless of if the certificate is EV or not (provided we found a valid chain). MozReview-Commit-ID: AuKrlBwX1Qh
security/manager/ssl/nsNSSCallbacks.cpp
security/manager/ssl/tests/unit/test_session_resumption.js
--- a/security/manager/ssl/nsNSSCallbacks.cpp
+++ b/security/manager/ssl/nsNSSCallbacks.cpp
@@ -949,26 +949,27 @@ AccumulateCipherSuite(Telemetry::Histogr
       break;
   }
   MOZ_ASSERT(value != 0);
   Telemetry::Accumulate(probe, value);
 }
 
 // In the case of session resumption, the AuthCertificate hook has been bypassed
 // (because we've previously successfully connected to our peer). That being the
-// case, we unfortunately don't know if the peer's server certificate verified
-// as extended validation or not. To address this, we attempt to build a
-// verified EV certificate chain here using as much of the original context as
-// possible (e.g. stapled OCSP responses, SCTs, the hostname, the first party
-// domain, etc.). Note that because we are on the socket thread, this must not
-// cause any network requests, hence the use of FLAG_LOCAL_ONLY.
-// Similarly, we need to determine the certificate's CT status.
+// case, we unfortunately don't know what the verified certificate chain was, if
+// the peer's server certificate verified as extended validation, or what its CT
+// status is (if enabled). To address this, we attempt to build a certificate
+// chain here using as much of the original context as possible (e.g. stapled
+// OCSP responses, SCTs, the hostname, the first party domain, etc.). Note that
+// because we are on the socket thread, this must not cause any network
+// requests, hence the use of FLAG_LOCAL_ONLY.
 static void
-DetermineEVAndCTStatusAndSetNewCert(RefPtr<nsSSLStatus> sslStatus,
-                                    PRFileDesc* fd, nsNSSSocketInfo* infoObject)
+RebuildVerifiedCertificateInformation(RefPtr<nsSSLStatus> sslStatus,
+                                      PRFileDesc* fd,
+                                      nsNSSSocketInfo* infoObject)
 {
   MOZ_ASSERT(sslStatus);
   MOZ_ASSERT(fd);
   MOZ_ASSERT(infoObject);
 
   if (!sslStatus || !fd || !infoObject) {
     return;
   }
@@ -996,18 +997,17 @@ DetermineEVAndCTStatusAndSetNewCert(RefP
   const SECItem* sctsFromTLSExtension = SSL_PeerSignedCertTimestamps(fd);
   if (sctsFromTLSExtension && sctsFromTLSExtension->len == 0) {
     // SSL_PeerSignedCertTimestamps returns null on error and empty item
     // when no extension was returned by the server. We always use null when
     // no extension was received (for whatever reason), ignoring errors.
     sctsFromTLSExtension = nullptr;
   }
 
-  int flags = mozilla::psm::CertVerifier::FLAG_LOCAL_ONLY |
-              mozilla::psm::CertVerifier::FLAG_MUST_BE_EV;
+  int flags = mozilla::psm::CertVerifier::FLAG_LOCAL_ONLY;
   if (!infoObject->SharedState().IsOCSPStaplingEnabled() ||
       !infoObject->SharedState().IsOCSPMustStapleEnabled()) {
     flags |= CertVerifier::FLAG_TLS_IGNORE_STATUS_REQUEST;
   }
 
   SECOidTag evOidPolicy;
   CertificateTransparencyInfo certificateTransparencyInfo;
   UniqueCERTCertList builtChain;
@@ -1025,16 +1025,21 @@ DetermineEVAndCTStatusAndSetNewCert(RefP
     infoObject->GetOriginAttributes(),
     &evOidPolicy,
     nullptr, // OCSP stapling telemetry
     nullptr, // key size telemetry
     nullptr, // SHA-1 telemetry
     nullptr, // pinning telemetry
     &certificateTransparencyInfo);
 
+  if (rv != Success) {
+    MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
+            ("HandshakeCallback: couldn't rebuild verified certificate info"));
+  }
+
   RefPtr<nsNSSCertificate> nssc(nsNSSCertificate::Create(cert.get()));
   if (rv == Success && evOidPolicy != SEC_OID_UNKNOWN) {
     sslStatus->SetCertificateTransparencyInfo(certificateTransparencyInfo);
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
             ("HandshakeCallback using NEW cert %p (is EV)", nssc.get()));
     sslStatus->SetServerCert(nssc, EVStatus::EV);
   } else {
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
@@ -1243,17 +1248,17 @@ void HandshakeCallback(PRFileDesc* fd, v
                                                 infoObject->GetPort());
     }
   }
 
   if (status->HasServerCert()) {
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
            ("HandshakeCallback KEEPING existing cert\n"));
   } else {
-    DetermineEVAndCTStatusAndSetNewCert(status, fd, infoObject);
+    RebuildVerifiedCertificateInformation(status, fd, infoObject);
   }
 
   nsCOMPtr<nsIX509CertList> succeededCertChain;
   // This always returns NS_OK, but the list could be empty. This is a
   // best-effort check for now. Bug 731478 will reduce the incidence of empty
   // succeeded cert chains through better caching.
   Unused << status->GetSucceededCertChain(getter_AddRefs(succeededCertChain));
   bool distrustImminent;
--- a/security/manager/ssl/tests/unit/test_session_resumption.js
+++ b/security/manager/ssl/tests/unit/test_session_resumption.js
@@ -39,16 +39,18 @@ function add_resume_non_ev_with_override
   add_connection_test("expired.example.com", PRErrorCodeSuccess, null,
     (transportSecurityInfo) => {
       ok(transportSecurityInfo.securityState &
          Ci.nsIWebProgressListener.STATE_CERT_USER_OVERRIDDEN,
          "expired.example.com should have STATE_CERT_USER_OVERRIDDEN flag");
       let sslStatus = transportSecurityInfo
                         .QueryInterface(Ci.nsISSLStatusProvider)
                         .SSLStatus;
+      ok(!sslStatus.succeededCertChain,
+         "ev-test.example.com should not have succeededCertChain set");
       ok(!sslStatus.isDomainMismatch,
          "expired.example.com should not have isDomainMismatch set");
       ok(sslStatus.isNotValidAtThisTime,
          "expired.example.com should have isNotValidAtThisTime set");
       ok(!sslStatus.isUntrusted,
          "expired.example.com should not have isUntrusted set");
       ok(!sslStatus.isExtendedValidation,
          "expired.example.com should not have isExtendedValidation set");
@@ -64,16 +66,18 @@ function add_one_ev_test() {
   add_connection_test("ev-test.example.com", PRErrorCodeSuccess, null,
     (transportSecurityInfo) => {
       ok(!(transportSecurityInfo.securityState &
            Ci.nsIWebProgressListener.STATE_CERT_USER_OVERRIDDEN),
          "ev-test.example.com should not have STATE_CERT_USER_OVERRIDDEN flag");
       let sslStatus = transportSecurityInfo
                         .QueryInterface(Ci.nsISSLStatusProvider)
                         .SSLStatus;
+      ok(sslStatus.succeededCertChain,
+         "ev-test.example.com should have succeededCertChain set");
       ok(!sslStatus.isDomainMismatch,
          "ev-test.example.com should not have isDomainMismatch set");
       ok(!sslStatus.isNotValidAtThisTime,
          "ev-test.example.com should not have isNotValidAtThisTime set");
       ok(!sslStatus.isUntrusted,
          "ev-test.example.com should not have isUntrusted set");
       ok(!gEVExpected || sslStatus.isExtendedValidation,
          "ev-test.example.com should have isExtendedValidation set " +
@@ -87,36 +91,79 @@ function add_one_ev_test() {
 // clearing the session cache, we should connect successfully again, this time
 // with session resumption. The certificate should again be EV in debug builds.
 function add_resume_ev_test() {
   const SERVER_PORT = 8888;
   let expectedRequestPaths = gEVExpected ? [ "ev-test-intermediate", "ev-test" ]
                                          : [ "ev-test" ];
   let responseTypes = gEVExpected ? [ "good", "good" ] : [ "good" ];
   // Since we cache OCSP responses, we only ever actually serve one set.
-  let ocspResponder = startOCSPResponder(SERVER_PORT, "localhost", "bad_certs",
-                                         expectedRequestPaths,
-                                         expectedRequestPaths.slice(),
-                                         null, responseTypes);
+  let ocspResponder;
+  // If we don't wrap this in an `add_test`, the OCSP responder will be running
+  // while we are actually running unrelated testcases, which can disrupt them.
+  add_test(() => {
+    ocspResponder = startOCSPResponder(SERVER_PORT, "localhost", "bad_certs",
+                                       expectedRequestPaths,
+                                       expectedRequestPaths.slice(),
+                                       null, responseTypes);
+    run_next_test();
+  });
   // We should be able to connect and verify the certificate as EV (in debug
   // builds).
   add_one_ev_test();
   // We should be able to connect again (using session resumption). In debug
   // builds, the certificate should be noted as EV. Again, it's important that
   // nothing clears the TLS cache in between these two operations.
   add_one_ev_test();
 
   add_test(() => {
     ocspResponder.stop(run_next_test);
   });
 }
 
+const GOOD_DOMAIN = "good.include-subdomains.pinning.example.com";
+
+// Helper function that adds a test that connects to a domain that should
+// succeed (but isn't EV) and verifies that its succeededCertChain gets set
+// appropriately.
+function add_one_non_ev_test() {
+  add_connection_test(GOOD_DOMAIN, PRErrorCodeSuccess, null,
+    (transportSecurityInfo) => {
+      ok(!(transportSecurityInfo.securityState &
+           Ci.nsIWebProgressListener.STATE_CERT_USER_OVERRIDDEN),
+         `${GOOD_DOMAIN} should not have STATE_CERT_USER_OVERRIDDEN flag`);
+      let sslStatus = transportSecurityInfo
+                        .QueryInterface(Ci.nsISSLStatusProvider)
+                        .SSLStatus;
+      ok(sslStatus.succeededCertChain,
+         `${GOOD_DOMAIN} should have succeededCertChain set`);
+      ok(!sslStatus.isDomainMismatch,
+         `${GOOD_DOMAIN} should not have isDomainMismatch set`);
+      ok(!sslStatus.isNotValidAtThisTime,
+         `${GOOD_DOMAIN} should not have isNotValidAtThisTime set`);
+      ok(!sslStatus.isUntrusted,
+         `${GOOD_DOMAIN} should not have isUntrusted set`);
+      ok(!sslStatus.isExtendedValidation,
+         `${GOOD_DOMAIN} should not have isExtendedValidation set`);
+    }
+  );
+}
+
+// This test is similar, except with non-extended validation. We should connect
+// successfully, and the certificate should not be EV. Without clearing the
+// session cache, we should connect successfully again, this time with session
+// resumption. In this case, though, we want to ensure the succeededCertChain is
+// set.
+function add_resume_non_ev_test() {
+  add_one_non_ev_test();
+  add_one_non_ev_test();
+}
+
 const statsPtr = getSSLStatistics();
 const toInt32 = ctypes.Int64.lo;
-const GOOD_DOMAIN = "good.include-subdomains.pinning.example.com";
 
 // Connect to the same domain with two origin attributes and check if any ssl
 // session is resumed.
 function add_origin_attributes_test(originAttributes1, originAttributes2,
                                     resumeExpected) {
   add_connection_test(GOOD_DOMAIN, PRErrorCodeSuccess, clearSessionCache, null,
                       null, originAttributes1);
 
@@ -141,18 +188,19 @@ function add_origin_attributes_test(orig
                         equal(toInt32(stats.sch_sid_cache_misses),
                               missesBeforeConnect + expectedMisses,
                               "Unexpected cache misses");
                       }, null, originAttributes2);
 }
 
 function run_test() {
   add_tls_server_setup("BadCertServer", "bad_certs");
+  add_resume_ev_test();
+  add_resume_non_ev_test();
   add_resume_non_ev_with_override_test();
-  add_resume_ev_test();
   add_origin_attributes_test({}, {}, true);
   add_origin_attributes_test({ userContextId: 1 }, { userContextId: 2 }, false);
   add_origin_attributes_test({ userContextId: 3 }, { userContextId: 3 }, true);
   add_origin_attributes_test({ firstPartyDomain: "foo.com" },
                              { firstPartyDomain: "bar.com" }, false);
   add_origin_attributes_test({ firstPartyDomain: "baz.com" },
                              { firstPartyDomain: "baz.com" }, true);
   run_next_test();