Wireshark · Wireshark-dev: Re: [Wireshark-dev] packet-smb not properly handling transact requests and responses ...

[Richard Sharpe <realrichardsharpe@xxxxxxxxx>]

On Sat, Jun 9, 2012 at 5:20 PM, Richard Sharpe
<realrichardsharpe@xxxxxxxxx> wrote:
> On Sat, Jun 9, 2012 at 4:51 PM, Richard Sharpe
> <realrichardsharpe@xxxxxxxxx> wrote:
>> On Sat, Jun 9, 2012 at 3:12 PM, Richard Sharpe
>> <realrichardsharpe@xxxxxxxxx> wrote:
>>> Hi folks,
>>>
>>> So, in Samba bug https://bugzilla.samba.org/show_bug.cgi?id=8989 you
>>> will find two captures relating to the handling of NT TRANSACT SET
>>> SECURITY DESCRIPTOR.
>>>
>>> Wireshark does not handle the dissection of these correctly, and I
>>> suspect, normal SMB TRANSACT and SMB TRANSACT2 requests/responses.
>>>
>>> In dissect_smb, in the code for a normal bidirectional request or
>>> response we lookup, using g_hash_table_lookup, the sip for the pid_mid
>>> for the current frame. However, we look it up in the unmatched
>>> requests.
>>>
>>> By the time we see a secondary, the original request with that pid_mid
>>> is no longer unmatched, so we have a null sip. Later, when we call
>>> smb_trans_defragment on the secondary (so we can give this fragment to
>>> the original request), we do not have a sip, so we do nothing.
>>>
>>> It seems that in dissect_smb, if the request is an XXX_SECONDARY, we
>>> should look up the sip in the matched packets not the unmatched
>>> packets.
>>>
>>> What say ye?
>>>
>>> I will give that a try to see if I can make progress.
>>
>> The following patch gets much closer to handling this, but it is not
>> yet correct. For example, it lables the secondary as an unknown and it
>> shows extra byte parameters against the primary.
>
> A minor mod fixes the problem of the extra byte parameters ... now to
> fix the two remaining problems:
>
> 1. The secondary is labeled as unknown. I suspect that it would better
> to actually associate the dissection with the last secondary ...
>
> 2. Requests that require multiple secondaries are not handled
> correctly. This will require adding some code from the response
> handling section.
>
> Feedback welcome.

OK, this patch fixes the re-assembly of NT_TRANSACT requests and
responses. I suspect that the others will have to be fixed similarly.
Patch is attached as well.

There is also the issue of handling the continuations better.

