[Paolo Abeni <paolo.abeni@xxxxxxxx>]

hi all,

authesserre samuel <sauthess@xxxxxxxxx> kindly pointed out an issue with
session renegotiation in the current ssl decryption code. 

Encrypted handshake message are decrypted, but the dissector try to
interpret the encrypted code. Renegotiation messages are therefore
ignored. The attached pcap trace and key can be used to trigger the
issue.

The attached patch fix the problem storing the decrypted version of
encrypted handshake message and dissecting it when available. The patch
also fix bad issue with des cipher (alike the issue fixed in my
previous post)

ciao,

Paolo

 
 
 --
 Email.it, the professional e-mail, gratis per te: http://www.email.it/f
 
 Sponsor:
 Se stai pensando di cambiare o acquistare la tua auto, la soluzione � richiedere un Credito Auto Findomestic, facile e senza anticipi
* 
 Clicca qui: http://adv.email.it/cgi-bin/foclick.cgi?mid=4968&d=27-4

Attachment: capture.pcap
Description: Binary data

-----BEGIN RSA PRIVATE KEY-----
MIICWwIBAAKBgQCkblMUCt4s42BVmvJCpq9HEi8Xzvq63E5jVjS5unNLeEQ9xmxp
pCWzYQKdCQQ/cj3YJ9OwWkV3tzbkJiPMEriu3qe2OoI8fCRZCviWQ4ujKTY/kX9d
xyOUKX8Kzgq9jZsvGReq1Y7sZqI36z9XUzzyqrt5GUuQfqejmf6ETInwPQIDAQAB
AoGAedqEWKsBIPTTtDziYYBTDnEsUxGA/685rCX7ZtQEkx4qPDlqqBMMGVW/8Q34
hugrap+BIgSTzHcLB6I4DwiksUpR08x0hf0oxqqjMo0KykhZDfUUfxR85JHUrFZM
GznurVhfSBXX4Il9Tgc/RPzD32FZ6gaz9sFumJh0LKKadeECQQDWOfP6+nIAvmyH
aRINErBSlK+xv2mZ4jEKvROIQmrpyNyoOStYLG/DRPlEzAIA6oQnowGgS6gwaibg
g7yVTgBpAkEAxH6dcwhIDRTILvtUdKSWB6vdhtXFGdebaU4cuUOW2kWwPpyIj4XN
D+rezwfptmeOr34DCA/QKCI/BWkbFDG2tQJAVAH971nvAuOp46AMeBvwETJFg8qw
Oqw81x02X6TMEEm4Xi+tE7K5UTXnGld2Ia3VjUWbCaUhm3rFLB39Af/IoQJAUn/G
o5GKjtN26SLk5sRjqXzjWcVPJ/Z6bdA6Bx71q1cvFFqsi3XmDxTRz6LG4arBIbWK
mEvrXa5jP2ZN1EC7MQJAYTfwPZ8/4x/USmA4vx9FKdADdDoZnA9ZSwezWaqa44My
bJ0SY/WmNU+Z4ldVIkcevwwwcxqLF399hjrXWhzlBQ==
-----END RSA PRIVATE KEY-----



Index: gtk/ssl-dlg.c
===================================================================
--- gtk/ssl-dlg.c	(revision 18016)
+++ gtk/ssl-dlg.c	(working copy)
@@ -140,10 +140,10 @@
     follow_info_t* follow_info = tapdata;
     SslDecryptedRecord* rec;
     int proto_ssl = (int) ssl;
-    StringInfo* data = p_get_proto_data(pinfo->fd, proto_ssl);
+    SslPacketInfo* pi = p_get_proto_data(pinfo->fd, proto_ssl);
 
     /* skip packet without decrypted data payload*/    
-    if (!data)
+    if (!pi || !pi->app_data.data)
         return 0;
     
     /* compute packet direction */
@@ -161,10 +161,10 @@
         rec->is_server = 1;
 
     /* update stream counter */
-    follow_info->bytes_written[rec->is_server] += data->data_len;
+    follow_info->bytes_written[rec->is_server] += pi->app_data.data_len;
     
     /* extract decrypted data and queue it locally */    
-    rec->data = data;
+    rec->data = &pi->app_data;
     follow_info->ssl_decrypted_data = g_list_append(
         follow_info->ssl_decrypted_data,rec);
 
