A while ago we've been discussing adding a new "async" keyword to run
certain functions asynchronously; Bro would proceed with other script
code until they complete. Summary is here:
https://www.bro.org/development/projects/broker-lang-ext.html#asynchronous-…
After my original proof-of-concept version, I now have a 2nd-gen
implementation mostly ready that internally unifies the machinery for
"when" and "async". That simplifies the code and, more importantly,
makes the two work together more smoothly. The branch for that work is
topic/robin/sync, I'll clean that up a bit more and would then be
interested in seeing somebody test drive it.
In the meantime I want to propose a slight change to the original
plan. In earlier discussions, we ended up concluding that introducing
a new keyword to trigger the asynchronous behaviour is useful for the
script writer, as it signals that semantics are different for these
calls. Example using the syntax we arrived at:
event bro_init()
{
local x = async lookup_hostname("www.icir.org"); # A
print "IP of www.icir.org is", x; # B
}
Once the DNS request is issued in line A, the event handler will be
suspended until the answer arrives. That means that other event
handlers may execute before line B, i.e., execution order isn't fully
deterministic anymore. The use of "async" is pointing out that
possibility.
However, look at the following example. Let's say we want to outsource
such DNS functionality into a separate framework, like in this toy
example:
# cat test.bro
@load my-dns
event bro_init()
{
local x = MyCoolDNSFramework::lookup("www.icir.org"); # A
print "IP of www.icir.org is", x; # B
}
# cat my-dns.bro
module MyCoolDNSFramework;
export {
global lookup: function(name: string) : set[addr];
}
function lookup(name: string) : set[addr] {
local addrs = async lookup_hostname(name); # C
return addrs; # D
}
That example behaves exactly as the 1st: execution may suspend between
lines A and B because the call to MyCoolDNSFramework::lookup()
executes an asynchronous function call internally (it will hold
between C and D). But in this 2nd example that behaviour is not
apparent at the call site in line A.
We could require using "async" in line A as well but that would be
extremely painful: whenever calling some function, one would need to
know whether internally the callee may end up using "async" somewhere
(potentially buried further deep inside its call stack).
I think we should instead just skip the "async" keyword altogether.
Requiring it at some places, but not others, hurts more than it helps
in my opinion. The 1st example would then just go back to look like
this:
event bro_init()
{
local x = lookup_hostname("www.icir.org"); # A
print "IP of www.icir.org is", x; # B
}
This would still behave the same as before: potential suspension
between A and B.
I don't think skipping "async" this would be a big deal for anything,
as the cases where the new behaviour may actually lead to significant
differences should be rare.
Thoughts?
Robin
--
Robin Sommer * ICSI/LBNL * robin(a)icir.org * www.icir.org/robin
Hi , I am very glad to write this email . I am a user of bro and recently I start to use BinPAC which is a subcomponent of bro . After learning the syntax of BinPAC , I wrote some simple BinPAC programs and tested them . I got a problem during the test and it really confused me . So I am writing to you and hope to get your help . I will describe my problem below .
My BinPAC version is 0.47 .
In the test I have two machines , A and B . One process on machine A sends messages to another process on machine B once per second . the message is in this format :
la(uint32) + lb (uint32) + s(a random string whose length is not fixed)
In the message , "la" and "lb" are both the length of the string "s" . For example , la = 26 , lb = 26 , s = "abcdefghijklmnopqrstuvwxyz" . Another example , la = 10 , lb = 10 , s = "helloworld" . So I wrote a BinPAC program (see file_1.pac in attachment) and it worked as expected . But when I made a small change to the BinPAC program (see file_2.pac in attachment), a bug existed .
In the first case , I defined a type "header" which contains "la : unint32" and "lb : uint32" and I defined another type "body" which only contains "s : bytestring" . And I defined a high-level type which contains "header" and "body" . Then I print out "la" and the length of "s" . It showed that the program worked properly , the output is like this :
238 238
309 309
311 311
339 339
344 344
252 252
290 290
312 312
298 298
300 300
281 281
...
That is what I want . The first number in each line is "la" and the second number is the length of "s" .
But when I didn't define the "header" type but wrote "la : uint32 ; lb : uint32" in the high-level type directly instead , it failed to work , I mean , nothing was printed out .
In "file_2.pac" , I wrote like this :
type trans_pdu(is_orig: bool) = record {
la: uint32 &byteorder=bigendian;
lb: uint32 &byteorder=bigendian;
body: trans_body(x) &requires(x);
} &let{
x = la;
} &length = x + 8;
I read the file generated by binpac ("file_2.cc") , and I added 2 lines into "file_2.cc" to debug it to see what would happen (the additional codes just print out buffering state , the address of "t_begin_of_data" , the address of "t_end_of_data" and "throw") . The following is part of the code of "file_2.cc" . Only the two "printf" sentences are added by me .
bool trans_pdu::ParseBuffer(flow_buffer_t t_flow_buffer, Contexttrans * t_context)
{
bool t_val_parsing_complete;
t_val_parsing_complete = false;
const_byteptr t_begin_of_data = t_flow_buffer->begin();
const_byteptr t_end_of_data = t_flow_buffer->end();
printf("buffering state: %d t_begin_of_data: %d t_end_of_data: %d \n" , buffering_state_ , (void*)t_begin_of_data , (void*)t_end_of_data);
switch ( buffering_state_ )
{
case 0:
if ( buffering_state_ == 0 )
{
t_flow_buffer->NewFrame(4, false);
buffering_state_ = 1;
}
buffering_state_ = 1;
break;
case 1:
{
buffering_state_ = 2;
// Checking out-of-bound for "trans_pdu:lb"
if ( (t_begin_of_data + 4) + (4) > t_end_of_data || (t_begin_of_data + 4) + (4) < (t_begin_of_data + 4) )
{
printf("throw\n");
// Handle out-of-bound condition
throw binpac::ExceptionOutOfBound("trans_pdu:lb",
(4) + (4),
(t_end_of_data) - (t_begin_of_data));
}
// Parse "la"
la_ = FixByteOrder(bigendian, *((uint32 const *) (t_begin_of_data)));
// Evaluate 'let' and 'withinput' fields
x_ = la();
t_flow_buffer->GrowFrame(x() + 8);
}
break;
case 2:
BINPAC_ASSERT(t_flow_buffer->ready());
if ( t_flow_buffer->ready() )
{
// Parse "lb"
lb_ = FixByteOrder(bigendian, *((uint32 const *) ((t_begin_of_data + 4))));
// Evaluate 'let' and 'withinput' fields
// Parse "body"
body_ = new trans_body(x());
int t_body__size;
t_body__size = body_->Parse((t_begin_of_data + 8), t_end_of_data);
// Evaluate 'let' and 'withinput' fields
t_val_parsing_complete = true;
if ( t_val_parsing_complete )
{
// Evaluate 'let' and 'withinput' fields
proc_ = t_context->flow()->proc_sample_message(this);
}
BINPAC_ASSERT(t_val_parsing_complete);
buffering_state_ = 0;
}
break;
default:
BINPAC_ASSERT(buffering_state_ <= 2);
break;
}
return t_val_parsing_complete;
}
void trans_flow::NewData(const_byteptr t_begin_of_data, const_byteptr t_end_of_data)
{
......
......
while ( ! t_dataunit_parsing_complete && flow_buffer_->ready() )
{
const_byteptr t_begin_of_data = flow_buffer()->begin();
const_byteptr t_end_of_data = flow_buffer()->end();
t_dataunit_parsing_complete = dataunit_->ParseBuffer(flow_buffer(), context_);
if ( t_dataunit_parsing_complete )
{
// Evaluate 'let' and 'withinput' fields
}
}
......
......
}
and the following is the output :
buffering state: 0 t_begin_of_data: 44665856 t_end_of_data: 44665856
buffering state: 1 t_begin_of_data: 44665856 t_end_of_data: 44665860
throw
buffering state: 0 t_begin_of_data: 44669328 t_end_of_data: 44669328
buffering state: 1 t_begin_of_data: 44669328 t_end_of_data: 44669332
throw
buffering state: 0 t_begin_of_data: 44687568 t_end_of_data: 44687568
buffering state: 1 t_begin_of_data: 44687568 t_end_of_data: 44687572
throw
buffering state: 0 t_begin_of_data: 44688176 t_end_of_data: 44688176
buffering state: 1 t_begin_of_data: 44688176 t_end_of_data: 44688180
throw
...
I guess that in " case 0 " , the sentence " t_flow_buffer->NewFrame(4, false) " is used to create a 4-byte frame to parse "la" , since "la" occupies the first 4 bytes of the message and BinPAC need it evaluate the length of the message ? After this operation , "t_begin_of_data " points to the begin of the message and "t_end_of_data" points to the end of "la" . But then " case 1 " will take place . "lb" will be checked whether out-of-bound . "t_begin_of_data + 4" is the begin of "lb" , " 4 " is the length of "lb" , but "t_end_of_data" still points to the end of la . So "(t_begin_of_data + 4) + (4) > t_end_of_data" was met , and the program didn't go ahead . However , that is only the direct reason but not the source reason . I guess the source reason is that I didn't write the BinPAC code in the correct way somewhere or maybe there is a small bug in BinPAC .
I really really hope to use BinPAC to deal with some protocol analysis . I tried to read the source code of BinPAC but failed to understand . I don't know what to do and I really want to get your help !
Wish to get your reply !
Good luck !
Yunchao Chen
2018.2.27
Bro-Dev Group,
ISSUE: I encountered an issue where Bro is not logging some rather
significant SMB1 commands in the smb_cmd.log file. I understand that some
SMB commands are deliberately omitted from the log (such as Negotiate
Protocol, Session Setup, and Tree Connect); however, I observe that an
instance of NT Create and Delete are not being recorded. I also understand
that some SMB messages are deliberately omitted based on the status code;
but the status codes ire STATUS_SUCCESS, so it should be logged. In this
particular traffic sample, there are more than 100+ SMB messages going back
and forth in the TCP stream, but only first several are recorded in
smb_cmd.log, then it stops. Please help.
Bro Version:
I am using the Bro v2.5.1 docker image I pulled from the following URL:
https://hub.docker.com/r/rsmmr/hilti/
PCAP File:
I downloaded the "smbtorture" pcap file from the Wireshark public
repository, at the URL:
https://wiki.wireshark.org/SampleCaptures?action=AttachFile&do=get&target=sm
btorture.cap.gz
The issue I observe corresponds to stream #1 extracted from the file above,
via filter: 'tcp.stream eq 1'. I attached a PCAP file containing stream #1
only.
PCAP Analysis of SMB Messages:
>From the PCAP file, using Wireshark, the following sequence of SMB Messages
are observed (summarized below as Request & Response pairs):
(01) Negotiate Protocol Req & Resp
(02) Session Setup AndX Req & Resp [x2]
(03) Tree Connect AndX Req & Resp
(04) Delete Req & Resp [file \torture_qfileinfo.txt]
(05) NT Create AndX Req & Resp [fid 4000, file
\torture_qfileinfo.txt]
(06) Write AndX Req & Resp
(07) Trans2 Req & Resp
(08) Set Information2 Req & Resp
(09) Query Information2 Req & Resp
(10) Query Information Req & Resp
(11) Query Information2 Req & Resp
(12) Trans2 Req & Resp [x57]
(13) Close Req & Resp [fid 4000]
(14) NT Create AndX Req & Resp [fid 4001, file TORTUR~1.TXT]
(15) Close Req & Resp [fid 4001]
(16) Delete Req & Resp [file \torture_qfileinfo.txt ->
formerly fid 4000]
(17) Tree Disconnect
Bro Analysis of smb_cmd.log:
The Bro smb_cmd.log records events (04) - (10). I understand that events
(01) - (03) are deliberately omitted from the log, but I am concerned that
nothing is logged after event (10), Query Information Req & Resp.
I think this is an important issue because the smb_cmd.log fails to record
two significant events in this TCP stream:
(i) A second file is created in step (14)
(ii) The first file (create in step [05]) is deleted in step
(16)
The SMB messages look well-formed in Wireshark. Nothing seems to be wrong.
The SMB status code is STATUS_SUCCESS for the requests and the responses, so
it should be logged.
Artifacts:
Attached are the following artifacts to help you reproduce the issue:
(a) ws_smbtorture_stream001.pcap (pcap of stream #1 only)
(b) test.bro script
(c) smb_cmd.log
(d) smb_files.log
(e) files.log
(f) conn.log
(g) packet_filter.log
Not sure what is going wrong. Please help.
Cheers,
Mark
I was wondering the other day if we could add CAF as submodule to
Broker, and then just start compiling it along with everything else. A
long time ago we decided generally against shipping dependencies along
with Bro, but in this case it might make people's lives quite a bit
easier, as hardly any Bro user will have CAF installed already. And
even if they had (say from an earlier Bro version), it might not be
the right version. If we included it with Broker, things would just
work.
We could even go a step further and compile CAF statically into
libbroker, so that in the end from a user's perspective all they deal
with is Broker: if they link against it, they get everything they
need.
Would that make sense?
Robin
--
Robin Sommer * ICSI/LBNL * robin(a)icir.org * www.icir.org/robin
One more Broker idea: I'm thinking we should add a queuing mechanism
to Broker that buffers outgoing messages for a while when a peer goes
down. Once it comes back up, we'd pass them on. That way an endpoint
could restart for example without us loosing data.
I'm not immediately sure how/where we'd integrate that. For outgoing
messages, we could add it to the transparent reconnect. However, for
incoming connections, where the local endpoint doesn't have a notion
of "that peer should be coming back", it might not be as straight
forward?
Robin
--
Robin Sommer * ICSI/LBNL * robin(a)icir.org * www.icir.org/robin
Hi everyone,
[This mail has been sent to bro@ first, but I think I might have more
luck (and answers) here. Sorry for the inconvenience to those who have
already read it.]
For a network recon framework I am working on (https://ivre.rocks/ --
for those interested), I would like to log each "TCP server banner"
Bro sees.
I call "TCP server banner" the first chunk of data a server sends,
before the client has sent data (if the client sends data before the
server, I don't want to log anything).
Here is what I have done so far (`PassiveRecon` is my module's name):
```
export {
redef tcp_content_deliver_all_resp = T;
[...]
}
[...]
event tcp_contents(c: connection, is_orig: bool, seq: count, contents: string)
{
if (! is_orig && seq == 1 && c$orig$num_pkts == 2)
{
Log::write(PassiveRecon::LOG, [$ts=c$start_time,
$host=c$id$resp_h,
$srvport=c$id$resp_p,
$recon_type=TCP_SERVER_BANNER,
$value=contents]);
}
}
```
Basically, I consider we have a "TCP server banner" when `is_orig` is
false, when `seq` equals 1 and when we have seen exactly two packets
from the client (which should be a SYN and the first ACK).
This solution generally works **but** I sometimes log a data chunk
when I should not, particularly if I have missed part of the
traffic.
As an example, the following Scapy script creates a PCAP file that
would trick my script into logging a "TCP server banner" while the
client has actually sent some data (and we have missed an ACK packet,
left as a comment in the script):
```
wrpcap("test.cap", [
Ether() / IP(dst="1.2.3.4", src="5.6.7.8") /
TCP(dport=80, sport=5678, flags="S", ack=0, seq=555678),
Ether() / IP(src="1.2.3.4", dst="5.6.7.8") /
TCP(sport=80, dport=5678, flags="SA", seq=111234, ack=555679),
# Ether() / IP(dst="1.2.3.4", src="5.6.7.8") /
# TCP(dport=80, sport=5678, flags="A", ack=111235, seq=555679),
Ether() / IP(dst="1.2.3.4", src="5.6.7.8") /
TCP(dport=80, sport=5678, flags="PA", ack=111235, seq=555679) / "DATA",
Ether() / IP(src="1.2.3.4", dst="5.6.7.8") /
TCP(sport=80, dport=5678, flags="PA", seq=111235, ack=555683) / "DATA"
])
```
Is there a way to know that I have not missed any packet from the
client and/or a way to know that the client has not sent any data on
the connection (like an equivalent of the `seq` parameter, but for the
`ack`)?
Also, when `seq` equals 1, am I certain that I have not missed any
packet from the server?
One more question: is there a better, cleaner, etc. way to do what I'm
trying to do?
Thanks a lot,
Pierre
--
Pierre
http://pierre.droids-corp.org/
Bro-Dev Group,
I am digging thru the BinPAC code for the DCE-RPC analyzer, and I noticed a couple of developer-comments that I think could be related, and perhaps even resolved, by a simple fix.
1. Developer BinPAC Comments
See Lines 153-155 of dce_rpc-protocol.pac [https://github.com/bro/bro/blob/master/src/analyzer/protocol/dce-rpc/dce_rp…], stating that DCE_RPC_ALTER_CONTEXT and DCE_RPC_ALTER_CONTEXT_RESP are not being handled correctly and consequently, the parsers for each one are disabled/commented out.
2. Issue / Problem: dce_rpc-protocol.pac
According to the original Open Group specification for DCE RPC (dated October 1997), the format of the AlterContext packet is identical to the Bind packet, and the format of the AlterContextResponse is identical to the BindAck. See the following URL for more info; or I could send you the PDF document separately, if needed.
http://pubs.opengroup.org/onlinepubs/9629399/chap12.htm#tagcjh_17_06_04_01http://pubs.opengroup.org/onlinepubs/9629399/chap12.htm#tagcjh_17_06_04_02
When looking at the BinPAC file, the type records for DCE_RPC_ALTER_CONTEXT and DCE_RPC_BIND are different, should be identical.
Similarly, the type records for DCE_RPC_ALTER_CONTEXT_RESP and DCE_RPC_BIND_ACK are very different, should be identical.
3. Proposed Fix: dce_rpc-protocol.pac
Modify the type record for DCE_RPC_ALTER_CONTEXT to be identical to DCE_RPC_BIND.
Modify the type record for DCE_RPC_ALTER_CONTEXT_RESP to be identical to DCE_RPC_BIND_ACK.
Remove '#' on Lines 154 and 155 to un-comment these lines and re-enable the parsers.
In dce_rpc-analyzer.pac, generate events resulting from the AlterContext packet to allow logging of the new binding information in script-land.
4. Developer Script-land Comments
See Lines 137 and 187 of main.bro [https://github.com/bro/bro/blob/master/scripts/base/protocols/dce-rpc/main.…], stating a condition where sometimes the binding is not seen. I can think of a couple of scenarios under which this would occur: (a) packet loss/drop; and (b) AlterContext packet not parsed. I think the fix described above will address (b) and help reduce the number instances where the binding isn't seen.
5. Bro Issue Tracker
I plan to submit this to Bro Issue Tracker. Just wanted to give you a heads up here.
Cheers!
Mark
Hi,
Here are 2 questions, one about usage of memory after free, another seems
to me like memory leak. Could you please either confirm that these are bugs
and suggest fixes or help me work out why no?
Function src/loggin/Manager.cc: bool Manager::Write(EnumVal* id, RecordVal*
columns) seems to be using memory that might have been freed already.
CreateWriter function can delete info_and_fields and still return non null
writer. Any suggestions how to fix it? Maybe do not delete info in
CreateWriter if old writer still exists? Will info's memory be freed
somewhere later?
I do not have a test case for this issue though.
// CreateWriter() will set the other fields in info.
writer = CreateWriter(stream->id, filter->writer,
info, filter->num_fields, arg_fields,
filter->local,
filter->remote, false, filter->name);
if ( ! writer )
{
Unref(columns);
return false;
}
}
// Alright, can do the write now.
threading::Value** vals = RecordToFilterVals(stream, filter,
columns);
if ( ! PLUGIN_HOOK_WITH_RESULT(HOOK_LOG_WRITE,
HookLogWrite(filter->writer->Type()->AsEnumType()->Lookup(fi
lter->writer->InternalInt()),
filter->name, *info,
filter->num_fields,
filter->fields, vals), true) )
Another question is - inside src/input/Manager.cc -> a lot of places where
ev is allocated from EnumVal, it is not being freed or Unrefed. Eg in
function Manager::Delete(...reader, vals) , it seems like predidx and ev
are allocated, but are not freed if there was not convert_error. This looks
like memory is leaked in all those cases.
if ( stream->pred )
{
int startpos = 0;
Val* predidx = ValueToRecordVal(i, vals,
stream->itype, &startpos, convert_error);
if ( convert_error )
Unref(predidx);
else
{
Ref(val);
EnumVal *ev = new
EnumVal(BifEnum::Input::EVENT_REMOVED, BifType::Enum::Input::Event);
streamresult =
CallPred(stream->pred, 3, ev, predidx, val);
if ( streamresult == false )
{
// keep it.
Unref(idxval);
success = true;
}
}
}
Thank you for suggestions,
Martina
Hi,
I am going to delete these (merged) branches thursday, unless someone
feels especially attached to them:
topic/dnthayer/ticket1821
topic/dnthayer/ticket1836
topic/dnthayer/ticket1863
topic/jazoff/contentline-limit
topic/jazoff/fix-gridftp
topic/jazoff/fix-intel-error
topic/jazoff/speedup-for
topic/robin/broker-logging
topic/robin/event-args
topic/robin/plugin-version-check
topic/seth/add-file-lookup-functions
topic/seth/input-thread-behavior
topic/seth/remove-dns-weird
topic/vladg/bit-1838
Johanna