Wireshark · Ethereal-dev: Re: Re: [Ethereal-dev] DCOM implementation, first try!

[Ulf Lamping <ulf.lamping@xxxxxx>]

 Todd Sabin  schrieb am 31.08.02 02:10:02:
> Ulf Lamping writes:
>
> > Hi list!
> >
> > Here is the first implementation of the DCOM dissection I mentioned earlier.
>
> As a sort of pre-comment, this is a really large patch, and difficult
> to consider in its entirety. I think it might make sense to re-submit
> it in stages, given that we now have an idea where you're going.
> (Actually, I'm not sure I agree with your direction, but more on that
> later.)
>
I agree with the idea of submitting in stages 100%,
and will send diffs step by step when we move further with this discussion.
It was my intention to give an overview of what I have done and start the discussion with it.

> > All files for DCOM currently called something like: "packet-dcerpc-xy.c"
> > (e.g. packet-dcerpc-oxid.c). I have called them "packet-dcom-xy.c",
> > as the DCOM dissectors mentioned above are sitting on top of DCOM, not on top of DCERPC,
> > and of course the DCOM implementation itself is not part of DCERPC.
>
> Err, well, this is really a matter of perspective, I think. IMHO, the
> best way to think of DCOM is as a 'discipline' for doing distributed,
> reference counted, object based RPC, not as an actual network protocol
> in and of itself. The actual DCOM wire protocol happens to just be a
> set of normal DCERPC interfaces used for activation, reference
> counting, oxid resolution, etc. (the dcerpc-oxid, dcerpc-remact, etc
> interfaces) along with whatever interfaces the objects in question
> support. The calls to those per-object interfaces are also DCERPC,
> with the qualification that they all have the object flag specified
> (hf_dcerpc_cn_flags_object), and the NDR for the requests starts with
> an ORPC_THIS, and the replies start with an ORPC_THAT. In other
> words, DCOM is 'invisible' on the wire, and everything can be fully
> described with DCERPC and NDR. (Sometimes, the NDR is used to shovel
> byte-bags around which have other, non-NDR, structure within.)
>
> Hopefully, that made some sense, because it's the basis for most of my
> comments below. :)

I disagree, but this is always a matter of perspective ;-).
Joke: You could say that DCERPC is only a special use of TCP and call it packet-tcp-dcerpc.c!
(Sometimes, TCP is used to shovel byte-bags around which have other, non-TCP, structure within. ;-)

You understand DCOM as a small extension of DCERPC, because DCOM has less "physical" representation of network data, but more a "logical" effect on the remote machine (as object orientation often does).

Compared for example to the spoolss things (which has a defined purpose) and other DCERPC interfaces, COM is a more general purpose mechanism, as you have written. It provides the user of DCOM with some general purpose functionality. I think a protocol layer is defined as something like this ;-)

But still another reason was to be able to distinguish DCOM from DCERPC files,
as the number of DCERPC files is constantly growing,
and the DCOM files will hopefully too.

-> I fear that in the future there will be A LOT of packet-dcerpc-xy files.

The DCOM files are a "block" as a whole (and functions as such)
and I still think it's a good idea to rename the files that way.

>
> > What I have done so far to add DCOM to Ethereal:
>
> Right, on to the code. Again, I think it'd be better to resubmit this
> in easier to digest pieces.
>
> > Changes in the existing code:
> > -----------------------------
> > Changes in dcerpc.c/.h:
>
> You modified the types of dcerpc.ver and dcerpc.ver_minor from UINT8
> to UINT16. Why? They're clearly 8 bit values on the wire.

Sorry, this was left over from testing (and playing around),
there is really no reason for this, please simply ignore it...

>
> You split each of dcerpc.cn_bind_trans_ver and ...ack_trans_ver into
> major and minor versions. Why? The OpenGroup spec says a transfer
> syntax id looks like:
>
> typedef struct {
> uuid_t if_uuid;
> u_int32 if_version;
> } p_syntax_id_t;
>
> Admittedly, that spec is pretty old. Do you have a more recent spec
> with something different in it?

...and please ignore this also.

>
> > new methods dissect_dcerpc_uuid() and similar
>
> The comment about interfaces, not uuids, having versions is true, but
> a bit off the mark. What I mean is that if you're going to use a uuid
> on the network, you need a version to go with it. For DCOM, the

Again, you are mixing uuids and interfaces, which is IMHO not right. The whole
implementation of the dcerpc_uuids hashtable (and symbols alike) should be renamed
to dcerpc_interfaces.
Reason: We are NOT hashing a uuid, but an interface (containing a uuid and a version).
If the uuid and version would be everytime so closely tied together as you write,
there would simply be no reason to seperate them.

