bug 1165911 - do more safety checks when gathering successful TLS connection telemetry r=Cykesiopka
authorDavid Keeler <dkeeler@mozilla.com>
Mon, 18 May 2015 10:37:38 -0700
changeset 244656 006a48240dfd928f5890277f262866047884b425
parent 244655 2ac9fe6d2333c1b93856f6f868d352e4c94bdead
child 244657 36ff3229daffc039c13b1f168a71858ecafca548
push id28786
push usercbook@mozilla.com
push dateWed, 20 May 2015 13:54:15 +0000
treeherdermozilla-central@8d8df22fe72d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersCykesiopka
bugs1165911
milestone41.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 1165911 - do more safety checks when gathering successful TLS connection telemetry r=Cykesiopka
security/manager/ssl/src/SSLServerCertVerification.cpp
--- a/security/manager/ssl/src/SSLServerCertVerification.cpp
+++ b/security/manager/ssl/src/SSLServerCertVerification.cpp
@@ -888,26 +888,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;
+  }
   UniquePtr<char, void(&)(void*)>
     commonName(CERT_GetCommonName(&cert->subject), PORT_Free);
   // 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",
@@ -1044,29 +1053,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) {
@@ -1112,22 +1131,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);