Wireshark · Wireshark-dev: [Wireshark-dev] [patch] sparse fix to SSL decryption code

[Paolo Abeni <paolo.abeni@xxxxxxxx>]

hello,

the attached patch fix some glitches in the ssl decryption code, namely:

- the StringInfo allocator is allowed to return a NULL pointer if the
requested data length is 0

- the cipher_suites table is expanded and many wrong values are now
fixed

- some duplicate code about ssl session state checking is unified into
the proper location

- the ssl session structure is now properly initialize (a couple of
fields where manged in the wrong way)

- added some more verbose debug messages

I tested the new code against the pcap trace available on the web site
and it work properly.

The patch is against svn revision 25668.

cheers,

Paolo

 
 
 --
 Email.it, the professional e-mail, gratis per te: http://www.email.it/f
 
 Sponsor:
 VOGLIA DI VACANZE ? 
* I Costahotels  sono gli alberghi specializzati in tour enogastronomici nell'entroterra Romagnolo. Rimini, Riccione, Misano.
 Clicca qui: http://adv.email.it/cgi-bin/foclick.cgi?mid=8034&d=4-7

Index: epan/dissectors/packet-ssl-utils.c
===================================================================
--- epan/dissectors/packet-ssl-utils.c	(revision 25665)
+++ epan/dissectors/packet-ssl-utils.c	(working copy)
@@ -549,7 +549,9 @@
 ssl_data_alloc(StringInfo* str, guint len)
 {
     str->data = g_malloc(len);
-    if (!str->data)
+    /* the allocator can return a null pointer for a size equal to 0, and that
+     * must be allowed */
+    if (len > 0 && !str->data)
         return -1;
     str->data_len = len;
     return 0;
@@ -936,29 +938,30 @@
     {8,KEX_RSA,SIG_RSA,ENC_DES,8,64,40,DIG_SHA,20,1, SSL_CIPHER_MODE_CBC},
     {9,KEX_RSA,SIG_RSA,ENC_DES,8,64,64,DIG_SHA,20,0, SSL_CIPHER_MODE_CBC},
     {10,KEX_RSA,SIG_RSA,ENC_3DES,8,192,192,DIG_SHA,20,0, SSL_CIPHER_MODE_CBC},
-    {11,KEX_DH,SIG_DSS,ENC_DES,8,64,40,DIG_SHA,20,1, SSL_CIPHER_MODE_STREAM},
-    {12,KEX_DH,SIG_DSS,ENC_DES,8,64,64,DIG_SHA,20,0, SSL_CIPHER_MODE_STREAM},
-    {13,KEX_DH,SIG_DSS,ENC_3DES,8,192,192,DIG_SHA,20,0, SSL_CIPHER_MODE_STREAM},
-    {14,KEX_DH,SIG_RSA,ENC_DES,8,64,40,DIG_SHA,20,1, SSL_CIPHER_MODE_STREAM},
-    {15,KEX_DH,SIG_RSA,ENC_DES,8,64,64,DIG_SHA,20,0, SSL_CIPHER_MODE_STREAM},
-    {16,KEX_DH,SIG_RSA,ENC_3DES,8,192,192,DIG_SHA,20,0, SSL_CIPHER_MODE_STREAM},
-    {17,KEX_DH,SIG_DSS,ENC_DES,8,64,40,DIG_SHA,20,1, SSL_CIPHER_MODE_STREAM},
-    {18,KEX_DH,SIG_DSS,ENC_DES,8,64,64,DIG_SHA,20,0, SSL_CIPHER_MODE_STREAM},
-    {19,KEX_DH,SIG_DSS,ENC_3DES,8,192,192,DIG_SHA,20,0, SSL_CIPHER_MODE_STREAM},
-    {20,KEX_DH,SIG_RSA,ENC_DES,8,64,40,DIG_SHA,20,1, SSL_CIPHER_MODE_STREAM},
-    {21,KEX_DH,SIG_RSA,ENC_DES,8,64,64,DIG_SHA,20,0, SSL_CIPHER_MODE_STREAM},
-    {22,KEX_DH,SIG_RSA,ENC_3DES,8,192,192,DIG_SHA,20,0, SSL_CIPHER_MODE_STREAM},
+    {11,KEX_DH,SIG_DSS,ENC_DES,8,64,40,DIG_SHA,20,1, SSL_CIPHER_MODE_CBC},
+    {12,KEX_DH,SIG_DSS,ENC_DES,8,64,64,DIG_SHA,20,0, SSL_CIPHER_MODE_CBC},
+    {13,KEX_DH,SIG_DSS,ENC_3DES,8,192,192,DIG_SHA,20,0, SSL_CIPHER_MODE_CBC},
+    {14,KEX_DH,SIG_RSA,ENC_DES,8,64,40,DIG_SHA,20,1, SSL_CIPHER_MODE_CBC},
+    {15,KEX_DH,SIG_RSA,ENC_DES,8,64,64,DIG_SHA,20,0, SSL_CIPHER_MODE_CBC},
+    {16,KEX_DH,SIG_RSA,ENC_3DES,8,192,192,DIG_SHA,20,0, SSL_CIPHER_MODE_CBC},
+    {17,KEX_DH,SIG_DSS,ENC_DES,8,64,40,DIG_SHA,20,1, SSL_CIPHER_MODE_CBC},
+    {18,KEX_DH,SIG_DSS,ENC_DES,8,64,64,DIG_SHA,20,0, SSL_CIPHER_MODE_CBC},
+    {19,KEX_DH,SIG_DSS,ENC_3DES,8,192,192,DIG_SHA,20,0, SSL_CIPHER_MODE_CBC},
+    {20,KEX_DH,SIG_RSA,ENC_DES,8,64,40,DIG_SHA,20,1, SSL_CIPHER_MODE_CBC},
+    {21,KEX_DH,SIG_RSA,ENC_DES,8,64,64,DIG_SHA,20,0, SSL_CIPHER_MODE_CBC},
+    {22,KEX_DH,SIG_RSA,ENC_3DES,8,192,192,DIG_SHA,20,0, SSL_CIPHER_MODE_CBC},
     {23,KEX_DH,SIG_NONE,ENC_RC4,1,128,40,DIG_MD5,16,1, SSL_CIPHER_MODE_STREAM},
     {24,KEX_DH,SIG_NONE,ENC_RC4,1,128,128,DIG_MD5,16,0, SSL_CIPHER_MODE_STREAM},
-    {25,KEX_DH,SIG_NONE,ENC_DES,8,64,40,DIG_MD5,16,1, SSL_CIPHER_MODE_STREAM},
-    {26,KEX_DH,SIG_NONE,ENC_DES,8,64,64,DIG_MD5,16,0, SSL_CIPHER_MODE_STREAM},
-    {27,KEX_DH,SIG_NONE,ENC_3DES,8,192,192,DIG_MD5,16,0, SSL_CIPHER_MODE_STREAM},
+    {25,KEX_DH,SIG_NONE,ENC_DES,8,64,40,DIG_MD5,16,1, SSL_CIPHER_MODE_CBC},
+    {26,KEX_DH,SIG_NONE,ENC_DES,8,64,64,DIG_MD5,16,0, SSL_CIPHER_MODE_CBC},
+    {27,KEX_DH,SIG_NONE,ENC_3DES,8,192,192,DIG_MD5,16,0, SSL_CIPHER_MODE_CBC},
     {47,KEX_RSA,SIG_RSA,ENC_AES,16,128,128,DIG_SHA,20,0, SSL_CIPHER_MODE_CBC},
+    {51,KEX_DH, SIG_RSA,ENC_AES,16,128,128,DIG_SHA,20,0, SSL_CIPHER_MODE_CBC}, 
     {53,KEX_RSA,SIG_RSA,ENC_AES256,16,256,256,DIG_SHA,20,0, SSL_CIPHER_MODE_CBC},
     {96,KEX_RSA,SIG_RSA,ENC_RC4,1,128,56,DIG_MD5,16,1, SSL_CIPHER_MODE_STREAM},
     {97,KEX_RSA,SIG_RSA,ENC_RC2,1,128,56,DIG_MD5,16,1, SSL_CIPHER_MODE_STREAM},
     {98,KEX_RSA,SIG_RSA,ENC_DES,8,64,64,DIG_SHA,20,1, SSL_CIPHER_MODE_STREAM},
-    {99,KEX_DH,SIG_DSS,ENC_DES,8,64,64,DIG_SHA,16,1, SSL_CIPHER_MODE_STREAM},
+    {99,KEX_DH,SIG_DSS,ENC_DES,8,64,64,DIG_SHA,16,1, SSL_CIPHER_MODE_CBC},
     {100,KEX_RSA,SIG_RSA,ENC_RC4,1,128,56,DIG_SHA,20,1, SSL_CIPHER_MODE_STREAM},
     {101,KEX_DH,SIG_DSS,ENC_RC4,1,128,56,DIG_SHA,20,1, SSL_CIPHER_MODE_STREAM},
     {102,KEX_DH,SIG_DSS,ENC_RC4,1,128,128,DIG_SHA,20,0, SSL_CIPHER_MODE_STREAM},
@@ -1039,12 +1042,19 @@
     usage_len = strlen(usage);
 
     /* initalize buffer for sha, md5 random seed*/
-    if (ssl_data_alloc(&sha_out, MAX(out->data_len,20)) < 0)
+    if (ssl_data_alloc(&sha_out, MAX(out->data_len,20)) < 0) {
+        ssl_debug_printf("tls_prf: can't allocate sha out\n");
         return -1;
-    if (ssl_data_alloc(&md5_out, MAX(out->data_len,16)) < 0)
+    }
+    if (ssl_data_alloc(&md5_out, MAX(out->data_len,16)) < 0) {
+        ssl_debug_printf("tls_prf: can't allocate md5 out\n");
         goto free_sha;
-    if (ssl_data_alloc(&seed, usage_len+rnd1->data_len+rnd2->data_len) < 0)
+    }
+    if (ssl_data_alloc(&seed, usage_len+rnd1->data_len+rnd2->data_len) < 0) {
+        ssl_debug_printf("tls_prf: can't allocate rnd %d\n", 
+                         usage_len+rnd1->data_len+rnd2->data_len);
         goto free_md5;
+    }
 
     ptr=seed.data;
     memcpy(ptr,usage,usage_len); ptr+=usage_len;
@@ -1053,10 +1063,14 @@
 
     /* initalize buffer for client/server seeds*/
     s_l=secret->data_len/2 + secret->data_len%2;
-    if (ssl_data_alloc(&s1, s_l) < 0)
+    if (ssl_data_alloc(&s1, s_l) < 0) {
+        ssl_debug_printf("tls_prf: can't allocate secret %d\n", s_l);
         goto free_seed;
-    if (ssl_data_alloc(&s2, s_l) < 0)
+    }
+    if (ssl_data_alloc(&s2, s_l) < 0) {
+        ssl_debug_printf("tls_prf: can't allocate secret(2) %d\n", s_l);
         goto free_s1;
+    }
 
     memcpy(s1.data,secret->data,s_l);
     memcpy(s2.data,secret->data + (secret->data_len - s_l),s_l);
@@ -1271,6 +1285,16 @@
     gint needed;
     guint8 *ptr,*c_wk,*s_wk,*c_mk,*s_mk,*c_iv = _iv_c,*s_iv = _iv_s;
 
+    /* check for enough info to proced */
+    guint need_all = SSL_CIPHER|SSL_CLIENT_RANDOM|SSL_SERVER_RANDOM|SSL_VERSION;
+    guint need_any = SSL_MASTER_SECRET | SSL_PRE_MASTER_SECRET;
+    if (((ssl_session->state & need_all) != need_all) || ((ssl_session->state & need_any) == 0)) {
+        ssl_debug_printf("ssl_generate_keyring_material not enough data to generate key "
+                         "(0x%02X required 0x%02X or 0x%02X)\n", ssl_session->state, 
+                         need_all|SSL_MASTER_SECRET, need_all|SSL_PRE_MASTER_SECRET);
+        return -1;
+    }
+
     /* if master_key is not yet generate, create it now*/
     if (!(ssl_session->state & SSL_MASTER_SECRET)) {
         ssl_debug_printf("ssl_generate_keyring_material:PRF(pre_master_secret)\n");
@@ -1281,6 +1305,10 @@
             return -1;
         }
         ssl_print_string("master secret",&ssl_session->master_secret);
+
+        /* the pre-master secret has been 'consumend' so we must clear it now */
+        ssl_session->state &= ~SSL_PRE_MASTER_SECRET;
+        ssl_session->state |= SSL_MASTER_SECRET;
     }
 
     /* Compute the key block. First figure out how much data we need*/
