Bug 1165911 - Do more safety checks when gathering successful TLS connection telemetry. r=Cykesiopka, a=lizzard
authorDavid Keeler <dkeeler@mozilla.com>
Wed, 20 May 2015 14:37:54 -0700
changeset 267546 cf713bf395eeca1fa3063f2a0ecf42260b5e85bf
parent 267545 962292dc9f4ffc59dbf60123895cd2fe25af9828
child 267547 85da196ff50df91659db167b5495d59c68ba1c16
push id830
push userraliiev@mozilla.com
push dateFri, 19 Jun 2015 19:24:37 +0000
treeherdermozilla-release@932614382a68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersCykesiopka, lizzard
bugs1165911
milestone39.0
Bug 1165911 - Do more safety checks when gathering successful TLS connection telemetry. r=Cykesiopka, a=lizzard
security/manager/ssl/src/SSLServerCertVerification.cpp
--- a/security/manager/ssl/src/SSLServerCertVerification.cpp
+++ b/security/manager/ssl/src/SSLServerCertVerification.cpp
@@ -890,26 +890,35 @@ GatherBaselineRequirementsTelemetry(cons
   CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
   PR_ASSERT(!(CERT_LIST_END(endEntityNode, certList) ||
               CERT_LIST_END(rootNode, certList)));
   if (CERT_LIST_END(endEntityNode, certList) ||
       CERT_LIST_END(rootNode, certList)) {
     return;
   }
   CERTCertificate* cert = endEntityNode->cert;
+  PR_ASSERT(cert);
+  if (!cert) {
+    return;
+  }
   mozilla::pkix::ScopedPtr<char, PORT_Free_string> commonName(
     CERT_GetCommonName(&cert->subject));
   // This only applies to certificates issued by authorities in our root
   // program.
+  CERTCertificate* rootCert = rootNode->cert;
+  PR_ASSERT(rootCert);
+  if (!rootCert) {
+    return;
+  }
   bool isBuiltIn = false;
-  SECStatus rv = IsCertBuiltInRoot(rootNode->cert, isBuiltIn);
+  SECStatus rv = IsCertBuiltInRoot(rootCert, isBuiltIn);
   if (rv != SECSuccess || !isBuiltIn) {
     PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
-           ("BR telemetry: '%s' not a built-in root (or IsCertBuiltInRoot "
-            "failed)\n", commonName.get()));
+           ("BR telemetry: root certificate for '%s' is not a built-in root "
+            "(or IsCertBuiltInRoot failed)\n", commonName.get()));
     return;
   }
   SECItem altNameExtension;
   rv = CERT_FindCertExtension(cert, SEC_OID_X509_SUBJECT_ALT_NAME,
                               &altNameExtension);
   if (rv != SECSuccess) {
     PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
            ("BR telemetry: no subject alt names extension for '%s'\n",
@@ -1046,29 +1055,39 @@ GatherEKUTelemetry(const ScopedCERTCertL
   CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
   PR_ASSERT(!(CERT_LIST_END(endEntityNode, certList) ||
               CERT_LIST_END(rootNode, certList)));
   if (CERT_LIST_END(endEntityNode, certList) ||
       CERT_LIST_END(rootNode, certList)) {
     return;
   }
   CERTCertificate* endEntityCert = endEntityNode->cert;
+  PR_ASSERT(endEntityCert);
+  if (!endEntityCert) {
+    return;
+  }
 
   // Only log telemetry if the root CA is built-in
+  CERTCertificate* rootCert = rootNode->cert;
+  PR_ASSERT(rootCert);
+  if (!rootCert) {
+    return;
+  }
   bool isBuiltIn = false;
-  SECStatus rv = IsCertBuiltInRoot(rootNode->cert, isBuiltIn);
+  SECStatus rv = IsCertBuiltInRoot(rootCert, isBuiltIn);
   if (rv != SECSuccess || !isBuiltIn) {
     return;
   }
 
   // Find the EKU extension, if present
   bool foundEKU = false;
   SECOidTag oidTag;
   CERTCertExtension* ekuExtension = nullptr;
-  for (size_t i = 0; endEntityCert->extensions[i]; i++) {
+  for (size_t i = 0; endEntityCert->extensions && endEntityCert->extensions[i];
+       i++) {
     oidTag = SECOID_FindOIDTag(&endEntityCert->extensions[i]->id);
     if (oidTag == SEC_OID_X509_EXT_KEY_USAGE) {
       foundEKU = true;
       ekuExtension = endEntityCert->extensions[i];
     }
   }
 
   if (!foundEKU) {
@@ -1114,22 +1133,27 @@ GatherEKUTelemetry(const ScopedCERTCertL
 void
 GatherRootCATelemetry(const ScopedCERTCertList& certList)
 {
   CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
   PR_ASSERT(rootNode);
   if (!rootNode) {
     return;
   }
-
-  // Only log telemetry if the certificate list is non-empty
-  if (!CERT_LIST_END(rootNode, certList)) {
-    AccumulateTelemetryForRootCA(Telemetry::CERT_VALIDATION_SUCCESS_BY_CA,
-                                 rootNode->cert);
+  PR_ASSERT(!CERT_LIST_END(rootNode, certList));
+  if (CERT_LIST_END(rootNode, certList)) {
+    return;
   }
+  CERTCertificate* rootCert = rootNode->cert;
+  PR_ASSERT(rootCert);
+  if (!rootCert) {
+    return;
+  }
+  AccumulateTelemetryForRootCA(Telemetry::CERT_VALIDATION_SUCCESS_BY_CA,
+                               rootCert);
 }
 
 // There are various things that we want to measure about certificate
 // chains that we accept.  This is a single entry point for all of them.
 void
 GatherSuccessfulValidationTelemetry(const ScopedCERTCertList& certList)
 {
   GatherBaselineRequirementsTelemetry(certList);