Re: Memory leaks are observed for libgnutls in multi-thread mode

[Nikos Mavrogiannopoulos]

tangtong wrote:
> Hi,Nikos
> After applying the patch, I get the following error during handshake:
> error number:-18 dec:An error was encountered at the TLS Finished packet 
> calculation.
> 
> My lib is based on git 2.9.4.

There is some issue with TLS1.2 hashes and handshake. Anyway the
attached patch should fix the issue you encounter.

The issue with TLS1.2 is that when a client that supports TLS1.2 tries
to connect to a server that doesn't support tls1.2 he will have SHA256
initiated instead of SHA1. I made a quick and dirty fix for it.

regards,
Nikos

diff --git a/lib/gnutls_cipher.c b/lib/gnutls_cipher.c
index 0d8c5e1..5f2ed62 100644
--- a/lib/gnutls_cipher.c
+++ b/lib/gnutls_cipher.c
@@ -418,6 +418,10 @@ _gnutls_compressed2ciphertext (gnutls_session_t session,
   return length;
 }
 
+#define DEINIT_MAC(td, ver, algo) \
+          if (algo != GNUTLS_MAC_NULL) \
+            mac_deinit (&td, NULL, ver);
+
 /* Deciphers the ciphertext packet, and puts the result to compress_data, of 
compress_size.
  * Returns the actual compressed packet size.
  */
@@ -473,6 +477,9 @@ _gnutls_ciphertext2compressed (gnutls_session_t session,
                                   ciphertext.size)) < 0)
        {
          gnutls_assert ();
+         DEINIT_MAC(td, ver, session->security_parameters.write_mac_algorithm);
+
+
          return ret;
        }
 
@@ -483,6 +490,7 @@ _gnutls_ciphertext2compressed (gnutls_session_t session,
       if ((ciphertext.size < blocksize) || (ciphertext.size % blocksize != 0))
        {
          gnutls_assert ();
+         DEINIT_MAC(td, ver, session->security_parameters.write_mac_algorithm);
          return GNUTLS_E_DECRYPTION_FAILED;
        }
 
@@ -492,6 +500,7 @@ _gnutls_ciphertext2compressed (gnutls_session_t session,
                                   ciphertext.size)) < 0)
        {
          gnutls_assert ();
+         DEINIT_MAC(td, ver, session->security_parameters.write_mac_algorithm);
          return ret;
        }
 
@@ -505,6 +514,7 @@ _gnutls_ciphertext2compressed (gnutls_session_t session,
          if (ciphertext.size == 0)
            {
              gnutls_assert ();
+              DEINIT_MAC(td, ver, 
session->security_parameters.write_mac_algorithm);
              return GNUTLS_E_DECRYPTION_FAILED;
            }
        }
@@ -537,6 +547,7 @@ _gnutls_ciphertext2compressed (gnutls_session_t session,
       break;
     default:
       gnutls_assert ();
+      DEINIT_MAC(td, ver, session->security_parameters.write_mac_algorithm);
       return GNUTLS_E_INTERNAL_ERROR;
     }
 
diff --git a/lib/gnutls_handshake.c b/lib/gnutls_handshake.c
index 83dc54e..742330d 100644
--- a/lib/gnutls_handshake.c
+++ b/lib/gnutls_handshake.c
@@ -69,11 +69,12 @@ int _gnutls_server_select_comp_method (gnutls_session_t 
session,
 
 /* Clears the handshake hash buffers and handles.
  */
-static void
+void
 _gnutls_handshake_hash_buffers_clear (gnutls_session_t session)
 {
   _gnutls_hash_deinit (&session->internals.handshake_mac_handle_md5, NULL);
   _gnutls_hash_deinit (&session->internals.handshake_mac_handle_sha, NULL);
+  _gnutls_hash_deinit (&session->internals.handshake_mac_handle, NULL);
   session->internals.handshake_mac_handle_init = 0;
   _gnutls_handshake_buffer_clear (session);
 }
@@ -198,7 +199,7 @@ static int
 _gnutls_finished (gnutls_session_t session, int type, void *ret)
 {
   const int siz = TLS_MSG_LEN;
-  opaque concat[36];
+  opaque concat[MAX_HASH_SIZE+16/*MD5*/];
   size_t len;
   const char *mesg;
   digest_hd_st td_md5;
@@ -216,29 +217,35 @@ _gnutls_finished (gnutls_session_t session, int type, 
void *ret)
          gnutls_assert ();
          return rc;
        }
-    }
 