@@ -1292,7 +1320,7 @@
     key_block.data_len = needed;
     key_block.data = g_malloc(needed);
     if (!key_block.data) {
-        ssl_debug_printf("ssl_generate_keyring_material can't allacate key_block\n");
+        ssl_debug_printf("ssl_generate_keyring_material can't allocate key_block (len %d)\n", needed);
         return -1;
     }
     ssl_debug_printf("ssl_generate_keyring_material sess key generation\n");
@@ -1467,6 +1495,7 @@
     ssl_debug_printf("ssl_generate_keyring_material: client seq %d, server seq %d\n",
         ssl_session->client_new->seq, ssl_session->server_new->seq);
     g_free(key_block.data);
+    ssl->state |= SSL_HAVE_SESSION_KEY;
     return 0;
 
 fail:
@@ -1520,6 +1549,7 @@
        This force keying material regeneration in
        case we're renegotiating */
     ssl_session->state &= ~(SSL_MASTER_SECRET|SSL_HAVE_SESSION_KEY);
+    ssl_session->state |= SSL_PRE_MASTER_SECRET;
     return 0;
 }
 
@@ -2263,9 +2293,9 @@
     ssl_session->client_random.data = ssl_session->_client_random;
     ssl_session->server_random.data = ssl_session->_server_random;
     ssl_session->master_secret.data_len = 48;
