[Bro] ssl binpac analyzer -- patches

jmzhou.ml at gmail.com jmzhou.ml at gmail.com
Fri Jun 1 14:08:55 PDT 2007


Oooops, I think there is a potential flaw in my Analysis point 2. Imagine
that a parsing will just consume an incoming packet - nothing more and
nothing less. After the parsing, data_available probably will not be false
because of the way flow buffer is implemented - orig_data_begin_ will only
be increased to orig_data_end_ upon the next request! So, at the while loop,
data_available = true, have_pending_request = true (from last request), and
ready = true (from last request, too). So we end up with entering the loop
again! In fact, to check orig_data_begin < orig_data_end_ in the current 
data_available is wrong from its intention.

I think we need to do something with the flow buffer implementation. The 
current method to increase orig_data_begin_ upon new request is tricky and
difficult to make things correct. My suggestion is to increase orig_data_begin_
in the end() method. The reason is: whenever you request data from the flow
buffer, you always call begin() and end() in sequence. These methods are the
only ways that you can access the data in the flow buffer. After the call
of end(), the decoder will not call begin() and end() again except in one
case which I will address shortly. Thus, it is safe to increase orig_data_begin_
pointer in the end() call. After this change, checking orig_data_begin_ <
orig_data_end_ in data_available() method is correct. BTW, it is not needed
to check buffer_n_ > 0 in data_available(). Because if something is buffered,
and we reach the entry point of the while loop, orig_data_begin_ must be
equal to orig_data_end_. The next parsing must use new data instead of the
data in flow buffer. In addition, in NewFrame and NewLine, there's no need
to increase orig_data_begin_ by frame_length_ after the change of end().

The only case that begin() and end() may be called twice is the &until check
of $input for array elements. The &until check will be done before actual
parsing of array elements. Thus, it needs to access the data, too. I suggest
to add a new method end_no_effect() to flow buffer which does not increase
orig_data_begin_. It is called before the &until check of $input. Moreover,
if the $input check is true, we must call end() in order to force flow buffer
to increase the orig_data_begin_ because there will be no parsing of array
element.

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
------------------------------------------------------------



More information about the Bro mailing list