Discussion:
Malformed packets in CORBA protocol plugin
Andy.Ling-JC/
18 years ago
Permalink
I originally posted this to the user list, but it was suggested
this is a better forum.

I am having problems with a CORBA protocol plugin. I have generated
the .c file using :-

omniidl -p c:\wireshark-0.99.3a\tools -b wireshark_be q_quentinv3.idl >
packet-quentinv3.c

and built and installed the .dll without problems.

But when I enable the protocol analyser, any IDL method that doesn't
have any arguments is marked as [Malformed Packet: Q_QUENTINV3]
Those with arguments have the arguments decoded correctly and
are not marked with any error.

If I disable my protocol analyser then no packets show errors.

I'm using version 0.99.3a on Win2K

Thanks for any help

Andy Ling
Anders Broman (AL/EAB)
18 years ago
Permalink
Hi,
Perhaps a fault in the GIOP dissector. Can you send the text output of
the failed decoding?
BR
Anders

-----Original Message-----
From: wireshark-dev-***@wireshark.org
[mailto:wireshark-dev-***@wireshark.org] On Behalf Of
***@quantel.com
Sent: den 7 december 2006 10:53
To: wireshark-***@wireshark.org
Subject: [Wireshark-dev] Malformed packets in CORBA protocol plugin

I originally posted this to the user list, but it was suggested this is
a better forum.

I am having problems with a CORBA protocol plugin. I have generated the
.c file using :-

omniidl -p c:\wireshark-0.99.3a\tools -b wireshark_be q_quentinv3.idl >
packet-quentinv3.c

and built and installed the .dll without problems.

But when I enable the protocol analyser, any IDL method that doesn't
have any arguments is marked as [Malformed Packet: Q_QUENTINV3] Those
with arguments have the arguments decoded correctly and are not marked
with any error.

If I disable my protocol analyser then no packets show errors.

I'm using version 0.99.3a on Win2K

Thanks for any help

Andy Ling
_______________________________________________
Wireshark-dev mailing list
Wireshark-***@wireshark.org
http://www.wireshark.org/mailman/listinfo/wireshark-dev
Andy.Ling-JC/
18 years ago
Permalink
Post by Anders Broman (AL/EAB)
Hi,
Perhaps a fault in the GIOP dissector. Can you send the text output of
the failed decoding?
BR
Anders
I'm not 100% sure which bit you are after, but the packet
bytes look like :-

Frame 199 (130 bytes on wire, 130 bytes captured)

0000 00 01 af 15 fd df 00 30 48 12 04 d4 08 00 45 00 .......0H.....E.
0010 00 74 11 d2 40 00 80 06 9a e6 0a a5 0b 78 0a a5 ***@........x..
0020 2d 0a 04 87 04 04 20 52 7c 07 0d a9 71 d6 50 18 -..... R|...q.P.
0030 fd bb 8e 33 00 00 47 49 4f 50 01 02 01 00 40 00 ***@.
0040 00 00 ec 00 00 00 03 00 00 00 00 00 00 00 1b 00 ................
0050 00 00 14 01 0f 00 52 53 54 45 6d a5 36 00 05 98 ......RSTEm.6...
0060 4a 00 00 00 01 00 00 00 01 00 00 00 02 00 0b 00 J...............
0070 00 00 67 65 74 52 65 66 54 69 6d 65 00 00 00 00 ..getRefTime....
0080 00 00 ..

And the decode window above shows:-

General Inter-ORB Protocol Request
Request id: 236
Response flags: SYNC_WITH_TARGET (3)
Reserved: 0 0 0
TargetAddress Discriminant: 0
KeyAddr (object key length): 27
KeyAddr (object key): ....RSTEm.6...J............
Operation length: 11
Request operation: getRefTime
ServiceContextList
Sequence Length: 0
[Malformed Packet: Q_QUENTINV3]

If I turn off our Q_QUENTINV3 protocol then the last line is not printed.

Another bit of information that might help. If I set the filter to giop
then the info in the main window looks like :-

