Re: packet-smb not properly handling transact requests and responses ...

[Richard Sharpe <realrichardsharpe () gmail com>]

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.

[rsharpe@localhost wireshark]$ svn diff epan/dissectors/packet-smb.?
Index: epan/dissectors/packet-smb.c
===================================================================
--- epan/dissectors/packet-smb.c        (revision 43181)
+++ epan/dissectors/packet-smb.c        (working copy)
@@ -17213,6 +17213,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 +17577,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);
@@ -17785,6 +17791,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 +17818,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
(何以解憂？唯有杜康。--曹操)
___________________________________________________________________________
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