-  rc =
-    _gnutls_hash_copy (&td_sha, &session->internals.handshake_mac_handle_sha);
-  if (rc < 0)
-    {
-      gnutls_assert ();
-      _gnutls_hash_deinit (&td_md5, NULL);
-      return rc;
-    }
+      rc =
+        _gnutls_hash_copy (&td_sha, 
&session->internals.handshake_mac_handle_sha);
+      if (rc < 0)
+        {
+          gnutls_assert ();
+          _gnutls_hash_deinit (&td_md5, NULL);
+          return rc;
+        }
 
-  if (!_gnutls_version_has_selectable_prf(ver))
-    {
-      _gnutls_hash_deinit (&td_md5, concat);
-      _gnutls_hash_deinit (&td_sha, &concat[16]);
-      len = 20 + 16;
+        _gnutls_hash_deinit (&td_md5, concat);
+        _gnutls_hash_deinit (&td_sha, &concat[16]);
+        len = 20 + 16;
     }
   else
     {
-      _gnutls_hash_deinit (&td_sha, concat);
-      len = _gnutls_hash_get_algo_len (td_sha.algorithm);
+      rc =
+        _gnutls_hash_copy (&td_sha, &session->internals.handshake_mac_handle);
+      if (rc < 0)
+        {
+          gnutls_assert ();
+          return rc;
+        }
+
+        _gnutls_hash_deinit (&td_sha, concat);
+        len = _gnutls_hash_get_algo_len (td_sha.algorithm);
     }
 
