[Bro] ssl binpac analyzer -- patches

[jmzhou.ml at gmail.com]

Yes, the changes work well at my side.

Some problems of binpac:

. Split binpac out of bro source tree. I think to make binpac standalone
makes testing/developing much easier. One can develop new analyzers and
test them with dedicated .pcap files.

. Binpac does not support SunRPC over TCP now. There are four extra bytes
prepended in RPC packets. Either TCP layer decoder should take care of 
these extra bytes, or the RPC decoder has to do something with it. In 
addition, &exportsourcedata is used in RPC/UDP decoder based on datagram
mode. It is not supported by flowunit mode. This means, we cannot simply
change the decoder from datagram mode to flowunit mode for RPC/TCP.
Finally, a Ref call is missing in the NewCall function when a call already
exists, and a Unref call is not correctly called in FinishCall. The
Ref/Unref calls also need to be re-written. These changes are necessary
to properly support retransmission. The problem in the original code is:
if you get two identical calls followed by two identical replies, the 
decoder will fail on the second reply. I met with this problem at my site.
I give the rough idea of the fix below:

     function NewCall (xid: uint32, call: RPC_Call): bool
     %{
          if (find_orig_call (xid))
          {
              handle_mismatch_between_current_call_and_previous_call;
              orig_call->msg()->Ref();
              return false;
          }
          else
          {
              insert_new_call_into_call_table (xid);
              new_call->msg()->Ref();
          }
     %}

     function FinishCall (call: RPC_Call): void
     %{
          xid = call->msg()->xid();  /* save it because msg may be freed below */
          int  refcount = Unref(call->msg());
          if (refcount <= 0)
              erase_xid_from_call_table;
     %}

     class RefCount
     {
         int Ref() { return ++count;}
         int Unref() {return -- count;}
         ...
     }

     inline int Unref (RefCount* x)
     {
         int   refcount = 0;
         if (x)
         {
              refcount = x->Unref();
              if (refcount <= 0)
                 delete x;
         }
         return refcount;
     }


Regards,

Jimmy


On Wed, 30 May 2007, Tobias Kiesling wrote:

> Jimmy,
>
> your suggestion sounds good. I implemented the changes and tested them with
> my ssl analyzer. Everything worked as expected. :-)
>
> Attached you can find the patch for these changes (to be applied with -p0 in
> the bro directory, after applying all of the other binpac patches that I
> provided before). Could you test this further with whatever binpac analyzers
> you have got?
>
> Anyone else any comments on this issue (Ruoming)?
>
> Tobias
>
>
> On 5/30/07, jmzhou.ml at gmail.com <jmzhou.ml at gmail.com> wrote:
>> 
>> On Tue, 29 May 2007, Tobias Kiesling wrote:
>> 
>> > On 5/29/07, jmzhou.ml at gmail.com <jmzhou.ml at gmail.com> wrote:
>> >
>> >> Patch 6: I think there is another issue here: if some buffering request
>> >> has been sent to the flow buffer, and the flow buffer is not ready with
>> >> partial data, we will end up with entering the loop - a waste of CPU
>> >> cycles.
>> >
>> >
>> > Maybe you are right, but what would you suggest as change? Do you want
>> to
>> > check whether the buffer is ready before entering the loop? But then it
>> has
>> > to be ensured that the buffer is properly initialized in any case.  At
>> the
>> > moment I cannot see all the consequences of such a change. And do you
>> think
>> > that the impact on performance is really relevant?
>> 
>> The cost can be a little bit expensive if there are many layers of
>> parsing.
>> You end up with many unnecessary calls to parsing functions and condition
>> jumps. One possible approach is like this:
>> 
>> . add a new boolean member have_pending_request to FlowBuffer, initialized
>> as false.
>> 
>> . set have_pending_request to true in call NewFrame and NewLine.
>> 
>> . reset have_pending_request to false in call DiscardData.
>> 
>> . change the while loop condition to:
>>     while (flow_buffer->data_available() &&
>>         (!flow_buffer->have_pending_request() || flow_buffer->ready()))
>> 
>> Analysis:
>>
>>   1. The first time, data_available = true, !have_pending_request = true,
>> we enter into the loop. Good.
>>
>>   2. All data are consumed, data_available = false, we do not enter into
>> the loop. Good.
>>
>>   3. A request is not satisfied because of partial data: data_available =
>> true, !have_pending_request = false, ready = false, we do not enter into
>> the loop. Good.
>>
>>   4. Parsing is forced to stop because of exception. data_available =
>> false.
>> We do not enter into the loop. Good.
>>
>>   5. Parsing current dataunit has finished, we still have residual data -
>> wrong data? data_available = true, !have_pending_request = false, ready =
>> true (from last parsing). We enter into the loop, and start a new round of
>> parsing. As expected. Good.
>> 
>> So far so good. :-)
>> 
>> Jimmy
>> 
>> ____________________________________________________________
>> The future is not set.  There is no fate but what we make
>> for ourselves.             - Terminator II, Judgment Day
>> ------------------------------------------------------------
>> 
>> 
>

____________________________________________________________
The future is not set.  There is no fate but what we make
for ourselves.             - Terminator II, Judgment Day
------------------------------------------------------------