-    ssl_session->server_data_for_iv.data = 0;
+    ssl_session->server_data_for_iv.data_len = 0;
     ssl_session->server_data_for_iv.data = ssl_session->_server_data_for_iv;
-    ssl_session->client_data_for_iv.data = 0;
+    ssl_session->client_data_for_iv.data_len = 0;
     ssl_session->client_data_for_iv.data = ssl_session->_client_data_for_iv;
     ssl_session->app_data_segment.data=NULL;
     ssl_session->app_data_segment.data_len=0;
Index: epan/dissectors/packet-ssl-utils.h
===================================================================
--- epan/dissectors/packet-ssl-utils.h	(revision 25665)
+++ epan/dissectors/packet-ssl-utils.h	(working copy)
@@ -195,6 +195,7 @@
 #define SSL_HAVE_SESSION_KEY    8
 #define SSL_VERSION             0x10
 #define SSL_MASTER_SECRET       0x20
+#define SSL_PRE_MASTER_SECRET   0x40
 
 #define SSL_CIPHER_MODE_STREAM  0
 #define SSL_CIPHER_MODE_CBC     1
@@ -287,6 +288,7 @@
     StringInfo server_random;
     StringInfo client_random;
     StringInfo master_secret;
+    /* the data store for this StringInfo must be allocated explicitly with a capture lifetime scope */
     StringInfo pre_master_secret;
     guchar _server_data_for_iv[24];
     StringInfo server_data_for_iv;
