Wireshark-dev: Re: [Wireshark-dev] Malformed packets in CORBA protocol plugin
Date: Tue, 12 Dec 2006 09:40:23 +0000
Guy Harris <[email protected]> wrote on 11/12/2006 18:50:43:

> [email protected] wrote:
> 
> > In dissect_giop_request_1_2 there is some code that looks like :-
> > 
> >   decode_ServiceContextList(tvb, request_tree, &offset,
> >                   stream_is_big_endian, GIOP_HEADER_SIZE);
> > 
> >   /*
> >    * GIOP 1.2 Request body must fall on an 8 octet alignment, taking 
into
> >    * account we are in a new tvbuff, GIOP_HEADER_SIZE octets from the
> >    * GIOP octet stream start.
> >    */
> > 
> >   set_new_alignment(&offset, GIOP_HEADER_SIZE, 8);
> > 
> > When there are no arguments the service context list is empty
> > so the call to set_new_alignment may and usually does, set
> > the offset beyond the length of the buffer.
> 
> Is that "empty" as in "the service context is absent" or "empty" as in 
> "the sequence length in the service context is 0"?
> 

The sequence length is 0.
After the call to decode_ServiceContextList the difference
between offset and tvb->(reported_)length is 0

After the call to set_new_alignment offset gets set beyond
the end of the buffer. In my test case, by 4 bytes.

> > proto_tree_add_item is designed to cope with offset equal to
> > the buffer length, but not beyond it. So it throws.
> 
> It *is* designed to cope with offset beyond the buffer length - an 
> offset beyond the buffer length is an obvious error, so it throws an 
> exception.
> 

Except in this case it is not an error. So it is not always "obvious"

> > A fairly "safe" fix is to change start_dissecting in wireshark_gen.py
> > to look like :-
> > 
> > static proto_tree *start_dissecting(tvbuff_t *tvb, packet_info *pinfo, 

> > proto_tree *ptree, int *offset) {
> > 
> >     proto_item *ti = NULL;
> >     proto_tree *tree = NULL;            /* init later, inside if(tree) 
*/
> > 
> >     if (check_col(pinfo->cinfo, COL_PROTOCOL))
> >         col_set_str(pinfo->cinfo, COL_PROTOCOL, "Q_QUENTINV3");
> > 
> >     /*
> >      * Do not clear COL_INFO, as nothing is being written there by
> >      * this dissector yet. So leave it as is from the GIOP dissector.
> >      * TODO: add something useful to COL_INFO
> >      *  if (check_col(pinfo->cinfo, COL_INFO))
> >      *     col_clear(pinfo->cinfo, COL_INFO);
> >      */
> > 
> > +    if (tvb_length_remaining(tvb, *offset) < 0)
> > +      return NULL ;
> 
> If the idea is that, if there are no arguments, the dissector should 
> quit, the problem is that, in that case, if there are *supposed* to be 
> arguments, no exception will be thrown if they're missing, so no 
> indication that a request that should have arguments will show up.
> 

True.

> I would, instead, have start_dissecting() take a gboolean argument 
> indicating whether the are any arguments to be dissected and, if so, 
> have it return after setting the protocol column.

Now it's getting beyond my abilities with python etc. I'm not sure
if omniidl and the python back-end can detect methods with & without
arguments to call start_dissecting correctly. Which is why I'm asking
for some help ;-)

Thanks for the input

Andy Ling