> version is always 0 (that's specified in draft-brown-dcom-v1-spec-03).
> So, I think the right thing is for dcerpc_get_uuid_name to also take a
> version. When you're looking up a uuid that has a version with it
> (e.g., in Bind pdus) you'd pass that along. If there's no version on
> the wire with the uuid (presumably, some dcom scenario) then you'd
> pass in 0 for the version.
>
> Also, some uuid's will never be found in there, as they're not
> associated with interfaces. An example is the object uuid, when
> present. Looking those up doesn't hurt anything, but it does waste
> time.

I disagree. The uuid is simply a kind of an "extended" data type and
used as this for example in several places in DCOM.
DCERPC itself seems to use it mainly for the interfaces, combined always with a version.

If we want to add for example the handling of the object uuids in the future,
we would need to change a lot again :(

Future Goal: a more general uuid hashtable which is not hashing the uuid AND version,
but only hashing the uuid itself, data of that item would look something like:

uuid_hashtable_item {
name
hashtable of interface information (containing the rest of the current data)
}

This would be a more general approach and prevents duplicating this table in the
DCOM dissector.

>
> > Special uuid registration for DCOM subdissectors (I'm not happy with this).
>
> I confess I'm not really clear about what you're trying to do here. I
> guess it's intended to be a way of factoring out common code from what
> the per-interface dissectors will need to do. Honestly, I'm not sure
> that it (adding a separate registration method for dcom interfaces) is
> worth it. If you defined a dissect_ndr_orpc_this(...) and a
> dissect_ndr_orpc_that(...), then subdissectors would just need to add
> one line to each of their dissectors. At which point, the DCOM
> dissectors are just normal DCERPC dissectors that happen to have
> something in common.
You are describing my first implementation ;-)

Which is not the way, I think it should be. If you look at the IDL definitions,
the THIS and THAT pointers are not included there, but are an implicit
thing added by DCOM itself on each call. And these two ARE the protocol elements
added by DCOM (as small as they may be), which are not defined by DCERPC and not
defined by the Microsoft interface IDLs.

But main reason was: The DCOM call would be displayed as a subtree element of the DCERPC
dissector.
Currently the DCOM call is displayed as a toplevel protocol below DCERPC,
which is the way I wanted it.

Another point: why do you want to hide the details, that this is a DCOM call instead of
a DCERPC call.

>
> Ideally, all these subdissectors would be automatically generated from
> IDL, anyway, which would make this irrelevant.

I have a dream... ;-)

Problem here: The Microsoft IDL is not complete and have bugs (or maybe call it features :-).

>
> > Changes in dcerpc-ndr.c:
> > added some simple datatypes: float, double, uuid
>
> I'm a little worried about the possibility of the float and double
> dissecting wrong, since you don't check the drep for the float format
> (as you noted). Actually, I'm not that worried about it, as I've
> never seen an IEEE float on the wire, let alone a VAX one. But I'm
> not necessarily typical. Maybe it's possible to put a "Not
> implemented" message in the tree instead of doing the wrong thing if
> it's not an IEEE float?
>

Sounds good to me.

> > New code implemented decoding the basic DCOM mechanisms (in WinNt4):
> > --------------------------------------------------------------------
> > implementation of a lot DCOM datatypes:
> > -BYTE,WORD,DWORD,...
> > -DATE,FILETIME,BSTR,OBJREF,DUALSTRINGARRAY,...
> > -VARIANT (currently not all varianttypes)
> > -SAFEARRAY (currently not all data types)
>
> Some of these types, esp. VARIANT and SAFEARRAY, are more particular
> to IDispatch than DCOM, per se. Also, I didn't look at them that
> closely, but I couldn't help but feeling that a lot of it could have
> been done through calls to the existing dissect_ndr.. stuff.

The VARIANT is a data type which is often used if you "talking" with Visual Basic,
and it is used a lot in other places of the Microsoft IDL's.
So I think it's in the place where it should be.

I tried to use the dissecting of the arrays and ... failed to use it. I don't
know if I did something wrong or found a bug. So I have implemented some simple
methods and done it that way.

There is no technical reason to do it that way,
this could be changed to use the ndr stuff, if I got:

a. time
b. the right way to use the ndr stuff

>
> > Implementation of the following DCOM-interfaces (still some lesser used methods missing):
> > -IOXIDResolver (now implemented)
> > -IRemoteActivation (now implemented)
>
> Didn't look at them too closely, but I think it makes sense to leave
> these as packet-dcerpc-..., for reasons above.
>
> > -IRemUnknown (newly implemented)
> > -IDispatch (newly implemented, not complete)
>
> Didn't look at these.
>
> > Conclusion:
> > -----------
> > I'm currently not satisfied with my implementation of the DCOM
> > subdissector protocol registration inside packet-dcerpc.c.
> >
> > I need some more example capture files, as there are still a lot of
> > ToBeDone's inside the code.
> >
> > Info: The code has the "ready to use" state, but is maybe not
> > "production stable".
>
> I hope you're not discouraged by my comments. I think you've done a
> lot of valuable work here, and it's just a matter of figuring out how
> best to work it into ethereal.
>
>
> Todd

Thanks for the last sentence (and for your work for the rest of the mail also, of course).
I think you found the points I was expecting to be the things to discuss.

As I have written above I will start to submit patches step by step,
starting with the datatypes float and double.

ULFL
______________________________________________________________________________
WEB.DE Club - jetzt testen fur 1 Euro! Nutzen Sie Ihre Chance 
unter https://digitaledienste.web.de/Club/formular/?mc=021105