Index: epan/dissectors/packet-ssl-utils.c
===================================================================
--- epan/dissectors/packet-ssl-utils.c	(revision 18016)
+++ epan/dissectors/packet-ssl-utils.c	(working copy)
@@ -179,6 +179,13 @@
     return gcry_cipher_map_name(name);
 }
 
+static inline void
+ssl_cipher_cleanup(gcry_cipher_hd_t *cipher)
+{
+    gcry_cipher_close(*cipher);
+    *cipher = NULL;
+}
+
 /* private key abstraction layer */
 static inline int 
 ssl_get_key_len(SSL_PRIVATE_KEY* pk) {return gcry_pk_get_nbits (pk); }
@@ -355,8 +362,8 @@
     {5,KEX_RSA,SIG_RSA,ENC_RC4,1,128,128,DIG_SHA,20,0, SSL_CIPHER_MODE_STREAM},
     {6,KEX_RSA,SIG_RSA,ENC_RC2,8,128,40,DIG_SHA,20,1, SSL_CIPHER_MODE_STREAM},
     {7,KEX_RSA,SIG_RSA,ENC_IDEA,8,128,128,DIG_SHA,20,0, SSL_CIPHER_MODE_STREAM},
-    {8,KEX_RSA,SIG_RSA,ENC_DES,8,64,40,DIG_SHA,20,1, SSL_CIPHER_MODE_STREAM},
-    {9,KEX_RSA,SIG_RSA,ENC_DES,8,64,64,DIG_SHA,20,0, SSL_CIPHER_MODE_STREAM},
+    {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},
@@ -591,7 +598,7 @@
     }
     if (ciph == 0) {
         ssl_debug_printf("ssl_create_decoder can't find cipher %s\n", 
-						 ciphers[(cipher_suite->enc-0x30) > 7 ? 7 : (cipher_suite->enc-0x30)]);
+            ciphers[(cipher_suite->enc-0x30) > 7 ? 7 : (cipher_suite->enc-0x30)]);
         return -1;
     }
     
@@ -600,6 +607,10 @@
     dec->cipher_suite=cipher_suite;
     dec->mac_key.data = dec->_mac_key;
     ssl_data_set(&dec->mac_key, mk, cipher_suite->dig_len);
+    dec->seq = 0;
+    
+    if (dec->evp)
+        ssl_cipher_cleanup(&dec->evp);
 
     if (ssl_cipher_init(&dec->evp,ciph,sk,iv,cipher_suite->mode) < 0) {
         ssl_debug_printf("ssl_create_decoder: can't create cipher id:%d mode:%d\n",
@@ -812,7 +823,9 @@
         ssl_debug_printf("ssl_generate_keyring_material can't init client decoder\n");        
         goto fail;
     }
-        
+      
+    ssl_debug_printf("ssl_generate_keyring_material client seq %d server seq %d\n",
+        ssl_session->client.seq, ssl_session->server.seq);
     g_free(key_block.data);
     return 0;
     
@@ -853,7 +866,7 @@
     /* Remove the master secret if it was there.
        This force keying material regeneration in
        case we're renegotiating */
-    ssl_session->state &= ~SSL_MASTER_SECRET;
+    ssl_session->state &= ~(SSL_MASTER_SECRET|SSL_HAVE_SESSION_KEY);
     return 0;
 }
  
@@ -926,10 +939,7 @@
 
     /* get cipher used for digest comptuation */
     md=ssl_get_digest_by_name(digests[decoder->cipher_suite->dig-0x40]);
-    ssl_debug_printf("ssl3_check_mac digest%s md %d\n",
-        digests[decoder->cipher_suite->dig-0x40], md);
     ssl_md_init(&mc,md);
-    ssl_debug_printf("ssl3_check_mac memory digest %p\n",mc);
 
     /* do hash computation on data && padding */
     ssl_md_update(&mc,decoder->mac_key.data,decoder->mac_key.data_len);
@@ -1009,19 +1019,19 @@
         return -1;
     }
     mac=out+worklen;
-    /*ssl_print_data("Record data",out,*outl);*/
 
     /* Now check the MAC */