Q_QUENTINV3 GIOP 1.2 Request 236: getRefTime[Malformed Packet]

Without the giop filter the "[Malformed Packet]" string is missing

Regards

Andy Ling
Anders Broman (AL/EAB)
18 years ago
Permalink
Hi,
You should try to see in packet-giop.c what happens after the output of:
ServiceContextList
Sequence Length: 0

My guess is that a sequence length of zero isn't handled properly. I have little time to look at this currently...

BR
Anders

________________________________

Från: wireshark-dev-bounces-IZ8446WsY0/***@public.gmane.org genom Andy.Ling-JC/***@public.gmane.org
Skickat: to 2006-12-07 14:56
Till: Developer support list for Wireshark
Ämne: Re: [Wireshark-dev] Malformed packets in CORBA protocol plugin
Post by Anders Broman (AL/EAB)
Hi,
Perhaps a fault in the GIOP dissector. Can you send the text output of
the failed decoding?
BR
Anders
I'm not 100% sure which bit you are after, but the packet
bytes look like :-

Frame 199 (130 bytes on wire, 130 bytes captured)

0000 00 01 af 15 fd df 00 30 48 12 04 d4 08 00 45 00 .......0H.....E.
0010 00 74 11 d2 40 00 80 06 9a e6 0a a5 0b 78 0a a5 ***@........x..
0020 2d 0a 04 87 04 04 20 52 7c 07 0d a9 71 d6 50 18 -..... R|...q.P.
0030 fd bb 8e 33 00 00 47 49 4f 50 01 02 01 00 40 00 ***@.
0040 00 00 ec 00 00 00 03 00 00 00 00 00 00 00 1b 00 ................
0050 00 00 14 01 0f 00 52 53 54 45 6d a5 36 00 05 98 ......RSTEm.6...
0060 4a 00 00 00 01 00 00 00 01 00 00 00 02 00 0b 00 J...............
0070 00 00 67 65 74 52 65 66 54 69 6d 65 00 00 00 00 ..getRefTime....
0080 00 00 ..

And the decode window above shows:-

General Inter-ORB Protocol Request
Request id: 236
Response flags: SYNC_WITH_TARGET (3)
Reserved: 0 0 0
TargetAddress Discriminant: 0
KeyAddr (object key length): 27
KeyAddr (object key): ....RSTEm.6...J............
Operation length: 11
Request operation: getRefTime
ServiceContextList
Sequence Length: 0
[Malformed Packet: Q_QUENTINV3]

If I turn off our Q_QUENTINV3 protocol then the last line is not printed.

Another bit of information that might help. If I set the filter to giop
then the info in the main window looks like :-

Q_QUENTINV3 GIOP 1.2 Request 236: getRefTime[Malformed Packet]

Without the giop filter the "[Malformed Packet]" string is missing

Regards

Andy Ling
Andy.Ling-wBXi8Sah/sOQVHkhH/
18 years ago
Permalink
Post by Andy.Ling-JC/
Hi,
ServiceContextList
Sequence Length: 0
My guess is that a sequence length of zero isn't handled properly. I
have little time to look at this currently...
BR
Anders
I'm struggling a little bit here.

packet-giop.c prints out all the giop stuff including the Sequence Length
field then calls the giop dissector using either
try_explicit_giop_dissector or try_heuristic_giop_dissector.

It looks like it is the heuristic one that is accepting the decode.

This calls the dissect method in my dissector, which appears to recognise
the getRefTime call in my example and returns TRUE having done very
little.

this then returns to packet-giop.c which calls CLEANUP_CALL_AND_POP

So which is the bit that generates the "Malformed packet" message?
I can't see anything in my dissector, so is it done by the cleanup
call or something later on?

Any help greatfully received

Andy Ling
Post by Andy.Ling-JC/
Hi,
Perhaps a fault in the GIOP dissector. Can you send the text output of
the failed decoding?
BR
Anders
I'm not 100% sure which bit you are after, but the packet
bytes look like :-

Frame 199 (130 bytes on wire, 130 bytes captured)

0000 00 01 af 15 fd df 00 30 48 12 04 d4 08 00 45 00 .......0H.....E.
0010 00 74 11 d2 40 00 80 06 9a e6 0a a5 0b 78 0a a5 ***@........x..
0020 2d 0a 04 87 04 04 20 52 7c 07 0d a9 71 d6 50 18 -..... R|...q.P.
0030 fd bb 8e 33 00 00 47 49 4f 50 01 02 01 00 40 00 ***@.
0040 00 00 ec 00 00 00 03 00 00 00 00 00 00 00 1b 00 ................
0050 00 00 14 01 0f 00 52 53 54 45 6d a5 36 00 05 98 ......RSTEm.6...
0060 4a 00 00 00 01 00 00 00 01 00 00 00 02 00 0b 00 J...............
0070 00 00 67 65 74 52 65 66 54 69 6d 65 00 00 00 00 ..getRefTime....
0080 00 00 ..

And the decode window above shows:-

General Inter-ORB Protocol Request
Request id: 236
Response flags: SYNC_WITH_TARGET (3)
Reserved: 0 0 0
TargetAddress Discriminant: 0
KeyAddr (object key length): 27
KeyAddr (object key): ....RSTEm.6...J............
Operation length: 11
Request operation: getRefTime
ServiceContextList
Sequence Length: 0
[Malformed Packet: Q_QUENTINV3]

If I turn off our Q_QUENTINV3 protocol then the last line is not printed.

Another bit of information that might help. If I set the filter to giop
then the info in the main window looks like :-

Q_QUENTINV3 GIOP 1.2 Request 236: getRefTime[Malformed Packet]

Without the giop filter the "[Malformed Packet]" string is missing

Regards

Andy Ling
Andy.Ling-wBXi8Sah/sOQVHkhH/
18 years ago
Permalink
Post by Andy.Ling-JC/
Hi,
ServiceContextList
Sequence Length: 0
My guess is that a sequence length of zero isn't handled properly. I
have little time to look at this currently...
OK, I now know where it is going wrong, but I don't know the right way
to fix it.

In my packet-quentinv3.c file the code generated has a function
called start_dissecting. This gets called for every recognised
packet and is throwing a ReportedBoundsError exception.

It looks like this is because it is trying to add a new tree item
for a 0 length sequence, so it shouldn't really need to.

Following this through. The call tree that throws the exception is :-

start_dissecting ->
proto_tree_add_item ->
alloc_field_info ->
get_hfi_and_length ->

In get_hfi_and_length the code that throws reads :-

switch (hfinfo->type) {

case FT_PROTOCOL:
/*
* We allow this to be zero-length - for
* example, an ONC RPC NULL procedure has
* neither arguments nor reply, so the
* payload for that protocol is empty.
*
* However, if the length is negative, the
* start offset is *past* the byte past the
* end of the tvbuff, so we throw an
* exception.
*/
*length = tvb_length_remaining(tvb, start);
if (*length < 0) {
/*
* Use "tvb_ensure_bytes_exist()"
* to force the appropriate exception
* to be thrown.
*/
tvb_ensure_bytes_exist(tvb, start, 0);
}

