Squid Assertion String.cc:221: "str"

This bug can be caused by certificates does not contain a CN field. In this
case the Ssl::ErrorDetail::cn method may return NULL causing this assertion
somewhere inside Ssl::ErrorDetail::buildDetail method, which expects always
a non NULL value from Ssl::ErrorDetail::cn and similar methods.

This patch try to hardening the Ssl::ErrorDetail error formating functions to
avoid always check for NULL values and also avoid sending wrong information
for various certificate fields in the case of an error while extracting the
information from certificate..

This is a Measurement Factory project

diff --git a/src/ssl/ErrorDetail.cc b/src/ssl/ErrorDetail.cc
index b42548bf8583b3adbb74daf0908d50d2042e8b0c..f717d4d8615665fb4ee3346b555cd06bca4a2be0 100644 (file)
--- a/src/ssl/ErrorDetail.cc
+++ b/src/ssl/ErrorDetail.cc
@@ -431,13 +431,13 @@ Ssl::ErrorDetail::err_frm_code Ssl::ErrorDetail::ErrorFormatingCodes[] = {
  */
 const char  *Ssl::ErrorDetail::subject() const
 {
-    if (!broken_cert)
-        return "[Not available]";
-
-    static char tmpBuffer[256]; // A temporary buffer
-    X509_NAME_oneline(X509_get_subject_name(broken_cert.get()), tmpBuffer,
-                      sizeof(tmpBuffer));
-    return tmpBuffer;
+    if (broken_cert.get()) {
+        static char tmpBuffer[256]; // A temporary buffer
+        if (X509_NAME_oneline(X509_get_subject_name(broken_cert.get()), tmpBuffer,
+                      sizeof(tmpBuffer)))
+            return tmpBuffer;
+    }
+    return "[Not available]";
 }
 
 // helper function to be used with Ssl::matchX509CommonNames
@@ -446,9 +446,11 @@ static int copy_cn(void *check_data,  ASN1_STRING *cn_data)
     String *str = (String *)check_data;
     if (!str) // no data? abort
         return 0;
-    if (str->size() > 0)
-        str->append(", ");
-    str->append((const char *)cn_data->data, cn_data->length);
+    if (cn_data && cn_data->length) {
+        if (str->size() > 0)
+            str->append(", ");
+        str->append((const char *)cn_data->data, cn_data->length);
+    }
     return 1;
 }
 
@@ -457,13 +459,14 @@ static int copy_cn(void *check_data,  ASN1_STRING *cn_data)
  */
 const char *Ssl::ErrorDetail::cn() const
 {
-    if (!broken_cert)
-        return "[Not available]";
-
-    static String tmpStr;  ///< A temporary string buffer
-    tmpStr.clean();
-    Ssl::matchX509CommonNames(broken_cert.get(), &tmpStr, copy_cn);
-    return tmpStr.termedBuf();
+    if (broken_cert.get()) {
+        static String tmpStr;  ///< A temporary string buffer
+        tmpStr.clean();
+        Ssl::matchX509CommonNames(broken_cert.get(), &tmpStr, copy_cn);
+        if (tmpStr.size())
+            return tmpStr.termedBuf();
+    }
+    return "[Not available]";
 }
 
 /**
@@ -471,12 +474,12 @@ const char *Ssl::ErrorDetail::cn() const
  */
 const char *Ssl::ErrorDetail::ca_name() const
 {
-    if (!broken_cert)
-        return "[Not available]";
-
-    static char tmpBuffer[256]; // A temporary buffer
-    X509_NAME_oneline(X509_get_issuer_name(broken_cert.get()), tmpBuffer, sizeof(tmpBuffer));
-    return tmpBuffer;
+    if (broken_cert.get()) {
+        static char tmpBuffer[256]; // A temporary buffer
+        if (X509_NAME_oneline(X509_get_issuer_name(broken_cert.get()), tmpBuffer, sizeof(tmpBuffer)))
+            return tmpBuffer;
+    }
+    return "[Not available]";
 }
 
 /**
@@ -484,13 +487,14 @@ const char *Ssl::ErrorDetail::ca_name() const
  */
 const char *Ssl::ErrorDetail::notbefore() const
 {
-    if (!broken_cert)
-        return "[Not available]";
-
-    static char tmpBuffer[256]; // A temporary buffer
-    ASN1_UTCTIME * tm = X509_get_notBefore(broken_cert.get());
-    Ssl::asn1timeToString(tm, tmpBuffer, sizeof(tmpBuffer));
-    return tmpBuffer;
+    if (broken_cert.get()) {
+        if (ASN1_UTCTIME * tm = X509_get_notBefore(broken_cert.get())) {
+            static char tmpBuffer[256]; // A temporary buffer
+            Ssl::asn1timeToString(tm, tmpBuffer, sizeof(tmpBuffer));
+            return tmpBuffer;
+        }
+    }
+    return "[Not available]";
 }
 
 /**
@@ -498,13 +502,14 @@ const char *Ssl::ErrorDetail::notbefore() const
  */
 const char *Ssl::ErrorDetail::notafter() const
 {
-    if (!broken_cert)
-        return "[Not available]";
-
-    static char tmpBuffer[256]; // A temporary buffer
-    ASN1_UTCTIME * tm = X509_get_notAfter(broken_cert.get());
-    Ssl::asn1timeToString(tm, tmpBuffer, sizeof(tmpBuffer));
-    return tmpBuffer;
+    if (broken_cert.get()) {
+        if (ASN1_UTCTIME * tm = X509_get_notAfter(broken_cert.get())) {
+            static char tmpBuffer[256]; // A temporary buffer
+            Ssl::asn1timeToString(tm, tmpBuffer, sizeof(tmpBuffer));
+            return tmpBuffer;
+        }
+    }
+    return "[Not available]";
 }
 
 /**