From: Christos Tsantilas <chtsanti@users.sourceforge.net>
Date: Wed, 3 Jun 2015 11:12:40 +0000 (+0300)
Subject: Squid Assertion String.cc:221: "str"
X-Git-Tag: merge-candidate-3-v1~92
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=ed58b5234e629f24dc617759b82bddaa53be2fb8;p=thirdparty%2Fsquid.git

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 b42548bf85..f717d4d861 100644
--- 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]";
 }
 
 /**