For the case where it throws *length = -1

I now need someone who undall this code to advise the best place to
fix it. I guess the dissector shouldn't add a tree item if there are
no arguments, but that requires changes to the python that generates
that file.

Thanks for any help

Andy Ling
Andy.Ling-JC/
18 years ago
Permalink
First a bit of history for those not keeping up.

I have found that any CORBA dissector built using the
omniidl / python stuff marks a packet as Malformed
for CORBA 1.2 requests that don't have any arguments.

I have tracked the problem down to a ReportedBoundsError
exception being thrown by start_dissecting calling
proto_tree_add_item when there are no arguments to
be added.

It looks like this is a CORBA 1.2 problem and is being
caused by packet-giop.c

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.

proto_tree_add_item is designed to cope with offset equal to
the buffer length, but not beyond it. So it throws.

There are several ways to fix this and I don't know enough about
the code to know which is the best.

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 (ptree) {
......


Regards

Andy Ling
Guy Harris
18 years ago
Permalink
...
Is that "empty" as in "the service context is absent" or "empty" as in
"the sequence length in the service context is 0"?
Post by Andy.Ling-JC/
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.
...
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.

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.
Andy.Ling-wBXi8Sah/sOQVHkhH/
18 years ago
Permalink
...
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.
Post by Guy Harris
Post by Andy.Ling-JC/
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"
...
True.
Post by Guy Harris
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
Guy Harris
18 years ago
Permalink
Post by Andy.Ling-wBXi8Sah/sOQVHkhH/
Post by Guy Harris
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.
Yes, it is. It makes no sense whatsoever to add a protocol tree item
that refers to data past the end of the packet, as that data doesn't
exist. It's designed to cope with the offset being beyond the buffer
length by throwing an exception to report an error. If the packet
isn't erroneous, then the bug isn't that proto_tree_add_item() isn't
doing what it should be doing (it's doing exactly what it should be
doing), it's that it shouldn't be called with that offset in the first
place.
Andy.Ling-wBXi8Sah/sOQVHkhH/
18 years ago
Permalink
...
Fine, but then why should it cope with an offset of 0. You shouldn't be
trying to add a protocol tree item when the offset is 0, there's
no data there either.

We could discuss the semantics of this all day, but it's not fixing
the problem. I have perfectly good CORBA packets that are showing
up as errors. I have spent a fair bit of time finding out why and
now I'd like some help to find the best way to fix it.

I agree with you that proto_tree_add_item should not be called if
there is nothing to add and I said as much in one of my earlier
posts, but the current code is based on the principle that this
can be done. To change this requires changing the python code
generator that creates the dissector source from the CORBA idl.
This is outside my experience. I also don't understand the
detail of the wireshark code to know the best way to fix this
problem.

So what is the best way to move forward on this?

Regards

Andy Ling
Guy Harris
18 years ago
Permalink
Post by Andy.Ling-wBXi8Sah/sOQVHkhH/
Fine, but then why should it cope with an offset of 0. You shouldn't be
trying to add a protocol tree item when the offset is 0, there's
no data there either.
Eh? An offset of 0 just means "beginning of the packet".

If you mean "length of 0", then a length of 0 is used in two places:

1) generated items, where neither the offset nor the length are meaningful;

