Merge remote-tracking branch 'origin/topic/seth/dns-srv-fix'

* origin/topic/seth/dns-srv-fix:
  No longer accidentally attempting to parse NBSTAT RRs as SRV RRs.
  Fix DNS SRV responses and a small issue with NBNS queries and label length.

BIT-1147 #merged

CHANGES

@@ -1,4 +1,21 @@
 
+2.2-213 | 2014-03-09 08:57:37 -0700
+
+  * No longer accidentally attempting to parse NBSTAT RRs as SRV RRs
+    in DNS analyzer. (Seth Hall)
+
+  * Fix DNS SRV responses and a small issue with NBNS queries and
+    label length. (Seth Hall)
+
+     - 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.
+
 2.2-210 | 2014-03-06 22:52:36 -0500
 
   * Improve SSL logging so that connections are logged even when the

NEWS

@@ -25,6 +25,9 @@ New Functionality
   parsing past the GRE header in between the delivery and payload IP
   packets.
 
+- The DNS analyzer now actually generates the dns_SRV_reply() event.
+  It had been documented before, yet was never raised.
+
 Changed Functionality
 ---------------------
 

VERSION

@@ -1 +1 @@
-2.2-210
+2.2-213

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;
 	}
 
@@ -421,9 +429,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

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;
 
@@ -275,7 +276,17 @@ 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:
@@ -400,7 +411,9 @@ 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 +646,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;
 	}
 

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