I'm developping support for ICMPv6 in Bro and I have a problem with an
include I can't solve.
I have just added "#include <netinet/icmp6.h>" in net_util.h and I get
errors not (apparently) related to this change :
if g++ -DHAVE_CONFIG_H -I. -I. -I.. -I. -I../src/binpac/lib -I../src
-I. -I.. -Ilibedit -I../linux-include -O -W -Wall -Wno-unused
-I../linux-include -g -O2 -MT dns_pac.o -MD -MP -MF ".deps/dns_pac.Tpo"
-c -o dns_pac.o dns_pac.cc; \
then mv -f ".deps/dns_pac.Tpo" ".deps/dns_pac.Po"; else rm -f
".deps/dns_pac.Tpo"; exit 1; fi
../src/Analyzer.h:100: warning: 'class Analyzer::OutputHandler' has
virtual functions but non-virtual destructor
../src/dns_pac.h:328: error: expected ',' or '...' before '.' token
../src/dns_pac.h:424: error: expected ';' before '.' token
../src/dns_pac.h:425: error: expected `;' before 'uint16'
../src/dns_pac.h:453: error: expected ';' before '.' token
../src/dns_pac.h:454: error: expected `;' before 'uint16'
dns_pac.cc:535: error: expected ',' or '...' before '.' token
dns_pac.cc: In constructor
'binpac::DNS::DNS_rdata::DNS_rdata(binpac::DNS::DNS_message*,
binpac::uint16)':
dns_pac.cc:547: error: request for member 'icmp6_type' in 'rr_hdr',
which is of non-class type 'binpac::uint16'
dns_pac.cc:548: error: argument of type 'binpac::uint16
(binpac::DNS::DNS_rdata::)()const' does not match 'binpac::uint16'
dns_pac.cc: In member function 'int binpac::DNS::DNS_rdata::Parse(const
binpac::uint8*, const binpac::uint8*, binpac::DNS::ContextDNS*, int)':
dns_pac.cc:614: error: 'rr_hdr' was not declared in this scope
dns_pac.cc: In member function 'int binpac::DNS::DNS_rr::Parse(const
binpac::uint8*, const binpac::uint8*, binpac::DNS::ContextDNS*, int)':
dns_pac.cc:779: error: 'rr_hdr' was not declared in this scope
dns_pac.cc: In member function 'Val*
binpac::DNS::DNS_Flow::build_dns_answer(binpac::DNS::DNS_rr*)':
dns_pac.cc:1244: error: 'class binpac::DNS::DNS_rr' has no member named
'rr_hdr'
dns_pac.cc: In member function 'Val*
binpac::DNS::DNS_Flow::build_edns_additional(binpac::DNS::DNS_rr*)':
dns_pac.cc:1280: error: 'class binpac::DNS::DNS_rr' has no member named
'rr_hdr'
dns_pac.cc: In member function 'bool
binpac::DNS::DNS_Flow::process_dns_rr(binpac::DNS::DNS_rr*)':
dns_pac.cc:1311: error: 'class binpac::DNS::DNS_rr' has no member named
'rr_hdr'
dns_pac.cc:1320: error: 'class binpac::DNS::DNS_rr' has no member named
'rr_hdr'
I have made this modification in the bro-1.2.1 sources without any other
changes.
I have the same problem on GNU/Linux and OpenBSD 4.
Does anyone have an idea how to solve/debug this problem ?
I have just added "#include <netinet/icmp6.h>" in net_util.h and I get
errors not (apparently) related to this change :
...
../src/dns_pac.h:328: error: expected ',' or '...' before '.' token
...
Does anyone have an idea how to solve/debug this problem ?
My first suggestion would be to compile using g++ -E to get the output
from the preprocessor (sent to stdout). Often a bizarre problem like this
after including a header is because the header introduces a #define that
conflicts with a name used elsewhere in a different context.
I have just added "#include <netinet/icmp6.h>" in net_util.h and I get
errors not (apparently) related to this change :
...
../src/dns_pac.h:328: error: expected ',' or '...' before '.' token
...
Does anyone have an idea how to solve/debug this problem ?
My first suggestion would be to compile using g++ -E to get the output
from the preprocessor (sent to stdout). Often a bizarre problem like this
after including a header is because the header introduces a #define that
conflicts with a name used elsewhere in a different context.
Indeed it was a #define that conflicted with another variable : rr_type.
The problem now is that I need to include icmp6.h but it conflicts with dns_pac.
What do you think is the best practice to solve this issue : rename the variable in dns_pac, include a modified version of icmp6.h with Bro... ?
What do you think is the best practice to solve this issue : rename the
variable in dns_pac, include a modified version of icmp6.h with Bro... ?
The first question is whether the icmp6.h include really needs to be in
net_util.h, or if it could be confined to the source file(s) that use the
definitions.
If it does, then following the include with #undef rr_type (and a comment
as to why) is probably the way to go, ugly as it is.
What do you think is the best practice to solve this issue : rename the variable in dns_pac, include a modified version of icmp6.h with Bro... ?
The first question is whether the icmp6.h include really needs to be in
net_util.h, or if it could be confined to the source file(s) that use the
definitions.
Actually I just need it in Sessions.cc (for the switch on the ICMP type) so I will use the #undef solution : it's less ugly than keeping a modified version of icmp6.h.
Actually I just need it in Sessions.cc (for the switch on the ICMP type)
so I will use the #undef solution : it's less ugly than keeping a
modified version of icmp6.h.
I think you misunderstood me. If you just need it in Sessions.cc, then
just include it in Sessions.cc rather than in net_util.h (and include it
after DNS-binpac.h, with a comment, if that's the only conflict). A little
better than using an #undef.
just include it in Sessions.cc rather than in net_util.h (and include it
after DNS-binpac.h, with a comment, if that's the only conflict). A little
better than using an #undef.
Patch 4: I can confirm that this is a bug. I had the same problem and
same fix at my side.
Patch 5: I think the new call (DoGenParseCode) in pac_type.cc should be
replaced by GenParseCode3. Imagine that a record field (identifier f1) is empty type and there are other fields (identifier f2, f3, ...) after
field f1. If you call DoGenParseCode, the identifier f1 will not be
evaluated like in GenParseCode3 (pac_type.cc:754-755). This will result
in re-entrance of RecordDataField::GenParseCode (pac_record.cc:420) because
when f2->prev()-GenParseCode (pac_record.cc:428) is called, env->Evaluated(
id()) will return false. This is an infinite loop - till at some point of
binpac there will be an assertion failure.
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.
Patch 4: I can confirm that this is a bug. I had the same problem and
same fix at my side.
Patch 5: I think the new call (DoGenParseCode) in pac_type.cc should be
replaced by GenParseCode3. Imagine that a record field (identifier f1)
is empty type and there are other fields (identifier f2, f3, …) after
field f1. If you call DoGenParseCode, the identifier f1 will not be
evaluated like in GenParseCode3 (pac_type.cc:754-755). This will result
in re-entrance of RecordDataField::GenParseCode (pac_record.cc:420) because
when f2->prev()-GenParseCode (pac_record.cc:428) is called, env->Evaluated(
id()) will return false. This is an infinite loop - till at some point of
binpac there will be an assertion failure.
I think I can see your point and also that it might generally be more safe to call GenParseCode3 instead (I attached the updated patch file).
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.
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?
. 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 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;
}
That's actually happening. Please stay tuned. Also, if you have
recommendation on a fast regexp library with BSD-like license, please
let me know. Note that we do not need perl-like captures, but only the
longest match.
I think to make binpac standalone
makes testing/developing much easier. One can develop new analyzers and
test them with dedicated .pcap files.
I agree likewise. In fact, one way to significantly improve testing in
binpac is to make a (proof-of-concept) script when an issue arises.
Such as in this case... By keeping this scripts around we can make
sure that old problems do not surface again.
. 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.
The way I imagine doing this is to consider RPC on TCP a trivial
lower-level protocol than RPC on UDP. For each RPC-on-TCP message, the
analyzer calls the datagram mode RPC analyzer's NewData() routine.
What do you think?
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.
First let's look at the case of RPC/UDP, it is possible that a RPC message
is transmitted in multiple IP packets due to IP fragmentation. Either the
lower layer should reassemble the entire RPC message from IP fragments, or
just it sends the fragment pieces to the RPC decoder in order. In the second
scenario, the RPC decoder has to use flowunit mode.
In the case of RPC/TCP, the first four bytes specifies the length of a RPC
record (not necessarily be a complete RPC message from my understanding of
the RPC RFC). It is called Record Marker. Because of the stream nature of
TCP, the RPC record may be transmitted in multiple TCP packets. Although
from some traces, I see that a TCP packet always contains the four bytes
and the complete RPC message. I do not think we can make a safe assumption
that this is always true. In addition, if a RPC record is not a complete
RPC message, this introduces another problem to the RPC decoder. A complete
RPC message may be transmitted in two records, and each record is prepended
with a Record Marker. The position to split the two records can be arbitrary!
I do not see a solution in the current binpac language to handle this kind
of cases.
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.
> The way I imagine doing this is to consider RPC on TCP a trivial
> lower-level protocol than RPC on UDP. For each RPC-on-TCP message, the
> analyzer calls the datagram mode RPC analyzer's NewData() routine.
> What do you think?
I think the problem is not that simple.
Hi, Jimmy,
Maybe I wasn't clear enough, but I don't understand the problems you
raised below.
What I mean by a "RPC-on-TCP" message is a complete RPC message,
rather than a fragment. Maybe that's where the confusion is. In any
case, I think the thin RPC-over-TCP layer can be fully isolated from
the datagram-mode RPC analyzer, as long as the RPC analyzer receives
only complete RPC messages. The reassembly has to be done in C++, but
it's relatively trivial.
First let's look at the case of RPC/UDP, it is possible that a RPC message
is transmitted in multiple IP packets due to IP fragmentation. Either the
lower layer should reassemble the entire RPC message from IP fragments, or
just it sends the fragment pieces to the RPC decoder in order.
binpac analyzers do not see IP fragments. They should all be
reassembled at lower level, as in Bro. It's the same story at TCP
level and at RPC-over-TCP level.
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 was well aware of that fact, but did not see a problem here, as in my SSL analyzer I manually assure that after parsing a data packet, the corresponding data is consumed. In that case, data_available() works just as intended.
I don’t think, that it is a good idea to introduce side effects into the end() function just for a slight increase in performance. On one hand, the end() function is in the public interface of the buffer and you should never significantly change the behavior of a public method (some binpac users might rely on that behavior), except for very good reasons. On the other hand, in cases where there might be a serious performance problem with the current behavior, the programmer can manually avoid this by explicitly consuming the data, like I do in my analyzer (of course this should be documented ;-).
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().
Again, data_available() is a public method that is not just intended for the single use we have discussed so far (I use it explicitly in my analyzer). You can always use it to check whether there is currently any data available, and of course this includes the buffer.