2) the top-level item for a protocol if the amount of data available
for the protocol is 0 - that allows the top-level item to be added, with
the exception thrown when you create the sub-item.
Post by Andy.Ling-wBXi8Sah/sOQVHkhH/
We could discuss the semantics of this all day, but it's not fixing
the problem. I have perfectly good CORBA packets that are showing
up as errors. I have spent a fair bit of time finding out why and
now I'd like some help to find the best way to fix it.
I agree with you that proto_tree_add_item should not be called if
there is nothing to add and I said as much in one of my earlier
posts, but the current code is based on the principle that this
can be done.
It's not as if somebody necessarily explicitly said "I'll write this on
the assumption that there are always parameters to a call" - it might
have been written without bearing in mind that there aren't (i.e., it's
not as if it's a Fundamental Design Decision).

I'd suggest moving the start_dissecting() calls into the routines that
process individual procedure requests and replies, and avoid generating
those calls if there are no items expected.

See, for example, genOperationRequest() in wireshark_gen.py; it could,
for example, start out by setting a flag to false and, before the
self.getCDR3() call, if the flag is false, put out the
start_dissecting() call and set the flag to true, so that the
start_dissecting() call is put out before the first parameter.
Andy.Ling-wBXi8Sah/sOQVHkhH/
18 years ago
Permalink
Post by Guy Harris
Post by Andy.Ling-wBXi8Sah/sOQVHkhH/
Fine, but then why should it cope with an offset of 0. You shouldn't be
trying to add a protocol tree item when the offset is 0, there's
no data there either.
Eh? An offset of 0 just means "beginning of the packet".
Sorry. What I meant was offset & buffer length are equal. i.e we've
reached the end of the buffer and there is no more data.
Post by Guy Harris
1) generated items, where neither the offset nor the length are meaningful;
2) the top-level item for a protocol if the amount of data available
for the protocol is 0 - that allows the top-level item to be added, with
the exception thrown when you create the sub-item.
So there are other good reasons for accepting 0 length that I didn't know
about. That's why I need the help.
...
I'm not suggesting that was the design decision. It might be just luck
that
it has worked so far. The point is, the only reason CORBA 1.0 requests
with no parameters don't generate errors it because the 0 length case
is not considered an error. Presumably if they had produced errors it
would have been fixed before now.
...
OK, that looks promising. I will investigate

