bug 1434831 - ensure nsITransportSecurityInfo.failedCertChain contains the *peer cert chain* r=jcj a=lizzard
authorDavid Keeler <dkeeler@mozilla.com>
Thu, 01 Feb 2018 12:29:04 -0800
changeset 454681 d15af2a5aa189688f925b8d0f0cfff767ca708c6
parent 454680 08e9d3a3a6b13997cac9fa92231f1b17cb00c527
child 454682 5a2d33d40dc320f449e5cbd3dcf81b341f9c84fa
push id1648
push usermtabara@mozilla.com
push dateThu, 01 Mar 2018 12:45:47 +0000
treeherdermozilla-release@cbb9688c2eeb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjcj, lizzard
bugs1434831, 1406856
milestone59.0
bug 1434831 - ensure nsITransportSecurityInfo.failedCertChain contains the *peer cert chain* r=jcj a=lizzard In bug 1406856 the failedCertChain property of nsITransportSecurityInfo was changed to hold the built certificate chain out parameter from the call to CertVerifier::VerifySSLServerCert. However, this was incorrect for two reasons: a) failedCertChain is supposed to be the peer cert chain delivered by the server in the TLS handshake and b) if VerifySSLServerCert returns a failing result, the out parameter is not guaranteed to hold any meaningful information, and must not be used. This patch sets failedCertChain to the appropriate value. MozReview-Commit-ID: BEXs5XH9SpK
security/manager/ssl/SSLServerCertVerification.cpp
security/manager/ssl/tests/unit/test_cert_chains.js
--- a/security/manager/ssl/SSLServerCertVerification.cpp
+++ b/security/manager/ssl/SSLServerCertVerification.cpp
@@ -1389,17 +1389,17 @@ AuthCertificate(CertVerifier& certVerifi
   MOZ_ASSERT(cert);
 
   // We want to avoid storing any intermediate cert information when browsing
   // in private, transient contexts.
   bool saveIntermediates =
     !(providerFlags & nsISocketProvider::NO_PERMANENT_STORAGE);
 
   SECOidTag evOidPolicy;
-  UniqueCERTCertList certList;
+  UniqueCERTCertList builtCertChain;
   CertVerifier::OCSPStaplingStatus ocspStaplingStatus =
     CertVerifier::OCSP_STAPLING_NEVER_CHECKED;
   KeySizeStatus keySizeStatus = KeySizeStatus::NeverChecked;
   SHA1ModeResult sha1ModeResult = SHA1ModeResult::NeverChecked;
   PinningTelemetryInfo pinningTelemetryInfo;
   CertificateTransparencyInfo certificateTransparencyInfo;
 
   int flags = 0;
@@ -1407,18 +1407,19 @@ AuthCertificate(CertVerifier& certVerifi
       !infoObject->SharedState().IsOCSPMustStapleEnabled()) {
     flags |= CertVerifier::FLAG_TLS_IGNORE_STATUS_REQUEST;
   }
 
   Result rv = certVerifier.VerifySSLServerCert(cert, stapledOCSPResponse,
                                                sctsFromTLSExtension, time,
                                                infoObject,
                                                infoObject->GetHostName(),
-                                               certList, saveIntermediates,
-                                               flags, infoObject->
+                                               builtCertChain,
+                                               saveIntermediates, flags,
+                                               infoObject->
                                                       GetOriginAttributes(),
                                                &evOidPolicy,
                                                &ocspStaplingStatus,
                                                &keySizeStatus, &sha1ModeResult,
                                                &pinningTelemetryInfo,
                                                &certificateTransparencyInfo);
 
   uint32_t evStatus = (rv != Success) ? 0                   // 0 = Failure
@@ -1449,18 +1450,18 @@ AuthCertificate(CertVerifier& certVerifi
   }
 
   if (rv == Success) {
     // Certificate verification succeeded. Delete any potential record of
     // certificate error bits.
     RememberCertErrorsTable::GetInstance().RememberCertHasError(infoObject,
                                                                 nullptr,
                                                                 SECSuccess);
-    GatherSuccessfulValidationTelemetry(certList);
-    GatherCertificateTransparencyTelemetry(certList,
+    GatherSuccessfulValidationTelemetry(builtCertChain);
+    GatherCertificateTransparencyTelemetry(builtCertChain,
                                   /*isEV*/ evOidPolicy != SEC_OID_UNKNOWN,
                                            certificateTransparencyInfo);
 
     // The connection may get terminated, for example, if the server requires
     // a client cert. Let's provide a minimal SSLStatus
     // to the caller that contains at least the cert and its status.
     RefPtr<nsSSLStatus> status(infoObject->SSLStatus());
     if (!status) {
@@ -1473,27 +1474,27 @@ AuthCertificate(CertVerifier& certVerifi
       evStatus = EVStatus::NotEV;
     } else {
       evStatus = EVStatus::EV;
     }
 
     RefPtr<nsNSSCertificate> nsc = nsNSSCertificate::Create(cert.get());
     status->SetServerCert(nsc, evStatus);
 
-    status->SetSucceededCertChain(Move(certList));
+    status->SetSucceededCertChain(Move(builtCertChain));
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
-        ("AuthCertificate setting NEW cert %p", nsc.get()));
+            ("AuthCertificate setting NEW cert %p", nsc.get()));
 
     status->SetCertificateTransparencyInfo(certificateTransparencyInfo);
   }
 
   if (rv != Success) {
     // Certificate validation failed; store the peer certificate chain on
     // infoObject so it can be used for error reporting.
-    infoObject->SetFailedCertChain(Move(certList));
+    infoObject->SetFailedCertChain(Move(peerCertChain));
     PR_SetError(MapResultToPRErrorCode(rv), 0);
   }
 
   return rv == Success ? SECSuccess : SECFailure;
 }
 
 /*static*/ SECStatus
 SSLServerCertVerificationJob::Dispatch(
@@ -1578,17 +1579,18 @@ SSLServerCertVerificationJob::Run()
 
     // Reset the error code here so we can detect if AuthCertificate fails to
     // set the error code if/when it fails.
     PR_SetError(0, 0);
     SECStatus rv = AuthCertificate(*mCertVerifier, mInfoObject, mCert,
                                    mPeerCertChain, mStapledOCSPResponse.get(),
                                    mSCTsFromTLSExtension.get(),
                                    mProviderFlags, mTime);
-    MOZ_ASSERT(mPeerCertChain || rv != SECSuccess,
+    MOZ_ASSERT((mPeerCertChain && rv == SECSuccess) ||
+               (!mPeerCertChain && rv != SECSuccess),
                "AuthCertificate() should take ownership of chain on failure");
     if (rv == SECSuccess) {
       uint32_t interval = (uint32_t) ((TimeStamp::Now() - mJobStartTime).ToMilliseconds());
       RefPtr<SSLServerCertVerificationResult> restart(
         new SSLServerCertVerificationResult(mInfoObject, 0,
                                             successTelemetry, interval));
       restart->Dispatch();
       Telemetry::Accumulate(Telemetry::SSL_CERT_ERROR_OVERRIDES, 1);
@@ -1759,17 +1761,18 @@ AuthCertificateHook(void* arg, PRFileDes
   // We can't do certificate verification on a background thread, because the
   // thread doing the network I/O may not interrupt its network I/O on receipt
   // of our SSLServerCertVerificationResult event, and/or it might not even be
   // a non-blocking socket.
 
   SECStatus rv = AuthCertificate(*certVerifier, socketInfo, serverCert,
                                  peerCertChain, stapledOCSPResponse,
                                  sctsFromTLSExtension, providerFlags, now);
-  MOZ_ASSERT(peerCertChain || rv != SECSuccess,
+  MOZ_ASSERT((peerCertChain && rv == SECSuccess) ||
+             (!peerCertChain && rv != SECSuccess),
              "AuthCertificate() should take ownership of chain on failure");
   if (rv == SECSuccess) {
     Telemetry::Accumulate(Telemetry::SSL_CERT_ERROR_OVERRIDES, 1);
     return SECSuccess;
   }
 
   PRErrorCode error = PR_GetError();
   if (error != 0) {
--- a/security/manager/ssl/tests/unit/test_cert_chains.js
+++ b/security/manager/ssl/tests/unit/test_cert_chains.js
@@ -97,16 +97,34 @@ function run_test() {
                " connection failure");
       let originalCertChain = build_cert_chain(["expired-ee", "test-ca"]);
       ok(originalCertChain.equals(securityInfo.failedCertChain),
          "failedCertChain should equal the original cert chain for an" +
          " overrideable connection failure");
     }
   );
 
+  // Test overrideable connection failure (failedCertChain should be non-null)
+  add_connection_test(
+    "unknownissuer.example.com",
+    SEC_ERROR_UNKNOWN_ISSUER,
+    null,
+    function withSecurityInfo(securityInfo) {
+      securityInfo.QueryInterface(Ci.nsITransportSecurityInfo);
+      test_security_info_serialization(securityInfo, SEC_ERROR_UNKNOWN_ISSUER);
+      notEqual(securityInfo.failedCertChain, null,
+               "failedCertChain should not be null for an overrideable" +
+               " connection failure");
+      let originalCertChain = build_cert_chain(["unknownissuer"]);
+      ok(originalCertChain.equals(securityInfo.failedCertChain),
+         "failedCertChain should equal the original cert chain for an" +
+         " overrideable connection failure");
+    }
+  );
+
   // Test non-overrideable error (failedCertChain should be non-null)
   add_connection_test(
     "inadequatekeyusage.example.com",
     SEC_ERROR_INADEQUATE_KEY_USAGE,
     null,
     function withSecurityInfo(securityInfo) {
       securityInfo.QueryInterface(Ci.nsITransportSecurityInfo);
       test_security_info_serialization(securityInfo, SEC_ERROR_INADEQUATE_KEY_USAGE);