Index: epan/dissectors/packet-ssl.c
===================================================================
--- epan/dissectors/packet-ssl.c	(revision 25665)
+++ epan/dissectors/packet-ssl.c	(working copy)
@@ -1872,16 +1872,6 @@
                     if (!ssl)
                         break;
 
-                    /* check for required session data */
-                    ssl_debug_printf("dissect_ssl3_handshake found SSL_HND_CLIENT_KEY_EXCHG state 0x%X\n",
-                        ssl->state);
-                    if ((ssl->state & (SSL_CIPHER|SSL_CLIENT_RANDOM|SSL_SERVER_RANDOM|SSL_VERSION)) !=
-                            (SSL_CIPHER|SSL_CLIENT_RANDOM|SSL_SERVER_RANDOM|SSL_VERSION)) {
-                        ssl_debug_printf("dissect_ssl3_handshake not enough data to generate key (required 0x%02X)\n",
-                            (SSL_CIPHER|SSL_CLIENT_RANDOM|SSL_SERVER_RANDOM|SSL_VERSION));
-                        break;
-                    }
-
                     /* get encrypted data, on tls1 we have to skip two bytes
                      * (it's the encrypted len and should be equal to record len - 2)
                      */
@@ -1916,7 +1906,7 @@
                         ssl_debug_printf("dissect_ssl3_handshake can't generate keyring material\n");
                         break;
                     }