Thanks

Andy Ling
Andy.Ling-wBXi8Sah/sOQVHkhH/
18 years ago
Permalink
Post by Guy Harris
I'd suggest moving the start_dissecting() calls into the routines that
process individual procedure requests and replies, and avoid generating
those calls if there are no items expected.
I've just started having a closer look at this and I have a question.

It looks like start_dissecting does a bit more than just set up the
proto tree. It calls check_col and col_set_str.

So is it safe to not call it when there are no items or would it
be better to add a parameter to start_dissecting to stop it trying
to add the proto tree if there are no items.

Thanks for any help

Andy Ling
Andy.Ling-wBXi8Sah/sOQVHkhH/
18 years ago
Permalink
Post by Guy Harris
I'd suggest moving the start_dissecting() calls into the routines that
process individual procedure requests and replies, and avoid generating
those calls if there are no items expected.
I've looked at this a bit deeper now and this looks really messy.
The decision to call start_dissecting isn't based just on the
number of parameters. It depends whether it is a request or
a reply and if it is a reply whether that returns anything or if
it is an exception. So I want to propose a different solution.

What if packet-giop.c is changed so that dissect_giop_request_1_2
changes to look like :-

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

+ if (tvb_length_remaining(tvb, offset) > 0)
set_new_alignment(&offset, GIOP_HEADER_SIZE, 8);

This stops the offset getting moved past the end of the buffer
when there is no more data.

Can anyone see any problem with this?

Regards

Andy Ling
Guy Harris
18 years ago
Permalink
Post by Andy.Ling-wBXi8Sah/sOQVHkhH/
Can anyone see any problem with this?
1) as indicated in my earlier mail, it won't throw an exception if you
have a bad packet that should have a request body but doesn't.

2) if you ignore that, it should be tvb_reported_length_remaining(),
because tvb_length_remaining() gives the amount of *captured* data, not
the amount of *actual* data, and you want to throw an exception if the
capture was cut short by a snapshot length and that meant that the
request body was present on the wire but wasn't in the capture.
Andy.Ling-wBXi8Sah/sOQVHkhH/
18 years ago
Permalink
Post by Guy Harris
Post by Andy.Ling-wBXi8Sah/sOQVHkhH/
Can anyone see any problem with this?
1) as indicated in my earlier mail, it won't throw an exception if
you
have a bad packet that should have a request body but doesn't.
Surely it will still throw later on if a dissector tries to read the
body. This will take the offset beyond the end of the buffer.
What I am proposing will make a 1.2 request packet with no body behave
exactly the same as a 1.0 request packet with no body.
Post by Guy Harris
2) if you ignore that, it should be tvb_reported_length_remaining(),
because tvb_length_remaining() gives the amount of *captured* data,
not
the amount of *actual* data, and you want to throw an exception if
the
capture was cut short by a snapshot length and that meant that the
request body was present on the wire but wasn't in the capture.
OK, I'll accept it should use the reported length.

So is this likely to get accepted as an "official" patch?

Thanks for the help

Andy Ling

Loading...