-    ssl_debug_printf("checking mac (len %d, version %X, ct %d)\n", worklen,ssl->version_netorder, ct);
+    ssl_debug_printf("checking mac (len %d, version %X, ct %d seq %d)\n", 
+        worklen, ssl->version_netorder, ct, decoder->seq);
     if(ssl->version_netorder==0x300){
         if(ssl3_check_mac(decoder,ct,out,worklen,mac) < 0) {
-            ssl_debug_printf("ssl_decrypt_record: mac falied\n");
+            ssl_debug_printf("ssl_decrypt_record: mac failed\n");
             return -1;
         }
     }
     else{
         if(tls_check_mac(decoder,ct,ssl->version_netorder,out,worklen,mac)< 0) {
-            ssl_debug_printf("ssl_decrypt_record: mac falied\n");
+            ssl_debug_printf("ssl_decrypt_record: mac failed\n");
             return -1;
         }
     }
Index: epan/dissectors/packet-ssl-utils.h
===================================================================
--- epan/dissectors/packet-ssl-utils.h	(revision 18016)
+++ epan/dissectors/packet-ssl-utils.h	(working copy)
@@ -113,11 +113,19 @@
 #define DIG_MD5         0x40
 #define DIG_SHA         0x41
 
-/*typedef struct _SslService {
-    address addr;
-    guint port;
-} SslService;*/
+struct tvbuff;
 
+typedef struct _SslRecordInfo {
+    struct tvbuff* tvb;
+    int id;
+    struct _SslRecordInfo* next;
+} SslRecordInfo;
+
+typedef struct {
+    StringInfo app_data;
+    SslRecordInfo* handshake_data; 
+} SslPacketInfo;
+
 typedef struct _SslDecryptSession {
     unsigned char _master_secret[48];
     unsigned char _session_id[256];
Index: epan/dissectors/packet-ssl.c
===================================================================
--- epan/dissectors/packet-ssl.c	(revision 18016)
+++ epan/dissectors/packet-ssl.c	(working copy)
@@ -231,6 +231,7 @@
 static GTree* ssl_associations = NULL;
 static dissector_handle_t ssl_handle = NULL;
 static StringInfo ssl_decrypted_data = {NULL, 0};
+static int ssl_decrypted_data_avail = 0;
 
 /* Hash Functions for ssl sessions table and private keys table*/
 static gint  
@@ -349,6 +350,44 @@
     return ret;
 }    
 
+/* add to packet data a newly allocated tvb with the specified real data*/
+static void
+ssl_add_record_info(packet_info *pinfo, unsigned char* data, int data_len, int record_id)
+{
+    unsigned char* real_data = se_alloc(data_len);
+    SslRecordInfo* rec = se_alloc(sizeof(SslRecordInfo));
+    SslPacketInfo* pi = p_get_proto_data(pinfo->fd, proto_ssl);
+    if (!pi)
+    {
+        pi = se_alloc0(sizeof(SslPacketInfo));
+        p_add_proto_data(pinfo->fd, proto_ssl,pi);
+    }
+    
+    rec->id = record_id;
+    rec->tvb = tvb_new_real_data(real_data, data_len, data_len);
+    memcpy(real_data, data, data_len);
+    
+    /* head insertion */
+    rec->next= pi->handshake_data;
+    pi->handshake_data = rec;
+}
+
+/* search in packet data the tvbuff associated to the specified id */
+static tvbuff_t* 
+ssl_get_record_info(packet_info *pinfo, int record_id)
+{
+    SslRecordInfo* rec;
+    SslPacketInfo* pi = p_get_proto_data(pinfo->fd, proto_ssl);
+    if (!pi)
+        return NULL;
+    
+    for (rec = pi->handshake_data; rec; rec = rec->next)
+        if (rec->id == record_id)
+            return rec->tvb;
+
+    return NULL;
+}
+
 /* initialize/reset per capture state data (ssl sessions cache) */
 static void 
 ssl_init(void)
@@ -1379,12 +1418,13 @@
     tap_queue_packet(ssl_tap, pinfo, (gpointer)proto_ssl);
 }
 
-static void 
+static int
 decrypt_ssl3_record(tvbuff_t *tvb, packet_info *pinfo, guint32 offset, 
         guint32 record_length, guint8 content_type, SslDecryptSession* ssl,
         gboolean save_plaintext)
 {
-    int len, direction;
+    int ret = 0;
+    int direction;
     SslDecoder* decoder;
     
     /* if we can decrypt and decryption have success
@@ -1393,7 +1433,7 @@
         record_length, ssl->state);
     if (!(ssl->state & SSL_HAVE_SESSION_KEY)) {
         ssl_debug_printf("decrypt_ssl3_record: no session key\n");
-        return ;
+        return ret;
     }
     
     /* retrive decoder for this packet direction*/    
@@ -1419,46 +1459,48 @@
     
     /* run decryption and add decrypted payload to protocol data, if decryption 
     * is successful*/
-    len = ssl_decrypted_data.data_len; 
-    if ((ssl_decrypt_record(ssl, decoder, 
-        content_type, tvb_get_ptr(tvb, offset, record_length),
-        record_length,  ssl_decrypted_data.data, &len) == 0) && 
-        save_plaintext)
+    ssl_decrypted_data_avail = ssl_decrypted_data.data_len; 
+    if (ssl_decrypt_record(ssl, decoder, 
+          content_type, tvb_get_ptr(tvb, offset, record_length),
+          record_length,  ssl_decrypted_data.data, &ssl_decrypted_data_avail) == 0)
+        ret = 1;
+    if (ret && save_plaintext)
     {
-        StringInfo* data = p_get_proto_data(pinfo->fd, proto_ssl);
-        if (!data) 
+        SslPacketInfo* pi = p_get_proto_data(pinfo->fd, proto_ssl);
+        if (!pi) 
         {
             ssl_debug_printf("decrypt_ssl3_record: allocating app_data %d "
-                "bytes for app data\n", len);
+                "bytes for app data\n", ssl_decrypted_data_avail);
             /* first app data record: allocate and put packet data*/
-            data = se_alloc(sizeof(StringInfo));
-            data->data = se_alloc(len);
-            data->data_len = len;
-            memcpy(data->data, ssl_decrypted_data.data, len);
+            pi = se_alloc0(sizeof(SslPacketInfo));
+            pi->app_data.data = se_alloc(ssl_decrypted_data_avail);
+            pi->app_data.data_len = ssl_decrypted_data_avail;
+            memcpy(pi->app_data.data, ssl_decrypted_data.data, ssl_decrypted_data_avail);
         }
         else { 
             unsigned char* store;
             /* update previus record*/
             ssl_debug_printf("decrypt_ssl3_record: reallocating app_data "
                 "%d bytes for app data (total %d appdata bytes)\n", 
-                len, data->data_len + len);
-            store = se_alloc(data->data_len + len);
-            memcpy(store, data->data, data->data_len);
-            memcpy(&store[data->data_len], ssl_decrypted_data.data, len);
-            data->data_len += len;
+                ssl_decrypted_data_avail, pi->app_data.data_len + ssl_decrypted_data_avail);
+            store = se_alloc(pi->app_data.data_len + ssl_decrypted_data_avail);
+            memcpy(store, pi->app_data.data, pi->app_data.data_len);
+            memcpy(&store[pi->app_data.data_len], ssl_decrypted_data.data, ssl_decrypted_data_avail);
+            pi->app_data.data_len += ssl_decrypted_data_avail;
             
             /* old decrypted data ptr here appare to be leaked, but it's 
              * collected by emem allocator */
-            data->data = store;
+            pi->app_data.data = store;
             
             /* data ptr is changed, so remove old one and re-add the new one*/
             ssl_debug_printf("decrypt_ssl3_record: removing old app_data ptr\n");
             p_remove_proto_data(pinfo->fd, proto_ssl);
         }
      
-        ssl_debug_printf("decrypt_ssl3_record: setting decrypted app_data ptr %p\n",data);
-        p_add_proto_data(pinfo->fd, proto_ssl, data);
+        ssl_debug_printf("decrypt_ssl3_record: setting decrypted app_data ptr %p\n",pi);
+        p_add_proto_data(pinfo->fd, proto_ssl, pi);
     }
+    return ret;
 }
 
 
@@ -1500,7 +1542,7 @@
     proto_tree *ti              = NULL;
     proto_tree *ssl_record_tree = NULL;
     guint32 available_bytes     = 0;
-    StringInfo* decrypted;
+    SslPacketInfo* pi;
     SslAssociation* association;
 
     available_bytes = tvb_length_remaining(tvb, offset);
@@ -1671,6 +1713,7 @@
             col_append_str(pinfo->cinfo, COL_INFO, "Change Cipher Spec");
         dissect_ssl3_change_cipher_spec(tvb, ssl_record_tree,
                                         offset, conv_version, content_type);
+        ssl_debug_printf("dissect_ssl3_change_cipher_spec\n");
         break;
     case SSL_ID_ALERT:
         if (ssl)
@@ -1680,12 +1723,27 @@
                            conv_version);
         break;
     case SSL_ID_HANDSHAKE:
-        if (ssl)
-            decrypt_ssl3_record(tvb, pinfo, offset, 
-                record_length, content_type, ssl, FALSE);
-        dissect_ssl3_handshake(tvb, pinfo, ssl_record_tree, offset,
+    {
+        tvbuff_t* decrypted=0;
+        /* try to decrypt handshake record, if possible. Store decrypted 
+         * record for later usage. The offset is used as 'key' to itentify
+         * this record into the packet (we can have multiple handshake records
+         * in the same frame) */
+        if (ssl && decrypt_ssl3_record(tvb, pinfo, offset, 
+                record_length, content_type, ssl, FALSE)) 
+            ssl_add_record_info(pinfo, ssl_decrypted_data.data, 
+                ssl_decrypted_data_avail, offset);
+        
+        /* try to retrive and use decrypted handshake record, if any. */
+        decrypted = ssl_get_record_info(pinfo, offset);
+        if (decrypted)
+            dissect_ssl3_handshake(decrypted, pinfo, ssl_record_tree, 0,
+                 decrypted->length, conv_version, ssl, content_type);
+        else 
+            dissect_ssl3_handshake(tvb, pinfo, ssl_record_tree, offset,
                                record_length, conv_version, ssl, content_type);
         break;
+    }
     case SSL_ID_APP_DATA:
         if (ssl)
             decrypt_ssl3_record(tvb, pinfo, offset, 
@@ -1711,32 +1769,33 @@
             association?association->info:"Application Data");
     
         /* show decrypted data info, if available */         
-        decrypted = p_get_proto_data(pinfo->fd, proto_ssl);
-        if (decrypted)
+        pi = p_get_proto_data(pinfo->fd, proto_ssl);
+        if (pi && pi->app_data.data)
         {
             tvbuff_t* new_tvb;
             
             /* try to dissect decrypted data*/
-            ssl_debug_printf("dissect_ssl3_record decrypted len %d\n", decrypted->data_len);
+            ssl_debug_printf("dissect_ssl3_record decrypted len %d\n", 
+                pi->app_data.data_len);
             
              /* create new tvbuff for the decrypted data */
-            new_tvb = tvb_new_real_data(decrypted->data, 
-                decrypted->data_len, decrypted->data_len);
+            new_tvb = tvb_new_real_data(pi->app_data.data, 
+                pi->app_data.data_len, pi->app_data.data_len);
             tvb_set_free_cb(new_tvb, g_free);
             /* tvb_set_child_real_data_tvbuff(tvb, new_tvb); */
             
             /* find out a dissector using server port*/
             if (association && association->handle) {
                 ssl_debug_printf("dissect_ssl3_record found association %p\n", association);
-                ssl_print_text_data("decrypted app data",decrypted->data, 
-                    decrypted->data_len);
+                ssl_print_text_data("decrypted app data",pi->app_data.data, 
+                    pi->app_data.data_len);
                 
                 call_dissector(association->handle, new_tvb, pinfo, ssl_record_tree);
             }
             /* add raw decrypted data only if a decoder is not found*/
             else 
                 proto_tree_add_string(ssl_record_tree, hf_ssl_record_appdata_decrypted, tvb,
-                        offset, decrypted->data_len, (char*) decrypted->data);
+                    offset, pi->app_data.data_len, (char*) pi->app_data.data);
         }
         else {
             tvb_ensure_bytes_exist(tvb, offset, record_length);
@@ -2121,11 +2180,6 @@
             ssl_restore_session(ssl); 
         }
         else {
-            /* reset state on renegotiation*/
-            if (!from_server)
-                ssl->state &= ~(SSL_HAVE_SESSION_KEY|SSL_MASTER_SECRET|
-                    SSL_CIPHER|SSL_SERVER_RANDOM);
-            
             tvb_memcpy(tvb,ssl->session_id.data, offset+33, session_id_length);
             ssl->session_id.data_len = session_id_length;
         }