Wireshark · Ethereal-dev: Re: [Ethereal-dev] ntlmssp decoding

[dheitmueller <dheitmueller@xxxxxxxxxxx>]

Ronnie,

Thanks for taking the time to review my code changes.  I will make the suggested changes, and post the revised patch.

In the long term, I am interested in decrypting the payload of subsequent packets, assuming Ethereal is given the end-user password.  At this juncture, I want to take an incremental approach and just get the fields properly dissected.  Admittedly, I am new to Ethereal development and am still trying to get accustomed to working with the codebase. 

Regarding the use of flags_set_truth for boolean parameters, could you point me to an example in CVS where you feel booleans are being properly dissected.  I copied the code from the dissect_dcerpc_cn () in packet-dcerpc.c, so if there is a better example, point me to it and I will change the code appropriately.

Thanks,

Devin

Quoting Ronnie Sahlberg <sahlberg@xxxxxxxxxxxxxxxx>:

> Hi,
> 
> Looks very good.
> But i have some suggestions:
> 
> 1, Change all // comments into proper C comments. Ethereal is supported
> and
> used on platforms where
> C++ style comments are not supported.
> 
> 2, Cosmetic change. Matter of preference, use you own judgement if you
> wnat
> to follow this suggestion or not.
> Where you dissect the boolean bitfields, you dissect them in the order
> least
> significant bit first
> and most significant bit last. Myself, I think it is easier to read and
> feels more natural if ethereal
> dissects them in the reverse order, ie the most significant bit first.
> I.e. consider just reersing the order you make the
> proto_tree_add_boolean()
> calls.
> 
> 3, in dissect_dcerpc_ntlmssp_negotiate()
> The dissectors are sometimes called, not to display anything on the
> screen
> but only to evaluate
> if display filters match. For these calls, dcerpc_tree will be NULL.
> A microoptimization common in other dissectors is to encapsulate the
>    proto_tree_add_xxx() proto_item_add_subtree() inside an if()
> statement.
> Consider encapsulating these calls in this function as
> 
> if(dcerpc_tree){
>     tf=proto_tree_add_uint(...
>     negotiate_flags_tree= ...
> }
> 
> 4, in dissect_dcerpc_ntlmssp_negotiate()
> You use tvb_get_letohs() to read the 16bit bitmask with the flags.
> This is a bug and will not work if the host sending the packet is big
> endian.
> The byteorder of DCERPC types are always encoded as the native format of
> the
> sender.
> There are other users of these things which are not x86 based. Think NT
> for
> MIPS/ALPHA/PPC/...
> (dead platforms now, but they do exist).
> Instead, when reading datatypes for DCERPC you should use the accessors
> specially made for DCERPC.
> These accessors are called dissect_ndr_xxx().
> 
> So in the functions where you use  tvb_get_letohs() to read a 16bit int,
> use
> instead something like:
>      dissect_ndr_uint16(tvb, offset, pinfo, NULL, drep,
> hf_ntlmssp_message_type, &negotiate_flags);
> Oh, you must also change the type for negotiate_flags from guint32 into
> guint16.
> 
> 
> Using NULL as the tree parameter since we dont want
> dissect_ndrt_uint16() to
> create an item in the tree.
> 
> 
> 5, Strings of ASCII characters of binary data is not really very hard.
> You declare a hf_  thingie of type FT_STRING or FT_BYTES and then just
> do a
> proto_tree_add_item().
> 
> 
> For your initial question:
> 4 Hours to add only 3 or four new fields is not too bad if the new
> fields
> are as big as these one were.
> You are not doing anything seriously wrong at all. It is just the
> learning
> curve.
> 
> 
> Oh, a final suggestion.
> You use some default flags_set_truth for all the booleans. While this is
> ok,
> it makes a nicer dissector (imho)
> by using dedicated TFS structs for each individual bit.
> It is a lot of extra work and often not really worth the effort to go
> this
> extra step but if you aim for dissector perfection ...
> Use your own judgement if it would be worth it to do this change.
> 
> 
> 
> Enough feedback for this iteration?
> 
> 
> ---
> Any plan to furhter enhance it later to verify or decrypt stuff if the
> NTLMSSP encryption keys are known and entered into ethereal?
> 
> 
> 
> 
> 
> 
> ----- Original Message -----
> From: "Devin Heitmueller"
> Sent: Saturday, July 06, 2002 1:30 PM
> Subject: [Ethereal-dev] ntlmssp decoding
> 
> 
> > I now have a newfound appreciation for how much work goes into writing
> > dissectors.
> >
> > I have made a few changes to further decode the DCERPC bind message to
> > show ntlmssp fields.  It has taken me about four hours to add three or
> > four fields.  I suspect this is either because I am doing something
> > seriously wrong, or I am still in the learning curve.
> >
> > Would it be possible for someone to review my attached changes, and
> > provide feedback?  In particular, I am interested in knowing if I am
> > using the correct primitives to decode the various data types, etc
> (for
> > example, I still can't figure out how to display strings).
> >
> > I am very interested in going further, but I would appreciate a sanity
> > check on what I have done thus far, so my patches do not get rejected.
> >
> > Any feedback would be greatly appreciated.
> >
> > Thanks,
> >
> > --
> > Devin Heitmueller
> > Senior Software Engineer
> > Netilla Networks Inc
> >
> 
> 



Devin Heitmueller
Senior Software Engineer
Netilla Networks Inc
732-652-5211