[Richard Sharpe <rsharpe@xxxxxxxxxx>]

On Sat, 6 Jul 2002, dheitmueller wrote:

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

Please not the subsequent comments by Tod Sabin as well.
 
> 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. 

This would be very interesting.

> 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.

See packet-smb.c, packet-...:

>static const true_false_string tfs_da_caching = {
>        "Do not cache this file",
>        "Caching permitted on this file"
>};

...

>        { &hf_smb_access_caching,
>                { "Caching", "smb.access.caching", FT_BOOLEAN, 16,
>                TFS(&tfs_da_caching), 0x1000, "Caching mode?", HFILL }},

And so on ...

> 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
> 
> 
> _______________________________________________
> Ethereal-dev mailing list
> Ethereal-dev@xxxxxxxxxxxx
> http://www.ethereal.com/mailman/listinfo/ethereal-dev
> 

-- 
Regards
-----
Richard Sharpe, rsharpe@xxxxxxxxxx, rsharpe@xxxxxxxxx, 
sharpe@xxxxxxxxxxxx