+
   if (type == GNUTLS_SERVER)
     {
       mesg = SERVER_MSG;
@@ -534,8 +541,12 @@ _gnutls_handshake_hash_pending (gnutls_session_t session)
 
   if (siz > 0)
     {
+      /* FIXME: we do not need MD5 and SHA if we are 
_gnutls_version_has_selectable_prf(ver) and
+       * this is _not_ the first client hello message */
       _gnutls_hash (&session->internals.handshake_mac_handle_sha, data, siz);
       _gnutls_hash (&session->internals.handshake_mac_handle_md5, data, siz);
+      if (session->internals.handshake_mac_handle.algorithm != 0)
+        _gnutls_hash (&session->internals.handshake_mac_handle, data, siz);
     }
 
   _gnutls_handshake_buffer_empty (session);
@@ -923,10 +934,15 @@ _gnutls_handshake_hash_add_sent (gnutls_session_t session,
 
   if (type != GNUTLS_HANDSHAKE_HELLO_REQUEST)
     {
+      /* FIXME: we do not need MD5 and SHA if we are 
_gnutls_version_has_selectable_prf(ver) and
+       * this is _not_ the first client hello message */
       _gnutls_hash (&session->internals.handshake_mac_handle_sha, dataptr,
                    datalen);
       _gnutls_hash (&session->internals.handshake_mac_handle_md5, dataptr,
                    datalen);
+      if (session->internals.handshake_mac_handle.algorithm != 0)
+          _gnutls_hash (&session->internals.handshake_mac_handle, dataptr,
+                   datalen);
     }
 
   return 0;
@@ -2171,8 +2187,7 @@ inline static int
 _gnutls_handshake_hash_init (gnutls_session_t session)
 {
   gnutls_protocol_t ver = gnutls_protocol_get_version (session);
-  gnutls_digest_algorithm_t hash_algo = GNUTLS_MAC_SHA1;
-
+  
   if (session->internals.handshake_mac_handle_init == 0)
     {
       int ret =
@@ -2185,21 +2200,35 @@ _gnutls_handshake_hash_init (gnutls_session_t session)
          return ret;
        }
 
-      /* The algorithm to compute hash over handshake messages must be
-        same as the one used as the basis for PRF.  By now we use
-        SHA256. */
-      if (_gnutls_version_has_selectable_prf (ver))
-       hash_algo = GNUTLS_MAC_SHA256;
-
       ret =
        _gnutls_hash_init (&session->internals.handshake_mac_handle_sha,
-                          hash_algo);
+                          GNUTLS_MAC_SHA1);
       if (ret < 0)
        {
          gnutls_assert ();
+         _gnutls_hash_deinit (&session->internals.handshake_mac_handle_md5, 
NULL);
          return GNUTLS_E_MEMORY_ERROR;
        }
 
+      /* The algorithm to compute hash over handshake messages must be
+        same as the one used as the basis for PRF.  By now we use
+        SHA256. */
+      if (_gnutls_version_has_selectable_prf (ver))
+        {
+          gnutls_digest_algorithm_t hash_algo = GNUTLS_MAC_SHA256;
+
+          ret =
+           _gnutls_hash_init (&session->internals.handshake_mac_handle,
+                          hash_algo);
+           if (ret < 0)
+            {
+              gnutls_assert ();
+              _gnutls_hash_deinit 
(&session->internals.handshake_mac_handle_md5, NULL);
+              _gnutls_hash_deinit 
(&session->internals.handshake_mac_handle_sha, NULL);
+              return GNUTLS_E_MEMORY_ERROR;
+            }
+        }
+
       session->internals.handshake_mac_handle_init = 1;
     }
 
diff --git a/lib/gnutls_handshake.h b/lib/gnutls_handshake.h
index f1b1bd6..2f484f5 100644
--- a/lib/gnutls_handshake.h
+++ b/lib/gnutls_handshake.h
@@ -55,6 +55,8 @@ int _gnutls_negotiate_version (gnutls_session_t session,
 int _gnutls_user_hello_func (gnutls_session_t session,
                             gnutls_protocol_t adv_version);
 
+void _gnutls_handshake_hash_buffers_clear (gnutls_session_t session);
+
 #define STATE session->internals.handshake_state
 /* This returns true if we have got there
  * before (and not finished due to an interrupt).
diff --git a/lib/gnutls_hash_int.c b/lib/gnutls_hash_int.c
index e55ae54..5e5b2aa 100644
--- a/lib/gnutls_hash_int.c
+++ b/lib/gnutls_hash_int.c
@@ -77,6 +77,7 @@ _gnutls_hash_init (digest_hd_st * dig, 
gnutls_digest_algorithm_t algorithm)
          gnutls_assert ();
          return GNUTLS_E_HASH_FAILED;
        }
+      dig->active = 1;
       return 0;
     }
 
@@ -89,6 +90,7 @@ _gnutls_hash_init (digest_hd_st * dig, 
gnutls_digest_algorithm_t algorithm)
       return result;
     }
 
+  dig->active = 1;
   return 0;
 }
 
@@ -119,10 +121,10 @@ _gnutls_hash_copy (digest_hd_st * dst, digest_hd_st * src)
 {
   int result;
 
+  memset(dst, 0, sizeof(*dst));
   dst->algorithm = src->algorithm;
-  dst->key = NULL;             /* it's a hash anyway */
-  dst->keysize = 0;
   dst->registered = src->registered;
+  dst->active = 1;
 
   if (src->registered)
     {
@@ -165,7 +167,14 @@ _gnutls_hash_output (digest_hd_st * handle, void *digest)
 void
 _gnutls_hash_deinit (digest_hd_st * handle, void *digest)
 {
-  _gnutls_hash_output (handle, digest);
+  if (handle->active != 1) {
+    return;
+  }
+
+  if (digest != NULL)
+    _gnutls_hash_output (handle, digest);
+
+  handle->active = 0;
 
   if (handle->registered && handle->hd.rh.ctx != NULL)
     {
@@ -269,6 +278,7 @@ _gnutls_hmac_init (digest_hd_st * dig, 
gnutls_mac_algorithm_t algorithm,
          return GNUTLS_E_HASH_FAILED;
        }
 
+      dig->active = 1;
       return 0;
     }
 
@@ -283,6 +293,7 @@ _gnutls_hmac_init (digest_hd_st * dig, 
gnutls_mac_algorithm_t algorithm,
 
   _gnutls_mac_ops.setkey (dig->hd.gc, key, keylen);
 
+  dig->active = 1;
   return 0;
 }
 
@@ -323,8 +334,14 @@ _gnutls_hmac_output (digest_hd_st * handle, void *digest)
 void
 _gnutls_hmac_deinit (digest_hd_st * handle, void *digest)
 {
-  _gnutls_hmac_output (handle, digest);
+  if (handle->active != 1) {
+    return;
+  }
+
+  if (digest)
+    _gnutls_hmac_output (handle, digest);
 
+  handle->active = 0;
   if (handle->registered && handle->hd.rh.ctx != NULL)
     {
       handle->hd.rh.cc->deinit (handle->hd.rh.ctx);
@@ -398,6 +415,7 @@ _gnutls_mac_deinit_ssl3 (digest_hd_st * handle, void 
*digest)
   if (padsize == 0)
     {
       gnutls_assert ();
+      _gnutls_hash_deinit (handle, NULL);
       return;
     }
 
@@ -407,6 +425,7 @@ _gnutls_mac_deinit_ssl3 (digest_hd_st * handle, void 
*digest)
   if (rc < 0)
     {
       gnutls_assert ();
+      _gnutls_hash_deinit (handle, NULL);
       return;
     }
 
diff --git a/lib/gnutls_hash_int.h b/lib/gnutls_hash_int.h
index 8017d12..d915af5 100644
--- a/lib/gnutls_hash_int.h
+++ b/lib/gnutls_hash_int.h
@@ -52,6 +52,7 @@ typedef struct
   gnutls_mac_algorithm_t algorithm;
   const void *key;
   int keysize;
+  int active;
 } digest_hd_st;
 
 int _gnutls_hmac_init (digest_hd_st*, gnutls_mac_algorithm_t algorithm,
diff --git a/lib/gnutls_int.h b/lib/gnutls_int.h
index 100ad37..fe5301c 100644
--- a/lib/gnutls_int.h
+++ b/lib/gnutls_int.h
@@ -475,6 +475,7 @@ typedef struct
                                         * message */
   digest_hd_st handshake_mac_handle_sha;       /* hash of the handshake 
messages */
   digest_hd_st handshake_mac_handle_md5;       /* hash of the handshake 
messages */
+  digest_hd_st handshake_mac_handle;   /* hash of the handshake messages for 
TLS 1.2+ */
   int handshake_mac_handle_init; /* 1 when the previous two were initialized */
 
   gnutls_buffer handshake_data_buffer; /* this is a buffer that holds the 
current handshake message */
diff --git a/lib/gnutls_state.c b/lib/gnutls_state.c
index fede2a0..cd08f44 100644
--- a/lib/gnutls_state.c
+++ b/lib/gnutls_state.c
@@ -201,6 +201,8 @@ deinit_internal_params (gnutls_session_t session)
   if (session->internals.params.free_rsa_params)
     gnutls_rsa_params_deinit (session->internals.params.rsa_params);
 
+  _gnutls_handshake_hash_buffers_clear(session);
+
   memset (&session->internals.params, 0, sizeof (session->internals.params));
 }
 
@@ -208,8 +210,8 @@ deinit_internal_params (gnutls_session_t session)
  * structure within the session, which depend on the current handshake.
  * This is used to allow further handshakes.
  */
-void
-_gnutls_handshake_internal_state_clear (gnutls_session_t session)
+static void
+_gnutls_handshake_internal_state_init (gnutls_session_t session)
 {
   session->internals.extensions_sent_size = 0;
 
@@ -231,6 +233,13 @@ _gnutls_handshake_internal_state_clear (gnutls_session_t 
session)
   session->internals.last_handshake_out = -1;
 
   session->internals.resumable = RESUME_TRUE;
+}
+
+void
+_gnutls_handshake_internal_state_clear (gnutls_session_t session)
+{
+  _gnutls_handshake_internal_state_init(session);
+
   _gnutls_free_datum (&session->internals.recv_buffer);
 
   deinit_internal_params (session);
@@ -336,7 +345,7 @@ gnutls_init (gnutls_session_t * session, 
gnutls_connection_end_t con_end)
    * as NULL or 0. This is why calloc is used.
    */
 
-  _gnutls_handshake_internal_state_clear (*session);
+  _gnutls_handshake_internal_state_init (*session);
 
   return 0;
 }