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
>