-                    ssl->state |= SSL_HAVE_SESSION_KEY;
+
                     ssl_save_session(ssl, ssl_session_hash);
                     ssl_debug_printf("dissect_ssl3_handshake session keys succesfully generated\n");
                 }
@@ -2246,20 +2236,11 @@
 
             /* if we have restored a session now we can have enought material
              * to build session key, check it out*/
-            if ((ssl->state &
-                    (SSL_CIPHER|SSL_CLIENT_RANDOM|SSL_SERVER_RANDOM|SSL_VERSION|SSL_MASTER_SECRET)) !=
-                    (SSL_CIPHER|SSL_CLIENT_RANDOM|SSL_SERVER_RANDOM|SSL_VERSION|SSL_MASTER_SECRET)) {
-                ssl_debug_printf("dissect_ssl3_hnd_srv_hello not enough data to generate key (required 0x%02X)\n",
-                    (SSL_CIPHER|SSL_CLIENT_RANDOM|SSL_SERVER_RANDOM|SSL_VERSION|SSL_MASTER_SECRET));
-                goto no_cipher;
-            }
-
             ssl_debug_printf("dissect_ssl3_hnd_srv_hello trying to generate keys\n");
             if (ssl_generate_keyring_material(ssl)<0) {
                 ssl_debug_printf("dissect_ssl3_hnd_srv_hello can't generate keyring material\n");
                 goto no_cipher;
             }
-            ssl->state |= SSL_HAVE_SESSION_KEY;
         }
 no_cipher:
 
@@ -2908,7 +2889,7 @@
             {
                 tvb_memcpy(tvb,ssl->session_id.data, offset, session_id_length);
                 ssl->session_id.data_len = session_id_length;
-                ssl->state &= ~(SSL_HAVE_SESSION_KEY|SSL_MASTER_SECRET|
+                ssl->state &= ~(SSL_HAVE_SESSION_KEY|SSL_MASTER_SECRET|SSL_PRE_MASTER_SECRET
                         SSL_CIPHER|SSL_SERVER_RANDOM);
             }
             offset += session_id_length;
@@ -3575,20 +3556,11 @@
     ssl_debug_printf("ssl_set_master_secret set MASTER SECRET -> state 0x%02X\n", ssl->state);
   }
 
-  if ((ssl->state &
-          (SSL_CIPHER|SSL_CLIENT_RANDOM|SSL_SERVER_RANDOM|SSL_VERSION|SSL_MASTER_SECRET)) !=
-          (SSL_CIPHER|SSL_CLIENT_RANDOM|SSL_SERVER_RANDOM|SSL_VERSION|SSL_MASTER_SECRET)) {
-      ssl_debug_printf("ssl_set_master_secret not enough data to generate key (has 0x%02X but required 0x%02X)\n",
-          ssl->state, (SSL_CIPHER|SSL_CLIENT_RANDOM|SSL_SERVER_RANDOM|SSL_VERSION|SSL_MASTER_SECRET));
-      return;
-  }
-
   ssl_debug_printf("ssl_set_master_secret trying to generate keys\n");
   if (ssl_generate_keyring_material(ssl)<0) {
       ssl_debug_printf("ssl_set_master_secret can't generate keyring material\n");
       return;
   }
-  ssl->state |= SSL_HAVE_SESSION_KEY;
 
   /* chenge ciphers immediately */
   ssl_change_cipher(ssl, TRUE);