Improve DNS analysis.

- Fix parsing of empty question sections (when QDCOUNT == 0).  In this
  case, the DNS parser would extract two 2-byte fields for use in either
  "dns_query_reply" or "dns_rejected" events (dependent on value of
  RCODE) as qclass and qtype parameters.  This is not correct, because
  such fields don't actually exist in the DNS message format when
  QDCOUNT is 0.  As a result, these events are no longer raised when
  there's an empty question section.  Scripts that depends on checking
  for an empty question section can do that in the "dns_message" event.

- Add a new "dns_unknown_reply" event, for when Bro does not know how
  to fully parse a particular resource record type.  This helps fix a
  problem in the default DNS scripts where the logic to complete
  request-reply pair matching doesn't work because it's waiting on more
  RR events to complete the reply.  i.e. it expects ANCOUNT number of
  dns_*_reply events and will wait until it gets that many before
  completing a request-reply pair and logging it to dns.log.  This could
  cause bogus replies to match a previous request if they happen to
  share a DNS transaction ID.
This commit is contained in:
Jon Siwek 2014-01-28 11:04:01 -06:00
parent 392d1cb759
commit 0e0e74e49c
7 changed files with 61 additions and 30 deletions

View file

@ -158,12 +158,17 @@ hook set_session(c: connection, msg: dns_msg, is_query: bool) &priority=5
# If this is either a query or this is the reply but
# no Info records are in the queue (we missed the query?)
# we need to create an Info record and put it in the queue.
if ( is_query ||
Queue::len(c$dns_state$pending[msg$id]) == 0 )
if ( is_query )
{
info = new_session(c, msg$id);
Queue::put(c$dns_state$pending[msg$id], info);
}
else if ( Queue::len(c$dns_state$pending[msg$id]) == 0 )
{
info = new_session(c, msg$id);
Queue::put(c$dns_state$pending[msg$id], info);
event conn_weird("dns_unmatched_reply", c, "");
}
if ( is_query )
# If this is a query, assign the newly created info variable
@ -202,17 +207,23 @@ hook set_session(c: connection, msg: dns_msg, is_query: bool) &priority=5
event dns_message(c: connection, is_orig: bool, msg: dns_msg, len: count) &priority=5
{
hook set_session(c, msg, is_orig);
if ( msg$QR && msg$rcode != 0 && msg$num_queries == 0 )
c$dns$rejected = T;
}
event DNS::do_reply(c: connection, msg: dns_msg, ans: dns_answer, reply: string) &priority=5
{
if ( ! msg$QR )
# This is weird: the inquirer must also be providing answers in
# the request, which is not what we want to track.
return;
if ( ans$answer_type == DNS_ANS )
{
if ( ! c?$dns )
{
event conn_weird("dns_unmatched_reply", c, "");
hook set_session(c, msg, F);
}
c$dns$AA = msg$AA;
c$dns$RA = msg$RA;
@ -265,6 +276,12 @@ event dns_request(c: connection, msg: dns_msg, query: string, qtype: count, qcla
c$dns$query = query;
}
event dns_unknown_reply(c: connection, msg: dns_msg, ans: dns_answer) &priority=5
{
event DNS::do_reply(c, msg, ans, fmt("<unknown type=%s>", ans$qtype));
}
event dns_A_reply(c: connection, msg: dns_msg, ans: dns_answer, a: addr) &priority=5
{
event DNS::do_reply(c, msg, ans, fmt("%s", a));