Wireshark-dev: Re: [Wireshark-dev] packet-smb not properly handling transact requests and respo
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.
>
> [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;
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.
--
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)
