Wireshark mailing list archives
Re: packet-smb not properly handling transact requests and responses ...
From: Richard Sharpe <realrichardsharpe () gmail com>
Date: Sat, 9 Jun 2012 20:10:48 -0700
On Sat, Jun 9, 2012 at 5:20 PM, Richard Sharpe <realrichardsharpe () gmail com> wrote:
On Sat, Jun 9, 2012 at 4:51 PM, Richard Sharpe <realrichardsharpe () gmail com> wrote:On Sat, Jun 9, 2012 at 3:12 PM, Richard Sharpe <realrichardsharpe () gmail com> 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:
___________________________________________________________________________ Sent via: Wireshark-dev mailing list <wireshark-dev () wireshark org> Archives: http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-request () wireshark org?subject=unsubscribe
Current thread:
- packet-smb not properly handling transact requests and responses ... Richard Sharpe (Jun 09)
- Re: packet-smb not properly handling transact requests and responses ... Richard Sharpe (Jun 09)
- Re: packet-smb not properly handling transact requests and responses ... Richard Sharpe (Jun 09)
- Re: packet-smb not properly handling transact requests and responses ... Richard Sharpe (Jun 09)
- Re: packet-smb not properly handling transact requests and responses ... Richard Sharpe (Jun 09)
- Re: packet-smb not properly handling transact requests and responses ... Richard Sharpe (Jun 09)