bug 1434831 - ensure nsITransportSecurityInfo.failedCertChain contains the *peer cert chain* r?jcj draft
authorDavid Keeler <dkeeler@mozilla.com>
Thu, 01 Feb 2018 12:29:04 -0800
changeset 750401 d68049fdb0a58453d757bcd693f438f9f8d262db
parent 750383 27e993937c984b0e2bd05aeb17d791c7c036ad21
push id97639
push userbmo:dkeeler@mozilla.com
push dateFri, 02 Feb 2018 01:08:41 +0000
reviewersjcj
bugs1434831, 1406856
milestone60.0a1
bug 1434831 - ensure nsITransportSecurityInfo.failedCertChain contains the *peer cert chain* r?jcj 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
@@ -1388,17 +1388,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;
@@ -1406,18 +1406,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
@@ -1448,18 +1449,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) {
@@ -1472,27 +1473,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(
@@ -1570,17 +1571,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);
@@ -1750,17 +1752,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);