Memory leak with multi threaded c++ application on ssl certificate validation

Question

We are using openssl library in our multi threaded c++ application. The application eats up all the memory within 3 days (7 GB instance) due to a memory leak, only if SSL certificate validation is enabled.

Please find my application flow here:

On app launch, We create 150 threads for syncing 30k users data and reserving one SSL_CTX_new object per thread. The same object is reused until the process is killed. The object SSL_CTX_new is created only once on thread initialization, it will be reused for all subsequent ssl connections.

We do the following on processing user's data:

A thread creates a new socket connection to the third party server via ssl and once the required data is fetched from the 3rd party server, ssl connection is terminated. Similarly all threads pick up one user at a time from the queue and fetches the data from the server.

We have to do above connect, fetch data and disconnect ssl connections for all 30k users.

Please find pseudocode example of our application:

ThreadTask() 
{
    ssldata* ssl_data;

   1. Creates SSL connection: user_ssl_new_connect(ssl_data)
   2. Fetch users data
   3. Terminate ssl connection: ssl_abort()
}



char* user_ssl_new_connect(ssldata* ssl_data)  {

  SSL_CTX *ssl_context = InitsslonePerThread();
  if (!ssl_context) {
    if (!(ssl_context = SSL_CTX_new (SSLv23_client_method ()) {
      retur NULL;
    } 
  } 

  SSL_CTX_set_options (ssl_context,SSL_OP_NO_COMPRESSION|SSL_MODE_RELEASE_BUFFERS);
  SSL_CTX_set_verify (ssl_context,SSL_VERIFY_PEER,ssl_open_verify);
  SSL_CTX_set_default_verify_paths (ssl_context);
  char * s = "sslpath"
  SSL_CTX_load_verify_locations (ssl_context,s,NIL);
  SetsslconnectionPerThread(ssl_context);

  if (!(ssl_data->sslconnection = (SSL *) SSL_new (ssl_context)))
    return NULL

  bio = BIO_new_socket (ssl_data->sockettcpsi,BIO_NOCLOSE);
  SSL_set_bio (ssl_data->sslconnection,bio,bio);
  SSL_set_connect_state(ssl_data->sslconnection);
  if (SSL_in_init(ssl_data->sslconnection)) SSL_total_renegotiations (ssl_data->sslconnection);
      /* now negotiate SSL */
  if ((retval = SSL_write (ssl_data->sslconnection,"",0)) < 0) {    
    return NULL
  }
      /* validating host names? */
  if ((err = ssl_validate_cert (cert = SSL_get_peer_certificate   (sslconnection),host))) {
      return NULL;
  }
}


// one ssl_context per thread in global variable 
ssl_context* InitsslonePerThread() {

  yULong threadid;
  threadid = (unsigned long) pthread_self();

  if ssl_context is not created for this threadid
     returns new ssl_context.
  else 
    returns previous ssl_context.
}

void SetsslconnectionPerThread(ssl_context*) {

  yULong threadid;
  threadid = (unsigned long) pthread_self();

  #setting ssl_context in global variable 
}


long ssl_abort (ssldata* ssl_data)
{
   if (ssl_data->sslconnection) {        /* close SSL connection */
        SSL_shutdown (ssl_data->sslconnection);
        SSL_free (ssl_data->sslconnection);
    }
    return NIL;
}



static char *ssl_validate_cert (X509 *cert,char *host)
{
    int i,n;
    char *s,*t,*ret;
    void *ext;
    GENERAL_NAME *name;

    char tmp[MAILTMPLEN];

    /* make sure have a certificate */
    if (!cert) ret = "No certificate from server";
    /* and that it has a name */
    else if (!cert->name) ret = "No name in certificate";
    /* locate CN */
    else if (s = strstr (cert->name,"/CN=")) {
        if (t = strchr (s += 4,'/')) *t = '\0';
        /* host name matches pattern? */
        ret = ssl_compare_hostnames (host,s) ? NIL :
              "Server name does not match certificate";
        if (t) *t = '/';        /* restore smashed delimiter */
        /* if mismatch, see if in extensions */
        if (ret && (ext = X509_get_ext_d2i (cert,NID_subject_alt_name,NIL,NIL)) &&
            (n = sk_GENERAL_NAME_num (ext)))
            /* older versions of OpenSSL use "ia5" instead of dNSName */
            for (i = 0; ret && (i < n); i++)
                if ((name = sk_GENERAL_NAME_value (ext,i)) &&
                    (name->type = GEN_DNS) && (s = name->d.ia5->data) &&
                    ssl_compare_hostnames (host,s)) ret = NIL;
    } else ret = "Unable to locate common name in certificate";
    return ret;
}

Answer 1

if ((err = ssl_validate_cert (cert = SSL_get_peer_certificate (sslconnection),host))) {
    return NULL;
}

You must free the X509* returned from SSL_get_peer_certificate using X509_free. There may be more leaks, but that one seems consistent with the description of your issue.

The memory leak is also why OpenSSL's TLS Client example immediately free's the X509*. Its reference counted, so its safe to decrement the count and use the X509* until the session is destroyed (the SSL*).

X509* cert = SSL_get_peer_certificate(ssl);
if(cert) { X509_free(cert); } /* Free immediately */
if(NULL == cert) handleFailure();
...

You may also be leaking some of the names returned from the name walk. I don't use IA5 strings, so I'm not certain. I use UTF8 strings, and they must be freed.

---

Here's some unrelated comments... You should probably include SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3:

SSL_CTX_set_options (ssl_context,SSL_OP_NO_COMPRESSION|SSL_MODE_RELEASE_BUFFERS);

You should also probably specify SSL_set_tlsext_host_name somewhere to use SNI for hosted environments, where the default site certificate may not be the target site's certificate. SNI is a TLS extension, so it speaks to the need for SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3.

You should also test the application well when using SSL_MODE_RELEASE_BUFFERS. I seem to recall it caused memory errors. Also see Issue 2167: OpenSSL fails if used from multiple threads and with SSL_MODE_RELEASE_BUFFERS, CVE-2010-5298, and Adam Langley's Overclocking SSL.

The sample program provided on the wiki TLS Client may also help you with you name matching. The best I can tell, the code is vulnerable to Marlinspike's embedded NULL tricks. See his Blackhat talk at More Tricks For Defeating SSL In Practice for more details.

---

This is just an observation... Since you are using C++, why are you not using smart pointers to manage your resources? Something like this works very well in practice, and it would have fixed the leak because X509_ptr has the destructor function specified:

X509_ptr cert(SSL_get_peer_certificate(ssl));
// Use cert, its free'd automatically
char* name = ssl_validate_cert(cert.get(), "www.example.com");

And:

using EC_KEY_ptr = std::unique_ptr<EC_KEY, decltype(&::EC_KEY_free)>;
using EC_GROUP_ptr = std::unique_ptr<EC_GROUP, decltype(&::EC_GROUP_free)>;
using EC_POINT_ptr = std::unique_ptr<EC_POINT, decltype(&::EC_POINT_free)>;

using DH_ptr = std::unique_ptr<DH, decltype(&::DH_free)>;

using RSA_ptr = std::unique_ptr<RSA, decltype(&::RSA_free)>;

using DSA_ptr = std::unique_ptr<DSA, decltype(&::DSA_free)>;

using EVP_PKEY_ptr = std::unique_ptr<EVP_PKEY, decltype(&::EVP_PKEY_free)>;

using BN_ptr = std::unique_ptr<BIGNUM, decltype(&::BN_free)>;

using FILE_ptr = std::unique_ptr<FILE, decltype(&::fclose)>;

using BIO_MEM_ptr = std::unique_ptr<BIO, decltype(&::BIO_free)>;
using BIO_FILE_ptr = std::unique_ptr<BIO, decltype(&::BIO_free)>;

using EVP_MD_CTX_ptr = std::unique_ptr<EVP_MD_CTX, decltype(&::EVP_MD_CTX_destroy)>;

using X509_ptr = std::unique_ptr<X509, decltype(&::X509_free)>;
using ASN1_INTEGER_ptr = std::unique_ptr<ASN1_INTEGER, decltype(&::ASN1_INTEGER_free)>;
using ASN1_TIME_ptr = std::unique_ptr<ASN1_TIME, decltype(&::ASN1_TIME_free)>;
using X509_EXTENSION_ptr = std::unique_ptr<X509_EXTENSION, decltype(&::X509_EXTENSION_free)>;

using X509_NAME_ptr = std::unique_ptr<X509_NAME, decltype(&::X509_NAME_free)>;
using X509_NAME_ENTRY_ptr = std::unique_ptr<X509_NAME_ENTRY, decltype(&::X509_NAME_ENTRY_free)>;

using X509_STORE_ptr = std::unique_ptr<X509_STORE, decltype(&::X509_STORE_free)>;
using X509_LOOKUP_ptr = std::unique_ptr<X509_LOOKUP, decltype(&::X509_LOOKUP_free)>;
using X509_STORE_CTX_ptr = std::unique_ptr<X509_STORE_CTX, decltype(&::X509_STORE_CTX_free)>;