From 9743959995b476ff4443b8ddf12e42060ed0a8bb Mon Sep 17 00:00:00 2001 From: Seth Hall Date: Wed, 5 Mar 2014 16:11:06 -0500 Subject: [PATCH 1/2] Fix DNS SRV responses and a small issue with NBNS queries and label length. - DNS SRV responses never had the code written to actually generate the dns_SRV_reply event. Adding this required extending the event a bit to add extra information. SRV responses now appear in the dns.log file correctly. - Fixed an issue where some Microsoft NetBIOS Name Service lookups would exceed the max label length for DNS and cause an incorrect "DNS_label_too_long" weird. --- scripts/base/protocols/dns/main.bro | 4 ++-- src/analyzer/protocol/dns/DNS.cc | 25 +++++++++++++++++++------ src/analyzer/protocol/dns/events.bif | 8 +++++++- 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/scripts/base/protocols/dns/main.bro b/scripts/base/protocols/dns/main.bro index 294220d1f2..8f83fbc677 100644 --- a/scripts/base/protocols/dns/main.bro +++ b/scripts/base/protocols/dns/main.bro @@ -421,9 +421,9 @@ event dns_WKS_reply(c: connection, msg: dns_msg, ans: dns_answer) &priority=5 hook DNS::do_reply(c, msg, ans, ""); } -event dns_SRV_reply(c: connection, msg: dns_msg, ans: dns_answer) &priority=5 +event dns_SRV_reply(c: connection, msg: dns_msg, ans: dns_answer, target: string, priority: count, weight: count, p: count) &priority=5 { - hook DNS::do_reply(c, msg, ans, ""); + hook DNS::do_reply(c, msg, ans, target); } # TODO: figure out how to handle these diff --git a/src/analyzer/protocol/dns/DNS.cc b/src/analyzer/protocol/dns/DNS.cc index b17a90dd61..5d45f9b05c 100644 --- a/src/analyzer/protocol/dns/DNS.cc +++ b/src/analyzer/protocol/dns/DNS.cc @@ -208,6 +208,7 @@ int DNS_Interpreter::ParseAnswer(DNS_MsgInfo* msg, int name_len = sizeof(name) - 1; u_char* name_end = ExtractName(data, len, name, name_len, msg_start); + if ( ! name_end ) return 0; @@ -400,7 +401,10 @@ int DNS_Interpreter::ExtractLabel(const u_char*& data, int& len, return 0; } - if ( label_len > 63 ) + if ( label_len > 63 && + // NetBIOS name service look ups can use + // longer labels + ntohs(analyzer->Conn()->RespPort()) != 137 ) { analyzer->Weird("DNS_label_too_long"); return 0; @@ -633,15 +637,24 @@ int DNS_Interpreter::ParseRR_SRV(DNS_MsgInfo* msg, u_char* name_end = ExtractName(data, len, name, name_len, msg_start); if ( ! name_end ) return 0; - *name_end = 0; // terminate name so we can use it in snprintf() if ( data - data_start != rdlength ) analyzer->Weird("DNS_RR_length_mismatch"); - // The following is just a placeholder. - char buf[2048]; - safe_snprintf(buf, sizeof(buf), "SRV %s priority=%d weight=%d port=%d", - name, priority, weight, port); + if ( dns_SRV_reply && ! msg->skip_event ) + { + val_list* vl = new val_list; + vl->append(analyzer->BuildConnVal()); + vl->append(msg->BuildHdrVal()); + vl->append(msg->BuildAnswerVal()); + vl->append(new StringVal(new BroString(name, name_end - name, 1))); + vl->append(new Val(priority, TYPE_COUNT)); + vl->append(new Val(weight, TYPE_COUNT)); + vl->append(new Val(port, TYPE_COUNT)); + + analyzer->ConnectionEvent(dns_SRV_reply, vl); + } + return 1; } diff --git a/src/analyzer/protocol/dns/events.bif b/src/analyzer/protocol/dns/events.bif index b43ac95f66..520e32fe6d 100644 --- a/src/analyzer/protocol/dns/events.bif +++ b/src/analyzer/protocol/dns/events.bif @@ -392,6 +392,12 @@ event dns_TXT_reply%(c: connection, msg: dns_msg, ans: dns_answer, str: string%) ## ## ans: The type-independent part of the parsed answer record. ## +## priority: Priority of the SRV response. +## +## weight: Weight of the SRV response. +## +## p: Port of the SRV response. +## ## .. bro:see:: dns_AAAA_reply dns_A_reply dns_CNAME_reply dns_EDNS_addl ## dns_HINFO_reply dns_MX_reply dns_NS_reply dns_PTR_reply dns_SOA_reply ## dns_TSIG_addl dns_TXT_reply dns_WKS_reply dns_end dns_full_request @@ -399,7 +405,7 @@ event dns_TXT_reply%(c: connection, msg: dns_msg, ans: dns_answer, str: string%) ## dns_mapping_unverified dns_mapping_valid dns_message dns_query_reply ## dns_rejected dns_request non_dns_request dns_max_queries dns_session_timeout ## dns_skip_addl dns_skip_all_addl dns_skip_all_auth dns_skip_auth -event dns_SRV_reply%(c: connection, msg: dns_msg, ans: dns_answer%); +event dns_SRV_reply%(c: connection, msg: dns_msg, ans: dns_answer, target: string, priority: count, weight: count, p: count%); ## Generated on DNS reply resource records when the type of record is not one ## that Bro knows how to parse and generate another more specific specific From bcdffe3212e717c6102f79b2306fb5e805598962 Mon Sep 17 00:00:00 2001 From: Seth Hall Date: Thu, 6 Mar 2014 09:06:23 -0500 Subject: [PATCH 2/2] No longer accidentally attempting to parse NBSTAT RRs as SRV RRs. The NetBios name service RFC (1002) specified NBSTAT (NetBios Status) resource records to have identifier 0x0021. The DNS SRV RFC specified SRV records to have identifier 33. Unfortunately those are the same number. :) We now check the resp port to handle this situation better so that we won't be attempting to parse NBSTAT records as SRV (which causes several weird messages). --- scripts/base/protocols/dns/main.bro | 8 ++++++++ src/analyzer/protocol/dns/DNS.cc | 13 ++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/scripts/base/protocols/dns/main.bro b/scripts/base/protocols/dns/main.bro index 8f83fbc677..348c361311 100644 --- a/scripts/base/protocols/dns/main.bro +++ b/scripts/base/protocols/dns/main.bro @@ -360,7 +360,15 @@ event dns_request(c: connection, msg: dns_msg, query: string, qtype: count, qcla # Note: I'm ignoring the name type for now. Not sure if this should be # worked into the query/response in some fashion. if ( c$id$resp_p == 137/udp ) + { query = decode_netbios_name(query); + if ( c$dns$qtype_name == "SRV" ) + { + # The SRV RFC used the ID used for NetBios Status RRs. + # So if this is NetBios Name Service we name it correctly. + c$dns$qtype_name = "NBSTAT"; + } + } c$dns$query = query; } diff --git a/src/analyzer/protocol/dns/DNS.cc b/src/analyzer/protocol/dns/DNS.cc index 5d45f9b05c..c5cf47b858 100644 --- a/src/analyzer/protocol/dns/DNS.cc +++ b/src/analyzer/protocol/dns/DNS.cc @@ -276,7 +276,18 @@ int DNS_Interpreter::ParseAnswer(DNS_MsgInfo* msg, break; case TYPE_SRV: - status = ParseRR_SRV(msg, data, len, rdlength, msg_start); + if ( ntohs(analyzer->Conn()->RespPort()) == 137 ) + { + // This is an NBSTAT (NetBIOS NODE STATUS) record. + // The SRV RFC reused the value that was already being + // used for this. + // We aren't parsing this yet. + status = 1; + } + else + { + status = ParseRR_SRV(msg, data, len, rdlength, msg_start); + } break; case TYPE_EDNS: