Wireshark · Ethereal-dev: Re: [Ethereal-dev] [DCE RPC] Incorrect dissection with CVS version 20040603153321

[Todd Sabin <tsabin@xxxxxxxxxxxxx>]

Ulf Lamping <ulf.lamping@xxxxxx> writes:

> I've introduced this change some time ago, as I had problems with
> the DCE-RPC connectionless conversation/defragmenting stuff, as some
> DCR-RPC fragments were incorrectly related to each other, which
> actually wasn't true.  See:
>
> http://www.ethereal.com/lists/ethereal-dev/200405/msg00530.html
>
> Adding the activity_id at the place mentioned fixed the problem, and
> I thought it was ok.
>
> As this seems to raise a new problem, I need some help on this now :-(

See the attached patch.  It does two related things.  First, it splits
the dcerpc_calls into dcerpc_cn_calls and dcerpc_dg_calls, as I talked
about doing previously.  (This doesn't help with the fragment problem,
but I think it's necessary for getting call matching to work correctly.)
I think that part of the patch can be used as is.

The second thing it does is add some new fragment handling for dcerpc
fragments.  The hash table for those needs to be keyed on both the
seqnum and the activity_id.  The patch does that, and works right,
AFAICT, but it's pretty ugly as far as duplicating code and adding
dcerpc specific code to reassemble.c.  I don't think you want to use
it as is, but it does show what needs to be accomplished.


Todd

Index: packet-dcerpc.c
===================================================================
RCS file: /cvsroot/ethereal/packet-dcerpc.c,v
retrieving revision 1.176
diff -u -r1.176 packet-dcerpc.c
--- packet-dcerpc.c	29 May 2004 04:34:51 -0000	1.176
+++ packet-dcerpc.c	12 Jun 2004 02:48:04 -0000
@@ -481,7 +481,7 @@
 dcerpc_reassemble_init(void)
 {
   fragment_table_init(&dcerpc_co_reassemble_table);
-  fragment_table_init(&dcerpc_cl_reassemble_table);
+  dcerpc_fragment_table_init(&dcerpc_cl_reassemble_table);
 }
 
 /*
@@ -774,35 +774,66 @@
  * To keep track of callid mappings.  Should really use some generic
  * conversation support instead.
  */
-static GHashTable *dcerpc_calls=NULL;
+static GHashTable *dcerpc_cn_calls=NULL;
+static GHashTable *dcerpc_dg_calls=NULL;
 
-typedef struct _dcerpc_call_key {
+typedef struct _dcerpc_cn_call_key {
     conversation_t *conv;
     guint32 call_id;
     guint16 smb_fid;
-} dcerpc_call_key;
+} dcerpc_cn_call_key;
 
-static GMemChunk *dcerpc_call_key_chunk=NULL;
+typedef struct _dcerpc_dg_call_key {
+    conversation_t *conv;
+    guint32 seqnum;
+    e_uuid_t act_id ;
+} dcerpc_dg_call_key;
+
+static GMemChunk *dcerpc_cn_call_key_chunk=NULL;
+
+static GMemChunk *dcerpc_dg_call_key_chunk=NULL;
 
 static GMemChunk *dcerpc_call_value_chunk=NULL;
 
 static gint
-dcerpc_call_equal (gconstpointer k1, gconstpointer k2)
+dcerpc_cn_call_equal (gconstpointer k1, gconstpointer k2)
 {
-    const dcerpc_call_key *key1 = (const dcerpc_call_key *)k1;
-    const dcerpc_call_key *key2 = (const dcerpc_call_key *)k2;
+    const dcerpc_cn_call_key *key1 = (const dcerpc_cn_call_key *)k1;
+    const dcerpc_cn_call_key *key2 = (const dcerpc_cn_call_key *)k2;
     return (key1->conv == key2->conv
             && key1->call_id == key2->call_id
             && key1->smb_fid == key2->smb_fid);
 }
 
+static gint
+dcerpc_dg_call_equal (gconstpointer k1, gconstpointer k2)
+{
+    const dcerpc_dg_call_key *key1 = (const dcerpc_dg_call_key *)k1;
+    const dcerpc_dg_call_key *key2 = (const dcerpc_dg_call_key *)k2;
+    return (key1->conv == key2->conv
+            && key1->seqnum == key2->seqnum
+            && (memcmp (&key1->act_id, &key2->act_id, sizeof (e_uuid_t)) == 0));
+}
+
 static guint
-dcerpc_call_hash (gconstpointer k)
+dcerpc_cn_call_hash (gconstpointer k)
 {
-    const dcerpc_call_key *key = (const dcerpc_call_key *)k;
+    const dcerpc_cn_call_key *key = (const dcerpc_cn_call_key *)k;
     return ((guint32)key->conv) + key->call_id + key->smb_fid;
 }
 
+static guint
+dcerpc_dg_call_hash (gconstpointer k)
+{
+    const dcerpc_dg_call_key *key = (const dcerpc_dg_call_key *)k;
+    return (((guint32)key->conv) + key->seqnum + key->act_id.Data1
+            + (key->act_id.Data2 << 16) + key->act_id.Data3
+            + (key->act_id.Data4[0] << 24) + (key->act_id.Data4[1] << 16)
+            + (key->act_id.Data4[2] << 8) + (key->act_id.Data4[3] << 0)
+            + (key->act_id.Data4[4] << 24) + (key->act_id.Data4[5] << 16)
+            + (key->act_id.Data4[6] << 8) + (key->act_id.Data4[7] << 0));
+}
+
 
 /* to keep track of matched calls/responses
    this one uses the same value struct as calls, but the key is the frame id
@@ -2816,23 +2847,22 @@
 		bind_key.conv=conv;
 		bind_key.ctx_id=ctx_id;
 		bind_key.smb_fid=get_smb_fid(pinfo->private_data);
-
 		if((bind_value=g_hash_table_lookup(dcerpc_binds, &bind_key)) ){
 			if(!(hdr->flags&PFC_FIRST_FRAG)){
-				dcerpc_call_key call_key;
+				dcerpc_cn_call_key call_key;
 				dcerpc_call_value *call_value;
 
 				call_key.conv=conv;
 				call_key.call_id=hdr->call_id;
 				call_key.smb_fid=get_smb_fid(pinfo->private_data);
-				if((call_value=g_hash_table_lookup(dcerpc_calls, &call_key))){
+				if((call_value=g_hash_table_lookup(dcerpc_cn_calls, &call_key))){
 					new_matched_key = g_mem_chunk_alloc(dcerpc_matched_key_chunk);
 					*new_matched_key = matched_key;
 					g_hash_table_insert (dcerpc_matched, new_matched_key, call_value);
 					value = call_value;
 				}
 			} else {
-				dcerpc_call_key *call_key;
+				dcerpc_cn_call_key *call_key;
 				dcerpc_call_value *call_value;
 
 				/* We found the binding and it is the first fragment 
@@ -2840,15 +2870,15 @@
 				   the call to both the call table and the 
 				   matched table
 				*/
-				call_key=g_mem_chunk_alloc (dcerpc_call_key_chunk);
+				call_key=g_mem_chunk_alloc (dcerpc_cn_call_key_chunk);
 				call_key->conv=conv;
 				call_key->call_id=hdr->call_id;
 				call_key->smb_fid=get_smb_fid(pinfo->private_data);
 
 				/* if there is already a matching call in the table
 				   remove it so it is replaced with the new one */
-				if(g_hash_table_lookup(dcerpc_calls, call_key)){
-					g_hash_table_remove(dcerpc_calls, call_key);
+				if(g_hash_table_lookup(dcerpc_cn_calls, call_key)){
+					g_hash_table_remove(dcerpc_cn_calls, call_key);
 				}
 
 				call_value=g_mem_chunk_alloc (dcerpc_call_value_chunk);
@@ -2861,7 +2891,7 @@
 				call_value->rep_frame=0;
 				call_value->max_ptr=0;
 				call_value->private_data = NULL;
-				g_hash_table_insert (dcerpc_calls, call_key, call_value);
+				g_hash_table_insert (dcerpc_cn_calls, call_key, call_value);
 
 				new_matched_key = g_mem_chunk_alloc(dcerpc_matched_key_chunk);
 				*new_matched_key = matched_key;
@@ -2948,14 +2978,14 @@
 	matched_key.call_id = hdr->call_id;
 	value=g_hash_table_lookup(dcerpc_matched, &matched_key);
 	if(!value){
-		dcerpc_call_key call_key;
+		dcerpc_cn_call_key call_key;
 		dcerpc_call_value *call_value;
 
 		call_key.conv=conv;
 		call_key.call_id=hdr->call_id;
 		call_key.smb_fid=get_smb_fid(pinfo->private_data);
 
-		if((call_value=g_hash_table_lookup(dcerpc_calls, &call_key))){
+		if((call_value=g_hash_table_lookup(dcerpc_cn_calls, &call_key))){
 			new_matched_key = g_mem_chunk_alloc(dcerpc_matched_key_chunk);
 			*new_matched_key = matched_key;
 			g_hash_table_insert (dcerpc_matched, new_matched_key, call_value);
@@ -3059,14 +3089,14 @@
 	matched_key.call_id = hdr->call_id;
 	value=g_hash_table_lookup(dcerpc_matched, &matched_key);
 	if(!value){
-		dcerpc_call_key call_key;
+		dcerpc_cn_call_key call_key;
 		dcerpc_call_value *call_value;
 
 		call_key.conv=conv;
 		call_key.call_id=hdr->call_id;
 		call_key.smb_fid=get_smb_fid(pinfo->private_data);
 
-		if((call_value=g_hash_table_lookup(dcerpc_calls, &call_key))){
+		if((call_value=g_hash_table_lookup(dcerpc_cn_calls, &call_key))){
 			new_matched_key = g_mem_chunk_alloc(dcerpc_matched_key_chunk);
 			*new_matched_key = matched_key;
 			g_hash_table_insert (dcerpc_matched, new_matched_key, call_value);
@@ -3787,8 +3817,8 @@
 	    }
 	}
 
-	fd_head = fragment_add_seq(tvb, offset, pinfo,
-			hdr->seqnum, dcerpc_cl_reassemble_table,
+	fd_head = fragment_add_dcerpc(tvb, offset, pinfo,
+			hdr->seqnum, &hdr->act_id, dcerpc_cl_reassemble_table,
 			hdr->frag_num, stub_length,
 			!(hdr->flags1 & PFCL1_LASTFRAG));
 	if (fd_head != NULL) {
@@ -3840,12 +3870,12 @@
     di=get_next_di();
     if(!(pinfo->fd->flags.visited)){
 	dcerpc_call_value *call_value;
-	dcerpc_call_key *call_key;
+	dcerpc_dg_call_key *call_key;
 
-	call_key=g_mem_chunk_alloc (dcerpc_call_key_chunk);
+	call_key=g_mem_chunk_alloc (dcerpc_dg_call_key_chunk);
 	call_key->conv=conv;
-	call_key->call_id=hdr->seqnum;
-	call_key->smb_fid=get_smb_fid(pinfo->private_data);
+	call_key->seqnum=hdr->seqnum;
+	call_key->act_id=hdr->act_id;
 
 	call_value=g_mem_chunk_alloc (dcerpc_call_value_chunk);
 	call_value->uuid = hdr->if_id;
@@ -3857,7 +3887,7 @@
 	call_value->rep_frame=0;
 	call_value->max_ptr=0;
 	call_value->private_data = NULL;
-	g_hash_table_insert (dcerpc_calls, call_key, call_value);
+	g_hash_table_insert (dcerpc_dg_calls, call_key, call_value);
 
 	new_matched_key = g_mem_chunk_alloc(dcerpc_matched_key_chunk);
 	new_matched_key->frame = pinfo->fd->num;
@@ -3906,13 +3936,13 @@
     di=get_next_di();
     if(!(pinfo->fd->flags.visited)){
 	dcerpc_call_value *call_value;
-	dcerpc_call_key call_key;
+	dcerpc_dg_call_key call_key;
 
 	call_key.conv=conv;
-	call_key.call_id=hdr->seqnum;
-	call_key.smb_fid=get_smb_fid(pinfo->private_data);
+	call_key.seqnum=hdr->seqnum;
+	call_key.act_id=hdr->act_id;
 
-	if((call_value=g_hash_table_lookup(dcerpc_calls, &call_key))){
+	if((call_value=g_hash_table_lookup(dcerpc_dg_calls, &call_key))){
 	    new_matched_key = g_mem_chunk_alloc(dcerpc_matched_key_chunk);
 	    new_matched_key->frame = pinfo->fd->num;
 	    new_matched_key->call_id = hdr->seqnum;
@@ -4331,17 +4361,28 @@
                                              200 * sizeof (dcerpc_bind_value),
                                              G_ALLOC_ONLY);
 	/* structures and data for CALL */
-	if (dcerpc_calls){
-		g_hash_table_destroy (dcerpc_calls);
+	if (dcerpc_cn_calls){
+		g_hash_table_destroy (dcerpc_cn_calls);
 	}
-	dcerpc_calls = g_hash_table_new (dcerpc_call_hash, dcerpc_call_equal);
-	if (dcerpc_call_key_chunk){
-		g_mem_chunk_destroy (dcerpc_call_key_chunk);
-	}
-	dcerpc_call_key_chunk = g_mem_chunk_new ("dcerpc_call_key_chunk",
-                                             sizeof (dcerpc_call_key),
-                                             200 * sizeof (dcerpc_call_key),
-                                             G_ALLOC_ONLY);
+	dcerpc_cn_calls = g_hash_table_new (dcerpc_cn_call_hash, dcerpc_cn_call_equal);
+	if (dcerpc_dg_calls){
+		g_hash_table_destroy (dcerpc_dg_calls);
+	}
+	dcerpc_dg_calls = g_hash_table_new (dcerpc_dg_call_hash, dcerpc_dg_call_equal);
+	if (dcerpc_cn_call_key_chunk){
+		g_mem_chunk_destroy (dcerpc_cn_call_key_chunk);
+	}
+	dcerpc_cn_call_key_chunk = g_mem_chunk_new ("dcerpc_cn_call_key_chunk",
+                                                sizeof (dcerpc_cn_call_key),
+                                                200 * sizeof (dcerpc_cn_call_key),
+                                                G_ALLOC_ONLY);
+	if (dcerpc_dg_call_key_chunk){
+		g_mem_chunk_destroy (dcerpc_dg_call_key_chunk);
+	}
+	dcerpc_dg_call_key_chunk = g_mem_chunk_new ("dcerpc_dg_call_key_chunk",
+                                                sizeof (dcerpc_dg_call_key),
+                                                200 * sizeof (dcerpc_dg_call_key),
+                                                G_ALLOC_ONLY);
 	if (dcerpc_call_value_chunk){
 		g_mem_chunk_destroy (dcerpc_call_value_chunk);
 	}
Index: reassemble.c
===================================================================
RCS file: /cvsroot/ethereal/reassemble.c,v
retrieving revision 1.48
diff -u -r1.48 reassemble.c
--- reassemble.c	15 May 2004 00:41:27 -0000	1.48
+++ reassemble.c	12 Jun 2004 02:48:05 -0000
@@ -31,7 +31,7 @@
 #include <epan/packet.h>
 
 #include "reassemble.h"
-
+#include "packet-dcerpc.h"
 
 typedef struct _fragment_key {
 	address src;
@@ -39,7 +39,15 @@
 	guint32	id;
 } fragment_key;
 
+typedef struct _dcerpc_fragment_key {
+	address src;
+	address dst;
+	guint32	id;
+    e_uuid_t act_id;
+} dcerpc_fragment_key;
+
 static GMemChunk *fragment_key_chunk = NULL;
+static GMemChunk *dcerpc_fragment_key_chunk = NULL;
 static GMemChunk *fragment_data_chunk = NULL;
 static int fragment_init_count = 200;
 
@@ -97,6 +105,39 @@
 	return hash_val;
 }
 
+static gint
+dcerpc_fragment_equal(gconstpointer k1, gconstpointer k2)
+{
+	const dcerpc_fragment_key* key1 = (const dcerpc_fragment_key*) k1;
+	const dcerpc_fragment_key* key2 = (const dcerpc_fragment_key*) k2;
+
+	/*key.id is the first item to compare since item is most
+	  likely to differ between sessions, thus shortcircuiting
+	  the comparasion of addresses.
+	*/
+	return (((key1->id == key2->id)
+             && (ADDRESSES_EQUAL(&key1->src, &key2->src))
+             && (ADDRESSES_EQUAL(&key1->dst, &key2->dst))
+             && (memcmp (&key1->act_id, &key2->act_id, sizeof (e_uuid_t)) == 0))
+            ? TRUE : FALSE);
+}
+
+static guint
+dcerpc_fragment_hash(gconstpointer k)
+{
+	const dcerpc_fragment_key* key = (const dcerpc_fragment_key*) k;
+	guint hash_val;
+
+	hash_val = 0;
+
+	hash_val += key->id;
+    hash_val += key->act_id.Data1;
+    hash_val += key->act_id.Data2 << 16;
+    hash_val += key->act_id.Data3;
+
+	return hash_val;
+}
+
 typedef struct _reassembled_key {
 	guint32	id;
 	guint32 frame;
@@ -214,6 +255,26 @@
 	}
 }
 
+void
+dcerpc_fragment_table_init(GHashTable **fragment_table)
+{
+	if (*fragment_table != NULL) {
+		/*
+		 * The fragment hash table exists.
+		 *
+		 * Remove all entries and free fragment data for
+		 * each entry.  (The key and value data is freed
+		 * by "reassemble_init()".)
+		 */
+		g_hash_table_foreach_remove(*fragment_table,
+				free_all_fragments, NULL);
+	} else {
+		/* The fragment table does not exist. Create it */
+		*fragment_table = g_hash_table_new(dcerpc_fragment_hash,
+				dcerpc_fragment_equal);
+	}
+}
+
 /*
  * Initialize a reassembled-packet table.
  */
@@ -246,6 +307,8 @@
 {
 	if (fragment_key_chunk != NULL)
 		g_mem_chunk_destroy(fragment_key_chunk);
+	if (dcerpc_fragment_key_chunk != NULL)
+		g_mem_chunk_destroy(fragment_key_chunk);
 	if (fragment_data_chunk != NULL)
 		g_mem_chunk_destroy(fragment_data_chunk);
 	if (reassembled_key_chunk != NULL)
@@ -254,6 +317,10 @@
 	    sizeof(fragment_key),
 	    fragment_init_count * sizeof(fragment_key),
 	    G_ALLOC_AND_FREE);
+	dcerpc_fragment_key_chunk = g_mem_chunk_new("dcerpc_fragment_key_chunk",
+	    sizeof(dcerpc_fragment_key),
+	    fragment_init_count * sizeof(dcerpc_fragment_key),
+	    G_ALLOC_AND_FREE);
 	fragment_data_chunk = g_mem_chunk_new("fragment_data_chunk",
 	    sizeof(fragment_data),
 	    fragment_init_count * sizeof(fragment_data),
@@ -1221,6 +1288,79 @@
 	}
 }
 
+fragment_data *
+fragment_add_dcerpc(tvbuff_t *tvb, int offset, packet_info *pinfo, guint32 id,
+                    void *v_act_id,
+                    GHashTable *fragment_table, guint32 frag_number,
+                    guint32 frag_data_len, gboolean more_frags)
+{
+	dcerpc_fragment_key key, *new_key;
+	fragment_data *fd_head;
+    e_uuid_t *act_id = (e_uuid_t *)v_act_id;
+
+	/* create key to search hash with */
+	key.src = pinfo->src;
+	key.dst = pinfo->dst;
+	key.id  = id;
+	key.act_id  = *act_id;
+
+	fd_head = g_hash_table_lookup(fragment_table, &key);
+
+	/* have we already seen this frame ?*/
+	if (pinfo->fd->flags.visited) {
+		if (fd_head != NULL && fd_head->flags & FD_DEFRAGMENTED) {
+			return fd_head;
+		} else {
+			return NULL;
+		}
+	}
+
+	if (fd_head==NULL){
+		/* not found, this must be the first snooped fragment for this
+                 * packet. Create list-head.
+		 */
+		fd_head=g_mem_chunk_alloc(fragment_data_chunk);
+
+		/* head/first structure in list only holds no other data than
+                 * 'datalen' then we don't have to change the head of the list
+                 * even if we want to keep it sorted
+                 */
+		fd_head->next=NULL;
+		fd_head->datalen=0;
+		fd_head->offset=0;
+		fd_head->len=0;
+		fd_head->flags=FD_BLOCKSEQUENCE;
+		fd_head->data=NULL;
+		fd_head->reassembled_in=0;
+
+		/*
+		 * We're going to use the key to insert the fragment,
+		 * so allocate a structure for it, and copy the
+		 * addresses, allocating new buffers for the address
+		 * data.
+		 */
+		new_key = g_mem_chunk_alloc(dcerpc_fragment_key_chunk);
+		COPY_ADDRESS(&new_key->src, &key.src);
+		COPY_ADDRESS(&new_key->dst, &key.dst);
+		new_key->id = key.id;
+		new_key->act_id = key.act_id;
+		g_hash_table_insert(fragment_table, new_key, fd_head);
+	}
+
+	if (fragment_add_seq_work(fd_head, tvb, offset, pinfo,
+				  frag_number, frag_data_len, more_frags)) {
+		/*
+		 * Reassembly is complete.
+		 */
+		return fd_head;
+	} else {
+		/*
+		 * Reassembly isn't complete.
+		 */
+		return NULL;
+	}
+}
+
 /*
  * This does the work for "fragment_add_seq_check()" and
  * "fragment_add_seq_next()".
Index: reassemble.h
===================================================================
RCS file: /cvsroot/ethereal/reassemble.h,v
retrieving revision 1.21
diff -u -r1.21 reassemble.h
--- reassemble.h	20 Dec 2003 03:21:20 -0000	1.21
+++ reassemble.h	12 Jun 2004 02:48:05 -0000
@@ -68,6 +68,7 @@
  * Initialize a fragment table.
  */
 extern void fragment_table_init(GHashTable **fragment_table);
+extern void dcerpc_fragment_table_init(GHashTable **fragment_table);
 
 /*
  * Initialize a reassembled-packet table.
@@ -108,6 +109,12 @@
     guint32 id, GHashTable *fragment_table, guint32 frag_number,
     guint32 frag_data_len, gboolean more_frags);
 
+extern fragment_data *
+fragment_add_dcerpc(tvbuff_t *tvb, int offset, packet_info *pinfo, guint32 id,
+                    void *act_id,
+                    GHashTable *fragment_table, guint32 frag_number,
+                    guint32 frag_data_len, gboolean more_frags);
+
 /*
  * These functions add a new fragment to the fragment hash table.
  * If this is the first fragment seen for this datagram, a new

-- 
Todd Sabin                                          <tsabin@xxxxxxxxxxxxx>