Index: epan/dissectors/packet-smb.c
===================================================================
--- epan/dissectors/packet-smb.c	(revision 43181)
+++ epan/dissectors/packet-smb.c	(working copy)
@@ -8852,7 +8852,8 @@
 dissect_nt_transaction_request(tvbuff_t *tvb, packet_info *pinfo _U_,
proto_tree *tree, int offset, proto_tree *smb_tree _U_)
 {
 	guint8 wc, sc;
-	guint32 pc=0, po=0, dc=0, od=0;
+	guint32 pc=0, pd = 0, po=0, dc=0, od=0, dd = 0;
+	guint32 td=0, tp=0;
 	smb_info_t *si;
 	smb_saved_info_t *sip;
 	int subcmd;
@@ -8860,6 +8861,9 @@
 	guint16 bc;
 	guint32 padcnt;
 	smb_nt_transact_info_t *nti=NULL;
+	fragment_data *r_fd = NULL;
+	tvbuff_t *pd_tvb=NULL;
+	gboolean save_fragmented;

 	ntd.subcmd = ntd.sd_len = ntd.ea_len = 0;

@@ -8887,11 +8891,13 @@


 	/* total param count */
-	proto_tree_add_item(tree, hf_smb_total_param_count, tvb, offset, 4,
ENC_LITTLE_ENDIAN);
+	tp = tvb_get_letohl(tvb, offset);
+	proto_tree_add_uint(tree, hf_smb_total_param_count, tvb, offset, 4, tp);
 	offset += 4;

 	/* total data count */
-	proto_tree_add_item(tree, hf_smb_total_data_count, tvb, offset, 4,
ENC_LITTLE_ENDIAN);
+	td = tvb_get_letohl(tvb, offset);
+	proto_tree_add_uint(tree, hf_smb_total_data_count, tvb, offset, 4, td);
 	offset += 4;

 	if(wc>=19){
@@ -8940,7 +8946,8 @@
 		/* primary request */
 	} else {
 		/* secondary request */
-		proto_tree_add_item(tree, hf_smb_data_disp32, tvb, offset, 4,
ENC_LITTLE_ENDIAN);
+		dd = tvb_get_letohl(tvb, offset);
+		proto_tree_add_uint(tree, hf_smb_data_disp32, tvb, offset, 4, dd);
 		offset += 4;
 	}

@@ -9003,8 +9010,54 @@

 	BYTE_COUNT;

-	/* parameters */
-	if(po>(guint32)offset){
+	/* reassembly of SMB NT Transaction data payload.
+	   In this section we do reassembly of both the data and parameters
+	   blocks of the SMB transaction command.
+	*/
+	save_fragmented = pinfo->fragmented;
+	/* do we need reassembly? */
+	if( (td&&(td!=dc)) || (tp&&(tp!=pc)) ){
+		/* oh yeah, either data or parameter section needs
+		   reassembly...
+		*/
+		pinfo->fragmented = TRUE;
+		if(smb_trans_reassembly){
+			/* ...and we were told to do reassembly */
+			if(pc && ((unsigned int)tvb_length_remaining(tvb, po)>=pc) ){
+				r_fd = smb_trans_defragment(tree, pinfo, tvb,
+							     po, pc, pd, td+tp);
+			}
+			if((r_fd==NULL) && dc && ((unsigned int)tvb_length_remaining(tvb,
od)>=dc) ){
+				r_fd = smb_trans_defragment(tree, pinfo, tvb,
+							     od, dc, dd+tp, td+tp);
+			}
+		}
+	}
+
+	/* if we got a reassembled fd structure from the reassembly routine we
+	   must create pd_tvb from it
+	*/
+	if(r_fd){
+		proto_item *frag_tree_item;
+
+		pd_tvb = tvb_new_child_real_data(tvb, r_fd->data, r_fd->datalen,
+						 r_fd->datalen);
+		add_new_data_source(pinfo, pd_tvb, "Reassembled SMB");
+
+		show_fragment_tree(r_fd, &smb_frag_items, tree, pinfo, pd_tvb,
&frag_tree_item);
+	}
+
+	if(pd_tvb){
+	  /* we have reassembled data, grab param and data from there */
+	  dissect_nt_trans_param_request(pd_tvb, pinfo, 0, tree, tp,
+					  &ntd, (guint16) tvb_length(pd_tvb), nti);
+	  dissect_nt_trans_data_request(pd_tvb, pinfo, tp, tree, td, &ntd, nti);
+	  COUNT_BYTES(bc); /* We are done */
+	} else {
+	  /* we do not have reassembled data, just use what we have in the
+	     packet as well as we can */
+	  /* parameters */
+	  if(po>(guint32)offset){
 		/* We have some initial padding bytes.
 		*/
 		padcnt = po-offset;
@@ -9013,15 +9066,15 @@
 		CHECK_BYTE_COUNT(padcnt);
 	        proto_tree_add_item(tree, hf_smb_padding, tvb, offset,
padcnt, ENC_NA);
 		COUNT_BYTES(padcnt);
-	}
-	if(pc){
+	  }
+	  if(pc){
 		CHECK_BYTE_COUNT(pc);
 		dissect_nt_trans_param_request(tvb, pinfo, offset, tree, pc, &ntd, bc, nti);
 		COUNT_BYTES(pc);
-	}
+	  }

-	/* data */
-	if(od>(guint32)offset){
+	  /* data */
+	  if(od>(guint32)offset){
 		/* We have some initial padding bytes.
 		*/
 		padcnt = od-offset;
@@ -9029,12 +9082,13 @@
 			padcnt = bc;
 	        proto_tree_add_item(tree, hf_smb_padding, tvb, offset,
padcnt, ENC_NA);
 		COUNT_BYTES(padcnt);
-	}
-	if(dc){
+	  }
+	  if(dc){
 		CHECK_BYTE_COUNT(dc);
 		dissect_nt_trans_data_request(
 			tvb, pinfo, offset, tree, dc, &ntd, nti);
 		COUNT_BYTES(dc);
+	  }
 	}

 	END_OF_SMB
@@ -9549,6 +9603,7 @@
 	  dissect_nt_trans_param_response(pd_tvb, pinfo, 0, tree, tp,
 					  &ntd, (guint16) tvb_length(pd_tvb));
 	  dissect_nt_trans_data_response(pd_tvb, pinfo, tp, tree, td, &ntd, nti);
+	  COUNT_BYTES(bc); /* We are done */
 	} else {
 	  /* we do not have reassembled data, just use what we have in the
 	     packet as well as we can */
@@ -17213,6 +17268,8 @@
 		g_hash_table_destroy(ct->unmatched);
 	if (ct->matched)
 		g_hash_table_destroy(ct->matched);
+	if (ct->primaries)
+		g_hash_table_destroy(ct->primaries);
 	if (ct->tid_service)
 		g_hash_table_destroy(ct->tid_service);
 	g_free(ct);
@@ -17575,6 +17632,10 @@
 			smb_saved_info_equal_matched);
 		si->ct->unmatched= g_hash_table_new(smb_saved_info_hash_unmatched,
 			smb_saved_info_equal_unmatched);
+		/* We used the same key format as the unmatched entries */
+		si->ct->primaries=g_hash_table_new(
+			smb_saved_info_hash_unmatched,
+			smb_saved_info_equal_unmatched);
 		si->ct->tid_service=g_hash_table_new(
 			smb_saved_info_hash_unmatched,
 			smb_saved_info_equal_unmatched);
@@ -17640,6 +17701,12 @@
 				new_key->pid_mid = pid_mid;
 				g_hash_table_insert(si->ct->matched, new_key,
 				    sip);
+			} else {
+				if (si->cmd == SMB_COM_TRANSACTION_SECONDARY ||
+				    si->cmd == SMB_COM_TRANSACTION2_SECONDARY ||
+				    si->cmd == SMB_COM_NT_TRANSACT_SECONDARY) {
+					sip = g_hash_table_lookup(si->ct->primaries, GUINT_TO_POINTER(pid_mid));
+				}
 			}
 		} else {
 			/* we have seen this packet before; check the
@@ -17785,6 +17852,12 @@
 						sip=NULL;
 					}
 				}
+			} else {
+				if (si->cmd == SMB_COM_TRANSACTION ||
+				    si->cmd == SMB_COM_TRANSACTION2 ||
+				    si->cmd == SMB_COM_NT_TRANSACT) {
+					sip = g_hash_table_lookup(si->ct->primaries, GUINT_TO_POINTER(pid_mid));
+				}
 			}
 			if(si->request){
 				sip = se_alloc(sizeof(smb_saved_info_t));
@@ -17806,6 +17879,13 @@
 				new_key->frame = sip->frame_req;
 				new_key->pid_mid = pid_mid;
 				g_hash_table_insert(si->ct->matched, new_key, sip);
+
+				/* If it is a TRANSACT cmd, insert in hash */
+				if (si->cmd == SMB_COM_TRANSACTION ||
+				    si->cmd == SMB_COM_TRANSACTION2 ||
+				    si->cmd == SMB_COM_NT_TRANSACT) {
+					g_hash_table_insert(si->ct->primaries, GUINT_TO_POINTER(pid_mid), sip);
+				}
 			}
 		} else {
 			/* we have seen this packet before; check the
Index: epan/dissectors/packet-smb.h
===================================================================
--- epan/dissectors/packet-smb.h	(revision 42332)
+++ epan/dissectors/packet-smb.h	(working copy)
@@ -282,6 +282,9 @@
 	/* these two tables are used to match requests with responses */
 	GHashTable *unmatched;
 	GHashTable *matched;
+	/* This table keeps primary transact requests so secondaries can find
+	   them */
+	GHashTable *primaries;

 	/* This table is used to track TID->services for a conversation */
 	GHashTable *tid_service;



-- 
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)

Attachment: packet-smb.nttrans.patch
Description: Binary data